[daily-grumpy-reviewer] Daily Grumpy Review - 2026-06-17

[github-actions]

19 grumpies found today. Every single one of them is a recurring pattern — nothing has been fixed since yesterday. The champion, token-expiry-check-inconsistency, has now appeared in 25 consecutive daily reviews. I'm no longer grumpy; I'm resigned. One new pattern added: maintenance-startcleanup-bare-slog.

📋 Full Grumpy Report

Review Overview

Metric	Value
Files Reviewed	24 source + 22 test files
Grumpies Found	19
Recurring Patterns	18 (all of them)
New Patterns	1 (maintenance-startcleanup-bare-slog)
Review Date	2026-06-17

---

🐹 Go Backend

HTTP Status Code Inconsistency (1 issue)

File	Location	Issue	Suggestion
handler/magiclink.go	~L130, ~L139	VerifyMagicLink returns HTTP 401 for an invalid/expired token	Return HTTP 400 to match ResetPassword and VerifyEmail — or change all three to 401. Pick one.

25th consecutive run (☠️ ASCENDED TO MYTH ☠️): MagicLinkHandler.VerifyMagicLink returns 401 for an expired token; PasswordResetHandler.ResetPassword and EmailVerificationHandler.VerifyEmail return 400 for the same class of error. Clients submitting a bad token to a magic-link endpoint get 401; clients submitting a bad token to a password-reset endpoint get 400. Inconsistent for no reason. Pick a convention and apply it everywhere.

---

Logging (4 issues)

File	Line	Issue	Suggestion
handler/helpers.go	~L108, L115, L134, L141, L155	issueTokens() calls bare slog.ErrorContext instead of accepting a *slog.Logger	Add logger *slog.Logger param; callers already have h.Logger
handler/oauth2_common.go	~L115, L170, L177	linkSubjectBestEffort() and createLinkNonce() use bare slog.WarnContext/slog.ErrorContext	Thread a *slog.Logger parameter through these package-level helpers
handler/oauth2_common.go	~L307	handleLinkCallback's inner errRedirect closure calls bare slog.ErrorContext	Pass the logger parameter that handleLinkCallback already receives into the closure
maintenance/maintenance.go	~L54, L62	StartCleanup uses bare slog.ErrorContext for panic recovery and cleaner errors	Add an optional *slog.Logger parameter so consuming applications can route cleanup logs through their configured logger

12th run for issueTokens-bare-slog. 14th run for handlelinkcallback-uses-global-slog. maintenance-startcleanup-bare-slog is new today. The irony: logOrDefault() exists exactly for this pattern, and three of the four files already import it.

---

Documentation & Comments (5 issues)

File	Location	Issue	Suggestion
handler/oauth2.go	OAuth2UserInfo.Name doc ~L22	Says "When empty, Email is used as a fallback" — but findOrCreateUser passes the empty name directly to CreateOIDCUser. No fallback exists.	Implement the fallback or remove the lie from the doc
handler/password_reset.go	Struct comment ~L21-23, RequestReset godoc ~L62-64	Both claim "Returns 503 if SendResetEmail is nil" — but Validate() now requires it. This code path is unreachable.	Remove the 503 mention from both docstrings
handler/auth.go	AuthHandler struct ~L33-36	CookieName, SecureCookies, DisableSignup, RequireVerification are exported fields with no godoc. Fields above them are documented.	Add one-line godoc per field
handler/auth.go + 4 others	Validate() methods	All handlers validate Users, JWT, session config — but NOT CookieName. An empty CookieName silently accepts requests and sets nameless cookies.	Add if h.CookieName == "" { return errors.New("AuthHandler misconfigured: CookieName is required") } to all five handlers
handler/oidc.go	OIDCHandler.Callback() ~L200	Hardcodes "/?oidc_login=1" redirect. OAuth2Handler has a configurable LoginRedirect field for exactly this.	Add LoginRedirect string to OIDCHandler with a matching helper

13th run for oauth2userinfo-name-doc-stale. 7th run for password-reset-handler-stale-503-docstring and auth-handler-undocumented-exported-fields. 11th run for cookiename-not-validated-at-startup. 2nd run for oidc-vs-oauth2-login-redirect-inconsistency.

---

Code Structure (5 issues)

File	Location	Issue	Suggestion
handler/helpers.go	issueTokens() ~L122-124	Inlines TTL fallback instead of using the existing defaultDuration() helper	ttl = defaultDuration(ttl, DefaultRefreshTokenTTL) — one line
handler/magiclink.go vs handler/email_verification.go	TTL computation	MagicLinkHandler wraps TTL in tokenTTL() method; EmailVerificationHandler inlines defaultDuration()	Pick one style and apply it consistently
handler/helpers.go	deleteUserResource()	9 parameters including 4 distinct string message args. Every call site passes nearly identical strings.	Collapse logMessage and internalMessage (identical at every call site) or use a config struct
handler/auth.go	Logout() ~L199-213	4 levels of nested if for a 15-line function	Extract tryRevokeSession() helper, flatten to 2 nesting levels
handler/oidc.go	NewOIDCHandler() ~L51	9 positional parameters, 5 adjacent strings. Easy to silently swap.	All other handlers use struct config — do the same

