fix(it): tamper the decoded GCM tag, not a base64 padding bit - #120
fix(it): tamper the decoded GCM tag, not a base64 padding bit#120cuioss-oliver wants to merge 1 commit into
Conversation
BffCookieStatelessnessIT.thePeerRefusesACookieItCannotUnseal flipped the
final base64url character of the sealed cookie ('A' -> 'B') to break the
authentication tag. That is not reliably a mutation: when the sealed
length is not a multiple of three, the final character carries 2 or 4
unused padding bits, and Base64.getUrlDecoder() discards them without
validating. Flipping only those bits re-decodes to the identical
ciphertext and tag, so the peer unseals the cookie successfully and
answers 200 instead of the expected 401.
The no-op needs the last character to be 'A' AND the sealed length to be
non-multiple-of-3 — roughly 4% of runs, since the ciphertext length
tracks the JWT size. That is why the suite passed on the PR head and
failed post-merge on main.
Mutate a byte of the decoded 16-byte GCM tag instead, which is
unambiguous and removes the probabilistic branch entirely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99PK9iS9vgHvttFZ7ueir
There was a problem hiding this comment.
Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe integration test’s sealed-cookie tampering helper now decodes the cookie, flips the final decoded byte, and re-encodes it as unpadded Base64URL. Its documentation and imports were updated accordingly. ChangesSealed-cookie tampering
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Closing unmerged and reopening as a fresh PR so the review bots get a clean run — CodeRabbit and Sourcery both hit account rate limits when this PR was first opened, so its review history is not representative. No code change: the branch is already rebased on |
Problem
BffCookieStatelessnessIT.thePeerRefusesACookieItCannotUnsealfailed post-merge onmain(341ab9e, run 30334562679):This is a defect in the test, not in the gateway. The peer was right to answer 200.
Root cause
The test broke the sealed cookie's authentication tag by flipping the final base64url character:
That is not reliably a mutation. The sealed payload is
version(1) || key-id(1) || nonce(12) || ciphertext || tag(16). When its byte length is not a multiple of three, the final base64url character carries 2 or 4 unused padding bits, andBase64.getUrlDecoder()discards them without validating.'A'(0b000000) →'B'(0b000001) flips only the lowest bit — a padding bit in that case — so the value re-decodes to the identical ciphertext and tag. The GCM tag still authenticates, the peer unseals the session correctly, and the route returns 200.Verified directly:
The no-op requires the last character to be
'A'and the sealed length to be a non-multiple of 3 — roughly 4% of runs, since the ciphertext length tracks the JWT size and varies per login. That is why the suite passed on the PR head (ba3edbc) and failed onmainshortly after.Fix
Mutate a byte of the decoded 16-byte GCM tag instead:
Always a real tag mutation, so the probabilistic branch is gone entirely. The Javadoc records the padding-bit trap so the original shape is not reintroduced.
Verification
verify -Ppre-commitgreentest-compile -pl integration-tests -amgreenNot in scope
Performance Benchmarkis also red onmain, but it is pre-existing and unrelated: the samerun-k6-passthrough-relay-benchmarkchecks, http_req_failedthreshold crossing (exit 99) fails on PR #119 (a workflow-version bump) and on #117 / #116 before it.benchmarks/**is another plan's surface; reported, not touched here.🤖 Generated with Claude Code
https://claude.ai/code/session_01G99PK9iS9vgHvttFZ7ueir
Summary by CodeRabbit