fix(oidc): invalidate current token on logout

[an9xyz]

Summary

• invalidate the current HTTP API token during OIDC logout
• remove matching uidtoken reverse index only when it atomically still points to the current token
• prevent Web/PC login from recreating a token that logout has already deleted
• add logout token_fail metrics label and regression coverage

Closes #227

Tests

• go test ./modules/user -run TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken
• go test ./modules/oidc -run 'TestAPI_Logout|TestCurrentTokenInvalidator|TestMetrics_LogoutTotal_Labels'
• git diff --check

Notes

• This intentionally invalidates only the current request token, not every token for the UID.

[an9xyz]

Addressed the blocking token resurrection race flagged in review.

What changed:

• Web/PC token reuse now refreshes an existing token with Redis SET XX semantics.
• If token: was already deleted by logout, login does not recreate it; it mints a fresh token instead.
• Added regression coverage: TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken.

Verification:

• go test ./modules/user -run TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken
• go test ./modules/oidc -run 'TestAPI_Logout|TestMetrics_LogoutTotal_Labels'
• git diff --check

[an9xyz]

Rebased on latest origin/main and pushed with --force-with-lease.

Also addressed the post-8277912 blocking feedback from Jerry-Xin:

• uidtoken reverse-index cleanup now uses an atomic Redis Lua compare-and-delete operation (GET == currentToken then DEL inside one script).
• Added regression coverage: TestCurrentTokenInvalidator_DoesNotDeleteConcurrentUIDTokenUpdate.

Verification after the change:

• go test ./modules/user -run TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken
• go test ./modules/oidc -run 'TestAPI_Logout|TestCurrentTokenInvalidator|TestMetrics_LogoutTotal_Labels'
• git diff --check

[Jerry-Xin]

✅ Re-Review: APPROVE

All three blocking issues from the previous rounds are now fixed:

1. Lua CAS atomic delete — uidtoken cleanup uses a Lua script (GET == want THEN DEL) instead of the non-atomic GET + DEL sequence. Concurrent login updating uidtoken between the two operations can no longer cause logout to delete the new session's reverse index.

2. Login-side SETXX — refreshExistingLoginToken uses Redis SET XX (set only if key exists). If logout has already deleted token:<old>, SETXX returns false and login generates a fresh UUID. No more token resurrection.

3. Redis client lifecycle — OIDC.Close() explicitly closes the redisCompareDeleter's Redis client. No resource leak.

Test coverage is solid:

• raceyCompareDeleter simulates concurrent login updating uidtoken during logout cleanup — verifies new login's index survives
• TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken — verifies deleted token key is not recreated by concurrent login
• TestAPI_Logout_InvalidatesCurrentHTTPTokenOnly — end-to-end: current token invalidated, other device tokens untouched

The complete chain is now race-free:

• Logout deletes token:<current> ✅
• Concurrent login SETXX detects missing key → generates new token ✅
• Logout Lua CAS prevents stale uidtoken deletion ✅
• Resources properly released ✅

Clean fix. Approve.

[yujiawei]

Code Review — PR #225 (octo-server)

Security-sensitive review of the OIDC logout token-invalidation change. I checked out the head SHA, built the changed modules, ran the new unit tests, and traced every login path that reuses a cached token. The OIDC-side fix and the execLogin (web/PC) fix are correct and well-tested. However, one token-reuse login path was left unpatched and reopens the exact resurrection window this PR is meant to close, so I'm requesting changes.

Verdict

CHANGES_REQUESTED — one P1 (logout-bypass via a second, unfixed login path). The rest of the change is solid.

What is correct (verified)

