amadan

oficina / docs Public

Restore detected Keymail OAuth with a CSP-safe continuation

Paul Campbell pushed by paul@keymail.dev 3fffd1be535ec87ab6e92da753046a4f1e4c754c parent c703a41
10 files changed, +524 −214
  • AGENTS.md +16 −9
  • internal/docs/admission_test.go +13 −41
  • internal/docs/app.go +4 −32
  • internal/docs/auth.go +29 −113
  • internal/docs/auth_allowlist_test.go +2 −4
  • internal/docs/auth_keymail_test.go +257 −0
  • internal/docs/auth_navigation.go +92 −0
  • internal/docs/auth_navigation_test.go +100 −0
  • internal/docs/render.go +3 −2
  • internal/docs/views/signin.html +8 −13

CI passed — run details

diff --git a/AGENTS.md b/AGENTS.md
index 2ce54d8..2fd2be7 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -24,8 +24,8 @@ nothing about either is in your training data.
Every document route — `GET /`, `POST /documents`, `GET /d/{sqid}` — sits
behind rastrillo/auth's `RequireSession` and is unreachable without a real
-session (`internal/docs/auth.go`). Sign-in is a magic link: `POST /signin`
-emails (or, in development, logs) a single-use link; following it
+session (`internal/docs/auth.go`). `POST /signin` uses Keymail OAuth when
+detected. Otherwise it emails (or, in development, logs) a single-use link; following it
(`GET /auth/verify`) sets a real session cookie; `POST /signout` (in the shell
bar's account menu on both signed-in screens) really revokes it,
not just clearing the cookie client-side. No passkey second factor is
@@ -77,13 +77,20 @@ the CARLOS Sheets session on their own deployed instance; every existing
test passed because they all tested admission, none tested delivery.
The fix is `$DOCS_ALLOWED_ADDRESSES` (`docs.Config.AllowedAddresses`,
-`internal/docs/auth.go`): a comma-separated, case/whitespace-insensitive
-allowlist, wired to *two* places — `rauth.Config.Authorize` (admission,
-belt-and-suspenders) and `gateBegin`, a wrapper mounted in front of
-`rauth.Auth.Begin` on `POST /signin` itself, which is the part that
-actually matters, since it is what stops the mail. A non-permitted
-address gets exactly the same `303 ?sent=1` response a permitted one
-gets — anything else would turn the endpoint into a membership oracle.
+`internal/docs/auth.go`), a normalized allowlist checked by both
+`rauth.Config.Authorize` and `admissionMailer`. The mailer suppresses delivery
+for refused addresses and failed grant lookups. Classification and rate limits
+run independently of admission, so an OAuth start does not disclose membership.
+Refused email starts still receive `303 ?sent=1`; their expiring tokens are never
+delivered. Callback authorization and per-request admission remain required.
+
+Detected Keymail addresses use OAuth, as §8.1 requires and Paul reaffirmed on
+2026-09-09. `force=1` remains an explicit email fallback. The OAuth start ends
+with a `200` continuation page (meta refresh plus link), preserving the pending
+cookie and the existing CSP: Safari enforces `form-action` across redirects.
+Never widen that directive to arbitrary Keymail hosts. Account linking still
+requires a fresh provider `AuthTime`; an ordinary OAuth session is not enough.
+
Empty is allowed only in development (admits everyone, so `TestSignIn`
and `run-dev.sh` need no setup); anywhere else it is a startup refusal,
the same fail-closed shape `Origin`/`InstanceKey`/`Mailer` already use,
diff --git a/internal/docs/admission_test.go b/internal/docs/admission_test.go
index 2c11100..0620ac5 100644
--- a/internal/docs/admission_test.go
+++ b/internal/docs/admission_test.go
@@ -5,8 +5,6 @@ import (
"errors"
"net/http"
"net/http/httptest"
- "net/url"
- "strings"
"testing"
)
@@ -147,47 +145,21 @@ func TestAdmitsFailsClosed(t *testing.T) {
}
}
-// gateBegin fails closed too, and nothing pinned it.
-//
-// A mutation flipping its condition to `err != nil || admitted` passed the
-// whole suite. Under it, a database stall turns POST /signin into an open
-// relay: every address a stranger types reaches Begin and gets a magic
-// link mailed. Config.AllowedAddresses' own comment calls gateBegin "the
-// wiring that actually matters" for exactly that, and admits() would still
-// refuse the session at callback — but the mail is already out, sent by us,
-// to an address we chose to accept. (Re-review 4, finding 1.)
-func TestGateBeginFailsClosed(t *testing.T) {
+// Delivery fails closed even when the grant store is unavailable.
+func TestAdmissionMailerFailsClosed(t *testing.T) {
list := allowlist{
- addrs: map[string]struct{}{"alice@example.com": {}},
- granted: func(string) (bool, error) {
- return false, errors.New("database is stalled")
- },
+ addrs: map[string]struct{}{"alice@example.com": {}},
+ granted: func(string) (bool, error) { return false, errors.New("database is stalled") },
}
-
- for _, c := range []struct {
- name string
- address string
- want bool
- }{
- {"a stranger during a failed check is refused", "mallory@example.com", false},
- {"a member is unaffected, the check never runs for them", "alice@example.com", true},
- } {
- t.Run(c.name, func(t *testing.T) {
- reached := false
- h := gateBegin(func(w http.ResponseWriter, r *http.Request) {
- reached = true
- }, list, "/signin")
-
- req := httptest.NewRequest(http.MethodPost, "/signin",
- strings.NewReader(url.Values{"address": {c.address}}.Encode()))
- req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
- h(httptest.NewRecorder(), req)
-
- if reached != c.want {
- t.Errorf("reached Begin = %v, want %v — a mail %s sent", reached, c.want,
- map[bool]string{true: "was not", false: "was"}[reached])
- }
- })
+ recorder := &recordingMailer{next: loggingMailerStub{}}
+ sender := admissionMailer{next: recorder, list: list}
+ for _, address := range []string{"mallory@example.com", "alice@example.com"} {
+ if err := sender.Send(t.Context(), address, "subject", "body"); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if recorder.sends != 1 || recorder.lastTo != "alice@example.com" {
+ t.Fatalf("delivery count=%d recipient=%q", recorder.sends, recorder.lastTo)
}
}
diff --git a/internal/docs/app.go b/internal/docs/app.go
index 26378fe..1e2e5aa 100644
--- a/internal/docs/app.go
+++ b/internal/docs/app.go
@@ -144,20 +144,9 @@ type Config struct {
// normalizeAddress) — a real config file has spaces after its
// commas, and people type capitals into an email field.
//
- // This is wired to TWO places, deliberately, not one:
- // rauth.Config.Authorize (newAuthPlugin, auth.go) gates admission,
- // and gateBegin (auth.go) gates POST /signin itself, ahead of
- // rauth.Auth.Begin. The second wiring is the one that actually
- // matters: rastrillo/auth's Authorize hook fires only in its own
- // admit() (rastrillo/auth's handlers.go), which runs after Begin
- // has already mailed the link, so Authorize alone defends the
- // session but not the mail — an Authorize-only fix would leave
- // this app's own SES sending identity an open relay for any
- // address a stranger cares to type into the sign-in form (the
- // defect Task 4 exists to close; credited to the CARLOS Sheets
- // session's finding on their own deployed instance). gateBegin's
- // short-circuit is what actually stops that: a non-permitted
- // address never reaches Begin, so no mail is ever sent for it.
+ // admissionMailer restricts email delivery, and rauth.Config.Authorize
+ // checks verified identities before minting a session. Classification
+ // does not depend on admission, so an OAuth start cannot reveal membership.
//
// Empty is allowed only when Env is "development"
// (resolveAllowlist below defaults that to "admit everyone", so
@@ -462,13 +451,6 @@ func New(cfg Config) (*App, error) {
return nil, fmt.Errorf("docs: build renderer: %w", err)
}
- // auth.go: rastrillo's magic-link plugin, mounted per its own
- // SKILL.md contract (POST /signin, GET /auth/callback, GET
- // /auth/verify, POST /signout) — never a passkey second factor;
- // see spec §8.1 and auth.go's own doc comment for why. list also
- // becomes rauth.Config.Authorize here (admission), and — separately,
- // below — gateBegin's argument (delivery); see AllowedAddresses'
- // own doc comment on Config for why both wirings exist.
authPlugin, err := newAuthPlugin(cfg, d.Writer(), instanceKey, mailer, list, logger)
if err != nil {
rt.Close(context.Background())
@@ -514,17 +496,7 @@ func New(cfg Config) (*App, error) {
} else {
home.mountPublic(r)
}
- // gateBegin, not authPlugin.Begin directly: see AllowedAddresses'
- // own doc comment on Config, and gateBegin's own doc comment
- // (auth.go), for why the allowlist has to sit in front of Begin
- // itself rather than only behind it as rauth.Config.Authorize.
- // "/signin" here must keep matching rauth's own default
- // SigninPath (auth.New's own zero-value default) — newAuthPlugin
- // never overrides Config.SigninPath, and nothing else in this file
- // does either, so the two literals staying equal is not a
- // coincidence needing its own sync mechanism, only something a
- // future SigninPath override would have to touch in both places.
- r.Post("/signin", gateBegin(authPlugin.Begin, list, "/signin"))
+ r.Post("/signin", h.beginSignin(authPlugin.Begin))
r.Get("/auth/callback", localAuthGuard(home, authPlugin.Callback))
r.Get("/auth/verify", localAuthGuard(home, authPlugin.Verify))
// The published document, and the only route here no session guards.
diff --git a/internal/docs/auth.go b/internal/docs/auth.go
index a9bf757..ae36017 100644
--- a/internal/docs/auth.go
+++ b/internal/docs/auth.go
@@ -1,20 +1,5 @@
-// Per spec §8.1 (reframed the day this task started): a single
-// instance signs people in itself, permanently, not as a stopgap for
-// a shared home that does not exist yet — a lone instance has no
-// "one person, many teams" problem, which is the problem a shared
-// home would exist to solve. It does so with rastrillo/auth's
-// turnkey magic-link plugin (SKILL.md §5, docs/site/magic-links.md
-// in the rastrillo module): an emailed, single-use link, no password,
-// no credential enrolled.
-//
-// Deliberately magic link only, never a passkey second factor:
-// passkey.Config.Origin derives its relying party ID from this
-// instance's own host, so a person signed into three team instances
-// would enrol three separate passkeys under this app — exactly the
-// per-team credential sprawl §8.1's ruling forbids. A magic link
-// enrols no credential at all, so it creates no relying party and
-// violates nothing. Task 1's brief is explicit that this is a design
-// ruling, not an oversight to "improve" on later.
+// Instance-local sign-in uses Keymail OAuth when detected and email otherwise.
+// Credentials remain at Keymail; Docs never enrolls a per-instance passkey.
package docs
import (
@@ -111,7 +96,7 @@ type recordingMailer struct {
mu sync.Mutex
next mail.Sender
// lastTo is recorded alongside lastBody, and it is not decoration.
- // gateBegin answers a refused address with exactly the 303 ?sent=1 a
+ // admissionMailer leaves a refused address with exactly the 303 ?sent=1 a
// real one gets — deliberately, so the endpoint is not a membership
// oracle — so a caller that reads only lastBody cannot tell "a link
// was mailed to this address" from "somebody else's link is still
@@ -170,7 +155,7 @@ type allowlist struct {
// the allowlist alone, and any construction before the store exists).
//
// It exists because $DOCS_ALLOWED_ADDRESSES gates who may receive a
- // magic link at all, and gateBegin deliberately answers identically for
+ // magic link at all, and suppressed delivery answers identically for
// a refused address so this endpoint is not a membership oracle.
// Without this clause, sharing a document to somebody outside the list
// produces a grant that can never be redeemed: the sharer sees success,
@@ -256,9 +241,8 @@ func (a allowlist) admits(address string) bool {
// Refusal, not "admit nobody" — a real choice, made deliberately, not
// the shape everything else in this file happens to use. An instance
// that silently admitted nobody would boot looking entirely healthy:
-// /signin renders, POST /signin answers its ordinary 303 ?sent=1 on
-// every submission (gateBegin's whole point is that a refused
-// submission and a genuine one are byte-identical), and nothing would
+// /signin renders and email submissions answer 303 ?sent=1 whether
+// delivery was permitted or suppressed. Nothing would
// ever surface the mistake short of a person noticing that no sign-in
// link ever arrives — on an app whose entire premise is one
// instance, one team, where "nobody configured" is exactly the state
@@ -290,87 +274,23 @@ func resolveAllowlist(cfg Config) (allowlist, error) {
return allowlist{}, fmt.Errorf("docs: $DOCS_ALLOWED_ADDRESSES is empty and Env is %q, not %q — refusing to start with no allowlist configured: an instance-per-team app admitting nobody would look identical to one working correctly (see resolveAllowlist's own doc comment), so this refuses rather than boots quietly wrong; set $DOCS_ALLOWED_ADDRESSES to this team's address(es)", cfg.Env, "development")
}
-// gateBegin wraps rauth.Auth.Begin so a non-permitted address never
-// reaches it at all — the actual fix for the defect Config.
-// AllowedAddresses' own doc comment describes: Begin is what sends
-// the mail, and rauth.Config.Authorize (wired in newAuthPlugin below,
-// too) is checked only afterward, inside rauth's own admit(), which
-// runs at the callback once a link has already been mailed and
-// clicked. Gating only Authorize would leave this app's own mail
-// sending identity an open relay for any address a stranger types
-// into the sign-in form; gating here means a non-permitted address
-// causes rauth's Begin to never run, so no mail is ever sent for it.
-//
-// It reads the submitted address with r.FormValue exactly the way
-// rauth's own Begin does (rastrillo/auth's handlers.go), so by the
-// time next (Begin, for a permitted address) reads it again,
-// net/http's request.ParseForm has already cached the parse —
-// ParseForm is documented idempotent, so parsing twice costs nothing
-// and cannot disagree with itself.
-//
-// A non-permitted address gets exactly the response Begin's own
-// success path sends: the same 303, to the same signinPath+"?sent=1",
-// built the same way (http.Redirect). That is deliberate, not merely
-// convenient: any observable difference — a different status, a
-// different body, even a differently-cased query string — would turn
-// this endpoint into a membership oracle, letting an attacker learn
-// which addresses this instance's allowlist contains by watching how
-// its response to each one differs. See
-// TestSigninResponseIdenticalForNonPermittedAddress (auth_test.go),
-// which asserts the two responses byte-for-byte, not merely "both are
-// 303".
-//
-// That guarantee only held for a non-keymail address: on the
-// permitted branch, this used to hand the request to Begin exactly as
-// received, and Begin auto-upgrades a permitted address that resolves
-// to a claimed keymail inbox to the keymail ceremony — a 303 to the
-// keymail authorize URL, not signinPath+"?sent=1". A non-permitted
-// address never reaches Begin at all, so it always got ?sent=1. That
-// reopened the same oracle one level narrower: an attacker who knows
-// (or suspects) an address is keymail-hosted could tell "on the
-// allowlist" from "not" by whether the response redirected to keymail
-// or to ?sent=1.
-//
-// force=1 is what selects the plain-magic-link path over that
-// classification (rastrillo/auth's handlers.go), and this app never
-// wants the keymail ceremony in the first place — spec §8.1's ruling,
-// this file's own package comment, and signin.html's form, which
-// already hardcodes a hidden force=1 field so no request built by
-// this app's own UI ever takes the keymail branch. Setting it here
-// too, unconditionally, on every permitted request — regardless of
-// what the caller's own form said — makes that the actual guarantee
-// for any request this handler ever forwards, not just the ones sent
-// through this app's form: every permitted address takes the identical
-// plain-magic-link path Begin would take for force=1, so the keymail
-// branch is never reached and never has a distinct response to leak
-// through. This costs nothing a legitimate caller wanted: it matches
-// what the UI already always sends, and it is not a behavior change
-// this app would ever notice.
-func gateBegin(next http.HandlerFunc, list allowlist, signinPath string) http.HandlerFunc {
- return func(w http.ResponseWriter, r *http.Request) {
- // A malformed body (ParseForm's error) reads back as address
- // == "" below, which is never on a non-empty allowlist and is
- // on an admit-everyone one only in development — the same
- // answer Begin itself would eventually reach for the same
- // malformed request, just reached here instead, before any
- // mail could be sent over it.
- _ = r.ParseForm()
- // A failed check refuses here, unlike requireAdmission's 503. At
- // sign-in there is nothing to lose by refusing — no unsaved work,
- // and the person can try again — and gateBegin's whole contract is
- // that a refusal is indistinguishable from a success, which a 503
- // would break by telling a stranger the check happened.
- admitted, err := list.allows(r.FormValue("address"))
- if err == nil && admitted {
- if r.Form == nil {
- r.Form = url.Values{}
- }
- r.Form.Set("force", "1")
- next(w, r)
- return
- }
- http.Redirect(w, r, signinPath+"?sent=1", http.StatusSeeOther)
+// admissionMailer restricts delivery without exposing membership through the
+// choice of sign-in ceremony. All addresses reach classification and the same
+// rate limits; only admitted addresses receive mail. Callback admission remains
+// authoritative, including for tokens minted before an address was removed.
+// The underlying flow stores an expiring token before Send; a suppressed token
+// is never delivered. A failed grant lookup suppresses delivery too.
+type admissionMailer struct {
+ next mail.Sender
+ list allowlist
+}
+
+func (m admissionMailer) Send(ctx context.Context, to, subject, body string) error {
+ admitted, err := m.list.allows(to)
+ if err != nil || !admitted {
+ return nil
}
+ return m.next.Send(ctx, to, subject, body)
}
// requireAdmission re-asks the front-door question on every request, and
@@ -481,16 +401,9 @@ func requireAdmissionOf(list allowlist, signinPath string, identity func(*http.R
// decision, InstanceKey and Mailer already resolved and fail-closed
// by resolveInstanceKey/buildMailer above. Authorize is list.admits —
// the fail-closed predicate form, because rauth's hook has no way to
-// say "could not tell" and refusing is the safe answer here —
-// belt-and-suspenders alongside gateBegin's own gate on POST /signin:
-// this defends admission itself (a link somehow minted for a
-// non-permitted address — a future code path, or a link mailed before
-// the allowlist changed — still cannot mint a session), even though
-// gateBegin's own gate is what actually stops the mail from going out
-// in the first place; see Config.AllowedAddresses' own doc comment
-// for why both wirings exist rather than either alone. SecondFactor
-// is left nil on purpose: no passkey step-up, ever, for the reason
-// this file's own package comment gives.
+// say "could not tell" and refusing is the safe answer here.
+// Delivery and callback admission are checked independently. No local passkey
+// second factor is enrolled; Keymail verifies credentials at its own origin.
func newAuthPlugin(cfg Config, writer *sql.DB, instanceKey string, mailer mail.Sender, list allowlist, logger *slog.Logger) (*rauth.Auth, error) {
signedInPath := "/"
if cfg.Home != nil {
@@ -501,7 +414,7 @@ func newAuthPlugin(cfg Config, writer *sql.DB, instanceKey string, mailer mail.S
DB: writer,
Origin: cfg.Origin,
InstanceKey: instanceKey,
- Mailer: mailer,
+ Mailer: admissionMailer{next: mailer, list: list},
Authorize: list.admits,
Logger: logger,
})
@@ -512,6 +425,8 @@ func newAuthPlugin(cfg Config, writer *sql.DB, instanceKey string, mailer mail.S
// §5/magic-links.md: "the sign-in page stays yours" — they report
// outcomes by redirecting back here with a query this handler reads.
type signinPage struct {
+ ForceEmail bool
+ ContinueURL string
Title string
Body string
AddressLabel string
@@ -537,6 +452,7 @@ type signinPage struct {
func (h *handlers) signin(w http.ResponseWriter, r *http.Request) {
q := r.URL.Query()
data := signinPage{
+ ForceEmail: q.Get("force") != "",
Title: rastrillo.T(r, "docs.signin.title"),
Body: rastrillo.T(r, "docs.signin.body"),
AddressLabel: rastrillo.T(r, "docs.signin.address.label"),
diff --git a/internal/docs/auth_allowlist_test.go b/internal/docs/auth_allowlist_test.go
index ddb20ef..6b6c943 100644
--- a/internal/docs/auth_allowlist_test.go
+++ b/internal/docs/auth_allowlist_test.go
@@ -59,9 +59,7 @@ func newAllowlistedTestApp(t *testing.T, allowed string) *App {
// uses to skip signin's own keymail classification (which would
// otherwise make a real outbound DNS/HTTPS call per address, per
// signin's own README, entirely beside the point of what these tests
-// check). force=1 does not, and must not, change gateBegin's own
-// decision: gateBegin looks only at the address field, not force —
-// see gateBegin's own doc comment (auth.go).
+// check). force=1 selects email but does not bypass admissionMailer.
func postSignin(app *App, address string) *httptest.ResponseRecorder {
form := url.Values{"address": {address}, "force": {"1"}}
req := httptest.NewRequest(http.MethodPost, "/signin", strings.NewReader(form.Encode()))
@@ -95,7 +93,7 @@ func lastMailBody(app *App) string {
// is the test whose absence let that ship: every existing test in
// this package (before this task) proved something about admission,
// and none of them ever looked at whether a Send happened. Before
-// gateBegin existed, this test failed exactly this way — see this
+// the delivery gate existed, this test failed exactly this way — see this
// file's own commit message for how that was confirmed live, not
// assumed.
func TestNonPermittedAddressSendsNoMail(t *testing.T) {
diff --git a/internal/docs/auth_keymail_test.go b/internal/docs/auth_keymail_test.go
new file mode 100644
index 0000000..e360963
--- /dev/null
+++ b/internal/docs/auth_keymail_test.go
@@ -0,0 +1,257 @@
+package docs
+
+import (
+ "context"
+ "crypto/sha256"
+ "encoding/base64"
+ "encoding/json"
+ "fmt"
+ "html"
+ "io"
+ "net"
+ "net/http"
+ "net/http/cookiejar"
+ "net/http/httptest"
+ "net/url"
+ "os"
+ "os/exec"
+ "regexp"
+ "strings"
+ "sync"
+ "testing"
+ "time"
+)
+
+// Process isolation keeps the resolver and default transport overrides out of
+// concurrent tests. Every DNS packet and HTTPS request stays in this fixture.
+func TestKeymailOAuthActualIntegration(t *testing.T) {
+ if os.Getenv("DOCS_KEYMAIL_TEST_CHILD") == "1" {
+ keymailOAuthChild(t)
+ return
+ }
+ ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second)
+ defer cancel()
+ cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestKeymailOAuthActualIntegration$", "-test.count=1", "-test.v")
+ cmd.Dir = t.TempDir()
+ for _, e := range os.Environ() {
+ if strings.HasPrefix(e, "DOCS_") || strings.HasPrefix(e, "HOME_") || strings.HasPrefix(e, "CARLOS_") || strings.HasPrefix(e, "AWS_") {
+ continue
+ }
+ cmd.Env = append(cmd.Env, e)
+ }
+ cmd.Env = append(cmd.Env, "DOCS_KEYMAIL_TEST_CHILD=1")
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ t.Fatalf("isolated OAuth test: %v\n%s", err, out)
+ }
+}
+
+type keymailTestTransport func(*http.Request) (*http.Response, error)
+
+func (f keymailTestTransport) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
+
+func keymailOAuthChild(t *testing.T) {
+ dns, err := net.ListenPacket("udp", "127.0.0.1:0")
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer dns.Close()
+ go func() {
+ b := make([]byte, 4096)
+ for {
+ n, a, e := dns.ReadFrom(b)
+ if e != nil {
+ return
+ }
+ if n < 12 {
+ continue
+ }
+ reply := append([]byte(nil), b[:n]...)
+ reply[2] = 0x81
+ reply[3] = 0x83
+ for i := 6; i < 12; i++ {
+ reply[i] = 0
+ }
+ _, _ = dns.WriteTo(reply, a)
+ }
+ }()
+ net.DefaultResolver = &net.Resolver{PreferGo: true, Dial: func(ctx context.Context, _, _ string) (net.Conn, error) {
+ return (&net.Dialer{}).DialContext(ctx, "udp", dns.LocalAddr().String())
+ }}
+ const origin = "https://docs.oauth.test"
+ const provider = "mail.oauth.test"
+ for _, tc := range []struct {
+ name, address, returned string
+ admitted, detected, badState, badPKCE bool
+ }{
+ {name: "allowed OAuth", address: "member@mail.oauth.test", admitted: true, detected: true},
+ {name: "denied OAuth", address: "outsider@mail.oauth.test", detected: true},
+ {name: "wrong address", address: "member@mail.oauth.test", returned: "outsider@mail.oauth.test", admitted: true, detected: true},
+ {name: "wrong state", address: "member@mail.oauth.test", admitted: true, detected: true, badState: true},
+ {name: "wrong PKCE", address: "member@mail.oauth.test", admitted: true, detected: true, badPKCE: true},
+ {name: "ordinary mail", address: "member@mail.oauth.test", admitted: true},
+ {name: "denied ordinary mail", address: "outsider@mail.oauth.test"},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ var mu sync.Mutex
+ calls := map[string]int{}
+ challenge := ""
+ exchanges := 0
+ http.DefaultTransport = keymailTestTransport(func(r *http.Request) (*http.Response, error) {
+ mu.Lock()
+ defer mu.Unlock()
+ if r.URL.Scheme != "https" || r.URL.Host != provider {
+ t.Errorf("unexpected outbound request %s", r.URL)
+ return nil, fmt.Errorf("fixture refuses outbound request")
+ }
+ calls[r.URL.Path]++
+ status := http.StatusOK
+ var body any
+ switch r.URL.Path {
+ case "/.well-known/keymail":
+ body = map[string]any{"version": "1", "reauth": true}
+ case "/api/federation/lookup":
+ if r.URL.Query().Get("addr") != tc.address {
+ t.Errorf("wrong lookup address")
+ }
+ if !tc.detected {
+ status = http.StatusNotFound
+ }
+ body = map[string]bool{"ok": tc.detected}
+ case "/api/oauth/token":
+ exchanges++
+ var in map[string]string
+ if r.Method != "POST" || json.NewDecoder(io.LimitReader(r.Body, 8192)).Decode(&in) != nil {
+ t.Error("invalid token request")
+ }
+ sum := sha256.Sum256([]byte(in["code_verifier"]))
+ if in["client_id"] != origin || in["redirect_uri"] != origin+"/auth/callback" || in["code"] != "one-code" || base64.RawURLEncoding.EncodeToString(sum[:]) != challenge || exchanges != 1 {
+ status = http.StatusBadRequest
+ body = map[string]string{"error": "invalid grant"}
+ } else {
+ address := tc.address
+ if tc.returned != "" {
+ address = tc.returned
+ }
+ body = map[string]any{"access_token": "synthetic-token", "token_type": "bearer", "address": address, "auth_time": time.Now().Unix()}
+ }
+ default:
+ t.Errorf("unexpected endpoint %s", r.URL.Path)
+ status = http.StatusNotFound
+ }
+ raw, _ := json.Marshal(body)
+ return &http.Response{StatusCode: status, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(string(raw))), Request: r}, nil
+ })
+ app, e := NewForTestWith(t, Config{Origin: origin, AllowedAddresses: "member@mail.oauth.test"})
+ if e != nil {
+ t.Fatal(e)
+ }
+ jar, _ := cookiejar.New(nil)
+ request := func(method, path, body string) *httptest.ResponseRecorder {
+ u, _ := url.Parse(origin + path)
+ req := httptest.NewRequest(method, u.String(), strings.NewReader(body))
+ req.Header.Set("Origin", origin)
+ req.Header.Set("Sec-Fetch-Site", "same-origin")
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ for _, c := range jar.Cookies(u) {
+ req.AddCookie(c)
+ }
+ rec := httptest.NewRecorder()
+ app.Handler().ServeHTTP(rec, req)
+ jar.SetCookies(u, rec.Result().Cookies())
+ return rec
+ }
+ rec := request("POST", "/signin", url.Values{"address": {tc.address}}.Encode())
+ mu.Lock()
+ lookups := calls["/api/federation/lookup"]
+ mu.Unlock()
+ if lookups != 1 {
+ t.Fatalf("actual classifier lookup count=%d, status=%d", lookups, rec.Code)
+ }
+ if !tc.detected {
+ if rec.Code != 303 || rec.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("mail start: %d %s", rec.Code, rec.Header().Get("Location"))
+ }
+ app.mailRecorder.mu.Lock()
+ sent := app.mailRecorder.sends
+ app.mailRecorder.mu.Unlock()
+ want := 0
+ if tc.admitted {
+ want = 1
+ }
+ if sent != want {
+ t.Fatalf("mail deliveries=%d want%d", sent, want)
+ }
+ return
+ }
+ if rec.Code != 200 || rec.Header().Get("Location") != "" {
+ t.Fatalf("OAuth continuation: %d", rec.Code)
+ }
+ var target *url.URL
+ for _, m := range regexp.MustCompile(`href="([^"]+)"`).FindAllStringSubmatch(rec.Body.String(), -1) {
+ u, e := url.Parse(html.UnescapeString(m[1]))
+ if e == nil && u.Host == provider && u.Path == "/oauth/authorize" {
+ target = u
+ }
+ }
+ if target == nil {
+ t.Fatal("no generated OAuth authorize link")
+ }
+ q := target.Query()
+ if q.Get("client_id") != origin || q.Get("redirect_uri") != origin+"/auth/callback" || q.Get("code_challenge_method") != "S256" || q.Get("prompt") != "login" || q.Get("state") == "" {
+ t.Fatal("OAuth bindings missing")
+ }
+ mu.Lock()
+ challenge = q.Get("code_challenge")
+ if tc.badPKCE {
+ challenge = "wrong"
+ }
+ mu.Unlock()
+ state := q.Get("state")
+ if tc.badState {
+ state = "wrong"
+ }
+ callback := request("GET", "/auth/callback?"+url.Values{"state": {state}, "code": {"one-code"}}.Encode(), "")
+ allowed := tc.admitted && tc.returned == "" && !tc.badState && !tc.badPKCE
+ home := request("GET", "/", "")
+ if allowed {
+ if callback.Code != 303 || home.Code != 200 {
+ t.Fatalf("authorized callback=%d home=%d", callback.Code, home.Code)
+ }
+ } else if home.Code == 200 {
+ t.Fatal("refused OAuth created admitted session")
+ }
+ if !allowed {
+ u, _ := url.Parse(origin)
+ for _, c := range jar.Cookies(u) {
+ if strings.Contains(c.Name, "session") {
+ t.Fatal("refused OAuth minted session")
+ }
+ }
+ }
+ app.mailRecorder.mu.Lock()
+ sent := app.mailRecorder.sends
+ app.mailRecorder.mu.Unlock()
+ if sent != 0 {
+ t.Fatalf("OAuth sent %d emails", sent)
+ }
+ mu.Lock()
+ got := exchanges
+ mu.Unlock()
+ want := 1
+ if tc.badState {
+ want = 0
+ }
+ if got != want {
+ t.Fatalf("token exchanges=%d want%d", got, want)
+ }
+ // Pending state is cleared after callback: replay cannot reach exchange.
+ request("GET", "/auth/callback?"+url.Values{"state": {q.Get("state")}, "code": {"one-code"}}.Encode(), "")
+ mu.Lock()
+ defer mu.Unlock()
+ if exchanges != want {
+ t.Fatal("callback replay reached token endpoint")
+ }
+ })
+ }
+}
diff --git a/internal/docs/auth_navigation.go b/internal/docs/auth_navigation.go
new file mode 100644
index 0000000..e8a83cb
--- /dev/null
+++ b/internal/docs/auth_navigation.go
@@ -0,0 +1,92 @@
+package docs
+
+import (
+ "bytes"
+ "net/http"
+ "net/url"
+ "strings"
+
+ "github.com/carlosframework/rastrillo"
+)
+
+// End the POST with a document before navigating to an identity provider.
+// Safari applies form-action to a redirect chain, including intermediate GETs.
+// The continuation keeps the original CSP and works without JavaScript.
+func (h *handlers) beginSignin(next http.HandlerFunc) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ result := &signinResponse{header: make(http.Header)}
+ next(result, r)
+ destination := result.header.Get("Location")
+ external := destination != "" && !strings.HasPrefix(destination, "/")
+ if result.status == http.StatusSeeOther && external {
+ if !keymailDestination(destination, h.origin) {
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusBadGateway)
+ return
+ }
+ for _, cookie := range result.header.Values("Set-Cookie") {
+ w.Header().Add("Set-Cookie", cookie)
+ }
+ w.Header().Set("Cache-Control", "no-store")
+ w.Header().Set("Referrer-Policy", "no-referrer")
+ data := signinPage{
+ Title: rastrillo.T(r, "docs.signin.title"),
+ SubmitLabel: rastrillo.T(r, "docs.signin.keymail"),
+ ContinueURL: destination,
+ }
+ if err := h.render.page(w, r, "signin", data); err != nil {
+ h.logger.Error("docs: render Keymail continuation", "err", err)
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)
+ }
+ return
+ }
+ for name, values := range result.header {
+ for _, value := range values {
+ w.Header().Add(name, value)
+ }
+ }
+ if result.status == 0 {
+ result.status = http.StatusOK
+ }
+ w.WriteHeader(result.status)
+ _, _ = w.Write(result.body.Bytes())
+ }
+}
+
+// The destination comes only from the identity plugin's server-generated
+// redirect, never from return_to or another submitted URL.
+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"
+}
+
+type signinResponse struct {
+ header http.Header
+ status int
+ body bytes.Buffer
+}
+
+func (w *signinResponse) Header() http.Header { return w.header }
+func (w *signinResponse) WriteHeader(status int) {
+ if w.status == 0 {
+ w.status = status
+ }
+}
+func (w *signinResponse) Write(p []byte) (int, error) {
+ if w.status == 0 {
+ w.WriteHeader(http.StatusOK)
+ }
+ return w.body.Write(p)
+}
diff --git a/internal/docs/auth_navigation_test.go b/internal/docs/auth_navigation_test.go
new file mode 100644
index 0000000..a1f9584
--- /dev/null
+++ b/internal/docs/auth_navigation_test.go
@@ -0,0 +1,100 @@
+package docs
+
+import (
+ "encoding/json"
+ "html"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "github.com/keymaildev/signin"
+)
+
+func TestKeymailContinuation(t *testing.T) {
+ app := newAllowlistedTestApp(t, "alice@example.test")
+ app.handlers.origin = "https://docs.example.test"
+ destination := (&signin.Keymail{Base: "https://keymail.example.test", Origin: app.handlers.origin}).AuthorizeURL("state", "challenge", true)
+ handler := app.handlers.beginSignin(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 := homeFormPolicy("https://home.example.test")
+ rec.Header().Set("Content-Security-Policy", policy)
+ handler(rec, httptest.NewRequest("POST", "/signin", nil))
+ if rec.Code != 200 || rec.Header().Get("Location") != "" {
+ t.Fatalf("status=%d location=%q", rec.Code, rec.Header().Get("Location"))
+ }
+ if rec.Header().Get("Content-Security-Policy") != policy {
+ t.Fatal("CSP changed")
+ }
+ if rec.Header().Get("Cache-Control") != "no-store" || rec.Header().Get("Referrer-Policy") != "no-referrer" {
+ t.Fatal("continuation must not cache or disclose the referring URL")
+ }
+ 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 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("missing continuation component %q", part)
+ }
+ }
+ if strings.Contains(body, `action="/signin"`) {
+ t.Fatal("continuation rendered another sign-in form")
+ }
+ if dir := os.Getenv("DOCS_OAUTH_BROWSER_FIXTURE"); dir != "" {
+ if err := os.MkdirAll(dir, 0700); err != nil {
+ t.Fatal(err)
+ }
+ data, err := json.Marshal(map[string]any{"body": rec.Body.String(), "headers": rec.Header(), "destination": destination})
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(filepath.Join(dir, "continuation.json"), data, 0600); err != nil {
+ t.Fatal(err)
+ }
+ }
+}
+
+func TestKeymailDestinationRefusals(t *testing.T) {
+ origin := "https://docs.example.test"
+ valid := (&signin.Keymail{Base: "https://keymail.example.test", Origin: origin}).AuthorizeURL("state", "challenge", false)
+ 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(origin), url.QueryEscape("https://foreign.example.test")),
+ strings.Replace(valid, "S256", "plain", 1),
+ } {
+ if keymailDestination(target, origin) {
+ t.Fatalf("accepted %q", target)
+ }
+ }
+ app := newAllowlistedTestApp(t, "alice@example.test")
+ app.handlers.origin = origin
+ rec := httptest.NewRecorder()
+ app.handlers.beginSignin(func(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "https://foreign.example.test", 303) })(rec, httptest.NewRequest("POST", "/signin", nil))
+ if rec.Code != 502 || rec.Header().Get("Location") != "" {
+ t.Fatalf("invalid redirect status=%d location=%q", rec.Code, rec.Header().Get("Location"))
+ }
+}
+
+func TestSigninEmailFallbackIsExplicit(t *testing.T) {
+ app := newAllowlistedTestApp(t, "alice@example.test")
+ for _, path := range []string{"/signin", "/signin?force=1&err=keymail"} {
+ rec := httptest.NewRecorder()
+ app.Handler().ServeHTTP(rec, httptest.NewRequest("GET", path, nil))
+ if rec.Code != 200 {
+ t.Fatal(rec.Code)
+ }
+ forced := strings.Contains(rec.Body.String(), `name="force" value="1"`)
+ if forced != strings.Contains(path, "force=1") {
+ t.Fatalf("email force for %s = %v", path, forced)
+ }
+ }
+}
diff --git a/internal/docs/render.go b/internal/docs/render.go
index 8122c49..e7dea7a 100644
--- a/internal/docs/render.go
+++ b/internal/docs/render.go
@@ -244,9 +244,10 @@ var Catalog = rastrillo.Catalog{
// either (rastrillo/auth's own handlers already refuse to be an
// oracle between "wrong" and "unknown").
"docs.signin.title": "Sign in",
- "docs.signin.body": "Enter your email address and we'll send you a link to sign in.",
+ "docs.signin.body": "Enter your email address to sign in.",
"docs.signin.address.label": "Email address",
- "docs.signin.submit": "Send link",
+ "docs.signin.submit": "Continue",
+ "docs.signin.keymail": "Continue to Keymail",
"docs.signin.sent": "Check your email for a link to sign in.",
"docs.signin.error.rate": "Too many attempts. Wait a few minutes and try again.",
"docs.signin.error.address": "That doesn't look like a valid email address.",
diff --git a/internal/docs/views/signin.html b/internal/docs/views/signin.html
index 0c173fb..109ef5e 100644
--- a/internal/docs/views/signin.html
+++ b/internal/docs/views/signin.html
@@ -8,16 +8,6 @@
already-translated strings; this template only decides whether
to show them.
- The form itself POSTs straight to rastrillo/auth's own Begin
- (mounted at POST /signin, app.go) — there is no handler of this
- app's own in that path at all. force=1: this app never offers a
- passkey or keymail ceremony (spec §8.1's ruling — see auth.go's
- package comment), so there is nothing for a person to choose
- between; force always sends the plain magic link, skipping
- signin's classification network call outright rather than
- silently paying its latency for a decision this app never acts
- on.
-
Writes the bare rst-* attribute grammar throughout, same
convention as list.html/show.html and the same test
(render_test.go's TestOurTemplatesUseTheAttributeGrammar) holds
@@ -125,14 +115,19 @@
{{template "callout" dict "Body" .ErrorMessage "Tone" "negative" "Alert" true}}
{{end}}
<div rst-box>
- <div rst-box-head>
+ {{if .Body}}<div rst-box-head>
<p>{{.Body}}</p>
- </div>
+ </div>{{end}}
+ {{if .ContinueURL}}
+ <meta http-equiv="refresh" content="0;url={{.ContinueURL}}">
+ <a rst-btn="primary" href="{{.ContinueURL}}">{{.SubmitLabel}}</a>
+ {{else}}
<form docs-signin-form method="post" action="/signin">
- <input type="hidden" name="force" value="1">
+ {{if .ForceEmail}}<input type="hidden" name="force" value="1">{{end}}
{{template "field" dict "ID" "signin-address" "Name" "address" "Type" "email" "Label" .AddressLabel "Autocomplete" "email" "Required" true}}
<button rst-btn="primary" type="submit">{{.SubmitLabel}}</button>
</form>
+ {{end}}
</div>
</div>
{{end}}