fix(sts): escape caller text in exchange error bodies - #246
Merged
Merged
Conversation
A token file written with `jq -r … > file` ends in a newline, which SDKs send as-is and the signature's base64 decode rejects with a 400. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The separate refused key cost an extra Cache API match before every
lookup, and the delete of the positive entry never found anything after
a 403. A refusal is now stored as {"trusted":false} under the same key
for REFUSED_TRUST_CACHE_SECS: one Cache API read per exchange.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Platform-token refusals and the API key path's non-key errors came from build_sts_error_response without it, and SDKs show only the message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The platform path takes its issuers' tokens ahead of the STS route, so an entry for AUTH_ISSUER would refuse every person exchange. Drop it at load with an error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fail-closed check ran before the API-key and platform short-circuits, so an empty AUTH_AUDIENCE also disabled exchanges that carry their own audience checks (platform) or none at all (keys). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RoleArn and a token's kid or alg reached the <Message> of an application/xml response unescaped, through multistore's build_sts_error_response: markup there ran as XHTML on the proxy origin, and an & made the body unparseable for SDKs. The API-key and platform paths now build their errors here with the message escaped. The person route's errors still come from multistore's builder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
🚀 Latest commit deployed to https://source-data-proxy-pr-246.source-coop.workers.dev
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked on #237. Until #237's branch has its latest review fixes pushed, this diff also shows those five commits; the change here is the last commit alone.
What I'm changing
The API-key and platform-token paths at
/.stsnow escape their error messages. Those messages can carry text the caller sent,RoleArnor a token'skidoralg, and multistore'sbuild_sts_error_responsewrote it unescaped into anapplication/xmlbody served withaccess-control-allow-origin: *. So:RoleArnran as XHTML on the proxy's origin, for example<x:script xmlns:x="http://www.w3.org/1999/xhtml">…</x:script>;&inRoleArnmade the body unparseable for SDKs.How I did it
sts_refusalmaps aProxyErrorto status, code and message the way multistore's builder does, then builds the body withsts_error_xml.sts_error_xml, which every error on these two paths goes through, escapes&,<and>in the message.build_sts_error_responseis no longer used in this crate.Not covered: the person issuer's route. Its errors come from multistore's router (
with_sts), which calls the same unescaped builder, so the fix there belongs in multistore'sbuild_sts_error_response.How to test it
tests/test_platform_trust.py::test_a_refusal_escapes_what_the_caller_sentsends a forged GitHub token with aRoleArncontaining an XHTML script element and&. It checks that the reply parses as XML and that the message carries theRoleArnas text. Locally againstwrangler devand the stub, 47 tests pass and 16 skip (the skipped ones need a GitHub token).PR Checklist
Related Issues
Follows the review of #237.
🤖 Generated with Claude Code