fix(it): tamper the decoded GCM tag, not a base64 padding bit - #121
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 🔍
|
…fix its tamper helper Two fixes in the cookie-session IT: 1. sealedCookieValueCarriesNoReadableToken asserted the sealed cookie's leading version byte is 1. FORMAT_VERSION is now 2, so assert 2. 2. tamperedCookieIsUnauthenticatedNeverServerError used the base64-padding-bit tamper helper that PR #121 replaced in BffCookieStatelessnessIT but left in place here. Flipping the final base64url character is not reliably a mutation: when the sealed length is not a multiple of three that character carries unused padding bits which the decoder discards, so the value re-decodes identically and the peer correctly answers 200. Adding the 9th sealed-payload field shifted the length so the degenerate case became deterministic rather than the ~4% flake #121 described. Apply the same decoded-GCM-tag mutation the sibling IT already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EfShpgB9HbUSf9n6zhP9r
1 participant
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.Note
Supersedes #120, which was closed unmerged. Same commit, no code change — reopened so the review bots get a clean run after both CodeRabbit and Sourcery hit account rate limits on the original.
🤖 Generated with Claude Code
https://claude.ai/code/session_01G99PK9iS9vgHvttFZ7ueir