7th run for issueTokens-defaultduration-inconsistency and magiclinkhandler-tokenttl-method-inconsistency. 6th run for deleteUserResource-parameter-overload. 5th run for logout-deep-nesting. 2nd run for newoIDchandler-parameter-overload.

---

WebAuthn / Passkey (2 issues)

File	Location	Issue	Suggestion
handler/passkey.go	loadWebAuthnCredentials() ~L124-126	Logs "skipping corrupted passkey credential" without slog.Any("error", err) — the actual error is silently dropped	Add , slog.Any("error", err) to the WarnContext call
handler/passkey.go	FinishAuthentication() ~L323-348	Uses 5 mutable outer-scope variables mutated inside a DiscoverableUserHandler closure	Extract a resolvePasskeyUser() helper that returns cleanly instead of side-effecting captured variables

7th run for loadwebauthn-credentials-error-dropped. 11th run for finishauthentication-closure-side-effects. The missing slog.Any("error", err) is literally a copy-paste from the line above it.

---

🧪 Testing

Critical Test Bug (1 issue)

File	Test	Issue	Suggestion
auth/totp_replay_test.go	TestTOTPUsedCodeCache_WasUsed_expiredEntry	Inserts entries using string keys ("user1\x00999999") but TOTPUsedCodeCache.entries uses totpCacheKey{userID, code} struct keys. WasUsed loads with struct key, finds nothing, returns false vacuously. The expiry-check branch is never reached.	Replace c.entries.Store("user1\x00999999", ...) with c.entries.Store(totpCacheKey{userID: "user1", code: "999999"}, ...)

5th run. This test exists specifically to verify expired entries are NOT reported as used. It does not test that. Anyone relying on it has false confidence in the replay-protection system.

---

📈 Pattern Analysis

Recurring Themes

Pattern	Occurrences Today	Total Seen	First Observed
token-expiry-check-inconsistency	1	25	2026-04-17
handlelinkcallback-uses-global-slog	1	14	2026-04-17
oauth2userinfo-name-doc-stale	1	13	2026-04-17
issueTokens-bare-slog	1	12	2026-04-17
cookiename-not-validated-at-startup	1	11	2026-05-01
finishauthentication-closure-side-effects	1	11	2026-05-01
issueTokens-defaultduration-inconsistency	1	7	2026-06-10
magiclinkhandler-tokenttl-method-inconsistency	1	7	2026-06-10
loadwebauthn-credentials-error-dropped	1	7	2026-06-11
password-reset-handler-stale-503-docstring	1	7	2026-06-11
auth-handler-undocumented-exported-fields	1	7	2026-06-11
deleteUserResource-parameter-overload	1	6	2026-06-12
totp-replay-test-wrong-key-type	1	5	2026-06-13
logout-deep-nesting	1	5	2026-06-13
oidc-vs-oauth2-login-redirect-inconsistency	1	2	2026-06-16
newoIDchandler-parameter-overload	1	2	2026-06-16
maintenance-startcleanup-bare-slog	1	1 (new)	2026-06-17

New Pattern

• maintenance-startcleanup-bare-slog: StartCleanup in maintenance/maintenance.go accepts no *slog.Logger and uses slog.ErrorContext directly for both panic recovery and cleaner error reporting. Consuming applications with custom slog handlers (JSON, structured, with trace IDs) will see cleanup failures bypass that configuration entirely. Fix: add an optional logger *slog.Logger parameter.

---

✅ Positive Highlights

• ✅ Consistent logOrDefault() usage across all handler methods — bare slog is confined to helpers and infrastructure, not handlers themselves
• ✅ listUserResources and deleteUserResource generics are used consistently by all three resource handlers — good deduplication
• ✅ No sql.ErrNoRows leaking into handler layer — the sentinel pattern has held since the April fix
• ✅ validateSessionConfig helper enforces RefreshCookieName is non-empty when Sessions is set — consistently called from all Validate() methods

---

💡 Recommendations

For immediate attention (≤5 minutes each):

1. auth/totp_replay_test.go — Replace string keys with totpCacheKey{...} struct keys in tests that bypass MarkUsed. One-line fix per test. Security-adjacent feature.
2. handler/passkey.go L126 — Add , slog.Any("error", err) to the WarnContext call. 22 characters.
3. handler/helpers.go L122-124 — Replace 3-line TTL block with ttl = defaultDuration(ttl, DefaultRefreshTokenTTL).
4. handler/password_reset.go — Remove the 503 text from struct comment and godoc. Delete two sentences.

Longer-term improvements:

1. Thread *slog.Logger into issueTokens(), createLinkNonce(), linkSubjectBestEffort(), handleLinkCallback(), and maintenance.StartCleanup()
2. Add CookieName != "" checks to all five handler Validate() methods
3. Resolve token-expiry-check-inconsistency — 25 reviews. Choose 400 or 401 for expired tokens and apply it everywhere.

---

Daily grumpy review for amalgamated-tools/goauth · Run §27716698194

Generated by Daily Grumpy Reviewer · ● 3.8M · ◷