feat(backend): Discord OAuth login with DB-backed sessions (#23) #31

2026-08-08T08:31:58+07:00

sulthan commented

2026-08-08 08:31:58 +07:00

Implements #23 per ADR-0002.

Discord authorization code grant (identify + guilds.members.read), form-encoded token exchange
Guild membership gate via the single-guild endpoint; optional DISCORD_REQUIRED_ROLE (empty default)
Owner Discord ID is the only identity allowed to sign in
Sessions are DB rows with opaque random ids; cookie carries only the id; expiry enforced; delete = revoke
HMAC session signing, derived key, and WEB_PASSWORD removed; no replacement signing secret
Login rate limiting preserved on the callback
Full flow tested through the real router against a local Discord stub (DISCORD_API_BASE)
Env: DISCORD_CLIENT_ID/_CLIENT_SECRET/_GUILD_ID/_REQUIRED_ROLE/_API_BASE/_REDIRECT_URI; docs updated

go test ./... passes.

Implements #23 per ADR-0002. - Discord authorization code grant (identify + guilds.members.read), form-encoded token exchange - Guild membership gate via the single-guild endpoint; optional DISCORD_REQUIRED_ROLE (empty default) - Owner Discord ID is the only identity allowed to sign in - Sessions are DB rows with opaque random ids; cookie carries only the id; expiry enforced; delete = revoke - HMAC session signing, derived key, and WEB_PASSWORD removed; no replacement signing secret - Login rate limiting preserved on the callback - Full flow tested through the real router against a local Discord stub (DISCORD_API_BASE) - Env: DISCORD_CLIENT_ID/_CLIENT_SECRET/_GUILD_ID/_REQUIRED_ROLE/_API_BASE/_REDIRECT_URI; docs updated go test ./... passes.

sulthan added 1 commit 2026-08-08 08:31:58 +07:00

feat(backend): Discord OAuth login with DB-backed sessions (#23 ) 13e8e73da7

The browser UI signs in with a Discord authorization code grant
(identify + guilds.members.read) instead of a shared password. Guild
membership is the gate; the owner's Discord ID is the only identity
allowed in while registration is closed. Sessions become rows in a
sessions table with opaque random ids — the cookie carries only the id,
looked up and expiry-checked per request — so deleting a row revokes a
session. HMAC cookie signing, its derived key, and WEB_PASSWORD are
gone, and no replacement signing secret is introduced (ADR-0002).

Discord's API base is configurable (DISCORD_API_BASE); the full flow is
tested through the real router against a local stub, including the
form-encoded token exchange Discord rejects if sent as JSON.

sulthan referenced this pull request

2026-08-08 08:41:55 +07:00

Log in with Discord #23

sulthan added 1 commit 2026-08-08 08:47:13 +07:00

refactor(web): harden oauth state store and session writes after review 1a7e130f9f

- Drop the FIFO from oauthStates: consumed states left entries behind, so
  an unrate-limited start/cancel cycle grew the slice without bound.
  Evict by oldest expiry instead — the map alone now bounds memory.
- CreateSession runs INSERT + expiry sweep in one transaction.
- slices.Contains replaces a hand-rolled contains; APIBase typo fixed.
- Stale comments and test paths updated; login hover uses --ember-ink.

sulthan commented

2026-08-08 08:47:18 +07:00

Two-axis review (Standards + Spec) done. Spec: PASS on all 12 acceptance criteria + no-markup/no-secret addendum, evidence per criterion in review. Standards: near-clean pass; the one medium finding (oauthStates FIFO could grow without bound via start/cancel cycles) plus 8 low/nit findings all fixed in 1a7e130 — FIFO replaced with oldest-expiry eviction, CreateSession now transactional, slices.Contains, APIBase rename, stale comments/paths, --ember-ink. go test ./... green.

Two-axis review (Standards + Spec) done. Spec: PASS on all 12 acceptance criteria + no-markup/no-secret addendum, evidence per criterion in review. Standards: near-clean pass; the one medium finding (oauthStates FIFO could grow without bound via start/cancel cycles) plus 8 low/nit findings all fixed in 1a7e130 — FIFO replaced with oldest-expiry eviction, CreateSession now transactional, slices.Contains, APIBase rename, stale comments/paths, --ember-ink. go test ./... green.

sulthan commented

2026-08-08 08:50:32 +07:00

Review round 1 — security review of the initial implementation (verdict: APPROVE-WITH-NITS). Findings and what was done, all folded into the initial commit 13e8e73:

[low] oauthStates eviction popped the FIFO front blindly — but take() removes states from the map, not the FIFO, so the front could already be consumed and the map could settle at ~2x its documented cap. Fixed: the eviction loop pops until the count actually drops.
[low] Expired session rows were deleted only when looked up; a device whose cookie was cleared without logout left its row forever. Fixed: CreateSession sweeps rows with expires_at < now() on every login — no background job (later moved into a transaction, see round 2).
[nit] Discord cancellation (?error=access_denied) rendered the misleading 'invalid or already used' and left the state unconsumed. Fixed: dedicated 'Sign-in was cancelled.' message, state consumed, and the path deliberately does not count against the rate limiter (it makes no Discord call and grants nothing).
[test gaps] No positive-path test for DISCORD_REQUIRED_ROLE (the role check branch never ran in a passing test); the member-endpoint-403 refusal was untested; refused callbacks were not asserted to make zero outbound calls; oauthStates TTL/eviction were untested. Fixed: TestDiscordLoginRequiresRolePositive, the member-403 case in TestDiscordLoginRefusesNonMember, zero-request assertions in the state-rejection tests, and internal/web/oauth_test.go.

go test ./... green.

Review round 1 — security review of the initial implementation (verdict: APPROVE-WITH-NITS). Findings and what was done, all folded into the initial commit 13e8e73: 1. [low] oauthStates eviction popped the FIFO front blindly — but take() removes states from the map, not the FIFO, so the front could already be consumed and the map could settle at ~2x its documented cap. Fixed: the eviction loop pops until the count actually drops. 2. [low] Expired session rows were deleted only when looked up; a device whose cookie was cleared without logout left its row forever. Fixed: CreateSession sweeps rows with expires_at < now() on every login — no background job (later moved into a transaction, see round 2). 3. [nit] Discord cancellation (?error=access_denied) rendered the misleading 'invalid or already used' and left the state unconsumed. Fixed: dedicated 'Sign-in was cancelled.' message, state consumed, and the path deliberately does not count against the rate limiter (it makes no Discord call and grants nothing). 4. [test gaps] No positive-path test for DISCORD_REQUIRED_ROLE (the role check branch never ran in a passing test); the member-endpoint-403 refusal was untested; refused callbacks were not asserted to make zero outbound calls; oauthStates TTL/eviction were untested. Fixed: TestDiscordLoginRequiresRolePositive, the member-403 case in TestDiscordLoginRefusesNonMember, zero-request assertions in the state-rejection tests, and internal/web/oauth_test.go. go test ./... green.

sulthan commented

2026-08-08 08:50:40 +07:00

Review round 2 — two-axis review (Standards + Spec) of 13e8e73.

Spec axis: PASS on all 12 acceptance criteria plus the no-markup/no-secret addendum, each backed by test evidence: the stub-driven full-flow test asserts the form-encoded token exchange field-by-field, the single-guild membership endpoint path, refusal parity (non-member / missing role / member-403 all identical), the reader-count side-effect check, session expiry, logout revoke, zero remaining signing code, callback rate limiting, and that no Discord-supplied string is ever rendered.

Standards axis: near-clean; 1 medium + 8 low/nit findings, all fixed in 1a7e130 (force-pushed over 13e8e73):

[medium] oauthStates.order grew without bound: take() left its FIFO entry behind and the eviction loop ran only when the map was full, so an unrate-limited /auth/discord + cancel cycle appended one entry forever (~100B/entry, ~10M cycles ≈ 1GB) — defeating the documented memory cap. Fixed: the FIFO is gone; put() evicts the state closest to expiring by scanning the map, so the map alone bounds memory. TestOAuthStateEviction asserts the cap still holds after consume+flood.
[low] CreateSession ran INSERT + expiry sweep as two Execs outside a transaction; a failed sweep returned an error while the session row silently persisted. Fixed: both statements in one transaction (matches Store.Upsert's convention).
[low] Hand-rolled contains() replaced with slices.Contains (stdlib, already used in the repo).
[low] Stale LoginLimiter comment still said 'mistyped password' — the password login no longer exists. Reworded.
[low] Migration 0005 comment claimed 'nothing sweeps them' while CreateSession sweeps. Corrected.
[nit] APIBBase typo → APIBase (public field; renamed across main.go, api_test.go, web_test.go).
[nit] Login button hover hardcoded #fff → var(--ember-ink) (token exists in both colour branches).
[nit] GetSession swallowed the expired-row delete error → marked best-effort with a comment.
[nit] Cookie tests still pointed at the removed /login route → now '/'.

go test ./... green after all fixes.

Review round 2 — two-axis review (Standards + Spec) of 13e8e73. Spec axis: PASS on all 12 acceptance criteria plus the no-markup/no-secret addendum, each backed by test evidence: the stub-driven full-flow test asserts the form-encoded token exchange field-by-field, the single-guild membership endpoint path, refusal parity (non-member / missing role / member-403 all identical), the reader-count side-effect check, session expiry, logout revoke, zero remaining signing code, callback rate limiting, and that no Discord-supplied string is ever rendered. Standards axis: near-clean; 1 medium + 8 low/nit findings, all fixed in 1a7e130 (force-pushed over 13e8e73): 1. [medium] oauthStates.order grew without bound: take() left its FIFO entry behind and the eviction loop ran only when the map was full, so an unrate-limited /auth/discord + cancel cycle appended one entry forever (~100B/entry, ~10M cycles ≈ 1GB) — defeating the documented memory cap. Fixed: the FIFO is gone; put() evicts the state closest to expiring by scanning the map, so the map alone bounds memory. TestOAuthStateEviction asserts the cap still holds after consume+flood. 2. [low] CreateSession ran INSERT + expiry sweep as two Execs outside a transaction; a failed sweep returned an error while the session row silently persisted. Fixed: both statements in one transaction (matches Store.Upsert's convention). 3. [low] Hand-rolled contains() replaced with slices.Contains (stdlib, already used in the repo). 4. [low] Stale LoginLimiter comment still said 'mistyped password' — the password login no longer exists. Reworded. 5. [low] Migration 0005 comment claimed 'nothing sweeps them' while CreateSession sweeps. Corrected. 6. [nit] APIBBase typo → APIBase (public field; renamed across main.go, api_test.go, web_test.go). 7. [nit] Login button hover hardcoded #fff → var(--ember-ink) (token exists in both colour branches). 8. [nit] GetSession swallowed the expired-row delete error → marked best-effort with a comment. 9. [nit] Cookie tests still pointed at the removed /login route → now '/'. go test ./... green after all fixes.

sulthan merged commit bcc6b45515 into main

2026-08-08 08:51:23 +07:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sulthan/mangaBookmark#31