amadan
Answer Astra's review (#21): forced-local return; a router-level panic test
1. Home's forced entry, /signin?force=1&return_to=…, now remembers its validated return like /signin/local does, so sent → Use a different address → second submission → verification lands on the page asked for. TestHomeLocalRetryKeepsTheReturnFromEitherEntry covers both entries; it failed first on the forced one (retry link /signin/local, no return). 2. TestARoutePanicIsTheErrorPageThroughTheRouter panics inside a real route through the production router, signed in and out: the 500 page with a reference and the viewer's chrome, and the panic's value in neither page nor log. Removing r.Use(a.recoverToErrorPage) turns it red (the panic escapes the router); the helper tests stay.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+96
−1
internal/memoria/chrome_test.go+91 −0internal/memoria/handlers.go+5 −1
CI passed — run details
CI log
=== vet === go vet ./... === fmt === === test === go test ./... ? amadan.net/oficina/memoria/cmd/memoria [no test files] ok amadan.net/oficina/memoria/internal/memoria 3.790s ok amadan.net/oficina/memoria/internal/memoria/chunk (cached) ok amadan.net/oficina/memoria/internal/memoria/fact (cached) ? amadan.net/oficina/memoria/internal/memoria/icons [no test files] ok amadan.net/oficina/memoria/internal/memoria/index (cached) ok amadan.net/oficina/memoria/internal/memoria/notion (cached) ok amadan.net/oficina/memoria/internal/memoria/provider (cached) ok amadan.net/oficina/memoria/internal/memoria/query (cached) ok amadan.net/oficina/memoria/internal/memoria/seal (cached) ok amadan.net/oficina/memoria/internal/memoria/sibling (cached) ok amadan.net/oficina/memoria/internal/memoriatest 2.088s ? amadan.net/oficina/memoria/scripts/writeestilo [no test files] ? amadan.net/oficina/memoria/tools/bench [no test files] ? amadan.net/oficina/memoria/tools/drive [no test files] ? amadan.net/oficina/memoria/tools/seed [no test files] ? amadan.net/oficina/memoria/tools/vendorassets [no test files] === migration-check === go run github.com/carlosframework/rastrillo/cmd/rastrillo migration check rastrillo migration check: models and migrations agree
diff --git a/internal/memoria/chrome_test.go b/internal/memoria/chrome_test.go| index 85f6ccd..905a37e 100644 |
| --- a/internal/memoria/chrome_test.go |
| +++ b/internal/memoria/chrome_test.go |
| @@ -6,10 +6,13 @@ import ( |
| "encoding/base64" |
| "encoding/hex" |
| "encoding/json" |
| + "html" |
| "io" |
| "log/slog" |
| "net/http" |
| "net/http/httptest" |
| + "net/url" |
| + "regexp" |
| "strings" |
| "testing" |
| "time" |
| @@ -213,3 +216,91 @@ func homeEnv(t *testing.T) map[string]string { |
| "MEMORIA_HOME_KEYS_BASE64": base64.StdEncoding.EncodeToString(raw), "MEMORIA_HOME_DEK_BASE64": base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{7}, 32)), |
| } |
| } |
| + |
| +// Astra on #21, finding 1: with Home configured, both ways into the local |
| +// form — connect's forced /signin?force=1 and /signin/local — keep an |
| +// explicit return_to through sent → "Use a different address" → a second |
| +// submission → verification. The forced entry used to land on "/". |
| +func TestHomeLocalRetryKeepsTheReturnFromEitherEntry(t *testing.T) { |
| + for _, entry := range []string{"/signin?force=1&return_to=%2Fadd%3Ffrom%3Dmail", "/signin/local?return_to=%2Fadd%3Ffrom%3Dmail"} { |
| + t.Run(entry, func(t *testing.T) { |
| + f := newHomeRouterFixture(t) |
| + if w := f.do("GET", entry, ""); w.Code != http.StatusOK { |
| + t.Fatalf("GET %s = %d", entry, w.Code) |
| + } |
| + sent := homeRedirect(t, f.do("POST", "/auth/begin", "address=alice%40example.test&force=1")) |
| + page := f.do("GET", sent, "").Body.String() |
| + m := regexp.MustCompile(`<a href="([^"]+)">Use a different address</a>`).FindStringSubmatch(page) |
| + if m == nil { |
| + t.Fatalf("no \"Use a different address\" on the sent page:\n%s", page) |
| + } |
| + again := html.UnescapeString(m[1]) |
| + if again != "/signin/local?return_to=%2Fadd%3Ffrom%3Dmail" { |
| + t.Fatalf("\"Use a different address\" goes to %q, want the local form with the return", again) |
| + } |
| + if w := f.do("GET", again, ""); w.Code != http.StatusOK { |
| + t.Fatalf("GET %s = %d", again, w.Code) |
| + } |
| + f.mail.body = "" |
| + homeRedirect(t, f.do("POST", "/auth/begin", "address=alice%40example.test&force=1")) |
| + link := regexp.MustCompile(`https://[^\s<>\"]+/auth/verify\?[^\s<>\"]+`).FindString(f.mail.body) |
| + u, err := url.Parse(link) |
| + if err != nil || link == "" { |
| + t.Fatal("no second link") |
| + } |
| + dest := homeRedirect(t, f.do("GET", u.RequestURI(), "")) |
| + for i := 0; i < 3 && strings.HasPrefix(dest, "/auth/"); i++ { |
| + dest = homeRedirect(t, f.do("GET", dest, "")) |
| + } |
| + if dest != "/add?from=mail" { |
| + t.Errorf("sign-in landed on %q, want /add?from=mail", dest) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// Astra on #21, finding 2: the panic tests above build recoverToErrorPage |
| +// by hand, so removing it from the router left them green. This one panics |
| +// inside a REAL route (the return cookie's clock, read by /signin/local) |
| +// through the production router, signed in and out, and checks the generic |
| +// 500 page with a reference, the viewer's chrome, and that the panic's |
| +// value reaches neither the page nor the log. |
| +func TestARoutePanicIsTheErrorPageThroughTheRouter(t *testing.T) { |
| + const sentinel = "secret-sentinel-passage" |
| + for _, signed := range []bool{true, false} { |
| + f := newHomeRouterFixture(t) |
| + if signed { |
| + f.local(t) |
| + } |
| + var logs bytes.Buffer |
| + f.app.logger = slog.New(slog.NewTextHandler(&logs, nil)) |
| + f.app.clock = func() time.Time { panic(sentinel) } |
| + r := httptest.NewRequest("GET", homeTestOrigin+"/signin/local?return_to=%2Fadd", nil) |
| + r.Header.Set("Accept", "text/html") |
| + for _, c := range f.cookies { |
| + r.AddCookie(c) |
| + } |
| + w := httptest.NewRecorder() |
| + func() { |
| + defer func() { |
| + if v := recover(); v != nil { |
| + t.Fatalf("signed in=%v: the panic escaped the router: %v", signed, v) |
| + } |
| + }() |
| + f.handler.ServeHTTP(w, r) |
| + }() |
| + b := w.Body.String() |
| + if w.Code != http.StatusInternalServerError || !strings.Contains(b, "<title>Something went wrong · Memoria</title>") || !strings.Contains(b, "rst-error-ref") { |
| + t.Errorf("signed in=%v: = %d, want the 500 page with a reference:\n%s", signed, w.Code, b) |
| + } |
| + if strings.Contains(b, "estilo-account") != signed { |
| + t.Errorf("signed in=%v: the 500 page's chrome is not the viewer's", signed) |
| + } |
| + if strings.Contains(b, sentinel) || strings.Contains(logs.String(), sentinel) { |
| + t.Errorf("signed in=%v: the panic's value reached the page or the log", signed) |
| + } |
| + if !strings.Contains(logs.String(), "ref=") { |
| + t.Errorf("signed in=%v: no reference in the log: %s", signed, logs.String()) |
| + } |
| + } |
| +} |
diff --git a/internal/memoria/handlers.go b/internal/memoria/handlers.go| index 4670d5e..d8a7d10 100644 |
| --- a/internal/memoria/handlers.go |
| +++ b/internal/memoria/handlers.go |
| @@ -1212,7 +1212,11 @@ func (a *app) localSignin(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| } |
| - if a.home == nil || r.URL.Path == "/signin/local" { |
| + // Remember an explicit, validated return whenever this form renders — |
| + // including Home's forced /signin?force=1 entry, which connect hands |
| + // here at /signin (Astra on #21, finding 1). The fresh-start reset |
| + // stays where it was: a bare standalone /signin or /signin/local. |
| + if a.home == nil || r.URL.Path == "/signin/local" || hasReturn { |
| switch { |
| case hasReturn: |
| a.rememberReturn(w, ret) |