amadan

oficina / fichas Public

Survive the keymail CSP trap on sign-in

The first real sign-in hung on a spinning button. keymail.dev delegates
to keymail, so auth.Begin answered the form POST with a 303 to keymail's
/oauth/authorize — and the page's CSP says form-action 'self', which
browsers enforce through a form's entire redirect chain, Safari including
intermediate GETs. The navigation was refused in the console and nothing
visible happened.

Ported Docs' fix rather than inventing one. keymailContinuation wraps
Begin, captures its response, and when it is a 303 to a keymail authorize
URL that validates, ends the POST with a document instead: a meta refresh
plus a plain link. A document navigating is not the form's redirect chain,
so the CSP stays exactly as strict as it was. Widening form-action to
keymail's host was the other available fix and the worse one, since it
would let any form on any page post there.

This is the one place fichas turns a URL into a link, so the destination
is validated as exactly the URL keymail's plugin builds for this origin —
https, no userinfo or fragment, the exact path, each parameter once, and
client_id, redirect_uri, scope and the PKCE method pinned. Anything else
is a 502 with no Location. The pending cookie is carried through, because
the callback fails on return without it, and the page is no-store and
no-referrer since the URL carries state and a PKCE challenge.

Tests are Docs', adapted, plus one Docs lacks: every non-keymail outcome
of Begin must pass through untouched, or the wrapper breaks the email path
to fix the keymail one.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev af4991aad3d5d5a52455fa860dcf942205d3d44f parent ae3ef98
5 files changed, +254 −3
  • go.mod +1 −1
  • internal/fichas/app.go +3 −1
  • internal/fichas/auth_navigation.go +120 −0
  • internal/fichas/auth_navigation_test.go +122 −0
  • internal/fichas/templates/signin.html +8 −1

CI passed — run details

