From 2da24cc2301fbb8b37bb12e83459859bf937e003 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 17 Apr 2026 14:54:58 -0700 Subject: [PATCH] fix(s3): return 403 on POST policy violation instead of 307 redirect (#9122) * fix(s3): return 403 on POST policy violation instead of 307 redirect CheckPostPolicy failures previously responded with HTTP 307 Temporary Redirect to the request URL, which causes clients to re-POST and obscures the failure. Return 403 AccessDenied so the client surfaces the error. * test(s3): exercise PostPolicyBucketHandler end-to-end for 403 mapping Replace the shallow ErrAccessDenied tautology test with one that builds a signed POST multipart request whose policy conditions cannot be satisfied, calls PostPolicyBucketHandler directly, and asserts HTTP 403 with no Location redirect header. Addresses gemini-code-assist review on PR #9122. * fix(s3): surface POST policy failure reason in AccessDenied response Add s3err.WriteErrorResponseWithMessage so a caller can keep the standard error code mapping while providing a specific Message. Use it from PostPolicyBucketHandler so the XML body carries the CheckPostPolicy error (e.g. which condition failed or that the policy expired) rather than the generic "Access Denied." description. Addresses gemini-code- assist review on PR #9122. * refactor(s3err): delegate WriteErrorResponse to WriteErrorResponseWithMessage The two helpers shared every line except the Message override. Fold WriteErrorResponse into a one-line delegation that passes an empty message, so the request-id/mux/apiError logic lives in exactly one place. Addresses gemini-code-assist review on PR #9122. --- .../s3api/s3api_object_handlers_postpolicy.go | 4 +- .../s3api_object_handlers_postpolicy_test.go | 114 ++++++++++++++++++ weed/s3api/s3err/error_handler.go | 12 ++ 3 files changed, 128 insertions(+), 2 deletions(-) diff --git a/weed/s3api/s3api_object_handlers_postpolicy.go b/weed/s3api/s3api_object_handlers_postpolicy.go index d16810f4e..c17953d5c 100644 --- a/weed/s3api/s3api_object_handlers_postpolicy.go +++ b/weed/s3api/s3api_object_handlers_postpolicy.go @@ -97,8 +97,8 @@ func (s3a *S3ApiServer) PostPolicyBucketHandler(w http.ResponseWriter, r *http.R // Make sure formValues adhere to policy restrictions. if err = policy.CheckPostPolicy(formValues, postPolicyForm); err != nil { - w.Header().Set("Location", r.URL.Path) - w.WriteHeader(http.StatusTemporaryRedirect) + glog.V(3).Infof("PostPolicy check failed for bucket %s: %v", bucket, err) + s3err.WriteErrorResponseWithMessage(w, r, s3err.ErrAccessDenied, err.Error()) return } diff --git a/weed/s3api/s3api_object_handlers_postpolicy_test.go b/weed/s3api/s3api_object_handlers_postpolicy_test.go index 0e181d7a1..fd6c65960 100644 --- a/weed/s3api/s3api_object_handlers_postpolicy_test.go +++ b/weed/s3api/s3api_object_handlers_postpolicy_test.go @@ -2,14 +2,20 @@ package s3api import ( "bytes" + "encoding/base64" + "encoding/hex" + "fmt" "io" "mime/multipart" "net/http" "net/http/httptest" "strings" + "sync" "testing" + "time" "github.com/gorilla/mux" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/stretchr/testify/assert" ) @@ -383,3 +389,111 @@ func TestPostPolicyBucketHandlerKeyExtraction(t *testing.T) { }) } } + +// TestPostPolicyBucketHandler_PolicyViolationReturns403 drives the handler +// end-to-end with a signed multipart POST whose policy conditions cannot be +// satisfied by the form fields. It verifies the handler responds 403 +// AccessDenied with no Location redirect, rather than the old 307 Temporary +// Redirect that obscured the policy failure. +func TestPostPolicyBucketHandler_PolicyViolationReturns403(t *testing.T) { + const ( + accessKey = "AKIATESTTESTTEST" + secretKey = "secret-key-for-tests" + region = "us-east-1" + service = "s3" + testBucket = "test-bucket" + ) + + iam := &IdentityAccessManagement{ + hashes: make(map[string]*sync.Pool), + hashCounters: make(map[string]*int32), + } + err := iam.loadS3ApiConfiguration(&iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{ + Name: "tester", + Credentials: []*iam_pb.Credential{{AccessKey: accessKey, SecretKey: secretKey}}, + Actions: []string{"Admin", "Read", "Write"}, + }}, + }) + assert.NoError(t, err, "loadS3ApiConfiguration should succeed") + + s3a := &S3ApiServer{ + option: &S3ApiServerOption{BucketsPath: "/buckets"}, + iam: iam, + } + // Pre-populate the bucket registry so validateTableBucketObjectPath sees + // a non-table bucket without needing a live filer connection. + s3a.bucketRegistry = &BucketRegistry{ + metadataCache: map[string]*BucketMetaData{ + testBucket: {Name: testBucket, IsTableBucket: false}, + }, + notFound: make(map[string]struct{}), + s3a: s3a, + } + + now := time.Now().UTC() + amzDate := now.Format(iso8601Format) + yyyymmddStr := now.Format(yyyymmdd) + credential := fmt.Sprintf("%s/%s/%s/%s/aws4_request", accessKey, yyyymmddStr, region, service) + expiration := now.Add(1 * time.Hour).Format("2006-01-02T15:04:05.000Z") + + policyJSON := fmt.Sprintf( + `{"expiration":"%s","conditions":[`+ + `["eq","$bucket","%s"],`+ + `["eq","$key","required.txt"],`+ + `["eq","$x-amz-credential","%s"],`+ + `["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],`+ + `["eq","$x-amz-date","%s"]`+ + `]}`, + expiration, testBucket, credential, amzDate, + ) + encodedPolicy := base64.StdEncoding.EncodeToString([]byte(policyJSON)) + + signingKey := getSigningKey(secretKey, yyyymmddStr, region, service) + signature := getSignature(signingKey, encodedPolicy) + // Sanity-check: the signature must be valid hex of the expected length so + // that doesPolicySignatureV4Match does not trip on formatting. + _, decodeErr := hex.DecodeString(signature) + assert.NoError(t, decodeErr, "computed signature should be hex-encoded") + + var buf bytes.Buffer + writer := multipart.NewWriter(&buf) + assert.NoError(t, writer.WriteField("bucket", testBucket)) + // Deliberately mismatch the policy's required key to force a violation. + assert.NoError(t, writer.WriteField("key", "wrong.txt")) + assert.NoError(t, writer.WriteField("x-amz-credential", credential)) + assert.NoError(t, writer.WriteField("x-amz-algorithm", "AWS4-HMAC-SHA256")) + assert.NoError(t, writer.WriteField("x-amz-date", amzDate)) + assert.NoError(t, writer.WriteField("policy", encodedPolicy)) + assert.NoError(t, writer.WriteField("x-amz-signature", signature)) + + filePart, err := writer.CreateFormFile("file", "payload.txt") + assert.NoError(t, err) + _, err = filePart.Write([]byte("contents that should never be uploaded")) + assert.NoError(t, err) + assert.NoError(t, writer.Close()) + + req := httptest.NewRequest(http.MethodPost, "/"+testBucket, &buf) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req = mux.SetURLVars(req, map[string]string{"bucket": testBucket}) + rec := httptest.NewRecorder() + + s3a.PostPolicyBucketHandler(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code, + "policy violation must return 403, actual body: %s", rec.Body.String()) + assert.NotEqual(t, http.StatusTemporaryRedirect, rec.Code, + "must not return 307 Temporary Redirect on policy violation") + assert.Empty(t, rec.Header().Get("Location"), + "403 response must not set a redirect Location header") + assert.Contains(t, rec.Body.String(), "AccessDenied", + "response body should identify AccessDenied; actual body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "Policy Condition failed", + "response body should carry the specific policy-failure message; actual body: %s", rec.Body.String()) + // Guard against the signing setup silently failing before the policy + // branch ever runs, which would make the 403 assertion meaningless. + assert.NotContains(t, rec.Body.String(), "SignatureDoesNotMatch", + "signature must match so the handler reaches the policy-check branch") + assert.NotContains(t, rec.Body.String(), "InvalidAccessKeyId", + "access key must be known so the handler reaches the policy-check branch") +} diff --git a/weed/s3api/s3err/error_handler.go b/weed/s3api/s3err/error_handler.go index ec819dfc8..180546e43 100644 --- a/weed/s3api/s3err/error_handler.go +++ b/weed/s3api/s3err/error_handler.go @@ -40,6 +40,15 @@ func WriteEmptyResponse(w http.ResponseWriter, r *http.Request, statusCode int) } func WriteErrorResponse(w http.ResponseWriter, r *http.Request, errorCode ErrorCode) { + WriteErrorResponseWithMessage(w, r, errorCode, "") +} + +// WriteErrorResponseWithMessage writes an S3 error response that uses the +// standard error code mapping (status + Code). When message is non-empty, +// it overrides the default Message field so the caller can surface why the +// request was rejected (e.g. which POST policy condition failed) instead +// of the generic APIError Description. +func WriteErrorResponseWithMessage(w http.ResponseWriter, r *http.Request, errorCode ErrorCode, message string) { r, reqID := request_id.Ensure(r) vars := mux.Vars(r) bucket := vars["bucket"] @@ -50,6 +59,9 @@ func WriteErrorResponse(w http.ResponseWriter, r *http.Request, errorCode ErrorC apiError := GetAPIError(errorCode) errorResponse := getRESTErrorResponse(apiError, r.URL.Path, bucket, object, reqID) + if message != "" { + errorResponse.Message = message + } WriteXMLResponse(w, r, apiError.HTTPStatusCode, errorResponse) PostLog(r, apiError.HTTPStatusCode, errorCode) }