amadan

oficina / slides Public

fix: sign-in review round 1

Closes review round 1 findings on Task 1 (magic-link sign-in):

Important 1 (gateBegin no-membership-oracle guarantee did not hold
under rate limiting): wrap the ResponseWriter (sameAnswer) and rewrite
any ?err=* Location rauth's Begin sets back to ?sent=1, so a permitted
address cannot be told apart from a denied one once its per-address
budget is spent. Doc comment now states the guarantee precisely and
what it costs. Covered by
TestSigninAnswerIsIdenticalOverTheRateLimitBudget (7 POSTs of an
allowed address, each byte-identical to a denied address's response).

Important 2 (app.cfg stored the raw Config, not the resolved one):
config.go's new resolve() returns the fully-resolved Config (Origin,
InstanceKey, Mailer, Env, Logger) alongside the parsed allowlist;
app.go's newApp assigns it back before constructing *app. Covered by
TestDevelopmentStoresTheResolvedConfig and
TestSuppliedConfigSurvivesResolution.

Important 3 (ensureUser upserted on every authenticated request):
read first (Take), fall through to upsert-then-read only on
ErrRecordNotFound. Covered by TestEnsureUserWritesOnceOnly, which
counts INSERT statements across three calls and asserts exactly one.

Ruling (SMTP env vars): cmd/slides/main.go's smtpSetting/smtpSecret
read $SLIDES_SMTP_* first and fall back to $CARLOS_SMTP_* per
variable (not per set), since `carlos email enable` delivers
credentials under the CARLOS_SMTP_* prefix; documented in both the
main.go doc comment and Config.Mailer's.

Minor 4: added postLegacy and
TestSigninAcceptsALegacyOriginOnlyPost, which omits Sec-Fetch-Site so
the Origin header (otherwise dead behind Sec-Fetch-Site in every other
test) is actually exercised against the app's resolved origin.

Minor 5: newAuthPlugin now passes SigninPath explicitly rather than
relying on rauth's matching default.

Minor 6: index's failure path now calls ErrorPage (ref logged) rather
than http.Error, matching the app's own error shell.

Minor 7: extractEnvFlag now refuses a valueless trailing -env instead
of silently reading it as "".

Verified: go build ./..., go vet ./..., gofmt -l . clean; go test
./internal/slidestest/ -run
'Signin|SignedOut|Verify|Development|User|Oracle' and go test
./internal/slides/... both pass in full. `make ci`'s vet and test
targets pass; its migration-check target fails
("expected exactly one app package under ./internal, found [core
slides]"), confirmed pre-existing on this branch's HEAD (b745b2f, Task
2's cherry-picked internal/core) before any of this round's changes —
out of scope here (core/, internal/core/ are off limits this round).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01563XGCgxcSZToEXwuTBsKE
claude pushed by paul@keymail.dev 8314eb565a11475572e32a889138f7d1bf89b425 parent b745b2f
9 files changed, +691 −103
  • cmd/slides/main.go +70 −18
  • internal/slides/app.go +50 −52
  • internal/slides/auth.go +122 −27
  • internal/slides/auth_test.go +137 −0
  • internal/slides/config.go +54 −0
  • internal/slides/config_test.go +140 −0
  • internal/slides/handlers.go +17 −6
  • internal/slidestest/harness_test.go +19 −0
  • internal/slidestest/signin_test.go +82 −0

Intent

Humanclaude~paul@keymail.dev

continue

full ledger: worktree-slice-1

