fix(review): address batch MINOR/def-in-depth follow-ups (bookshelf-syyzy) #1437
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-syyzy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Addresses 5 non-blocking review MINOR/def-in-depth items deferred from the tm38/bxtat/bz643.7/ifwpa/fip0 merge batch:
isTrustedEndSessionEndpointnow ALSO requiresissuerURL.Scheme == "https"(was already forcing https on the end_session_endpoint side; now pinned on both sides).setOIDCIDTokenCookieSameSite changed from Lax to Strict — confirmed both call sites that read this cookie (handler.go login-page + logout handlers) are same-site initiated requests; no cross-site flow depends on Lax.Its that bundled twoExpects each into separate one-Expect-per-It blocks sharingBeforeEach/JustBeforeEach.e2e/browser/journey_reader_test.go— the two dropped pure-selector annotation Its are actually covered by the retained "opens the annotations panel when the Highlights button is clicked" It (real click + poll), not Journey-11/e2e-api as previously stated.oidc_settings_test.goIt title "clamps a below-minimum value up to the minimum" to "treats a negative value as unset rather than clamping it to the minimum" — the body has no minimum-clamp path.internal/bookdrop/service.go's inlinealreadyKnown, existsErr :=into the function's topvar (...)block per project convention.Test plan
go build ./...— cleanmake test— all packages greengo build -tags e2e ./e2e/...— compilesmake lint— 0 issuesmake e2e-policy-check/make test-policy-check— OK./scripts/check-coverage.sh— zero uncovered statement blocksinternal/users/oidc_service_test.go) covering the new issuer-scheme-must-be-https branch.Closes bead bookshelf-syyzy on merge.
- OIDC end-session: pin issuer scheme to https in isTrustedEndSessionEndpoint, not just the end_session_endpoint side (defense in depth on the RP-initiated logout open-redirect check). - OIDC ID-token cookie: SameSite Lax -> Strict — it is only ever read from same-site logout requests, never a cross-site navigation. - Split two OIDC-callback test Its that bundled two Expects each into separate one-Expect-per-It blocks sharing BeforeEach/JustBeforeEach. - Correct the dropped-annotation-Its justification comment in journey_reader_test.go — real coverage is via the "opens the annotations panel when clicked" It, not Journey-11/e2e-api. - Rename a misleading oidc_settings_test.go It title ("clamps a below-minimum value") to describe what it actually asserts (negative -> treated as unset). - Hoist bookdrop/service.go's inline alreadyKnown/existsErr := into the function's top var (...) block per project convention. Closes bead bookshelf-syyzy on merge.[MINOR] internal/users/oidc_handler_test.go:294-297 — one remaining multi-Expect It not split
The PR's stated goal (per bookshelf-syyzy item #2 / #1392 bxtat) is splitting multi-Expect
OIDC-callback Its into one-Expect-per-It, and it does split the two Context-nested cases at
lines 306-317. But the top-level "redirects to home on success when no next cookie is present"
It (lines 294-297) still bundles two Expects (
w.CodeandLocationheader) in one It. Notintroduced by this PR, but since this PR is specifically doing that split elsewhere in the same
file/Describe, it's an easy miss to close out completely rather than leave inconsistent.
Fix: split into two Its mirroring the pattern already applied below (e.g. "redirects with
StatusFound" / "redirects to home").
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Review — PR #1437 (bookshelf-syyzy)
Scope: OIDC end-session-endpoint https-scheme hardening + id-token cookie SameSite tightening. Reviewed full diff (
git diff origin/main...origin/bd-bookshelf-syyzy).Analysis
isTrustedEndSessionEndpointhttps requirement (internal/users/oidc_service.go:1384-1390)issuerURL.Scheme != "https"check is symmetric with the pre-existingendSessionURL.Scheme != "https"check a few lines above — both legs of the comparison (the redirect target and the trust anchor) are now scheme-pinned.url.Parselowercases the scheme on parse (verified:url.Parse("HTTPS://Example.COM/path")→Scheme="https"), so there's no casing bypass.https://evil.com@issuer.example/) don't bypass this:net/urlcorrectly separatesUserfromHost, andHostname()returns only the host component, so an attacker-controlled userinfo segment does not influence theEqualFoldhost comparison.cfg.Issuer(admin-configured, not attacker-influenceable) rather thanmeta.Issuer(from the fetched discovery document) — correct choice, since the discovery response itself is the untrusted input being defended against.false→ falls back to local-only logout, never silently trusts.internal/settings/oidc_settings.goexplicitly permitshttp://issuers for loopback addresses (dev/test IdPs). With this change, a loopback-http-configured instance will now always fail the trust check and fall back to local logout — RP-initiated logout becomes permanently unavailable for that config. This looks like an intentional trade-off (the code comment says "Require https + same host"), not a bug, but worth confirming it's understood as a deliberate scope-reduction rather than an oversight.SameSite Lax → Strict on
bookshelf_oidc_idtoken(internal/users/oidc_handler.go:127-149)logoutHandler(internal/users/handler.go:325), reached viaPOST /logout. Cross-site POST form/fetch submissions were never covered by SameSite=Lax's top-level-GET-navigation exception in the first place, so this cookie was never attachable cross-site in practice under Lax either — the Strict flip closes the (already narrow) Lax carve-out with no observed regression to the callback-write / logout-read flow.HttpOnly+ conditionallySecure(unchanged) — no XSS-exfil regression.Multi-user scoping: untouched by this diff — no per-user data paths touched.
Other diff contents (e2e/browser/journey_reader_test.go comment update, internal/bookdrop/service.go var-hoist refactor, internal/settings + internal/users test description/split-It changes): non-security, cosmetic/test-hygiene only. The bookdrop refactor is a pure
var-block hoist per project style (project-conventions.md), no behavior change (variable still initialized in the same order at the same call site). The twoItsplits in oidc_handler_test.go move a secondExpectinto its ownItper the one-Expect-per-It test convention — no coverage change.Deferred / out of scope
The dial-time SSRF (validating the discovery endpoint / end_session_endpoint host before actually making the outbound HTTP request, e.g. rejecting private/loopback IPs on the dial, not just the trust-comparison) and JWKS login-path symmetry are tracked separately under bookshelf-qapga / bookshelf-r5dix per the dispatch note. This diff does not introduce or worsen that class of issue — it only adds a comparison-side scheme check, no new outbound dial.
Findings
No blockers, majors, or minors found. The change correctly hardens the trust check with no bypass identified and no regression to the SameSite-Strict-affected flow.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor