feat(oidc): RP-Initiated Logout — backend issues end_session URL on logout

[an9xyz]

Summary

Implements RP-Initiated Logout for OIDC login: the backend now hands the
frontend a ready-to-use end_session_url so a logout can terminate both the
local Octo session and the IdP (Aegis) browser SSO session.

Why backend-owned: the code → token exchange happens server-side, so the
frontend never holds the id_token and cannot build the id_token_hint itself.
The backend caches the verified id_token at login and assembles the
end_session URL at logout. Companion frontend issue: Mininglamp-OSS/octo-web#184.

Changes

• oidc_client.go — resolve end_session_endpoint from discovery
  (provider.Claims), expose EndSessionEndpoint().
• callback — on success (both the direct IssueSession path and the
  self-service bind path), cache the verified id_token AES-256-GCM
  encrypted in Redis. The bind path stashes it under a bind-token (jti) key
  at bind_pending and promotes it to the uid key on /bind/confirm or
  /bind/create success (one-time consumed).
• logout — kick + revoke unchanged; now also returns end_session_url
  (id_token_hint + a server-configured post_logout_redirect_uri). The
  id_token is consumed atomically (Lua GETDEL).
• config — three new optional env vars (see below).

Configuration (new, all optional)

Env	Default	Purpose
OCTO_OIDC_POST_LOGOUT_REDIRECT_URI	"" (feature off)	Post-logout redirect target (a hard-coded login page). Acts as the master switch and the redirect allowlist — it is never taken from the request body.
OCTO_OIDC_PROVIDER_END_SESSION_URL	""	Override/fallback for the IdP end_session endpoint; only needed if discovery omits end_session_endpoint.
OCTO_OIDC_PROVIDER_ID_TOKEN_TTL	168h (7d)	TTL for the cached id_token; clamped to >0 to avoid a never-expiring key. Parsed by time.ParseDuration (no d unit — use 168h, not 7d).

Backward compatibility

Purely additive and opt-in:

• With OCTO_OIDC_POST_LOGOUT_REDIRECT_URI unset (default), the id_token
  store is not constructed — no PII-bearing JWT is persisted, no extra
  Redis connection pool — and logout returns exactly the prior
  {"status":200}. Behavior is byte-for-byte unchanged.
• The only API-contract change is an optional additional field
  (end_session_url) on the logout response; JSON consumers ignore unknown
  fields.
• No SQL / migration / schema changes. No new required config. No changes
  outside modules/oidc/.

Security

• id_token is encrypted at rest (same key derivation as the refresh token)
  and never logged (nor is the assembled URL that embeds it).
• post_logout_redirect_uri is server-side hard-coded, so it doubles as the
  allowlist (no open-redirect surface from the request body).

Testing

• go test ./modules/oidc -race (unit + integration) — pass.
• gofmt / go vet clean; D23 lint-direct-error-response gate — no new
  direct error responses.
• New coverage: discovery end_session_endpoint parsing; logout URL build /
  escaping / one-time consume; degrade paths (no id_token / no redirect config /
  store disabled); callback caches id_token; bind confirm/create promote the
  stashed id_token to uid.

Ops prerequisites (before enabling in an environment)

• Register the post_logout_redirect_uri on the Aegis side (IdP logout
  redirect allowlist).
• Confirm whether Aegis end_session requires id_token_hint.

Closes #215

[Jerry-Xin]

The PR is in scope for octo-server: it extends the existing OIDC module’s logout flow and related bind/callback paths.

💬 Non-blocking

• 🟡 Warning: api.go accepts any url.Parse-valid end_session endpoint. Since the frontend will likely top-level navigate to this value, consider rejecting non-absolute or non-HTTP(S) endpoints before returning end_session_url.
• 🔵 Suggestion: api.go enables the Redis id-token store solely from PostLogoutRedirectURI. If discovery has no end_session_endpoint and no override is configured, the service may persist encrypted id_tokens even though logout can never use them. Consider enabling the store only when a usable endpoint is also known.

✅ Highlights

• Good one-time consumption semantics using atomic Lua GET+DEL in logout_idtoken.go.
• Callback and bind flows both preserve the verified id_token path needed for RP-Initiated Logout.
• The feature is opt-in and degrades cleanly to the prior local-only logout response when prerequisites are missing.

Verification: targeted new tests passed. Full go test ./modules/oidc could not complete in this environment because Redis/MySQL integration dependencies on 127.0.0.1:6379 and 127.0.0.1:3306 were unavailable.

[lml2468]

APPROVED — PR #217 at e0beb3a

Security Assessment

This is a well-designed, security-conscious RP-Initiated Logout implementation. No blocking issues found.

Verified Security Properties

