From bfea14320a3b5430a5ce24ea54c90700f6b8a567 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 17 Apr 2026 14:55:12 -0700 Subject: [PATCH] fix(s3): reject unknown POST policy conditions and extra x-amz form fields (#9124) * fix(s3): reject unknown POST policy conditions and extra x-amz form fields CheckPostPolicy previously accepted policy conditions with unknown $keys (e.g. "$foo") as satisfied, and only rejected stray X-Amz-Meta-* form fields. Reject unknown condition keys outright, and extend the extra- input-fields check to all X-Amz-* form fields except the reserved auth/signing headers. Matches AWS S3 POST Object behavior. * refactor(s3): drop redundant $x-amz-meta- prefix check in CheckPostPolicy The $x-amz- prefix already subsumes $x-amz-meta-, so the explicit $x-amz-meta- check adds no coverage. Simplify the else-if condition. Addresses gemini-code-assist review on PR #9124. * style(s3): align unknown-key policy error with [op, key, value] trailer Reformat the unknown-condition-key error in CheckPostPolicy to include the same "[op, key, value]" trailer used by the other condition-failed messages. The value slot is empty because no comparison occurs for an unknown key. The descriptive "unknown condition key" suffix is kept so operators can still tell this failure from a mismatched value. * fix(s3): honor starts-with prefix-stem POST policies when checking extras AWS POST policies use ["starts-with","$x-amz-meta-",""] to allow any X-Amz-Meta-* form field. The previous exact-match policyXAmzKeys would flag every X-Amz-Meta-Foo as an "Extra input fields" failure because only the stem X-Amz-Meta- was stored. Track starts-with conditions whose key ends in "-" with an empty value as prefix stems, and accept any X-Amz-* form field matching one of those stems. * fix(s3): validate value prefix for starts-with POST policy stems Drop the policy.Value == "" gate when detecting prefix-stem conditions so that ["starts-with","$x-amz-meta-","pfx-"] is recognized as a prefix rule. Track the required value prefix alongside the name prefix, enforce it against every matching form field in the extras loop, and skip the prefix-stem condition in the main iteration (it has no single form field to evaluate). Also include policy.Value in the unknown-condition error trailer for clearer debugging. Addresses gemini-code-assist review on PR #9124. * fix(s3): check every matching POST policy rule, not just the first The extras loop exited early on exact-key match and broke on the first matching prefix stem. Per AWS, a form field must satisfy every policy condition that applies to it, so an exact-match field must still honor any overlapping starts-with stem's value prefix, and multiple stems on the same field must all hold. Drop both early exits: start matched from the exact-key lookup, iterate all prefix stems, and fail on the first value-prefix violation. Addresses gemini-code-assist review on PR #9124. --- weed/s3api/policy/post-policy_test.go | 276 ++++++++++++++++++++++++++ weed/s3api/policy/postpolicyform.go | 101 ++++++++-- 2 files changed, 360 insertions(+), 17 deletions(-) diff --git a/weed/s3api/policy/post-policy_test.go b/weed/s3api/policy/post-policy_test.go index 8b054023a..13cf9fa15 100644 --- a/weed/s3api/policy/post-policy_test.go +++ b/weed/s3api/policy/post-policy_test.go @@ -29,6 +29,7 @@ import ( "net/url" "regexp" "strings" + "testing" "time" "unicode/utf8" ) @@ -376,3 +377,278 @@ func getScope(t time.Time, region string) string { }, "/") return scope } + +// buildParsedPolicy is a small test helper that assembles a JSON policy +// document with the supplied condition snippets and parses it into a +// PostPolicyForm. Using ParsePostPolicyForm avoids having to construct the +// anonymous struct in PostPolicyForm.Conditions.Policies directly. +func buildParsedPolicy(t *testing.T, conditions string) PostPolicyForm { + t.Helper() + expiration := time.Now().UTC().Add(24 * time.Hour).Format(iso8601TimeFormat) + raw := fmt.Sprintf(`{"expiration":"%s","conditions":[%s]}`, expiration, conditions) + ppf, err := ParsePostPolicyForm(raw) + if err != nil { + t.Fatalf("ParsePostPolicyForm failed: %v\npolicy: %s", err, raw) + } + return ppf +} + +// TestCheckPostPolicy_RejectsUnknownConditionKey verifies that a policy +// containing a condition key that is neither in startsWithConds nor prefixed +// with $x-amz- is rejected instead of being silently accepted. +func TestCheckPostPolicy_RejectsUnknownConditionKey(t *testing.T) { + ppf := buildParsedPolicy(t, `["eq","$foo","bar"]`) + + form := http.Header{} + form.Set("Foo", "bar") + + err := CheckPostPolicy(form, ppf) + if err == nil { + t.Fatalf("expected error for unknown condition key, got nil") + } + if !strings.Contains(err.Error(), "unknown condition key") { + t.Fatalf("expected 'unknown condition key' error, got: %v", err) + } + if !strings.Contains(err.Error(), "$foo") { + t.Fatalf("expected error to name the offending key, got: %v", err) + } +} + +// TestCheckPostPolicy_RejectsExtraXAmzFormField verifies that stray X-Amz-* +// form fields (beyond the reserved auth/signing ones) are rejected when no +// matching policy condition is declared. +func TestCheckPostPolicy_RejectsExtraXAmzFormField(t *testing.T) { + ppf := buildParsedPolicy(t, `["eq","$bucket","mybucket"]`) + + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Storage-Class", "STANDARD") + + err := CheckPostPolicy(form, ppf) + if err == nil { + t.Fatalf("expected error for extra X-Amz-Storage-Class field, got nil") + } + if !strings.Contains(err.Error(), "Extra input fields") { + t.Fatalf("expected 'Extra input fields' error, got: %v", err) + } + if !strings.Contains(err.Error(), "X-Amz-Storage-Class") { + t.Fatalf("expected error to name the offending field, got: %v", err) + } +} + +// TestCheckPostPolicy_AllowsXAmzAuthFields verifies that the reserved +// auth/signing X-Amz-* headers are accepted even when no policy condition +// mentions them, because clients must always send these. +func TestCheckPostPolicy_AllowsXAmzAuthFields(t *testing.T) { + ppf := buildParsedPolicy(t, `["eq","$bucket","mybucket"]`) + + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Signature", "deadbeef") + form.Set("X-Amz-Credential", "AKIA/20260417/us-east-1/s3/aws4_request") + form.Set("X-Amz-Algorithm", "AWS4-HMAC-SHA256") + form.Set("X-Amz-Date", "20260417T000000Z") + form.Set("X-Amz-Security-Token", "session-token") + + if err := CheckPostPolicy(form, ppf); err != nil { + t.Fatalf("expected no error with only reserved auth fields, got: %v", err) + } +} + +// TestCheckPostPolicy_AllowsMatchingXAmzField verifies that an X-Amz-* form +// field is accepted when there is a matching equality policy condition. +func TestCheckPostPolicy_AllowsMatchingXAmzField(t *testing.T) { + ppf := buildParsedPolicy(t, `["eq","$bucket","mybucket"],["eq","$x-amz-storage-class","STANDARD"]`) + + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Storage-Class", "STANDARD") + + if err := CheckPostPolicy(form, ppf); err != nil { + t.Fatalf("expected no error for matching X-Amz-Storage-Class, got: %v", err) + } +} + +// TestCheckPostPolicy_ExistingXAmzMetaCheckStillWorks is a regression test +// for the pre-existing behavior: a stray X-Amz-Meta-* form field without a +// matching condition is still rejected. +func TestCheckPostPolicy_ExistingXAmzMetaCheckStillWorks(t *testing.T) { + ppf := buildParsedPolicy(t, `["eq","$bucket","mybucket"]`) + + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Meta-Foo", "bar") + + err := CheckPostPolicy(form, ppf) + if err == nil { + t.Fatalf("expected error for extra X-Amz-Meta-Foo field, got nil") + } + if !strings.Contains(err.Error(), "Extra input fields") { + t.Fatalf("expected 'Extra input fields' error, got: %v", err) + } + if !strings.Contains(err.Error(), "X-Amz-Meta-Foo") { + t.Fatalf("expected error to name the offending field, got: %v", err) + } +} + +// TestCheckPostPolicy_AllowsStartsWithPrefixStem covers the AWS convention +// where ["starts-with","$x-amz-meta-",""] permits any X-Amz-Meta-* form +// field. Without prefix-stem handling, such fields would be wrongly +// rejected as "Extra input fields". +func TestCheckPostPolicy_AllowsStartsWithPrefixStem(t *testing.T) { + ppf := buildParsedPolicy(t, + `["eq","$bucket","mybucket"],["starts-with","$x-amz-meta-",""]`, + ) + + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Meta-Foo", "bar") + form.Set("X-Amz-Meta-Another", "baz") + + if err := CheckPostPolicy(form, ppf); err != nil { + t.Fatalf("expected no error for prefix-matched meta fields, got: %v", err) + } +} + +// TestCheckPostPolicy_PrefixStemDoesNotCoverOtherPrefixes ensures the +// prefix allowance is scoped: a starts-with stem for x-amz-meta- must not +// whitelist unrelated x-amz-* fields like x-amz-storage-class. +func TestCheckPostPolicy_PrefixStemDoesNotCoverOtherPrefixes(t *testing.T) { + ppf := buildParsedPolicy(t, + `["eq","$bucket","mybucket"],["starts-with","$x-amz-meta-",""]`, + ) + + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Storage-Class", "STANDARD") + + err := CheckPostPolicy(form, ppf) + if err == nil { + t.Fatalf("expected error for X-Amz-Storage-Class not covered by meta prefix, got nil") + } + if !strings.Contains(err.Error(), "X-Amz-Storage-Class") { + t.Fatalf("expected error to name X-Amz-Storage-Class, got: %v", err) + } +} + +// TestCheckPostPolicy_PrefixStemEnforcesValuePrefix covers a starts-with +// prefix-stem policy with a non-empty required value prefix: matching +// fields must have values that satisfy the value prefix. +func TestCheckPostPolicy_PrefixStemEnforcesValuePrefix(t *testing.T) { + ppf := buildParsedPolicy(t, + `["eq","$bucket","mybucket"],["starts-with","$x-amz-meta-","pfx-"]`, + ) + + // Value satisfies the required prefix: accepted. + okForm := http.Header{} + okForm.Set("Bucket", "mybucket") + okForm.Set("X-Amz-Meta-Foo", "pfx-bar") + if err := CheckPostPolicy(okForm, ppf); err != nil { + t.Fatalf("expected no error when meta value matches required prefix, got: %v", err) + } + + // Value does not satisfy the required prefix: rejected as policy failure. + badForm := http.Header{} + badForm.Set("Bucket", "mybucket") + badForm.Set("X-Amz-Meta-Foo", "other") + err := CheckPostPolicy(badForm, ppf) + if err == nil { + t.Fatalf("expected error when meta value misses required prefix, got nil") + } + if !strings.Contains(err.Error(), "Policy Condition failed") { + t.Fatalf("expected 'Policy Condition failed' error, got: %v", err) + } + if !strings.Contains(err.Error(), "pfx-") { + t.Fatalf("expected error to reference the required value prefix, got: %v", err) + } +} + +// TestCheckPostPolicy_ExactAndPrefixBothEnforced covers a field that is +// simultaneously covered by an exact-key condition and a prefix-stem +// condition. Both must be satisfied; exact-match alone does not let the +// field skip the stem's value-prefix check. +func TestCheckPostPolicy_ExactAndPrefixBothEnforced(t *testing.T) { + ppf := buildParsedPolicy(t, + `["eq","$bucket","mybucket"],`+ + `["eq","$x-amz-meta-tag","gold"],`+ + `["starts-with","$x-amz-meta-","lvl-"]`, + ) + + form := http.Header{} + form.Set("Bucket", "mybucket") + // Satisfies the exact eq condition but not the starts-with stem. + form.Set("X-Amz-Meta-Tag", "gold") + + err := CheckPostPolicy(form, ppf) + if err == nil { + t.Fatalf("expected error: exact-match field must still satisfy overlapping prefix stem, got nil") + } + if !strings.Contains(err.Error(), "Policy Condition failed") { + t.Fatalf("expected 'Policy Condition failed' error, got: %v", err) + } + if !strings.Contains(err.Error(), "lvl-") { + t.Fatalf("expected error to reference the stem's value prefix, got: %v", err) + } +} + +// TestCheckPostPolicy_MultiplePrefixStemsAllEnforced covers a field that +// matches multiple starts-with prefix stems. All stems must hold; a value +// that satisfies one but not the other must be rejected. +func TestCheckPostPolicy_MultiplePrefixStemsAllEnforced(t *testing.T) { + ppf := buildParsedPolicy(t, + `["eq","$bucket","mybucket"],`+ + `["starts-with","$x-amz-meta-","pfx-"],`+ + `["starts-with","$x-amz-meta-color-","red-"]`, + ) + + // Satisfies the broader stem but not the color- stem's value prefix. + form := http.Header{} + form.Set("Bucket", "mybucket") + form.Set("X-Amz-Meta-Color-Main", "pfx-green") + + err := CheckPostPolicy(form, ppf) + if err == nil { + t.Fatalf("expected error: field must satisfy every matching prefix stem, got nil") + } + if !strings.Contains(err.Error(), "Policy Condition failed") { + t.Fatalf("expected 'Policy Condition failed' error, got: %v", err) + } + if !strings.Contains(err.Error(), "red-") { + t.Fatalf("expected error to reference the failing value prefix, got: %v", err) + } + + // A policy with compatible stems (broad allows anything, narrow + // requires "red-") accepts a value honoring both. + compatible := buildParsedPolicy(t, + `["eq","$bucket","mybucket"],`+ + `["starts-with","$x-amz-meta-",""],`+ + `["starts-with","$x-amz-meta-color-","red-"]`, + ) + okForm := http.Header{} + okForm.Set("Bucket", "mybucket") + okForm.Set("X-Amz-Meta-Color-Main", "red-main") + if err := CheckPostPolicy(okForm, compatible); err != nil { + t.Fatalf("expected no error when value satisfies both compatible stems, got: %v", err) + } +} + +// TestCheckPostPolicy_UnknownKeyErrorIncludesPolicyValue ensures the +// unknown-condition error surfaces the policy value in its [op, key, value] +// trailer so operators can tell which of several unknown keys failed. +func TestCheckPostPolicy_UnknownKeyErrorIncludesPolicyValue(t *testing.T) { + ppf := buildParsedPolicy(t, `["eq","$foo","custom-value"]`) + + err := CheckPostPolicy(http.Header{}, ppf) + if err == nil { + t.Fatalf("expected error for unknown condition key, got nil") + } + if !strings.Contains(err.Error(), "custom-value") { + t.Fatalf("expected error to include policy.Value 'custom-value', got: %v", err) + } + if !strings.Contains(err.Error(), "$foo") { + t.Fatalf("expected error to include policy.Key '$foo', got: %v", err) + } + if !strings.Contains(err.Error(), "unknown condition key") { + t.Fatalf("expected 'unknown condition key' suffix, got: %v", err) + } +} diff --git a/weed/s3api/policy/postpolicyform.go b/weed/s3api/policy/postpolicyform.go index 011f782d7..46bf38075 100644 --- a/weed/s3api/policy/postpolicyform.go +++ b/weed/s3api/policy/postpolicyform.go @@ -215,6 +215,27 @@ func checkPolicyCond(op string, input1, input2 string) bool { return false } +// xAmzPrefixRule captures a starts-with policy condition whose key ends in +// "-" and therefore matches a prefix of form field names, e.g. +// ["starts-with","$x-amz-meta-","pfx-"]. Any form field whose name starts +// with namePrefix must have a value starting with valuePrefix. +type xAmzPrefixRule struct { + policyKey string + namePrefix string + valuePrefix string +} + +// postPolicyAuthFields enumerates the X-Amz-* form fields that clients are +// required to send with every POST Object request for signing/authentication. +// These must be accepted even when no matching policy condition is declared. +var postPolicyAuthFields = map[string]bool{ + "X-Amz-Signature": true, + "X-Amz-Credential": true, + "X-Amz-Algorithm": true, + "X-Amz-Date": true, + "X-Amz-Security-Token": true, +} + // CheckPostPolicy - apply policy conditions and validate input values. // (http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-HTTPPOSTConstructPolicy.html) func CheckPostPolicy(formValues http.Header, postPolicyForm PostPolicyForm) error { @@ -222,20 +243,57 @@ func CheckPostPolicy(formValues http.Header, postPolicyForm PostPolicyForm) erro if !postPolicyForm.Expiration.After(time.Now().UTC()) { return fmt.Errorf("Invalid according to Policy: Policy expired") } - // map to store the metadata - metaMap := make(map[string]string) + // Track $x-amz-* policy conditions in canonical form. A starts-with + // condition on a key that itself ends with "-" (e.g. + // ["starts-with","$x-amz-meta-","pfx-"]) is the AWS convention for + // allowing any form field sharing that name prefix; capture the name + // prefix together with its required value prefix so the extras check + // can both accept matching fields and enforce the value constraint. + policyXAmzKeys := make(map[string]bool) + var policyXAmzPrefixes []xAmzPrefixRule for _, policy := range postPolicyForm.Conditions.Policies { - if strings.HasPrefix(policy.Key, "$x-amz-meta-") { - formCanonicalName := http.CanonicalHeaderKey(strings.TrimPrefix(policy.Key, "$")) - metaMap[formCanonicalName] = policy.Value + if !strings.HasPrefix(policy.Key, "$x-amz-") { + continue } + formCanonicalName := http.CanonicalHeaderKey(strings.TrimPrefix(policy.Key, "$")) + if policy.Operator == policyCondStartsWith && strings.HasSuffix(policy.Key, "-") { + policyXAmzPrefixes = append(policyXAmzPrefixes, xAmzPrefixRule{ + policyKey: policy.Key, + namePrefix: formCanonicalName, + valuePrefix: policy.Value, + }) + continue + } + policyXAmzKeys[formCanonicalName] = true } - // Check if any extra metadata field is passed as input + // Reject any X-Amz-* form field that has no matching policy condition, + // except for the reserved auth/signing fields clients must always send. + // A field may be covered by an exact-key condition, by any number of + // prefix-stem rules, or both. Check every applicable rule: AWS requires + // all matching conditions to be satisfied, so exact-match coverage does + // not let a field skip the value-prefix enforcement of overlapping + // starts-with stems, and multiple stems on the same field must all hold. + // Prefix-stem conditions are then skipped in the main loop below because + // no single form field corresponds to the prefix itself. for key := range formValues { - if strings.HasPrefix(key, "X-Amz-Meta-") { - if _, ok := metaMap[key]; !ok { - return fmt.Errorf("Invalid according to Policy: Extra input fields: %s", key) + if !strings.HasPrefix(key, "X-Amz-") { + continue + } + if postPolicyAuthFields[key] { + continue + } + matched := policyXAmzKeys[key] + for _, rule := range policyXAmzPrefixes { + if !strings.HasPrefix(key, rule.namePrefix) { + continue } + matched = true + if !strings.HasPrefix(formValues.Get(key), rule.valuePrefix) { + return fmt.Errorf("Invalid according to Policy: Policy Condition failed: [%s, %s, %s]", policyCondStartsWith, rule.policyKey, rule.valuePrefix) + } + } + if !matched { + return fmt.Errorf("Invalid according to Policy: Extra input fields: %s", key) } } @@ -249,6 +307,13 @@ func CheckPostPolicy(formValues http.Header, postPolicyForm PostPolicyForm) erro formCanonicalName := http.CanonicalHeaderKey(strings.TrimPrefix(policy.Key, "$")) // Operator for the current policy condition op := policy.Operator + // Prefix-stem x-amz-* conditions (key ending in "-" with starts-with) + // are validated above against every matching form field; skip them + // here so we do not fail on the non-existent literal form field whose + // name equals the prefix itself. + if strings.HasPrefix(policy.Key, "$x-amz-") && op == policyCondStartsWith && strings.HasSuffix(policy.Key, "-") { + continue + } // If the current policy condition is known if startsWithSupported, condFound := startsWithConds[policy.Key]; condFound { // Check if the current condition supports starts-with operator @@ -260,15 +325,17 @@ func CheckPostPolicy(formValues http.Header, postPolicyForm PostPolicyForm) erro if !condPassed { return fmt.Errorf("Invalid according to Policy: Policy Condition failed") } - } else { - // This covers all conditions X-Amz-Meta-* and X-Amz-* - if strings.HasPrefix(policy.Key, "$x-amz-meta-") || strings.HasPrefix(policy.Key, "$x-amz-") { - // Check if policy condition is satisfied - condPassed = checkPolicyCond(op, formValues.Get(formCanonicalName), policy.Value) - if !condPassed { - return fmt.Errorf("Invalid according to Policy: Policy Condition failed: [%s, %s, %s]", op, policy.Key, policy.Value) - } + } else if strings.HasPrefix(policy.Key, "$x-amz-") { + // Check if policy condition is satisfied + condPassed = checkPolicyCond(op, formValues.Get(formCanonicalName), policy.Value) + if !condPassed { + return fmt.Errorf("Invalid according to Policy: Policy Condition failed: [%s, %s, %s]", op, policy.Key, policy.Value) } + } else { + // Unknown condition key: neither in startsWithConds nor a $x-amz-* + // prefixed key. AWS rejects these outright instead of silently + // treating the condition as satisfied. + return fmt.Errorf("Invalid according to Policy: Policy Condition failed: [%s, %s, %s]: unknown condition key", op, policy.Key, policy.Value) } }