feat(bff): complete cookie variant docs, ADRs and unique session ids - #125
Merged
Conversation
The BffRuntime and BffRuntimeProducer javadoc still described the runtime as server-mode-only, contradicting BffRuntimeProducer.isBffMode(), which activates the runtime for either recognised session.mode (server or cookie) provided a redirect_uri is configured. Rewrite the eight server-mode-only sites in BffRuntime.java (class javadoc, public constructor, inert(), isActive() and the three inert @throws clauses) to mode-neutral wording naming both modes, and drop the stale server-mode qualifier from BffRuntimeProducer's class javadoc, lazy-discovery paragraph and tokenValidator @param so they agree with its own activation paragraph. The literal token server-mode no longer occurs in either file. Javadoc only: no signature, import or behaviour change. dispatch()'s already mode-neutral @throws is left untouched. D5 hypothesis (re-source the user-info endpoint from the sealed cookie) is recorded as ALREADY-SATISFIED, shipped by PLAN-07A: BffRuntimeProducer.java :215-218/:274-275 builds UserInfoEndpoint over the mode-selected SessionBinding, and BffCookieStatelessnessIT.bothInstancesResolveTheSameIdentity() (:106-123) covers the cookie-mode user-info path end-to-end across two instances. No re-implementation and no new cookie-mode user-info IT were added. Co-Authored-By: Claude <noreply@anthropic.com>
… nonce CookieSessionBinding derived the cookie-mode sessionId from a salted digest over the login instant and sub only, so two logins by the same subject inside one clock second collided. A collision cross-wires TokenRefreshCoordinator's single-flight inFlight keying between two genuinely distinct sessions. Mint a per-session random nonce at login and fold it into the digest: - SealedSessionPayload gains a ninth mandatory component sessionNonce, appended in declaration order and read back in decode(). FIELD_COUNT rises to 9 with a strict guard — no legacy eight-field acceptance path, since admitting one would mean synthesizing a nonce and silently changing the derived identity. - SealedSessionCookieCodec.FORMAT_VERSION rises to 2. The version byte is bound into the GCM associated data, so a v1 cookie fails authentication outright rather than being mis-parsed against the nine-field shape. - SessionRecord gains a tenth component Optional<String> sessionNonce, normalized to empty in the canonical constructor and redacted from toString(). Server mode always leaves it empty; only the cookie binding populates it. - CookieSessionBinding mints the nonce once in bind() (32 secure-random bytes, base64url, matching newSessionId()'s width), re-seals it verbatim in persist(), and folds it into derivedIdentity(). persist() fails loud rather than re-minting when a record carries no nonce. - TokenRefreshCoordinator.rotate() copies the previous record's nonce onto the rotated one; rotate() rebuilds field-by-field, so an uncopied component is silently dropped and a dropped nonce would change the identity mid-session. SessionBinding.persist()'s signature is unchanged and the identity remains a digest — the raw nonce is never used as, or substituted for, the sessionId. Consumer sweep for the SessionRecord canonical-constructor break: a module-wide word-boundary identifier search across all four modules returned 178 hits (api-sheriff main and test; integration-tests and benchmarks empty as expected). Classified: 3 direct new SessionRecord(...) sites, all in InMemorySessionStoreTest and updated to the 10-arg form; 0 destructuring or pattern-match sites; the remainder builder() calls or plain type references, all source-compatible. Test contract updated for the new shape, including regressions that an eight-field payload and a v1-stamped cookie are both rejected, that two logins by one subject in a single second yield distinct identities, that the identity is byte-identical across a re-seal, and that rotate() propagates the nonce. CookieKeyMaterialTest also constructed the payload directly and was updated although the plan did not enumerate it. Both cookie ITs pass unmodified. Co-Authored-By: Claude <noreply@anthropic.com>
…-0018/0019 Variant 3 (cookie-based BFF) had only a design section, while Variant 2 carried all three documentation layers. This lands the missing layers, fixes the design section's factual defects, and records the architectural decisions. Design layer (doc/variants/03-bff-cookie.adoc): - session.encryption_key was described as required. It is not: omitting it selects the generate-on-startup key mode. Rewritten to describe the key as optional, naming what declaring it buys (sharing sessions across replicas, surviving restart) and the cost of omitting it. The YAML sample now carries the optionality inline, matching previous_key's existing annotation. - Documented that the variant is stateless after login but not during it: the pending authorization is per-instance, so a login must complete on the instance that began it. Session affinity is required for the login leg only. - Documented the sealed-cookie format bump and its forced-re-login consequence. Operator layer (doc/user/bff-cookie.adoc, new): client registration, mode selection, the two key-material modes and their trade-off, decrypt-only rotation, the 4 KB cookie budget with remediation guidance, what IdP-driven destruction cannot do here, login-leg affinity, and the upgrade consequence. Contributor layer (doc/development/bff-cookie.adoc, new): where the cookie code lives, the SessionBinding seam and why the reserved endpoints stay mode-neutral, the sealed-cookie wire format with the rule that a format change requires a FORMAT_VERSION bump, the derived identity and its stability-and-uniqueness constraints, and the unit/IT entry points including the two-instance topology. ADRs: - ADR-0018 records the BFF session-mode architecture: reserved-path precedence, the fixed CSRF model, the one SessionBinding seam, the cookie key-material two-mode decision, and the per-session identity nonce. The first four originate in earlier BFF work and are marked retroactive in the context. - ADR-0019 records the two inbound-validation relaxations with their bounding constraints: reserved paths bypass the url-parameter value pipeline (they terminate at the gateway and their handlers validate their own parameters), and the cookie-header length raise is necessarily gateway-wide because inbound validation runs before route selection. Indexes: added the new guides to doc/user/README.adoc, doc/development/ README.adoc and doc/README.adoc's Deployment Variants table. The Architecture Decisions table gained ADR-0018/0019 and, as a Boy-Scout fix, the four rows that were missing entirely (ADR-0010, 0011, 0012, 0017) — it now enumerates all 19 ADRs in the corpus. doc/architecture.adoc references both new ADRs and drops the same stale server-mode-only framing corrected earlier in BffRuntime: the producer activates for either recognised session.mode, and both modes drive the same wiring. Co-Authored-By: Claude <noreply@anthropic.com>
…e-docs-adr Collapse two near-identical SessionRecord-rebuild helpers in CookieSessionBindingTest into one shared withRotatedAccessToken helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EfShpgB9HbUSf9n6zhP9r
![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: 59 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 (10)
📝 WalkthroughWalkthroughChangesCookie session nonce and BFF mode
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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 🔍(Review updated until commit 365b30e)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.java (1)
54-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce a nonblank nonce across both contracts. Null checks alone allow
""to become an identity input, defeating the nonce’s uniqueness guarantee for malformed reconstructed records.
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.java#L54-L78: reject blanksessionNoncevalues in the canonical constructor.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.java#L74-L102: retainOptional.empty()for server mode, but reject present blank nonce values.As per coding guidelines, “validate all inputs and outputs and use secure defaults.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.java` around lines 54 - 78, Enforce nonblank session nonces in both record contracts: in SealedSessionPayload’s canonical constructor, reject null or blank sessionNonce values; in SessionRecord, preserve Optional.empty() for server mode but reject any present blank nonce. Apply validation consistently to prevent blank nonce values from entering identity inputs.Source: Coding guidelines
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java (1)
106-119: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComplete the required public API Javadocs.
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java#L106-L119: add@since, thread-safety notes, and usage examples to the constructor documentation.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java#L137-L147: add the required documentation elements toinert()andstepUpCoordinator().api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java#L157-L160: documentisActive()with the required API metadata.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java#L167-L181: completesessionStage()andcsrfDefence()documentation.api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java#L127-L131: complete the public constructor documentation.As per coding guidelines, every public or protected class and method must include these elements.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java` around lines 106 - 119, Complete the public API Javadocs: in api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java lines 106-119, document the constructor with `@since`, thread-safety guidance, and usage examples; lines 137-147, add the required metadata to inert() and stepUpCoordinator(); lines 157-160, document isActive(); and lines 167-181, complete sessionStage() and csrfDefence(). Also complete the public constructor Javadoc in api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java lines 127-131, ensuring every public or protected API includes the required metadata and examples where applicable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java`:
- Around line 100-104: Update the Javadoc for the version-2 payload in
SealedSessionCookieCodec to state that unseal() rejects version-1 cookies at the
explicit version != FORMAT_VERSION guard before Cipher#doFinal, rather than
attributing rejection to GCM authentication; preserve the fail-closed and
re-login behavior description.
In `@CLAUDE.md`:
- Around line 136-141: Update the automated review comment workflow in the
“Handle the automated review comments from all three reviewers” section to
require running both “verify -Ppre-commit” and “verify” successfully with zero
errors and warnings before every bot-fix commit. Preserve the existing fix,
commit, push, reply, and resolve sequence, and reference the repository’s
established pre-commit guidance if appropriate.
In
`@doc/adr/0019-Reserved_BFF_paths_bypass_the_url-parameter_value_pipeline_and_the_cookie-header_length_relaxation_is_gateway-wide.adoc`:
- Around line 79-82: Update the stage policy description to scope the raised
maximum header-value length exclusively to inbound Cookie header values. Remove
Set-Cookie from the stated relaxation unless a separate response-header
validator is explicitly covered by the codec contract, and preserve the
unaffected bearer-only and server-mode behavior.
In `@doc/architecture.adoc`:
- Around line 186-195: The session-resolution documentation incorrectly says
cookie mode has “no store at all.” Update the cookie-mode description to say it
has no session store while acknowledging that login or pending-authorization
state may still use the existing PendingAuthorizationStore created by
BffRuntimeProducer; leave the session-state resolution behavior unchanged.
In `@doc/development/bff-cookie.adoc`:
- Line 77: Update the format layout documentation to explicitly identify
version(1) as a one-byte field and indicate that its current value is 2,
avoiding ambiguity with the format version guidance below. Keep the remaining
field sizes and layout unchanged.
In `@doc/user/bff-cookie.adoc`:
- Around line 57-58: Update the BFF cookie documentation to remove the claim
that the gateway ignores the oidc block unless a route’s effective auth is
require: session. Describe activation using the actual
BffRuntimeProducer.isBffMode() inputs: the global OIDC redirect_uri and
recognized session.mode, without asserting a route-auth prerequisite.
In `@doc/variants/03-bff-cookie.adoc`:
- Around line 195-211: Update the earlier portability claim in
doc/variants/03-bff-cookie.adoc to apply only to established-session requests,
replacing the broader “any request” wording while preserving the existing
explanation that login redirects and callbacks require the same instance.
- Around line 176-184: Update the cookie-session documentation to remove claims
about splitting oversized refresh or ID tokens into separate cookies, since the
SealedSessionCookieCodec/CookieSessionBinding path rejects over-budget seals.
Document reducing token size or switching to server mode as the supported
alternatives.
- Around line 154-155: Update the earlier Cookie Design key-management table to
match the optional encryption_key contract documented in the configuration
example: allow startup key generation when SHERIFF_SESSION_KEY is omitted, while
preserving the previous_key decrypt-only rotation behavior. Ensure the table no
longer states that a single environment-supplied shared key is required.
---
Outside diff comments:
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.java`:
- Around line 54-78: Enforce nonblank session nonces in both record contracts:
in SealedSessionPayload’s canonical constructor, reject null or blank
sessionNonce values; in SessionRecord, preserve Optional.empty() for server mode
but reject any present blank nonce. Apply validation consistently to prevent
blank nonce values from entering identity inputs.
In
`@api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java`:
- Around line 106-119: Complete the public API Javadocs: in
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.java
lines 106-119, document the constructor with `@since`, thread-safety guidance, and
usage examples; lines 137-147, add the required metadata to inert() and
stepUpCoordinator(); lines 157-160, document isActive(); and lines 167-181,
complete sessionStage() and csrfDefence(). Also complete the public constructor
Javadoc in
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.java
lines 127-131, ensuring every public or protected API includes the required
metadata and examples where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 955e2418-60c7-4599-899e-64ce1a12b8f6
📒 Files selected for processing (23)
CLAUDE.mdapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinator.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieKeyMaterialTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBindingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodecTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayloadTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinatorTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/session/InMemorySessionStoreTest.javadoc/README.adocdoc/adr/0018-BFF_session_mode_is_one_SessionBinding_seam_behind_a_fixed_reserved-path_and_CSRF_model.adocdoc/adr/0019-Reserved_BFF_paths_bypass_the_url-parameter_value_pipeline_and_the_cookie-header_length_relaxation_is_gateway-wide.adocdoc/architecture.adocdoc/development/README.adocdoc/development/bff-cookie.adocdoc/user/README.adocdoc/user/bff-cookie.adocdoc/variants/03-bff-cookie.adoc
The cookie-mode per-session nonce is minted from a static final SecureRandom on the login path. GraalVM initializes static state at build time by default, so the class initializer baked a seeded SecureRandom into the image heap and native-image failed with 'Detected an instance of Random/SplittableRandom class in the image heap'. Register the class with --initialize-at-run-time, matching the three sibling BFF classes that already hold a static SecureRandom for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EfShpgB9HbUSf9n6zhP9r
…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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-sheriff/src/main/resources/application.properties (1)
49-55: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake native-image runtime-initialization coverage authoritative.
The current
CookieSessionBindingentry is correct, but this remains a hand-maintained list that must mirror source declarations ofstatic final SecureRandom. A future BFF class can be omitted silently, causing native-image failures or build-time-captured randomness. Use a verified package-level rule where safe, or generate the option from the authoritative source set.As per path instructions, a hardcoded list that must mirror a set defined elsewhere must be derived at build or run time.
Also applies to: 63-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-sheriff/src/main/resources/application.properties` around lines 49 - 55, Replace the hand-maintained SecureRandom class list in the native-image runtime-initialization configuration with an authoritative generated option or a safe package-level rule covering every BFF class that declares static final SecureRandom. Ensure future matching classes are included automatically while preserving runtime initialization for CookieSessionBinding and the existing native-image option format.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api-sheriff/src/main/resources/application.properties`:
- Around line 49-55: Replace the hand-maintained SecureRandom class list in the
native-image runtime-initialization configuration with an authoritative
generated option or a safe package-level rule covering every BFF class that
declares static final SecureRandom. Ensure future matching classes are included
automatically while preserving runtime initialization for CookieSessionBinding
and the existing native-image option format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b70901a-39e7-46a6-b7cf-62db3528bff6
📒 Files selected for processing (2)
api-sheriff/src/main/resources/application.propertiesintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java
…xist The earlier version of step 8 claimed benchmark.yml is the only genuinely post-merge workflow. That was derived from grepping .github/workflows/ and finding no deploy-snapshot, but deploy-snapshot is a job of the reusable cuioss-organization/reusable-maven-build.yml that maven.yml delegates to, so it never appears in this repo's workflow files. maven.yml triggers on push to main, and deploy-snapshot is skipped on pull requests but runs on that push. It is therefore a real post-merge job — and unlike benchmark.yml it is NOT attached to the PR, so it has to be looked up by merge commit. Step 8 now covers both, and names the distinction that makes the second one easy to miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EfShpgB9HbUSf9n6zhP9r
Collaborator
Author
Triage dispositionsIn reply to comment_id:
|
…iant set Fixes six CodeRabbit findings on PR #125 where the Variant 3 documentation contradicted the shipped implementation: - architecture.adoc: cookie mode has no *session* store, but PendingAuthorizationStore.InMemory is assembled in both modes - development/bff-cookie.adoc: wire-format sizes now use the codec's own nB notation, so version(1B) no longer reads as format version 1 - user/bff-cookie.adoc: BFF activation is (recognised session.mode AND redirect_uri), never route effective auth - variants/03-bff-cookie.adoc: key-management row reconciled with the optional encryption_key contract - variants/03-bff-cookie.adoc: removed the non-existent cookie-splitting claim; over-budget seals fail - variants/03-bff-cookie.adoc: statelessness claim narrowed to established sessions, cross-referencing the login-affinity subsection
…it gates CodeRabbit finding e23e61 on PR #125: Git Workflow step 6 told the agent to fix-and-commit for a valid bot comment without naming the repository's own commit precondition, so the bot-fix path could read as exempt from the CRITICAL Pre-Commit Process gates. Adds a single cross-referencing clause; the canonical commands stay single-sourced in that section.
Two CodeRabbit findings on PR #125: - 997d43: the FORMAT_VERSION javadoc named GCM authentication as the v1-rejection mechanism. unseal() rejects a foreign version at an explicit guard before the Cipher is constructed, so decryption is never attempted; the associated-data binding is an additional property, not the gate. Javadoc-only correction — unseal()'s control flow and guard order are unchanged. - aae09b: both canonical constructors guarded sessionNonce with a null check only, so the empty string was accepted and would silently degrade the derived session identity back to the colliding pre-nonce shape. SealedSessionPayload now rejects null and blank; SessionRecord keeps the nonce Optional (absent stays the valid server-mode shape) and rejects only a present-but-blank value. Neither rejection message carries the nonce.
Sonar java:S6213 (MAJOR) — 'record' is a Java restricted identifier, so a method named record() trips the restricted-identifier naming rule. The helper was introduced by the SessionRecordNonceContract tests in 8483687. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013EfShpgB9HbUSf9n6zhP9r
github-merge-queue
Bot
removed this pull request from the merge queue due to a manual request
Jul 29, 2026
cuioss-oliver
added a commit
that referenced
this pull request
Jul 29, 2026
PR #125 landed ADR-0018 and ADR-0019 while this branch was queued, so the number this ADR was authored under is taken. The rebase was conflict-free because the filenames differ, which is exactly why the collision had to be caught by inspection rather than by git. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary
Completes BFF Variant 3 (cookie, stateless) ahead of the 0.1.0 cut: fixes a
cookie-mode session-identity collision defect, pays the documentation and ADR
debt left by PLAN-06/PLAN-07A, and lands two
CLAUDE.mdgit-workflowcorrections. 6 deliverables, 23 files, 6 commits on
feature/plan-07b-bff-cookie-docs-adrtargetingmain.BREAKING: the sealed-cookie wire format changes
(
SealedSessionPayload.FIELD_COUNT8 -> 9,SealedSessionCookieCodec.FORMAT_VERSION1 -> 2). Every existing cookie-modesession is invalidated on deploy and users must re-login. Per pre-1.0 rules
there is deliberately no legacy 8-field acceptance path in
decode().Changes
Security fix — unique cookie-mode session identity (deliverable 2)
sessionIdwasSHA-256(salt || loginEpochSecond || sub)truncated to 16bytes. Two logins by the same subject inside the same wall-clock second
produced the same
sessionId.TokenRefreshCoordinator's single-flightinFlightmap is keyed on that id, so the collision cross-wired the twosessions' token refreshes — one session's refresh could complete against the
other session's tokens.
Fix: a 32-byte
SecureRandomper-session nonce is minted once at login andcarried inside the sealed payload as a 9th field, included in the identity
digest, and re-encoded verbatim on every re-seal (
rotate()reusesprevious.sessionId(), so the nonce must stay stable across re-seals —TokenRefreshCoordinator's single-flight semantics depend on it).api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionPayload.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/CookieSessionBinding.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/session/SessionRecord.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/refresh/TokenRefreshCoordinator.javaSealedSessionPayloadTest.java,SealedSessionCookieCodecTest.java,CookieSessionBindingTest.java,TokenRefreshCoordinatorTest.java— addsthe missing test asserting two same-second logins for the same
subyielddistinct
sessionIds, plus a re-seal-stability test.Javadoc correction (deliverable 1 — originating-spec D5 already satisfied)
The originating spec's D5 ("re-source
UserInfoEndpointfrom the sealedcookie") had already shipped in PLAN-07A, so this deliverable resolved to
correcting the stale server-mode-only wording left in the BFF runtime
javadoc rather than any new implementation:
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.javaDocumentation debt (deliverables 3 and 4)
doc/variants/03-bff-cookie.adoc— corrects thefalse "required
session.encryption_key" claim and annotates the samplewith the condition under which the key is actually needed.
doc/user/bff-cookie.adocanddoc/development/bff-cookie.adoc, indexedfrom
doc/user/README.adoc,doc/development/README.adoc, anddoc/README.adoc. Layer (b) documents the forced-re-login consequence ofthe
FORMAT_VERSIONbump and thePendingAuthorizationStorecross-instance login limitation ("stateless after login").
New ADRs (deliverable 5)
doc/adr/0018-BFF_session_mode_is_one_SessionBinding_seam_behind_a_fixed_reserved-path_and_CSRF_model.adoc— the reserved-path model, the fixed CSRF model, the session-mode
server-vs-cookie split, the cookie key-material two-mode decision, and the
session-identity nonce/
FORMAT_VERSIONdecision from deliverable 2.doc/adr/0019-Reserved_BFF_paths_bypass_the_url-parameter_value_pipeline_and_the_cookie-header_length_relaxation_is_gateway-wide.adoc— the two security-sensitive relaxations previously documented only in
javadoc (
BasicChecksStage.reservedPathMatcherskipping the inboundurl-parameter pipeline; the cookie-header length relaxation being
gateway-wide rather than per-anchor).
doc/architecture.adocupdated to reference both ADRs.CLAUDE.md corrections (deliverable 6)
.github/workflows/pr-agent.yml) instead of the retired Gemini reference.re-checks the PR's real post-merge workflow (
benchmark.yml,pull_request: types: [closed]) and asserts zero unresolved reviewthreads via
ci pr comments --pr-number N --unresolved-only.Test Plan
python3 .plan/execute-script.py plan-marshall:build-maven:maven run --command-args "verify -Ppre-commit"python3 .plan/execute-script.py plan-marshall:build-maven:maven run --command-args "verify"SealedSessionPayloadTest/SealedSessionCookieCodecTestcases(same-second collision, re-seal stability) pass
Related Issues
No linked issue for this plan.
Generated by plan-finalize skill
Summary by CodeRabbit
New Features
Cookie/Set-Cookieheader values.Bug Fixes
Documentation