• OIDC.logout → InvalidateCurrentToken (modules/oidc/api.go:915, :1360): deletes token:<current> (the key AuthMiddleware reads — confirmed AuthMiddleware uses c.GetHeader("token") and looks up tokenPrefix+token), then atomically compare-deletes the uidtoken:<flag><uid> reverse index via a Lua GET==ARGV[1] then DEL script (:82, DeleteIfValue at :1409). The compare-and-delete correctly avoids clobbering a concurrent re-login that already rotated the index to a new token. TestCurrentTokenInvalidator_DoesNotDeleteConcurrentUIDTokenUpdate exercises this race and passes.
• execLogin web/PC reuse (modules/user/api.go:1512, :1528–:1546): reused tokens are now refreshed with SET ... XX (refreshExistingLoginToken → SetIfExists). In go-redis v6 (v6.15.9) SetXX maps the nil reply to (false, nil) (verified in BoolCmd.readReply), so when token:<oldToken> was already deleted by logout, the path falls through to a fresh UUID instead of recreating the dead key. Legitimate web multi-login is preserved (key exists → XX refresh succeeds → same token). TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken covers this and passes.
• Same Redis keyspace: the two new raw rd.NewClient(MustBuildOptions(...)) clients use the same Addr/Pass and DB 0 as ctx.Cache() (common.NewRedisCache(RedisAddr, RedisPass)), and follow the established repo pattern (state_store_redis.go, sync_lock.go, etc.). The OIDC client is closed via the module Stop: o.Close wiring.
• Build of ./modules/oidc/... ./modules/user/... is clean; gofmt/go vet clean on changed files. The new metric label token_fail and the independent-counter refactor in logout are correct (each failure dimension now increments independently).

P1 — Blocking: scan-login path still resurrects a logged-out token

loginWithAuthCode (the QR scan-login handler, route POST /user/login_authcode/:auth_code, modules/user/api.go:1982) reuses the cached token the same way execLogin used to — but it was not given the SetXX treatment:

// modules/user/api.go:2024
token, err := u.ctx.Cache().Get(fmt.Sprintf("%s%d%s", ...UIDTokenCachePrefix, flag, scaner))
...
if strings.TrimSpace(token) == "" {        // :2031
    token = util.GenerUUID()
}
...
// modules/user/api.go:2114  — unconditional, recreates the key
err = u.ctx.Cache().SetAndExpire(...TokenCachePrefix+token, tokenPayload, ...TokenExpire)

This is the identical TOCTOU the PR fixes elsewhere. logout deletes token:<current> first and compare-deletes the uidtoken index second; in the window between those two steps a concurrent scan-login (default flag = Web, modules/user/api.go:1988) for the same uid reads the still-present uidtoken:1<uid>, gets the just-logged-out token string, and re-SetAndExpires token:<that token> — resurrecting the session. Logout returned 200 but the bearer token works again, which is exactly the logout-bypass this PR exists to prevent. The path is a live, rate-limited route (modules/user/api.go:297), reachable for any uid including OIDC-provisioned users.

Required change: apply the same guard to loginWithAuthCode — reuse the cached token only if token:<oldToken> still exists. The cleanest option is to route it through the same SET XX helper used by execLogin (refreshExistingLoginToken), falling back to a fresh UUID when the refresh returns false. Pure reordering inside logout would only shrink the window, not close it.

P2 / non-blocking

• redisExistingTokenSetter's client (modules/user/api.go:158) is never closed — User has no Close. It's a process-lifetime singleton created once at startup, so not a leak in practice, but worth a comment or a Close hook for symmetry with the OIDC side.
• Two independent Redis clients are now created at startup (OIDC redisCompareDeleter + user redisExistingTokenSetter), each with its own pool. Consistent with the existing per-store-client pattern in this repo, so acceptable; just noting the growing client count.
• InvalidateCurrentToken issues a compare-del for all three device flags on every logout (up to 3 Redis round trips). Fine for logout volume; only flagging for awareness.

Manual-verification call-outs for a human reviewer (security-sensitive)

• Confirm the threat model treats scan-login (loginWithAuthCode) as in-scope for "a logged-out token must stay dead." If it is (it should be), the P1 above must be fixed before merge.
• Confirm in the deployed config that Redis runs on DB 0 for both ctx.Cache() and the new raw clients (the options builder sets no DB index — it inherits the default 0, matching ctx.Cache()).

[lml2468]

Re-review (261fff1)

New commit fix(user): guard scan-login token reuse with SET XX addresses Allen's blocking point about the QR code login path.

What changed

loginWithAuthCode now mirrors the execLogin SETXX pattern:

1. reuseExistingToken flag set when uidtoken yields a non-empty old token
2. refreshExistingLoginToken (SETXX) checks if token:<old> still exists before reuse
3. If SETXX returns false (deleted by concurrent logout), generates new UUID
4. Token decision moved BEFORE UpdateIMToken — IM always gets the final token

