amadan
fix: carry force=1 so a failed keymail sign-in has a way out
When keymail's callback fails, auth redirects to ?force=1&err=keymail — the force is not decoration, it is an affordance the framework hands the page. Begin reads a non-empty "force" and sends a magic link instead of bouncing back to keymail. The sign-in form did not carry it, so a keymail user whose OAuth failed was redirected to keymail again on every retry, with no exit. Also corrects a false comment in this file. It claimed a second click on an expired link fell through to the generic branch. It does not: auth redirects ?err=expired from all three of its not-found paths, and rastrillo's own auth_test.go asserts it for the second Verify. The claim reached me from Docs, who had inferred it from TakeLink deleting the row without reading the redirect vocabulary at the other end, and who corrected it themselves. The default branch was still worth having — the codes that actually reach it are ?err=1 and, until now, ?err=keymail — but the reasoning written into the file was wrong and is now recorded accurately, including where it came from. Finding the keymail loop is a direct result of checking their correction against the source instead of taking it: the grep that confirmed ?err=expired also showed ?force=1&err=keymail two lines away. guards: the AST check missed dot imports entirely. A dot-imported UserID is a bare identifier, not a selector, so `import . ".../sessions"` walked straight past it — the one case a textual grep would have caught and the AST version did not. Docs found this when they took the guard. All three import styles are now verified by planting a call and reading the message: plain, aliased, and dot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
3 files changed,
+73
−22
internal/guardstest/userid_test.go+24 −9internal/sheets/signin.go+31 −13internal/sheets/signin_test.go+18 −0
CI passed — run details
Intent
diff --git a/internal/guardstest/userid_test.go b/internal/guardstest/userid_test.go| index d9b739a..b00c7ba 100644 |
| --- a/internal/guardstest/userid_test.go |
| +++ b/internal/guardstest/userid_test.go |
| @@ -50,7 +50,10 @@ func TestNoSessionsUserID(t *testing.T) { |
| // Local name for the sessions package in THIS file: it may be |
| // aliased, dot-imported, or absent. No import, no violation |
| // possible. |
| - local := "" |
| + // A dot-import puts UserID in the file scope as a bare |
| + // identifier, so a SelectorExpr check alone would miss it |
| + // entirely. Docs hit exactly that when they took this guard. |
| + local, dotImported := "", false |
| for _, imp := range f.Imports { |
| p, err := strconv.Unquote(imp.Path.Value) |
| if err != nil || p != sessionsPkg { |
| @@ -60,6 +63,9 @@ func TestNoSessionsUserID(t *testing.T) { |
| if imp.Name != nil { |
| local = imp.Name.Name |
| } |
| + if local == "." { |
| + dotImported = true |
| + } |
| } |
| if local == "" { |
| continue |
| @@ -70,20 +76,29 @@ func TestNoSessionsUserID(t *testing.T) { |
| if !ok { |
| return true |
| } |
| - sel, ok := call.Fun.(*ast.SelectorExpr) |
| - if !ok || sel.Sel.Name != forbiddenIdent { |
| + switch fn := call.Fun.(type) { |
| + case *ast.SelectorExpr: |
| + pkg, ok := fn.X.(*ast.Ident) |
| + if !ok || pkg.Name != local || fn.Sel.Name != forbiddenIdent { |
| + return true |
| + } |
| + case *ast.Ident: |
| + if !dotImported || fn.Name != forbiddenIdent { |
| + return true |
| + } |
| + default: |
| return true |
| } |
| - pkg, ok := sel.X.(*ast.Ident) |
| - if !ok || pkg.Name != local { |
| - return true |
| + qualifier, use := local+".", local+".Current(r).Subject" |
| + if dotImported { |
| + qualifier, use = "", "Current(r).Subject" |
| } |
| pos := fset.Position(call.Pos()) |
| - t.Errorf("%s:%d: calls %s.%s, which returns (0, false) for every "+ |
| + 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) |
| + "empty list. Use %s.", |
| + rel, pos.Line, qualifier, forbiddenIdent, use) |
| return true |
| }) |
| } |
diff --git a/internal/sheets/signin.go b/internal/sheets/signin.go| index cb005ed..31aac98 100644 |
| --- a/internal/sheets/signin.go |
| +++ b/internal/sheets/signin.go |
| @@ -10,15 +10,18 @@ import "net/http" |
| // ask for a new link — so an unrecognised code must say that too, not |
| // fall through to an app-wide "something went wrong". |
| // |
| -// That is not hypothetical tidiness. auth's Verify consumes the link row |
| -// with DELETE ... RETURNING, so clicking one expired link twice produces |
| -// two DIFFERENT outcomes: the first finds an expired row (?err=expired), |
| -// the second finds no row at all and reports a generic failure. The |
| -// second is the more common real case — someone clicking an old link out |
| -// of an earlier email — and it is exactly when dropping the "request a |
| -// new one" instruction leaves them with nowhere to go. Docs found this |
| -// on their own sign-in page; it comes from the shape of auth's |
| -// vocabulary, so any app mounting these handlers inherits it. |
| +// The codes that actually reach the default are ?err=1 (an internal |
| +// failure in Begin) and, before it was handled explicitly below, |
| +// ?err=keymail. |
| +// |
| +// An earlier version of this comment claimed a second click on an |
| +// expired link fell through here. It does not: auth redirects |
| +// ?err=expired from all three of its not-found paths, and rastrillo's |
| +// own auth_test.go asserts it for the second Verify. That claim came |
| +// to me from Docs, who had inferred it from TakeLink deleting the row |
| +// without reading the redirect vocabulary at the other end; they |
| +// corrected it, and it is recorded here because the wrong version was |
| +// briefly the justification for this function. |
| func signinMessage(q map[string][]string) string { |
| first := func(k string) string { |
| if v := q[k]; len(v) > 0 { |
| @@ -60,10 +63,20 @@ const ( |
| <main> |
| <h1>Sign in to Oficina Sheets</h1> |
| ` |
| - signinForm = `<form method="post" action="/signin"> |
| + signinFormOpen = `<form method="post" action="/signin"> |
| <label for="address">Email address</label> |
| <input id="address" name="address" type="email" autocomplete="email" required> |
| -<button type="submit">Email me a link</button> |
| +` |
| + // forceField is the affordance auth asks for. When a keymail |
| + // sign-in fails, auth redirects to ?force=1&err=keymail |
| + // specifically so this page can offer the plain-email path: Begin |
| + // reads a non-empty "force" and uses a magic link instead of |
| + // bouncing back to keymail. Without carrying it, a keymail user |
| + // whose OAuth fails is redirected to keymail again on every retry, |
| + // with no way out of the loop. |
| + forceField = `<input type="hidden" name="force" value="1"> |
| +` |
| + signinFormClose = `<button type="submit">Email me a link</button> |
| </form> |
| <p>We email you a link instead of asking for a password. It signs you in |
| once and then stops working.</p> |
| @@ -75,10 +88,15 @@ once and then stops working.</p> |
| // handleSignin serves the sign-in page. |
| func (a *App) handleSignin(w http.ResponseWriter, r *http.Request) { |
| + q := r.URL.Query() |
| w.Header().Set("Content-Type", "text/html; charset=utf-8") |
| w.Write([]byte(signinHead)) |
| - if msg := signinMessage(r.URL.Query()); msg != "" { |
| + if msg := signinMessage(q); msg != "" { |
| w.Write([]byte("<p>" + msg + "</p>\n")) |
| } |
| - w.Write([]byte(signinForm)) |
| + w.Write([]byte(signinFormOpen)) |
| + if q.Get("force") != "" { |
| + w.Write([]byte(forceField)) |
| + } |
| + w.Write([]byte(signinFormClose)) |
| } |
diff --git a/internal/sheets/signin_test.go b/internal/sheets/signin_test.go| index 57bcead..c6dc596 100644 |
| --- a/internal/sheets/signin_test.go |
| +++ b/internal/sheets/signin_test.go |
| @@ -119,3 +119,21 @@ func TestAuthRoutesAbsentWithoutAuth(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// TestSigninCarriesForce: when auth redirects a failed keymail sign-in |
| +// to ?force=1&err=keymail, the page must post force back, or Begin |
| +// bounces the user to keymail again and the loop has no exit. |
| +func TestSigninCarriesForce(t *testing.T) { |
| + body := func(url string) string { |
| + rec := httptest.NewRecorder() |
| + New(nil, nil, nil).Router().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, url, nil)) |
| + return rec.Body.String() |
| + } |
| + with := body("/signin?force=1&err=keymail") |
| + if !strings.Contains(with, `name="force"`) { |
| + t.Error("force=1 page does not carry the force field") |
| + } |
| + if without := body("/signin"); strings.Contains(without, `name="force"`) { |
| + t.Error("ordinary page carries a force field it was not given") |
| + } |
| +} |