1. id_token at rest: AES-256-GCM encrypted in Redis (same key derivation as refresh token), base64-encoded ciphertext — no plaintext id_token in storage. ✅
2. id_token leakage: Never logged. buildEndSessionURL logs only the endpoint on parse error, never the assembled URL containing id_token_hint. ✅
3. One-time consume: Lua GETDEL script provides atomic get-and-delete, eliminating concurrent double-logout extraction window. ✅
4. No open redirect: post_logout_redirect_uri is server-side hard-coded via env var, never taken from request body. Single value = single allowlist entry. ✅
5. Opt-in by default: With OCTO_OIDC_POST_LOGOUT_REDIRECT_URI unset, idTokenStore is nil, no Redis connection pool created, no PII cached, logout response unchanged. ✅
6. TTL safety: IDTokenTTL <= 0 clamped to 7d default, preventing never-expiring Redis keys (go-redis Set with 0 TTL = persistent). ✅
7. Graceful degradation: All id_token save/take failures are best-effort (Warn-level log, no login/logout blocking). ✅
8. Bind path: Two-phase stash (jti key at bind_pending) → promote (uid key at confirm/create) correctly handles the bind flow where uid is unknown at callback time. jti namespace isolation via bind: prefix prevents collision with uid keys. ✅
9. URL construction: Uses url.Parse + url.Values.Set + url.Values.Encode — proper query parameter escaping. Test TestBuildEndSessionURL_OverrideAndEscaping verifies round-trip of special characters. ✅
10. Discovery parsing: provider.Claims(&extra) failure is silently ignored — Client construction succeeds, logout falls back to config override or degrades. ✅

Code Quality

• 304 lines of tests covering: happy path, no-id-token degrade, no-redirect-config degrade, nil-store degrade, callback stores id_token, bind stash/promote for both confirm and create, URL override + escaping, discovery endpoint parsing.
• All test assertions verify both positive (URL present, correct parameters) and negative (consumed after use, not leaked) properties.

CI Note

• check-sprint fails: linked issue #215 exists but has no Sprint set on the Octo Board. Administrative fix needed (assign Sprint W22 to issue #215).
• Build/i18n/Lint/PersonalMsgSendReq passed. Test/Vet/CodeQL were pending at review time.

No state Parameter

The end_session URL omits the optional state parameter — code comment explains Aegis discovery does not declare it. Per RP-Initiated Logout spec, state is OPTIONAL and only useful when the RP needs to correlate the post-logout redirect. Since the redirect target is a static login page, omission is correct.

No findings.

[yujiawei]

Code Review — PR #217 (octo-server)

feat(oidc): RP-Initiated Logout — backend issues end_session URL on logout

Independent review of the OIDC RP-Initiated Logout feature. Verified locally at head e0beb3a: go vet ./modules/oidc/ clean, gofmt clean (no changed file is dirty), and all new unit tests pass (TestAPI_Logout_*, TestBuildEndSessionURL_*, TestClient_EndSessionEndpoint_*, TestAPI_Callback_*, TestAPI_BindConfirm/Create_PromotesIDToken).

Verdict

APPROVED. No P0/P1 blockers. The feature is opt-in (constructed only when OCTO_OIDC_POST_LOGOUT_REDIRECT_URI is set), the cached id_token is AES-256-GCM encrypted at rest, one-time consumed via an atomic Lua GETDEL, and never logged. Backward compatibility is genuinely byte-for-byte when disabled. The findings below are hardening, comment-accuracy, and test-coverage improvements — none block merge, but several are worth addressing before enabling in production.

Strengths (verified)

• Encryption at rest reuses the audited Encryptor (AES-256-GCM, crypto.go); value is base64 of nonce‖ct‖tag — no plaintext JWT in Redis.
• One-time consume — Take uses the same luaGetDel as the state store (logout_idtoken.go:74), closing the concurrent double-logout double-emit window.
• Resource lifecycle — idTokens pool is closed in Close() (api.go:803-808), placed before the stateStore==nil early-return; created before NewClient so the Discovery-failure o.Close() path also releases it. Verified leak-safe and nil-safe.
• Feature gate — when PostLogoutRedirectURI is unset, o.idTokens stays nil, so no PII-bearing JWT is ever persisted. The <=0 TTL clamp (config.go:187-189) correctly neutralizes the go-redis "never-expire" footgun.
• Degrade contract — every Save/Take failure is best-effort: login/bind/logout still succeed; only the optional end_session_url is omitted. The genuinely security-relevant logout ops (Kick, RevokeRefreshByUID) keep their own error accounting and are not swallowed.

Findings

P2 — Misleading security comment: "frontend never holds id_token" is no longer accurate

api.go:826-828 justifies the backend-builds-URL design with "前端从不持有 id_token". But buildEndSessionURL puts the raw id_token into the id_token_hint query param (api.go:933) and logout returns that full URL in the JSON body (api.go:884-886); the frontend then does a top-level navigation. So the frontend does hold the token (in the URL), and it is exposed to the JS layer, browser history, the Referer header, and the IdP's HTTP access logs. This is inherent to OIDC RP-Initiated Logout (the spec requires id_token_hint in the front-channel redirect) and the token is single-use + non-replayable, so it is not an exploitable vuln — but for a security-sensitive change the rationale comment should be corrected to state the real trust model ("frontend cannot construct the hint; backend supplies a single-use URL that necessarily carries it") and the front-channel exposure should be explicitly accepted.

P2 — No scheme/host validation on PostLogoutRedirectURI / EndSessionURL