diff --git a/cmd/slides/main.go b/cmd/slides/main.go
index 945173b..1e6db24 100644
--- a/cmd/slides/main.go
+++ b/cmd/slides/main.go
@@ -3,6 +3,7 @@
package main
import (
+ "fmt"
"log/slog"
"os"
"path/filepath"
@@ -24,15 +25,23 @@ import (
// with its own flag.FlagSet, which knows only -socket/-addr/-db and
// errors on anything else, so a flag this binary adds on top must be
// stripped out first.
-func extractEnvFlag(args []string) (env string, rest []string) {
+//
+// A trailing "-env" with nothing after it is an error, not an empty
+// value. Silently reading as "" would make chooseEnv fall through to
+// $SLIDES_ENV and then to production — so `slides -env` (a typo for
+// `-env development`, or a truncated argv) would boot a production app
+// while its operator believed they had asked for a development one.
+// Refusing is the only answer that cannot be misread.
+func extractEnvFlag(args []string) (env string, rest []string, err error) {
for i := 0; i < len(args); i++ {
a := args[i]
switch {
case a == "-env" || a == "--env":
- if i+1 < len(args) {
- env = args[i+1]
- i++
+ if i+1 >= len(args) {
+ return "", nil, fmt.Errorf("%s needs a value, for example: %s development", a, a)
}
+ env = args[i+1]
+ i++
continue
case strings.HasPrefix(a, "-env="):
env = strings.TrimPrefix(a, "-env=")
@@ -43,7 +52,7 @@ func extractEnvFlag(args []string) (env string, rest []string) {
}
rest = append(rest, a)
}
- return env, rest
+ return env, rest, nil
}
// chooseEnv decides slides.Config.Env from -env's extracted value
@@ -74,21 +83,60 @@ func chooseEnv(flagEnv, osEnv string) string {
return slides.EnvProduction
}
-// newMailer builds the magic-link sender from $SLIDES_SMTP_* — the
-// same vars rastrillo/mail.FromEnv reads (HOST, PORT, USER, PASS,
-// FROM). It is built only when the two values with no sane default are
-// both set: FromEnv itself falls back to a warning-and-log sender when
-// they are missing, which is the wrong answer outside development, so
-// that decision is left to slides.New. A nil return here is what lets
-// New refuse to start rather than quietly logging live credentials.
-func newMailer(logger *slog.Logger) mail.Sender {
- if strings.TrimSpace(os.Getenv("SLIDES_SMTP_HOST")) == "" {
- return nil
+// smtpSetting reads one SMTP setting, this app's own variable first and
+// the platform's as the fallback: $SLIDES_SMTP_HOST, else
+// $CARLOS_SMTP_HOST.
+//
+// Per variable, not per set. `carlos email enable` delivers
+// CARLOS_SMTP_{HOST,PORT,USER,PASS,FROM}, and the common case is
+// wanting every one of them except FROM — the address this app sends
+// as — which $SLIDES_SMTP_FROM then overrides on its own. An
+// all-or-nothing choice between the two prefixes would make that
+// ordinary case impossible to express without restating the platform's
+// own credentials in this app's variables, which is exactly the kind of
+// copy that goes stale after a credential rotation.
+//
+// Trimmed, because these five arrive from a deploy-time env block or a
+// secret store, where a trailing newline is a routine accident and a
+// leading space in a hostname is never meant.
+func smtpSetting(name string) string {
+ if v := strings.TrimSpace(os.Getenv("SLIDES_SMTP_" + name)); v != "" {
+ return v
+ }
+ return strings.TrimSpace(os.Getenv("CARLOS_SMTP_" + name))
+}
+
+// smtpSecret is smtpSetting for the two values that are credentials:
+// same per-variable fallback, no trimming. rastrillo/mail.FromEnv does
+// not trim these either, and for the same reason — a password's
+// surrounding whitespace may be part of the password, and silently
+// eating it would fail authentication with no way to see why.
+func smtpSecret(name string) string {
+ if v := os.Getenv("SLIDES_SMTP_" + name); v != "" {
+ return v
}
- if strings.TrimSpace(os.Getenv("SLIDES_SMTP_FROM")) == "" {
+ return os.Getenv("CARLOS_SMTP_" + name)
+}
+
+// newMailer builds the magic-link sender from the SMTP settings above.
+//
+// mail.SMTP directly rather than mail.FromEnv, because FromEnv reads
+// one prefix and this needs the per-variable SLIDES-then-CARLOS
+// fallback. It also falls back to a warning-and-log sender when HOST or
+// FROM is missing, which is the wrong answer outside development: a nil
+// return here is what lets slides.New refuse to start rather than
+// quietly logging live credentials in a real deployment's log.
+//
+// HOST and FROM are the two values with no sane default, so either one
+// missing means this instance has no relay. PORT defaults to 587 inside
+// mail.SMTP; USER empty means anonymous submission.
+func newMailer(logger *slog.Logger) mail.Sender {
+ host, from := smtpSetting("HOST"), smtpSetting("FROM")
+ if host == "" || from == "" {
return nil
}
- return mail.FromEnv("SLIDES", logger)
+ logger.Info("slides: SMTP configured", "host", host, "from", from)
+ return mail.SMTP(host, smtpSetting("PORT"), from, smtpSecret("USER"), smtpSecret("PASS"))
}
// newBlobs binds the object store: the platform's bucket when it
@@ -126,7 +174,11 @@ func main() {
// -env is stripped before Resolve sees argv (extractEnvFlag), and
// it is the only thing that can ever select "development"
// (chooseEnv).
- envFlag, rest := extractEnvFlag(os.Args[1:])
+ envFlag, rest, err := extractEnvFlag(os.Args[1:])
+ if err != nil {
+ logger.Error("read flags", "err", err)
+ os.Exit(1)
+ }
os.Args = append(os.Args[:1], rest...)
env := chooseEnv(envFlag, os.Getenv("SLIDES_ENV"))
diff --git a/internal/slides/app.go b/internal/slides/app.go
index 49cec6c..4f25d94 100644
--- a/internal/slides/app.go
+++ b/internal/slides/app.go
@@ -2,11 +2,11 @@ package slides
import (
"context"
- "log/slog"
"net/http"
"github.com/go-chi/chi/v5"
+ rauth "amadan.net/rastrillo/rastrillo/auth"
"amadan.net/rastrillo/rastrillo/csrf"
"amadan.net/rastrillo/rastrillo/db"
"amadan.net/rastrillo/rastrillo/migrate"
@@ -20,69 +20,24 @@ import (
// logger arguments the scaffold took have grown into Config (config.go),
// because sign-in needs four more settings and each of them is a
// startup refusal when it is missing outside development. Every one of
-// those refusals happens here, before the database is touched, so a
+// those refusals happens before the database is touched, so a
// misconfigured instance never reaches the point of serving a request.
//
// The database handle is the caller's (cmd/slides/main.go's db.Open),
// not one this opens: SKILL.md §1's Resolve/Serve split depends on
// exactly one connection to the file.
func New(d *db.DB, cfg Config) (*http.ServeMux, error) {
- logger := cfg.Logger
- if logger == nil {
- logger = slog.Default()
- }
- if cfg.Env == "" {
- cfg.Env = EnvProduction
- }
-
- // Fail closed, loudly, and before anything opens: each of these
- // four decides whether sign-in is safe, and each may default only
- // when Env is exactly "development" (config.go's package comment).
- origin, err := resolveOrigin(cfg)
- if err != nil {
- return nil, err
- }
- instanceKey, err := resolveInstanceKey(cfg)
- if err != nil {
- return nil, err
- }
- mailer, err := resolveMailer(cfg, logger)
- if err != nil {
- return nil, err
- }
- list, err := resolveAllowlist(cfg)
- if err != nil {
- return nil, err
- }
-
- // Apply BootSchema, not Schema: BootSchema is everything this
- // app needs at boot (its own migrations plus sessions' and auth's),
- // while Schema (migrations.go) stays just this app's own so the
- // generator and its check never see a subsystem's tables and try
- // to drop them.
- migrated, err := migrate.Apply(context.Background(), d, BootSchema)
+ a, authPlugin, err := newApp(d, cfg)
if err != nil {
return nil, err
}
- // One line per boot. Result exists so this can be logged, and
- // until something does, a fleet-wide adoption — every database
- // stamped, no DDL run, exactly once, never again — produces no
- // output at all, which is the last event you want to be silent.
- logger.Info("migrate", "applied", migrated.Applied,
- "skipped", migrated.Skipped, "adopted", migrated.Adopted)
-
- authPlugin, err := newAuthPlugin(origin, instanceKey, d.Writer(), mailer, list, logger)
- if err != nil {
- return nil, err
- }
-
- a := &app{db: d, logger: logger, cfg: cfg}
r := chi.NewRouter()
// App-wide, from day one: every state-changing route added below
// is born covered. Origin-checking, not tokens — nothing to mint
- // or forget in a form.
- r.Use(csrf.Protect(origin))
+ // or forget in a form. a.cfg.Origin, not the caller's cfg.Origin:
+ // what this protects has to be the origin the app resolved.
+ r.Use(csrf.Protect(a.cfg.Origin))
// Unauthenticated: the front door and rauth's four handlers. The
// sign-in page has to be reachable signed out or nobody could ever
@@ -91,7 +46,7 @@ func New(d *db.DB, cfg Config) (*http.ServeMux, error) {
// gateBegin, not authPlugin.Begin directly — see its own doc
// comment: the allowlist has to sit in FRONT of Begin, because
// Begin is what sends the mail.
- r.Post(signinPath, gateBegin(authPlugin.Begin, list))
+ r.Post(signinPath, gateBegin(authPlugin.Begin, a.allowed, a.logger))
r.Get("/auth/callback", authPlugin.Callback)
r.Get("/auth/verify", authPlugin.Verify)
r.Post("/signout", authPlugin.Signout)
@@ -113,3 +68,46 @@ func New(d *db.DB, cfg Config) (*http.ServeMux, error) {
mux.Handle("/", r)
return mux, nil
}
+
+// newApp is everything New does before the router: resolve the config
+// fail-closed, apply the schema, build the sign-in plugin.
+//
+// Separate from New so this package's own tests can assert on the
+// resolved *app — that a.cfg carries the origin the app is really
+// protecting rather than the empty string a development caller passed
+// in (config_test.go). New itself hands back only a mux, which cannot
+// answer that question.
+func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) {
+ // Fail closed, loudly, and before anything opens: Origin,
+ // InstanceKey, Mailer and AllowedAddresses each decide whether
+ // sign-in is safe, and each may default only when Env is exactly
+ // "development" (config.go's package comment).
+ cfg, list, err := resolve(cfg)
+ if err != nil {
+ return nil, nil, err
+ }
+ logger := cfg.Logger
+
+ // Apply BootSchema, not Schema: BootSchema is everything this
+ // app needs at boot (its own migrations plus sessions' and auth's),
+ // while Schema (migrations.go) stays just this app's own so the
+ // generator and its check never see a subsystem's tables and try
+ // to drop them.
+ migrated, err := migrate.Apply(context.Background(), d, BootSchema)
+ if err != nil {
+ return nil, nil, err
+ }
+ // One line per boot. Result exists so this can be logged, and
+ // until something does, a fleet-wide adoption — every database
+ // stamped, no DDL run, exactly once, never again — produces no
+ // output at all, which is the last event you want to be silent.
+ logger.Info("migrate", "applied", migrated.Applied,
+ "skipped", migrated.Skipped, "adopted", migrated.Adopted)
+
+ authPlugin, err := newAuthPlugin(cfg, d.Writer(), list)
+ if err != nil {
+ return nil, nil, err
+ }
+
+ return &app{db: d, logger: logger, cfg: cfg, allowed: list}, authPlugin, nil
+}
diff --git a/internal/slides/auth.go b/internal/slides/auth.go
index fc84364..d547802 100644
--- a/internal/slides/auth.go
+++ b/internal/slides/auth.go
@@ -71,7 +71,7 @@ func resolveMailer(cfg Config, logger *slog.Logger) (mail.Sender, error) {
return cfg.Mailer, nil
}
if !cfg.development() {
- return nil, fmt.Errorf("slides: mail is not configured (set $SLIDES_SMTP_HOST and $SLIDES_SMTP_FROM) and Env is %q, not %q — refusing to start: a sign-in that cannot send its link is not sign-in", cfg.Env, EnvDevelopment)
+ return nil, fmt.Errorf("slides: mail is not configured and Env is %q, not %q — refusing to start: a sign-in that cannot send its link is not sign-in. Set $SLIDES_SMTP_HOST and $SLIDES_SMTP_FROM, or run `carlos email enable` for this app and let $CARLOS_SMTP_HOST supply the relay", cfg.Env, EnvDevelopment)
}
// mail.Logged labels every send loudly itself — see its own doc
// comment. Never reached outside development.
@@ -133,39 +133,103 @@ func resolveAllowlist(cfg Config) (allowlist, error) {
// sending identity an open relay for any address a stranger types into
// the form.
//
-// A non-permitted address gets exactly the response the permitted path
-// sends: the same 303, to the same signinPath+"?sent=1". That is
-// deliberate. Any observable difference — a different status, a
-// different body, even a differently-cased query — would turn this
-// endpoint into a membership oracle, letting anyone learn which
-// addresses this instance admits by watching how its answers differ.
+// THE GUARANTEE: every answer this endpoint gives is byte-identical —
+// `303 Location: /signin?sent=1`, no body — whatever address was
+// submitted and whatever happened next. Any observable difference at
+// all would turn this into a membership oracle, letting anyone learn
+// which addresses this instance admits by watching how its answers
+// differ.
+//
+// Making the denied path match the happy path is only half of that, and
+// the half that is easy to get wrong is the other one: the PERMITTED
+// path has outcomes the denied path can never reach. `signin.Flow.Begin`
+// enforces a per-address budget (5 in a rolling 15 minutes) and a
+// per-IP one (20), and over either budget rauth redirects to
+// `?err=rate` instead — so six POSTs of one candidate address would
+// separate "on the list" (`?err=rate` on the sixth) from "not on it"
+// (`?sent=1` forever), and the whole guarantee with it. `?err=address`
+// and `?err=1` are the same shape of leak: only a permitted address can
+// reach either.
+//
+// So the permitted branch runs under sameAnswer, which rewrites every
+// one of those outcomes back to `?sent=1` on the way out. What that
+// costs is inline feedback: a person over the budget, or one whose
+// address rauth could not parse, is told a link is on its way when it
+// is not. On a closed allowlist that was already true — such an address
+// is almost always denied and short-circuited above — so the loss is
+// confined to development's admit-everyone list, where the log carries
+// the real outcome instead. That is the trade, made deliberately: a
+// recoverable "wait five minutes and try again" that nobody sees, in
+// exchange for an allowlist nobody can enumerate.
+//
+// The denied branch deliberately spends no budget of its own. That is
+// not an oversight — a budget consumed only by denied addresses would
+// itself be observable, since a permitted address's own budget would
+// then behave differently depending on what else had been tried.
+// Rate limiting the front door as a whole belongs at the edge, not
+// here, and the per-IP budget inside Begin already bounds what a
+// permitted address can spend.
//
// force=1 is set unconditionally on the permitted branch, not left to
// what the caller's form said. It selects rauth's plain-magic-link path
// over keymail classification, which this app never wants (see this
// file's package comment). Without it a permitted keymail-hosted
// address would redirect out to keymail while a non-permitted one got
-// ?sent=1 — the same oracle, one level narrower. It also saves the
-// classification network call outright.
+// ?sent=1 — the same oracle again. It also saves the classification
+// network call outright.
//
// r.ParseForm is documented idempotent, so reading the address here and
// again inside Begin costs nothing and cannot disagree with itself. A
// malformed body reads back as address == "", which is never on a
// non-empty allowlist — the same answer Begin would eventually reach,
// just reached before any mail could go out.
-func gateBegin(next http.HandlerFunc, list allowlist) http.HandlerFunc {
+func gateBegin(next http.HandlerFunc, list allowlist, logger *slog.Logger) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
_ = r.ParseForm()
if !list.allows(r.FormValue("address")) {
- http.Redirect(w, r, signinPath+"?sent=1", http.StatusSeeOther)
+ http.Redirect(w, r, sentLocation, http.StatusSeeOther)
return
}
if r.Form == nil {
r.Form = url.Values{}
}
r.Form.Set("force", "1")
- next(w, r)
+ next(&sameAnswer{ResponseWriter: w, logger: logger}, r)
+ }
+}
+
+// sentLocation is the one answer POST /signin ever gives — see
+// gateBegin's guarantee. Spelled once so the two paths that write it
+// cannot drift apart by a character.
+const sentLocation = signinPath + "?sent=1"
+
+// sameAnswer is the ResponseWriter gateBegin hands to Begin: it
+// rewrites any `?err=…` outcome back to sentLocation before the status
+// line goes out, so a permitted address cannot be told apart from a
+// denied one by its failure mode. See gateBegin's own doc comment for
+// what that buys and what it costs.
+//
+// It hooks WriteHeader rather than the Location header itself because
+// that is the last moment a header is still settable: http.Redirect
+// sets Location and then calls WriteHeader, so by intercepting the
+// second we see the first already written and can still change it. The
+// body is not a concern on this route — http.Redirect writes its little
+// HTML body only for GET and HEAD, and this handler is mounted on POST
+// alone.
+type sameAnswer struct {
+ http.ResponseWriter
+ logger *slog.Logger
+}
+
+func (s *sameAnswer) WriteHeader(status int) {
+ if loc := s.Header().Get("Location"); loc != "" && loc != sentLocation && strings.HasPrefix(loc, signinPath+"?err=") {
+ // Logged, not swallowed: the person who submitted the form is
+ // told nothing, so the operator has to be told everything.
+ s.logger.Warn("slides: sign-in refused; answering as if sent, to keep the allowlist unobservable",
+ "outcome", loc)
+ s.Header().Set("Location", sentLocation)
}
+ s.ResponseWriter.WriteHeader(status)
}
// newAuthPlugin wires rastrillo/auth per SKILL.md §5: exactly one *Auth
@@ -180,14 +244,25 @@ func gateBegin(next http.HandlerFunc, list allowlist) http.HandlerFunc {
// issued for a non-permitted address (mailed before the allowlist
// changed, say). SecondFactor stays nil, per this file's package
// comment.
-func newAuthPlugin(origin, instanceKey string, writer *sql.DB, mailer mail.Sender, list allowlist, logger *slog.Logger) (*rauth.Auth, error) {
+//
+// cfg must be the resolved Config (config.go's resolve), not a
+// caller's raw one: Origin, InstanceKey and Mailer are all required
+// here and all three may have been filled in by a development default.
+//
+// SigninPath is passed explicitly even though signinPath is already
+// rauth's own zero-value default. rauth redirects every outcome to its
+// copy of that literal and app.go mounts the page at ours; saying it
+// once, out loud, is what keeps the two from drifting silently if
+// either default ever changes.
+func newAuthPlugin(cfg Config, writer *sql.DB, list allowlist) (*rauth.Auth, error) {
return rauth.New(rauth.Config{
DB: writer,
- Origin: origin,
- InstanceKey: instanceKey,
- Mailer: mailer,
+ Origin: cfg.Origin,
+ InstanceKey: cfg.InstanceKey,
+ Mailer: cfg.Mailer,
Authorize: list.allows,
- Logger: logger,
+ SigninPath: signinPath,
+ Logger: cfg.Logger,
})
}
@@ -205,23 +280,43 @@ var presenceColours = []string{
// sessions.UserID answers (0, false) and scoping on it would file every
// deck under user_id = 0.
//
-// ON CONFLICT DO NOTHING on the unique Email index rather than a read
-// followed by a write: two requests arriving together for a person's
-// very first sign-in would otherwise both see no row and both insert
-// one, and only the unique index would stop them — as an error on the
-// loser's request rather than as a shared row.
+// Read first, and only write when the read came back empty. This runs
+// on every authenticated request — currentUser calls it before any
+// handler does its own work — and SQLite has one writer: an
+// unconditional upsert here would open a write transaction per page
+// view, serialising every reader in the app behind a statement whose
+// answer is almost always "this row already exists". The read goes to
+// the reader pool (db.Open routes per statement), so the steady state
+// costs one indexed SELECT and no lock at all.
+//
+// The write, when it happens, is still ON CONFLICT DO NOTHING on the
+// unique Email index rather than a bare INSERT: the read above is not a
+// lock, so two requests arriving together on a person's very first
+// sign-in can both miss and both try to insert. DO NOTHING makes the
+// loser a no-op instead of an error, and the read-back below gives both
+// of them the same row.
func ensureUser(ctx context.Context, g *gorm.DB, address string) (User, error) {
email := normalizeAddress(address)
if email == "" {
return User{}, errors.New("slides: cannot map an empty address to a user")
}
- u := User{Email: email, Colour: colourFor(email)}
- if err := g.WithContext(ctx).Clauses(clause.OnConflict{DoNothing: true}).Create(&u).Error; err != nil {
+
+ var found User
+ err := g.WithContext(ctx).Where("email = ?", email).Take(&found).Error
+ switch {
+ case err == nil:
+ return found, nil
+ case !errors.Is(err, gorm.ErrRecordNotFound):
+ return User{}, fmt.Errorf("slides: load user %q: %w", email, err)
+ }
+
+ // First sight of this address: create the row, then read it back
+ // rather than trusting the insert's own return — DO NOTHING leaves
+ // ID at zero when a concurrent request won the race.
+ created := User{Email: email, Colour: colourFor(email)}
+ if err := g.WithContext(ctx).Clauses(clause.OnConflict{DoNothing: true}).Create(&created).Error; err != nil {
return User{}, fmt.Errorf("slides: create user %q: %w", email, err)
}
- // DoNothing leaves ID at zero when the row already existed, so read
- // it back either way rather than trusting the insert's return.
- var found User
if err := g.WithContext(ctx).Where("email = ?", email).Take(&found).Error; err != nil {
return User{}, fmt.Errorf("slides: load user %q: %w", email, err)
}
diff --git a/internal/slides/auth_test.go b/internal/slides/auth_test.go
new file mode 100644
index 0000000..df1b1e5
--- /dev/null
+++ b/internal/slides/auth_test.go
@@ -0,0 +1,137 @@
+package slides
+
+import (
+ "context"
+ "testing"
+
+ "gorm.io/gorm"
+)
+
+// countCreates registers a GORM callback that counts INSERT statements
+// on the handle, returning a function that reads the count. It is the
+// only honest way to answer "did that request write?": a row's id and
+// CreatedAt look identical whether the second call inserted nothing or
+// ran an upsert that conflicted, and SQLite's AUTOINCREMENT sequence
+// does not move for a DO NOTHING either. Counting the statement itself
+// is the observation that distinguishes them.
+func countCreates(t *testing.T, g *gorm.DB) func() int {
+ t.Helper()
+ var n int
+ if err := g.Callback().Create().Before("gorm:create").Register("slides_test:count_creates", func(*gorm.DB) {
+ n++
+ }); err != nil {
+ t.Fatalf("register create callback: %v", err)
+ }
+ return func() int { return n }
+}
+
+// TestEnsureUserWritesOnceOnly is the regression test for a real
+// performance bug: ensureUser used to run INSERT ... ON CONFLICT DO
+// NOTHING followed by a SELECT on EVERY authenticated request, because
+// currentUser calls it before any handler does its own work. SQLite has
+// one writer, so that was a write transaction per page view,
+// serialising every reader in the app behind a statement whose answer
+// was almost always "this row already exists".
+//
+// It now reads first and writes only on a miss. Three calls, one
+// insert.
+func TestEnsureUserWritesOnceOnly(t *testing.T) {
+ d := testDB(t)
+ if _, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil {
+ t.Fatalf("newApp: %v", err)
+ }
+ creates := countCreates(t, d.G)
+
+ ctx := context.Background()
+ first, err := ensureUser(ctx, d.G, "paul@example.test")
+ if err != nil {
+ t.Fatalf("ensureUser (first): %v", err)
+ }
+ if first.ID == 0 {
+ t.Fatal("ensureUser (first) returned a row with no id")
+ }
+ if got := creates(); got != 1 {
+ t.Fatalf("first sign-in ran %d inserts, want 1", got)
+ }
+
+ for i := 2; i <= 3; i++ {
+ again, err := ensureUser(ctx, d.G, "paul@example.test")
+ if err != nil {
+ t.Fatalf("ensureUser (call %d): %v", i, err)
+ }
+ if again.ID != first.ID {
+ t.Errorf("call %d returned id %d, want the same row %d", i, again.ID, first.ID)
+ }
+ if !again.CreatedAt.Equal(first.CreatedAt) {
+ t.Errorf("call %d rewrote CreatedAt (%v, was %v)", i, again.CreatedAt, first.CreatedAt)
+ }
+ if got := creates(); got != 1 {
+ t.Fatalf("after call %d the app had run %d inserts, want the 1 from first sign-in", i, got)
+ }
+ }
+}
+
+// TestEnsureUserNormalizesTheAddress: the address is the identity, so
+// the same person typing capitals or a stray space is the same row —
+// and only one row, since a second would mean one person owning two
+// disjoint sets of decks depending on which one a request found.
+func TestEnsureUserNormalizesTheAddress(t *testing.T) {
+ d := testDB(t)
+ if _, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil {
+ t.Fatalf("newApp: %v", err)
+ }
+ ctx := context.Background()
+
+ first, err := ensureUser(ctx, d.G, "Paul@Example.Test")
+ if err != nil {
+ t.Fatalf("ensureUser: %v", err)
+ }
+ if first.Email != "paul@example.test" {
+ t.Errorf("stored Email = %q, want it normalized", first.Email)
+ }
+ again, err := ensureUser(ctx, d.G, " paul@example.test ")
+ if err != nil {
+ t.Fatalf("ensureUser (again): %v", err)
+ }
+ if again.ID != first.ID {
+ t.Errorf("a differently-spelled address got id %d, want the same row %d", again.ID, first.ID)
+ }
+
+ var n int64
+ if err := d.G.Model(&User{}).Count(&n).Error; err != nil {
+ t.Fatalf("count users: %v", err)
+ }
+ if n != 1 {
+ t.Errorf("users table has %d rows, want 1", n)
+ }
+}
+
+// TestEnsureUserRefusesAnEmptyAddress: an empty Subject would otherwise
+// create a row owning decks that nobody could ever sign in as.
+func TestEnsureUserRefusesAnEmptyAddress(t *testing.T) {
+ d := testDB(t)
+ if _, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}); err != nil {
+ t.Fatalf("newApp: %v", err)
+ }
+ if _, err := ensureUser(context.Background(), d.G, " "); err == nil {
+ t.Fatal("ensureUser accepted a blank address; want an error")
+ }
+}
+
+// TestColourForIsStable: a presence colour is only worth recognising if
+// it is the same colour tomorrow, and on every instance.
+func TestColourForIsStable(t *testing.T) {
+ first := colourFor("paul@example.test")
+ if first != colourFor("paul@example.test") {
+ t.Error("colourFor is not deterministic")
+ }
+ var found bool
+ for _, c := range presenceColours {
+ if c == first {
+ found = true
+ }
+ }
+ if !found {
+ t.Errorf("colourFor returned %q, which is not in the palette", first)
+ }
+}
diff --git a/internal/slides/config.go b/internal/slides/config.go
index 32f003a..b71e399 100644
--- a/internal/slides/config.go
+++ b/internal/slides/config.go
@@ -76,6 +76,11 @@ type Config struct {
// that cannot send its link is not sign-in, and quietly logging a
// live credential in a real deployment's log is the mistake that
// fallback exists to flag, not to absorb.
+ //
+ // cmd/slides/main.go builds it from $SLIDES_SMTP_{HOST,PORT,USER,
+ // PASS,FROM}, falling back per variable to the $CARLOS_SMTP_*
+ // equivalent that `carlos email enable` delivers — see smtpSetting
+ // there for why the fallback is per variable rather than per set.
Mailer mail.Sender
// AllowedAddresses is the only set of addresses this instance ever
@@ -115,6 +120,55 @@ type Config struct {
// development reports whether cfg is in the one Env that may default.
func (cfg Config) development() bool { return cfg.Env == EnvDevelopment }
+// resolve turns the config a caller supplied into the config the app
+// actually runs on: Env, Logger, Origin, InstanceKey and Mailer each
+// either filled in or refused, plus the parsed allowlist. Every
+// fail-closed rule in this file is applied here, once, before anything
+// opens.
+//
+// It returns a Config rather than five values because what New stores
+// on the app must be the RESOLVED one. Storing the caller's raw Config
+// was a real bug: in development a.cfg.Origin was "" while the app was
+// protecting http://localhost:8091, and a.cfg.Mailer was nil while mail
+// was going to the log — so any later code reading its own config would
+// have read a description of the app that was not true.
+func resolve(cfg Config) (Config, allowlist, error) {
+ if cfg.Env == "" {
+ cfg.Env = EnvProduction
+ }
+ if cfg.Logger == nil {
+ cfg.Logger = slog.Default()
+ }
+
+ origin, err := resolveOrigin(cfg)
+ if err != nil {
+ return Config{}, allowlist{}, err
+ }
+ cfg.Origin = origin
+
+ instanceKey, err := resolveInstanceKey(cfg)
+ if err != nil {
+ return Config{}, allowlist{}, err
+ }
+ cfg.InstanceKey = instanceKey
+
+ mailer, err := resolveMailer(cfg, cfg.Logger)
+ if err != nil {
+ return Config{}, allowlist{}, err
+ }
+ cfg.Mailer = mailer
+
+ // AllowedAddresses is deliberately NOT rewritten onto cfg: the
+ // parsed, normalized form is the allowlist type, and leaving the
+ // raw slice in place keeps one obvious answer to "what was
+ // configured" beside the derived answer to "what is admitted".
+ list, err := resolveAllowlist(cfg)
+ if err != nil {
+ return Config{}, allowlist{}, err
+ }
+ return cfg, list, nil
+}
+
// resolveOrigin applies Config.Origin's own rule: a non-empty value is
// trimmed and validated, an empty one falls back to DefaultOrigin in
// development and is refused anywhere else.
diff --git a/internal/slides/config_test.go b/internal/slides/config_test.go
new file mode 100644
index 0000000..17060bc
--- /dev/null
+++ b/internal/slides/config_test.go
@@ -0,0 +1,140 @@
+package slides
+
+import (
+ "io"
+ "log/slog"
+ "path/filepath"
+ "testing"
+
+ "amadan.net/rastrillo/rastrillo/db"
+ "amadan.net/rastrillo/rastrillo/mail"
+)
+
+// quietLogger keeps the suite's output pristine: newApp logs one
+// migrate line per call, which is right in a real boot and noise here.
+func quietLogger() *slog.Logger {
+ return slog.New(slog.NewTextHandler(io.Discard, nil))
+}
+
+func testDB(t *testing.T) *db.DB {
+ t.Helper()
+ d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), quietLogger())
+ if err != nil {
+ t.Fatalf("db.Open: %v", err)
+ }
+ t.Cleanup(func() { d.Close() })
+ return d
+}
+
+// TestDevelopmentStoresTheResolvedConfig is the regression test for a
+// real bug: New resolved Origin/InstanceKey/Mailer into locals, handed
+// those to the auth plugin, and then stored the caller's RAW Config on
+// the app. So in development a.cfg.Origin was "" while the app was
+// protecting http://localhost:8091, and a.cfg.Mailer was nil while
+// mail was going to the log — the app's own description of itself was
+// false, and the first later task to read it would have been misled.
+//
+// Package-internal because a.cfg is unexported and this is precisely a
+// question about what New stored, which the *http.ServeMux New returns
+// cannot answer.
+func TestDevelopmentStoresTheResolvedConfig(t *testing.T) {
+ a, _, err := newApp(testDB(t), Config{Env: EnvDevelopment, Logger: quietLogger()})
+ if err != nil {
+ t.Fatalf("newApp: %v", err)
+ }
+ if a.cfg.Origin != DefaultOrigin {
+ t.Errorf("a.cfg.Origin = %q, want the resolved %q", a.cfg.Origin, DefaultOrigin)
+ }
+ if a.cfg.InstanceKey != devInstanceKey {
+ t.Errorf("a.cfg.InstanceKey = %q, want the resolved %q", a.cfg.InstanceKey, devInstanceKey)
+ }
+ if a.cfg.Mailer == nil {
+ t.Error("a.cfg.Mailer is nil; want the resolved development sender")
+ } else if mail.IsConfigured(a.cfg.Mailer) {
+ t.Error("a.cfg.Mailer delivers for real in development; want mail.Logged")
+ }
+ if a.cfg.Env != EnvDevelopment {
+ t.Errorf("a.cfg.Env = %q, want %q", a.cfg.Env, EnvDevelopment)
+ }
+ if a.cfg.Logger == nil {
+ t.Error("a.cfg.Logger is nil; resolve must never leave it unset")
+ }
+ // The allowlist that actually admits, beside the raw list that was
+ // configured: development's empty list means "admit everyone".
+ if !a.allowed.allows("anyone@example.test") {
+ t.Error("development allowlist refused an address; want admit-everyone")
+ }
+}
+
+// TestSuppliedConfigSurvivesResolution is the other direction: what a
+// caller did set must come back unchanged, trimmed but not replaced.
+func TestSuppliedConfigSurvivesResolution(t *testing.T) {
+ given := Config{
+ Origin: " https://slides.example.test ",
+ Env: EnvProduction,
+ InstanceKey: "a-real-key",
+ Mailer: mail.SMTP("smtp.example.test", "", "slides@example.test", "", ""),
+ AllowedAddresses: []string{"Paul@Example.Test "},
+ Logger: quietLogger(),
+ }
+ got, list, err := resolve(given)
+ if err != nil {
+ t.Fatalf("resolve: %v", err)
+ }
+ if got.Origin != "https://slides.example.test" {
+ t.Errorf("Origin = %q, want it trimmed and kept", got.Origin)
+ }
+ if got.InstanceKey != given.InstanceKey {
+ t.Errorf("InstanceKey = %q, want %q", got.InstanceKey, given.InstanceKey)
+ }
+ if got.Mailer == nil {
+ t.Error("Mailer was dropped")
+ }
+ // The allowlist normalizes case and whitespace on both sides, so a
+ // config file with spaces after its commas and a person typing
+ // capitals both work.
+ if !list.allows("paul@example.test") {
+ t.Error("allowlist refused the normalized form of a configured address")
+ }
+ if list.allows("someone@else.test") {
+ t.Error("allowlist admitted an address nobody configured")
+ }
+}
+
+// TestProductionRefusesEachMissingSetting pins the fail-closed rule at
+// the resolve level, one setting at a time, so a regression names which
+// one stopped refusing. The HTTP-level counterpart lives in
+// internal/slidestest.
+func TestProductionRefusesEachMissingSetting(t *testing.T) {
+ full := func() Config {
+ return Config{
+ Origin: "https://slides.example.test",
+ Env: EnvProduction,
+ InstanceKey: "a-real-key",
+ Mailer: mail.SMTP("smtp.example.test", "", "slides@example.test", "", ""),
+ AllowedAddresses: []string{"paul@example.test"},
+ Logger: quietLogger(),
+ }
+ }
+ for _, tc := range []struct {
+ name string
+ bend func(*Config)
+ }{
+ {"Origin", func(c *Config) { c.Origin = "" }},
+ {"Origin whitespace only", func(c *Config) { c.Origin = " " }},
+ {"Origin with a path", func(c *Config) { c.Origin = "https://slides.example.test/app" }},
+ {"Origin with no host", func(c *Config) { c.Origin = "http://:8091" }},
+ {"InstanceKey", func(c *Config) { c.InstanceKey = "" }},
+ {"Mailer", func(c *Config) { c.Mailer = nil }},
+ {"AllowedAddresses", func(c *Config) { c.AllowedAddresses = nil }},
+ {"AllowedAddresses all blank", func(c *Config) { c.AllowedAddresses = []string{"", " "} }},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ cfg := full()
+ tc.bend(&cfg)
+ if _, _, err := resolve(cfg); err == nil {
+ t.Fatalf("resolve with %s bent returned no error; want a startup refusal", tc.name)
+ }
+ })
+ }
+}
diff --git a/internal/slides/handlers.go b/internal/slides/handlers.go
index cd7de25..2c964b7 100644
--- a/internal/slides/handlers.go
+++ b/internal/slides/handlers.go
@@ -8,13 +8,17 @@ import (
"amadan.net/rastrillo/rastrillo/db"
)
-// app holds what every handler needs: the database, the logger, and
-// the resolved config — which is also where the object store arrives
+// app holds what every handler needs: the database, the logger, the
+// RESOLVED config — every fail-closed default already applied, so
+// a.cfg.Origin is the origin this app is really protecting and
+// a.cfg.Mailer is the sender it is really using — and the parsed
+// allowlist. Config is also where the object store arrives
// (Config.Blobs) for the deck screens that come next.
type app struct {
- db *db.DB
- logger *slog.Logger
- cfg Config
+ db *db.DB
+ logger *slog.Logger
+ cfg Config
+ allowed allowlist
}
// indexPage is GET /'s view data — the deck list. Every string on it is
@@ -43,7 +47,14 @@ func (a *app) index(w http.ResponseWriter, r *http.Request) {
if !ok {
// Past RequireSession this means the users row could not be
// read (currentUser logged why), not that nobody is signed in.
- http.Error(w, rastrillo.T(r, "slides.error.generic"), http.StatusInternalServerError)
+ // ErrorPage, not http.Error: a failure here gets the app's own
+ // page in the app's own shell, the same one a panic gets, and
+ // a ref a person can quote — plain text on a blank page is
+ // what this app renders when the renderer itself is broken,
+ // which is not the case here.
+ ref := rastrillo.NewRef()
+ a.logger.Error("slides: index: no current user past the session gate", "ref", ref)
+ ErrorPage(w, r, http.StatusInternalServerError, ref)
return
}
person := u.Name
diff --git a/internal/slidestest/harness_test.go b/internal/slidestest/harness_test.go
index b2721a3..b74a492 100644
--- a/internal/slidestest/harness_test.go
+++ b/internal/slidestest/harness_test.go
@@ -180,6 +180,25 @@ func post(t *testing.T, h http.Handler, target string, form url.Values, cookies
return rec
}
+// postLegacy is post without the Sec-Fetch-Site header, with origin as
+// the only same-origin evidence.
+//
+// It exists because post sends both, and csrf.Protect checks
+// Sec-Fetch-Site FIRST and never consults Origin at all when it is
+// present — so every ordinary test here leaves the app's configured
+// Origin entirely unexercised, and would pass with a wrong one. This is
+// the shape a non-browser client (or an old browser) sends, and the
+// only shape that puts the app's own resolved origin under test.
+func postLegacy(t *testing.T, h http.Handler, target, origin string, form url.Values) *httptest.ResponseRecorder {
+ t.Helper()
+ req := httptest.NewRequest(http.MethodPost, target, strings.NewReader(form.Encode()))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ req.Header.Set("Origin", origin)
+ rec := httptest.NewRecorder()
+ h.ServeHTTP(rec, req)
+ return rec
+}
+
// signedIn runs the whole magic-link flow for address against the fake
// mailer — POST /signin, read the link out of the captured message,
// GET it — and returns the cookies the verify step set. Every later
diff --git a/internal/slidestest/signin_test.go b/internal/slidestest/signin_test.go
index 58c79dd..dbe5ccb 100644
--- a/internal/slidestest/signin_test.go
+++ b/internal/slidestest/signin_test.go
@@ -87,6 +87,88 @@ func TestSigninMailsAllowedAddressOnly(t *testing.T) {
}
}
+// TestSigninAnswerIsIdenticalOverTheRateLimitBudget is the sharp end of
+// the no-oracle guarantee, and the case the first version of gateBegin
+// got wrong.
+//
+// A denied address is short-circuited before rastrillo/auth's Begin, so
+// it can answer ?sent=1 forever. A permitted address reaches Begin,
+// which enforces a per-address budget of 5 in a rolling window and
+// redirects to ?err=rate once it is spent. So without the fix, six
+// POSTs of one candidate address separate "on the list" from "not on
+// it" — the whole allowlist, enumerable by anyone, one address at a
+// time.
+//
+// Seven POSTs, every one byte-identical to what a denied address gets.
+// The mailer stops at 5, which is the other half of the proof: the
+// answers are the same because the failure is hidden, not because the
+// budget quietly stopped being enforced.
+func TestSigninAnswerIsIdenticalOverTheRateLimitBudget(t *testing.T) {
+ app := newApp(t)
+
+ denied := post(t, app, "/signin", url.Values{"address": {deniedAddress}})
+ if denied.Code != http.StatusSeeOther {
+ t.Fatalf("POST /signin (denied): status %d, want %d", denied.Code, http.StatusSeeOther)
+ }
+ wantLoc := denied.Header().Get("Location")
+ wantBody := denied.Body.String()
+ if wantLoc != "/signin?sent=1" {
+ t.Fatalf("POST /signin (denied): Location = %q, want %q", wantLoc, "/signin?sent=1")
+ }
+
+ const attempts = 7
+ for i := 1; i <= attempts; i++ {
+ rec := post(t, app, "/signin", url.Values{"address": {allowedAddress}})
+ if rec.Code != denied.Code {
+ t.Errorf("POST /signin (allowed, attempt %d): status %d, want %d — identical to a denied address", i, rec.Code, denied.Code)
+ }
+ if got := rec.Header().Get("Location"); got != wantLoc {
+ t.Errorf("POST /signin (allowed, attempt %d): Location = %q, want %q — identical to a denied address", i, got, wantLoc)
+ }
+ if got := rec.Body.String(); got != wantBody {
+ t.Errorf("POST /signin (allowed, attempt %d): body %q, want %q — identical to a denied address", i, got, wantBody)
+ }
+ }
+
+ // signin's per-address budget is 5; the two attempts past it are
+ // the ones whose ?err=rate the guard rewrote.
+ if n := len(app.mailer.messages()); n != 5 {
+ t.Errorf("mailer captured %d messages over %d attempts, want the 5 the per-address budget allows", n, attempts)
+ }
+}
+
+// TestSigninAcceptsALegacyOriginOnlyPost covers the path a
+// Sec-Fetch-Site header hides: csrf.Protect checks that header first
+// when a browser sends it, and only falls back to comparing Origin
+// against the app's configured origin when it is absent entirely — a
+// non-browser client, or an old one. The post helper sends both, so
+// without this test the Origin half of the app's config would never be
+// exercised at all, and a wrong resolved origin would pass the suite.
+//
+// It is also the one test that observes the RESOLVED origin from
+// outside the package: the app was configured with testOrigin, and this
+// is what proves that string is what csrf.Protect is holding requests
+// to.
+func TestSigninAcceptsALegacyOriginOnlyPost(t *testing.T) {
+ app := newApp(t)
+
+ ok := postLegacy(t, app, "/signin", testOrigin, url.Values{"address": {allowedAddress}})
+ if ok.Code != http.StatusSeeOther {
+ t.Errorf("POST /signin with Origin %q and no Sec-Fetch-Site: status %d, want %d", testOrigin, ok.Code, http.StatusSeeOther)
+ }
+ if n := len(app.mailer.messages()); n != 1 {
+ t.Errorf("a same-origin legacy POST mailed %d links, want 1", n)
+ }
+
+ refused := postLegacy(t, app, "/signin", "http://evil.test", url.Values{"address": {allowedAddress}})
+ if refused.Code == http.StatusSeeOther {
+ t.Errorf("POST /signin with Origin %q and no Sec-Fetch-Site: status %d, want a refusal", "http://evil.test", refused.Code)
+ }
+ if n := len(app.mailer.messages()); n != 1 {
+ t.Errorf("a cross-origin POST mailed a link; mailer has %d messages, want the 1 from the same-origin post", n)
+ }
+}
+
// TestVerifyMintsSessionAndUserRow: following the emailed link both
// establishes a usable session and creates this app's own person row —
// the address-to-id mapping every later owner-scoped query needs, since