feat(users): OIDC RP-initiated single logout [shot:settings] (bookshelf-tm38.9) #1398
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-tm38.9"
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?
Summary
On local
/logout, redirect the browser to the OIDC provider'send_session_endpoint(OpenID Connect RP-Initiated Logout 1.0) so the IdP session ends too, when:
cookie set at OIDC callback, cleared on local login and on logout),
off — not all IdPs support it and some operators don't want the extra redirect
hop), and
end_session_endpoint.post_logout_redirect_uriis derived from the trusted request host (never fromuser input, mirroring
oidcRedirectURL) so it is same-origin by construction.Falls back to the existing local-only
/loginredirect when any condition aboveisn't met, or when discovery fails transiently — a user's logout is never
blocked by an IdP hiccup.
Adds the admin toggle to the OIDC settings page (canonical
.provider-settings-*/
.form-hintcomponents, no inlinestyle=) and updates the OIDC docs pagewith a Single Logout section.
Test plan
internal/users: unit tests forOIDCLogoutRedirectURL,logoutHandler(with/without OIDC session, JSON client, discovery failure), OIDC callback
sets/omits the ID-token cookie, local login clears a stale one — 100%
coverage on all new/changed functions.
internal/settings: unit tests for the newRPInitiatedLogoutsettinground-tripping through save/load/
GetOIDCConfig— 100% coverage.static/js: Vitest coverage for the newrpInitiatedLogoutcontrollertarget/payload field — 100% coverage.
e2e/browser: extended the existingJourney: SettingsOIDC spec toassert the toggle renders, gets saved, and persists across reload — ran
locally (
ginkgo --focus "Journey: Settings"), 14/14 passed.make lint/make test/go build ./...all green locally.docs/content/docs/administering/oidc.mdwith a SingleLogout section (end-user/admin framing).
Closes bead bookshelf-tm38.9 on merge.
On local /logout, redirect the browser to the OIDC provider's end_session_endpoint (OpenID Connect RP-Initiated Logout 1.0) so the IdP session ends too, when: - the user's session came from OIDC (detected via a short-lived HttpOnly ID-token cookie set at OIDC callback, cleared on local login and on logout), - the new "RP-initiated logout" admin toggle in Settings → OIDC is on (default off — not all IdPs support it and some operators don't want the extra redirect hop), and - the provider's discovery document exposes an end_session_endpoint. post_logout_redirect_uri is derived from the trusted request host (never from user input, mirroring oidcRedirectURL) so it is same-origin by construction. Falls back to the existing local-only /login redirect when any condition above isn't met, or when discovery fails transiently — a user's logout is never blocked by an IdP hiccup. Adds the admin toggle to the OIDC settings page and updates the OIDC docs page with a Single Logout section. Closes bead bookshelf-tm38.9 on merge.Settings journey screenshot (settings-shell-email-tab-modal)
Settings journey screenshot (settings-shell-metadata-tab)
Settings journey screenshot (settings-ratelimits-comicvine-throttled)
Settings journey screenshot (settings-comic-weights-max-saves)
Settings journey screenshot (settings-comic-field-priority-matrix-saved)
Settings journey screenshot (oidc-settings-form-fields)
Settings journey screenshot (oidc-rp-initiated-logout-toggle-checked)
Settings journey screenshot (oidc-settings-after-reload)
Settings journey screenshot (oidc-test-connection-result)
Settings journey screenshot (oidc-group-mapping-modal-open)
Settings journey screenshot (oidc-group-mapping-after-create)
Settings journey screenshot (oidc-group-mapping-after-edit)
Settings journey screenshot (oidc-group-mapping-after-delete)
Settings journey screenshot (sidecar-settings-toggle-saved)
Settings journey screenshot (sidecar-settings-toggle-persisted)
[MAJOR] internal/users/oidc_handler.go:107-125,238 — ID-token cookie is not "short-lived": it lives as long as the refresh token (default 30 days)
setOIDCIDTokenCookieis documented as stashing the raw OIDC ID token "in a short-lived, HttpOnly cookie", butoidcCallbackHandler(line 238) calls it withexpiry = result.RefreshTokenExpiry, andRefreshTokenTTLdefaults to 720h/30 days (internal/config/config.go:444). The ID token contains PII/claims (sub, email, name, groups, aud, iss) and its ownexpclaim is normally minutes-to-hours — far shorter than 30 days. Storing the raw ID token in a cookie for the full refresh-token lifetime (a) needlessly widens the PII-exposure window (30 days of disk/browser-cookie-store persistence for a token whose IdP-side validity is long past), and (b) is likely to break the very feature it supports: several IdPs (Keycloak, Auth0, etc.) reject anid_token_hintwhose ownexphas elapsed, so RP-initiated logout will silently fall back to local-only logout for any session older than the ID token's real TTL — which for most providers is much less than 30 days. Fix: bind the cookie's expiry to the ID token's ownexpclaim (already verified inverifyIDToken/oidcVerifyAndProvision) or toAccessTokenExpiry(short), notRefreshTokenExpiry.[MINOR] internal/users/oidc_service.go —
stategenerated for RP-initiated logout is never validated on returnOIDCLogoutRedirectURLcallsgenerateOIDCState()and appendsstateto the end_session_endpoint redirect, but the IdP's post-logout callback lands on the trusted/loginroute (oidcPostLogoutRedirectURI), which does not read or verify anystatequery parameter. The value is emitted per spec but is otherwise dead protection — harmless, but worth a short comment noting it's intentionally unverified (no CSRF-relevant action happens at/login) so a future reader doesn't assume it's checked somewhere.[MINOR] internal/settings/oidc_handler.go:158 — stray trailing blank-line removal is unrelated churn
Line 158 removes a trailing blank line at EOF unrelated to this feature; harmless but adds unrelated diff noise. No action needed, just noting for a tighter diff next time.
Wiring, admin gating, fallback correctness, cookie flags (HttpOnly/Secure/SameSite=Lax), same-origin post_logout_redirect_uri construction, curried DI, black-box tests, JS/CSS convention reuse, and docs are all verified correct — see notes below.
Verified as correct (no findings):
OIDCLogoutRedirectURLintoDeps;logoutHandler(internal/users/handler.go) reads thebookshelf_oidc_idtokencookie, callslogoutOIDCRedirectURL, and redirects (HTML) or returnslogout_url(JSON) when non-empty, else falls back to/login. Not inert.PUT /settings/oidcstays behindadminRequired(internal/settings/routes.go:62) — unchanged, still applies to the newrp_initiated_logoutfield.OIDCLogoutRedirectURLreturns"", nil(not an error) on disabled feature, missing id token, discovery failure, or missingend_session_endpoint— logout never hangs or 500s in those cases.clearOIDCIDTokenCookieis called on both local login (handler.go) and logout, preventing staleid_token_hintleakage across session types; covered by tests.r.Host/AllowedHost+ hardcoded/loginpath, mirroring the existingoidcRedirectURLpattern — not attacker-influenced beyond the same Host-header trust boundary that already exists for the OIDC callback URL.package users_test/settings_test), curried DI preserved, JS controller has new Vitest coverage for the checkbox, browser e2e journey extended to persist+reload the toggle..golangci.yml/coverage-exclusion changes.docs/content/docs/administering/oidc.mdupdated with end-user admin instructions, no source-code references.REVIEW VERDICT: 0 blocker, 1 major, 2 minor
Security review — PR #1398 (bookshelf-tm38.9, OIDC RP-initiated single logout)
[MAJOR] internal/users/oidc_service.go:804-826 — discovery-supplied
end_session_endpointis redirected to with onlyurl.ParsevalidationOIDCLogoutRedirectURLtakesmeta.EndSessionEndpointstraight from the provider's/.well-known/openid-configurationdiscovery document and, after only a syntacticurl.Parsecheck, builds a URL that includes the rawid_token_hint(the user's OIDC ID token — a PII-bearing artifact) and 302-redirects the browser to it (internal/users/handler.go:369-372). Unlike the JWKS/token endpoints (which are only ever used for a server-side fetch), this is a browser redirect target, so if the configured issuer's discovery document is ever compromised or malicious (rotated infra, on-path MITM outside TLS pinning, or an intentionally hostile "identity provider" an admin points at),end_session_endpointcan be set to an arbitrary off-origin URL. Pergamum will then redirect every logging-out user's browser to that URL with their raw ID token attached as a query parameter — a discovery-driven open redirect that also exfiltrates the ID token to an attacker-controlled host (classic phishing/token-leak vector, and the same trust-boundary gap flagged forjwks_uriin #1396). RFC 8414 §3.3/OIDC Discovery's issuer-match check (already present inDiscoverOIDCMeta) only prevents a substituted document from a different issuer being accepted — it does nothing to constrain what the legitimate configured issuer's document is allowed to put inend_session_endpoint.Fix: validate
endSessionURL.Scheme == "https"(reject non-https before redirecting) and, ideally, thatendSessionURL.Hostmatches (or is a subdomain of) the configured issuer's host — mirroring whatever guard is chosen for thejwks_urifinding in #1396 so both discovery-sourced endpoints get the same allowlisting treatment.[MINOR] internal/users/oidc_service.go:814,823 —
stateis generated for the logout redirect but never verified on returnA
stateparameter is generated and attached to theend_session_endpointredirect, but thepost_logout_redirect_uriis a static/loginroute with no logout-callback handler that checks the returnedstatematches what was issued (no cookie/session correlation is set up before the redirect). This has no real security impact here since the local Pergamum session is already fully torn down (cookies cleared) before the redirect fires, so there's nothing sensitive left to protect on return — but thestateparam as implemented provides no actual CSRF/binding value, just spec-compliance decoration. Consider either wiring a matching cookie + verification on/login?state=...or noting in the code comment thatstatehere is emitted for spec compliance only and not verified.What was checked and found OK:
discoveryCached.discover(same cache/singleflight used by login), fetching from the admin-configuredissuerURL — not a new unguarded server-side fetch, and not attacker-influenceable input (unlike a per-request URL). No new SSRF surface introduced by this diff.bookshelf_oidc_idtoken,internal/users/oidc_handler.go:614-643) isHttpOnly,Secure(mirrorsSecureCookiesconfig),SameSite=Lax,Path=/— not JS-readable, matches the existing access/refresh cookie posture. Properly cleared (MaxAge=-1) on both logout and local login (defends against a stale OIDC cookie surviving into a fresh local-only session and leaking asid_token_hint).post_logout_redirect_uriis built exclusively from the trusted request-host derivation (oidcPostLogoutRedirectURI, mirrors the existingoidcRedirectURLpattern) — never from request body/query input — so it is same-origin by construction; not attacker-controllable./settings/oidcPUT (the admin toggle) is unchanged and still wrapped inadminRequired(internal/settings/routes.go:88); no gating regression./logoutremains POST-only, unchanged route registration; no new unauthenticated surface.slog.Warncalls on discovery/parse failure log onlyerr, not the token or URL content.end_session_endpointpaths degrade gracefully to local-only logout rather than blocking or erroring the user's logout (fail-open on the feature, not fail-open on security).style=/ CSP issues in the new template block (templates/pages/settings_shell.html).REVIEW VERDICT: 0 blocker, 1 major, 1 minor
Settings journey screenshot (settings-shell-email-tab-modal)
Settings journey screenshot (settings-shell-metadata-tab)
Settings journey screenshot (settings-ratelimits-comicvine-throttled)
Settings journey screenshot (settings-comic-weights-max-saves)
Settings journey screenshot (settings-comic-field-priority-matrix-saved)
Settings journey screenshot (oidc-settings-form-fields)
Settings journey screenshot (oidc-rp-initiated-logout-toggle-checked)
Settings journey screenshot (oidc-settings-after-reload)
Settings journey screenshot (oidc-test-connection-result)
Settings journey screenshot (oidc-group-mapping-modal-open)
Settings journey screenshot (oidc-group-mapping-after-create)
Settings journey screenshot (oidc-group-mapping-after-edit)
Settings journey screenshot (oidc-group-mapping-after-delete)
Settings journey screenshot (sidecar-settings-toggle-saved)
Settings journey screenshot (sidecar-settings-toggle-persisted)
Security re-review — PR #1398 (bookshelf-tm38.9), commit
232bccb8Re-reviewed the fix for the
end_session_endpointopen-redirect/token-exfil MAJOR againstinternal/users/oidc_service.go,oidc_handler.go,service.go, and the accompanying tests.Verification of the fix
Host-trust gate is enforced before any browser redirect.
logoutHandler(internal/users/handler.go:284) →logoutOIDCRedirectURL→d.OIDCLogoutRedirectURL→OIDCLogoutRedirectURL(oidc_service.go:1269) parsesmeta.EndSessionEndpointand callsisTrustedEndSessionEndpoint(endSessionURL, cfg.Issuer)(oidc_service.go:~1340) before theid_token_hint/redirect URL is ever built. On any failure it returns("", nil), and the handler falls back tohttp.Redirect(w, r, "/login", ...)— a fail-safe, non-erroring fallback with no off-origin exposure of the ID token.Adversarially tested the host-compare (
strings.EqualFold(endSessionURL.Hostname(), issuerURL.Hostname())after requiringScheme == "https") against the classic bypass set:https://provider.example.com.evil.com/logout(suffix trick)provider.example.com.evil.com, no match)https://evil.com/logout?x=provider.example.com(query trick)https://PROVIDER.EXAMPLE.COM/logout(case)EqualFoldhandles ithttps://provider.example.com./logout(trailing dot)https://provider.example.com:8443/logout(port)Hostname()strips port, correct per RFC (port doesn't affect origin-trust intent here)https://user@provider.example.com@evil.com/logout(userinfo confusion)net/urlresolves the rightmost@as the userinfo/host separator, soHostname()correctly returnsevil.comhttps://provider.example.com:443@evil.com/logoutHostname()=evil.comhttps://xn--80ak6aa92e.com/logout(punycode)https:evil.com/logout(opaque/no-host)Hostname()="", no matchhttp://,javascript:,://bad)Scheme != "https"guard / parse-error fallbackNo bypass found. The comparison is an exact hostname match, not
strings.Contains/strings.HasSuffix, andurl.Hostname()already strips port + userinfo per Go'snet/urlsemantics, so none of the classic parser-confusion tricks (userinfo-as-host, subdomain suffix, query-param spoofing) get through. This matches the unit tests added inoidc_service_test.go(different-host, http-scheme, unparseable endpoint, unparseable issuer all assertExpect(url, err).To(BeEmpty())).One structural note (not a bypass, just worth naming): the trust anchor is
cfg.Issuer— the admin-configured issuer string — not theissuerfield returned inside the discovery document itself. That's the correct trust root (admin config is trusted input; the discovery response'send_session_endpointis the untrusted field being validated), so this is not a finding, just confirming the threat model is right.ID-token cookie expiry.
idTokenExpiryFromClaims(oidc_service.go) now derives the cookie'sExpiresfrom the verified ID token's ownexpclaim, with a defensive fallback tonow + accessTTL(short) ifexpis absent — never the 30-day refresh TTL. This is a genuine reduction of the PII-exposure window: previously the cookie (carryingsub/email/name/groupsvia the raw ID token) could persist up to 30 days; now it's bounded by the token's own lifetime (typically minutes-to-an-hour), and even the defensive fallback only extends to the access-token TTL, not the refresh TTL. Covered byoidc_coverage_test.go's new "binds the ID-token cookie expiry to the token's own exp claim" and "ID token without an exp claim" specs.Cookie flags on
oidcIDTokenCookieName(oidc_handler.go:setOIDCIDTokenCookie) are unchanged from the pre-fix shape:HttpOnly: true,Secure: secure(wired fromd.SecureCookies),SameSite: http.SameSiteLaxMode,Path: "/". Appropriately locked down for a token that's read server-side only (never JS-accessible) and only leaves the origin via the now-validated end_session_endpoint redirect.Fallback correctness confirmed for every failure branch: OIDC disabled,
RPInitiatedLogoutoff, emptyidTokenHint, discovery failure, missingend_session_endpoint, unparseable endpoint/issuer, non-https, different host — all return("", nil)and the caller redirects to local/loginwith no ID token ever attached to an off-origin URL.getConfigfailure andgenerateOIDCStatefailure are the only paths that propagate a realerror(both pre-redirect, no ID token exposure either way).Findings
No new blockers or majors found. The original open-redirect/token-exfil MAJOR is closed by
isTrustedEndSessionEndpoint, and the ID-token cookie expiry MAJOR is closed byidTokenExpiryFromClaims.[MINOR] internal/users/oidc_service.go:1349 (isTrustedEndSessionEndpoint) — Consider also requiring
issuerURL.Scheme == "https"(or otherwise validatingcfg.Issueris well-formed with a host) so a misconfiguredhttp://issuer can't accidentally validate; today it doesn't matter becauseendSessionURL.Schemeis independently forced tohttps, but pinning both sides makes the invariant self-evident to a future reader. Not exploitable as written — pure defense-in-depth/readability.[MINOR] internal/users/oidc_handler.go:118 (setOIDCIDTokenCookie) —
SameSite: http.SameSiteLaxModeis fine (cookie is HttpOnly and never needed cross-site), but since this cookie is only ever read server-side within same-site navigations (login/logout POST + callback redirect),SameSite: http.SameSiteStrictModewould tighten it further with no functional loss. Not a vulnerability as shipped.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
Code Review — bookshelf-tm38.11 (configurable OIDC session duration)
[BLOCKER] internal/app/app.go:359-372 — SessionDuration override is never wired into production, feature is dead code
This PR adds
SessionDuration time.Durationtoappwire.OIDCRuntimeConfig(internal/appwire/appwire.go:854-856) and correctly threads it throughinternal/settings/oidc_settings.go'sGetOIDCConfigandinternal/users/wire.go:123(SessionDuration: rc.SessionDuration). But the one production call site that constructsappwire.OIDCRuntimeConfig— theGetOIDCConfigclosure ininternal/app/app.go(lines 354-373) — builds the struct with an explicit field list (Enabled,Issuer,ClientID, ...OIDCOnlyMode) and does not copyrc.SessionDurationfromsettings.OIDCRuntimeConfiginto it. Since this diff does not touchinternal/app/app.goat all, the field silently defaults to zero at runtime, soeffectiveRefreshTTL(internal/users/oidc_service.go:452) always falls back to the global TTL — an admin who sets a session duration override in the settings UI has it saved and re-displayed (round-trips through the DB/UI, per the new e2e journey step), but it never actually changes any issued OIDC session's expiry.This is uncaught because every test that exercises the new behavior stubs
GetOIDCConfig/cfg.SessionDurationdirectly (internal/settings/oidc_settings_test.go, internal/users/oidc_coverage_test.go) and the new browser e2e journey only asserts the form value persists (e2e/browser/journey_settings_test.go:417-419) — none of them go through the realapp.Newwiring closure in app.go that is the only place this actually gets consumed.Fix: add
SessionDuration: rc.SessionDuration,to the struct literal ininternal/app/app.go'sGetOIDCConfigclosure (~line 372, alongsideOIDCOnlyMode). Also worth flagging (pre-existing, out of scope for this bead but same shape of bug):DefaultPermissions/DefaultLibraryIDsare also missing from that same struct literal — a prior feature (JIT auto-provision defaults) may have the identical wiring gap; suggest filing a follow-up bead to audit/complete that mapping and consider adding a genuine end-to-end (realapp.New) e2e/integration assertion that an OIDC login actually respects a configured session-duration override, so this class of gap can't recur silently.[MAJOR] internal/users/oidc_coverage_test.go:2338-2361 — new wall-clock (
time.Now()) assertions in the test'sItbodiesThree new
Itblocks assertcapturedRefreshTTL.ExpiryDate/result.AccessTokenExpiryagainsttime.Now().Add(...)computed at assertion time (not injected/frozen), with a 5-secondBeTemporally("~", ..., 5*time.Second)tolerance. Per.claude/rules/review-standard.md→ "Flake prevention" this is exactly the banned pattern: asserting on real elapsed time between the action (JustBeforeEachinvokinghandleCallback, which internally computesnow.Add(refreshTTL)) and the assertion. 5s is generous for a fast unit test, but under CI load/parallel test contention it is not impossible to exceed, and the fix is cheap. The file already has access tologger/config plumbed by closures — prefer injecting a fixednow(mirroring the pattern already used elsewhere in this codebase, e.g.internal/users/device_store_test.go:48fakeNow, andinternal/users/service_test.go:257fixedNow) rather than a wall-clock tolerance window.Fix: thread a fixed
now time.Timeinto whatever computesExpiryDate/AccessTokenExpiryin this flow (or, if that's out of reach without touching production code in this bead, at minimum capturetime.Now()once beforeJustBeforeEachruns and assert against that captured value with a tight tolerance, removing the skew between per-Ittime.Now()calls and the actual event).[MINOR] templates/pages/settings_shell.html:1361 — session-duration input has
min="1"but nomaxattributeThe clamp is enforced server-side (
oidcSessionDurationMaxHours= 8760h/1yr) so this is not a correctness issue, but addingmax="8760"to the<input type="number">would give the admin instant client-side feedback consistent with themin="1"already present, rather than only discovering the clamp after a round-trip save+reload.REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Code Review — bookshelf-tm38.11 (configurable OIDC session duration)
[BLOCKER] internal/app/app.go:359-372 — SessionDuration override is never wired into production, feature is dead code
This PR adds
SessionDuration time.Durationtoappwire.OIDCRuntimeConfig(internal/appwire/appwire.go:854-856) and correctly threads it throughinternal/settings/oidc_settings.go'sGetOIDCConfigandinternal/users/wire.go:123(SessionDuration: rc.SessionDuration). But the one production call site that constructsappwire.OIDCRuntimeConfig— theGetOIDCConfigclosure ininternal/app/app.go(lines 354-373) — builds the struct with an explicit field list (Enabled,Issuer,ClientID, ...OIDCOnlyMode) and does not copyrc.SessionDurationfromsettings.OIDCRuntimeConfiginto it. Since this diff does not touchinternal/app/app.goat all, the field silently defaults to zero at runtime, soeffectiveRefreshTTL(internal/users/oidc_service.go:452) always falls back to the global TTL — an admin who sets a session duration override in the settings UI has it saved and re-displayed (round-trips through the DB/UI, per the new e2e journey step), but it never actually changes any issued OIDC session's expiry.This is uncaught because every test that exercises the new behavior stubs
GetOIDCConfig/cfg.SessionDurationdirectly (internal/settings/oidc_settings_test.go, internal/users/oidc_coverage_test.go) and the new browser e2e journey only asserts the form value persists (e2e/browser/journey_settings_test.go:417-419) — none of them go through the realapp.Newwiring closure in app.go that is the only place this actually gets consumed.Fix: add
SessionDuration: rc.SessionDuration,to the struct literal ininternal/app/app.go'sGetOIDCConfigclosure (~line 372, alongsideOIDCOnlyMode). Also worth flagging (pre-existing, out of scope for this bead but same shape of bug):DefaultPermissions/DefaultLibraryIDsare also missing from that same struct literal — a prior feature (JIT auto-provision defaults) may have the identical wiring gap; suggest filing a follow-up bead to audit/complete that mapping and consider adding a genuine end-to-end (realapp.New) e2e/integration assertion that an OIDC login actually respects a configured session-duration override, so this class of gap can't recur silently.[MAJOR] internal/users/oidc_coverage_test.go:2338-2361 — new wall-clock (
time.Now()) assertions in the test'sItbodiesThree new
Itblocks assertcapturedRefreshTTL.ExpiryDate/result.AccessTokenExpiryagainsttime.Now().Add(...)computed at assertion time (not injected/frozen), with a 5-secondBeTemporally("~", ..., 5*time.Second)tolerance. Per.claude/rules/review-standard.md→ "Flake prevention" this is exactly the banned pattern: asserting on real elapsed time between the action (JustBeforeEachinvokinghandleCallback, which internally computesnow.Add(refreshTTL)) and the assertion. 5s is generous for a fast unit test, but under CI load/parallel test contention it is not impossible to exceed, and the fix is cheap. The file already has access tologger/config plumbed by closures — prefer injecting a fixednow(mirroring the pattern already used elsewhere in this codebase, e.g.internal/users/device_store_test.go:48fakeNow, andinternal/users/service_test.go:257fixedNow) rather than a wall-clock tolerance window.Fix: thread a fixed
now time.Timeinto whatever computesExpiryDate/AccessTokenExpiryin this flow (or, if that's out of reach without touching production code in this bead, at minimum capturetime.Now()once beforeJustBeforeEachruns and assert against that captured value with a tight tolerance, removing the skew between per-Ittime.Now()calls and the actual event).[MINOR] templates/pages/settings_shell.html:1361 — session-duration input has
min="1"but nomaxattributeThe clamp is enforced server-side (
oidcSessionDurationMaxHours= 8760h/1yr) so this is not a correctness issue, but addingmax="8760"to the<input type="number">would give the admin instant client-side feedback consistent with themin="1"already present, rather than only discovering the clamp after a round-trip save+reload.REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Code re-review of the fix commit (
232bccb8, bookshelf-tm38.9)Reviewed the delta commit
fix(review): address PR #1398 MAJORs — end_session_endpoint SSRF/open-redirect + ID-token cookie TTLagainstorigin/mainandorigin/bd-bookshelf-tm38.9, plus the surrounding diff for consistency.Confirmed resolved
idTokenExpiryFromClaims(claims, now, accessTTL)readsclaims["exp"].(float64)(correct —encoding/jsondecodes JWTNumericDatetofloat64) and returnstime.Unix(int64(exp), 0), falling back tonow.Add(accessTTL)only whenexpis absent/non-numeric (internal/users/oidc_service.go:878-883).oidcFinalizeLogincomputes it (oidc_service.go:893) and threads it intooidcIssueTokens(..., idTokenExpiry, ...)(oidc_service.go:894), which setsLoginResult.IDTokenExpiry = idTokenExpiry(oidc_service.go:868).oidcCallbackHandlernow callssetOIDCIDTokenCookie(w, result.IDToken, secureCookies, result.IDTokenExpiry)(internal/users/oidc_handler.go:764) — confirmed viagit show 232bccb8e -- internal/users/oidc_handler.gothat this line changed fromresult.RefreshTokenExpirytoresult.IDTokenExpiry, and it is the only caller ofoidcIssueTokens/setOIDCIDTokenCookie. Value is genuinely bound to the token's ownexp, not the 30-day refresh TTL.isTrustedEndSessionEndpoint(SSRF/open-redirect MAJOR) is correct. Requireshttpsscheme andstrings.EqualFold(endSessionURL.Hostname(), issuerURL.Hostname())(oidc_service.go:991-999). All non-matching/malformed cases (getConfigerror, disabled/off, discovery failure, missing/unparseableend_session_endpoint, off-host, http-not-https, unparseable issuer) return("", nil)cleanly and fall back to local-only logout — verified inOIDCLogoutRedirectURL(oidc_service.go:918-981) and covered by the newoidc_service_test.gocontexts (off-host, http, unparseable issuer).git diff origin/main 797929974 -- internal/settings/oidc_handler.goshows the base commit accidentally stripped the file's trailing newline; the fix commit's+blank line at EOF restores it. Confirmedgit diff origin/main origin/bd-bookshelf-tm38.9 -- internal/settings/oidc_handler.gonow shows only the intendedRPInitiatedLogoutfield addition — byte-identical elsewhere.oidcIssueTokens/oidcFinalizeLogin/LoginResultis consistent; single call site, no other caller broken. No.golangci.ymlorscripts/check-coverage.shexclusions added. New tests are black-box and one-Expect-per-It.Findings
[MAJOR] internal/users/oidc_handler_test.go:213-266 — no test locks the exact call site that was just fixed (cookie Expiry ← IDTokenExpiry, not RefreshTokenExpiry)
The
oidcCallbackHandlerDescribeblock'scallbackResultonly setsAccessToken/RefreshToken/IDToken—IDTokenExpiryandRefreshTokenExpiryare both left at their zero value. The only handler-level assertion is that a cookie namedbookshelf_oidc_idtokenis present (oidc_handler_test.go:265-271); nothing asserts itsExpires/MaxAge. The regression coverage added in this fix commit (oidc_coverage_test.goresult.IDTokenExpiryassertion,oidc_service_test.go) only provesLoginResult.IDTokenExpiryis computed correctly from claims — it never exercises thesetOIDCIDTokenCookie(w, result.IDToken, secureCookies, result.IDTokenExpiry)call site in the handler. If a future edit silently revertedresult.IDTokenExpiryback toresult.RefreshTokenExpiryat that call site (the exact bug this fix addresses), no test in the suite would fail — the 30-day-cookie regression could ship undetected.Fix: in the
oidcCallbackHandlerDescribe, setcallbackResult.IDTokenExpiryandcallbackResult.RefreshTokenExpiryto two distinct, distinguishable times inBeforeEach, then add anItasserting thebookshelf_oidc_idtokencookie'sExpiresequalscallbackResult.IDTokenExpiry(and, ideally, that it does NOT equalRefreshTokenExpiry) so this exact regression is test-locked at the layer where it actually manifests (the cookie on the wire), not just at theLoginResultcomputation layer.[MINOR] internal/users/oidc_service.go:966 — comment grammar
"Do not assume a future reader that it is checked."reads awkwardly (subject/object swapped). Suggested:"Do not let a future reader assume it is checked."REVIEW VERDICT: 0 blocker, 1 major, 1 minor
Settings journey screenshot (settings-shell-email-tab-modal)
Settings journey screenshot (settings-shell-metadata-tab)
Settings journey screenshot (settings-ratelimits-comicvine-throttled)
Settings journey screenshot (settings-comic-weights-max-saves)
Settings journey screenshot (settings-comic-field-priority-matrix-saved)
Settings journey screenshot (oidc-settings-form-fields)
Settings journey screenshot (oidc-rp-initiated-logout-toggle-checked)
Settings journey screenshot (oidc-settings-after-reload)
Settings journey screenshot (oidc-test-connection-result)
Settings journey screenshot (oidc-group-mapping-modal-open)
Settings journey screenshot (oidc-group-mapping-after-create)
Settings journey screenshot (oidc-group-mapping-after-edit)
Settings journey screenshot (oidc-group-mapping-after-delete)
Settings journey screenshot (sidecar-settings-toggle-saved)
Settings journey screenshot (sidecar-settings-toggle-persisted)
c89340c4768a746189f3Settings journey screenshot (settings-shell-email-tab-modal)
Settings journey screenshot (settings-shell-metadata-tab)
Settings journey screenshot (settings-ratelimits-comicvine-throttled)
Settings journey screenshot (settings-comic-weights-max-saves)
Settings journey screenshot (settings-comic-field-priority-matrix-saved)
Settings journey screenshot (oidc-settings-form-fields)
Settings journey screenshot (oidc-rp-initiated-logout-toggle-checked)
Settings journey screenshot (oidc-settings-after-reload)
Settings journey screenshot (oidc-test-connection-structured-diagnostic)
Settings journey screenshot (oidc-group-mapping-modal-open)
Settings journey screenshot (oidc-group-mapping-after-create)
Settings journey screenshot (oidc-group-mapping-after-edit)
Settings journey screenshot (oidc-group-mapping-after-delete)
Settings journey screenshot (sidecar-settings-toggle-saved)
Settings journey screenshot (sidecar-settings-toggle-persisted)
zombor referenced this pull request2026-08-08 16:29:53 +00:00