config.go:155-156 load both via plain getString with zero validation; buildEndSessionURL only checks non-empty + url.Parse success (url.Parse accepts javascript:, relative values, etc.). EndSessionURL is the host the browser navigates to with the id_token attached — a typo'd/wrong env value sends the token to an arbitrary host. Both are ops-trust inputs, so this is misconfiguration-hardening, not a client-facing vuln. But the codebase already establishes the exact pattern elsewhere — return_to.go:ValidateReturnTo (http/https + host allowlist) and bind_config.go OCTO_OIDC_BIND_REDIRECT_BASE (must be absolute https, explicitly rejecting javascript:/data:). Recommend a startup-time scheme==https + IsAbs check on both new values so a misconfig fails loudly at boot instead of silently weakening the "single value = allowlist" premise.

P2 — Best-effort degrade paths are untested (declared-but-unused error hooks)

fakeIDTokenStore declares saveErr and takeErr injection fields (logout_idtoken_test.go:22-23) and implements the return branches, but no test ever assigns them. As a result the PR's headline "best-effort degrade" contract is unproven on its error branches: buildEndSessionURL Take-error → Warn + omit URL + still 200 (api.go:917-922), and callback Save-error → still complete login (api.go:766-770). For a security-sensitive auth path, please add: (a) takeErr injected → logout still 200 and omits end_session_url; (b) saveErr injected → callback still 302/login succeeds.

P2 — bind:<jti> stash overloads the uid parameter / shares the id_token keyspace

saveBindIDTokenHint/promoteBindIDToken pass bindIDTokenKey(jti) ("bind:"+jti) through the uid parameter of Save/Take, so the bind stash lands at oidc:idtoken:bind:<jti> — same prefix as the real oidc:idtoken:<uid> cache. Isolation rests entirely on the comment assumption that uids never start with bind: (logout_idtoken.go:114-115), with no type/validation enforcement. Currently safe (uids are system-generated; jti is 32B base64), but a dedicated SaveByKey/separate stash interface, or a distinct physical prefix, would make the isolation a code guarantee rather than a convention.

nit — TTL doc/clamp polish

• The IDTokenTTL comment (config.go:89) cites the default as "7d", but time.ParseDuration rejects the d unit — an operator who copies OCTO_OIDC_PROVIDER_ID_TOKEN_TTL=7d would silently fall back to the default. Suggest writing 168h in the comment to match a parseable form. (Harm is low: the var is only referenced in a Go source comment, and the fallback value equals the intended 7d.)
• Consider clamping the cache TTL to min(IDTokenTTL, time-until-id_token.exp) — IDTokenClaims.Expiry is already decoded at callback, so a short-lived id_token need not sit encrypted at rest for 7 days after it is useless.
• No test covers OCTO_OIDC_PROVIDER_ID_TOKEN_TTL parsing or the <=0 clamp; a small config_test.go case (default / 168h override / negative→clamp) would lock in the footgun guard.

For the human reviewer (security-sensitive)

