amadan
guards: match sessions.UserID by AST, not by text
The grep version banned the string, so it also flagged the rule's own explanation. It needed one exemption when written, and would have needed another for every comment warning a future implementer off the symbol — an exemption list that grows until it covers the violation it exists to catch. Docs hit exactly this shape (four warning comments, zero call sites) and made the same change; the idea is theirs. This walks the AST for call expressions where the receiver resolves to the sessions import in that file. No exemptions, prose is free, and it follows an import alias. Verified against all three cases rather than the one that motivated it: a plain sessions.UserID(r) call fails with file and line; a file that only mentions the symbol in a comment passes, where the grep version failed; and an aliased import (sx "…/sessions") still fails, reporting the local name. A file that does not import sessions is skipped before any walk, so the rule cannot fire on an unrelated UserID method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
1 file changed,
+64
−25
internal/guardstest/userid_test.go+64 −25
Intent
diff --git a/internal/guardstest/userid_test.go b/internal/guardstest/userid_test.go| index a1de666..d9b739a 100644 |
| --- a/internal/guardstest/userid_test.go |
| +++ b/internal/guardstest/userid_test.go |
| @@ -1,8 +1,10 @@ |
| package guards |
| import ( |
| - "os" |
| - "strings" |
| + "go/ast" |
| + "go/parser" |
| + "go/token" |
| + "strconv" |
| "testing" |
| ) |
| @@ -11,41 +13,78 @@ import ( |
| // |
| // 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. |
| +// signed-in email address, so the parse fails on EVERY request — UserID |
| +// can never legitimately succeed here. rastrillo's own magic-links page |
| +// calls this "the most expensive mistake you can make with this plugin". |
| // |
| -// 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. |
| +// 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, so the page |
| +// renders as a plausible empty list rather than failing. Same shape as |
| +// scope.Owned on a team-shared table (AGENTS.md) — both fail as a |
| +// believable answer instead of an error. |
| // |
| // 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" |
| +// This walks the AST for CALL EXPRESSIONS rather than grepping for the |
| +// string. A textual match also hits comments, so the moment this rule is |
| +// explained anywhere in prose the guard needs an exemption — and an |
| +// exemption list is a second thing to maintain that quietly grows until |
| +// it covers the violation it was meant to catch. Docs hit exactly that |
| +// (four warning comments, no call sites) and made the same change. |
| +const ( |
| + sessionsPkg = "github.com/carlosframework/rastrillo/sessions" |
| + forbiddenIdent = "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) |
| + fset := token.NewFileSet() |
| + f, err := parser.ParseFile(fset, path, nil, 0) |
| if err != nil { |
| - t.Fatalf("read %s: %v", rel, err) |
| + t.Fatalf("guards: parse %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) |
| + |
| + // Local name for the sessions package in THIS file: it may be |
| + // aliased, dot-imported, or absent. No import, no violation |
| + // possible. |
| + local := "" |
| + for _, imp := range f.Imports { |
| + p, err := strconv.Unquote(imp.Path.Value) |
| + if err != nil || p != sessionsPkg { |
| + continue |
| + } |
| + local = "sessions" |
| + if imp.Name != nil { |
| + local = imp.Name.Name |
| } |
| } |
| + if local == "" { |
| + continue |
| + } |
| + |
| + ast.Inspect(f, func(n ast.Node) bool { |
| + call, ok := n.(*ast.CallExpr) |
| + if !ok { |
| + return true |
| + } |
| + sel, ok := call.Fun.(*ast.SelectorExpr) |
| + if !ok || sel.Sel.Name != forbiddenIdent { |
| + return true |
| + } |
| + pkg, ok := sel.X.(*ast.Ident) |
| + if !ok || pkg.Name != local { |
| + return true |
| + } |
| + pos := fset.Position(call.Pos()) |
| + t.Errorf("%s:%d: calls %s.%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 %s.Current(r).Subject.", |
| + rel, pos.Line, local, forbiddenIdent, local) |
| + return true |
| + }) |
| } |
| } |