CI log 
=== vet ===
go vet ./...
=== fmt ===
=== test ===
go test ./...
?   	amadan.net/oficina/fichas/cmd/fichas	[no test files]
ok  	amadan.net/oficina/fichas/internal/fichas	0.163s
?   	amadan.net/oficina/fichas/internal/fichas/icons	[no test files]
ok  	amadan.net/oficina/fichas/internal/fichas/seal	0.002s
ok  	amadan.net/oficina/fichas/internal/fichastest	0.113s
=== migration-check ===
go run github.com/carlosframework/rastrillo/cmd/rastrillo migration check
rastrillo migration check: models and migrations agree
diff --git a/go.mod b/go.mod
index 23e13e5..74d1b1d 100644
--- a/go.mod
+++ b/go.mod
@@ -8,6 +8,7 @@ require (
github.com/carlosframework/rastrillo v0.23.0
github.com/chromedp/chromedp v0.14.2
github.com/go-chi/chi/v5 v5.3.2
+ github.com/keymaildev/signin v0.1.1
golang.org/x/crypto v0.55.0
gorm.io/gorm v1.31.2
)
@@ -24,7 +25,6 @@ require (
github.com/google/uuid v1.6.0 // indirect
github.com/jinzhu/inflection v1.0.0 // indirect
github.com/jinzhu/now v1.1.5 // indirect
- github.com/keymaildev/signin v0.1.1 // indirect
github.com/mattn/go-isatty v0.0.20 // indirect
github.com/ncruces/go-strftime v1.0.0 // indirect
github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
diff --git a/internal/fichas/app.go b/internal/fichas/app.go
index d66e8ce..27e762a 100644
--- a/internal/fichas/app.go
+++ b/internal/fichas/app.go
@@ -65,7 +65,9 @@ func App(d *db.DB, cfg Config, logger *slog.Logger) (*http.ServeMux, *app, error
// session is obtained. beginGuard runs before rastrillo's classifier
// can dial a domain the visitor chose.
r.Get(signinPath, a.signin)
- r.Post("/auth/begin", a.beginGuard(a.authBegin))
+ // Order matters: admission before the classifier can dial, then the
+ // continuation around Begin so a keymail redirect survives the CSP.
+ r.Post("/auth/begin", a.beginGuard(a.keymailContinuation(a.authBegin)))
r.Get("/auth/callback", a.authCallback)
r.Get("/auth/verify", a.authVerify)
r.Post("/signout", a.authSignout)
diff --git a/internal/fichas/auth_navigation.go b/internal/fichas/auth_navigation.go
new file mode 100644
index 0000000..8f594a4
--- /dev/null
+++ b/internal/fichas/auth_navigation.go
@@ -0,0 +1,120 @@
+package fichas
+
+import (
+ "bytes"
+ "net/http"
+ "net/url"
+ "strings"
+)
+
+// keymailContinuation ends the sign-in POST with a page rather than a
+// redirect when rastrillo sends the browser on to keymail.
+//
+// **The keymail CSP trap.** When the submitted address's domain delegates
+// to keymail, auth.Begin answers the form POST with a 303 to keymail's
+// /oauth/authorize. The page's CSP says `form-action 'self'`, and browsers
+// enforce form-action through a form's whole redirect chain — Safari
+// including intermediate GETs — so the navigation is refused and the button
+// spins forever. The first sign-in to the deployed app hit exactly this.
+//
+// The fix is Docs' (internal/docs/auth_navigation.go), ported rather than
+// reinvented: capture Begin's response, and if it is a 303 to a keymail
+// authorize URL that validates, end the POST with a document carrying a
+// meta refresh and a plain link. A document navigating is not the form's
+// redirect chain, so the CSP stays exactly as strict as it was — widening
+// form-action to keymail's host would have been the other fix, and a worse
+// one, because it would let any form on any page post there.
+//
+// Works with scripts off: the meta refresh needs none, and the link is the
+// fallback if a browser ignores the refresh.
+func (a *app) keymailContinuation(next http.HandlerFunc) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ result := &capturedResponse{header: make(http.Header)}
+ next(result, r)
+
+ destination := result.header.Get("Location")
+ external := destination != "" && !strings.HasPrefix(destination, "/")
+ if result.status == http.StatusSeeOther && external {
+ // Refuse anything that is not exactly the URL keymail's
+ // plugin builds. The destination comes from rastrillo's
+ // server-side redirect and never from a submitted value,
+ // but this is the one place fichas turns a URL into a link,
+ // so it is also the one place an open redirect could be born.
+ if !keymailDestination(destination, a.cfg.Origin) {
+ a.logger.Error("sign-in redirected somewhere that is not keymail's authorize endpoint")
+ http.Error(w, "sign-in could not continue", http.StatusBadGateway)
+ return
+ }
+ // The pending cookie rastrillo set is what the callback
+ // checks; drop it and the round trip fails on return.
+ for _, c := range result.header.Values("Set-Cookie") {
+ w.Header().Add("Set-Cookie", c)
+ }
+ w.Header().Set("Cache-Control", "no-store")
+ // The URL carries state and a PKCE challenge. It must not
+ // leak onward as a Referer.
+ w.Header().Set("Referrer-Policy", "no-referrer")
+ render(w, "signin", map[string]any{"ContinueURL": destination})
+ return
+ }
+
+ for name, values := range result.header {
+ for _, v := range values {
+ w.Header().Add(name, v)
+ }
+ }
+ if result.status == 0 {
+ result.status = http.StatusOK
+ }
+ w.WriteHeader(result.status)
+ _, _ = w.Write(result.body.Bytes())
+ }
+}
+
+// keymailDestination accepts only the authorize URL keymail's plugin
+// builds for THIS origin: https, no userinfo, no fragment, the exact path,
+// each required parameter present exactly once, and client_id,
+// redirect_uri, scope and the PKCE method pinned to what fichas asked for.
+// Docs' predicate, unchanged.
+func keymailDestination(destination, origin string) bool {
+ u, err := url.Parse(destination)
+ if err != nil || u.Scheme != "https" || u.Host == "" || u.User != nil ||
+ u.Fragment != "" || u.Path != "/oauth/authorize" || u.RawPath != "" {
+ return false
+ }
+ q, err := url.ParseQuery(u.RawQuery)
+ if err != nil {
+ return false
+ }
+ for _, key := range []string{"client_id", "redirect_uri", "scope", "code_challenge_method", "state", "code_challenge"} {
+ if len(q[key]) != 1 || q.Get(key) == "" {
+ return false
+ }
+ }
+ origin = strings.TrimSuffix(origin, "/")
+ return q.Get("client_id") == origin &&
+ q.Get("redirect_uri") == origin+"/auth/callback" &&
+ q.Get("scope") == "identify" &&
+ q.Get("code_challenge_method") == "S256"
+}
+
+// capturedResponse records what a handler wrote so the wrapper can decide
+// whether to pass it through or replace it.
+type capturedResponse struct {
+ header http.Header
+ status int
+ body bytes.Buffer
+}
+
+func (c *capturedResponse) Header() http.Header { return c.header }
+func (c *capturedResponse) WriteHeader(status int) {
+ if c.status == 0 {
+ c.status = status
+ }
+}
+func (c *capturedResponse) Write(p []byte) (int, error) {
+ if c.status == 0 {
+ c.WriteHeader(http.StatusOK)
+ }
+ return c.body.Write(p)
+}
diff --git a/internal/fichas/auth_navigation_test.go b/internal/fichas/auth_navigation_test.go
new file mode 100644
index 0000000..8534056
--- /dev/null
+++ b/internal/fichas/auth_navigation_test.go
@@ -0,0 +1,122 @@
+package fichas
+
+import (
+ "html"
+ "io"
+ "log/slog"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+
+ "github.com/keymaildev/signin"
+)
+
+const navOrigin = "https://fichas.example.test"
+
+func navApp() *app {
+ return &app{
+ cfg: Config{Origin: navOrigin},
+ logger: slog.New(slog.NewTextHandler(io.Discard, nil)),
+ }
+}
+
+// The trap itself: auth.Begin answers the sign-in POST with a 303 to
+// keymail, the page's CSP says form-action 'self', and the browser refuses
+// the navigation. The continuation must end the POST with a page instead,
+// leave the CSP untouched, and carry rastrillo's pending cookie through —
+// the callback fails on return without it.
+func TestKeymailContinuation(t *testing.T) {
+ destination := (&signin.Keymail{Base: "https://keymail.example.test", Origin: navOrigin}).
+ AuthorizeURL("state", "challenge", true)
+ handler := navApp().keymailContinuation(func(w http.ResponseWriter, r *http.Request) {
+ http.SetCookie(w, &http.Cookie{Name: "__Host-pending", Value: "opaque", Path: "/",
+ Secure: true, HttpOnly: true, SameSite: http.SameSiteLaxMode})
+ http.Redirect(w, r, destination, http.StatusSeeOther)
+ })
+
+ rec := httptest.NewRecorder()
+ policy := "default-src 'self'; form-action 'self'"
+ rec.Header().Set("Content-Security-Policy", policy)
+ handler(rec, httptest.NewRequest(http.MethodPost, "/auth/begin", nil))
+
+ if rec.Code != http.StatusOK || rec.Header().Get("Location") != "" {
+ t.Fatalf("status=%d location=%q; the POST must end with a page, not a redirect",
+ rec.Code, rec.Header().Get("Location"))
+ }
+ if got := rec.Header().Get("Content-Security-Policy"); got != policy {
+ t.Fatalf("CSP changed to %q; widening form-action is the fix this avoids", got)
+ }
+ if rec.Header().Get("Cache-Control") != "no-store" || rec.Header().Get("Referrer-Policy") != "no-referrer" {
+ t.Fatal("the continuation carries state and a PKCE challenge: it must not cache or leak as a Referer")
+ }
+
+ cookies := rec.Result().Cookies()
+ if len(cookies) != 1 || cookies[0].Name != "__Host-pending" || cookies[0].Value != "opaque" ||
+ !cookies[0].Secure || !cookies[0].HttpOnly || cookies[0].SameSite != http.SameSiteLaxMode {
+ t.Fatalf("lost the pending cookie: %v", cookies)
+ }
+
+ body := html.UnescapeString(rec.Body.String())
+ for _, part := range []string{
+ `http-equiv="refresh"`,
+ `content="0;url=` + destination + `"`,
+ `href="` + destination + `"`,
+ } {
+ if !strings.Contains(body, part) {
+ t.Fatalf("the continuation is missing %q", part)
+ }
+ }
+ if strings.Contains(body, `action="/auth/begin"`) {
+ t.Fatal("the continuation rendered another sign-in form")
+ }
+}
+
+// This is the one place fichas turns a URL into a link, so it is the one
+// place an open redirect could be born. Anything that is not exactly
+// keymail's authorize URL for this origin is refused.
+func TestKeymailDestinationRefusals(t *testing.T) {
+ valid := (&signin.Keymail{Base: "https://keymail.example.test", Origin: navOrigin}).
+ AuthorizeURL("state", "challenge", false)
+ if !keymailDestination(valid, navOrigin) {
+ t.Fatalf("refused the URL keymail's own plugin builds: %s", valid)
+ }
+ for _, target := range []string{
+ "javascript:alert(1)",
+ "//keymail.example.test/oauth/authorize",
+ strings.Replace(valid, "https:", "http:", 1),
+ strings.Replace(valid, "keymail.example.test", "user@keymail.example.test", 1),
+ valid + "#fragment",
+ valid + "&state=duplicate",
+ strings.Replace(valid, "/oauth/authorize", "/other", 1),
+ strings.ReplaceAll(valid, url.QueryEscape(navOrigin), url.QueryEscape("https://foreign.example.test")),
+ strings.Replace(valid, "S256", "plain", 1),
+ } {
+ if keymailDestination(target, navOrigin) {
+ t.Errorf("accepted %q", target)
+ }
+ }
+
+ rec := httptest.NewRecorder()
+ navApp().keymailContinuation(func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, "https://foreign.example.test", http.StatusSeeOther)
+ })(rec, httptest.NewRequest(http.MethodPost, "/auth/begin", nil))
+ if rec.Code != http.StatusBadGateway || rec.Header().Get("Location") != "" {
+ t.Fatalf("an off-keymail redirect: status=%d location=%q, want 502 and no Location",
+ rec.Code, rec.Header().Get("Location"))
+ }
+}
+
+// Every other outcome of Begin — the ?sent=1 redirect, an error page —
+// must pass through untouched, or the wrapper breaks the email path to fix
+// the keymail one.
+func TestNonKeymailOutcomesPassThrough(t *testing.T) {
+ rec := httptest.NewRecorder()
+ navApp().keymailContinuation(func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, "/signin?sent=1", http.StatusSeeOther)
+ })(rec, httptest.NewRequest(http.MethodPost, "/auth/begin", nil))
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("a local redirect: status=%d location=%q", rec.Code, rec.Header().Get("Location"))
+ }
+}
diff --git a/internal/fichas/templates/signin.html b/internal/fichas/templates/signin.html
index cbc315e..70946db 100644
--- a/internal/fichas/templates/signin.html
+++ b/internal/fichas/templates/signin.html
@@ -1,7 +1,14 @@
{{define "content"}}
{{template "page-header" (dict "Title" "Sign in to fichas")}}
-{{if .Sent}}
+{{if .ContinueURL}}
+ {{/* The keymail continuation. A document navigating is not the sign-in
+ form's redirect chain, so form-action 'self' stays as strict as it
+ was. The meta refresh needs no script; the link is the fallback. */}}
+ <meta http-equiv="refresh" content="0;url={{.ContinueURL}}">
+ <p>Continuing to keymail to confirm it's you.</p>
+ <p><a rst-btn="primary" href="{{.ContinueURL}}">Continue to keymail</a></p>
+{{else if .Sent}}
{{template "callout" (dict
"Tone" "positive"
"Title" "Check your email"