1. Confirm the IdP (Aegis) treats id_token_hint purely as a logout hint and does not accept it as a bearer/assertion elsewhere (verified that octo-server itself never re-accepts it — it issues its own session post-callback).
2. Confirm the front-channel id_token exposure (URL/history/Referer/IdP access logs) is acceptable per your threat model — it is spec-inherent to RP-Initiated Logout.
3. Ensure OCTO_OIDC_POST_LOGOUT_REDIRECT_URI is registered on the Aegis logout-redirect allowlist before enabling (per the PR's ops checklist).

[an9xyz]

Addressed the non-blocking review findings in 8f223d1e (all opt-in, no behavior change when disabled):

1. Comment accuracy — corrected the logout rationale: the frontend does carry the id_token (via id_token_hint); the comment now states the real trust model (front-channel exposure is RP-Initiated-Logout-inherent and accepted; token is single-use, atomically consumed, never re-accepted as a bearer).
2. Scheme validation — OCTO_OIDC_POST_LOGOUT_REDIRECT_URI / OCTO_OIDC_PROVIDER_END_SESSION_URL are now validated at startup (fail-loud) as absolute https, rejecting relative / javascript: / non-http(s). Dev escape hatch OCTO_OIDC_LOGOUT_ALLOW_INSECURE=1, mirroring validateBindRedirectBase.
3. Store gating — the id_token store is now constructed only when an end_session endpoint is also usable (discovery or override), not just when the redirect URI is set; moved after client construction. No tokens persisted if logout can never use them.
4. Degrade tests — added: Take error → logout still 200 + omits end_session_url; Save error → callback still completes login.
5. TTL — doc fixed (ParseDuration rejects 7d; use 168h) + config tests for default/override/invalid/clamp and the URL validator.

Deferred (documented as convention, currently safe): bind-stash keyspace isolation (#4) and clamping cache TTL to the id_token's own exp (nit). Happy to follow up if you'd like those in this PR.

Local: go test ./modules/oidc -race (unit + integration) green, gofmt/go vet clean, D23 ratchet clean.

[lml2468]

APPROVED — re-review at 8f223d1

Previous round feedback fully addressed:

1. URL scheme validation (Jerry-Xin + yujiawei): validateLogoutURL now rejects relative, javascript:, data:, ftp:, and http:// URLs at startup. Dev escape hatch via OCTO_OIDC_LOGOUT_ALLOW_INSECURE=1 follows existing bind pattern. ✅
2. Unnecessary Redis pool (Allen + yujiawei): idTokenStore init moved after client construction, now guarded by PostLogoutRedirectURI != "" && endSessionEndpoint() != "" — if discovery lacks endpoint and no override configured, no pool created. ✅
3. Comment accuracy (yujiawei): Logout comment now explicitly acknowledges id_token front-channel exposure is protocol-inherent (browser history, JS, Referer), not a side-effect of this implementation. ✅
4. IDTokenTTL doc (new): Comment now warns time.ParseDuration does not recognize "7d", must use "168h". ✅

New test coverage (+96 lines):

• TestValidateLogoutURL — 7 cases covering empty/https/relative/javascript/ftp/http-default/http-insecure
• TestLoadConfig_RejectsInsecureLogoutURL — integration test proving startup rejection
• TestLoadConfig_IDTokenTTL — 5 cases: default, override, invalid "7d" fallback, zero/negative clamping
• TestAPI_Logout_TakeError_OmitsURL — Redis Take failure degrade
• TestAPI_Callback_IDTokenSaveError_LoginStillSucceeds — Redis Save failure degrade

check-sprint now passing. All prior security verification carries forward. No new findings.

[yujiawei]

Code Review — PR #217 (octo-server)

feat(oidc): RP-Initiated Logout — backend issues end_session URL on logout

Independent review of the RP-Initiated Logout implementation. Scope: backend caches the verified id_token at login (encrypted) and, on logout, returns a single-use end_session_url (id_token_hint + a server-configured post_logout_redirect_uri) so the frontend can terminate the IdP SSO session. Reviewed at head 8f223d1e2e50574adb7334e73bf0f94cb20ef252.

I checked out the PR locally and verified: go build ./modules/oidc/ ✅, go vet ./modules/oidc/ ✅, and the new unit tests (logout URL build / one-time consume / all degrade paths / config validation / bind promote / callback cache) all pass.

1. Verification

Item	Result	Evidence
No open-redirect surface — post_logout_redirect_uri is server-configured, never from request body	✅	api.go:962-963 reads o.cfg.Provider.PostLogoutRedirectURI; config.go:316-320 loaded from env only
id_token encrypted at rest (AES-256-GCM, same key as refresh token)	✅	logout_idtoken.go:54-66 encrypts then base64; crypto.go:45-51 random-nonce GCM
One-time, atomic consume (no concurrent double-take)	✅	logout_idtoken.go:73-74 reuses luaGetDel (atomic GET+DEL, state_store_redis.go:21-27)
id_token / assembled URL never logged	✅	Only zap.Error(err) / trace_id / uid logged; grep of api.go/logout_idtoken.go shows no token value in any log call
Startup fail-loud URL validation (rejects relative / javascript: / non-https)	✅	config.go:219-242 validateLogoutURL, called at config.go:197,200; covered by TestValidateLogoutURL
id_token store only constructed when feature is actually usable (no PII JWT persisted otherwise)	✅	api.go:167-173 gates on PostLogoutRedirectURI != "" && endSessionEndpoint() != ""
Redis client released on Close (no fd leak)	✅	api.go:804-809 placed before the stateStore == nil early-return
TTL <= 0 clamp (avoids never-expiring key)	✅	config.go:190-191; covered by TestLoadConfig_IDTokenTTL
Backward compatible / opt-in (unset → byte-for-byte prior {"status":200})	✅	api.go:887-893 only adds the field when non-empty; TestAPI_Logout_NilStore_OmitsURL etc.
Best-effort degrade — login/bind never blocked by id_token store errors	✅	api.go:767-770 (callback Save), 949-955 / 961-975 (bind); TestAPI_Callback_IDTokenSaveError_LoginStillSucceeds
Bind path coverage (stash under jti → promote to uid on confirm/create)	✅	api.go:584, api_bind.go:209-211,485-487; TestAPI_BindConfirm/Create_PromotesIDToken

2. Findings

No P0 (correctness/security/data-loss/build-break) and no P1 (functional blocker) issues found. The items below are P2 / suggestions and do not block merge.

P2-1 — Discovery-derived end_session_endpoint is not scheme-validated

validateLogoutURL (config.go:219) fail-louds the config-override EndSessionURL and PostLogoutRedirectURI, but the endpoint resolved from the IdP Discovery document (oidc_client.go:181-187 → endSessionEndpoint() at api.go:902-904) is used verbatim to build the URL that embeds id_token_hint. If the IdP's Discovery ever advertised an http:// (or otherwise unexpected) end_session_endpoint, the backend would assemble a URL carrying the id_token over a non-https scheme.

Risk is low — Discovery is fetched over the issuer's HTTPS endpoint and the IdP is the trust anchor — so this is not a blocker. But given the security_sensitive classification, a human should confirm that Aegis's Discovery end_session_endpoint is https, and consider running the discovery-derived endpoint through the same https-only check (or at least logging a warning on a non-https discovery value) for defense in depth.

P2-2 — id_token is consumed before url.Parse(endpoint)

In buildEndSessionURL (api.go:923 then ~937), Take (GETDEL) deletes the cached id_token first; if the subsequent url.Parse(endpoint) fails, the token is already burned and no URL is returned, so that logout degrades to local-only and a retry has nothing left to consume. Impact is minimal because the endpoint is static config/discovery (a parse failure is deterministic, not per-request), but parsing/validating the endpoint before consuming the token would be slightly cleaner.

P2-3 — Single cached id_token per uid across devices (note, not a bug)

Save overwrites the per-uid key, so only the most recent login's id_token is retained, and logout consumes that single value. For multi-device sessions, a logout on one device consumes the token and a concurrent logout on another degrades to local-only. This is consistent with the documented best-effort RP-Initiated Logout semantics (any valid recent id_token_hint suffices to terminate the IdP session) — flagging only so it's a conscious design choice rather than an oversight.

3. Suggestions

• Consider the P2-1 hardening (https check or warn on discovery-derived endpoint) as a follow-up.
• The two ops prerequisites in the PR description (register post_logout_redirect_uri on Aegis; confirm whether Aegis end_session requires id_token_hint) are real gating items — ensure they're tracked before enabling in any environment.

4. Additional observations

• The IDTokenTTL env parses with time.ParseDuration (h/m/s only); "7d" silently falls back to the 7d default. This is correctly documented in config.go:90-92 and covered by a test — good catch by the author.
• bindIDTokenKey uses a "bind:" prefix that is namespace-isolated from uid keys, and jti is a 32B base64 token, so no key-collision/injection concern (logout_idtoken.go:628-635).
• Encryption/consume reuse the existing refresh-token Encryptor and state-store luaGetDel rather than introducing new crypto/primitives — consistent with the module and lower risk.

Overall this is a careful, well-tested, opt-in change with a sound trust model. Approving; the P2 items are non-blocking follow-ups, with P2-1 worth a human confirmation given the security-sensitive scope.

[an9xyz]

Addressed the P2 items from the re-review in f60283ef:

• P2-1 — buildEndSessionURL now runs the resolved end_session_endpoint (discovery or config override) through validateLogoutURL, so a non-https endpoint can't carry the id_token over an insecure scheme. Closes the gap where only the config-override/redirect URI were validated at startup.
• P2-2 — reordered to validate + parse the endpoint before the atomic GETDEL, so a (deterministic) parse failure no longer burns the id_token with nothing to retry.
• P2-3 (single cached id_token per uid across devices) — left as-is: intended best-effort RP-Initiated Logout semantics, as you noted.

Tests touching the http httptest mock endpoint set OCTO_OIDC_LOGOUT_ALLOW_INSECURE=1 (models http-in-dev / https-in-prod). go test ./modules/oidc -race (unit + integration) green, go vet / D23 clean.

[lml2468]

APPROVED — re-review at f60283e

This commit addresses yujiawei's suggestion about id_token consumption timing:

1. Validate before consume: validateLogoutURL + url.Parse now run BEFORE idTokens.Take(). If the endpoint is invalid (non-https, unparseable), the id_token is NOT consumed — logout can be retried and still produce a valid end_session_url once the endpoint issue is resolved. Previously, Take() (GETDEL, irreversible) ran first, so an invalid endpoint would silently burn the token.

2. Discovery endpoint validation: validateLogoutURL now covers both config-override AND discovery-sourced end_session_endpoint, catching an IdP that returns http:// in its discovery document.

3. Tests: 3 existing tests using MockProvider (httptest = http://) correctly add OCTO_OIDC_LOGOUT_ALLOW_INSECURE=1 to pass the new validation gate.

The reordering comment is thorough and explains both motivations clearly. No new findings.

[yujiawei]

Code Review — PR #217 (octo-server)

Verdict: APPROVED. This is a careful, well-tested, security-aware implementation of RP-Initiated Logout. No P0/P1 blockers found. A few P2/informational notes below; none gate the merge.

Verification summary

Area	Result	Evidence
Builds	✅	go build ./modules/oidc/ clean
Tests	✅	All 41 PR-specific unit tests pass (-race). Remaining failures in the package are integration tests needing localhost MySQL/Redis (bind_store_redis_test.go:17, TestRedisBindStore_*) — environment-only, untouched by this PR
gofmt / vet	✅	PR files clean; go vet ./modules/oidc/ clean
id_token encrypted at rest	✅	AES-256-GCM, random nonce per encrypt — crypto.go:45-52; ciphertext base64'd before Redis — logout_idtoken.go:60-66
One-time consume (no double-spend)	✅	Atomic Lua GETDEL reused from state store — logout_idtoken.go:73-74, state_store_redis.go:21-29
Open-redirect surface	✅	post_logout_redirect_uri is server-config-only, never read from request body — config.go:80-83, api.go:929-930
https enforced on logout URLs	✅	validateLogoutURL forces absolute https at startup (config) AND at runtime on the discovery-sourced endpoint — config.go:219-242, api.go:907-911
Token not wasted on bad endpoint	✅	Endpoint validated/parsed before Take consumes the id_token — api.go:907-922
No token in logs	✅	Only the endpoint (never the assembled URL/id_token) is logged — api.go:908,914,924
Auth required for logout	✅	uid == "" rejected; token fetched by the authenticated uid only, so no cross-user id_token disclosure — api.go:849 + TestAPI_Logout_NoAuth_Rejected
TTL footgun guard	✅	IDTokenTTL <= 0 clamped to 7d to avoid a never-expiring key — config.go:188-192
Resource cleanup	✅	redisIDTokenStore.Close() released in OIDC.Close() before the early stateStore == nil return — api.go:801-808
Off-by-default / additive	✅	Store only constructed when redirect URI + end_session endpoint are both present — api.go:162-171; logout response byte-for-byte unchanged otherwise

Strengths worth calling out

• Correct ordering of side effects. Validating + parsing the endpoint before the irreversible GETDEL means a transient bad endpoint doesn't burn the user's one-shot id_token; logout stays retryable. (api.go:902-922)
• Runtime re-validation of the discovery endpoint. Even though the override is checked at startup, the discovery-sourced end_session_endpoint is re-validated for https at logout time, so a compromised/misconfigured IdP can't downgrade an id_token-bearing URL to http. (api.go:907-911)
• Honest threat-model documentation. The comment block at api.go:830-838 explicitly acknowledges that id_token_hint is exposed to browser history / Referer / IdP logs by protocol design, and justifies acceptability (one-time, non-replayable, never reused as a bearer). Good security hygiene.
• Bind-path key namespacing. The bind:<jti> staging key is namespaced under the same prefix and promoted to the uid key on confirm/create; uid can't collide with the bind: prefix. (logout_idtoken.go:108-115, api.go:960-983)

P2 / Nits (non-blocking)

1. 7d duration footgun (docs vs. parser). The PR body's config table lists the default as 7d, but OCTO_OIDC_PROVIDER_ID_TOKEN_TTL is parsed with time.ParseDuration, which does not accept a d suffix — setting "7d" silently falls back to the default. The code comment at config.go:90-92 already documents this, but the operator-facing PR/ops docs should show 168h to avoid a silent misconfiguration. Consider either supporting a day suffix or correcting the table.

2. id_token retained up to 7d (encrypted). With the feature enabled, every successful OIDC login caches a PII-bearing signed JWT in Redis for the TTL. Encryption with DM_OIDC_RT_ENC_KEY mitigates at-rest exposure, and the id_token_hint is not itself an auth credential, so impact is low — but worth a human nod on the threat model, especially the blast radius if the encryption key were to leak.

For human reviewer (security_sensitive)

• Confirm the two ops prerequisites in the PR description are tracked before any environment enables the feature: (a) register post_logout_redirect_uri in the Aegis logout-redirect allowlist, and (b) confirm whether Aegis /end_session requires id_token_hint. If Aegis rejects logout requests lacking other params (e.g. client_id/state), the assembled URL may need extension — currently only id_token_hint + post_logout_redirect_uri are sent (api.go:923-926).
• Sanity-check that no other code path persists the raw id_token unencrypted; this review found only the encrypted store path.

[Jerry-Xin]

Scope check: PASS. This PR extends the existing modules/oidc backend logout flow, so it belongs in this repository.

One-line summary: The implementation is mergeable; I found only non-blocking hardening and edge-case concerns.

💬 Non-Blocking

• 🟡 Warning: modules/oidc/api.go constructs the id-token Redis store when an end_session_endpoint merely exists, but the endpoint is not validated until logout. If discovery returns an invalid or insecure endpoint, callbacks will still persist encrypted ID tokens even though logout can never return end_session_url. Consider validating o.endSessionEndpoint() before enabling o.idTokens.
• 🟡 Warning: modules/oidc/api.go returns an end_session_url containing id_token_hint in a JSON response. Since that response now contains sensitive token material, consider setting Cache-Control: no-store and Pragma: no-cache when the field is present.
• 🔵 Suggestion: modules/oidc/logout_idtoken.go stores one ID token per UID. This is simple and matches the current global logout behavior, but in multi-browser or multi-device cases it may use the most recent login’s ID token rather than the current browser session’s token. If Aegis enforces sid matching, this could degrade IdP logout reliability.

✅ Highlights

• Good opt-in behavior: with no post-logout redirect configured, the store stays disabled and the prior logout response remains unchanged.
• The ID token is encrypted at rest, consumed atomically with Lua GET+DEL, and never logged as a full URL.
• Bind confirm/create promotion paths are covered, which closes an easy-to-miss flow.
• Targeted tests passed: go test ./modules/oidc -run 'Test(API_Logout|API_Callback_IDToken|API_Callback_BindPending|BuildEndSessionURL|LoadConfig_IDTokenTTL|ValidateLogoutURL|LoadConfig_RejectsInsecureLogoutURL|Client_EndSessionEndpoint|API_BindConfirm_PromotesIDToken|API_BindCreate_PromotesIDToken)'.

Full go test ./modules/oidc could not complete in this workspace because existing integration tests require local Redis/MySQL on 127.0.0.1.

[Jerry-Xin]

✅ APPROVE

id_token consumption timing fix is clean and correct.

What changed this round:

• buildEndSessionURL now validates the endpoint URL (validateLogoutURL + url.Parse) before calling Take/GETDEL, preventing irreversible token burn on invalid endpoints
• Discovery-sourced end_session_endpoint also runs through the same https validation path
• 3 httptest cases correctly set OCTO_OIDC_LOGOUT_ALLOW_INSECURE=1 for the http mock endpoints

Overall assessment (full PR):

• AES-256-GCM encrypted id_token in Redis with Lua GETDEL atomic consumption — no concurrent double-take window
• PostLogoutRedirectURI server-side only, no open redirect risk
• Feature disabled by default; dual guard (redirect URI + end_session endpoint) controls Redis pool creation
• 365-line test file covers happy path, 4 degradation paths, bind stash/promote, URL escaping, Save/Take errors
• Close() properly releases the Redis connection pool alongside bindStore

💬 Non-blocking:

• If discovery has no end_session_endpoint and no EndSessionURL override is configured, id_token storage is correctly skipped (dual guard in New()), but a startup info-level log noting "RP-Initiated Logout disabled: no end_session endpoint" would aid operator visibility

✅ Highlights: Thorough defense-in-depth — validate-before-consume ordering, per-field scheme enforcement, env-gated http exception for dev, and the saveBindIDTokenHint→promoteBindIDToken migration path for bind flows is well-designed

[an9xyz]

Addressed the latest non-blocking items in af92bdda:

• A — New() now enables the id_token store only when the resolved end_session endpoint is a valid https URL (not merely non-empty), so a non-https/malformed discovery endpoint no longer causes callbacks to persist encrypted id_tokens that logout can't use. Complements the existing runtime check in buildEndSessionURL.
• D — when PostLogoutRedirectURI is set but no usable endpoint is available, an Info log ("RP-Initiated Logout disabled: …") is emitted for operator visibility.
• B — the logout response now sets Cache-Control: no-store + Pragma: no-cache when it carries end_session_url (which embeds id_token_hint), per OAuth/OIDC security BCP / RFC 6749 §5.1. Covered by an assertion in TestAPI_Logout_ReturnsEndSessionURL.

Left as documented best-effort design: one id_token per uid across devices / sid matching (your 🔵 note).

go test ./modules/oidc -race (unit + integration) green, go vet / D23 clean.

[lml2468]

APPROVED — re-review at af92bdd

This commit addresses Jerry-Xin's remaining suggestions:

1. Cache-Control headers: Logout response now sets Cache-Control: no-store + Pragma: no-cache when end_session_url is present (contains id_token_hint). Per RFC 6749 §5.1 and OAuth/OIDC security BCP. Test asserts the header. ✅

2. Startup diagnostic log: When PostLogoutRedirectURI is configured but end_session endpoint is unavailable or non-https, an Info-level log explains why RP-logout is disabled — helps ops diagnose "why is end_session_url missing". ✅

3. Tighter init guard: validateLogoutURL now also runs at idTokenStore construction time (not just at buildEndSessionURL runtime). Non-https endpoint = no Redis pool created = no PII cached. ✅

All reviewer feedback from all rounds has been addressed. check-sprint green. No new findings.

[Jerry-Xin]

Summary: The PR is in scope for octo-server and cleanly extends the existing OIDC logout flow without blocking issues.

💬 Non-blocking

• 🔵 Suggestion — modules/oidc/logout_idtoken.go: Consider adding an integration test for the production redisIDTokenStore to assert encrypted-at-rest storage and Lua-backed one-time consume. The fake store tests cover behavior well, but not the Redis implementation itself.
• 🔵 Suggestion — modules/oidc/api.go: The response correctly sets Cache-Control: no-store and Pragma: no-cache when returning end_session_url. Adding Expires: 0 would be a small extra hardening measure for older intermediaries.

✅ Highlights

• The feature is opt-in and preserves the previous logout response when disabled.
• post_logout_redirect_uri is server-owned, avoiding a request-controlled redirect surface.
• The cached id_token is encrypted and consumed atomically on logout.
• Bind confirm/create paths correctly promote the pending token to the resolved UID.

Verification:

• go test ./modules/oidc could not complete locally because Redis 127.0.0.1:6379 and MySQL 127.0.0.1:3306 were unavailable.
• Targeted new/unit tests passed with go test ./modules/oidc -run 'Test(API_Logout|API_Callback_IDToken|API_Callback_BindPending|BuildEndSessionURL|Client_EndSessionEndpoint|LoadConfig_IDTokenTTL|ValidateLogoutURL|LoadConfig_RejectsInsecureLogoutURL|API_BindConfirm_PromotesIDToken|API_BindCreate_PromotesIDToken)'.

[yujiawei]

Code Review — PR #217 (octo-server)

Scope: RP-Initiated Logout for OIDC. Backend caches the verified id_token at login (AES-256-GCM in Redis) and assembles an end_session_url (id_token_hint + server-configured post_logout_redirect_uri) on logout. 10 files, all under modules/oidc/, +909/-8.

Verdict: APPROVED. No P0/P1. The design is sound, the security-sensitive paths are handled carefully, and test coverage is thorough. A few optional notes below — none block merge.

1. Verification of the PR's claims

Claim	Result	Evidence
id_token cached only after verification	✅	api.go:478 VerifyIDToken runs before any Save; callback caches at api.go:773-779 using the already-verified rawID.
One-time, atomic consumption	✅	logout_idtoken.go:74 reuses luaGetDel (atomic GET+DEL) from state_store_redis.go:21; concurrent double-logout cannot double-spend.
Encrypted at rest, never logged	✅	logout_idtoken.go:54-66 AES-256-GCM via the shared Encryptor (random nonce per encrypt, crypto.go:46-52); all error logs record only the endpoint/trace, never the token or assembled URL (api.go:946-957).
TTL clamped to >0	✅	config.go:188-191 clamps <=0 to 7d; covered by TestLoadConfig_IDTokenTTL (zero/negative/invalid-7d cases).
No open-redirect from request body	✅	post_logout_redirect_uri is only ever read from o.cfg.Provider.PostLogoutRedirectURI (api.go:962); never bound from the request. Server config doubles as the allowlist.
https-only, covering discovery-sourced endpoints	✅	validateLogoutURL (config.go:226) enforces absolute https at startup for config values, AND buildEndSessionURL re-validates the resolved endpoint at request time (api.go:945) — so a malicious/misconfigured IdP discovery doc returning http:// or javascript: cannot leak the token.
Cache-Control: no-store on token-bearing responses	✅	Set only when end_session_url is present (api.go:898-901).
Purely additive / opt-in, byte-for-byte unchanged when off	✅	With OCTO_OIDC_POST_LOGOUT_REDIRECT_URI unset, the store is never constructed (api.go:167), o.idTokens stays nil, and logout returns exactly {"status":200} with no extra headers. Covered by TestAPI_Logout_NilStore_OmitsURL / TestAPI_Logout_NoRedirectConfig_OmitsURL.
Redis pool released on shutdown	✅	Close() releases the dedicated id_token client (api.go:809-817), correctly placed before the stateStore == nil early return.
Bind-path stash→promote	✅	callback stashes under bind:<jti> (api.go:592), promoted to the uid key on bindConfirm/bindCreate success (api_bind.go:339,489); one-time consumed via Take. Covered by TestAPI_BindConfirm_PromotesIDToken / TestAPI_BindCreate_PromotesIDToken.

Build clean (go build ./modules/oidc/...); targeted tests pass (go test ./modules/oidc -run 'Logout|EndSession|IDToken|Config|Promote|Bind…').

2. Security analysis (this PR is auth/token-sensitive)

• Token-in-URL exposure is inherent to RP-Initiated Logout (id_token_hint is an RFC front-channel parameter) and is correctly bounded: the cached token is single-use, atomically invalidated on read, never reused by the server as a bearer/assertion, and never logged. This is the right tradeoff and is well-documented in the code.
• Key-namespace isolation between normal entries (oidc:idtoken:<uid>) and bind stash (oidc:idtoken:bind:<jti>) is collision-free: uids never start with bind:, and jti is 32 random bytes (bind_service.go:731). The req.Token used by promote is validated by authcodeRe before use.
• Graceful degradation — every new failure path (encrypt/save fail at login, take/decrypt fail at logout, missing endpoint, nil store) degrades to "local-only logout" and never blocks the login or logout response. TestAPI_Callback_IDTokenSaveError_LoginStillSucceeds confirms login is unaffected.
• Store gating correctly requires both a configured redirect URI AND a usable https end_session endpoint (api.go:167-181) before persisting any PII-bearing JWT — no token is stored if the feature can't actually emit a URL.

An adversarial pass across token-lifecycle, open-redirect/leak, crypto/resource, and degradation dimensions surfaced no confirmed defects.

3. Optional notes (non-blocking)

• (suggestion) id_token_hint size vs. URL length. Aegis id_tokens embedding identity claims can be large; the assembled end_session_url could approach front-channel URL-length limits in some browsers/IdPs. Worth a quick check against the real Aegis token size before enabling in prod, but not a code change.
• (nit) Ops checklist already in the PR body — registering post_logout_redirect_uri on the Aegis allowlist and confirming whether Aegis requires id_token_hint are the two real go-live gates. Flagging for the human enabling this in an environment.

4. Items for human verification before enabling

1. Register the configured post_logout_redirect_uri in the Aegis (IdP) logout-redirect allowlist.
2. Confirm Aegis end_session accepts the assembled parameters (id_token_hint, post_logout_redirect_uri) and whether state is required (this PR intentionally omits state since Aegis discovery doesn't declare it).

No blocking issues found. Approving.