oficina / sheets Public

fix: do not email sign-in links to addresses that cannot sign in

rastrillo/auth consults Config.Authorize at admission — the moment a
verified address would receive a session — not when the link is sent. The
security property holds either way: a stranger cannot get a session no
matter how many links they redeem.

What it leaves open is abuse rather than access. Anyone who can reach the
instance could make it email a live sign-in link to any address they
named, at whatever rate the IP limiter allows. The recipient gets
unsolicited mail and our sending identity wears it.

Found by POSTing a stranger's address at the deployed instance and
getting ?sent=1 back. Every unit test passed, because they all tested
admission and none tested delivery — the gap was between two things that
were each individually correct.

The refusal is deliberately invisible. Answering "that address cannot
sign in here" would make this endpoint a membership oracle: anyone could
enumerate the team by watching which addresses get a different answer. A
non-admitted address now gets byte-identical output to an admitted one
and no email. The cost is accepted and real — a member who mistypes their
own address is told to check their email and nothing arrives.

Four tests, with a recording mailer so "no email was sent" is observed
rather than asserted: strangers get none, members still get one (without
which, refusing everyone would pass), member and stranger are
indistinguishable in status, Location and body, and a capitalised member
address still receives one. Verified by removing the gate and watching
the stranger test fail with the address it leaked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS

Paul Campbell pushed by paul@keymail.dev 1 month ago 67712173e8223c8dfbc2dad4de6e0c648b87321b parent 52c5175

9 files changed, +191 −13

cmd/sheets/main.go +1 −1
internal/sheets/app.go +9 −2
internal/sheets/app_test.go +1 −1
internal/sheets/auth.go +2 −2
internal/sheets/auth_test.go +3 −3
internal/sheets/begin.go +43 −0
internal/sheets/begin_test.go +128 −0
internal/sheets/page_test.go +1 −1
internal/sheets/signin_test.go +3 −3

Intent

full ledger: core

diff --git a/cmd/sheets/main.go b/cmd/sheets/main.go

index 61c598e..29ce1c9 100644

--- a/cmd/sheets/main.go

+++ b/cmd/sheets/main.go

@@ -80,7 +80,7 @@ func main() {

os.Exit(1)

}

- app := sheets.New(d.G, signin)

+ app := sheets.New(d.G, signin, sheets.Admits(authEnv.Owners))

// rastrillo.Options.Mux wants a *http.ServeMux; the app's own

// router is a chi.Router (an http.Handler), so it mounts under one.

diff --git a/internal/sheets/app.go b/internal/sheets/app.go

index 78597f7..3820d23 100644

--- a/internal/sheets/app.go

+++ b/internal/sheets/app.go

