amadan
Gate POST /signin on an address allowlist, before Begin sends mail
rastrillo/auth's Config.Authorize is only ever consulted at admission (the callback, after a link is clicked) — never at send time, in Begin. With Authorize left nil (as it was), that meant anyone who could reach this instance could POST any address to /signin and get a working sign-in link mailed to it: an open relay on our own SES sending identity, and unrestricted account creation on an instance-per-team app. Found by the CARLOS Sheets session on their own deployed instance; every existing test passed because they all tested admission, none tested delivery. $DOCS_ALLOWED_ADDRESSES (docs.Config.AllowedAddresses) is wired to two places, not one: rauth.Config.Authorize (admission, belt-and-suspenders) and a new gateBegin wrapper mounted in front of rauth.Auth.Begin on POST /signin itself — the wiring that actually matters, since Begin is what sends the mail. A non-permitted address gets byte-identical output to a permitted one (same status, same Location, same body) so the endpoint cannot become a membership oracle; TestSigninResponseIdenticalForNonPermittedAddress asserts the raw bytes. Comparison is case-insensitive and whitespace-trimmed. An empty allowlist admits everyone only in Env "development" (so TestSignIn and run-dev.sh need no setup); anywhere else it's a startup refusal, matching Origin/InstanceKey/Mailer's existing fail-closed shape — chosen over silently admitting nobody, since a silently-empty allowlist would boot looking identical to a working instance and could go unnoticed indefinitely, where a refusal is loud and gets fixed the same day. Confirmed TestNonPermittedAddressSendsNoMail actually catches the regression: temporarily wiring POST /signin straight to authPlugin.Begin (bypassing gateBegin, leaving only the Authorize wiring) made it fail with mail recorded for a non-permitted address; restoring gateBegin fixes it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
5 files changed,
+512
−15
AGENTS.md+27 −0cmd/docs/main.go+18 −6internal/docs/app.go+56 −3internal/docs/auth.go+135 −6internal/docs/auth_allowlist_test.go+276 −0
diff --git a/AGENTS.md b/AGENTS.md| index dd5280f..f4f295f 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -63,6 +63,33 @@ env var (or with `$DOCS_ENV=development` and no flag), is |
| `"production"` — `"development"` always takes an explicit `-env` |
| flag, never a silent fallback. |
| +### Sign-in allowlist (Task 4 security fix, pre-launch) |
| + |
| +Real sign-in above closed "who can get a session" but left a separate |
| +hole open: `rastrillo/auth`'s `Config.Authorize` — the admission gate — |
| +is consulted only in its own callback handler, after `POST /signin` |
| +(`Begin`) has already emailed the link. With `Authorize` left `nil` (as |
| +it was through Task 3), that meant anyone who could reach the instance |
| +could POST any address and get a working sign-in link mailed to it — |
| +an open relay on our own SES sending identity, and unrestricted account |
| +creation on an app whose whole premise is instance-per-team. Found by |
| +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. |
| +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, |
| +chosen over silently admitting nobody because a silently-empty allowlist |
| +would boot looking identical to a working instance. |
| + |
| ## Running it locally |
| `./scripts/run-dev.sh` builds and runs the app in one command: `-env |
diff --git a/cmd/docs/main.go b/cmd/docs/main.go| index 59164b6..3780808 100644 |
| --- a/cmd/docs/main.go |
| +++ b/cmd/docs/main.go |
| @@ -218,6 +218,17 @@ func main() { |
| } |
| } |
| + // $DOCS_ALLOWED_ADDRESSES is the comma-separated allowlist |
| + // docs.Config.AllowedAddresses' own doc comment describes — the |
| + // only addresses this instance ever mails a sign-in link to or |
| + // admits a session for. Left empty here when unset: docs.New |
| + // (via resolveAllowlist, internal/docs/auth.go) refuses to start |
| + // on an empty one outside development, the same fail-closed shape |
| + // as InstanceKey and Mailer just above — never "admit everyone", |
| + // which is what an unset allowlist reaching a public instance |
| + // would otherwise silently mean. |
| + allowedAddresses := os.Getenv("DOCS_ALLOWED_ADDRESSES") |
| + |
| // chooseEnv: -env wins; $DOCS_ENV wins next EXCEPT for |
| // "development" itself, which chooseEnv never honours from an env |
| // var (fix round 2 — see its own doc comment); the default with |
| @@ -241,12 +252,13 @@ func main() { |
| // — otherwise Serve would open a second connection to the file New |
| // already owns. |
| cfg := docs.Config{ |
| - DBPath: opts.DBPath, |
| - Env: env, |
| - Origin: origin, |
| - InstanceKey: instanceKey, |
| - Mailer: mailer, |
| - Logger: logger, |
| + DBPath: opts.DBPath, |
| + Env: env, |
| + Origin: origin, |
| + InstanceKey: instanceKey, |
| + Mailer: mailer, |
| + AllowedAddresses: allowedAddresses, |
| + Logger: logger, |
| } |
| app, err := docs.New(cfg) |
| if err != nil { |
diff --git a/internal/docs/app.go b/internal/docs/app.go| index b9a0c67..5be3dfe 100644 |
| --- a/internal/docs/app.go |
| +++ b/internal/docs/app.go |
| @@ -129,6 +129,37 @@ type Config struct { |
| // split Origin/InstanceKey use. |
| Mailer mail.Sender |
| + // AllowedAddresses is the comma-separated allowlist of the only |
| + // addresses this instance admits — $DOCS_ALLOWED_ADDRESSES |
| + // (cmd/docs/main.go), the same "<APP>_..." env-var convention |
| + // Origin and the SMTP vars above use. Compared case-insensitively, |
| + // each entry trimmed of surrounding whitespace (auth.go's |
| + // 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. |
| + // |
| + // Empty is allowed only when Env is "development" |
| + // (resolveAllowlist below defaults that to "admit everyone", so |
| + // TestSignIn and scripts/run-dev.sh keep working with no setup of |
| + // their own) — anywhere else, empty is a startup refusal; see |
| + // resolveAllowlist's own doc comment for why that beats the |
| + // alternative of silently admitting nobody. |
| + AllowedAddresses string |
| + |
| // Logger defaults to slog.Default(). |
| Logger *slog.Logger |
| } |
| @@ -264,6 +295,15 @@ func New(cfg Config) (*App, error) { |
| if err != nil { |
| return nil, err |
| } |
| + // Same fail-closed shape again: an instance with nobody configured |
| + // to sign in either refuses to start (outside development) or, in |
| + // development, admits everyone — never a silent "admits nobody" |
| + // that would boot looking healthy. See resolveAllowlist's own doc |
| + // comment for the full reasoning. |
| + list, err := resolveAllowlist(cfg) |
| + if err != nil { |
| + return nil, err |
| + } |
| d, err := db.Open(cfg.DBPath, logger) |
| if err != nil { |
| @@ -295,8 +335,11 @@ func New(cfg Config) (*App, error) { |
| // 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. |
| - authPlugin, err := newAuthPlugin(cfg, d.Writer(), instanceKey, mailer, logger) |
| + // 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()) |
| d.Close() |
| @@ -316,7 +359,17 @@ func New(cfg Config) (*App, error) { |
| // static assets that page (and the shell around it) needs to |
| // render at all. |
| r.Get("/signin", h.signin) |
| - r.Post("/signin", authPlugin.Begin) |
| + // 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.Get("/auth/callback", authPlugin.Callback) |
| r.Get("/auth/verify", authPlugin.Verify) |
| r.Post("/signout", authPlugin.Signout) |
diff --git a/internal/docs/auth.go b/internal/docs/auth.go| index 2b7d0c1..bfc5ff1 100644 |
| --- a/internal/docs/auth.go |
| +++ b/internal/docs/auth.go |
| @@ -126,22 +126,151 @@ func (m *recordingMailer) Send(ctx context.Context, to, subject, body string) er |
| // carry for the sign-in link itself. |
| var magicLinkPattern = regexp.MustCompile(`https?://\S+/auth/verify\?token=\S+`) |
| +// normalizeAddress puts address into the one form allowlist compares |
| +// and stores: trimmed of surrounding whitespace, lowercased. Shared |
| +// by resolveAllowlist (building the set) and allowlist.allows |
| +// (checking against it) so the two can never drift into comparing |
| +// differently-normalized forms of the same address. |
| +func normalizeAddress(address string) string { |
| + return strings.ToLower(strings.TrimSpace(address)) |
| +} |
| + |
| +// allowlist is Config.AllowedAddresses ($DOCS_ALLOWED_ADDRESSES), |
| +// parsed and normalized — see that field's own doc comment on Config |
| +// for what it gates and why it is wired into two different places. |
| +// |
| +// addrs == nil means "admit every address" — resolveAllowlist's |
| +// development-only default (the same shape devInstanceKey uses for |
| +// InstanceKey: spelled out here rather than left as an accidental |
| +// nil, and refused everywhere else). addrs != nil means "admit |
| +// nothing but exactly this set"; resolveAllowlist never returns a |
| +// non-nil-but-empty map outside development, since an empty parse |
| +// there is a startup refusal, not a value this type carries. |
| +type allowlist struct { |
| + addrs map[string]struct{} |
| +} |
| + |
| +// allows reports whether address may sign in. |
| +func (a allowlist) allows(address string) bool { |
| + if a.addrs == nil { |
| + return true |
| + } |
| + _, ok := a.addrs[normalizeAddress(address)] |
| + return ok |
| +} |
| + |
| +// resolveAllowlist applies Config.AllowedAddresses' own fail-closed |
| +// rule (see its doc comment on Config): a non-empty, comma-separated |
| +// list always wins, parsed and normalized; an empty one is fine only |
| +// in development, where it defaults to "admit everyone" so TestSignIn |
| +// and scripts/run-dev.sh need no setup of their own; anywhere else it |
| +// is a startup refusal. |
| +// |
| +// 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 |
| +// 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 |
| +// a fresh, not-yet-configured deployment starts in. A startup refusal |
| +// is loud: the process does not come up, whoever is deploying it sees |
| +// that immediately, and cannot ship it half-configured by accident. |
| +// The honest cost is that a misconfigured instance is down rather |
| +// than merely closed — weighed and accepted here because "down" gets |
| +// fixed the same day, while "quietly admits nobody" on a public |
| +// hostname could sit unnoticed indefinitely. This is the same |
| +// tradeoff Origin, InstanceKey and Mailer already make in this file |
| +// and app.go: a misconfigured instance refuses to start, never |
| +// quietly does the wrong thing. |
| +func resolveAllowlist(cfg Config) (allowlist, error) { |
| + addrs := make(map[string]struct{}) |
| + for _, addr := range strings.Split(cfg.AllowedAddresses, ",") { |
| + addr = normalizeAddress(addr) |
| + if addr == "" { |
| + continue |
| + } |
| + addrs[addr] = struct{}{} |
| + } |
| + if len(addrs) > 0 { |
| + return allowlist{addrs: addrs}, nil |
| + } |
| + if cfg.Env == "development" { |
| + return allowlist{}, nil // addrs stays nil: admit everyone |
| + } |
| + 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". |
| +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() |
| + if list.allows(r.FormValue("address")) { |
| + next(w, r) |
| + return |
| + } |
| + http.Redirect(w, r, signinPath+"?sent=1", http.StatusSeeOther) |
| + } |
| +} |
| + |
| // newAuthPlugin wires rastrillo/auth per its SKILL.md contract: one |
| // *Auth built at boot (New — SKILL.md's own warning against building |
| // a Flow per request, since the rate limiter's budget lives on the |
| // value), Origin as both the emailed link's base and the cookie |
| // decision, InstanceKey and Mailer already resolved and fail-closed |
| -// by resolveInstanceKey/buildMailer above. Authorize is left nil — |
| -// every verified address is admitted; Task 2 layers ownership on this |
| -// hook, not this task. SecondFactor is left nil on purpose: no |
| -// passkey step-up, ever, for the reason this file's own package |
| -// comment gives. |
| -func newAuthPlugin(cfg Config, writer *sql.DB, instanceKey string, mailer mail.Sender, logger *slog.Logger) (*rauth.Auth, error) { |
| +// by resolveInstanceKey/buildMailer above. Authorize is list.allows — |
| +// 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. |
| +func newAuthPlugin(cfg Config, writer *sql.DB, instanceKey string, mailer mail.Sender, list allowlist, logger *slog.Logger) (*rauth.Auth, error) { |
| return rauth.New(rauth.Config{ |
| DB: writer, |
| Origin: cfg.Origin, |
| InstanceKey: instanceKey, |
| Mailer: mailer, |
| + Authorize: list.allows, |
| Logger: logger, |
| }) |
| } |
diff --git a/internal/docs/auth_allowlist_test.go b/internal/docs/auth_allowlist_test.go| new file mode 100644 |
| index 0000000..215e535 |
| --- /dev/null |
| +++ b/internal/docs/auth_allowlist_test.go |
| @@ -0,0 +1,276 @@ |
| +package docs |
| + |
| +import ( |
| + "bytes" |
| + "context" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "path/filepath" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// loggingMailerStub is a minimal mail.Sender good enough to satisfy |
| +// Config.Mailer's own "non-nil outside development" fail-closed |
| +// check (buildMailer, auth.go), so |
| +// TestNewRefusesMissingAllowlistOutsideDevelopment reaches the |
| +// allowlist check it actually means to pin rather than failing |
| +// earlier on an unrelated one. It never actually sends anything — |
| +// nothing in that test signs in. |
| +type loggingMailerStub struct{} |
| + |
| +func (loggingMailerStub) Send(ctx context.Context, to, subject, body string) error { return nil } |
| + |
| +// newAllowlistedTestApp builds a development App exactly like |
| +// NewForTest (testing.go), except with AllowedAddresses set |
| +// explicitly. NewForTest itself cannot exercise the allowlist at all: |
| +// an empty AllowedAddresses in development means "admit everyone" |
| +// (resolveAllowlist's own doc comment, auth.go) — precisely so |
| +// TestSignIn and every other test built on NewForTest need no |
| +// allowlist setup of their own. These tests are the ones that |
| +// actually need a non-default allowlist configured, so they build |
| +// their own App via New directly, package-internal (like |
| +// origin_test.go and render_test.go) so they can also reach |
| +// app.mailRecorder — unexported, and deliberately not something |
| +// testing.go exposes outside this package: nothing outside auth.go's |
| +// own tests should need to know whether mail was sent versus merely |
| +// that a link came back, and TestSignIn already covers the latter. |
| +func newAllowlistedTestApp(t *testing.T, allowed string) *App { |
| + t.Helper() |
| + app, err := New(Config{ |
| + DBPath: filepath.Join(t.TempDir(), "docs.db"), |
| + Env: "development", |
| + AllowedAddresses: allowed, |
| + }) |
| + if err != nil { |
| + t.Fatalf("New: %v", err) |
| + } |
| + t.Cleanup(func() { |
| + if cerr := app.Close(); cerr != nil { |
| + t.Errorf("App.Close: %v", cerr) |
| + } |
| + }) |
| + return app |
| +} |
| + |
| +// postSignin POSTs address to /signin as a same-origin browser form |
| +// would, with force=1 — the same escape hatch TestSignIn (testing.go) |
| +// 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). |
| +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())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + req.Header.Set("Sec-Fetch-Site", "same-origin") |
| + rec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(rec, req) |
| + return rec |
| +} |
| + |
| +// lastMailBody reads back app.mailRecorder's most recently recorded |
| +// send, "" if nothing has been sent yet (or since the caller last |
| +// cleared it) — the direct way to observe whether Begin actually ran, |
| +// as opposed to TestSignIn's own success-shaped check (a link came |
| +// back), which a test proving the ABSENCE of a send cannot use: there |
| +// is no link to find in an empty body. |
| +func lastMailBody(app *App) string { |
| + app.mailRecorder.mu.Lock() |
| + defer app.mailRecorder.mu.Unlock() |
| + return app.mailRecorder.lastBody |
| +} |
| + |
| +// TestNonPermittedAddressSendsNoMail is this task's whole point, |
| +// stated as literally as possible: the defect (Config.AllowedAddresses' |
| +// own doc comment on Config has the full story — credited to the |
| +// CARLOS Sheets session's finding on their own deployed instance) was |
| +// never that a non-permitted address could sign in; rauth.Config. |
| +// Authorize already stopped that, once wired. It was that Begin sends |
| +// mail to any address at all, regardless of Authorize, because |
| +// Authorize is only ever consulted afterward, at the callback. This |
| +// 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 |
| +// file's own commit message for how that was confirmed live, not |
| +// assumed. |
| +func TestNonPermittedAddressSendsNoMail(t *testing.T) { |
| + app := newAllowlistedTestApp(t, "permitted@example.com") |
| + |
| + rec := postSignin(app, "stranger@evil.example") |
| + |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("POST /signin for a non-permitted address: status %d, want %d", rec.Code, http.StatusSeeOther) |
| + } |
| + if loc := rec.Header().Get("Location"); !strings.Contains(loc, "sent=1") { |
| + t.Fatalf("POST /signin for a non-permitted address redirected to %q, want a %q outcome — the refusal must be invisible, not merely non-fatal", loc, "?sent=1") |
| + } |
| + if body := lastMailBody(app); body != "" { |
| + t.Fatalf("POST /signin for a non-permitted address sent mail (recorded body: %q) — Begin must never run for an address the allowlist refuses", body) |
| + } |
| +} |
| + |
| +// TestSigninResponseIdenticalForNonPermittedAddress is fix |
| +// requirement 3, checked the way its own reasoning demands: not |
| +// "both responses are a 303" but the actual bytes — status, Location |
| +// and body — compared directly. Any observable difference here would |
| +// turn POST /signin into a membership oracle: an attacker who can |
| +// tell "sent" from "not sent" has learned whether a given address is |
| +// on this instance's allowlist, which is exactly the kind of leak an |
| +// admission check alone would not create (it would 403 visibly) but |
| +// a badly-built send-time gate could. |
| +func TestSigninResponseIdenticalForNonPermittedAddress(t *testing.T) { |
| + app := newAllowlistedTestApp(t, "permitted@example.com") |
| + |
| + permitted := postSignin(app, "permitted@example.com") |
| + nonPermitted := postSignin(app, "stranger@evil.example") |
| + |
| + if permitted.Code != http.StatusSeeOther { |
| + t.Fatalf("POST /signin for the permitted address: status %d, want %d", permitted.Code, http.StatusSeeOther) |
| + } |
| + if got, want := nonPermitted.Code, permitted.Code; got != want { |
| + t.Errorf("status differs: non-permitted=%d permitted=%d", got, want) |
| + } |
| + if got, want := nonPermitted.Header().Get("Location"), permitted.Header().Get("Location"); got != want { |
| + t.Errorf("Location differs: non-permitted=%q permitted=%q", got, want) |
| + } |
| + if got, want := nonPermitted.Header().Get("Content-Type"), permitted.Header().Get("Content-Type"); got != want { |
| + t.Errorf("Content-Type differs: non-permitted=%q permitted=%q", got, want) |
| + } |
| + if got, want := nonPermitted.Body.Bytes(), permitted.Body.Bytes(); !bytes.Equal(got, want) { |
| + t.Errorf("body differs: non-permitted=%q permitted=%q", got, want) |
| + } |
| +} |
| + |
| +// TestPermittedAddressStillSignsIn proves the fix does not break the |
| +// working path: a permitted address must still complete the full |
| +// magic-link ceremony end to end, mirroring |
| +// TestMagicLinkThenSignedInRouteWorks (auth_test.go) but against an |
| +// App with a real, non-empty allowlist instead of NewForTest's |
| +// admit-everyone default. |
| +func TestPermittedAddressStillSignsIn(t *testing.T) { |
| + app := newAllowlistedTestApp(t, "permitted@example.com") |
| + |
| + link, err := app.TestSignIn(t, "permitted@example.com") |
| + if err != nil { |
| + t.Fatalf("TestSignIn: %v", err) |
| + } |
| + |
| + verifyReq := httptest.NewRequest(http.MethodGet, link, nil) |
| + verifyRec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(verifyRec, verifyReq) |
| + cookies := verifyRec.Result().Cookies() |
| + if len(cookies) == 0 { |
| + t.Fatal("following the magic link set no session cookie") |
| + } |
| + |
| + req := httptest.NewRequest(http.MethodGet, "/", nil) |
| + for _, c := range cookies { |
| + req.AddCookie(c) |
| + } |
| + rec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(rec, req) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET / signed in as a permitted address: status %d, want 200", rec.Code) |
| + } |
| +} |
| + |
| +// TestAllowlistIsCaseAndWhitespaceInsensitive is fix requirement 1's |
| +// own explicit demand: real config files have spaces after their |
| +// commas, and people type capitals — $DOCS_ALLOWED_ADDRESSES= |
| +// "Permitted@Example.com" must still admit |
| +// "permitted@example.com" and vice versa, in both directions |
| +// (the configured entry's own case/whitespace, and the submitted |
| +// address's). |
| +func TestAllowlistIsCaseAndWhitespaceInsensitive(t *testing.T) { |
| + app := newAllowlistedTestApp(t, " Permitted@Example.com , other@example.com") |
| + |
| + for _, variant := range []string{ |
| + "permitted@example.com", |
| + "PERMITTED@EXAMPLE.COM", |
| + " permitted@example.com ", |
| + "Permitted@Example.com", |
| + } { |
| + if _, err := app.TestSignIn(t, variant); err != nil { |
| + t.Errorf("TestSignIn(%q): %v, want the allowlisted address to be admitted regardless of case or surrounding whitespace", variant, err) |
| + } |
| + } |
| +} |
| + |
| +// TestResolveAllowlistParsesCommaSeparatedList pins |
| +// resolveAllowlist's own parsing directly: comma-separated, each |
| +// entry trimmed and lowercased, blank entries (a trailing comma, or |
| +// stray whitespace between two commas) dropped rather than becoming a |
| +// spurious "" that could never match a real address anyway but would |
| +// otherwise clutter the set. |
| +func TestResolveAllowlistParsesCommaSeparatedList(t *testing.T) { |
| + list, err := resolveAllowlist(Config{ |
| + Env: "production", |
| + AllowedAddresses: " Alice@Example.com, bob@example.com ,, carol@example.com ", |
| + }) |
| + if err != nil { |
| + t.Fatalf("resolveAllowlist: %v", err) |
| + } |
| + for _, addr := range []string{"alice@example.com", "ALICE@EXAMPLE.COM", "bob@example.com", "carol@example.com"} { |
| + if !list.allows(addr) { |
| + t.Errorf("list.allows(%q) = false, want true", addr) |
| + } |
| + } |
| + if list.allows("mallory@example.com") { |
| + t.Error("list.allows(\"mallory@example.com\") = true, want false — not on the configured list") |
| + } |
| +} |
| + |
| +// TestResolveAllowlistEmptyOutsideDevelopmentRefuses is fix |
| +// requirement 4's decision, pinned directly: an unset |
| +// $DOCS_ALLOWED_ADDRESSES must never mean "allow everyone" outside |
| +// development. resolveAllowlist's own doc comment has the full |
| +// reasoning for refusing at startup rather than the alternative |
| +// (silently admitting nobody) — this test only pins the outcome: |
| +// New must refuse to start, the same fail-closed shape |
| +// TestNewRefusesMissingInstanceKeyOutsideDevelopment and |
| +// TestNewRefusesMissingMailOutsideDevelopment (auth_test.go) already |
| +// pin for InstanceKey and Mailer. |
| +func TestResolveAllowlistEmptyOutsideDevelopmentRefuses(t *testing.T) { |
| + for _, env := range []string{"production", "staging", ""} { |
| + if _, err := resolveAllowlist(Config{Env: env}); err == nil { |
| + t.Errorf("resolveAllowlist with Env %q and no AllowedAddresses: err = nil, want a refusal", env) |
| + } |
| + } |
| +} |
| + |
| +// TestResolveAllowlistEmptyInDevelopmentAdmitsEveryone pins the other |
| +// half of the same decision: development's own convenience default |
| +// (TestSignIn, scripts/run-dev.sh) needs no allowlist configured at |
| +// all, exactly as it needed no InstanceKey or Mailer configured |
| +// before this task. |
| +func TestResolveAllowlistEmptyInDevelopmentAdmitsEveryone(t *testing.T) { |
| + list, err := resolveAllowlist(Config{Env: "development"}) |
| + if err != nil { |
| + t.Fatalf("resolveAllowlist: %v", err) |
| + } |
| + if !list.allows("anyone@example.com") { |
| + t.Error("an empty AllowedAddresses in development must admit everyone") |
| + } |
| +} |
| + |
| +// TestNewRefusesMissingAllowlistOutsideDevelopment is the New-level |
| +// version of TestResolveAllowlistEmptyOutsideDevelopmentRefuses, |
| +// exercised the way a real misconfigured deployment would actually |
| +// hit it — through New itself, alongside its own InstanceKey/Mailer |
| +// siblings in auth_test.go. |
| +func TestNewRefusesMissingAllowlistOutsideDevelopment(t *testing.T) { |
| + _, err := New(Config{ |
| + Env: "production", |
| + Origin: "https://docs.example.com", |
| + InstanceKey: "some-real-key", |
| + Mailer: loggingMailerStub{}, |
| + }) |
| + if err == nil { |
| + t.Fatal("New must refuse to start outside development with no allowlist configured") |
| + } |
| +} |