Tests

• Source guard (api_authcode_token_test.go): Verifies refreshExistingLoginToken call exists in loginWithAuthCode, is before UpdateIMToken, and !reuseExistingToken fallback is present. Regression-proof.
• E2E (api_authcode_e2e_test.go): Real Redis, two cases: (a) deleted token → new UUID issued, old not resurrected; (b) existing token → properly reused (Web multi-device preserved).

Full chain now covers ALL login paths

• execLogin (password/SMS): SETXX ✅
• loginWithAuthCode (QR scan): SETXX ✅
• Logout: token delete + Lua CAS uidtoken cleanup ✅
• Redis client lifecycle ✅

No remaining gaps. APPROVED.

[Jerry-Xin]

✅ Re-Review: APPROVE

All blocking issues resolved across four rounds:

1. Token invalidation — logout correctly deletes token:<current> ✅
2. Login SETXX (execLogin) — Web/PC token reuse guarded by SET XX; deleted tokens cannot be resurrected ✅
3. Login SETXX (loginWithAuthCode) — scan-login path now uses the same refreshExistingLoginToken guard ✅
4. Lua CAS atomic uidtoken cleanup — GET == want THEN DEL prevents concurrent login's index from being deleted ✅
5. Redis client lifecycle — OIDC.Close() properly closes the compare-deleter client ✅

Test coverage is thorough:

• Unit: raceyCompareDeleter simulates concurrent uidtoken update during logout
• Unit: TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken
• Source guard: locks refreshExistingLoginToken before UpdateIMToken in authcode path
• E2E: real Redis validates both invariants (deleted → new token / existing → reuse)

All login entry points now have SETXX guards. No gaps remain.

[yujiawei]

Code Review — PR #225 (octo-server)

Verdict: APPROVED — security-sensitive (auth/token). No blocking (P0/P1) issues found. A couple of non-blocking notes below.

Summary of the change

This PR closes a real session-fixation / logout-bypass gap: previously OIDC logout only quit the IM connection and revoked refresh tokens, leaving the HTTP API token (token:<token>) live, and a concurrent or subsequent Web/PC/scan login could re-create a token that logout had just deleted. The fix:

1. OIDC logout now invalidates the current HTTP token (cacheCurrentTokenInvalidator): deletes token:<token>, then for each device flag compare-deletes the uidtoken:<flag><uid> reverse index only if it still points at the current token (atomic Redis Lua compare-del, with a non-atomic Get+Delete fallback for the in-memory test path). Correctly scoped to the current device — it does not nuke other devices' tokens.
2. Login paths close the resurrection race (execLogin Web/PC reuse branch and loginWithAuthCode): when reusing a token reverse-looked-up from uidtoken, the write now uses SET XX (SetIfExists). If the token key was already deleted by a concurrent logout, SET XX fails and the path falls back to a fresh UUID instead of resurrecting the logged-out token. loginWithAuthCode was reordered so the final token decision happens before UpdateIMToken, ensuring IM receives the token that is actually persisted.
3. Metrics: adds a token_fail label and refactors the logout result switch so token/kick/revoke failures are counted independently in the same request.

1. Verification

• ✅ Logout actually invalidates the session token. logout → o.tokenKill.InvalidateCurrentToken(ctx, uid, c.GetHeader("token")) (modules/oidc/api.go:914-921). The token header is the same one AuthMiddleware authenticates with (octo-lib .../wkhttp/http.go:271, c.GetHeader("token")), and logout is gated on uid != "" (api.go:893-897), so the value is a post-auth session token.
• ✅ uidtoken index is only removed when it still points at the current token. Lua compare-del is atomic (api.go:37-42, 197-210); a concurrent new login that rewrote the index is preserved. Covered by TestCurrentTokenInvalidator_DoesNotDeleteConcurrentUIDTokenUpdate and TestAPI_Logout_InvalidatesCurrentHTTPTokenOnly.
• ✅ Login does not resurrect a logged-out token. SET XX + fallback-to-new-UUID in both execLogin (api.go:1525-1552) and loginWithAuthCode (api.go:2105-2128). Covered by TestRefreshExistingLoginToken_DoesNotRecreateDeletedToken and the contract/e2e guards.
• ✅ Token decision precedes UpdateIMToken in loginWithAuthCode — verified by source-contract test TestLoginWithAuthCode_ReusedTokenGuardedBySetXX.
• ✅ Web/PC multi-login not broken — reuse path still returns the old token when it remains valid (TestLoginWithAuthCode_E2E_TokenResurrection "token仍存在时_正常复用旧token").
• ✅ Both Redis clients (cache + compare-deleter) target the same RedisAddr/DB (pkg/redis/options.go:24-44 vs octo-lib config/context.go:107-121), so the compare-delete sees the same keys as the cache. The OIDC compare-deleter client is properly closed in OIDC.Close() (api.go:72-79).
• ✅ go build / go vet / gofmt / git diff --check clean; the dependency-free unit tests pass.