@@ -14,6 +14,11 @@ import (

type App struct {

db *gorm.DB

auth *auth.Auth

+ // admit is the allowlist, consulted before a link is SENT.

+ // auth.Config.Authorize covers the same list at redemption; this

+ // one exists so we do not email strangers. See handleBegin.

+ admit func(string) bool

}

// New builds an App. db may be nil in tests that do not touch storage.

@@ -23,7 +28,9 @@ type App struct {

// without a database or a mailer. Production always passes one — main

// refuses to boot without the configuration auth needs, so there is no

// path there that reaches this with nil.

-func New(db *gorm.DB, a *auth.Auth) *App { return &App{db: db, auth: a} }

+func New(db *gorm.DB, a *auth.Auth, admit func(string) bool) *App {

+ return &App{db: db, auth: a, admit: admit}

// Router returns the mounted handler.

func (a *App) Router() http.Handler {

@@ -48,7 +55,7 @@ func (a *App) Router() http.Handler {

// the paths its own docs use. Verify reads ?token=; the path is

// baked into every emailed link, so moving it later invalidates

// links already in people's inboxes.

- r.Post("/signin", a.auth.Begin)

+ r.Post("/signin", a.handleBegin)

r.Get("/auth/callback", a.auth.Callback)

r.Get("/auth/verify", a.auth.Verify)

r.Post("/signout", a.auth.Signout)

diff --git a/internal/sheets/app_test.go b/internal/sheets/app_test.go

index d6007c0..10baba0 100644

--- a/internal/sheets/app_test.go

+++ b/internal/sheets/app_test.go

@@ -12,7 +12,7 @@ import (

// ahead of this one and wins (see the comment on Router in app.go); this

// test only proves the app's router itself is wired and reachable.

func TestHealthz(t *testing.T) {

- app := New(nil, nil)

+ app := New(nil, nil, nil)

req := httptest.NewRequest(http.MethodGet, "/healthz", nil)

rec := httptest.NewRecorder()

app.Router().ServeHTTP(rec, req)

diff --git a/internal/sheets/auth.go b/internal/sheets/auth.go

index 011afc6..4861efc 100644

--- a/internal/sheets/auth.go

+++ b/internal/sheets/auth.go

@@ -135,7 +135,7 @@ func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger) (*auth.Auth, error) {

Origin: env.Origin,

InstanceKey: env.InstanceKey,

Mailer: env.Mailer,

- Authorize: admits(env.Owners),

+ Authorize: Admits(env.Owners),

SigninPath: "/signin",

Subject: "Your Sheets sign-in link",

Logger: logger,

@@ -154,7 +154,7 @@ func NewAuth(db *sql.DB, env AuthEnv, logger *slog.Logger) (*auth.Auth, error) {

// normalisation — addresses are case-sensitive before the @ per RFC

// 5321, and no mainstream provider honours that — so this matches what

// operators expect when they type an allowlist.

-func admits(owners []string) func(string) bool {

+func Admits(owners []string) func(string) bool {

allowed := make(map[string]struct{}, len(owners))

for _, o := range owners {

allowed[o] = struct{}{}

diff --git a/internal/sheets/auth_test.go b/internal/sheets/auth_test.go

index 1680408..c3b31ba 100644

--- a/internal/sheets/auth_test.go

+++ b/internal/sheets/auth_test.go

@@ -131,7 +131,7 @@ func TestLoopbackIsNotPrefixSpoofable(t *testing.T) {

}

func TestAdmitsIsFailClosed(t *testing.T) {

- allow := admits(splitOwners("Paul@Example.com, other@example.com,"))

+ allow := Admits(splitOwners("Paul@Example.com, other@example.com,"))

for _, tc := range []struct {

address string

want bool

@@ -144,13 +144,13 @@ func TestAdmitsIsFailClosed(t *testing.T) {

{"", false},

} {

if got := allow(tc.address); got != tc.want {

- t.Errorf("admits(%q) = %v, want %v", tc.address, got, tc.want)

+ t.Errorf("Admits(%q) = %v, want %v", tc.address, got, tc.want)

}

// TestSplitOwnersDropsBlanks: a trailing comma or a stray space must not

-// put an empty string in the allowlist, which admits("") would then let

+// put an empty string in the allowlist, which Admits("") would then let

// through.

func TestSplitOwnersDropsBlanks(t *testing.T) {

for _, raw := range []string{"", " ", ",", " , , "} {

diff --git a/internal/sheets/begin.go b/internal/sheets/begin.go

new file mode 100644

index 0000000..50efd5d

--- /dev/null

+++ b/internal/sheets/begin.go

@@ -0,0 +1,43 @@

+package sheets

+import "net/http"

+// handleBegin gates POST /signin on the allowlist BEFORE auth mints and

+// emails a link.

+//

+// rastrillo/auth checks Config.Authorize at admission — the moment a

+// verified address would receive a session — not when the link is sent.

+// That is the right place for the security decision, and the security

+// property holds without this: an address that is not admitted cannot

+// get a session no matter how many links it redeems.

+//

+// What it leaves open is abuse, not access. Without this gate, anyone

+// who can reach the instance can make it email a live sign-in link to

+// any address they name, at whatever rate the IP limiter allows. The

+// recipient gets unsolicited mail from us and our sending identity wears

+// it. Found by POSTing a stranger's address at the deployed instance and

+// getting ?sent=1 back — the unit tests all passed, because they tested

+// admission rather than delivery.

+//

+// The refusal is deliberately INVISIBLE. Answering "that address cannot

+// sign in" would turn this endpoint into a membership oracle: anyone

+// could enumerate who is on the team by watching which addresses get a

+// different answer. So a non-admitted address gets exactly the response

+// an admitted one gets, and no email. The cost is real and accepted: a

+// member who mistypes their own address is told to check their email and

+// nothing arrives. Every mainstream provider makes the same trade.

+func (a *App) handleBegin(w http.ResponseWriter, r *http.Request) {

+ // Parse before reading the field: Begin would do this itself, and

+ // this handler has to read the address before deciding to call it.

+ if err := r.ParseForm(); err != nil {

+ http.Error(w, "bad form", http.StatusBadRequest)

+ return

+ }

+ if a.admit != nil && !a.admit(r.FormValue("address")) {

+ // Identical to auth's own success redirect, so the two cases are

+ // indistinguishable from outside.

+ http.Redirect(w, r, "/signin?sent=1", http.StatusSeeOther)

+ return

+ }

+ a.auth.Begin(w, r)

diff --git a/internal/sheets/begin_test.go b/internal/sheets/begin_test.go

new file mode 100644

index 0000000..3229bf8

--- /dev/null

+++ b/internal/sheets/begin_test.go

@@ -0,0 +1,128 @@

+package sheets

+import (

+ "context"

+ "net/http"

+ "net/http/httptest"

+ "path/filepath"

+ "strings"

+ "sync"

+ "testing"

+ rdb "github.com/carlosframework/rastrillo/db"

+ "github.com/carlosframework/rastrillo/migrate"

+// recorder is a mail.Sender that delivers nothing and remembers who it

+// was asked to write to.

+type recorder struct {

+ mu sync.Mutex

+ sent []string

+func (m *recorder) Send(_ context.Context, to, _, _ string) error {

+ m.mu.Lock()

+ defer m.mu.Unlock()

+ m.sent = append(m.sent, to)

+ return nil

+func (m *recorder) addresses() []string {

+ m.mu.Lock()

+ defer m.mu.Unlock()

+ return append([]string(nil), m.sent...)

+const testOrigin = "https://sheets.example.com"

+func appWithRecorder(t *testing.T) (*App, *recorder) {

+ t.Helper()

+ d, err := rdb.Open(filepath.Join(t.TempDir(), "test.db"), quietLogger())

+ if err != nil {

+ t.Fatalf("open db: %v", err)

+ }

+ t.Cleanup(func() { d.Close() })

+ if _, err := migrate.Apply(context.Background(), d, BootSchema); err != nil {

+ t.Fatalf("migrate: %v", err)

+ }

+ rec := &recorder{}

+ owners := []string{"paul@example.com"}

+ a, err := NewAuth(d.Writer(), AuthEnv{

+ Origin: testOrigin,

+ InstanceKey: "test-instance-key",

+ Owners: owners,

+ Mailer: rec,

+ }, quietLogger())

+ if err != nil {

+ t.Fatalf("NewAuth: %v", err)

+ }

+ return New(d.G, a, Admits(owners)), rec

+func postSignin(t *testing.T, app *App, address string) *httptest.ResponseRecorder {

+ t.Helper()

+ req := httptest.NewRequest(http.MethodPost, "/signin",

+ strings.NewReader("address="+address))

+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")

+ req.Header.Set("Origin", testOrigin)

+ w := httptest.NewRecorder()

+ app.Router().ServeHTTP(w, req)

+ return w

+// TestBeginDoesNotEmailStrangers is the defect this handler exists for.

+// auth checks Authorize at redemption, so without the gate a stranger

+// still cannot get a session — but the instance cheerfully emails them a

+// live link, which is abuse of our sending identity, not access control.

+func TestBeginDoesNotEmailStrangers(t *testing.T) {

+ app, mail := appWithRecorder(t)

+ w := postSignin(t, app, "stranger@example.com")

+ if got := mail.addresses(); len(got) != 0 {

+ t.Errorf("emailed a stranger: %v", got)

+ }

+ if w.Code != http.StatusSeeOther {

+ t.Errorf("status = %d, want 303", w.Code)

+ }

+// TestBeginStillEmailsMembers is the other half: the gate must not have

+// broken the thing it guards. Without this, refusing everyone would pass

+// the test above.

+func TestBeginStillEmailsMembers(t *testing.T) {

+ app, mail := appWithRecorder(t)

+ postSignin(t, app, "paul@example.com")

+ got := mail.addresses()

+ if len(got) != 1 || got[0] != "paul@example.com" {

+ t.Fatalf("member mail = %v, want one to paul@example.com", got)

+ }

+// TestBeginIsNotAMembershipOracle: the refusal has to be invisible, or

+// the endpoint becomes a way to enumerate the team. Member and stranger

+// must be indistinguishable from outside — same status, same location.

+func TestBeginIsNotAMembershipOracle(t *testing.T) {

+ app, _ := appWithRecorder(t)

+ member := postSignin(t, app, "paul@example.com")

+ stranger := postSignin(t, app, "stranger@example.com")

+ if member.Code != stranger.Code {

+ t.Errorf("status differs: member %d, stranger %d", member.Code, stranger.Code)

+ }

+ if m, s := member.Header().Get("Location"), stranger.Header().Get("Location"); m != s {

+ t.Errorf("Location differs: member %q, stranger %q", m, s)

+ }

+ if member.Body.String() != stranger.Body.String() {

+ t.Error("body differs between member and stranger")

+ }

+// TestBeginGateIsCaseInsensitive: the allowlist is compared lowercased,

+// so a member typing their address with different capitalisation must

+// still receive a link rather than being silently dropped.

+func TestBeginGateIsCaseInsensitive(t *testing.T) {

+ app, mail := appWithRecorder(t)

+ postSignin(t, app, "PAUL@example.com")

+ if got := mail.addresses(); len(got) != 1 {

+ t.Errorf("mail for capitalised member address = %v, want one", got)

+ }

diff --git a/internal/sheets/page_test.go b/internal/sheets/page_test.go

index 5771117..beb8fd5 100644

--- a/internal/sheets/page_test.go

+++ b/internal/sheets/page_test.go

@@ -10,7 +10,7 @@ import (

func get(t *testing.T, path string) *httptest.ResponseRecorder {

t.Helper()

rec := httptest.NewRecorder()

- New(nil, nil).Router().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil))

+ New(nil, nil, nil).Router().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil))

return rec

}

diff --git a/internal/sheets/signin_test.go b/internal/sheets/signin_test.go

index 0a68db0..57bcead 100644

--- a/internal/sheets/signin_test.go

+++ b/internal/sheets/signin_test.go

@@ -47,7 +47,7 @@ func TestSigninMessageSentAndEmpty(t *testing.T) {

func TestSigninPageDoesNotEchoQuery(t *testing.T) {

rec := httptest.NewRecorder()

req := httptest.NewRequest(http.MethodGet, "/signin?err=%3Cscript%3Ealert(1)%3C/script%3E", nil)

- New(nil, nil).Router().ServeHTTP(rec, req)

+ New(nil, nil, nil).Router().ServeHTTP(rec, req)

if rec.Code != http.StatusOK {

t.Fatalf("GET /signin = %d", rec.Code)

}

@@ -80,7 +80,7 @@ func testAuth(t *testing.T) *App {

if err != nil {

t.Fatalf("NewAuth: %v", err)

}

- return New(d.G, a)

+ return New(d.G, a, Admits([]string{"paul@example.com"}))

}

// TestAuthRoutesAreMounted proves the four handlers are reachable. It

@@ -107,7 +107,7 @@ func TestAuthRoutesAreMounted(t *testing.T) {

// pins that those codes really do mean "not mounted", so the assertion

// there is measuring what it claims to.

func TestAuthRoutesAbsentWithoutAuth(t *testing.T) {

- app := New(nil, nil)

+ app := New(nil, nil, nil)

for _, tc := range []struct{ method, path string }{

{http.MethodPost, "/signin"},

{http.MethodGet, "/auth/verify"},