feat(demo-client): add demo SPA module with Playwright E2E suite - #141
Merged
Conversation
…docs Doc-first deliverable 1 of the demo SPA + Playwright E2E plan: the design and integration-sample documentation that governs every later implementation deliverable. - doc/development/demo-client.adoc (contributor layer): why the demo exists, the module layout, why the SPA is served by the gateway rather than a side-car static server, the opt-in skipPlaywrightTests / e2e-demo build mechanism, how to run the suite locally, the Chromium --host-resolver-rules mapping the pinned realm frontendUrl forces, the four TokenSheriff divergences not copied, and the production-image exclusion. Records two standing prohibitions verbatim: no Accept-based expectation on the info endpoint (the 401-vs-302 split is PATH-based), and that a Compose profile cannot express the trimmed bring-up. - doc/user/demo-client.adoc (integrator layer): the browser-facing contract, the four identity-disclosure states the claims parameter selects, the same-origin returnUrl rule and its silent-fallback behaviour, the Cache-Control: no-store guarantee, the asset-anchor configuration, and why oidc.logout.final_redirect must name a concrete file rather than /. - demo-client/README.adoc: the module's one-screen quickstart, pointing at both layers rather than restating them. - Both doc index tables enumerate the new page. Aligns with ADR-0014 (asset serving as a second terminal action) without restating it. doc/plan/11-demo-client-e2e.adoc is deliberately not created. Co-Authored-By: Claude <noreply@anthropic.com>
…profile
Deliverable 2: the demo-client Maven module shell and its npm toolchain, with
Playwright off by default so the module contributes no work to the default lane.
- demo-client/pom.xml: packaging pom, parent api-sheriff-parent, empty
dependencies, every Java plugin skipped by property, and pinned node/npm plus
frontend-maven-plugin versions. EVERY frontend-maven-plugin and
exec-maven-plugin execution lives inside the e2e-demo profile — a deliberate
divergence from the reference module, which wires them unconditionally — so a
default reactor build has no executions to skip and downloads no Node.
skipPlaywrightTests defaults to true; the profile flips it and binds the
lifecycle: stop-dev-environment @ pre-clean, install-node-and-npm + npm
install @ initialize, npm run lint:strict @ verify, start-dev-environment @
pre-integration-test, npx --yes playwright test @ integration-test,
stop-dev-environment @ post-integration-test. Base URLs and the Keycloak host
URL come from POM properties, so no port number is restated in JavaScript.
installDirectory is pinned to ${project.build.directory} so the toolchain
lands under the already-ignored target/ tree.
- demo-client/package.json: private, type module, engines.node >= 20, the
lint/lint:strict/format/format:check scripts, and exactly the approved
devDependency set — @axe-core/playwright deliberately absent.
- demo-client/eslint.config.js: flat config with browser globals for the SPA
sources and node globals for the suite sources, eslint-plugin-security
enabled and eslint-config-prettier last.
skipPublishing is set alongside maven.deploy.skip because the latter is not what
suppresses publication under cui-java-parent — the same reasoning already
recorded in integration-tests/pom.xml.
Co-Authored-By: Claude <noreply@anthropic.com>
Deliverable 3: add <module>demo-client</module> to the root reactor, placed last so the existing module ordering is untouched. This is its own deliverable rather than a line folded into the module skeleton because the failure mode of a module existing on disk but never entering the reactor is silent: everything compiles and nothing runs. Verified: 'verify -Ppre-commit' passes with the module in the reactor, and 'validate -pl demo-client' resolves the project — which it could not do if the module were absent from <modules>. The module contributes no work to the default lane, since every npm-touching execution lives inside its e2e-demo profile. Refreshing the git-tracked architecture inventory so demo-client becomes a nameable module is deliberately not folded in here; it is its own deliverable. Co-Authored-By: Claude <noreply@anthropic.com>
Deliverable 4: the plain HTML/CSS/JS single-page app that renders the BFF
contract. No framework, no bundler, no build step — the served files are the
authored files, so the compose bind-mount needs no dist/ and no build ordering.
- index.html: one page — status region, identity panel, claim-view selector and
the two navigation buttons. app.css and app.js load as plain relative assets;
no inline script and no inline style.
- app.js: the whole client in one readable file. It fetches /auth/userinfo with
credentials same-origin and branches on status — 200 renders the identity
panel, 401 renders the RFC 9457 problem and the login affordance. The fetch
carries redirect: 'error', encoding as a runtime assertion the contract that
this endpoint is an XHR probe and never redirects. All four disclosure states
are reachable from the UI: curated default view, explicit selection,
claims=*, and a claim outside the operator allowlist whose 403 is a visible,
demonstrated state. Login and Logout are top-level navigations via
window.location.assign, never fetches. Session metadata (expires_at,
auth_time, acr) renders from the session member. Nothing is logged, no
storage is written, no cookie is read, and every value is rendered through
textContent — there is no HTML-building sink in the file.
- app.css: minimal readable styling, no component library, no framework, no
preprocessor.
- landing.html: the post-logout landing target — a heading, one sentence and a
link back to index.html. Reuses app.css and loads NO JavaScript. Addressed
explicitly as /assets/demo/landing.html, because the directory asset source
performs no directory-index resolution.
The HYPOTHESIS holds: no change under api-sheriff/src/main/java/** was needed.
The three reserved-endpoint sources were read as the authority for the status
codes, the claims semantics, the 403 allowlist rejection and the no-store
guarantee, and none required modification.
Two changes outside the SPA sources, both forced by actually RUNNING the lint
gate rather than assuming it:
- eslint-plugin-security is raised from ^3.0.0 to ^4.0.1. Version 3 crashes
under ESLint 10 ('TypeError: context.getSourceCode is not a function' in
detect-unsafe-regex), so the gate could never have passed as first written.
This is a version bump inside the already-approved dependency set, not a new
dependency.
- package-lock.json is committed so the e2e-demo lane installs reproducibly.
Verified: 'npm run lint:strict' passes with zero errors and zero warnings over
the SPA sources.
Co-Authored-By: Claude <noreply@anthropic.com>
Deliverable 5: declare the demo asset route and mount the SPA, so the demo is served BY the gateway and is therefore same-origin with the reserved /auth paths — the property that makes the browser-facing contract observable at all. - endpoints/assets.yaml: add a THIRD route, assets-demo (/assets/demo -> source: directory, /app/demo), under the EXISTING assets-public anchor. This file is mounted into all six gateway instances, four of which overlay only gateway.yaml, so a new anchor would have to be declared in all four documents or those overlays would boot-fail on a route resolving to an undeclared anchor. Reusing assets-public means zero new anchors and zero anchor-lockstep divergence. - docker-compose.yml: two read-only bind mounts of demo-client/src/main/resources/spa onto /app/demo, one on api-sheriff and one on api-sheriff-cookie. No service, port, environment entry or depends_on edge is touched, and no profiles: key is introduced. - sheriff-config/gateway.yaml and sheriff-config-cookie/gateway.yaml: one line each — oidc.logout.final_redirect from / to /assets/demo/landing.html — so RP-initiated logout lands on a real public page instead of a deny-by-default 404. The mtls and ws-admission overlays keep final_redirect: / and are untouched; final_redirect is a per-instance value of the same class as redirect_uri and post_logout_redirect_uri, which already differ across all four documents. Read-only audit, performed rather than assumed: both untouched overlays declare the identical assets-public anchor (type: asset, access: public, GET/HEAD), so the new route resolves everywhere; and BffLogoutIT asserts only that the Location header is PRESENT, never its value, so no existing test asserts the literal / and no test file needed updating. Verified against the live stack: - all five gateway instances report readiness 200 with the shared route added; - the SPA serves 200 text/html on both 10443 and 10445, app.js serves text/javascript with X-Content-Type-Options: nosniff; - /assets/demo/landing.html serves 200 while /assets/demo/ returns 404, since the directory source performs no directory-index resolution; - the pre-existing /assets/static route still serves 200; - the full existing IT suite is green: 90 completed, 0 failures, 0 errors, 0 skipped, 0 flakes — proving the additive route and the two mounts are behaviour-neutral for everything except the new /assets/demo surface. Co-Authored-By: Claude <noreply@anthropic.com>
Deliverable 6: playwright.config.js, the shared fixtures and login helper, and the four spec files. Two projects, session-server (10443) and session-cookie (10445), run the SAME specs and differ ONLY in baseURL, so "the two session modes are browser-observably identical" is an executable assertion. - playwright.config.js: ESM, serial (workers 1, retries 0) because the suite drives one shared stack with real server-side sessions. No HTML reporter, so no report server is spawned. Chromium carries --host-resolver-rules=MAP keycloak:8443 -> the published host address, derived from KEYCLOAK_HOST_URL rather than a literal port; the realm pins frontendUrl https://keycloak:8443, so without it a real browser dead-ends at DNS. Every URL and credential comes from the environment the POM supplies. - utils/constants.js: reserved paths, SPA entry points and the operator allowlist. No port number appears in JavaScript. - utils/keycloak-login.js: drives the real Keycloak form. - fixtures/test-fixtures.js: a page already on the SPA, and a probe that issues same-origin fetches FROM INSIDE the page context so the HttpOnly session cookie is attached exactly as the SPA attaches it. Driving the same request from APIRequestContext would use a separate cookie jar and prove something weaker than what a browser observes. - The four specs assert the anonymous contract, login and identity, the claim views and the allowlist cap, and the logout round-trip. Every assertion is PATH-based; no spec sends an Accept header to the info endpoint or expects it to vary on one. Screenshots are captured on the SUCCESS path as documentation artifacts. lint:strict passes with zero errors and zero warnings. The suite was RUN, not merely linted, and it found a real defect — see the Q-Gate finding recorded for this plan (b37339). 6 of 22 specs pass in both projects (the whole anonymous contract, and the landing page as public static content). The 16 login-dependent specs fail identically in BOTH session modes because the browser login dead-ends at /auth/callback: the gateway requests response_mode=form_post, so Keycloak auto-submits a CROSS-SITE POST to the callback, and the pending-auth binding cookie is minted SameSite=Lax — which a browser does not send on a cross-site POST. The landed Java IT suite cannot observe this, because its helper replays cookies from a Map it manages itself and SameSite is a browser policy RestAssured never enforces. That is a gateway finding, not a knob: fixing it needs a production change, and deliverable 4 explicitly instructed that such a case be REPORTED rather than patched here. No control was weakened and no assertion softened to make the suite pass. Co-Authored-By: Claude <noreply@anthropic.com>
The demo suite could not complete a login. Three independent defects were in the way; this lands all three fixes plus the demo bring-up scripts, the opt-in CI job and the documentation that describes them. 1. Authorization response mode. The BFF now requests response_mode=query, so the OIDC callback arrives as a top-level GET and the SameSite=Lax __Host-sheriff-binding cookie is actually sent on it. The previous form_post callback was a cross-site POST, on which the browser withheld the binding cookie and every login leg failed. The callback re-parses the genuinely raw, never map-collapsed query string, so the BFF-13 duplicate-parameter defence survives the mode switch. 2. Login return-URL wire name. GatewayEdgeRoute read the return target from a return_to query parameter while the endpoint's own documented contract, the demo SPA, the Playwright helper and every internal identifier already used returnUrl. The constant and its value are renamed together (RETURN_TO_PARAM/"return_to" -> RETURN_URL_PARAM/"returnUrl"); no fallback alias and no deprecation shim, per the pre-1.0 rules. A matched positive/negative control in GatewayEdgeRouteBffWiringTest drives a live Vert.x edge and pins the wire name, so a revert to return_to fails the build instead of silently degrading to the default return URL. 3. Stale native runner. build-native-if-needed.sh only checked whether a runner EXISTED, so any run after a source change silently reused a stale binary and made a landed fix read as "the fix did not work". It now compares the runner's mtime against api-sheriff/src, api-sheriff/pom.xml and the reactor pom and rebuilds when any input is newer; the skip path states why it considers the runner fresh. Its header documents the caller obligation that a native rebuild implies an image rebuild, since compose up alone reuses the stale image. Also: the e2e-demo profile now sets skipTests=false. The module-level skipTests=true (which keeps inherited surefire off a module with no Java) was also suppressing frontend-maven-plugin's integration-test-phase Playwright execution, so the profile reported BUILD SUCCESS having run ZERO specs and the CI job was vacuously green. The override is scoped to the opt-in profile, so a default reactor build of demo-client stays a genuine no-op. Verified: Playwright 22/22 green across both projects (session-server 11, session-cookie 11) with the full screenshot set produced for each and lint:strict clean; Java integration-test suite 90/90 green; quality gate and full verify both green. Co-Authored-By: Claude <noreply@anthropic.com>
…t-e2e Remove a committed OpenRewrite marker and narrow the reflective-walk catch in BffRuntimeProducerTest, drop the caller-less lint script, inline the realm credentials that no producer supplies, drop a subsumed CSS selector, and correct a comment that restated the wrong command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWrjv4rz7RTiKFa1gabqfk
…ient-e2e Replace 'npm install' with 'npm ci' so the committed lock file is authoritative and the install fails closed on drift, and replace the npx auto-confirm execution with the npm goal running 'run test' so the locked Playwright binary is used instead of a potentially unpinned registry fetch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWrjv4rz7RTiKFa1gabqfk
![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, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 37 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 (14)
📝 WalkthroughWalkthroughThe change switches BFF authorization callbacks from ChangesBFF query-mode flow
Estimated code review effort: 5 (Critical) | ~120 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 1ed98c3)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/pending/BindingCookieCodecTest.java (1)
81-88: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert
SameSite=Laxon the clearing header.Line 87 checks
SameSite=Laxonly onsetCookie. A future change can remove the attribute fromtoClearingSetCookieHeader()and this regression test will still pass.Proposed test update
() -> assertTrue(setCookie.contains("; SameSite=Lax"), - "Lax is correct AND sufficient because the callback is a top-level GET: " + setCookie)); + "Lax is correct AND sufficient because the callback is a top-level GET: " + setCookie), + () -> assertTrue(clearing.contains("; SameSite=Lax"), + "the clearing cookie must retain the same SameSite attribute: " + clearing));demo-client/pom.xml (1)
192-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider running
lint:strictbefore the suite.
npm-lint-strictbinds toverify, which runs afterintegration-test. A lint failure therefore surfaces only after a full browser run and a container bring-up. Binding it toprocess-resourcesortestgives the same signal in seconds.♻️ Proposed phase change
<execution> <id>npm-lint-strict</id> - <phase>verify</phase> + <phase>process-resources</phase> <goals> <goal>npm</goal> </goals>
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e11cee6-f64a-42aa-9388-493935db93ef
⛔ Files ignored due to path filters (1)
demo-client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/demo-client-e2e.yml.gitignore.plan/project-architecture/_project.json.plan/project-architecture/demo-client-maven/enriched.json.plan/project-architecture/demo-client-npm/enriched.jsonapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/login/LoginFlow.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/login/QueryResponseModeAuthorizationRequestBuilder.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/pending/BindingCookieCodec.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpoint.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/runtime/BffRuntime.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/EdgeHardeningOptions.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRoute.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.javaapi-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducer.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/login/QueryResponseModeAuthorizationRequestBuilderTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/pending/BindingCookieCodecTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/bff/reserved/CallbackEndpointTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.javademo-client/README.adocdemo-client/eslint.config.jsdemo-client/fixtures/test-fixtures.jsdemo-client/package.jsondemo-client/playwright.config.jsdemo-client/pom.xmldemo-client/scripts/start-dev-environment.shdemo-client/scripts/stop-dev-environment.shdemo-client/src/main/resources/spa/app.cssdemo-client/src/main/resources/spa/app.jsdemo-client/src/main/resources/spa/index.htmldemo-client/src/main/resources/spa/landing.htmldemo-client/tests/01-anonymous-contract.spec.jsdemo-client/tests/02-login-and-identity.spec.jsdemo-client/tests/03-claim-views-and-allowlist.spec.jsdemo-client/tests/04-logout.spec.jsdemo-client/utils/constants.jsdemo-client/utils/keycloak-login.jsdoc/LogMessages.adocdoc/architecture.adocdoc/configuration.adocdoc/development/README.adocdoc/development/demo-client.adocdoc/user/README.adocdoc/user/demo-client.adocintegration-tests/docker-compose.ymlintegration-tests/scripts/build-native-if-needed.shintegration-tests/src/main/docker/sheriff-config-cookie/gateway.yamlintegration-tests/src/main/docker/sheriff-config/endpoints/assets.yamlintegration-tests/src/main/docker/sheriff-config/gateway.yamlintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.javaintegration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffLoginInitiationIT.javapom.xml
Collaborator
Author
Triage dispositionsIn reply to comment_id:
|
…ed pom The e2e-demo profile was hardened late in this plan: npm install became npm ci, the npx invocation became npm run test, the lint moved from verify to process-resources, and an explicit browser install was added. The lifecycle table, the local-iteration snippets and the module's architecture metadata still described the pre-hardening commands, so they pointed contributors at the exact npx path the hardening removed. Also records what the readiness derivation now covers (no port literal remains in the bring-up script) and why a failing local run deliberately leaves the containers up while CI tears them down.
…sitively on the clearing cookie java:S7467 at BffRuntimeProducerTest: the multi-catch bound a variable the body never reads, so it becomes the unnamed variable (JEP 456, final since Java 22). The comment explaining why the multi-catch is deliberately narrow is preserved verbatim - it carries reasoning the unnamed variable does not. BindingCookieCodecTest asserted SameSite=Lax positively on the set header but only asserted the ABSENCE of SameSite=None on the clearing header. Absent is not None, so dropping the attribute from toClearingSetCookieHeader() entirely would have passed every assertion. Verified against BindingCookieCodec that Lax is what it emits, then fenced it positively.
…eaving the page on Loading probeUserInfo awaited fetch with no rejection handling and all three call sites discarded the rejection with void, so a rejected probe left the page permanently showing 'Loading...' with the only trace an unhandled rejection in the console. Two causes reach that path, and the second is the point: a transport/TLS failure, and a REDIRECT - probeUserInfo sets redirect: 'error' precisely because the info endpoint must never redirect, so the contract violation the probe exists to assert was the one thing it swallowed. renderTransportFailure resets every panel render() owns, so no stale identity survives, and hides both navigation buttons because a failed probe discloses nothing about whether a session exists. Covered by a new spec that aborts the /auth/userinfo request at the transport layer; it passes under both the session-server and session-cookie projects.
…ly, and derive every port Container leak on a failing run: frontend-maven-plugin's npm goal aborts the build immediately on a non-zero exit, so Maven never reaches post-integration-test and its teardown is skipped. Reproduced empirically - a deliberately failed suite left keycloak, api-sheriff and api-sheriff-cookie up holding 10443/10445/1443/1090. The CI job runs verify, not clean verify, so the pre-clean teardown never covered it either. Fixed with an if: always() teardown step; the local path deliberately keeps the stack up for diagnosis, which is recorded as a decision at the pom execution and documented for developers. Browser install: nothing installed one. The suite only ever worked because the playwright package's npm postinstall happens to download a browser. demo-client-e2e.yml has no pull_request trigger and has therefore never run, so its first execution is a fresh runner. npm run install-browser makes it a declared step; proven on a genuinely cold cache (184 MiB downloaded into a path that did not exist). The plain install is used rather than the with-deps variant because this execution is on the local developer path too, where root is not available. Lint moved from verify to process-resources so a lint failure aborts before the container bring-up, and therefore cannot leak containers either. Port derivation: the Keycloak health probe pinned localhost:1090 and the closing banner restated 10443, 10445 and 1443, in a script whose own header declares that ports are derived from the resolved Compose model and never restated. keycloak now carries the same management-scheme label the gateways do, so one derivation covers all three services and also yields the public ports the banner prints. The probe also gains -f: without it curl exits 0 on the 503 Keycloak answers while starting, so the wait cleared on port-accept rather than on readiness - the very race the gate exists to remove. The banner now names npm run test, not the npx path this plan's hardening removed.
Collaborator
Author
|
/review |
… the duplicate 0025 ADR-0018 gains an Amendment recording that the fixed SameSite=Lax policy of its own decision (ii) determines the OIDC authorization-response transport: the cookie side cannot move, so the transport side must. Records the chosen response_mode=query path, the accepted code-in-URL tradeoff, four rejected alternatives, and four mitigations stated as standing obligations. Two ADRs both carried the number 0025 after a cross-branch collision. The inbound-validation record merged later and had zero inbound references, so it moves to 0026; all ~25 existing ADR-0025 references point at the TLS record and stay valid. Zero dangling cross-references after the sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWrjv4rz7RTiKFa1gabqfk
cuioss-oliver
added a commit
that referenced
this pull request
Aug 2, 2026
Rebasing onto main brought in PR #141 (demo-client SPA), which added a GatewayConfig.builder().oidc(Optional.of(fullOidc())) call site in GatewayEdgeRouteBffWiringTest. Git auto-merged that hunk without conflict because neither side edited the same lines, but the site is semantically incompatible with this plan's conversion: oidc() now takes a @nullable OidcConfig, not an Optional. Unwrapped to oidc(fullOidc()). The remaining Optional uses in this file are computed returns (registry.match, locationOptional, BoundSession) and stay per the storage-vs-computation rule. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 3, 2026
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.
Intent
Add a deliberately simple JavaScript/SPA demo client that (a) proves the BFF works end to end
against a real Keycloak IdP, and (b) ships as the integration sample a downstream frontend team
copies. Exercised by a Playwright suite with functional assertions and screenshots, run against
both session modes (
serverandcookie) from one parameterized suite.Implements PLAN-11 of the
api-sheriff-roadmapepic. Doc-first: the design and integratordocumentation were authored before the implementation and govern it.
Scope Deviation Accepted
The demo found a real gateway defect, and fixing it required a production change this plan's
spec explicitly said to report rather than patch. The operator overrode that boundary deliberately.
The defect. The engine drove the authorization request with
response_mode=form_post, soKeycloak auto-submits a cross-site POST to
/auth/callback. The pending-auth binding cookie__Host-sheriff-bindingis mintedSameSite=Lax, and Lax cookies are not sent on a cross-sitePOST — only on top-level GET navigations. A real browser therefore dropped the binding cookie on
the callback leg and the flow dead-ended, identically in both session modes. 16 of 22 Playwright
specs failed on it.
The landed Java IT suite structurally cannot catch this:
BffKeycloakLoginFlowreplays cookiesfrom a
Mapit manages itself, andSameSiteis a browser policy RestAssured never enforces.Closing exactly that gap is why this demo exists.
The fix.
response_mode=query, so the callback is a top-level GET andSameSite=Laxapplies.The accepted tradeoff. The authorization code now appears in the URL query string
(referrer / access-log / browser-history exposure) — the standard reason
form_postis preferred.This was accepted knowingly. The alternative,
SameSite=Noneon the binding cookie, wasrejected because it weakens the exact binding control the cookie exists to provide.
Mitigations verified against source, not assumed (finalize security audit):
code_challenge/code_challenge_methodare emittedunconditionally and a non-S256 provider throws. The verifier lives server-side in
PendingAuthorizationStore; the browser holds only the record id, so a leaked code isunredeemable.
consumeperforms an atomic take-and-remove undersynchronized, with a 5-minute TTL.CallbackParameters.toString()redactscodeandstate.Residual exposure, reported not silently accepted: Playwright failure traces publish the
code-bearing callback URL as a 30-day CI artifact (finding
1dee9a, informational). The code issingle-use and PKCE-bound, so it is not redeemable — but this exposure did not exist under
form_post.Also fixed:
return_tovsreturnUrl(pre-existing)LoginInitiationEndpoint's javadoc documented the login parameter asreturnUrl, butGatewayEdgeRoutereadreturn_to— a naming inconsistency present onmainbefore this plan,with
return_todocumented nowhere. The wire parameter is nowreturnUrl, aligning theimplementation with its own contract. Pre-1.0, so no alias and no deprecation shim.
Five test files that use
return_tomerely as an arbitrary query-parameter name to exercise theURL-parameter security pipeline were deliberately left untouched.
Security posture
The finalize security audit verified all of the following against source:
carry
__Host-/Path=//Secure/HttpOnly/SameSite=Lax.SameSite=Noneappearsnowhere in the tree.
code/state(the CVE-2026-9689 class), asserted at both the endpoint and the
BffRuntime.dispatchseam.CALLBACKfromneedsReservedBodyReadended a pre-authentication body read any unauthenticated POST couldtrigger. Back-channel logout keeps its path and ceiling unchanged; a new test pins the removal.
demo-clientispackaging: pom,excluded from the native image and the deploy, and off
api-sheriff's-ampath. Theassets-demoroute and the SPA mount exist only underintegration-tests/, never as a shippeddefault. The SPA uses
textContentexclusively (zero HTML-building sinks), reads no cookie andwrites no storage.
CsrfDefenceis untouched.npm installreplaced withnpm ciso the committed lock file isauthoritative and the install fails closed on drift; the npx auto-confirm execution replaced with
the
npmgoal runningrun testso the locked Playwright binary is used.Verification
[session-server]+ 11[session-cookie]), with the full five-state screenshot set per project.verify -Ppre-commit) and full verify green (134 + 1501 + 38 tests).--max-warnings 0clean.Two vacuous-green traps were found and closed rather than accepted:
<skipTests>true</skipTests>also suppressed the frontend-maven-plugin execution, so
verify -Pe2e-demoreturned BUILD SUCCESSin 30s having run zero specs (which also made the new CI job vacuously green); and
build-native-if-needed.shexistence-checked the runner instead of checking staleness, so a runafter a production-source change silently used a stale native binary.
Deliverables
doc/development/) and integrator guide (doc/user/), plus the module READMEdemo-client/module skeleton with an opt-ine2e-demoPlaywright profilepom.xml<modules>registrationtype: asset,access: public) — first consumer ofPLAN-10's asset serving outside its own tests
node_modules/gitignore entryCompose reuse: the demo brings up exactly three containers via explicit service selection
(
keycloak,api-sheriff,api-sheriff-cookie) with--no-deps. A Compose profile cannotexpress this — a service with no
profiles:key always starts, so profiles only add to the defaultset; trimming via profiles would have required tagging the other nine services and would have broken
the existing IT suite's default bring-up and its readiness discovery.
docker-compose.ymlisstructurally unchanged.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YWrjv4rz7RTiKFa1gabqfk
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
returnUrl; legacy values fall back safely.