2. Findings

No P0/P1. Two non-blocking notes:

P2 — redisExistingTokenSetter Redis client has no close path (modules/user/api.go:158-162). User.New() creates a dedicated rd.Client that is never closed, and User has no Close() method. In practice this is not a leak that grows: User is a process-lifetime singleton registered once via register.AddModule, and the module already holds an identical never-closed client (rlRedis, api.go:191), so this follows existing precedent rather than introducing new behavior. Worth a follow-up to give User a Close() that releases both clients for shutdown hygiene, but it is not a merge blocker for this PR.

nit — neutral observation, pre-existing, out of scope. loginWithAuthCode's token payload omits Role (api.go:2095-2099) whereas execLogin includes it (api.go:1517-1522). This is not introduced by this PR — the same omission exists on main (the PR only relocated the auth.Encode block, it did not change its fields). Flagging only so it isn't mistaken for a regression here; if QR-login role propagation matters it deserves its own ticket.

3. Recommendations

• (Optional, follow-up) Add a User.Close() that closes existingTokenSetter and rlRedis, mirroring OIDC.Close().
• (Optional, separate ticket) Decide whether scan-login tokens should carry Role for consistency with execLogin.

4. Additional notes for a human verifier (security-sensitive)

• The fix deliberately invalidates only the current device's HTTP token, not all tokens for the UID — confirm this matches the intended logout semantics (single-device logout). The code comments state this is intentional.
• The atomicity guarantee against the login/logout race rests on the Redis SET XX and Lua compare-del executing against the live Redis. The in-memory MemoryCache fallback (deleteIndexIfCurrentToken Get+Delete) is test-only and non-atomic — fine because production always injects the Lua-backed redisCompareDeleter.

Overall this is a focused, well-tested security fix that closes the logout-bypass / token-resurrection gap correctly. Approving.

[lml2468]

Re-review (ca5e246)

Same scan-login SETXX fix as 261fff, rebased. E2E test replaced with a CI-portable real-Redis integration test (api_authcode_token_redis_test.go) that validates SETXX semantics directly without WuKongIM dependency. Source guard and logic unchanged.

APPROVED — same assessment as previous review.

[Jerry-Xin]

✅ Re-Review: APPROVE

Delta from 261fff → ca5e24: rebase only, scan-login SETXX logic unchanged. E2E split into api_authcode_token_redis_test.go (real Redis) + api_authcode_token_test.go (source guard). No new issues.

All login entry points guarded, full chain race-free. Approve stands.

[yujiawei]

Code Review — PR #225 (octo-server)

Scope: fix(oidc): invalidate current token on logout — head ca5e246, base f3793b3.
Security-sensitive (auth/session). Reviewed against all token reuse/invalidation paths in the repo, not just the diff.

Verdict: APPROVED

The PR delivers its stated guarantee — a logged-out HTTP token cannot be resurrected — and closes the cross-file TOCTOU window across all three relevant paths. No blocking (P0/P1) issues found. A few non-blocking nits below.

What the change does

1. OIDC logout now invalidates the current HTTP session token (modules/oidc/api.go). device_quit only tears down the IM long-connection; HTTP API access is still gated by token:<token> in Redis, so logout must explicitly delete it.
   • InvalidateCurrentToken deletes token:<current> and then, for each device flag, atomically compare-deletes the uidtoken:<flag><uid> reverse index via a Lua GET == ARGV then DEL script (redisCompareDeleter). This avoids clobbering a concurrent re-login that has already repointed the index to a new token.
   • Only the current token is removed, not all of the user's tokens — other devices stay logged in. Verified by TestAPI_Logout_InvalidatesCurrentHTTPTokenOnly.
