Fix origin_server_auth URL encoding for mixed-encoding URLs #12802

bryancall · 2026-01-12T22:04:11Z

Summary

Fixed the isUriEncoded() and canonicalEncode() functions in the origin_server_auth plugin to properly handle URLs with mixed encoding (some characters encoded, some not).

Bug Description

When a URL has mixed encoding (e.g., /app/(channel)/%5B%5Bparts%5D%5D/page.js where parentheses are NOT encoded but brackets ARE encoded), the AWS v4 signature calculation was incorrect:

isUriEncoded() found %5B and returned true, incorrectly assuming the entire string was fully encoded
canonicalEncode() returned the string as-is
Signature was calculated for the partially-encoded path
S3/GCP expected signature for the fully-encoded canonical path
Result: 403 SignatureDoesNotMatch

AWS SigV4 Canonical URI Encoding

Per the AWS SigV4 spec:

"URI-encode every byte except the unreserved characters: 'A'-'Z', 'a'-'z', '0'-'9', '-', '.', '_', and '~'."

This means characters like (, ), !, *, ' must be percent-encoded in the canonical URI for signature calculation, even though they are listed as "safe" for S3 object key names.

Character	S3 "Safe" for key names	SigV4 Unreserved	Must encode for signature
`(`	✅ Yes	❌ No	✅ Yes (`%28`)
`)`	✅ Yes	❌ No	✅ Yes (`%29`)
`!`	✅ Yes	❌ No	✅ Yes (`%21`)
`*`	✅ Yes	❌ No	✅ Yes (`%2A`)
`'`	✅ Yes	❌ No	✅ Yes (`%27`)

Fix

isUriEncoded(): Now checks the ENTIRE string and returns false if ANY character that should be encoded is found unencoded
canonicalEncode(): For partially-encoded strings, decodes first then re-encodes to ensure consistent canonical output
Added uriDecode() helper function

Testing

