amadan
guards: ban sessions.UserID, which cannot succeed in this app
rastrillo/auth sets a session's Subject to the signed-in email address. sessions.UserID parses Subject as an integer and returns (0, false) when that fails — so in this app it fails on every request, always. That would be harmless if it errored. It returns 0, and the idiom the signature invites is `uid, _ := sessions.UserID(r)`. Dropping the ok scopes every subsequent query to user 0: no rows match, and the page renders as a plausible empty list rather than failing. Same shape as scope.Owned on a team-shared table, which AGENTS.md already bans for the same reason — both fail as a believable answer instead of an error. Docs hit this on their own sign-in work and suggested having a reviewer grep the whole app for it. A reviewer forgets between now and the third person to touch this; a test does not. The guard passed vacuously when written, since no code here uses sessions yet — which is the failure mode this branch keeps finding, so it was not kept on that basis. Verified by planting `uid, _ := sessions.UserID(r)` in internal/sheets and watching it fail with the file and line, then removing it and watching it pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
1 file changed,
+51
−0
internal/guardstest/userid_test.go+51 −0
Intent
diff --git a/internal/guardstest/userid_test.go b/internal/guardstest/userid_test.go| new file mode 100644 |
| index 0000000..a1de666 |
| --- /dev/null |
| +++ b/internal/guardstest/userid_test.go |
| @@ -0,0 +1,51 @@ |
| +package guards |
| + |
| +import ( |
| + "os" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// sessions.UserID is unusable in this app, and unusable in a way that |
| +// does not announce itself. |
| +// |
| +// It parses the current session's Subject as an integer and returns |
| +// (0, false) when that fails. rastrillo/auth sets Subject to the |
| +// signed-in email address (auth.go, Identity.Address), so the parse |
| +// fails on EVERY request — UserID can never legitimately succeed here. |
| +// |
| +// That would be harmless if it errored. It returns 0. A caller that |
| +// drops the ok, as `uid, _ := sessions.UserID(r)` invites, scopes every |
| +// subsequent query to user 0: no rows match, so the page renders as a |
| +// plausible empty list rather than failing. It is the same shape as |
| +// scope.Owned on a team-shared table (AGENTS.md), and the reason both |
| +// are banned outright rather than reviewed case by case. |
| +// |
| +// Use sessions.Current(r) and its Subject (the address) instead. |
| +// |
| +// A reviewer was suggested for this. A reviewer forgets; this does not. |
| +const forbiddenUserID = "sessions.UserID" |
| + |
| +func TestNoSessionsUserID(t *testing.T) { |
| + root := repoRoot(t) |
| + for _, path := range goFiles(t, root) { |
| + rel := relPath(t, root, path) |
| + // This file names the symbol in order to forbid it. |
| + if rel == "internal/guardstest/userid_test.go" { |
| + continue |
| + } |
| + src, err := os.ReadFile(path) |
| + if err != nil { |
| + t.Fatalf("read %s: %v", rel, err) |
| + } |
| + for i, line := range strings.Split(string(src), "\n") { |
| + if strings.Contains(line, forbiddenUserID) { |
| + t.Errorf("%s:%d: uses %s, which returns (0, false) for every "+ |
| + "request in this app because auth's Subject is an email "+ |
| + "address — a dropped ok scopes the query to user 0 and "+ |
| + "renders as an empty list. Use sessions.Current(r).Subject.", |
| + rel, i+1, forbiddenUserID) |
| + } |
| + } |
| + } |
| +} |