2. Login paths no longer resurrect a deleted token. Both web/PC reuse paths now refresh the reused token with SET XX (refreshExistingLoginToken → SetIfExists), so a token is only rewritten if token:<oldToken> still exists; otherwise they fall back to a fresh UUID:
   • execLogin (modules/user/api.go:1528-1551)
   • loginWithAuthCode / scan login (modules/user/api.go:2105-2127)

Verification

Item	Result	Evidence
Logout deletes current token: + matching uidtoken: index only	✅	modules/oidc/api.go:1362-1397; TestAPI_Logout_InvalidatesCurrentHTTPTokenOnly
Reverse-index delete is atomic (no clobber of concurrent re-login)	✅	Lua compare-del api.go:78-83; TestCurrentTokenInvalidator_DoesNotDeleteConcurrentUIDTokenUpdate
execLogin web/PC reuse guarded by SET XX with UUID fallback	✅	modules/user/api.go:1528-1551
Scan-login (loginWithAuthCode) reuse guarded by SET XX with UUID fallback	✅	modules/user/api.go:2105-2127; source-contract lock TestLoginWithAuthCode_ReusedTokenGuardedBySetXX
Token-reuse decision finalized before the IM call (IM gets the final token)	✅	token cache write moved ahead of UpdateIMToken; asserted by the source-contract test
SET XX on a missing key returns (false, nil), not an error → fallback fires	✅	go-redis v6.15.9 BoolCmd.readReply maps redis.Nil → val=false, err=nil
Raw redis clients share the same keyspace as ctx.Cache()	✅	all clients default to DB 0; BuildOptions sets only Addr/Pass/TLS, NewRedisCache likewise
uidtoken cleanup covers the full DeviceFlag enum	✅	loop over {APP, Web, PC}; enum has exactly those three (octo-lib config/msg.go:32-37)
Logout route is behind AuthMiddleware (uid + token header guaranteed)	✅	modules/oidc/api.go:319-320
Logout token header matches the key AuthMiddleware reads	✅	both use c.GetHeader("token") (octo-lib pkg/wkhttp/http.go:307)
OIDC raw redis client is closed on shutdown	✅	OIDC.Close() closes redisCompareDeleter
Metrics gain a token_fail label; counter logic emits each failed dimension	✅	metrics.go; logout_total label set + test updated
Build / vet	✅	go build + go vet on both modules clean
Unit tests (infra-independent)	✅	new SET XX / contract / invalidator tests pass; integration tests are env-gated on MySQL/Redis

Non-blocking notes (nits / for a human to eyeball)

1. Orphan token key on a failed IM call (nit). In loginWithAuthCode, the token cache write now precedes UpdateIMToken. If the IM call errors or returns Ban after a new-UUID branch wrote token:<uuid>, that key lingers until its TTL expires. It is never returned to any client and the uidtoken index is not written, so there is no security impact — just a short-lived orphan. The reuse/SET XX branch only refreshes an already-valid token, so it is benign. Optional: write the token cache after the IM success check, or clean up on the error path.
2. Raw redis client lifecycle in User (nit). redisExistingTokenSetter's client is never closed (no User.Close). This matches the existing rate-limiter client in the same file (modules/user/api.go:191), so it is a pre-existing, process-lifetime-singleton pattern rather than a regression. OIDC's side does close its client.
3. Security note for a human reviewer. The correctness of the cross-client compare-delete rests on all redis clients targeting the same DB (DB 0). That holds today because neither BuildOptions nor NewRedisCache sets a DB index. Worth keeping in mind if a non-zero db is ever introduced into the redis config.

Conclusion

This is a clean, well-tested fix. It addresses the previously-identified second resurrection path (scan login) in addition to the OIDC + web/PC paths, the atomic compare-delete correctly avoids racing a concurrent re-login, and the test suite locks the key invariants (including a source-contract guard so the SET XX guard cannot be silently regressed). Approving; the nits above are optional follow-ups, not merge blockers.