diff --git a/tests/integration/WebsiteHosting.go b/tests/integration/WebsiteHosting.go index 3fde8464..2e0302df 100644 --- a/tests/integration/WebsiteHosting.go +++ b/tests/integration/WebsiteHosting.go @@ -862,3 +862,55 @@ func WebsiteHosting_options_preflight_missing_origin(s *S3Conf) error { return checkWebsiteErrorResponse(resp, s3err.GetAPIError(s3err.ErrMissingCORSOrigin)) }) } + +// WebsiteHosting_url_encoded_object_key tests that object keys containing +// characters requiring percent-encoding are served, and that a key holding a +// literal percent sign is not decoded twice. +func WebsiteHosting_url_encoded_object_key(s *S3Conf) error { + testName := "WebsiteHosting_url_encoded_object_key" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + err := putBucketWebsiteConfig(s3client, bucket, &types.WebsiteConfiguration{ + IndexDocument: &types.IndexDocument{ + Suffix: getPtr("index.html"), + }, + }) + if err != nil { + return err + } + if err := grantPublicBucketPolicy(s3client, bucket, policyTypeObject); err != nil { + return err + } + + for _, test := range []struct { + key string + path string + }{ + {"my file.html", "/my%20file.html"}, + {"my dir/index.html", "/my%20dir/"}, + {"café.html", "/caf%C3%A9.html"}, + {"a%20b.html", "/a%2520b.html"}, + } { + content := "" + test.key + "" + _, err = putObjectWithData(int64(len(content)), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &test.key, + Body: strings.NewReader(content), + ContentType: getPtr("text/html"), + }, s3client) + if err != nil { + return err + } + + resp, err := websiteGet(s, bucket, test.path, nil) + if err != nil { + return err + } + + if err := checkWebsiteResponse(resp, http.StatusOK, []byte(content)); err != nil { + return fmt.Errorf("%s: %w", test.path, err) + } + } + + return nil + }) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 383123da..79baff1d 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -717,6 +717,7 @@ func TestWebsiteHosting(ts *TestState) { ts.Run(WebsiteHosting_options_preflight_access_granted) ts.Run(WebsiteHosting_options_preflight_access_forbidden) ts.Run(WebsiteHosting_options_preflight_missing_origin) + ts.Run(WebsiteHosting_url_encoded_object_key) } func TestPreflightOPTIONSEndpoint(ts *TestState) { @@ -1892,6 +1893,7 @@ func GetIntTests() IntTests { "WebsiteHosting_options_preflight_access_granted": WebsiteHosting_options_preflight_access_granted, "WebsiteHosting_options_preflight_access_forbidden": WebsiteHosting_options_preflight_access_forbidden, "WebsiteHosting_options_preflight_missing_origin": WebsiteHosting_options_preflight_missing_origin, + "WebsiteHosting_url_encoded_object_key": WebsiteHosting_url_encoded_object_key, "PreflightOPTIONS_non_existing_bucket": PreflightOPTIONS_non_existing_bucket, "PreflightOPTIONS_missing_origin": PreflightOPTIONS_missing_origin, "PreflightOPTIONS_invalid_request_method": PreflightOPTIONS_invalid_request_method, diff --git a/website/handler.go b/website/handler.go index c84d7623..c436dfb5 100644 --- a/website/handler.go +++ b/website/handler.go @@ -19,6 +19,7 @@ import ( "fmt" "io" "net/http" + "net/url" "strconv" "strings" @@ -125,12 +126,28 @@ func (c *websiteController) Options(ctx fiber.Ctx) error { func registerWebsiteRoutes(app *fiber.App, be backend.Backend, domain string) { controller := newWebsiteController(be, domain) + // percent-decode the request path, so object keys containing special + // characters resolve to the actual key. The s3 api and admin servers + // mount the same middleware. + app.Use("*", decodeURL) + app.Head("*", controller.Head) app.Get("*", controller.Get) app.Options("*", controller.Options) app.All("*", controller.MethodNotAllowed) } +// decodeURL wraps the shared DecodeURL middleware to continue the chain and to +// report malformed percent-encoding as an html error page. +func decodeURL(ctx fiber.Ctx) error { + if err := middlewares.DecodeURL(ctx); err != nil { + debuglogger.Logf("failed to unescape the request path: %v", err) + return sendError(ctx, s3err.GetAPIError(s3err.ErrInvalidURI)) + } + + return ctx.Next() +} + func setCORSPreflightHeaders(ctx fiber.Ctx, allowConfig *auth.CORSAllowanceConfig) { ctx.Set("Access-Control-Allow-Origin", allowConfig.Origin) ctx.Set("Access-Control-Allow-Methods", allowConfig.Methods) @@ -195,8 +212,6 @@ func (c *websiteController) resolveRequest(ctx fiber.Ctx) (*websiteRequestInfo, return nil, err } - fmt.Println(bucket) - key := strings.TrimPrefix(ctx.Path(), "/") if err := validateWebsiteNames(bucket, key); err != nil { return nil, err @@ -411,10 +426,8 @@ func handleRedirectAll(ctx fiber.Ctx, redirect *s3response.RedirectAllRequestsTo protocol = ctx.Scheme() } - location := fmt.Sprintf("%s://%s/%s", protocol, redirect.HostName, key) - if query := string(ctx.Request().URI().QueryString()); query != "" { - location += "?" + query - } + location := websiteLocation(protocol, redirect.HostName, key, + string(ctx.Request().URI().QueryString())) return sendRedirect(ctx, http.StatusMovedPermanently, location) } @@ -444,13 +457,24 @@ func applyRedirect(ctx fiber.Ctx, redirect *s3response.Redirect, condition *s3re } } - location := fmt.Sprintf("%s://%s/%s", protocol, host, key) - if query := string(ctx.Request().URI().QueryString()); query != "" { - location += "?" + query - } + location := websiteLocation(protocol, host, key, + string(ctx.Request().URI().QueryString())) return sendRedirect(ctx, httpCode, location) } +// websiteLocation builds a redirect target url. The object key is +// percent-encoded, as the keys reaching this point are decoded. +func websiteLocation(protocol, host, key, query string) string { + location := url.URL{ + Scheme: protocol, + Host: host, + Path: "/" + key, + RawQuery: query, + } + + return location.String() +} + func sendRedirect(ctx fiber.Ctx, statusCode int, location string) error { ctx.Set("Location", location) _, _ = utils.EnsureRequestIDs(ctx) diff --git a/website/handler_test.go b/website/handler_test.go index 9d983f6d..626ff858 100644 --- a/website/handler_test.go +++ b/website/handler_test.go @@ -27,6 +27,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/gofiber/fiber/v3" + "github.com/valyala/fasthttp" "github.com/versity/versitygw/auth" "github.com/versity/versitygw/backend" "github.com/versity/versitygw/s3err" @@ -44,6 +45,7 @@ type websiteTestBackend struct { objectErrors map[string]error public bool calls []string + objectKeys []string } func (b *websiteTestBackend) record(call string) { @@ -96,6 +98,7 @@ func (b *websiteTestBackend) HeadObject(_ context.Context, input *s3.HeadObjectI if input == nil || input.Key == nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } + b.objectKeys = append(b.objectKeys, *input.Key) if err, ok := b.objectErrors[*input.Key]; ok { return nil, err } @@ -119,6 +122,7 @@ func (b *websiteTestBackend) GetObject(_ context.Context, input *s3.GetObjectInp if input == nil || input.Key == nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } + b.objectKeys = append(b.objectKeys, *input.Key) if err, ok := b.objectErrors[*input.Key]; ok { return nil, err } @@ -970,6 +974,165 @@ func TestWebsiteHandlerMethodNotAllowed(t *testing.T) { } } +func TestWebsiteHandlerDecodesURLEncodedPath(t *testing.T) { + tests := []struct { + name string + method string + path string + wantKey string + }{ + { + name: "space in the object key", + method: http.MethodGet, + path: "/my%20file.html", + wantKey: "my file.html", + }, + { + name: "space in the directory prefix resolves the index document", + method: http.MethodGet, + path: "/my%20dir/", + wantKey: "my dir/index.html", + }, + { + name: "utf8 multibyte characters", + method: http.MethodHead, + path: "/caf%C3%A9.html", + wantKey: "café.html", + }, + { + name: "literal plus sign is not turned into a space", + method: http.MethodGet, + path: "/c++notes.html", + wantKey: "c++notes.html", + }, + { + name: "percent in the object name is decoded only once", + method: http.MethodGet, + path: "/a%2520b.txt", + wantKey: "a%20b.txt", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body := "content of " + tt.wantKey + be := newWebsiteTestBackend(t, s3response.WebsiteConfiguration{ + IndexDocument: &s3response.IndexDocument{Suffix: "index.html"}, + }, map[string]string{tt.wantKey: body}, true) + + resp := websiteRequestWithMethod(t, be, tt.method, tt.path) + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want %d (requested keys: %v)", + resp.StatusCode, http.StatusOK, be.objectKeys) + } + if len(be.objectKeys) != 1 || be.objectKeys[0] != tt.wantKey { + t.Fatalf("requested keys = %v, want [%q]", be.objectKeys, tt.wantKey) + } + if tt.method != http.MethodGet { + return + } + if got := readBody(t, resp); got != body { + t.Fatalf("body = %q, want %q", got, body) + } + }) + } +} + +// TestWebsiteHandlerEncodedPathTraversalIsRejected covers the validation +// hardening the path decoding brings: before decoding, the literal "%2E%2E%2F" +// contained no "..", so it passed the object name validation. +func TestWebsiteHandlerEncodedPathTraversalIsRejected(t *testing.T) { + be := newWebsiteTestBackend(t, s3response.WebsiteConfiguration{ + IndexDocument: &s3response.IndexDocument{Suffix: "index.html"}, + }, nil, true) + + resp := websiteRequest(t, be, "/%2E%2E%2Fprivate.html") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusBadRequest) + } + if len(be.objectKeys) != 0 { + t.Fatalf("traversal should not reach the backend, got keys: %v", be.objectKeys) + } +} + +// TestWebsiteHandlerInvalidPercentEncoding exercises the decodeURL middleware +// directly: net/http refuses to build a request with a malformed escape, so +// such a path can only come from a raw client. +func TestWebsiteHandlerInvalidPercentEncoding(t *testing.T) { + fctx := &fasthttp.RequestCtx{} + fctx.Request.SetRequestURI("/%zz") + ctx := fiber.New().AcquireCtx(fctx) + + if err := decodeURL(ctx); err != nil { + t.Fatalf("decodeURL returned an error: %v", err) + } + if got := ctx.Response().StatusCode(); got != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", got, http.StatusBadRequest) + } + if got := string(ctx.Response().Header.Peek("x-amz-error-code")); got != "InvalidURI" { + t.Fatalf("x-amz-error-code = %q, want %q", got, "InvalidURI") + } +} + +func TestWebsiteHandlerRedirectLocationIsEncoded(t *testing.T) { + tests := []struct { + name string + config s3response.WebsiteConfiguration + path string + wantLocation string + }{ + { + name: "RedirectAllRequestsTo keeps the key encoded", + config: s3response.WebsiteConfiguration{ + RedirectAllRequestsTo: &s3response.RedirectAllRequestsTo{ + HostName: "target.test", + Protocol: "https", + }, + }, + path: "/my%20file.html", + wantLocation: "https://target.test/my%20file.html", + }, + { + name: "routing rule prefix replacement keeps the suffix encoded", + config: s3response.WebsiteConfiguration{ + IndexDocument: &s3response.IndexDocument{Suffix: "index.html"}, + RoutingRules: []s3response.RoutingRule{ + { + Condition: &s3response.RoutingRuleCondition{ + KeyPrefixEquals: "old dir/", + }, + Redirect: &s3response.Redirect{ + ReplaceKeyPrefixWith: "new dir/", + }, + }, + }, + }, + path: "/old%20dir/my%20file.html", + wantLocation: "http://site.test/new%20dir/my%20file.html", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + be := newWebsiteTestBackend(t, tt.config, nil, true) + + resp := websiteRequest(t, be, tt.path) + defer resp.Body.Close() + + if resp.StatusCode != http.StatusMovedPermanently { + t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusMovedPermanently) + } + if got := resp.Header.Get("Location"); got != tt.wantLocation { + t.Fatalf("Location = %q, want %q", got, tt.wantLocation) + } + }) + } +} + func newWebsiteTestBackend(t *testing.T, config s3response.WebsiteConfiguration, objects map[string]string, public bool) *websiteTestBackend { t.Helper()