Closes#5. Phase 1, step 2 of the tracking epic #20. Builds on #4 (same session cookie for both paths).
What's here
API (internal/api/oidc.go)
GET /auth/oidc/login — authorization-code + PKCE (S256) with random state + nonce, stashed in a short-lived HttpOnly pansy_oidc_tx cookie (SameSite=Lax so it survives the IdP's top-level redirect back), then redirects to the IdP.
GET /auth/oidc/callback — verifies state (constant-time), exchanges the code with the PKCE verifier, verifies the ID token signature/issuer/audience/expiry and nonce, then provisions and starts a pansy session. Every failure clears the tx cookie and redirects to /login?error=…; success → /gardens.
Lazy discovery (oidcClient.ensure): first request performs discovery (10s timeout) and caches it; a failure is retried on the next request, so a momentarily-unreachable IdP never stops the server — or local auth — from starting.
Routes are registered only when OIDC is ready (issuer + client ID + PANSY_BASE_URL), so an unconfigured instance 404s them, matching what /auth/providers advertises.
Service (internal/service/auth.go) — LoginOIDC provisioning order:
(issuer, subject) match → returning user;
else a verified email match → linked onto the existing account (no split local/OIDC accounts);
else JIT-create (IdP gates access, so PANSY_REGISTRATION is bypassed) — reusing #4's atomic CreateUser (first user still becomes admin atomically).
Security choices: an unverified email that collides with an existing account is refused (account-takeover guard); an identity with no email is refused (email is the account key).
Store: GetUserByOIDC, LinkOIDC (UNIQUE-pair backstop). Config: OIDCReady(); /auth/providers reports oidc from it; button label default → "Sign in with Authentik". PANSY_LOCAL_AUTH=false rejects/hides local auth but leaves OIDC working.
Verification
go build/go vet/go test ./... green (CGO off, GOWORK=off). go-oidc + oauth2 are pure Go.
Service tests: JIT create + admin, repeat login (no dup), link-by-verified-email (local password still works after), unverified-collision refusal, no-email refusal, name fallback to email local-part, OIDC works with local auth disabled.
API tests (fake discovery server, no network/IdP): routes 404 when unconfigured, providers reports oidc, login → 302 with code_challenge/S256/state/nonce + tx cookie, callback state-missing → error=state, provider-error → error=oidc.
Smoke-tested a running binary with an unreachable issuer: /auth/oidc/login → 302 error=oidc_unavailable, server stays up, local register/login still work, providers reports oidc:true.
Full round-trip against a live Authentik (per the issue's AC) needs the real IdP and is best verified on deploy; the token-exchange/verify path is exercised structurally here and the provisioning logic is unit-tested.
Closes #5. Phase 1, step 2 of the tracking epic #20. Builds on #4 (same session cookie for both paths).
## What's here
**API** (`internal/api/oidc.go`)
- `GET /auth/oidc/login` — authorization-code + **PKCE (S256)** with random `state` + `nonce`, stashed in a short-lived HttpOnly `pansy_oidc_tx` cookie (SameSite=Lax so it survives the IdP's top-level redirect back), then redirects to the IdP.
- `GET /auth/oidc/callback` — verifies `state` (constant-time), exchanges the code with the PKCE verifier, verifies the ID token signature/issuer/audience/expiry **and** nonce, then provisions and starts a pansy session. Every failure clears the tx cookie and redirects to `/login?error=…`; success → `/gardens`.
- **Lazy discovery** (`oidcClient.ensure`): first request performs discovery (10s timeout) and caches it; a failure is retried on the next request, so a momentarily-unreachable IdP never stops the server — or local auth — from starting.
- Routes are registered **only when OIDC is ready** (issuer + client ID + `PANSY_BASE_URL`), so an unconfigured instance 404s them, matching what `/auth/providers` advertises.
**Service** (`internal/service/auth.go`) — `LoginOIDC` provisioning order:
1. `(issuer, subject)` match → returning user;
2. else a **verified** email match → linked onto the existing account (no split local/OIDC accounts);
3. else JIT-create (IdP gates access, so `PANSY_REGISTRATION` is bypassed) — reusing #4's atomic `CreateUser` (first user still becomes admin atomically).
Security choices: an **unverified** email that collides with an existing account is refused (account-takeover guard); an identity with **no email** is refused (email is the account key).
**Store**: `GetUserByOIDC`, `LinkOIDC` (UNIQUE-pair backstop).
**Config**: `OIDCReady()`; `/auth/providers` reports `oidc` from it; button label default → "Sign in with Authentik". `PANSY_LOCAL_AUTH=false` rejects/hides local auth but leaves OIDC working.
## Verification
- `go build`/`go vet`/`go test ./...` green (CGO off, GOWORK=off). go-oidc + oauth2 are pure Go.
- Service tests: JIT create + admin, repeat login (no dup), link-by-verified-email (local password still works after), unverified-collision refusal, no-email refusal, name fallback to email local-part, OIDC works with local auth disabled.
- API tests (fake discovery server, no network/IdP): routes 404 when unconfigured, providers reports oidc, login → 302 with `code_challenge`/`S256`/`state`/`nonce` + tx cookie, callback state-missing → `error=state`, provider-error → `error=oidc`.
- Smoke-tested a running binary with an unreachable issuer: `/auth/oidc/login` → 302 `error=oidc_unavailable`, server stays up, local register/login still work, providers reports `oidc:true`.
## Out of scope / notes
- Login UI + OIDC button (#6). Back-channel/IdP-initiated logout, refresh tokens, Authentik group→role mapping (post-v1).
- Full round-trip against a live Authentik (per the issue's AC) needs the real IdP and is best verified on deploy; the token-exchange/verify path is exercised structurally here and the provisioning logic is unit-tested.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
OIDC is pansy's primary login path (Authentik the target IdP); local
auth (#4) remains the fallback and both issue the same session cookie.
- deps: github.com/coreos/go-oidc/v3 + golang.org/x/oauth2 (both pure Go;
CGO stays off).
- api/oidc.go: lazy issuer discovery (retried per-request, never crashes
a server that also serves local auth), GET /auth/oidc/login builds an
authorization-code URL with PKCE S256 + random state + nonce stashed in
a short-lived HttpOnly cookie, GET /auth/oidc/callback verifies state
(constant-time), exchanges the code with the PKCE verifier, verifies the
ID token + nonce, and starts a pansy session. Failures redirect to
/login?error=... ; success to /gardens.
- service.LoginOIDC: (issuer,subject) match -> login; else *verified*
email match -> link onto the existing account; else JIT-create (the IdP
gates access, so PANSY_REGISTRATION doesn't apply). Unverified email
colliding with an existing account is refused (takeover guard); no email
is refused (email is the account key). Reuses the atomic CreateUser.
- store: GetUserByOIDC + LinkOIDC (unique-pair backstop).
- config: OIDCReady() (needs issuer+client+BaseURL for the redirect URI);
/auth/providers now reports oidc from it and defaults the button label
to "Sign in with Authentik". OIDC routes are only registered when ready,
so an unconfigured instance 404s them.
- PANSY_LOCAL_AUTH=false rejects/hides local auth but not OIDC.
Tests: service provisioning (JIT, repeat login, link, unverified-collision
refusal, no-email, name fallback, works with local auth off); api
(routes-absent-when-unconfigured, providers reporting, login redirect with
PKCE params + tx cookie via a fake discovery server, callback state/error
paths). Smoke-tested: unreachable issuer degrades to error=oidc_unavailable
with the server still up and local auth working.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-18 21:23:42Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ⚠️ **security** — could not complete
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Blocking issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠Discovery mutex serializes all OIDC login/callback requests behind a full 10s timeout during IdP outages, with no negative caching/backoff
performance · flagged by 2 models
internal/api/oidc.go:63-76 — oidcClient.ensure holds o.mu for the entire duration of oidc.NewProvider, a network call bounded to oidcDiscoveryTimeout (10s). Concurrent /oidc/login and /oidc/callback requests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: the if o.provider != nil { return nil } fast-path (line 66) only…
🪰 Gadfly · advisory
🟠 **Discovery mutex serializes all OIDC login/callback requests behind a full 10s timeout during IdP outages, with no negative caching/backoff**
_performance · flagged by 2 models_
- `internal/api/oidc.go:63-76` — `oidcClient.ensure` holds `o.mu` for the entire duration of `oidc.NewProvider`, a network call bounded to `oidcDiscoveryTimeout` (10s). Concurrent `/oidc/login` and `/oidc/callback` requests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: the `if o.provider != nil { return nil }` fast-path (line 66) only…
<sub>🪰 Gadfly · advisory</sub>
🟠oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper
maintainability · flagged by 2 models
oidcCallback is a long, repetitive function with ~11 near-identical failure-redirect sites (internal/api/oidc.go:130-226). Eight are c.Redirect(http.StatusFound, "/login?error=oidc"); return preceded by an slog.* call with slightly varying keys. A small helper like oidcFail(c, code string, logMsg string, logAttrs ...any) would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a gl…
🪰 Gadfly · advisory
🟠 **oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper**
_maintainability · flagged by 2 models_
- **`oidcCallback` is a long, repetitive function with ~11 near-identical failure-redirect sites** (`internal/api/oidc.go:130-226`). Eight are `c.Redirect(http.StatusFound, "/login?error=oidc"); return` preceded by an `slog.*` call with slightly varying keys. A small helper like `oidcFail(c, code string, logMsg string, logAttrs ...any)` would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a gl…
<sub>🪰 Gadfly · advisory</sub>
⚪empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above
maintainability · flagged by 1 model
internal/api/oidc.go:130-226 — oidcCallback repeats the same three-line shape (slog.X(...); c.Redirect(http.StatusFound, "/login?error=..."); return) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception,…
🪰 Gadfly · advisory
⚪ **empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above**
_maintainability · flagged by 1 model_
- `internal/api/oidc.go:130-226` — `oidcCallback` repeats the same three-line shape (`slog.X(...)`; `c.Redirect(http.StatusFound, "/login?error=...")`; `return`) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception,…
<sub>🪰 Gadfly · advisory</sub>
🔴oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout.
error-handling · flagged by 2 models
internal/api/oidc.go:164 — token exchange and ID-token verification run on the unbounded request context.ctx := c.Request.Context() (line 131) is passed directly to h.oidc.oauth.Exchange(ctx, ...) (line 164) and h.oidc.verifier.Verify(ctx, rawIDToken) (line 178). Only ensure() gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server's WriteTimeout is 30s (cmd/pansy/main.go:65), and the tx cookie is already cleared at line 134 befo…
🪰 Gadfly · advisory
🔴 **oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout.**
_error-handling · flagged by 2 models_
- **`internal/api/oidc.go:164` — token exchange and ID-token verification run on the unbounded request context.** `ctx := c.Request.Context()` (line 131) is passed directly to `h.oidc.oauth.Exchange(ctx, ...)` (line 164) and `h.oidc.verifier.Verify(ctx, rawIDToken)` (line 178). Only `ensure()` gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server's `WriteTimeout` is 30s (`cmd/pansy/main.go:65`), and the tx cookie is already cleared at line 134 befo…
<sub>🪰 Gadfly · advisory</sub>
🟡All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render
maintainability · flagged by 1 model
internal/api/oidc.go:215 — LoginOIDC provisioning failures all collapse to error=oidc.LoginOIDC returns distinct sentinels (ErrOIDCNoEmail, ErrOIDCEmailUnverified, ErrInvalidInput — confirmed in domain/domain.go:39-45) that the UI could meaningfully distinguish, but the callback maps every one to the generic ?error=oidc at line 216. This mirrors the existing writeServiceError mapping philosophy but discards information that would help the login page show e.g. "email no…
🪰 Gadfly · advisory
🟡 **All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render**
_maintainability · flagged by 1 model_
- **`internal/api/oidc.go:215` — `LoginOIDC` provisioning failures all collapse to `error=oidc`.** `LoginOIDC` returns distinct sentinels (`ErrOIDCNoEmail`, `ErrOIDCEmailUnverified`, `ErrInvalidInput` — confirmed in `domain/domain.go:39-45`) that the UI could meaningfully distinguish, but the callback maps every one to the generic `?error=oidc` at line 216. This mirrors the existing `writeServiceError` mapping philosophy but discards information that would help the login page show e.g. "email no…
<sub>🪰 Gadfly · advisory</sub>
🟡OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector
error-handling, maintainability, security · flagged by 2 models
internal/api/oidc.go:229-241 — tx cookie is not integrity-protected (low severity). The pansy_oidc_tx cookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't read the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residua…
🪰 Gadfly · advisory
🟡 **OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector**
_error-handling, maintainability, security · flagged by 2 models_
- **`internal/api/oidc.go:229-241` — tx cookie is not integrity-protected (low severity).** The `pansy_oidc_tx` cookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't *read* the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residua…
<sub>🪰 Gadfly · advisory</sub>
🔴readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "".
correctness, error-handling, maintainability · flagged by 4 models
internal/api/oidc.go:253 — readOIDCTxCookie does not require tx.Nonce. The validity check at line 253 is tx.State == "" || tx.Verifier == ""; Nonce is not required. A hand-crafted or stale cookie yielding a tx with empty Nonce passes the check, and the nonce compare at line 184 (ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))) succeeds whenever the IdP also returns an empty nonce (many IdPs omit the nonce claim, leaving idToken.Nonce == ""), so the comparison…
🪰 Gadfly · advisory
🔴 **readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "".**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`internal/api/oidc.go:253` — `readOIDCTxCookie` does not require `tx.Nonce`.** The validity check at line 253 is `tx.State == "" || tx.Verifier == ""`; `Nonce` is not required. A hand-crafted or stale cookie yielding a tx with empty `Nonce` passes the check, and the nonce compare at line 184 (`ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))`) succeeds whenever the IdP also returns an empty nonce (many IdPs omit the `nonce` claim, leaving `idToken.Nonce == ""`), so the comparison…
<sub>🪰 Gadfly · advisory</sub>
internal/api/oidc.go:265 — randToken duplicates existing token-generation patternrandToken is a near-exact clone of newSessionToken in internal/service/service.go:62: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper in service or a new internal/util package) so entropy/size requirements don't drift between the two call sites.
🪰 Gadfly · advisory
🟡 **randToken duplicates newSessionToken token-generation pattern**
_maintainability · flagged by 1 model_
- **`internal/api/oidc.go:265` — `randToken` duplicates existing token-generation pattern** `randToken` is a near-exact clone of `newSessionToken` in `internal/service/service.go:62`: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper in `service` or a new `internal/util` package) so entropy/size requirements don't drift between the two call sites.
<sub>🪰 Gadfly · advisory</sub>
🟡TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding
error-handling · flagged by 1 model
internal/service/auth.go:67-109 (LoginOIDC) — the returning-user-vs-JIT-create decision is a check-then-act race: GetUserByOIDC (a SELECT, line 73) and CreateUser (an INSERT, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new (issuer, subject) both miss the lookup and both call CreateUser; the loser trips the idx_users_oidc UNIQUE index (internal/store/migrations/0001_init.sql:27), which CreateUser maps to `domain.ErrEmailTak…
🪰 Gadfly · advisory
🟡 **TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding**
_error-handling · flagged by 1 model_
- `internal/service/auth.go:67-109` (`LoginOIDC`) — the returning-user-vs-JIT-create decision is a check-then-act race: `GetUserByOIDC` (a `SELECT`, line 73) and `CreateUser` (an `INSERT`, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new `(issuer, subject)` both miss the lookup and both call `CreateUser`; the loser trips the `idx_users_oidc` UNIQUE index (`internal/store/migrations/0001_init.sql:27`), which `CreateUser` maps to `domain.ErrEmailTak…
<sub>🪰 Gadfly · advisory</sub>
🔴LinkOIDC silently overwrites existing OIDC identity on email-linked user
correctness · flagged by 2 models
internal/service/auth.go:93 — LinkOIDC silently overwrites an existing OIDC identityLoginOIDC resolves a user by email and unconditionally calls LinkOIDC, which executes an UPDATE that stomps any existing oidc_issuer / oidc_subject on that row. The comment on LinkOIDC says it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issue…
🪰 Gadfly · advisory
🔴 **LinkOIDC silently overwrites existing OIDC identity on email-linked user**
_correctness · flagged by 2 models_
* **`internal/service/auth.go:93` — `LinkOIDC` silently overwrites an existing OIDC identity** `LoginOIDC` resolves a user by email and unconditionally calls `LinkOIDC`, which executes an `UPDATE` that stomps any existing `oidc_issuer` / `oidc_subject` on that row. The comment on `LinkOIDC` says it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issue…
<sub>🪰 Gadfly · advisory</sub>
🔴LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking.
error-handling, security · flagged by 2 models
internal/service/auth.go:103 — LoginOIDC JIT-creates an account (which may become the first/admin user) when email_verified is false and the email doesn't collide. The email-collision branch (lines 88-96) correctly requires id.EmailVerified before linking, but the JIT-creation fallthrough (lines 98-108) does not. With PANSY_LOCAL_AUTH=false, an identity whose unverified email matches no existing account is provisioned, and store.CreateUser makes it admin iff the table is empty…
🪰 Gadfly · advisory
🔴 **LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking.**
_error-handling, security · flagged by 2 models_
- **`internal/service/auth.go:103` — `LoginOIDC` JIT-creates an account (which may become the first/admin user) when `email_verified` is false and the email doesn't collide.** The email-collision branch (lines 88-96) correctly requires `id.EmailVerified` before linking, but the JIT-creation fallthrough (lines 98-108) does not. With `PANSY_LOCAL_AUTH=false`, an identity whose unverified email matches no existing account is provisioned, and `store.CreateUser` makes it admin iff the table is empty…
<sub>🪰 Gadfly · advisory</sub>
⚪Duplicated issuer literal bypasses the oidcIdentity test helper
maintainability · flagged by 1 model
Duplicated issuer string "https://idp.example" in TestProvidersReflectsConfig (internal/service/auth_test.go:293,300) — the same constant is also hardcoded inside the oidcIdentity helper (line 307). The helper exists to centralize this; the two extra inlines bypass it.
🪰 Gadfly · advisory
⚪ **Duplicated issuer literal bypasses the oidcIdentity test helper**
_maintainability · flagged by 1 model_
- **Duplicated issuer string `"https://idp.example"` in `TestProvidersReflectsConfig`** (`internal/service/auth_test.go:293,300`) — the same constant is also hardcoded inside the `oidcIdentity` helper (line 307). The helper exists to centralize this; the two extra inlines bypass it.
<sub>🪰 Gadfly · advisory</sub>
🟠LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error.
error-handling, performance · flagged by 5 models
internal/store/users.go:127 — LinkOIDC ignores RowsAffected() == 0.ExecContext (lines 128-135) returns no error and RowsAffected() is never checked; the function then calls GetUserByID(ctx, userID) (line 142), which returns domain.ErrNotFound for a non-existent userID. A LinkOIDC against a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: check RowsAffected() and return a…
🪰 Gadfly · advisory
🟠 **LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error.**
_error-handling, performance · flagged by 5 models_
- **`internal/store/users.go:127` — `LinkOIDC` ignores `RowsAffected() == 0`.** `ExecContext` (lines 128-135) returns no error and `RowsAffected()` is never checked; the function then calls `GetUserByID(ctx, userID)` (line 142), which returns `domain.ErrNotFound` for a non-existent userID. A `LinkOIDC` against a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: check `RowsAffected()` and return a…
<sub>🪰 Gadfly · advisory</sub>
🟡LinkOIDC returns ErrEmailTaken for OIDC identity collision
maintainability · flagged by 4 models
internal/store/users.go:137 — LinkOIDC returns semantically wrong error on OIDC identity collision A UNIQUE violation on (oidc_issuer, oidc_subject) is mapped to domain.ErrEmailTaken, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicated ErrOIDCIdentityTaken (or similar) would make the code self-documenting and prevent future confusion i…
🪰 Gadfly · advisory
🟡 **LinkOIDC returns ErrEmailTaken for OIDC identity collision**
_maintainability · flagged by 4 models_
- **`internal/store/users.go:137` — `LinkOIDC` returns semantically wrong error on OIDC identity collision** A UNIQUE violation on `(oidc_issuer, oidc_subject)` is mapped to `domain.ErrEmailTaken`, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicated `ErrOIDCIdentityTaken` (or similar) would make the code self-documenting and prevent future confusion i…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 15 findings (9 with multi-model agreement)
Finding
Where
Models
Lens
🟠
LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error.
internal/store/users.go:127
5/5
error-handling, performance
🔴
readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "".
internal/api/oidc.go:253
4/5
correctness, error-handling, maintainability
🟡
LinkOIDC returns ErrEmailTaken for OIDC identity collision
internal/store/users.go:137
4/5
maintainability
🔴
oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout.
internal/api/oidc.go:164
2/5
error-handling
🔴
LinkOIDC silently overwrites existing OIDC identity on email-linked user
internal/service/auth.go:93
2/5
correctness
🔴
LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking.
internal/service/auth.go:103
2/5
error-handling, security
🟠
Discovery mutex serializes all OIDC login/callback requests behind a full 10s timeout during IdP outages, with no negative caching/backoff
internal/api/oidc.go:63
2/5
performance
🟠
oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper
internal/api/oidc.go:130
2/5
maintainability
🟡
OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector
internal/api/oidc.go:229
2/5
error-handling, maintainability, security
6 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟡
writeServiceError missing cases for new ErrOIDCNoEmail / ErrOIDCEmailUnverified sentinels
internal/api/auth.go:216
opencode/glm-5.2:cloud
maintainability
🟡
All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render
TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding
internal/service/auth.go:67
claude-code/sonnet
error-handling
⚪
empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above
internal/api/oidc.go:158
claude-code/sonnet
maintainability
⚪
Duplicated issuer literal bypasses the oidcIdentity test helper
internal/service/auth_test.go:293
opencode/glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
Good — json:"-" means OIDC identity fields never leak into the /auth/me or user JSON responses. I've verified the core security-relevant paths against the actual repo. Nothing material stood out in this lens.
VERDICT: No material issues found
The OIDC implementation is solid from a security standpoint:
CSRF/state: fresh state per login, stashed in an HttpOnly, host-only, SameSite=Lax cookie, compared constant-time against the callback param (internal/api/oidc.go:145-152). Verified this prevents login-CSRF: an attacker completing their own flow and forwarding the code/state to a victim fails because the victim's browser lacks the matching tx cookie.
PKCE: S256 challenge generated via oauth2.GenerateVerifier()/S256ChallengeOption, verifier sent only in the server-side token exchange (oidc.go:127,163-167) — never exposed to the browser beyond the opaque tx cookie.
Nonce: generated, stashed, and constant-time compared against the verified ID token's nonce (oidc.go:184-188) — replay protection independent of state.
ID token verification: uses provider.Verifier(&oidc.Config{ClientID: ...}), which by default enforces issuer, audience/client-id, signature, and expiry (go-oidc defaults, not overridden here) — confirmed no SkipClientIDCheck/SkipIssuerCheck/InsecureSkipSignatureCheck options are set.
Provisioning/account-takeover guard (internal/service/auth.go:80-108): identity resolution is (issuer,subject) → verified-email-link → JIT-create, in that order; unverified email collisions and missing email_verified claims fail closed to ErrOIDCEmailUnverified rather than silently linking. Verified email_verified is read as a plain Go bool from claims, so a missing claim decodes to false (fail-safe).
Email normalization is consistent between Register/Login/LoginOIDC (all route through normalizeEmail), and the users.email column is UNIQUE COLLATE NOCASE (internal/store/migrations/0001_init.sql:16), so there's no case-mismatch bypass of the collision check.
No open redirect: all c.Redirect targets in the callback are hardcoded literals (/login?error=..., /gardens); no user-controlled next/redirect_uri parameter is honored.
No secret/token leakage: access/ID tokens are never logged, only error values; OIDCIssuer/OIDCSubject are tagged json:"-" on domain.User so they don't leak via /auth/me or provisioning responses; client secret only flows into the oauth2.Config used for the token exchange.
Injection: all store queries are parameterized (GetUserByOIDC, LinkOIDC); no string-built SQL from claim data.
SSRF surface: discovery/token-exchange targets (issuer, endpoints from discovery) come from operator-set env config, not from request-time/attacker-controlled input, so this isn't attacker-triggerable SSRF.
One thing I checked but didn't flag as a finding: the pansy_oidc_tx cookie (state/verifier/nonce) is plain base64 JSON, not HMAC-signed. In principle a party that can already write cookies to this exact host (not a subdomain — the cookie is host-only) could pre-seed known state/verifier values, but that only lets them complete a login they initiate themselves — it doesn't cross a security boundary against another user's session, so I'm not reporting it as a real vulnerability, just noting it was considered.
🎯 Correctness — No material issues found
All test helpers exist as expected. I've verified the diff against the checked-out repo (service, store, config, api layers, schema migration, and existing test helpers). No compilation or cross-file inconsistencies found, and the OIDC provisioning logic, PKCE/nonce/state handling, and email-link/JIT decision tree are internally consistent and match the library APIs used.
Review
VERDICT: No material issues found
Findings within the correctness lens, after reading the actual checked-out source (not just the diff):
LoginOIDC resolution order (internal/service/auth.go:67-109) is sound: (issuer,subject) lookup guarded against empty strings, verified-email link, then JIT create. Verified the oidc_issuer/oidc_subject UNIQUE index (internal/store/migrations/0001_init.sql:27) and that SQLite treats NULL pairs as distinct, so local-only users (NULL,NULL) can't collide — no false "returning user" match.
PKCE/nonce/state flow in internal/api/oidc.go is correctly wired: oauth2.GenerateVerifier() → oauth2.S256ChallengeOption on the auth URL → oauth2.VerifierOption on Exchange (matching verifier/challenge pair), oidc.Nonce set on the auth URL and checked against idToken.Nonce via constant-time compare, state checked before any network call. Order of operations (state check before ensure()/token exchange) avoids wasted work on CSRF-failed requests.
GetUserByOIDC/LinkOIDC (internal/store/users.go:108-143) match the domain User.OIDCIssuer/OIDCSubject (*string) fields and userColumns/scanUser correctly.
startSession extraction (internal/api/auth.go:163-184) is a straightforward, behavior-preserving refactor reused correctly by both the OIDC callback and the existing local-auth path — verified startSessionAndRespond still sets the cookie via the same code path.
CreateUser(..., true) for JIT provisioning correctly reuses the existing atomic first-user-admin/registration-gate INSERT (internal/store/users.go:54-79); allowSignup=true bypasses the closed-registration gate as intended without disturbing the atomic admin assignment.
version is incremented but not compared in LinkOIDC's UPDATE — checked this isn't a new gap; no other store method in this codebase uses version for optimistic-concurrency WHERE clauses either, so this is consistent with existing (pre-PR) practice, not a regression.
csrfGuard only guards non-GET methods, and both new OIDC routes are GET, so no interaction/bug there.
No logic bugs, no incorrect constants/thresholds (10-minute tx-cookie TTL and 10s discovery timeout are reasonable, non-semantic operational values, not domain-derived quantities), and no broken cross-file references found.
🧹 Code cleanliness & maintainability — Minor issues
Review
VERDICT: Minor issues
internal/api/oidc.go:130-226 — oidcCallback repeats the same three-line shape (slog.X(...); c.Redirect(http.StatusFound, "/login?error=..."); return) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception, see below). Extracting a small helper (e.g. func (h *handlers) oidcFail(c *gin.Context, code string, logFn func())) would cut real repetition and make the function's control flow (provider error → state → discovery → code → exchange → id_token → verify → nonce → claims → provision → session) easier to scan.
internal/api/oidc.go:158-162 — the code == "" failure path is the only failure branch in oidcCallback that doesn't log anything before redirecting, while every sibling branch (slog.Error/slog.Warn) does. Confirmed directly in the source: lines 159-162 go straight from the if code == "" check to c.Redirect with no slog call. That's an inconsistency introduced by the duplication above — worth fixing at the same time a shared helper is introduced, since a missing-code case is exactly the kind of thing an operator would want in the logs too.
⚡ Performance — Minor issues
Confirmed: LinkOIDC is called once per OIDC login flow (in LoginOIDC), matching the draft's characterization. Both findings hold up against the actual code.
VERDICT: Minor issues found
internal/api/oidc.go:63-87 (oidcClient.ensure) — the discovery mutex is held for the full duration of a failed/slow discovery attempt, and failures aren't negatively cached. ensure() does o.mu.Lock(); defer o.mu.Unlock() before calling oidc.NewProvider(dctx, o.issuer), a network round-trip bounded only by oidcDiscoveryTimeout (10s). Every concurrent call to /auth/oidc/login or /auth/oidc/callback serializes on this single mutex while the IdP is unreachable/slow, so request N can wait up to N × 10s before even getting a chance to fail, tying up that many handler goroutines/connections. This is a real availability/perf risk during an IdP outage, though it only manifests then. A singleflight-style dedup or a short negative-cache backoff would bound the blast radius.
internal/store/users.go:127-143 (LinkOIDC) — does an UPDATE followed by a separate GetUserByID round-trip rather than a single RETURNING statement. This runs once per user (first OIDC login linking, confirmed as the only call site in internal/service/auth.go:93), so the extra round-trip is negligible — trivial, not blocking.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
internal/service/auth.go:67-109 (LoginOIDC) — the returning-user-vs-JIT-create decision is a check-then-act race: GetUserByOIDC (a SELECT, line 73) and CreateUser (an INSERT, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new (issuer, subject) both miss the lookup and both call CreateUser; the loser trips the idx_users_oidc UNIQUE index (internal/store/migrations/0001_init.sql:27), which CreateUser maps to domain.ErrEmailTaken (internal/store/users.go:62-63) even though no email conflict occurred. This surfaces to the browser only as the generic error=oidc redirect (internal/api/oidc.go:214-218). Confirmed internal/store/sqlite.go:46-51 pins SetMaxOpenConns(1) only for :memory: DSNs, so a real file-backed deployment runs a multi-connection pool and the race is reachable. There's no mutex or transaction anywhere in LoginOIDC serializing this. Only sequential re-login is exercised in tests (internal/service/auth_test.go:310-332).
internal/api/oidc.go:152-183 — discovery is explicitly bounded by oidcDiscoveryTimeout (10s, comment at lines 30-32: "a hung IdP can't wedge a request goroutine"), applied via context.WithTimeout in ensure (line 70). But the token exchange (h.oidc.oauth.Exchange(ctx, code, ...), line 164) and ID-token verification (h.oidc.verifier.Verify(ctx, rawIDToken), line 178, which fetches JWKS on first use) run on the bare request context (ctx := c.Request.Context(), line 131) with no deadline applied. Confirmed no timeout middleware or context deadline exists anywhere upstream — grep over internal/api/api.go for Timeout/context.With returns nothing, and cmd/pansy/main.go:63-66 only sets the http.Server's ReadTimeout/ReadHeaderTimeout/WriteTimeout/IdleTimeout, none of which cancel Request.Context() mid-handler. A slow/hanging IdP token endpoint can wedge a callback-handling goroutine indefinitely — the exact failure mode discovery was hardened against, left open one function down.
Both findings confirmed against the actual code. Outputting the corrected review.
VERDICT: Minor issues
Reviewed the OIDC login flow through the security lens (authn/authz, CSRF, injection, session handling, takeover guards). The core design is sound: PKCE S256, random state + nonce, constant-time state/nonce comparison, issuer/subject taken from the verified token rather than raw claims, ID-token verification via go-oidc (signature/issuer/audience/expiry), unverified-email-collision refusal, no-email refusal, and no error-detail leakage to the browser (provider ?error= is logged but never reflected; all redirects go to hardcoded /login?error=<code> or /gardens). Verified oidc.go:130-226, auth.go:67-109, users.go:108-143, and the schema constraints in 0001_init.sql.
Findings:
internal/api/oidc.go:229-241 — tx cookie is not integrity-protected (low severity). The pansy_oidc_tx cookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't read the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residual risk is login-CSRF/session-fixation only if a separate cookie-injection vector exists on the host (in which case the session cookie is already at risk). Noting it because it's the one place untrusted-browser-state is trusted without integrity. Suggested fix if hardening is desired: HMAC the cookie payload with a server key, or move the tx to server-side storage keyed by the state.
internal/service/auth.go:103-108 — PANSY_REGISTRATION=closed is silently bypassed by OIDC JIT, and the first OIDC user becomes admin (low confidence, documented as intended).LoginOIDC calls CreateUser(..., true), so every user the IdP admits gets an account regardless of the registration policy, and CreateUser's is_admin = (count=0) makes the first such user admin. The code comment states this is intentional ("the IdP gates access"), and the security model is sound if the IdP is trusted to gate access. The caveat: an operator who sets PANSY_REGISTRATION=closed expecting "no new accounts can be created" may not realize OIDC overrides that, and a misconfigured IdP that allows open signup would hand an attacker the first/admin account. Worth a config-level note or a guardrail (e.g., require an explicit PANSY_OIDC_JIT=true opt-in) rather than a code change.
No injection (SQL uses parameterized queries; issuer/subject/email are bound, not interpolated), no SSRF (issuer is admin-configured), no open redirect (all redirect targets are hardcoded), and no authz bypass found.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the change through the correctness lens, verifying against the checked-out code.
Verified items (all correct):
PKCE S256 flow (internal/api/oidc.go:115-123,164): GenerateVerifier() → S256ChallengeOption at auth URL, VerifierOption at exchange. Matches the oauth2 library contract; challenge = BASE64URL(SHA256(verifier)).
State/nonce binding (oidc.go:133-150,184-188): tx cookie carries state+verifier+nonce; callback does constant-time state compare (short-circuits on missing tx/state) and constant-time nonce compare against the verified token's idToken.Nonce. Fails closed if the IdP omits nonce ("" vs non-empty tx.Nonce).
Issuer/subject provenance (oidc.go:206-213): Issuer/Subject taken from the verified idToken, not raw claims; the go-oidc verifier already checked iss against the discovered issuer, so the value stored by LoginOIDC is consistent with GetUserByOIDC's lookup key.
Provisioning order (internal/service/auth.go:67-108): (issuer,subject) → verified-email link → JIT, with unverified-collision refusal and no-email refusal. Email is normalized (lowercased) before both the collision lookup and CreateUser; GetUserByEmail's NOCASE collation agrees. The [email protected] link test exercises this.
JIT admin bootstrap (auth.go:103-108 → store/users.go:54-78): CreateUser(allowSignup=true) uses (SELECT count(*) FROM users) = 0 for is_admin inside the same INSERT, so the first OIDC user is atomically admin even with PANSY_LOCAL_AUTH=false / PANSY_REGISTRATION=closed. Test confirms.
UNIQUE-index backstop (store/users.go:127-143, migration 0001_init.sql:27): LinkOIDC overwriting a pair already owned by another row trips idx_users_oidc → ErrEmailTaken; concurrent same-identity JIT logins fail the second INSERT cleanly and resolve on retry via GetUserByOIDC. No duplicate accounts.
Cookie lifecycle (oidc.go:133-134): tx cookie is cleared at the top of the callback on every path (provider error, state mismatch, success), preventing replay. MaxAge=600s, SameSite=Lax, Secure derived from PANSY_BASE_URL consistently with the session cookie.
emailLocalPart (auth.go:283-289): i > 0 correctly returns the local part for well-formed emails and falls back to the whole string otherwise.
One design choice I considered and deliberately did not flag as a bug: an unverified email that does not collide with an existing account still JIT-creates an account (auth.go:88-96 only refuses the collision case). That is explicitly the PR's stated design ("an unverified email that collides… is refused"), so it is intended behavior, not a correctness defect.
No correctness issues to report.
🧹 Code cleanliness & maintainability — Minor issues
All three findings confirmed against the actual source.
VERDICT: Minor issues
internal/api/oidc.go:215 — LoginOIDC provisioning failures all collapse to error=oidc.LoginOIDC returns distinct sentinels (ErrOIDCNoEmail, ErrOIDCEmailUnverified, ErrInvalidInput — confirmed in domain/domain.go:39-45) that the UI could meaningfully distinguish, but the callback maps every one to the generic ?error=oidc at line 216. This mirrors the existing writeServiceError mapping philosophy but discards information that would help the login page show e.g. "email not verified" vs. "provider error". Minor maintainability/UX wart; consider at least routing ErrOIDCEmailUnverified to a distinct ?error=email_unverified code so the UI layer has something to render later.
internal/store/users.go:138 — LinkOIDC UNIQUE-collision backstop reuses ErrEmailTaken (409 "email already registered") for a non-email conflict. A duplicate (issuer, subject) pair trips the OIDC-identity UNIQUE index and is mapped to domain.ErrEmailTaken at line 138, whose doc/comment (domain.go:28-30) is explicitly "registration collided with an existing account's email" and whose API message is "an account with that email already exists." Reusing an email-specific sentinel for an OIDC-identity collision is a leaky/misleading abstraction; a future caller that surfaces this error to a user gets the wrong message. Suggested fix: introduce an ErrOIDCIdentityTaken (or a generic ErrConflict) and map the index violation to it.
internal/api/oidc.go:231-235 — setOIDCTxCookie silently returns without setting the cookie on a marshal error. As the comment notes, json.Marshal of the hand-built struct can't realistically fail, so the early return after only logging leaves the caller (oidcLogin at line 117) to proceed and issue a 302 to the IdP with no tx cookie — the subsequent callback would then fail at the state check with error=state rather than failing fast at login. This is dead-in-practice but the control flow is subtly wrong (continue-after-failure). Suggested fix: treat it as an internal error and redirect to /login?error=oidc instead of falling through to the provider redirect.
⚡ Performance — No material issues found
I've reviewed the diff through the performance lens, checking the new OIDC flow, the lazy discovery caching, and the service/store provisioning queries.
VERDICT: No material issues found
I verified the following potential hotspots and they're all fine for this workload:
oidcClient.ensure (internal/api/oidc.go:63-87) — holds o.mu across the discovery network call (up to oidcDiscoveryTimeout=10s). This serializes concurrent OIDC requests only during cold-start discovery; once o.provider != nil, each request takes the lock, checks the nil-guard, and returns. That serialization is actually desirable (prevents a thundering herd of redundant discovery fetches), and OIDC login/callback is a low-frequency path, not a hot loop. Read at oidc.go:63-87.
LoginOIDC (internal/service/auth.go) provisioning — up to 2-3 queries (GetUserByOIDC → GetUserByEmail → CreateUser/LinkOIDC). This is a login path with no N+1, no loops, no unbounded growth. Confirmed by reading the service and store code.
LinkOIDC (internal/store/users.go:120-141) issues an UPDATE then a separate GetUserByID SELECT (two round-trips instead of a RETURNING single statement). This is a one-time-per-linking operation on a login path, not material.
No new allocations on hot paths, no missing pagination, no blocking calls inside loops. The tx-cookie marshal/base64 per login is trivial.
No performance regressions introduced by this change.
🧯 Error handling & edge cases — Minor issues
All three findings confirmed against the actual code:
oidc.go:253 — Confirmed: readOIDCTxCookie guards tx.State == "" and tx.Verifier == "" but not tx.Nonce == "". The nonce check at line 184 is subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1; with both empty, this compares equal and passes. However, in the normal flow nonce is always set from randToken() at line 108 (and err2 is checked), so the only way tx.Nonce is empty is if an attacker tampers the cookie — but the cookie is HttpOnly + signed-by-our-domain... actually it's not signed, just base64 JSON. So a tampered cookie could set Nonce empty. The finding's framing is valid as defense-in-depth.
users.go:127 — Confirmed: LinkOIDC does ExecContext UPDATE, checks error but not RowsAffected, then falls through to GetUserByID. If zero rows matched (account deleted between GetUserByEmail and LinkOIDC), GetUserByID returns domain.ErrNotFound, surfaced as generic error=oidc.
oidc.go:229 — Confirmed: setOIDCTxCookie on marshal error logs and returns without setting the cookie; caller at line 117-123 proceeds to redirect to the IdP regardless. The doomed flow would fail at callback with error=state. Theoretical since struct holds only strings.
All three survive verification.
Minor issues
internal/api/oidc.go:253 — readOIDCTxCookie does not validate that tx.Nonce is non-empty. It guards only tx.State == "" and tx.Verifier == "" (line 253). The nonce check at line 184 is subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)), so an empty tx.Nonce combined with an ID token that carries no nonce claim (a non-compliant IdP, or a cookie tampered to blank the nonce) compares equal and passes vacuously, disabling nonce binding (replay protection) for that flow. State + PKCE still hold, so this is defense-in-depth, not a break. Fix: add || tx.Nonce == "" to the guard so a tx with no nonce is rejected outright. Verified against lines 243–257 and 184.
internal/store/users.go:127 — LinkOIDC ignores RowsAffected. The UPDATE ... WHERE id = ? (lines 128–135) returns no error when it matches zero rows (e.g. the account was deleted between GetUserByEmail and LinkOIDC). Execution falls through to GetUserByID (line 142), which returns domain.ErrNotFound, surfaced to the user as a generic error=oidc (via service.LoginOIDC at auth.go:93 and the callback at oidc.go:215). No data corruption, but the link silently no-ops and the error is misleading rather than a clear "user gone." Fix: check res.RowsAffected(); if 0, return domain.ErrNotFound explicitly instead of relying on the follow-up lookup.
internal/api/oidc.go:229 — setOIDCTxCookie swallows the marshal failure and lets oidcLogin proceed. On json.Marshal error (lines 231–234) it logs and returns without setting the cookie, but the caller (lines 117–123) then redirects to the IdP regardless, producing a flow guaranteed to fail at callback with error=state. The comment acknowledges marshal "cannot fail in practice" (the struct holds only generated strings), so this is a theoretical edge; a cleaner behavior would treat it as a hard failure and redirect to /login?error=oidc rather than launching a doomed redirect.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
The OIDC implementation is solid from a security perspective:
PKCE (S256): Correctly implemented with oauth2.GenerateVerifier() and oauth2.VerifierOption on exchange.
CSRF protection: Random 32-byte state parameter generated per-login, verified with subtle.ConstantTimeCompare on callback.
Nonce verification: Fresh random nonce per-login, checked against the ID token with constant-time comparison after full signature/issuer/audience/expiry verification by go-oidc.
Cookie hardening: pansy_oidc_tx is HttpOnly, Secure (when HTTPS), SameSite=Lax (appropriate for the IdP top-level redirect), and cleared immediately on callback.
Account takeover guard: Email linking only occurs when email_verified=true; unverified collisions and missing emails are refused.
No secret leakage: clientSecret is never logged; token exchange errors are logged without reflecting the authorization code.
No injection vectors: All store queries use parameterized SQL; OIDC claims are not interpolated into URLs or HTML.
Session hygiene: startSession creates a fresh bearer token — no session fixation risk.
The one thing I chased — readOIDCTxCookie not validating tx.Nonce != "" — is harmless because an empty nonce would fail the later subtle.ConstantTimeCompare against any real IdP token.
🎯 Correctness — Blocking issues found
Blocking issues found
internal/service/auth.go:93 — LinkOIDC silently overwrites an existing OIDC identity LoginOIDC resolves a user by email and unconditionally calls LinkOIDC, which executes an UPDATE that stomps any existing oidc_issuer / oidc_subject on that row. The comment on LinkOIDC says it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issuer A / subject A) and later authenticates via IdP B with the same verified email, her OIDC identity is overwritten to IdP B. She can no longer log in via IdP A. Verified by reading internal/service/auth.go:86-93 and internal/store/users.go:127-143. Fix: Before linking, verify existing.OIDCIssuer == nil && existing.OIDCSubject == nil; if the user already has OIDC credentials, return a distinct error (e.g., ErrOIDCAlreadyLinked or treat it as ErrEmailTaken) instead of overwriting.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/store/users.go:138: LinkOIDC maps a UNIQUE violation on (oidc_issuer, oidc_subject) to domain.ErrEmailTaken. The comment acknowledges this is a "generic conflict" backstop, but reusing an error whose name and message literally mean "email already registered" for an OIDC identity collision is a leaky abstraction. A future maintainer calling LinkOIDC from a new code path (or simply reading traces/logs) will be misled. Add a domain-wide ErrConflict (or an OIDC-specific ErrOIDCIdentityTaken) and use it here instead.
⚡ Performance — No material issues found
No material issues found
🧯 Error handling & edge cases — Minor issues
Minor issues
internal/api/oidc.go:230-234 — setOIDCTxCookie logs and silently returns when json.Marshal fails, leaving oidcLogin to redirect the browser to the IdP without the transaction cookie. The callback will then fail with a generic error=state. While json.Marshal on a struct of strings cannot practically fail, the error should be propagated so the caller can redirect immediately with an informative error code rather than sending the user through a broken round-trip.
internal/api/oidc.go:253 — readOIDCTxCookie validates tx.State != "" and tx.Verifier != "" but does not check tx.Nonce != "". If a malformed cookie omits the nonce, the function returns true, and the later nonce comparison at line 184 will still fail (assuming the IdP echoes back the original nonce), but validating all three fields is cleaner defense-in-depth.
internal/store/users.go:127-143 — LinkOIDC issues an UPDATE ... WHERE id = ? but never checks RowsAffected. If the user row is deleted between the preceding GetUserByEmail and this call, ExecContext returns no error and GetUserByID subsequently returns ErrNotFound. Checking RowsAffected == 0 and returning ErrNotFound directly would make the failure mode explicit.
opencode/glm-5.2:cloud (opencode) — Blocking issues found
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🎯 Correctness — Minor issues
VERDICT: Minor issues
internal/service/auth.go:93 / internal/store/users.go:127 — verified-email linking can overwrite an existing, different OIDC identity (account lockout/takeover).LoginOIDC's step 2 (auth.go:87-93) calls LinkOIDC whenever a verified email matches an existing account, with no check that the matched account doesn't already carry a different OIDC identity. LinkOIDC (store/users.go:127-143) unconditionally UPDATEs oidc_issuer/oidc_subject regardless of their current values. Flow: (1) local user Bob registers with [email protected]; (2) Bob's first OIDC login with subject sub-A + verified email links → oidc_subject=sub-A; (3) a later login with a different subject sub-B and the same verified email reaches step 2 (GetUserByOIDC(issuer, sub-B) → ErrNotFound since sub-B ≠ sub-A, GetUserByEmail → finds Bob, EmailVerified=true), and LinkOIDCoverwritessub-A with sub-B. Bob is now locked out of OIDC and the sub-B holder controls the account. The "verified email" guard does not prevent this; both identities present a verified email. Suggested fix: in step 2, refuse to link when existing.OIDCIssuer != nil && (*existing.OIDCIssuer != id.Issuer || *existing.OIDCSubject != id.Subject), returning a sentinel like domain.ErrOIDCIdentityConflict instead of silently overwriting.
🧹 Code cleanliness & maintainability — Minor issues
All four findings verified against the actual source:
oidcCallback redirect repetition — Confirmed: 11 c.Redirect(http.StatusFound, "/login?error=...") sites in oidc.go lines 130-226 (eight with error=oidc), plus 2 more in oidcLogin. The handler is ~95 lines with deeply nested failure paths.
LinkOIDC → ErrEmailTaken misnomer — Confirmed at store/users.go:137-138; the comment at lines 122-126 explicitly calls it a "(issuer,subject)" backstop that "shouldn't happen," yet maps to a sentinel named "email already registered."
writeServiceError missing new sentinels — Confirmed: auth.go:216-231 switch has no cases for ErrOIDCNoEmail/ErrOIDCEmailUnverified (defined at domain.go:39-45). The only current caller (oidcCallback) bypasses writeServiceError and redirects, so this is latent.
Duplicated issuer literal — Confirmed: "https://idp.example" at auth_test.go:293, :300, and inside the oidcIdentity helper at :307.
VERDICT: Minor issues
oidcCallback is a long, repetitive function with ~11 near-identical failure-redirect sites (internal/api/oidc.go:130-226). Eight are c.Redirect(http.StatusFound, "/login?error=oidc"); return preceded by an slog.* call with slightly varying keys. A small helper like oidcFail(c, code string, logMsg string, logAttrs ...any) would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a glance.
LinkOIDC maps an (issuer, subject) UNIQUE collision to ErrEmailTaken (internal/store/users.go:138). The sentinel's name and docstring ("email already registered") describe a different constraint than the one being hit; the comment on line 122-126 even admits it "shouldn't happen." A maintainer tracing an OIDC provisioning failure through LoginOIDC would be misled into thinking the email collided. A dedicated ErrOIDCIdentityTaken (or a generic ErrConflict) would communicate the actual cause.
writeServiceError is missing cases for the two new domain sentinels (internal/api/auth.go:216-231). ErrOIDCNoEmail and ErrOIDCEmailUnverified were added in domain/domain.go:39-45 but the central service→HTTP mapping table wasn't updated. Today the only caller (oidcCallback) bypasses writeServiceError and redirects, so this is latent rather than broken — but leaving the table incomplete invites a future caller to fall through to the default: INTERNAL branch for what is really a client-side problem.
Duplicated issuer string "https://idp.example" in TestProvidersReflectsConfig (internal/service/auth_test.go:293,300) — the same constant is also hardcoded inside the oidcIdentity helper (line 307). The helper exists to centralize this; the two extra inlines bypass it.
⚡ Performance — Minor issues
VERDICT: Minor issues
internal/api/oidc.go:63-76 — oidcClient.ensure holds o.mu for the entire duration of oidc.NewProvider, a network call bounded to oidcDiscoveryTimeout (10s). Concurrent /oidc/login and /oidc/callback requests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: the if o.provider != nil { return nil } fast-path (line 66) only short-circuits after a successful discovery, so the lock-during-I/O only bites on the first cold request(s). Impact is bounded to the cold-start window and a low-QPS endpoint, so it's minor — but the standard fix is: check-then-release-lock → do discovery unlocked → re-acquire and double-check before assigning (or use a singleflight-style in-flight marker). No other serialization on this path (no per-request DB write beyond the normal session insert).
🧯 Error handling & edge cases — Blocking issues found
I've verified each finding against the actual code. Here's my corrected review:
Review — 🧯 Error handling & edge cases
Verdict: Blocking issues found
internal/api/oidc.go:164 — token exchange and ID-token verification run on the unbounded request context.ctx := c.Request.Context() (line 131) is passed directly to h.oidc.oauth.Exchange(ctx, ...) (line 164) and h.oidc.verifier.Verify(ctx, rawIDToken) (line 178). Only ensure() gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server's WriteTimeout is 30s (cmd/pansy/main.go:65), and the tx cookie is already cleared at line 134 before exchange, so a slow IdP can leave the user on a blank page with the tx cookie already consumed. Fix: wrap the exchange+verify block in context.WithTimeout(ctx, ...) with defer cancel().
internal/api/oidc.go:253 — readOIDCTxCookie does not require tx.Nonce. The validity check at line 253 is tx.State == "" || tx.Verifier == ""; Nonce is not required. A hand-crafted or stale cookie yielding a tx with empty Nonce passes the check, and the nonce compare at line 184 (ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))) succeeds whenever the IdP also returns an empty nonce (many IdPs omit the nonce claim, leaving idToken.Nonce == ""), so the comparison becomes "" == "" and replay/mix-up protection silently degrades to a no-op. Fix: require tx.Nonce != "" in readOIDCTxCookie and reject idToken.Nonce == "" at verify time.
internal/store/users.go:127 — LinkOIDC ignores RowsAffected() == 0.ExecContext (lines 128-135) returns no error and RowsAffected() is never checked; the function then calls GetUserByID(ctx, userID) (line 142), which returns domain.ErrNotFound for a non-existent userID. A LinkOIDC against a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: check RowsAffected() and return a wrapped error on 0.
internal/service/auth.go:103 — LoginOIDC JIT-creates an account (which may become the first/admin user) when email_verified is false and the email doesn't collide. The email-collision branch (lines 88-96) correctly requires id.EmailVerified before linking, but the JIT-creation fallthrough (lines 98-108) does not. With PANSY_LOCAL_AUTH=false, an identity whose unverified email matches no existing account is provisioned, and store.CreateUser makes it admin iff the table is empty (users.go:57), so the first user to log in via OIDC with an unverified email becomes admin. Fix: require id.EmailVerified for JIT provisioning, not only for linking.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
After a thorough security review of the OIDC implementation, I verified the following:
PKCE: Uses oauth2.S256ChallengeOption with a randomly generated verifier, properly stashed in the HttpOnly pansy_oidc_tx cookie and verified during code exchange.
CSRF protection: Random state parameter generated via crypto/rand, compared with subtle.ConstantTimeCompare against the cookie value.
Nonce verification: Random nonce generated via crypto/rand, verified with subtle.ConstantTimeCompare against the ID token claim.
Cookie security: pansy_oidc_tx is HttpOnly, SameSite=Lax, and Secure when PANSY_BASE_URL is HTTPS. The cookie is cleared immediately on callback arrival (before validation), preventing replay.
Token validation: Uses go-oidc/v3 verifier with ClientID check, which validates signature, issuer, audience, and expiry.
Input validation: Service-layer LoginOIDC refuses unverified email collisions (account-takeover guard) and refuses identities with no email.
SQL injection: All new store methods (GetUserByOIDC, LinkOIDC) use parameterized queries.
Secret handling: clientSecret is not logged or exposed in URLs; errors are logged server-side with generic redirects to the client.
Session fixation protection: startSession issues a new random session token on every OIDC login, same as local auth.
One minor observation: readOIDCTxCookie validates State and Verifier are non-empty but does not validate Nonce. In practice this is not exploitable because randToken() always generates a non-empty nonce and the IdP is expected to echo it back, but adding tx.Nonce == "" to the validation would make the defense-in-depth more explicit.
🎯 Correctness — Minor issues
Verdict: Minor issues
internal/api/oidc.go:253 — readOIDCTxCookie validates that tx.State and tx.Verifier are non-empty, but it does not validate tx.Nonce. While setOIDCTxCookie always populates all three fields, a tampered/malformed cookie could carry an empty nonce. In that case the callback’s manual nonce check (subtle.ConstantTimeCompare(idToken.Nonce, tx.Nonce)) would pass if the IdP happens to omit the nonce claim, weakening replay protection. Add tx.Nonce == "" to the validation condition.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/api/oidc.go:265 — randToken duplicates existing token-generation pattern randToken is a near-exact clone of newSessionToken in internal/service/service.go:62: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper in service or a new internal/util package) so entropy/size requirements don't drift between the two call sites.
internal/api/oidc.go:253 — readOIDCTxCookie inconsistently validates transaction fields
The cookie parser checks tx.State == "" and tx.Verifier == "", but does not check tx.Nonce == "". All three values are generated together in oidcLogin and all three are critical for the callback flow. The validation should be consistent; if a malformed cookie omits the nonce, the failure is deferred to the later ConstantTimeCompare instead of being caught at parse time.
internal/store/users.go:137 — LinkOIDC returns semantically wrong error on OIDC identity collision
A UNIQUE violation on (oidc_issuer, oidc_subject) is mapped to domain.ErrEmailTaken, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicated ErrOIDCIdentityTaken (or similar) would make the code self-documenting and prevent future confusion if any caller starts branching on the error type.
⚡ Performance — No material issues found
No material issues found
Performance lens: Nothing material.
Discovery is correctly lazy-cached with a mutex; only one discovery request is ever made.
The OIDC transaction cookie uses compact base64-JSON encoding, and randToken allocates only a 32-byte slice per call — well within normal bounds for an auth flow.
LoginOIDC performs at most two sequential DB lookups plus one write, which is the minimum possible for the provisioning/linking semantics described.
No hot loops, unbounded growth, or quadratic patterns are introduced.
🧯 Error handling & edge cases — Minor issues
Minor issues found
internal/api/oidc.go:253 — readOIDCTxCookie validates tx.State and tx.Verifier are non-empty after decoding, but does not require tx.Nonce != "".
A tampered cookie with an empty nonce would pass the struct-level check. Then, if the IdP (or a malicious replay) returns an ID token that also omits the nonce, the subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) at line 184 would compare two empty strings and succeed, bypassing the nonce replay protection. Fix: add || tx.Nonce == "" to the validation condition on line 253.
internal/store/users.go:127-142 — LinkOIDC does not inspect RowsAffected after the UPDATE. If the target user is deleted between GetUserByEmail and LinkOIDC, zero rows are updated, yet the code falls through to GetUserByID which then returns ErrNotFound. The caller in oidcCallback treats this as a generic error=oidc redirect, which is safe but confusing. Fix: after ExecContext, check res.RowsAffected() and return ErrNotFound if zero, avoiding a needless second query and giving the service layer a clear signal.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 15 findings (9 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error. | `internal/store/users.go:127` | 5/5 | error-handling, performance |
| 🔴 | readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "". | `internal/api/oidc.go:253` | 4/5 | correctness, error-handling, maintainability |
| 🟡 | LinkOIDC returns ErrEmailTaken for OIDC identity collision | `internal/store/users.go:137` | 4/5 | maintainability |
| 🔴 | oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout. | `internal/api/oidc.go:164` | 2/5 | error-handling |
| 🔴 | LinkOIDC silently overwrites existing OIDC identity on email-linked user | `internal/service/auth.go:93` | 2/5 | correctness |
| 🔴 | LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking. | `internal/service/auth.go:103` | 2/5 | error-handling, security |
| 🟠 | Discovery mutex serializes all OIDC login/callback requests behind a full 10s timeout during IdP outages, with no negative caching/backoff | `internal/api/oidc.go:63` | 2/5 | performance |
| 🟠 | oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper | `internal/api/oidc.go:130` | 2/5 | maintainability |
| 🟡 | OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector | `internal/api/oidc.go:229` | 2/5 | error-handling, maintainability, security |
<details><summary>6 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | writeServiceError missing cases for new ErrOIDCNoEmail / ErrOIDCEmailUnverified sentinels | `internal/api/auth.go:216` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render | `internal/api/oidc.go:215` | glm-5.2:cloud | maintainability |
| 🟡 | randToken duplicates newSessionToken token-generation pattern | `internal/api/oidc.go:265` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding | `internal/service/auth.go:67` | claude-code/sonnet | error-handling |
| ⚪ | empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above | `internal/api/oidc.go:158` | claude-code/sonnet | maintainability |
| ⚪ | Duplicated issuer literal bypasses the oidcIdentity test helper | `internal/service/auth_test.go:293` | opencode/glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Good — `json:"-"` means OIDC identity fields never leak into the `/auth/me` or user JSON responses. I've verified the core security-relevant paths against the actual repo. Nothing material stood out in this lens.
## VERDICT: No material issues found
The OIDC implementation is solid from a security standpoint:
- **CSRF/state**: fresh `state` per login, stashed in an HttpOnly, host-only, `SameSite=Lax` cookie, compared constant-time against the callback param (`internal/api/oidc.go:145-152`). Verified this prevents login-CSRF: an attacker completing their own flow and forwarding the code/state to a victim fails because the victim's browser lacks the matching tx cookie.
- **PKCE**: S256 challenge generated via `oauth2.GenerateVerifier()`/`S256ChallengeOption`, verifier sent only in the server-side token exchange (`oidc.go:127,163-167`) — never exposed to the browser beyond the opaque tx cookie.
- **Nonce**: generated, stashed, and constant-time compared against the verified ID token's nonce (`oidc.go:184-188`) — replay protection independent of `state`.
- **ID token verification**: uses `provider.Verifier(&oidc.Config{ClientID: ...})`, which by default enforces issuer, audience/client-id, signature, and expiry (go-oidc defaults, not overridden here) — confirmed no `SkipClientIDCheck`/`SkipIssuerCheck`/`InsecureSkipSignatureCheck` options are set.
- **Provisioning/account-takeover guard** (`internal/service/auth.go:80-108`): identity resolution is `(issuer,subject)` → verified-email-link → JIT-create, in that order; unverified email collisions and missing `email_verified` claims fail closed to `ErrOIDCEmailUnverified` rather than silently linking. Verified `email_verified` is read as a plain Go `bool` from claims, so a missing claim decodes to `false` (fail-safe).
- **Email normalization** is consistent between `Register`/`Login`/`LoginOIDC` (all route through `normalizeEmail`), and the `users.email` column is `UNIQUE COLLATE NOCASE` (`internal/store/migrations/0001_init.sql:16`), so there's no case-mismatch bypass of the collision check.
- **No open redirect**: all `c.Redirect` targets in the callback are hardcoded literals (`/login?error=...`, `/gardens`); no user-controlled `next`/`redirect_uri` parameter is honored.
- **No secret/token leakage**: access/ID tokens are never logged, only `error` values; `OIDCIssuer`/`OIDCSubject` are tagged `json:"-"` on `domain.User` so they don't leak via `/auth/me` or provisioning responses; client secret only flows into the `oauth2.Config` used for the token exchange.
- **Injection**: all store queries are parameterized (`GetUserByOIDC`, `LinkOIDC`); no string-built SQL from claim data.
- **SSRF surface**: discovery/token-exchange targets (`issuer`, endpoints from discovery) come from operator-set env config, not from request-time/attacker-controlled input, so this isn't attacker-triggerable SSRF.
One thing I checked but didn't flag as a finding: the `pansy_oidc_tx` cookie (state/verifier/nonce) is plain base64 JSON, not HMAC-signed. In principle a party that can already write cookies to this exact host (not a subdomain — the cookie is host-only) could pre-seed known `state`/`verifier` values, but that only lets them complete a login they initiate themselves — it doesn't cross a security boundary against another user's session, so I'm not reporting it as a real vulnerability, just noting it was considered.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
All test helpers exist as expected. I've verified the diff against the checked-out repo (service, store, config, api layers, schema migration, and existing test helpers). No compilation or cross-file inconsistencies found, and the OIDC provisioning logic, PKCE/nonce/state handling, and email-link/JIT decision tree are internally consistent and match the library APIs used.
## Review
**VERDICT: No material issues found**
Findings within the correctness lens, after reading the actual checked-out source (not just the diff):
- `LoginOIDC` resolution order (`internal/service/auth.go:67-109`) is sound: (issuer,subject) lookup guarded against empty strings, verified-email link, then JIT create. Verified the `oidc_issuer`/`oidc_subject` UNIQUE index (`internal/store/migrations/0001_init.sql:27`) and that SQLite treats NULL pairs as distinct, so local-only users (NULL,NULL) can't collide — no false "returning user" match.
- PKCE/nonce/state flow in `internal/api/oidc.go` is correctly wired: `oauth2.GenerateVerifier()` → `oauth2.S256ChallengeOption` on the auth URL → `oauth2.VerifierOption` on `Exchange` (matching verifier/challenge pair), `oidc.Nonce` set on the auth URL and checked against `idToken.Nonce` via constant-time compare, `state` checked before any network call. Order of operations (state check before `ensure()`/token exchange) avoids wasted work on CSRF-failed requests.
- `GetUserByOIDC`/`LinkOIDC` (`internal/store/users.go:108-143`) match the domain `User.OIDCIssuer/OIDCSubject` (`*string`) fields and `userColumns`/`scanUser` correctly.
- `startSession` extraction (`internal/api/auth.go:163-184`) is a straightforward, behavior-preserving refactor reused correctly by both the OIDC callback and the existing local-auth path — verified `startSessionAndRespond` still sets the cookie via the same code path.
- `CreateUser(..., true)` for JIT provisioning correctly reuses the existing atomic first-user-admin/registration-gate INSERT (`internal/store/users.go:54-79`); `allowSignup=true` bypasses the closed-registration gate as intended without disturbing the atomic admin assignment.
- `version` is incremented but not compared in `LinkOIDC`'s `UPDATE` — checked this isn't a new gap; no other store method in this codebase uses `version` for optimistic-concurrency `WHERE` clauses either, so this is consistent with existing (pre-PR) practice, not a regression.
- `csrfGuard` only guards non-GET methods, and both new OIDC routes are GET, so no interaction/bug there.
No logic bugs, no incorrect constants/thresholds (10-minute tx-cookie TTL and 10s discovery timeout are reasonable, non-semantic operational values, not domain-derived quantities), and no broken cross-file references found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Review
**VERDICT: Minor issues**
- `internal/api/oidc.go:130-226` — `oidcCallback` repeats the same three-line shape (`slog.X(...)`; `c.Redirect(http.StatusFound, "/login?error=...")`; `return`) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception, see below). Extracting a small helper (e.g. `func (h *handlers) oidcFail(c *gin.Context, code string, logFn func())`) would cut real repetition and make the function's control flow (provider error → state → discovery → code → exchange → id_token → verify → nonce → claims → provision → session) easier to scan.
- `internal/api/oidc.go:158-162` — the `code == ""` failure path is the only failure branch in `oidcCallback` that doesn't log anything before redirecting, while every sibling branch (`slog.Error`/`slog.Warn`) does. Confirmed directly in the source: lines 159-162 go straight from the `if code == ""` check to `c.Redirect` with no `slog` call. That's an inconsistency introduced by the duplication above — worth fixing at the same time a shared helper is introduced, since a missing-code case is exactly the kind of thing an operator would want in the logs too.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
Confirmed: `LinkOIDC` is called once per OIDC login flow (in `LoginOIDC`), matching the draft's characterization. Both findings hold up against the actual code.
**VERDICT: Minor issues found**
- `internal/api/oidc.go:63-87` (`oidcClient.ensure`) — the discovery mutex is held for the full duration of a failed/slow discovery attempt, and failures aren't negatively cached. `ensure()` does `o.mu.Lock(); defer o.mu.Unlock()` before calling `oidc.NewProvider(dctx, o.issuer)`, a network round-trip bounded only by `oidcDiscoveryTimeout` (10s). Every concurrent call to `/auth/oidc/login` or `/auth/oidc/callback` serializes on this single mutex while the IdP is unreachable/slow, so request *N* can wait up to `N × 10s` before even getting a chance to fail, tying up that many handler goroutines/connections. This is a real availability/perf risk during an IdP outage, though it only manifests then. A `singleflight`-style dedup or a short negative-cache backoff would bound the blast radius.
- `internal/store/users.go:127-143` (`LinkOIDC`) — does an `UPDATE` followed by a separate `GetUserByID` round-trip rather than a single `RETURNING` statement. This runs once per user (first OIDC login linking, confirmed as the only call site in `internal/service/auth.go:93`), so the extra round-trip is negligible — trivial, not blocking.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `internal/service/auth.go:67-109` (`LoginOIDC`) — the returning-user-vs-JIT-create decision is a check-then-act race: `GetUserByOIDC` (a `SELECT`, line 73) and `CreateUser` (an `INSERT`, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new `(issuer, subject)` both miss the lookup and both call `CreateUser`; the loser trips the `idx_users_oidc` UNIQUE index (`internal/store/migrations/0001_init.sql:27`), which `CreateUser` maps to `domain.ErrEmailTaken` (`internal/store/users.go:62-63`) even though no email conflict occurred. This surfaces to the browser only as the generic `error=oidc` redirect (`internal/api/oidc.go:214-218`). Confirmed `internal/store/sqlite.go:46-51` pins `SetMaxOpenConns(1)` only for `:memory:` DSNs, so a real file-backed deployment runs a multi-connection pool and the race is reachable. There's no mutex or transaction anywhere in `LoginOIDC` serializing this. Only sequential re-login is exercised in tests (`internal/service/auth_test.go:310-332`).
- `internal/api/oidc.go:152-183` — discovery is explicitly bounded by `oidcDiscoveryTimeout` (10s, comment at lines 30-32: "a hung IdP can't wedge a request goroutine"), applied via `context.WithTimeout` in `ensure` (line 70). But the token exchange (`h.oidc.oauth.Exchange(ctx, code, ...)`, line 164) and ID-token verification (`h.oidc.verifier.Verify(ctx, rawIDToken)`, line 178, which fetches JWKS on first use) run on the bare request context (`ctx := c.Request.Context()`, line 131) with no deadline applied. Confirmed no timeout middleware or context deadline exists anywhere upstream — `grep` over `internal/api/api.go` for `Timeout`/`context.With` returns nothing, and `cmd/pansy/main.go:63-66` only sets the `http.Server`'s `ReadTimeout`/`ReadHeaderTimeout`/`WriteTimeout`/`IdleTimeout`, none of which cancel `Request.Context()` mid-handler. A slow/hanging IdP token endpoint can wedge a callback-handling goroutine indefinitely — the exact failure mode discovery was hardened against, left open one function down.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
Both findings confirmed against the actual code. Outputting the corrected review.
## VERDICT: Minor issues
Reviewed the OIDC login flow through the security lens (authn/authz, CSRF, injection, session handling, takeover guards). The core design is sound: PKCE S256, random state + nonce, constant-time state/nonce comparison, issuer/subject taken from the *verified* token rather than raw claims, ID-token verification via `go-oidc` (signature/issuer/audience/expiry), unverified-email-collision refusal, no-email refusal, and no error-detail leakage to the browser (provider `?error=` is logged but never reflected; all redirects go to hardcoded `/login?error=<code>` or `/gardens`). Verified `oidc.go:130-226`, `auth.go:67-109`, `users.go:108-143`, and the schema constraints in `0001_init.sql`.
Findings:
- **`internal/api/oidc.go:229-241` — tx cookie is not integrity-protected (low severity).** The `pansy_oidc_tx` cookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't *read* the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residual risk is login-CSRF/session-fixation only if a separate cookie-injection vector exists on the host (in which case the session cookie is already at risk). Noting it because it's the one place untrusted-browser-state is trusted without integrity. Suggested fix if hardening is desired: HMAC the cookie payload with a server key, or move the tx to server-side storage keyed by the state.
- **`internal/service/auth.go:103-108` — `PANSY_REGISTRATION=closed` is silently bypassed by OIDC JIT, and the first OIDC user becomes admin (low confidence, documented as intended).** `LoginOIDC` calls `CreateUser(..., true)`, so every user the IdP admits gets an account regardless of the registration policy, and `CreateUser`'s `is_admin = (count=0)` makes the first such user admin. The code comment states this is intentional ("the IdP gates access"), and the security model is sound *if* the IdP is trusted to gate access. The caveat: an operator who sets `PANSY_REGISTRATION=closed` expecting "no new accounts can be created" may not realize OIDC overrides that, and a misconfigured IdP that allows open signup would hand an attacker the first/admin account. Worth a config-level note or a guardrail (e.g., require an explicit `PANSY_OIDC_JIT=true` opt-in) rather than a code change.
No injection (SQL uses parameterized queries; issuer/subject/email are bound, not interpolated), no SSRF (issuer is admin-configured), no open redirect (all redirect targets are hardcoded), and no authz bypass found.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed the change through the correctness lens, verifying against the checked-out code.
Verified items (all correct):
- **PKCE S256 flow** (`internal/api/oidc.go:115-123,164`): `GenerateVerifier()` → `S256ChallengeOption` at auth URL, `VerifierOption` at exchange. Matches the oauth2 library contract; challenge = `BASE64URL(SHA256(verifier))`.
- **State/nonce binding** (`oidc.go:133-150,184-188`): tx cookie carries state+verifier+nonce; callback does constant-time state compare (short-circuits on missing tx/state) and constant-time nonce compare against the *verified* token's `idToken.Nonce`. Fails closed if the IdP omits nonce (`""` vs non-empty `tx.Nonce`).
- **Issuer/subject provenance** (`oidc.go:206-213`): `Issuer`/`Subject` taken from the verified `idToken`, not raw claims; the go-oidc verifier already checked `iss` against the discovered issuer, so the value stored by `LoginOIDC` is consistent with `GetUserByOIDC`'s lookup key.
- **Provisioning order** (`internal/service/auth.go:67-108`): (issuer,subject) → verified-email link → JIT, with unverified-collision refusal and no-email refusal. Email is normalized (lowercased) before both the collision lookup and `CreateUser`; `GetUserByEmail`'s NOCASE collation agrees. The `[email protected]` link test exercises this.
- **JIT admin bootstrap** (`auth.go:103-108` → `store/users.go:54-78`): `CreateUser(allowSignup=true)` uses `(SELECT count(*) FROM users) = 0` for `is_admin` inside the same INSERT, so the first OIDC user is atomically admin even with `PANSY_LOCAL_AUTH=false` / `PANSY_REGISTRATION=closed`. Test confirms.
- **UNIQUE-index backstop** (`store/users.go:127-143`, migration `0001_init.sql:27`): `LinkOIDC` overwriting a pair already owned by another row trips `idx_users_oidc` → `ErrEmailTaken`; concurrent same-identity JIT logins fail the second INSERT cleanly and resolve on retry via `GetUserByOIDC`. No duplicate accounts.
- **Cookie lifecycle** (`oidc.go:133-134`): tx cookie is cleared at the top of the callback on every path (provider error, state mismatch, success), preventing replay. `MaxAge=600s`, `SameSite=Lax`, `Secure` derived from `PANSY_BASE_URL` consistently with the session cookie.
- **`emailLocalPart`** (`auth.go:283-289`): `i > 0` correctly returns the local part for well-formed emails and falls back to the whole string otherwise.
One design choice I considered and deliberately did *not* flag as a bug: an *unverified* email that does **not** collide with an existing account still JIT-creates an account (`auth.go:88-96` only refuses the *collision* case). That is explicitly the PR's stated design ("an unverified email that collides… is refused"), so it is intended behavior, not a correctness defect.
No correctness issues to report.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings confirmed against the actual source.
VERDICT: Minor issues
- **`internal/api/oidc.go:215` — `LoginOIDC` provisioning failures all collapse to `error=oidc`.** `LoginOIDC` returns distinct sentinels (`ErrOIDCNoEmail`, `ErrOIDCEmailUnverified`, `ErrInvalidInput` — confirmed in `domain/domain.go:39-45`) that the UI could meaningfully distinguish, but the callback maps every one to the generic `?error=oidc` at line 216. This mirrors the existing `writeServiceError` mapping philosophy but discards information that would help the login page show e.g. "email not verified" vs. "provider error". Minor maintainability/UX wart; consider at least routing `ErrOIDCEmailUnverified` to a distinct `?error=email_unverified` code so the UI layer has something to render later.
- **`internal/store/users.go:138` — `LinkOIDC` UNIQUE-collision backstop reuses `ErrEmailTaken` (409 "email already registered") for a non-email conflict.** A duplicate `(issuer, subject)` pair trips the OIDC-identity UNIQUE index and is mapped to `domain.ErrEmailTaken` at line 138, whose doc/comment (`domain.go:28-30`) is explicitly "registration collided with an existing account's email" and whose API message is "an account with that email already exists." Reusing an email-specific sentinel for an OIDC-identity collision is a leaky/misleading abstraction; a future caller that surfaces this error to a user gets the wrong message. Suggested fix: introduce an `ErrOIDCIdentityTaken` (or a generic `ErrConflict`) and map the index violation to it.
- **`internal/api/oidc.go:231-235` — `setOIDCTxCookie` silently returns without setting the cookie on a marshal error.** As the comment notes, `json.Marshal` of the hand-built struct can't realistically fail, so the early `return` after only logging leaves the caller (`oidcLogin` at line 117) to proceed and issue a 302 to the IdP with **no tx cookie** — the subsequent callback would then fail at the state check with `error=state` rather than failing fast at login. This is dead-in-practice but the control flow is subtly wrong (continue-after-failure). Suggested fix: treat it as an internal error and redirect to `/login?error=oidc` instead of falling through to the provider redirect.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've reviewed the diff through the performance lens, checking the new OIDC flow, the lazy discovery caching, and the service/store provisioning queries.
**VERDICT: No material issues found**
I verified the following potential hotspots and they're all fine for this workload:
- **`oidcClient.ensure` (`internal/api/oidc.go:63-87`)** — holds `o.mu` across the discovery network call (up to `oidcDiscoveryTimeout`=10s). This serializes concurrent OIDC requests *only* during cold-start discovery; once `o.provider != nil`, each request takes the lock, checks the nil-guard, and returns. That serialization is actually desirable (prevents a thundering herd of redundant discovery fetches), and OIDC login/callback is a low-frequency path, not a hot loop. Read at `oidc.go:63-87`.
- **`LoginOIDC` (`internal/service/auth.go`) provisioning** — up to 2-3 queries (`GetUserByOIDC` → `GetUserByEmail` → `CreateUser`/`LinkOIDC`). This is a login path with no N+1, no loops, no unbounded growth. Confirmed by reading the service and store code.
- **`LinkOIDC` (`internal/store/users.go:120-141`)** issues an UPDATE then a separate `GetUserByID` SELECT (two round-trips instead of a `RETURNING` single statement). This is a one-time-per-linking operation on a login path, not material.
- No new allocations on hot paths, no missing pagination, no blocking calls inside loops. The tx-cookie marshal/base64 per login is trivial.
No performance regressions introduced by this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings confirmed against the actual code:
1. **`oidc.go:253`** — Confirmed: `readOIDCTxCookie` guards `tx.State == ""` and `tx.Verifier == ""` but not `tx.Nonce == ""`. The nonce check at line 184 is `subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1`; with both empty, this compares equal and passes. However, in the normal flow `nonce` is always set from `randToken()` at line 108 (and `err2` is checked), so the only way `tx.Nonce` is empty is if an attacker tampers the cookie — but the cookie is HttpOnly + signed-by-our-domain... actually it's not signed, just base64 JSON. So a tampered cookie could set Nonce empty. The finding's framing is valid as defense-in-depth.
2. **`users.go:127`** — Confirmed: `LinkOIDC` does `ExecContext` UPDATE, checks error but not `RowsAffected`, then falls through to `GetUserByID`. If zero rows matched (account deleted between `GetUserByEmail` and `LinkOIDC`), `GetUserByID` returns `domain.ErrNotFound`, surfaced as generic `error=oidc`.
3. **`oidc.go:229`** — Confirmed: `setOIDCTxCookie` on marshal error logs and `return`s without setting the cookie; caller at line 117-123 proceeds to redirect to the IdP regardless. The doomed flow would fail at callback with `error=state`. Theoretical since struct holds only strings.
All three survive verification.
Minor issues
- **`internal/api/oidc.go:253` — `readOIDCTxCookie` does not validate that `tx.Nonce` is non-empty.** It guards only `tx.State == ""` and `tx.Verifier == ""` (line 253). The nonce check at line 184 is `subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))`, so an empty `tx.Nonce` combined with an ID token that carries no `nonce` claim (a non-compliant IdP, or a cookie tampered to blank the nonce) compares equal and **passes vacuously**, disabling nonce binding (replay protection) for that flow. State + PKCE still hold, so this is defense-in-depth, not a break. Fix: add `|| tx.Nonce == ""` to the guard so a tx with no nonce is rejected outright. Verified against lines 243–257 and 184.
- **`internal/store/users.go:127` — `LinkOIDC` ignores `RowsAffected`.** The `UPDATE ... WHERE id = ?` (lines 128–135) returns no error when it matches zero rows (e.g. the account was deleted between `GetUserByEmail` and `LinkOIDC`). Execution falls through to `GetUserByID` (line 142), which returns `domain.ErrNotFound`, surfaced to the user as a generic `error=oidc` (via `service.LoginOIDC` at `auth.go:93` and the callback at `oidc.go:215`). No data corruption, but the link silently no-ops and the error is misleading rather than a clear "user gone." Fix: check `res.RowsAffected()`; if 0, return `domain.ErrNotFound` explicitly instead of relying on the follow-up lookup.
- **`internal/api/oidc.go:229` — `setOIDCTxCookie` swallows the marshal failure and lets `oidcLogin` proceed.** On `json.Marshal` error (lines 231–234) it logs and `return`s without setting the cookie, but the caller (lines 117–123) then redirects to the IdP regardless, producing a flow guaranteed to fail at callback with `error=state`. The comment acknowledges marshal "cannot fail in practice" (the struct holds only generated strings), so this is a theoretical edge; a cleaner behavior would treat it as a hard failure and redirect to `/login?error=oidc` rather than launching a doomed redirect.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
The OIDC implementation is solid from a security perspective:
- **PKCE (S256)**: Correctly implemented with `oauth2.GenerateVerifier()` and `oauth2.VerifierOption` on exchange.
- **CSRF protection**: Random 32-byte `state` parameter generated per-login, verified with `subtle.ConstantTimeCompare` on callback.
- **Nonce verification**: Fresh random nonce per-login, checked against the ID token with constant-time comparison after full signature/issuer/audience/expiry verification by `go-oidc`.
- **Cookie hardening**: `pansy_oidc_tx` is HttpOnly, `Secure` (when HTTPS), `SameSite=Lax` (appropriate for the IdP top-level redirect), and cleared immediately on callback.
- **Account takeover guard**: Email linking only occurs when `email_verified=true`; unverified collisions and missing emails are refused.
- **No secret leakage**: `clientSecret` is never logged; token exchange errors are logged without reflecting the authorization `code`.
- **No injection vectors**: All store queries use parameterized SQL; OIDC claims are not interpolated into URLs or HTML.
- **Session hygiene**: `startSession` creates a fresh bearer token — no session fixation risk.
The one thing I chased — `readOIDCTxCookie` not validating `tx.Nonce != ""` — is harmless because an empty nonce would fail the later `subtle.ConstantTimeCompare` against any real IdP token.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
Blocking issues found
* **`internal/service/auth.go:93` — `LinkOIDC` silently overwrites an existing OIDC identity**
`LoginOIDC` resolves a user by email and unconditionally calls `LinkOIDC`, which executes an `UPDATE` that stomps any existing `oidc_issuer` / `oidc_subject` on that row. The comment on `LinkOIDC` says it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issuer A / subject A) and later authenticates via IdP B with the same verified email, her OIDC identity is overwritten to IdP B. She can no longer log in via IdP A. Verified by reading `internal/service/auth.go:86-93` and `internal/store/users.go:127-143`.
**Fix:** Before linking, verify `existing.OIDCIssuer == nil && existing.OIDCSubject == nil`; if the user already has OIDC credentials, return a distinct error (e.g., `ErrOIDCAlreadyLinked` or treat it as `ErrEmailTaken`) instead of overwriting.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- **`internal/store/users.go:138`**: `LinkOIDC` maps a UNIQUE violation on `(oidc_issuer, oidc_subject)` to `domain.ErrEmailTaken`. The comment acknowledges this is a "generic conflict" backstop, but reusing an error whose name and message literally mean "email already registered" for an OIDC identity collision is a leaky abstraction. A future maintainer calling `LinkOIDC` from a new code path (or simply reading traces/logs) will be misled. Add a domain-wide `ErrConflict` (or an OIDC-specific `ErrOIDCIdentityTaken`) and use it here instead.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`internal/api/oidc.go:230-234`** — `setOIDCTxCookie` logs and silently returns when `json.Marshal` fails, leaving `oidcLogin` to redirect the browser to the IdP without the transaction cookie. The callback will then fail with a generic `error=state`. While `json.Marshal` on a struct of strings cannot practically fail, the error should be propagated so the caller can redirect immediately with an informative error code rather than sending the user through a broken round-trip.
- **`internal/api/oidc.go:253`** — `readOIDCTxCookie` validates `tx.State != ""` and `tx.Verifier != ""` but does not check `tx.Nonce != ""`. If a malformed cookie omits the nonce, the function returns `true`, and the later nonce comparison at line 184 will still fail (assuming the IdP echoes back the original nonce), but validating all three fields is cleaner defense-in-depth.
- **`internal/store/users.go:127-143`** — `LinkOIDC` issues an `UPDATE ... WHERE id = ?` but never checks `RowsAffected`. If the user row is deleted between the preceding `GetUserByEmail` and this call, `ExecContext` returns no error and `GetUserByID` subsequently returns `ErrNotFound`. Checking `RowsAffected == 0` and returning `ErrNotFound` directly would make the failure mode explicit.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
VERDICT: Minor issues
- **`internal/service/auth.go:93` / `internal/store/users.go:127` — verified-email linking can overwrite an existing, different OIDC identity (account lockout/takeover).** `LoginOIDC`'s step 2 (`auth.go:87-93`) calls `LinkOIDC` whenever a *verified* email matches an existing account, with no check that the matched account doesn't already carry a *different* OIDC identity. `LinkOIDC` (`store/users.go:127-143`) unconditionally `UPDATE`s `oidc_issuer`/`oidc_subject` regardless of their current values. Flow: (1) local user Bob registers with `[email protected]`; (2) Bob's first OIDC login with subject `sub-A` + verified email links → `oidc_subject=sub-A`; (3) a later login with a *different* subject `sub-B` and the *same verified* email reaches step 2 (`GetUserByOIDC(issuer, sub-B)` → `ErrNotFound` since `sub-B` ≠ `sub-A`, `GetUserByEmail` → finds Bob, `EmailVerified=true`), and `LinkOIDC` **overwrites** `sub-A` with `sub-B`. Bob is now locked out of OIDC and the `sub-B` holder controls the account. The "verified email" guard does not prevent this; both identities present a verified email. Suggested fix: in step 2, refuse to link when `existing.OIDCIssuer != nil && (*existing.OIDCIssuer != id.Issuer || *existing.OIDCSubject != id.Subject)`, returning a sentinel like `domain.ErrOIDCIdentityConflict` instead of silently overwriting.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All four findings verified against the actual source:
1. **oidcCallback redirect repetition** — Confirmed: 11 `c.Redirect(http.StatusFound, "/login?error=...")` sites in `oidc.go` lines 130-226 (eight with `error=oidc`), plus 2 more in `oidcLogin`. The handler is ~95 lines with deeply nested failure paths.
2. **LinkOIDC → ErrEmailTaken misnomer** — Confirmed at `store/users.go:137-138`; the comment at lines 122-126 explicitly calls it a "(issuer,subject)" backstop that "shouldn't happen," yet maps to a sentinel named "email already registered."
3. **writeServiceError missing new sentinels** — Confirmed: `auth.go:216-231` switch has no cases for `ErrOIDCNoEmail`/`ErrOIDCEmailUnverified` (defined at `domain.go:39-45`). The only current caller (`oidcCallback`) bypasses `writeServiceError` and redirects, so this is latent.
4. **Duplicated issuer literal** — Confirmed: `"https://idp.example"` at `auth_test.go:293`, `:300`, and inside the `oidcIdentity` helper at `:307`.
VERDICT: Minor issues
- **`oidcCallback` is a long, repetitive function with ~11 near-identical failure-redirect sites** (`internal/api/oidc.go:130-226`). Eight are `c.Redirect(http.StatusFound, "/login?error=oidc"); return` preceded by an `slog.*` call with slightly varying keys. A small helper like `oidcFail(c, code string, logMsg string, logAttrs ...any)` would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a glance.
- **`LinkOIDC` maps an `(issuer, subject)` UNIQUE collision to `ErrEmailTaken`** (`internal/store/users.go:138`). The sentinel's name and docstring ("email already registered") describe a different constraint than the one being hit; the comment on line 122-126 even admits it "shouldn't happen." A maintainer tracing an OIDC provisioning failure through `LoginOIDC` would be misled into thinking the email collided. A dedicated `ErrOIDCIdentityTaken` (or a generic `ErrConflict`) would communicate the actual cause.
- **`writeServiceError` is missing cases for the two new domain sentinels** (`internal/api/auth.go:216-231`). `ErrOIDCNoEmail` and `ErrOIDCEmailUnverified` were added in `domain/domain.go:39-45` but the central service→HTTP mapping table wasn't updated. Today the only caller (`oidcCallback`) bypasses `writeServiceError` and redirects, so this is latent rather than broken — but leaving the table incomplete invites a future caller to fall through to the `default: INTERNAL` branch for what is really a client-side problem.
- **Duplicated issuer string `"https://idp.example"` in `TestProvidersReflectsConfig`** (`internal/service/auth_test.go:293,300`) — the same constant is also hardcoded inside the `oidcIdentity` helper (line 307). The helper exists to centralize this; the two extra inlines bypass it.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `internal/api/oidc.go:63-76` — `oidcClient.ensure` holds `o.mu` for the entire duration of `oidc.NewProvider`, a network call bounded to `oidcDiscoveryTimeout` (10s). Concurrent `/oidc/login` and `/oidc/callback` requests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: the `if o.provider != nil { return nil }` fast-path (line 66) only short-circuits after a successful discovery, so the lock-during-I/O only bites on the first cold request(s). Impact is bounded to the cold-start window and a low-QPS endpoint, so it's minor — but the standard fix is: check-then-release-lock → do discovery unlocked → re-acquire and double-check before assigning (or use a singleflight-style in-flight marker). No other serialization on this path (no per-request DB write beyond the normal session insert).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
I've verified each finding against the actual code. Here's my corrected review:
# Review — 🧯 Error handling & edge cases
**Verdict: Blocking issues found**
- **`internal/api/oidc.go:164` — token exchange and ID-token verification run on the unbounded request context.** `ctx := c.Request.Context()` (line 131) is passed directly to `h.oidc.oauth.Exchange(ctx, ...)` (line 164) and `h.oidc.verifier.Verify(ctx, rawIDToken)` (line 178). Only `ensure()` gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server's `WriteTimeout` is 30s (`cmd/pansy/main.go:65`), and the tx cookie is already cleared at line 134 before exchange, so a slow IdP can leave the user on a blank page with the tx cookie already consumed. Fix: wrap the exchange+verify block in `context.WithTimeout(ctx, ...)` with `defer cancel()`.
- **`internal/api/oidc.go:253` — `readOIDCTxCookie` does not require `tx.Nonce`.** The validity check at line 253 is `tx.State == "" || tx.Verifier == ""`; `Nonce` is not required. A hand-crafted or stale cookie yielding a tx with empty `Nonce` passes the check, and the nonce compare at line 184 (`ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))`) succeeds whenever the IdP also returns an empty nonce (many IdPs omit the `nonce` claim, leaving `idToken.Nonce == ""`), so the comparison becomes `"" == ""` and replay/mix-up protection silently degrades to a no-op. Fix: require `tx.Nonce != ""` in `readOIDCTxCookie` and reject `idToken.Nonce == ""` at verify time.
- **`internal/store/users.go:127` — `LinkOIDC` ignores `RowsAffected() == 0`.** `ExecContext` (lines 128-135) returns no error and `RowsAffected()` is never checked; the function then calls `GetUserByID(ctx, userID)` (line 142), which returns `domain.ErrNotFound` for a non-existent userID. A `LinkOIDC` against a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: check `RowsAffected()` and return a wrapped error on 0.
- **`internal/service/auth.go:103` — `LoginOIDC` JIT-creates an account (which may become the first/admin user) when `email_verified` is false and the email doesn't collide.** The email-collision branch (lines 88-96) correctly requires `id.EmailVerified` before linking, but the JIT-creation fallthrough (lines 98-108) does not. With `PANSY_LOCAL_AUTH=false`, an identity whose unverified email matches no existing account is provisioned, and `store.CreateUser` makes it admin iff the table is empty (`users.go:57`), so the first user to log in via OIDC with an *unverified* email becomes admin. Fix: require `id.EmailVerified` for JIT provisioning, not only for linking.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
After a thorough security review of the OIDC implementation, I verified the following:
- **PKCE**: Uses `oauth2.S256ChallengeOption` with a randomly generated verifier, properly stashed in the HttpOnly `pansy_oidc_tx` cookie and verified during code exchange.
- **CSRF protection**: Random `state` parameter generated via `crypto/rand`, compared with `subtle.ConstantTimeCompare` against the cookie value.
- **Nonce verification**: Random `nonce` generated via `crypto/rand`, verified with `subtle.ConstantTimeCompare` against the ID token claim.
- **Cookie security**: `pansy_oidc_tx` is HttpOnly, SameSite=Lax, and Secure when `PANSY_BASE_URL` is HTTPS. The cookie is cleared immediately on callback arrival (before validation), preventing replay.
- **Token validation**: Uses `go-oidc/v3` verifier with `ClientID` check, which validates signature, issuer, audience, and expiry.
- **Input validation**: Service-layer `LoginOIDC` refuses unverified email collisions (account-takeover guard) and refuses identities with no email.
- **SQL injection**: All new store methods (`GetUserByOIDC`, `LinkOIDC`) use parameterized queries.
- **Secret handling**: `clientSecret` is not logged or exposed in URLs; errors are logged server-side with generic redirects to the client.
- **Session fixation protection**: `startSession` issues a new random session token on every OIDC login, same as local auth.
One minor observation: `readOIDCTxCookie` validates `State` and `Verifier` are non-empty but does not validate `Nonce`. In practice this is not exploitable because `randToken()` always generates a non-empty nonce and the IdP is expected to echo it back, but adding `tx.Nonce == ""` to the validation would make the defense-in-depth more explicit.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict:** Minor issues
* **`internal/api/oidc.go:253`** — `readOIDCTxCookie` validates that `tx.State` and `tx.Verifier` are non-empty, but it does not validate `tx.Nonce`. While `setOIDCTxCookie` always populates all three fields, a tampered/malformed cookie could carry an empty nonce. In that case the callback’s manual nonce check (`subtle.ConstantTimeCompare(idToken.Nonce, tx.Nonce)`) would pass if the IdP happens to omit the nonce claim, weakening replay protection. Add `tx.Nonce == ""` to the validation condition.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`internal/api/oidc.go:265` — `randToken` duplicates existing token-generation pattern**
`randToken` is a near-exact clone of `newSessionToken` in `internal/service/service.go:62`: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper in `service` or a new `internal/util` package) so entropy/size requirements don't drift between the two call sites.
- **`internal/api/oidc.go:253` — `readOIDCTxCookie` inconsistently validates transaction fields**
The cookie parser checks `tx.State == ""` and `tx.Verifier == ""`, but does *not* check `tx.Nonce == ""`. All three values are generated together in `oidcLogin` and all three are critical for the callback flow. The validation should be consistent; if a malformed cookie omits the nonce, the failure is deferred to the later `ConstantTimeCompare` instead of being caught at parse time.
- **`internal/store/users.go:137` — `LinkOIDC` returns semantically wrong error on OIDC identity collision**
A UNIQUE violation on `(oidc_issuer, oidc_subject)` is mapped to `domain.ErrEmailTaken`, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicated `ErrOIDCIdentityTaken` (or similar) would make the code self-documenting and prevent future confusion if any caller starts branching on the error type.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- **Performance lens**: Nothing material.
- Discovery is correctly lazy-cached with a mutex; only one discovery request is ever made.
- The OIDC transaction cookie uses compact base64-JSON encoding, and `randToken` allocates only a 32-byte slice per call — well within normal bounds for an auth flow.
- `LoginOIDC` performs at most two sequential DB lookups plus one write, which is the minimum possible for the provisioning/linking semantics described.
- No hot loops, unbounded growth, or quadratic patterns are introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues found**
- `internal/api/oidc.go:253` — `readOIDCTxCookie` validates `tx.State` and `tx.Verifier` are non-empty after decoding, but **does not require `tx.Nonce != ""`**.
A tampered cookie with an empty nonce would pass the struct-level check. Then, if the IdP (or a malicious replay) returns an ID token that also omits the nonce, the `subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))` at line 184 would compare two empty strings and succeed, bypassing the nonce replay protection.
**Fix:** add `|| tx.Nonce == ""` to the validation condition on line 253.
- `internal/store/users.go:127-142` — `LinkOIDC` does not inspect `RowsAffected` after the `UPDATE`. If the target user is deleted between `GetUserByEmail` and `LinkOIDC`, zero rows are updated, yet the code falls through to `GetUserByID` which then returns `ErrNotFound`. The caller in `oidcCallback` treats this as a generic `error=oidc` redirect, which is safe but confusing.
**Fix:** after `ExecContext`, check `res.RowsAffected()` and return `ErrNotFound` if zero, avoiding a needless second query and giving the service layer a clear signal.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Fixes from the PR #24 adversarial review (graded 23 real / 1 false positive):
Security / correctness
- LinkOIDC no longer overwrites a different stored identity: the UPDATE
matches only when the row has no identity yet or already carries this
exact one, so a second IdP asserting the same verified email can't
hijack or lock out an account (returns ErrOIDCIdentityConflict). Uses
a single UPDATE...RETURNING (also fixes the ignored-RowsAffected /
misleading-ErrNotFound path and the round-trip).
- Provisioning now requires a verified email for BOTH linking and JIT
creation (was: linking only), so an unverified-email identity can't
create an account — nor become the first admin on a fresh instance,
nor squat an email a real user later owns.
- OIDC-identity collisions surface as the dedicated ErrOIDCIdentityConflict
instead of the email-specific ErrEmailTaken.
Robustness
- readOIDCTxCookie requires a non-empty nonce (an empty one would make the
callback's nonce check pass vacuously).
- Callback token exchange + verify run under a 15s context timeout so a
slow IdP can't outlast the server write timeout.
- ensure() performs discovery outside the mutex, so concurrent cold-start
requests don't serialize behind one another's full timeout.
- setOIDCTxCookie returns its marshal error; oidcLogin aborts rather than
redirecting to the IdP with no tx cookie.
Maintainability
- redirectAuthError helper dedups the ~dozen callback redirects (and the
empty-code path now logs like the rest).
- Distinct login error codes (no_email / email_unverified / oidc_conflict)
for the UI; writeServiceError maps the OIDC sentinels; shared test issuer
const.
Tests: unverified email refused for both link and JIT; identity-overwrite
refused while the original identity keeps working.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve
merged commit 92eb64a790 into main2026-07-18 21:34:36 +00:00
steve
deleted branch phase-1-oidc2026-07-18 21:34:37 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #5. Phase 1, step 2 of the tracking epic #20. Builds on #4 (same session cookie for both paths).
What's here
API (
internal/api/oidc.go)GET /auth/oidc/login— authorization-code + PKCE (S256) with randomstate+nonce, stashed in a short-lived HttpOnlypansy_oidc_txcookie (SameSite=Lax so it survives the IdP's top-level redirect back), then redirects to the IdP.GET /auth/oidc/callback— verifiesstate(constant-time), exchanges the code with the PKCE verifier, verifies the ID token signature/issuer/audience/expiry and nonce, then provisions and starts a pansy session. Every failure clears the tx cookie and redirects to/login?error=…; success →/gardens.oidcClient.ensure): first request performs discovery (10s timeout) and caches it; a failure is retried on the next request, so a momentarily-unreachable IdP never stops the server — or local auth — from starting.PANSY_BASE_URL), so an unconfigured instance 404s them, matching what/auth/providersadvertises.Service (
internal/service/auth.go) —LoginOIDCprovisioning order:(issuer, subject)match → returning user;PANSY_REGISTRATIONis bypassed) — reusing #4's atomicCreateUser(first user still becomes admin atomically).Security choices: an unverified email that collides with an existing account is refused (account-takeover guard); an identity with no email is refused (email is the account key).
Store:
GetUserByOIDC,LinkOIDC(UNIQUE-pair backstop).Config:
OIDCReady();/auth/providersreportsoidcfrom it; button label default → "Sign in with Authentik".PANSY_LOCAL_AUTH=falserejects/hides local auth but leaves OIDC working.Verification
go build/go vet/go test ./...green (CGO off, GOWORK=off). go-oidc + oauth2 are pure Go.code_challenge/S256/state/nonce+ tx cookie, callback state-missing →error=state, provider-error →error=oidc./auth/oidc/login→ 302error=oidc_unavailable, server stays up, local register/login still work, providers reportsoidc:true.Out of scope / notes
🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-18 21:23:42Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +60,4 @@// ensure performs discovery once (idempotent) and builds the verifier + oauth2// config. Safe for concurrent callers; a failure leaves the client uninitialized// so the next request retries.func (o *oidcClient) ensure(ctx context.Context) error {🟠 Discovery mutex serializes all OIDC login/callback requests behind a full 10s timeout during IdP outages, with no negative caching/backoff
performance · flagged by 2 models
internal/api/oidc.go:63-76—oidcClient.ensureholdso.mufor the entire duration ofoidc.NewProvider, a network call bounded tooidcDiscoveryTimeout(10s). Concurrent/oidc/loginand/oidc/callbackrequests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: theif o.provider != nil { return nil }fast-path (line 66) only…🪰 Gadfly · advisory
@@ -0,0 +127,4 @@// verifier), verify the ID token and nonce, provision/link the user, and start a// pansy session. Every failure clears the tx cookie and redirects to the login// page with an error code rather than leaking details to the browser.func (h *handlers) oidcCallback(c *gin.Context) {🟠 oidcCallback repeats ~11 near-identical /login?error=oidc redirect sites; extract a helper
maintainability · flagged by 2 models
oidcCallbackis a long, repetitive function with ~11 near-identical failure-redirect sites (internal/api/oidc.go:130-226). Eight arec.Redirect(http.StatusFound, "/login?error=oidc"); returnpreceded by anslog.*call with slightly varying keys. A small helper likeoidcFail(c, code string, logMsg string, logAttrs ...any)would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a gl…🪰 Gadfly · advisory
@@ -0,0 +155,4 @@return}code := c.Query("code")⚪ empty-code failure path skips the slog call every other branch has, an inconsistency from the duplication above
maintainability · flagged by 1 model
internal/api/oidc.go:130-226—oidcCallbackrepeats the same three-line shape (slog.X(...);c.Redirect(http.StatusFound, "/login?error=...");return) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception,…🪰 Gadfly · advisory
@@ -0,0 +161,4 @@return}token, err := h.oidc.oauth.Exchange(ctx, code, oauth2.VerifierOption(tx.Verifier))🔴 oauth2.Exchange and idToken.Verify run on the unbounded request context; a slow IdP can exceed the server's WriteTimeout (30s), leaving the user on a blank page with the tx cookie already cleared. Wrap the exchange+verify block in context.WithTimeout.
error-handling · flagged by 2 models
internal/api/oidc.go:164— token exchange and ID-token verification run on the unbounded request context.ctx := c.Request.Context()(line 131) is passed directly toh.oidc.oauth.Exchange(ctx, ...)(line 164) andh.oidc.verifier.Verify(ctx, rawIDToken)(line 178). Onlyensure()gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server'sWriteTimeoutis 30s (cmd/pansy/main.go:65), and the tx cookie is already cleared at line 134 befo…🪰 Gadfly · advisory
@@ -0,0 +212,4 @@Name: name,})if err != nil {slog.Warn("api: oidc provisioning failed", "error", err)🟡 All LoginOIDC provisioning errors collapse to generic error=oidc, discarding distinct sentinels (no-email, unverified-email) the UI could render
maintainability · flagged by 1 model
internal/api/oidc.go:215—LoginOIDCprovisioning failures all collapse toerror=oidc.LoginOIDCreturns distinct sentinels (ErrOIDCNoEmail,ErrOIDCEmailUnverified,ErrInvalidInput— confirmed indomain/domain.go:39-45) that the UI could meaningfully distinguish, but the callback maps every one to the generic?error=oidcat line 216. This mirrors the existingwriteServiceErrormapping philosophy but discards information that would help the login page show e.g. "email no…🪰 Gadfly · advisory
@@ -0,0 +226,4 @@}// setOIDCTxCookie writes the login transaction as a compact base64 JSON cookie.func (h *handlers) setOIDCTxCookie(c *gin.Context, tx oidcTx) {🟡 OIDC tx cookie holds PKCE verifier/nonce/state without integrity protection (HMAC/signature); standard pattern but relies on absence of any cookie-injection vector
error-handling, maintainability, security · flagged by 2 models
internal/api/oidc.go:229-241— tx cookie is not integrity-protected (low severity). Thepansy_oidc_txcookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't read the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residua…🪰 Gadfly · advisory
@@ -0,0 +250,4 @@return oidcTx{}, false}var tx oidcTxif err := json.Unmarshal(raw, &tx); err != nil || tx.State == "" || tx.Verifier == "" {🔴 readOIDCTxCookie does not require tx.Nonce, so an empty nonce in both tx and token passes ConstantTimeCompare (""==""), silently disabling nonce/replay protection. Require tx.Nonce != "" and reject idToken.Nonce == "".
correctness, error-handling, maintainability · flagged by 4 models
internal/api/oidc.go:253—readOIDCTxCookiedoes not requiretx.Nonce. The validity check at line 253 istx.State == "" || tx.Verifier == "";Nonceis not required. A hand-crafted or stale cookie yielding a tx with emptyNoncepasses the check, and the nonce compare at line 184 (ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))) succeeds whenever the IdP also returns an empty nonce (many IdPs omit thenonceclaim, leavingidToken.Nonce == ""), so the comparison…🪰 Gadfly · advisory
@@ -0,0 +262,4 @@}// randToken returns 32 bytes of URL-safe randomness for state/nonce values.func randToken() (string, error) {🟡 randToken duplicates newSessionToken token-generation pattern
maintainability · flagged by 1 model
internal/api/oidc.go:265—randTokenduplicates existing token-generation patternrandTokenis a near-exact clone ofnewSessionTokenininternal/service/service.go:62: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper inserviceor a newinternal/utilpackage) so entropy/size requirements don't drift between the two call sites.🪰 Gadfly · advisory
@@ -44,0 +64,4 @@// An unverified email that collides with an existing account is refused (it// would let anyone who can assert that email at the IdP take over the account).// An identity with no email can't be provisioned (email is the account key).func (s *Service) LoginOIDC(ctx context.Context, id OIDCIdentity) (*domain.User, error) {🟡 TOCTOU race between GetUserByOIDC and CreateUser lets concurrent JIT logins for the same new identity fail with a misleading ErrEmailTaken instead of one succeeding
error-handling · flagged by 1 model
internal/service/auth.go:67-109(LoginOIDC) — the returning-user-vs-JIT-create decision is a check-then-act race:GetUserByOIDC(aSELECT, line 73) andCreateUser(anINSERT, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new(issuer, subject)both miss the lookup and both callCreateUser; the loser trips theidx_users_oidcUNIQUE index (internal/store/migrations/0001_init.sql:27), whichCreateUsermaps to `domain.ErrEmailTak…🪰 Gadfly · advisory
@@ -44,0 +90,4 @@if !id.EmailVerified {return nil, domain.ErrOIDCEmailUnverified}return s.store.LinkOIDC(ctx, existing.ID, id.Issuer, id.Subject)🔴 LinkOIDC silently overwrites existing OIDC identity on email-linked user
correctness · flagged by 2 models
internal/service/auth.go:93—LinkOIDCsilently overwrites an existing OIDC identityLoginOIDCresolves a user by email and unconditionally callsLinkOIDC, which executes anUPDATEthat stomps any existingoidc_issuer/oidc_subjecton that row. The comment onLinkOIDCsays it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issue…🪰 Gadfly · advisory
@@ -44,0 +100,4 @@if name == "" {name = emailLocalPart(email)}return s.store.CreateUser(ctx, &domain.User{🔴 LoginOIDC JIT-creates an account (and may make it the first/admin user) when email_verified is false and the email doesn't collide; with PANSY_LOCAL_AUTH=false an unverified-email IdP user can become the first admin. Require EmailVerified for JIT provisioning, not only for linking.
error-handling, security · flagged by 2 models
internal/service/auth.go:103—LoginOIDCJIT-creates an account (which may become the first/admin user) whenemail_verifiedis false and the email doesn't collide. The email-collision branch (lines 88-96) correctly requiresid.EmailVerifiedbefore linking, but the JIT-creation fallthrough (lines 98-108) does not. WithPANSY_LOCAL_AUTH=false, an identity whose unverified email matches no existing account is provisioned, andstore.CreateUsermakes it admin iff the table is empty…🪰 Gadfly · advisory
@@ -293,0 +290,4 @@// OIDC configured with a base URL → reported ready.ready := openConfig()ready.BaseURL = "https://pansy.example.com"ready.OIDC = config.OIDCConfig{Issuer: "https://idp.example", ClientID: "cid", ButtonLabel: "Sign in with Authentik"}⚪ Duplicated issuer literal bypasses the oidcIdentity test helper
maintainability · flagged by 1 model
"https://idp.example"inTestProvidersReflectsConfig(internal/service/auth_test.go:293,300) — the same constant is also hardcoded inside theoidcIdentityhelper (line 307). The helper exists to centralize this; the two extra inlines bypass it.🪰 Gadfly · advisory
@@ -108,0 +124,4 @@// another user's identity pair trips the UNIQUE index and maps to ErrEmailTaken// as a generic conflict (it shouldn't happen — the caller looks up by identity// first — but the index is the backstop).func (d *DB) LinkOIDC(ctx context.Context, userID int64, issuer, subject string) (*domain.User, error) {🟠 LinkOIDC ignores RowsAffected()==0; an UPDATE against a non-existent userID silently succeeds at the UPDATE step and only surfaces as ErrNotFound from the trailing GetUserByID, masking a programming error.
error-handling, performance · flagged by 5 models
internal/store/users.go:127—LinkOIDCignoresRowsAffected() == 0.ExecContext(lines 128-135) returns no error andRowsAffected()is never checked; the function then callsGetUserByID(ctx, userID)(line 142), which returnsdomain.ErrNotFoundfor a non-existent userID. ALinkOIDCagainst a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: checkRowsAffected()and return a…🪰 Gadfly · advisory
@@ -108,0 +134,4 @@issuer, subject, userID,)if err != nil {if isUniqueViolation(err) {🟡 LinkOIDC returns ErrEmailTaken for OIDC identity collision
maintainability · flagged by 4 models
internal/store/users.go:137—LinkOIDCreturns semantically wrong error on OIDC identity collision A UNIQUE violation on(oidc_issuer, oidc_subject)is mapped todomain.ErrEmailTaken, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicatedErrOIDCIdentityTaken(or similar) would make the code self-documenting and prevent future confusion i…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 15 findings (9 with multi-model agreement)
internal/store/users.go:127internal/api/oidc.go:253internal/store/users.go:137internal/api/oidc.go:164internal/service/auth.go:93internal/service/auth.go:103internal/api/oidc.go:63internal/api/oidc.go:130internal/api/oidc.go:2296 single-model findings (lower confidence)
internal/api/auth.go:216internal/api/oidc.go:215internal/api/oidc.go:265internal/service/auth.go:67internal/api/oidc.go:158internal/service/auth_test.go:293Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Good —
json:"-"means OIDC identity fields never leak into the/auth/meor user JSON responses. I've verified the core security-relevant paths against the actual repo. Nothing material stood out in this lens.VERDICT: No material issues found
The OIDC implementation is solid from a security standpoint:
stateper login, stashed in an HttpOnly, host-only,SameSite=Laxcookie, compared constant-time against the callback param (internal/api/oidc.go:145-152). Verified this prevents login-CSRF: an attacker completing their own flow and forwarding the code/state to a victim fails because the victim's browser lacks the matching tx cookie.oauth2.GenerateVerifier()/S256ChallengeOption, verifier sent only in the server-side token exchange (oidc.go:127,163-167) — never exposed to the browser beyond the opaque tx cookie.oidc.go:184-188) — replay protection independent ofstate.provider.Verifier(&oidc.Config{ClientID: ...}), which by default enforces issuer, audience/client-id, signature, and expiry (go-oidc defaults, not overridden here) — confirmed noSkipClientIDCheck/SkipIssuerCheck/InsecureSkipSignatureCheckoptions are set.internal/service/auth.go:80-108): identity resolution is(issuer,subject)→ verified-email-link → JIT-create, in that order; unverified email collisions and missingemail_verifiedclaims fail closed toErrOIDCEmailUnverifiedrather than silently linking. Verifiedemail_verifiedis read as a plain Goboolfrom claims, so a missing claim decodes tofalse(fail-safe).Register/Login/LoginOIDC(all route throughnormalizeEmail), and theusers.emailcolumn isUNIQUE COLLATE NOCASE(internal/store/migrations/0001_init.sql:16), so there's no case-mismatch bypass of the collision check.c.Redirecttargets in the callback are hardcoded literals (/login?error=...,/gardens); no user-controllednext/redirect_uriparameter is honored.errorvalues;OIDCIssuer/OIDCSubjectare taggedjson:"-"ondomain.Userso they don't leak via/auth/meor provisioning responses; client secret only flows into theoauth2.Configused for the token exchange.GetUserByOIDC,LinkOIDC); no string-built SQL from claim data.issuer, endpoints from discovery) come from operator-set env config, not from request-time/attacker-controlled input, so this isn't attacker-triggerable SSRF.One thing I checked but didn't flag as a finding: the
pansy_oidc_txcookie (state/verifier/nonce) is plain base64 JSON, not HMAC-signed. In principle a party that can already write cookies to this exact host (not a subdomain — the cookie is host-only) could pre-seed knownstate/verifiervalues, but that only lets them complete a login they initiate themselves — it doesn't cross a security boundary against another user's session, so I'm not reporting it as a real vulnerability, just noting it was considered.🎯 Correctness — No material issues found
All test helpers exist as expected. I've verified the diff against the checked-out repo (service, store, config, api layers, schema migration, and existing test helpers). No compilation or cross-file inconsistencies found, and the OIDC provisioning logic, PKCE/nonce/state handling, and email-link/JIT decision tree are internally consistent and match the library APIs used.
Review
VERDICT: No material issues found
Findings within the correctness lens, after reading the actual checked-out source (not just the diff):
LoginOIDCresolution order (internal/service/auth.go:67-109) is sound: (issuer,subject) lookup guarded against empty strings, verified-email link, then JIT create. Verified theoidc_issuer/oidc_subjectUNIQUE index (internal/store/migrations/0001_init.sql:27) and that SQLite treats NULL pairs as distinct, so local-only users (NULL,NULL) can't collide — no false "returning user" match.internal/api/oidc.gois correctly wired:oauth2.GenerateVerifier()→oauth2.S256ChallengeOptionon the auth URL →oauth2.VerifierOptiononExchange(matching verifier/challenge pair),oidc.Nonceset on the auth URL and checked againstidToken.Noncevia constant-time compare,statechecked before any network call. Order of operations (state check beforeensure()/token exchange) avoids wasted work on CSRF-failed requests.GetUserByOIDC/LinkOIDC(internal/store/users.go:108-143) match the domainUser.OIDCIssuer/OIDCSubject(*string) fields anduserColumns/scanUsercorrectly.startSessionextraction (internal/api/auth.go:163-184) is a straightforward, behavior-preserving refactor reused correctly by both the OIDC callback and the existing local-auth path — verifiedstartSessionAndRespondstill sets the cookie via the same code path.CreateUser(..., true)for JIT provisioning correctly reuses the existing atomic first-user-admin/registration-gate INSERT (internal/store/users.go:54-79);allowSignup=truebypasses the closed-registration gate as intended without disturbing the atomic admin assignment.versionis incremented but not compared inLinkOIDC'sUPDATE— checked this isn't a new gap; no other store method in this codebase usesversionfor optimistic-concurrencyWHEREclauses either, so this is consistent with existing (pre-PR) practice, not a regression.csrfGuardonly guards non-GET methods, and both new OIDC routes are GET, so no interaction/bug there.No logic bugs, no incorrect constants/thresholds (10-minute tx-cookie TTL and 10s discovery timeout are reasonable, non-semantic operational values, not domain-derived quantities), and no broken cross-file references found.
🧹 Code cleanliness & maintainability — Minor issues
Review
VERDICT: Minor issues
internal/api/oidc.go:130-226—oidcCallbackrepeats the same three-line shape (slog.X(...);c.Redirect(http.StatusFound, "/login?error=...");return) across essentially every failure branch. Verified by reading the full function: failure exits occur at lines 137-141, 145-150, 152-156, 158-162, 164-169, 171-176, 178-183, 184-188, 196-200, 214-218, and 220-224 — eleven branches total, ten of which follow the log+redirect+return shape (the code == "" branch at 158-162 is the exception, see below). Extracting a small helper (e.g.func (h *handlers) oidcFail(c *gin.Context, code string, logFn func())) would cut real repetition and make the function's control flow (provider error → state → discovery → code → exchange → id_token → verify → nonce → claims → provision → session) easier to scan.internal/api/oidc.go:158-162— thecode == ""failure path is the only failure branch inoidcCallbackthat doesn't log anything before redirecting, while every sibling branch (slog.Error/slog.Warn) does. Confirmed directly in the source: lines 159-162 go straight from theif code == ""check toc.Redirectwith noslogcall. That's an inconsistency introduced by the duplication above — worth fixing at the same time a shared helper is introduced, since a missing-code case is exactly the kind of thing an operator would want in the logs too.⚡ Performance — Minor issues
Confirmed:
LinkOIDCis called once per OIDC login flow (inLoginOIDC), matching the draft's characterization. Both findings hold up against the actual code.VERDICT: Minor issues found
internal/api/oidc.go:63-87(oidcClient.ensure) — the discovery mutex is held for the full duration of a failed/slow discovery attempt, and failures aren't negatively cached.ensure()doeso.mu.Lock(); defer o.mu.Unlock()before callingoidc.NewProvider(dctx, o.issuer), a network round-trip bounded only byoidcDiscoveryTimeout(10s). Every concurrent call to/auth/oidc/loginor/auth/oidc/callbackserializes on this single mutex while the IdP is unreachable/slow, so request N can wait up toN × 10sbefore even getting a chance to fail, tying up that many handler goroutines/connections. This is a real availability/perf risk during an IdP outage, though it only manifests then. Asingleflight-style dedup or a short negative-cache backoff would bound the blast radius.internal/store/users.go:127-143(LinkOIDC) — does anUPDATEfollowed by a separateGetUserByIDround-trip rather than a singleRETURNINGstatement. This runs once per user (first OIDC login linking, confirmed as the only call site ininternal/service/auth.go:93), so the extra round-trip is negligible — trivial, not blocking.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
internal/service/auth.go:67-109(LoginOIDC) — the returning-user-vs-JIT-create decision is a check-then-act race:GetUserByOIDC(aSELECT, line 73) andCreateUser(anINSERT, line 103) are separate round-trips with no transaction. Two concurrent callbacks for the same new(issuer, subject)both miss the lookup and both callCreateUser; the loser trips theidx_users_oidcUNIQUE index (internal/store/migrations/0001_init.sql:27), whichCreateUsermaps todomain.ErrEmailTaken(internal/store/users.go:62-63) even though no email conflict occurred. This surfaces to the browser only as the genericerror=oidcredirect (internal/api/oidc.go:214-218). Confirmedinternal/store/sqlite.go:46-51pinsSetMaxOpenConns(1)only for:memory:DSNs, so a real file-backed deployment runs a multi-connection pool and the race is reachable. There's no mutex or transaction anywhere inLoginOIDCserializing this. Only sequential re-login is exercised in tests (internal/service/auth_test.go:310-332).internal/api/oidc.go:152-183— discovery is explicitly bounded byoidcDiscoveryTimeout(10s, comment at lines 30-32: "a hung IdP can't wedge a request goroutine"), applied viacontext.WithTimeoutinensure(line 70). But the token exchange (h.oidc.oauth.Exchange(ctx, code, ...), line 164) and ID-token verification (h.oidc.verifier.Verify(ctx, rawIDToken), line 178, which fetches JWKS on first use) run on the bare request context (ctx := c.Request.Context(), line 131) with no deadline applied. Confirmed no timeout middleware or context deadline exists anywhere upstream —grepoverinternal/api/api.goforTimeout/context.Withreturns nothing, andcmd/pansy/main.go:63-66only sets thehttp.Server'sReadTimeout/ReadHeaderTimeout/WriteTimeout/IdleTimeout, none of which cancelRequest.Context()mid-handler. A slow/hanging IdP token endpoint can wedge a callback-handling goroutine indefinitely — the exact failure mode discovery was hardened against, left open one function down.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings confirmed against the actual code. Outputting the corrected review.
VERDICT: Minor issues
Reviewed the OIDC login flow through the security lens (authn/authz, CSRF, injection, session handling, takeover guards). The core design is sound: PKCE S256, random state + nonce, constant-time state/nonce comparison, issuer/subject taken from the verified token rather than raw claims, ID-token verification via
go-oidc(signature/issuer/audience/expiry), unverified-email-collision refusal, no-email refusal, and no error-detail leakage to the browser (provider?error=is logged but never reflected; all redirects go to hardcoded/login?error=<code>or/gardens). Verifiedoidc.go:130-226,auth.go:67-109,users.go:108-143, and the schema constraints in0001_init.sql.Findings:
internal/api/oidc.go:229-241— tx cookie is not integrity-protected (low severity). Thepansy_oidc_txcookie is plain base64 JSON holding the PKCE verifier + nonce + state, with no HMAC/signature. The state CSRF check still holds (an attacker can't read the victim's cookie, and the cookie is host-only + HttpOnly + SameSite=Lax), and the verifier alone is useless without a matching authorization code, so this is the common "PKCE-in-cookie" pattern rather than a clear hole. The residual risk is login-CSRF/session-fixation only if a separate cookie-injection vector exists on the host (in which case the session cookie is already at risk). Noting it because it's the one place untrusted-browser-state is trusted without integrity. Suggested fix if hardening is desired: HMAC the cookie payload with a server key, or move the tx to server-side storage keyed by the state.internal/service/auth.go:103-108—PANSY_REGISTRATION=closedis silently bypassed by OIDC JIT, and the first OIDC user becomes admin (low confidence, documented as intended).LoginOIDCcallsCreateUser(..., true), so every user the IdP admits gets an account regardless of the registration policy, andCreateUser'sis_admin = (count=0)makes the first such user admin. The code comment states this is intentional ("the IdP gates access"), and the security model is sound if the IdP is trusted to gate access. The caveat: an operator who setsPANSY_REGISTRATION=closedexpecting "no new accounts can be created" may not realize OIDC overrides that, and a misconfigured IdP that allows open signup would hand an attacker the first/admin account. Worth a config-level note or a guardrail (e.g., require an explicitPANSY_OIDC_JIT=trueopt-in) rather than a code change.No injection (SQL uses parameterized queries; issuer/subject/email are bound, not interpolated), no SSRF (issuer is admin-configured), no open redirect (all redirect targets are hardcoded), and no authz bypass found.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the change through the correctness lens, verifying against the checked-out code.
Verified items (all correct):
internal/api/oidc.go:115-123,164):GenerateVerifier()→S256ChallengeOptionat auth URL,VerifierOptionat exchange. Matches the oauth2 library contract; challenge =BASE64URL(SHA256(verifier)).oidc.go:133-150,184-188): tx cookie carries state+verifier+nonce; callback does constant-time state compare (short-circuits on missing tx/state) and constant-time nonce compare against the verified token'sidToken.Nonce. Fails closed if the IdP omits nonce (""vs non-emptytx.Nonce).oidc.go:206-213):Issuer/Subjecttaken from the verifiedidToken, not raw claims; the go-oidc verifier already checkedissagainst the discovered issuer, so the value stored byLoginOIDCis consistent withGetUserByOIDC's lookup key.internal/service/auth.go:67-108): (issuer,subject) → verified-email link → JIT, with unverified-collision refusal and no-email refusal. Email is normalized (lowercased) before both the collision lookup andCreateUser;GetUserByEmail's NOCASE collation agrees. The[email protected]link test exercises this.auth.go:103-108→store/users.go:54-78):CreateUser(allowSignup=true)uses(SELECT count(*) FROM users) = 0foris_admininside the same INSERT, so the first OIDC user is atomically admin even withPANSY_LOCAL_AUTH=false/PANSY_REGISTRATION=closed. Test confirms.store/users.go:127-143, migration0001_init.sql:27):LinkOIDCoverwriting a pair already owned by another row tripsidx_users_oidc→ErrEmailTaken; concurrent same-identity JIT logins fail the second INSERT cleanly and resolve on retry viaGetUserByOIDC. No duplicate accounts.oidc.go:133-134): tx cookie is cleared at the top of the callback on every path (provider error, state mismatch, success), preventing replay.MaxAge=600s,SameSite=Lax,Securederived fromPANSY_BASE_URLconsistently with the session cookie.emailLocalPart(auth.go:283-289):i > 0correctly returns the local part for well-formed emails and falls back to the whole string otherwise.One design choice I considered and deliberately did not flag as a bug: an unverified email that does not collide with an existing account still JIT-creates an account (
auth.go:88-96only refuses the collision case). That is explicitly the PR's stated design ("an unverified email that collides… is refused"), so it is intended behavior, not a correctness defect.No correctness issues to report.
🧹 Code cleanliness & maintainability — Minor issues
All three findings confirmed against the actual source.
VERDICT: Minor issues
internal/api/oidc.go:215—LoginOIDCprovisioning failures all collapse toerror=oidc.LoginOIDCreturns distinct sentinels (ErrOIDCNoEmail,ErrOIDCEmailUnverified,ErrInvalidInput— confirmed indomain/domain.go:39-45) that the UI could meaningfully distinguish, but the callback maps every one to the generic?error=oidcat line 216. This mirrors the existingwriteServiceErrormapping philosophy but discards information that would help the login page show e.g. "email not verified" vs. "provider error". Minor maintainability/UX wart; consider at least routingErrOIDCEmailUnverifiedto a distinct?error=email_unverifiedcode so the UI layer has something to render later.internal/store/users.go:138—LinkOIDCUNIQUE-collision backstop reusesErrEmailTaken(409 "email already registered") for a non-email conflict. A duplicate(issuer, subject)pair trips the OIDC-identity UNIQUE index and is mapped todomain.ErrEmailTakenat line 138, whose doc/comment (domain.go:28-30) is explicitly "registration collided with an existing account's email" and whose API message is "an account with that email already exists." Reusing an email-specific sentinel for an OIDC-identity collision is a leaky/misleading abstraction; a future caller that surfaces this error to a user gets the wrong message. Suggested fix: introduce anErrOIDCIdentityTaken(or a genericErrConflict) and map the index violation to it.internal/api/oidc.go:231-235—setOIDCTxCookiesilently returns without setting the cookie on a marshal error. As the comment notes,json.Marshalof the hand-built struct can't realistically fail, so the earlyreturnafter only logging leaves the caller (oidcLoginat line 117) to proceed and issue a 302 to the IdP with no tx cookie — the subsequent callback would then fail at the state check witherror=staterather than failing fast at login. This is dead-in-practice but the control flow is subtly wrong (continue-after-failure). Suggested fix: treat it as an internal error and redirect to/login?error=oidcinstead of falling through to the provider redirect.⚡ Performance — No material issues found
I've reviewed the diff through the performance lens, checking the new OIDC flow, the lazy discovery caching, and the service/store provisioning queries.
VERDICT: No material issues found
I verified the following potential hotspots and they're all fine for this workload:
oidcClient.ensure(internal/api/oidc.go:63-87) — holdso.muacross the discovery network call (up tooidcDiscoveryTimeout=10s). This serializes concurrent OIDC requests only during cold-start discovery; onceo.provider != nil, each request takes the lock, checks the nil-guard, and returns. That serialization is actually desirable (prevents a thundering herd of redundant discovery fetches), and OIDC login/callback is a low-frequency path, not a hot loop. Read atoidc.go:63-87.LoginOIDC(internal/service/auth.go) provisioning — up to 2-3 queries (GetUserByOIDC→GetUserByEmail→CreateUser/LinkOIDC). This is a login path with no N+1, no loops, no unbounded growth. Confirmed by reading the service and store code.LinkOIDC(internal/store/users.go:120-141) issues an UPDATE then a separateGetUserByIDSELECT (two round-trips instead of aRETURNINGsingle statement). This is a one-time-per-linking operation on a login path, not material.No performance regressions introduced by this change.
🧯 Error handling & edge cases — Minor issues
All three findings confirmed against the actual code:
oidc.go:253— Confirmed:readOIDCTxCookieguardstx.State == ""andtx.Verifier == ""but nottx.Nonce == "". The nonce check at line 184 issubtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1; with both empty, this compares equal and passes. However, in the normal flownonceis always set fromrandToken()at line 108 (anderr2is checked), so the only waytx.Nonceis empty is if an attacker tampers the cookie — but the cookie is HttpOnly + signed-by-our-domain... actually it's not signed, just base64 JSON. So a tampered cookie could set Nonce empty. The finding's framing is valid as defense-in-depth.users.go:127— Confirmed:LinkOIDCdoesExecContextUPDATE, checks error but notRowsAffected, then falls through toGetUserByID. If zero rows matched (account deleted betweenGetUserByEmailandLinkOIDC),GetUserByIDreturnsdomain.ErrNotFound, surfaced as genericerror=oidc.oidc.go:229— Confirmed:setOIDCTxCookieon marshal error logs andreturns without setting the cookie; caller at line 117-123 proceeds to redirect to the IdP regardless. The doomed flow would fail at callback witherror=state. Theoretical since struct holds only strings.All three survive verification.
Minor issues
internal/api/oidc.go:253—readOIDCTxCookiedoes not validate thattx.Nonceis non-empty. It guards onlytx.State == ""andtx.Verifier == ""(line 253). The nonce check at line 184 issubtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)), so an emptytx.Noncecombined with an ID token that carries nononceclaim (a non-compliant IdP, or a cookie tampered to blank the nonce) compares equal and passes vacuously, disabling nonce binding (replay protection) for that flow. State + PKCE still hold, so this is defense-in-depth, not a break. Fix: add|| tx.Nonce == ""to the guard so a tx with no nonce is rejected outright. Verified against lines 243–257 and 184.internal/store/users.go:127—LinkOIDCignoresRowsAffected. TheUPDATE ... WHERE id = ?(lines 128–135) returns no error when it matches zero rows (e.g. the account was deleted betweenGetUserByEmailandLinkOIDC). Execution falls through toGetUserByID(line 142), which returnsdomain.ErrNotFound, surfaced to the user as a genericerror=oidc(viaservice.LoginOIDCatauth.go:93and the callback atoidc.go:215). No data corruption, but the link silently no-ops and the error is misleading rather than a clear "user gone." Fix: checkres.RowsAffected(); if 0, returndomain.ErrNotFoundexplicitly instead of relying on the follow-up lookup.internal/api/oidc.go:229—setOIDCTxCookieswallows the marshal failure and letsoidcLoginproceed. Onjson.Marshalerror (lines 231–234) it logs andreturns without setting the cookie, but the caller (lines 117–123) then redirects to the IdP regardless, producing a flow guaranteed to fail at callback witherror=state. The comment acknowledges marshal "cannot fail in practice" (the struct holds only generated strings), so this is a theoretical edge; a cleaner behavior would treat it as a hard failure and redirect to/login?error=oidcrather than launching a doomed redirect.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The OIDC implementation is solid from a security perspective:
oauth2.GenerateVerifier()andoauth2.VerifierOptionon exchange.stateparameter generated per-login, verified withsubtle.ConstantTimeCompareon callback.go-oidc.pansy_oidc_txis HttpOnly,Secure(when HTTPS),SameSite=Lax(appropriate for the IdP top-level redirect), and cleared immediately on callback.email_verified=true; unverified collisions and missing emails are refused.clientSecretis never logged; token exchange errors are logged without reflecting the authorizationcode.startSessioncreates a fresh bearer token — no session fixation risk.The one thing I chased —
readOIDCTxCookienot validatingtx.Nonce != ""— is harmless because an empty nonce would fail the latersubtle.ConstantTimeCompareagainst any real IdP token.🎯 Correctness — Blocking issues found
Blocking issues found
internal/service/auth.go:93—LinkOIDCsilently overwrites an existing OIDC identityLoginOIDCresolves a user by email and unconditionally callsLinkOIDC, which executes anUPDATEthat stomps any existingoidc_issuer/oidc_subjecton that row. The comment onLinkOIDCsays it is meant for "first OIDC login for a pre-existing local account", but the code does not check that the matched user is local-only. If Alice originally logged in via IdP A (so her row already has issuer A / subject A) and later authenticates via IdP B with the same verified email, her OIDC identity is overwritten to IdP B. She can no longer log in via IdP A. Verified by readinginternal/service/auth.go:86-93andinternal/store/users.go:127-143.Fix: Before linking, verify
existing.OIDCIssuer == nil && existing.OIDCSubject == nil; if the user already has OIDC credentials, return a distinct error (e.g.,ErrOIDCAlreadyLinkedor treat it asErrEmailTaken) instead of overwriting.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/store/users.go:138:LinkOIDCmaps a UNIQUE violation on(oidc_issuer, oidc_subject)todomain.ErrEmailTaken. The comment acknowledges this is a "generic conflict" backstop, but reusing an error whose name and message literally mean "email already registered" for an OIDC identity collision is a leaky abstraction. A future maintainer callingLinkOIDCfrom a new code path (or simply reading traces/logs) will be misled. Add a domain-wideErrConflict(or an OIDC-specificErrOIDCIdentityTaken) and use it here instead.⚡ Performance — No material issues found
No material issues found
🧯 Error handling & edge cases — Minor issues
Minor issues
internal/api/oidc.go:230-234—setOIDCTxCookielogs and silently returns whenjson.Marshalfails, leavingoidcLoginto redirect the browser to the IdP without the transaction cookie. The callback will then fail with a genericerror=state. Whilejson.Marshalon a struct of strings cannot practically fail, the error should be propagated so the caller can redirect immediately with an informative error code rather than sending the user through a broken round-trip.internal/api/oidc.go:253—readOIDCTxCookievalidatestx.State != ""andtx.Verifier != ""but does not checktx.Nonce != "". If a malformed cookie omits the nonce, the function returnstrue, and the later nonce comparison at line 184 will still fail (assuming the IdP echoes back the original nonce), but validating all three fields is cleaner defense-in-depth.internal/store/users.go:127-143—LinkOIDCissues anUPDATE ... WHERE id = ?but never checksRowsAffected. If the user row is deleted between the precedingGetUserByEmailand this call,ExecContextreturns no error andGetUserByIDsubsequently returnsErrNotFound. CheckingRowsAffected == 0and returningErrNotFounddirectly would make the failure mode explicit.opencode/glm-5.2:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🎯 Correctness — Minor issues
VERDICT: Minor issues
internal/service/auth.go:93/internal/store/users.go:127— verified-email linking can overwrite an existing, different OIDC identity (account lockout/takeover).LoginOIDC's step 2 (auth.go:87-93) callsLinkOIDCwhenever a verified email matches an existing account, with no check that the matched account doesn't already carry a different OIDC identity.LinkOIDC(store/users.go:127-143) unconditionallyUPDATEsoidc_issuer/oidc_subjectregardless of their current values. Flow: (1) local user Bob registers with[email protected]; (2) Bob's first OIDC login with subjectsub-A+ verified email links →oidc_subject=sub-A; (3) a later login with a different subjectsub-Band the same verified email reaches step 2 (GetUserByOIDC(issuer, sub-B)→ErrNotFoundsincesub-B≠sub-A,GetUserByEmail→ finds Bob,EmailVerified=true), andLinkOIDCoverwritessub-Awithsub-B. Bob is now locked out of OIDC and thesub-Bholder controls the account. The "verified email" guard does not prevent this; both identities present a verified email. Suggested fix: in step 2, refuse to link whenexisting.OIDCIssuer != nil && (*existing.OIDCIssuer != id.Issuer || *existing.OIDCSubject != id.Subject), returning a sentinel likedomain.ErrOIDCIdentityConflictinstead of silently overwriting.🧹 Code cleanliness & maintainability — Minor issues
All four findings verified against the actual source:
c.Redirect(http.StatusFound, "/login?error=...")sites inoidc.golines 130-226 (eight witherror=oidc), plus 2 more inoidcLogin. The handler is ~95 lines with deeply nested failure paths.store/users.go:137-138; the comment at lines 122-126 explicitly calls it a "(issuer,subject)" backstop that "shouldn't happen," yet maps to a sentinel named "email already registered."auth.go:216-231switch has no cases forErrOIDCNoEmail/ErrOIDCEmailUnverified(defined atdomain.go:39-45). The only current caller (oidcCallback) bypasseswriteServiceErrorand redirects, so this is latent."https://idp.example"atauth_test.go:293,:300, and inside theoidcIdentityhelper at:307.VERDICT: Minor issues
oidcCallbackis a long, repetitive function with ~11 near-identical failure-redirect sites (internal/api/oidc.go:130-226). Eight arec.Redirect(http.StatusFound, "/login?error=oidc"); returnpreceded by anslog.*call with slightly varying keys. A small helper likeoidcFail(c, code string, logMsg string, logAttrs ...any)would collapse these to one-liners and shrink the handler by ~40 lines, making the actual flow (state → exchange → verify → provision → session) readable at a glance.LinkOIDCmaps an(issuer, subject)UNIQUE collision toErrEmailTaken(internal/store/users.go:138). The sentinel's name and docstring ("email already registered") describe a different constraint than the one being hit; the comment on line 122-126 even admits it "shouldn't happen." A maintainer tracing an OIDC provisioning failure throughLoginOIDCwould be misled into thinking the email collided. A dedicatedErrOIDCIdentityTaken(or a genericErrConflict) would communicate the actual cause.writeServiceErroris missing cases for the two new domain sentinels (internal/api/auth.go:216-231).ErrOIDCNoEmailandErrOIDCEmailUnverifiedwere added indomain/domain.go:39-45but the central service→HTTP mapping table wasn't updated. Today the only caller (oidcCallback) bypasseswriteServiceErrorand redirects, so this is latent rather than broken — but leaving the table incomplete invites a future caller to fall through to thedefault: INTERNALbranch for what is really a client-side problem.Duplicated issuer string
"https://idp.example"inTestProvidersReflectsConfig(internal/service/auth_test.go:293,300) — the same constant is also hardcoded inside theoidcIdentityhelper (line 307). The helper exists to centralize this; the two extra inlines bypass it.⚡ Performance — Minor issues
VERDICT: Minor issues
internal/api/oidc.go:63-76—oidcClient.ensureholdso.mufor the entire duration ofoidc.NewProvider, a network call bounded tooidcDiscoveryTimeout(10s). Concurrent/oidc/loginand/oidc/callbackrequests serialize behind the first discovery attempt; if the IdP is slow at cold start, each waiting request blocks sequentially for up to 10s rather than all failing fast or sharing one in-flight discovery. Verified: theif o.provider != nil { return nil }fast-path (line 66) only short-circuits after a successful discovery, so the lock-during-I/O only bites on the first cold request(s). Impact is bounded to the cold-start window and a low-QPS endpoint, so it's minor — but the standard fix is: check-then-release-lock → do discovery unlocked → re-acquire and double-check before assigning (or use a singleflight-style in-flight marker). No other serialization on this path (no per-request DB write beyond the normal session insert).🧯 Error handling & edge cases — Blocking issues found
I've verified each finding against the actual code. Here's my corrected review:
Review — 🧯 Error handling & edge cases
Verdict: Blocking issues found
internal/api/oidc.go:164— token exchange and ID-token verification run on the unbounded request context.ctx := c.Request.Context()(line 131) is passed directly toh.oidc.oauth.Exchange(ctx, ...)(line 164) andh.oidc.verifier.Verify(ctx, rawIDToken)(line 178). Onlyensure()gets a 10s timeout (line 70); the network calls to the IdP token endpoint and JWKS do not. The server'sWriteTimeoutis 30s (cmd/pansy/main.go:65), and the tx cookie is already cleared at line 134 before exchange, so a slow IdP can leave the user on a blank page with the tx cookie already consumed. Fix: wrap the exchange+verify block incontext.WithTimeout(ctx, ...)withdefer cancel().internal/api/oidc.go:253—readOIDCTxCookiedoes not requiretx.Nonce. The validity check at line 253 istx.State == "" || tx.Verifier == "";Nonceis not required. A hand-crafted or stale cookie yielding a tx with emptyNoncepasses the check, and the nonce compare at line 184 (ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))) succeeds whenever the IdP also returns an empty nonce (many IdPs omit thenonceclaim, leavingidToken.Nonce == ""), so the comparison becomes"" == ""and replay/mix-up protection silently degrades to a no-op. Fix: requiretx.Nonce != ""inreadOIDCTxCookieand rejectidToken.Nonce == ""at verify time.internal/store/users.go:127—LinkOIDCignoresRowsAffected() == 0.ExecContext(lines 128-135) returns no error andRowsAffected()is never checked; the function then callsGetUserByID(ctx, userID)(line 142), which returnsdomain.ErrNotFoundfor a non-existent userID. ALinkOIDCagainst a deleted/never-existed user is thus indistinguishable from a successful link to a present account at the UPDATE step, masking a programming error. Fix: checkRowsAffected()and return a wrapped error on 0.internal/service/auth.go:103—LoginOIDCJIT-creates an account (which may become the first/admin user) whenemail_verifiedis false and the email doesn't collide. The email-collision branch (lines 88-96) correctly requiresid.EmailVerifiedbefore linking, but the JIT-creation fallthrough (lines 98-108) does not. WithPANSY_LOCAL_AUTH=false, an identity whose unverified email matches no existing account is provisioned, andstore.CreateUsermakes it admin iff the table is empty (users.go:57), so the first user to log in via OIDC with an unverified email becomes admin. Fix: requireid.EmailVerifiedfor JIT provisioning, not only for linking.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
After a thorough security review of the OIDC implementation, I verified the following:
oauth2.S256ChallengeOptionwith a randomly generated verifier, properly stashed in the HttpOnlypansy_oidc_txcookie and verified during code exchange.stateparameter generated viacrypto/rand, compared withsubtle.ConstantTimeCompareagainst the cookie value.noncegenerated viacrypto/rand, verified withsubtle.ConstantTimeCompareagainst the ID token claim.pansy_oidc_txis HttpOnly, SameSite=Lax, and Secure whenPANSY_BASE_URLis HTTPS. The cookie is cleared immediately on callback arrival (before validation), preventing replay.go-oidc/v3verifier withClientIDcheck, which validates signature, issuer, audience, and expiry.LoginOIDCrefuses unverified email collisions (account-takeover guard) and refuses identities with no email.GetUserByOIDC,LinkOIDC) use parameterized queries.clientSecretis not logged or exposed in URLs; errors are logged server-side with generic redirects to the client.startSessionissues a new random session token on every OIDC login, same as local auth.One minor observation:
readOIDCTxCookievalidatesStateandVerifierare non-empty but does not validateNonce. In practice this is not exploitable becauserandToken()always generates a non-empty nonce and the IdP is expected to echo it back, but addingtx.Nonce == ""to the validation would make the defense-in-depth more explicit.🎯 Correctness — Minor issues
Verdict: Minor issues
internal/api/oidc.go:253—readOIDCTxCookievalidates thattx.Stateandtx.Verifierare non-empty, but it does not validatetx.Nonce. WhilesetOIDCTxCookiealways populates all three fields, a tampered/malformed cookie could carry an empty nonce. In that case the callback’s manual nonce check (subtle.ConstantTimeCompare(idToken.Nonce, tx.Nonce)) would pass if the IdP happens to omit the nonce claim, weakening replay protection. Addtx.Nonce == ""to the validation condition.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/api/oidc.go:265—randTokenduplicates existing token-generation patternrandTokenis a near-exact clone ofnewSessionTokenininternal/service/service.go:62: both generate 32 random bytes and base64-raw-URL-encode them. The only difference is the error message. This is copy-paste that should be shared (e.g. extract a small exported helper inserviceor a newinternal/utilpackage) so entropy/size requirements don't drift between the two call sites.internal/api/oidc.go:253—readOIDCTxCookieinconsistently validates transaction fieldsThe cookie parser checks
tx.State == ""andtx.Verifier == "", but does not checktx.Nonce == "". All three values are generated together inoidcLoginand all three are critical for the callback flow. The validation should be consistent; if a malformed cookie omits the nonce, the failure is deferred to the laterConstantTimeCompareinstead of being caught at parse time.internal/store/users.go:137—LinkOIDCreturns semantically wrong error on OIDC identity collisionA UNIQUE violation on
(oidc_issuer, oidc_subject)is mapped todomain.ErrEmailTaken, whose name and message ("email already registered") describe an entirely different collision. The comment calls this a "generic conflict," but no generic conflict sentinel exists. Adding a dedicatedErrOIDCIdentityTaken(or similar) would make the code self-documenting and prevent future confusion if any caller starts branching on the error type.⚡ Performance — No material issues found
No material issues found
randTokenallocates only a 32-byte slice per call — well within normal bounds for an auth flow.LoginOIDCperforms at most two sequential DB lookups plus one write, which is the minimum possible for the provisioning/linking semantics described.🧯 Error handling & edge cases — Minor issues
Minor issues found
internal/api/oidc.go:253—readOIDCTxCookievalidatestx.Stateandtx.Verifierare non-empty after decoding, but does not requiretx.Nonce != "".A tampered cookie with an empty nonce would pass the struct-level check. Then, if the IdP (or a malicious replay) returns an ID token that also omits the nonce, the
subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce))at line 184 would compare two empty strings and succeed, bypassing the nonce replay protection.Fix: add
|| tx.Nonce == ""to the validation condition on line 253.internal/store/users.go:127-142—LinkOIDCdoes not inspectRowsAffectedafter theUPDATE. If the target user is deleted betweenGetUserByEmailandLinkOIDC, zero rows are updated, yet the code falls through toGetUserByIDwhich then returnsErrNotFound. The caller inoidcCallbacktreats this as a genericerror=oidcredirect, which is safe but confusing.Fix: after
ExecContext, checkres.RowsAffected()and returnErrNotFoundif zero, avoiding a needless second query and giving the service layer a clear signal.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.