Added unit tests for mixed encoding scenarios
Added tests for all S3 "safe" characters that need SigV4 encoding (!, *, ', (, ))
Added autest for end-to-end verification
All 60 test cases pass (184 assertions)

References

AWS SigV4 spec: https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html
S3 object key naming: https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html

Added unit tests and autest to demonstrate the isUriEncoded() bug in the origin_server_auth plugin (s3_auth). Bug: When a URL has mixed encoding (some characters encoded, some not), isUriEncoded() incorrectly returns true upon finding the first %XX sequence, assuming the entire string is already encoded. This causes canonicalEncode() to skip re-encoding unencoded characters like parentheses. Example: Client sends: /app/(channel)/%5B%5Bparts%5D%5D/page.js isUriEncoded(): Returns true (finds %5B) canonicalEncode(): Returns as-is (thinks already encoded) Signature for: /app/(channel)/%5B%5Bparts%5D%5D/page.js <- WRONG S3 expects: /app/%28channel%29/%5B%5Bparts%5D%5D/page.js This results in S3 returning 403 SignatureDoesNotMatch. Unit tests marked with [!mayfail] to document expected-to-fail behavior. Autest passes with mock server but would fail with real S3.

Fixed the isUriEncoded() and canonicalEncode() functions to properly handle URLs with mixed encoding (some characters encoded, some not). Bug: URLs like /app/(channel)/%5B%5Bparts%5D%5D/page.js would cause S3 signature mismatch because: - isUriEncoded() found %5B and returned true (incorrectly) - canonicalEncode() returned the string as-is - Signature was calculated for partially-encoded path - S3 expected signature for fully-encoded path Fix: 1. isUriEncoded() now checks the ENTIRE string and returns false if ANY character that should be encoded is found unencoded. 2. canonicalEncode() now always decodes first, then re-encodes using AWS canonical rules. This ensures consistent output regardless of input. Added uriDecode() helper function to decode percent-encoded characters. Updated unit tests to verify the correct behavior. This fixes YTSATS-4835.

Copilot

Pull request overview

This PR fixes a critical bug in the origin_server_auth plugin where mixed URL encoding (some characters encoded, others not) caused AWS SigV4 signature mismatches with S3/GCP.

Changes:

Added uriDecode() function to decode percent-encoded URLs
Modified isUriEncoded() to check if ALL characters requiring encoding are encoded (not just if ANY encoded character exists)
Modified canonicalEncode() to decode-then-reencode partially-encoded URLs for consistent canonical output

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
plugins/origin_server_auth/aws_auth_v4.cc	Added uriDecode() function and fixed isUriEncoded()/canonicalEncode() to handle mixed encoding
plugins/origin_server_auth/unit_tests/test_aws_auth_v4.h	Exposed uriDecode() and canonicalEncode() functions for unit testing
plugins/origin_server_auth/unit_tests/test_aws_auth_v4.cc	Added comprehensive unit tests for mixed encoding scenarios and S3 safe characters
tests/gold_tests/pluginTest/origin_server_auth/s3_url_encoding.test.py	Added integration test demonstrating the bug and verifying the fix
tests/gold_tests/pluginTest/origin_server_auth/rules/s3_url_encoding.test_input	Added test configuration with fake AWS credentials

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

plugins/origin_server_auth/aws_auth_v4.cc

tests/gold_tests/pluginTest/origin_server_auth/s3_url_encoding.test.py

plugins/origin_server_auth/unit_tests/test_aws_auth_v4.cc

plugins/origin_server_auth/aws_auth_v4.cc

AWS SigV4 requires uppercase hex digits in percent-encoding (e.g., %2F not %2f). URLs with lowercase hex were incorrectly passing through isUriEncoded() without normalization, causing potential signature mismatch with S3. Changes: - isUriEncoded() now returns false for lowercase hex digits, triggering canonicalEncode() to normalize via decode/re-encode - Added comprehensive tests for lowercase hex handling - Improved documentation comments Fixes issue identified by Copilot review.

Simple URIs like /path/foo.jpg with only unreserved characters were unnecessarily being decoded and re-encoded. Now isUriEncoded() returns true for strings that are already in canonical form (no reserved chars needing encoding), avoiding the unnecessary processing.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

plugins/origin_server_auth/aws_auth_v4.cc

tests/gold_tests/pluginTest/origin_server_auth/s3_url_encoding.test.py

plugins/origin_server_auth/unit_tests/test_aws_auth_v4.cc

plugins/origin_server_auth/aws_auth_v4.cc

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

plugins/origin_server_auth/unit_tests/test_aws_auth_v4.cc

plugins/origin_server_auth/aws_auth_v4.cc

plugins/origin_server_auth/unit_tests/test_aws_auth_v4.cc

plugins/origin_server_auth/aws_auth_v4.cc

plugins/origin_server_auth/unit_tests/test_aws_auth_v4.cc

plugins/origin_server_auth/aws_auth_v4.cc

- Cast char to unsigned char before calling isxdigit/isalnum/islower to avoid undefined behavior with signed char - Update comments to past tense since the bug is now fixed - Fix bounds check order in isUriEncoded to check length before access

The function now returns true for strings that are already in canonical form, which includes both: - Properly percent-encoded strings with uppercase hex - Strings with only unreserved characters (no encoding needed) The old name 'isUriEncoded' was misleading since it returned true for strings that have NO encoding at all.

maskit · 2026-01-14T16:38:18Z

canonicalEncode(): For partially-encoded strings, decodes first then re-encodes to ensure consistent canonical output

How do you know if it's partial/mixed encoding? The input might be unencoded and the expectation could be encoding %5B as %255B.

bryancall added 2 commits January 12, 2026 13:35

bryancall added this to the 10.2.0 milestone Jan 12, 2026

bryancall self-assigned this Jan 12, 2026

bryancall added s3_auth s3_auth plugin Bug labels Jan 12, 2026

bryancall changed the title ~~Fix S3 auth URL encoding for mixed-encoding URLs~~ Fix origin_server_auth URL encoding for mixed-encoding URLs Jan 12, 2026

bryancall requested review from Copilot and jasmine-nahrain January 12, 2026 22:42

Copilot started reviewing on behalf of bryancall January 12, 2026 22:42 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

bryancall requested a review from Copilot January 13, 2026 17:17

Copilot started reviewing on behalf of bryancall January 13, 2026 17:17 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

bryancall requested a review from Copilot January 13, 2026 17:30

Copilot started reviewing on behalf of bryancall January 13, 2026 17:32 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

bryancall added 2 commits January 13, 2026 09:46

jasmine-nahrain approved these changes Jan 13, 2026

View reviewed changes

Fix origin_server_auth URL encoding for mixed-encoding URLs #12802

Are you sure you want to change the base?

Fix origin_server_auth URL encoding for mixed-encoding URLs #12802

Conversation

bryancall commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bug Description

AWS SigV4 Canonical URI Encoding

Fix

Testing

References

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maskit commented Jan 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bryancall commented Jan 12, 2026 •

edited

Loading