amadan

oficina / meet Public

fix: codex code review round 1 (C1-2, C1-3, C1-4, C1-7, C1-8)

C1-2 (P1) An outsider's address reached the log twice over: rastrillo/mail
names the recipient in every delivery error and rastrillo/auth logs it, and
GORM's warning and slow-query lines carried interpolated query parameters.
meet.RedactMailErrors wraps the sender, logging the failure itself with the
address struck out and returning an error that carries nothing; build now
gives GORM a logger with ParameterizedQueries set, writing through slog.

C1-3 (P1) followLink read the link, then minted a grant from the meeting,
so a link replaced in between still minted — and standing checks the grant,
not its origin. store.mintGrantViaLink re-reads the link by id inside the
grant's own transaction and answers not-live, which followLink turns into
the same 404 an unknown token gets.

C1-4 (P2) consumePending trusted a pending row read before its transaction
opened. It now re-selects the newest unexpired row for the fingerprint
inside the transaction and proceeds only when it is still the one the
caller was handed; a mismatch deletes nothing, so the row that superseded
it survives.

C1-7 (P2) Expired pending rows were excluded from reads but never deleted,
and nothing ever called auth.Sweep, so an unclicked magic link kept a
plaintext address in auth_links for the life of the instance. store.sweep
and app.sweepOnce run at boot and hourly.

C1-8 (P2) Gates that were missing or passed for the wrong reason: the
no-standing test asked for a meeting that did not exist; the shared-limiter
test proved the five-per-address budget, not the twenty-per-client one; the
replaced-link test could not see whether a grant row existed; the WAL
privacy test skipped every file-read error. All four fixed, and the missing
§13 rows added: Authorize's refusal, the standing table, the one 404's byte
equality, a member completing in a second browser, and both signed-in
fallbacks after a replacement.

C1-1 (P1) is deferred: persons.email holds a verified address in plaintext
because that is the suite's shape for a person (spec §2.1, as in Docs,
Slides and Calendar), and §0.1 promises a fingerprint for pending_joins
only. Filed as a suite question; AGENTS.md's seal bullet now says which
table is which. C1-5 and C1-6 were already closed on main.

docs/superpowers/reviews/2026-09-09-codex-code-r1.md carries the review
verbatim and the disposition of every finding.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6JGj6G5cLrt2Frm8yywgK
Paul Campbell pushed by paul@keymail.dev 5fdddcfd433f60c0e3a66957b5ec9c210051b38c parent fd626d6
13 files changed, +872 −18
  • AGENTS.md +5 −2
  • cmd/meet/main.go +5 −1
  • docs/superpowers/reviews/2026-09-09-codex-code-r1.md +137 −0
  • internal/meet/app.go +55 −0
  • internal/meet/auth_test.go +125 −0
  • internal/meet/handlers_join.go +11 −1
  • internal/meet/mailredact.go +90 −0
  • internal/meet/mailredact_test.go +74 −0
  • internal/meet/store.go +58 −4
  • internal/meet/store_test.go +133 −0
  • internal/meettest/holding_test.go +27 −5
  • internal/meettest/join_test.go +145 −4
  • internal/meettest/meetings_test.go +7 −1
diff --git a/AGENTS.md b/AGENTS.md
index 6bd07ef..6d355c4 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -21,8 +21,11 @@ mechanically.
with `rastrillo generate`.
- `internal/meet/seal/` — the app's only encryption surface. A title
is sealed by the store and opened by the store; no handler holds a
- plaintext title it did not get from `store.title`. An outsider's
- address is stored only as a keyed fingerprint.
+ plaintext title it did not get from `store.title`. `pending_joins` is
+ the fingerprinted table: an outsider's address is stored there only as
+ a keyed fingerprint, while `persons.email` holds a verified address in
+ plaintext, the suite's current shape for a person (spec §2.1, as in
+ Docs, Slides and Calendar).
- `internal/meet/store.go` — every database access. `standing.go` is
the entry authority (spec §2.3), computed per request, never stored.
- Sign-in: `auth.go` is Docs' shape with the pending-join exception of
diff --git a/cmd/meet/main.go b/cmd/meet/main.go
index 0e0a725..7ff70f5 100644
--- a/cmd/meet/main.go
+++ b/cmd/meet/main.go
@@ -42,7 +42,11 @@ func main() {
}
defer d.Close()
- mailer := newMailer(logger)
+ // Wrapped, not raw: rastrillo/mail names the recipient in every
+ // delivery error and rastrillo/auth logs it. The wrapper logs the
+ // failure with the address struck out and hands back an error that
+ // carries nothing (review C1-2).
+ mailer := meet.RedactMailErrors(newMailer(logger), logger)
cfg := meet.Config{
Env: *env,
diff --git a/docs/superpowers/reviews/2026-09-09-codex-code-r1.md b/docs/superpowers/reviews/2026-09-09-codex-code-r1.md
new file mode 100644
index 0000000..6a43c28
--- /dev/null
+++ b/docs/superpowers/reviews/2026-09-09-codex-code-r1.md
@@ -0,0 +1,137 @@
+# Code review round 1 — GPT-6 via Codex, 2026-09-09
+
+Reviewer: GPT-6 (Codex), read-only sandbox. Pinned to `cae60ed`; the
+checkout advanced during the review, so C1-5 and C1-6 were already fixed
+on the branch by the time it landed. Fixes applied on `fix/codex-code-r1`.
+
+## The review, verbatim
+
+Review pinned to `cae60ed`. The checkout advanced during review; later changes are excluded. No files modified. Tests were inspected, not executed in the read-only sandbox.
+
+**C1-1 — P1 — `internal/meet/store.go:55`, `internal/meet/auth.go:23`: Outsider addresses persist after verification.**
+
+`ensurePerson` writes every verified outsider’s plaintext address into `persons.email` and its local part into `persons.name`. Auth leaves `SubjectFor` unset, so rastrillo also writes the plaintext address into `sessions.subject`. Joining `guest_grants` to `persons` identifies outsiders by meeting. Fingerprinting `pending_joins` does not protect these copies.
+
+This follows §2.1’s person model but contradicts the stated outsider-address privacy guarantee; the conflict is real, not resolved by sealing pending rows.
+
+Correction: use an opaque session subject, a keyed address lookup, and sealed address/name fields. Resolve the conflicting model contract explicitly and test the complete outsider journey against database and WAL bytes.
+
+**C1-2 — P1 — `internal/meet/auth.go:27`, `cmd/meet/main.go:30`: Production error logging exposes outsider addresses.**
+
+Rastrillo’s `mail/mail.go:163` wraps every SMTP delivery failure as `send to <address>: <error>`. Its `auth/handlers.go:51` logs that error through Meet’s logger. An outsider requesting a link during an SMTP outage therefore leaves their address in logs before ever signing in.
+
+Separately, `db.Open` enables GORM warning/slow-query logging without `ParameterizedQueries`. Slow or failed `ensurePerson` queries can log interpolated addresses.
+
+Correction: sanitize mail errors before returning them to auth, and configure database logging to exclude parameter values. Add captured-log tests for failed SMTP delivery and slow/failed person queries.
+
+**C1-3 — P1 — `internal/meet/handlers_join.go:13`, `internal/meet/handlers_join.go:36`: A replaced link can mint a new grant.**
+
+The signed-in path checks the link outside the grant transaction:
+
+1. A signed-in outsider requests link A; `liveLink` returns it as live.
+2. A member replaces A with B and commits.
+3. The outsider’s request calls `mintGrant` using the previously loaded meeting.
+
+`mintGrant` never checks A. The resulting grant remains usable because standing checks the grant, not its originating link. SQLite’s single writer does not serialize these separate reads and writes into one operation.
+
+Correction: resolve the originating link, check revocation and meeting state, and mint the grant within one transaction. Add a controlled concurrent replacement test.
+
+**C1-4 — P2 — `internal/meet/auth.go:223`, `internal/meet/store.go:304`: Continuation trusts a pending row that may already have been consumed.**
+
+`latestPending` runs before the transaction. `consumePending` deletes rows but never verifies that its supplied `pend` still exists or remains the newest row.
+
+For example, request R1 reads pending A and pauses. A newer pending B is written and consumed by R2, deleting both rows. R1 then resumes and can mint a grant for A from its stale Go value, despite no pending row remaining. The unique grant index only prevents duplicates for the same meeting.
+
+Correction: select the current pending row inside the same transaction that validates, grants and deletes. Test overlapping continuations involving different meetings.
+
+**C1-5 — P2 — `cmd/meet/main.go:21`: The two flag parsers make supported launch modes fail.**
+
+The global parser knows only `-env`, so `meet -addr :8080` and activation with `--socket … --db …` exit before `rastrillo.Resolve`.
+
+Conversely, `meet -env development` passes the first parser but fails in `Resolve`, which reparses the unchanged `os.Args` and recognizes only `socket`, `addr` and `db`. The advertised development mode cannot start.
+
+Correction: establish one coordinated argument parser, or remove app-specific arguments before passing the remainder to the framework. Test actual executable invocations for development, TCP, socket activation and `serve`.
+
+**C1-6 — P2 — `cmd/meet/main.go:38`: Platform mail configuration is ignored.**
+
+At `cae60ed`, only `MEET_SMTP_HOST` and `MEET_SMTP_FROM` create a mailer, and `mail.FromEnv("MEET", …)` reads only that prefix. With only the platform-delivered `CARLOS_SMTP_*` variables, production refuses startup.
+
+Meet cannot send using `carlos email enable` configuration alone. The existing binary can send without a code change if an operator separately supplies matching `MEET_SMTP_*` variables.
+
+Correction: add per-setting CARLOS fallbacks while retaining explicit Meet overrides. Trim HOST/FROM before testing presence: whitespace currently passes the outer check but makes `FromEnv` return a logging mailer, which production accepts as non-nil.
+
+**C1-7 — P2 — `internal/meet/store.go:278`, `internal/meet/app.go:69`: Expired sign-in records are never swept.**
+
+Expired pending rows are excluded from reads, not deleted. Only consumption deletes them. Meet also never calls `auth.Sweep`; rastrillo deletes an `auth_links` row when its token is presented, otherwise only through that explicit sweep.
+
+An unclicked or unsuccessfully delivered magic link therefore leaves its plaintext outsider address in `auth_links` indefinitely, beyond its fifteen-minute validity. Abandoned pending rows and expired sessions also accumulate.
+
+Correction: run bounded expiry cleanup at boot and periodically. Test unclicked links and abandoned pending rows. Deletion does not erase historical WAL or backup copies; sealing the auth link store is required for that stronger guarantee.
+
+**C1-8 — P2 — `internal/meettest/join_test.go:131`, `internal/meettest/holding_test.go:22`: Required gates remain missing or pass for the wrong reason.**
+
+The supposedly shared-limiter test sends six requests for one address. It proves the five-per-address limit, not exhaustion of the twenty-per-client allowance. It would pass if the client limiter disappeared.
+
+The no-standing test requests a nonexistent meeting, so it passes before `standingFor` runs. The replaced-link test’s final signed-out request cannot prove that no grant row exists. The database privacy test silently skips every file-read error, allowing a vacuous pass.
+
+Correction: assert the intended state directly, exercise existing meetings, fail unexpected file-read errors, and complete this §13 coverage:
+
+| Slice 1 gate | Coverage at `cae60ed` |
+|---|---|
+| Meeting storage, replacement and ending | `TestCreateMeetingSealsTitleAndMintsOneLiveLink`, `TestReplaceLinkRevokesTheOldOne`, `TestEndMeetingKillsItsLinks` |
+| Grant expiry and idempotency | `TestGrantExpiresTwoHoursAfterStartOrCreation`, `TestExpiredGrantIsRefusedEntry`; concurrent minting missing |
+| One-404 rule | Unknown/replaced/ended statuses covered across `TestUnknownRevokedAndEndedLinksAreOne404` and the two meeting-action tests; response equality missing |
+| Outsider emailed-link round trip, cookie present | `TestSignedOutOutsiderSignsInThroughTheLinkInTheSameBrowser` |
+| Same round trip, cookie absent at click | `TestEmailedLinkWorksInAnotherBrowser` |
+| `Authorize` admits pending address | Covered by both round-trip tests |
+| `Authorize` refuses unknown verified address | Missing; refusing an outsider before `Begin` does not exercise `Authorize` |
+| Revocation during sign-in | `TestLinkReplacedBetweenMailAndClickAdmitsNobody`; concurrent revocation missing |
+| `/continue`: live pending, member/non-member | `TestSignedOutMemberFollowsALinkAndLandsOnItsHoldingPage` and outsider round-trip tests |
+| `/continue`: absent pending, member/non-member | `TestMemberSignsInAndLandsOnTheList`, `TestNonMemberSessionSeesWelcomeNotTheList` |
+| `/continue`: expired pending, member/non-member | Member covered by `TestExpiredPendingRowAdmitsNobody`; non-member stops at `Authorize`, so its continuation case is missing |
+| Signed-out member completes in another browser | Missing; member test uses the same browser |
+| Replacement before click: outsider 403, signed-in fallback, no grants | Outsider 403 covered; signed-in `/` and `/welcome` fallbacks and direct no-grant assertions missing |
+| Outsider budgets | `TestOutsiderBudgetsStopMailAtFivePerLinkAndTenOverall`, `TestBudgetSlidesItsWindow`; concurrency missing |
+| One member address exhausts shared allowance | Missing; existing test reaches only the address limit |
+| Standing table test | Missing; no-standing HTTP substitute uses an absent meeting |
+| Database/WAL privacy before and after checkpoint | `TestTheDatabaseNeverHoldsAPlaintextTitle`; file-read failures are ignored |
+| `make ci` | Prior record reports green; not independently executed here |
+
+**Verdict: request changes.**
+
+## Dispositions
+
+| Finding | Disposition |
+|---|---|
+| C1-1 — outsider addresses in `persons.email` and `sessions.subject` | **Deferred: suite question, filed.** Controller ruling: the persons row with a plaintext email is the suite-wide shape (Docs, Slides, Calendar), and spec §2.1 says so. The plan's "outsider addresses never reach the database" overstated §0.1, which promises a fingerprint for `pending_joins` only. `AGENTS.md`'s seal bullet now says which table is which. No code change. |
+| C1-2 — mail errors carry the address into the log | **Closed** at `internal/meet/mailredact.go:30` (`RedactMailErrors`), wired at `cmd/meet/main.go:49`. The wrapper logs the failure itself, with the root cause's type and every occurrence of the recipient replaced by `[address]`, and returns a bare `meet: mail delivery failed`, so `auth/handlers.go:51` never sees an address. |
+| C1-2 — GORM logs interpolated parameters | **Closed** at `internal/meet/app.go:92`: `db.Open`'s GORM logger is replaced with one carrying `ParameterizedQueries: true` at the same thresholds, writing through the app's slog logger (`gormWriter`, `internal/meet/app.go:25`). A slow or failing `ensurePerson` now logs `?`, not an address. |
+| C1-3 — a replaced link can mint a grant | **Closed** at `internal/meet/store.go:265` (`mintGrantViaLink`) and `internal/meet/handlers_join.go:41`. The link is re-read by id inside the grant's own transaction; a revoked link or an ended meeting answers false, and `followLink` turns that into the same 404 an unknown token gets. `mintGrant` stays for `consumePending`, which already runs in a transaction that re-checks the link. |
+| C1-4 — a stale pending row can mint | **Closed** at `internal/meet/store.go:354`. `consumePending` re-selects the newest unexpired row for the fingerprint inside the transaction and proceeds only when its id is the one the caller was handed. A mismatch deletes nothing, so the row that superseded it survives and the caller falls through to `/` or `/welcome`. |
+| C1-5 — two flag parsers | **Closed before this wave** (85025bc, fd626d6): `extractEnvFlag` strips `-env` from argv before `rastrillo.Resolve` reparses it (`cmd/meet/main.go:111`). |
+| C1-6 — platform mail configuration ignored | **Closed before this wave** (85025bc, fd626d6): `smtpSetting` and `smtpSecret` read `MEET_SMTP_*` first and fall back to `CARLOS_SMTP_*` per variable, trimming HOST and FROM (`cmd/meet/main.go:80`). |
+| C1-7 — expired records never swept | **Closed** at `internal/meet/store.go:378` (`store.sweep`) and `internal/meet/app.go:56` (`app.sweepOnce`), run once at boot (`internal/meet/app.go:123`) and hourly thereafter (`internal/meet/app.go:124`). The hourly pass also calls `auth.Sweep`, which clears rastrillo's `auth_links` and its expired sessions. Counts at info, failures at error. |
+| C1-8 — tests | **Closed.** Per-gate below. |
+
+### C1-8, gate by gate
+
+| Gate | Now |
+|---|---|
+| Database/WAL privacy fails on unexpected read errors | `TestTheDatabaseNeverHoldsAPlaintextTitle` skips only `os.IsNotExist` and fails every other read error — `internal/meettest/meetings_test.go:111`. |
+| No standing on a meeting that exists | `TestHoldingPageIs404ForNoStanding` rewritten: a signed-in outsider holding a grant for one meeting gets 404 on a second real meeting, and 404 on the first once its grant has run out — `internal/meettest/holding_test.go:27`. |
+| `Authorize` refuses an unknown verified address | `TestAuthorizeAdmitsMembersAndLivePendingRowsOnly`, package-internal — `internal/meet/auth_test.go:42`. Unknown false; allowlisted true; a live pending row true; false once its link is replaced. |
+| Signed-out member completes in another browser | `TestSignedOutMemberCompletesInAnotherBrowser` — `internal/meettest/join_test.go`. |
+| Replacement before click: signed-in fallbacks, no grant | `TestReplacedLinkFallbackForSignedInPerson` — `internal/meettest/join_test.go`. Member lands on `/`; non-member on `/welcome`, and their holding page for that meeting 404s, which is the direct no-grant assertion. |
+| Standing table | `TestStandingTable`, package-internal — `internal/meet/auth_test.go:76`. Six cases over allowlisted × grant × expired. |
+| One 404, bytes and all | `TestUnknownRevokedAndEndedLinksAreOne404` now requests an unknown token, a revoked token and an ended meeting's token, and asserts all three statuses are 404 and all three bodies are identical. |
+| One client exhausts the shared allowance | `TestSharedClientAllowanceIsExhaustedByOneMemberAddress` — twenty-one different allowlisted addresses from the `RemoteAddr` `httptest` fixes at `192.0.2.1:1234`; the twenty-first answers `303 /signin?err=rate` with exactly twenty mails sent. The limit is twenty: `signin.Flow.IPBudget` defaults to 20 and `AddressBudget` to 5 (`github.com/keymaildev/signin@v0.1.1/flow.go:123,137`). |
+| C1-2 regression | `TestMailErrorsCarryNoAddress` — `internal/meet/mailredact_test.go`. A failing inner sender whose error contains the address; neither the returned error nor a captured slog handler's output contains it. |
+| C1-3 regression | `TestAReplacedLinkMintsNoGrant` — `internal/meet/store_test.go`. Minting through the old link id after a replacement answers not-live and writes no grant row. |
+| C1-4 regression | `TestAStalePendingRowMintsNothingAndDeletesNothing` — `internal/meet/store_test.go`. Two rows for one address; consuming with the older id mints nothing and leaves both rows; the newer id then works and takes both. |
+| C1-7 regression | `TestSweepDeletesOnlyExpiredPendingRows` — `internal/meet/store_test.go`. |
+
+Not covered, and named rather than claimed: the concurrency the review
+asks for (concurrent minting, concurrent revocation, concurrent budget
+consumption) is held by the transaction boundary and SQLite's single
+writer connection, not asserted by a racing test. `auth.Sweep` is
+exercised only by being called on the boot path.
diff --git a/internal/meet/app.go b/internal/meet/app.go
index d32cddb..827de3c 100644
--- a/internal/meet/app.go
+++ b/internal/meet/app.go
@@ -2,11 +2,13 @@ package meet
import (
"context"
+ "fmt"
"log/slog"
"net/http"
"time"
"github.com/go-chi/chi/v5"
+ gormlogger "gorm.io/gorm/logger"
rauth "amadan.net/rastrillo/rastrillo/auth"
"amadan.net/rastrillo/rastrillo/csrf"
@@ -32,6 +34,44 @@ type app struct {
// expire grants and pending rows without sleeping.
func (a *app) SetClockForTest(now func() time.Time) { a.store.now = now }
+// gormWriter is GORM's logger.Writer over the app's slog logger, so
+// GORM's warnings land in one log stream rather than a second stdout
+// format. rastrillo/db has the same adapter, unexported.
+type gormWriter struct{ log *slog.Logger }
+
+func (w gormWriter) Printf(format string, args ...any) {
+ w.log.Warn(fmt.Sprintf(format, args...))
+}
+
+// sweepEvery is how often expired rows are cleared. Nothing depends on
+// it for correctness — every read filters on expiry — so the interval
+// is about not keeping records past their life, not about safety.
+const sweepEvery = time.Hour
+
+// sweepOnce clears Meet's expired pending rows and rastrillo's expired
+// auth links and sessions (review C1-7). An unclicked magic link holds a
+// plaintext address in auth_links, and rastrillo only deletes it when
+// the token is presented or when Sweep is called; nobody was calling
+// Sweep.
+func (a *app) sweepOnce(ctx context.Context) {
+ if n, err := a.store.sweep(ctx, a.store.now()); err != nil {
+ a.logger.Error("meet: sweep pending joins", "err", err)
+ } else if n > 0 {
+ a.logger.Info("meet: swept pending joins", "deleted", n)
+ }
+ if err := a.auth.Sweep(time.Now()); err != nil {
+ a.logger.Error("meet: sweep auth links and sessions", "err", err)
+ }
+}
+
+func (a *app) sweepForever() {
+ t := time.NewTicker(sweepEvery)
+ defer t.Stop()
+ for range t.C {
+ a.sweepOnce(context.Background())
+ }
+}
+
// build wires the whole app: refusals, schema, router, static files. It
// returns the *app alongside the mux so tests can reach into it (see
// NewForTest); production code uses App, which discards the *app.
@@ -44,6 +84,18 @@ func build(d *db.DB, cfg Config) (*http.ServeMux, *app, error) {
if err != nil {
return nil, nil, err
}
+ // rastrillo/db configures GORM's logger without ParameterizedQueries,
+ // so a slow or failing query is logged with its values interpolated
+ // into the SQL. ensurePerson looks a person up by address, so that
+ // one log line would carry an outsider's plaintext address (review
+ // C1-2). Same thresholds as db.Open, placeholders instead of values.
+ d.G.Logger = gormlogger.New(gormWriter{rc.Logger}, gormlogger.Config{
+ SlowThreshold: 200 * time.Millisecond,
+ LogLevel: gormlogger.Warn,
+ IgnoreRecordNotFoundError: true,
+ ParameterizedQueries: true,
+ })
+
migrated, err := migrate.Apply(context.Background(), d, BootSchema)
if err != nil {
return nil, nil, err
@@ -68,6 +120,9 @@ func build(d *db.DB, cfg Config) (*http.ServeMux, *app, error) {
}
a.auth = authPlugin
+ a.sweepOnce(context.Background())
+ go a.sweepForever()
+
r := chi.NewRouter()
r.Use(csrf.Protect(rc.Origin))
diff --git a/internal/meet/auth_test.go b/internal/meet/auth_test.go
new file mode 100644
index 0000000..8c66dc4
--- /dev/null
+++ b/internal/meet/auth_test.go
@@ -0,0 +1,125 @@
+package meet
+
+import (
+ "context"
+ "io"
+ "log/slog"
+ "path/filepath"
+ "testing"
+ "time"
+
+ "amadan.net/rastrillo/rastrillo/db"
+)
+
+// testApp builds the whole app over a fresh temp database and hands back
+// the *app itself, which NewForTest deliberately does not. These two
+// tables are about internals — Authorize and standingFor — that no HTTP
+// request reaches directly.
+func testApp(t *testing.T, members ...string) *app {
+ t.Helper()
+ logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+ d, err := db.Open(filepath.Join(t.TempDir(), "a.db"), logger)
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { d.Close() })
+ _, a, err := build(d, Config{
+ Env: EnvDevelopment, Origin: "http://app.test",
+ AllowedAddresses: members, Logger: logger,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ now := time.Date(2026, 9, 8, 10, 0, 0, 0, time.UTC)
+ a.SetClockForTest(func() time.Time { return now })
+ return a
+}
+
+// Review C1-8: refusing an outsider in front of Begin never exercises
+// Authorize, so nothing proved that Authorize itself refuses an address
+// with no pending row. Spec §3.2: on the allowlist, or holding an
+// unexpired pending row whose link is still live.
+func TestAuthorizeAdmitsMembersAndLivePendingRowsOnly(t *testing.T) {
+ a := testApp(t, "paul@example.com")
+ ctx := context.Background()
+ m, tok, err := a.store.createMeeting(ctx, "x", nil, "person:1")
+ if err != nil {
+ t.Fatal(err)
+ }
+ link, _, _, _ := a.store.liveLink(ctx, tok)
+
+ if a.authorize("unknown@example.com") {
+ t.Fatal("an address with no pending row was admitted")
+ }
+ if !a.authorize("paul@example.com") {
+ t.Fatal("an allowlisted address was refused")
+ }
+ if !a.authorize("PAUL@Example.com ") {
+ t.Fatal("an allowlisted address was refused on case or whitespace")
+ }
+ if err := a.store.writePending(ctx, "guest@example.com", link); err != nil {
+ t.Fatal(err)
+ }
+ if !a.authorize("guest@example.com") {
+ t.Fatal("an address with a live pending row was refused")
+ }
+ if _, err := a.store.replaceLink(ctx, m.ID, "person:1"); err != nil {
+ t.Fatal(err)
+ }
+ if a.authorize("guest@example.com") {
+ t.Fatal("an address whose link was replaced was still admitted")
+ }
+}
+
+// Review C1-8: the one-per-request entry authority (spec §2.3), asserted
+// directly rather than through the holding page's status code.
+func TestStandingTable(t *testing.T) {
+ ctx := context.Background()
+ for _, tc := range []struct {
+ name string
+ allowlisted bool
+ grant bool
+ expired bool
+ want Standing
+ }{
+ {name: "allowlisted, no grant", allowlisted: true, want: StandingMember},
+ {name: "allowlisted, live grant", allowlisted: true, grant: true, want: StandingMember},
+ {name: "allowlisted, expired grant", allowlisted: true, grant: true, expired: true, want: StandingMember},
+ {name: "outsider, no grant", want: StandingNone},
+ {name: "outsider, live grant", grant: true, want: StandingGuest},
+ {name: "outsider, expired grant", grant: true, expired: true, want: StandingNone},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ const address = "someone@example.com"
+ members := []string{"other@example.com"}
+ if tc.allowlisted {
+ members = append(members, address)
+ }
+ a := testApp(t, members...)
+ m, _, err := a.store.createMeeting(ctx, "x", nil, "person:1")
+ if err != nil {
+ t.Fatal(err)
+ }
+ p, err := a.store.ensurePerson(ctx, address)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if tc.grant {
+ if _, err := a.store.mintGrant(ctx, m, p.ID); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if tc.expired {
+ after := a.store.now().Add(grantLife + time.Minute)
+ a.SetClockForTest(func() time.Time { return after })
+ }
+ got, err := a.standingFor(ctx, p, m)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got != tc.want {
+ t.Fatalf("standing %s, want %s", got, tc.want)
+ }
+ })
+ }
+}
diff --git a/internal/meet/handlers_join.go b/internal/meet/handlers_join.go
index e0b960a..738b7cd 100644
--- a/internal/meet/handlers_join.go
+++ b/internal/meet/handlers_join.go
@@ -33,11 +33,21 @@ func (a *app) followLink(w http.ResponseWriter, r *http.Request) {
return
}
if !a.isMember(p) {
- if _, err := a.store.mintGrant(r.Context(), m, p.ID); err != nil {
+ // Via the link, not the meeting: this handler read the link
+ // before it resolved the person, and a member may have replaced
+ // it in between. The re-read happens inside the grant's own
+ // transaction, and a link that is no longer live gets the same
+ // 404 an unknown token gets (review C1-3).
+ _, live, err := a.store.mintGrantViaLink(r.Context(), link.ID, p.ID)
+ if err != nil {
a.logger.Error("meet: mint grant", "err", err)
ErrorPage(w, r, http.StatusInternalServerError, "")
return
}
+ if !live {
+ ErrorPage(w, r, http.StatusNotFound, "")
+ return
+ }
}
http.Redirect(w, r, "/m/"+m.Sqid+"/join", http.StatusSeeOther)
}
diff --git a/internal/meet/mailredact.go b/internal/meet/mailredact.go
new file mode 100644
index 0000000..cc0aeb9
--- /dev/null
+++ b/internal/meet/mailredact.go
@@ -0,0 +1,90 @@
+package meet
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "log/slog"
+ "strings"
+
+ "amadan.net/rastrillo/rastrillo/mail"
+)
+
+// RedactMailErrors wraps a mail sender so a delivery failure never
+// carries the recipient's address out of this package (review C1-2).
+//
+// rastrillo/mail wraps every SMTP failure as "send to <address>: <err>"
+// (mail/mail.go:163) and rastrillo/auth logs that error verbatim
+// (auth/handlers.go:51). An outsider who asks for a magic link during an
+// SMTP outage would therefore leave their plaintext address in the log,
+// before they have signed in and before Meet has decided to admit them —
+// which spec §0.1's falsifiable claim covers as squarely as the database
+// does. So the wrapper logs the failure itself, with the error's type and
+// the address struck out, and hands rastrillo an error that says only
+// that delivery failed.
+//
+// Only Send is wrapped because only Send is used: signin.Mailer (the
+// interface rastrillo/auth ultimately hands the mailer to) declares that
+// one method. A ListSender passed in here loses SendList, deliberately —
+// Meet sends one kind of message.
+func RedactMailErrors(inner mail.Sender, logger *slog.Logger) mail.Sender {
+ if inner == nil {
+ return nil
+ }
+ if logger == nil {
+ logger = slog.Default()
+ }
+ return &redactingSender{inner: inner, logger: logger}
+}
+
+type redactingSender struct {
+ inner mail.Sender
+ logger *slog.Logger
+}
+
+func (s *redactingSender) Send(ctx context.Context, to, subject, body string) error {
+ err := s.inner.Send(ctx, to, subject, body)
+ if err == nil {
+ return nil
+ }
+ s.logger.Error("meet: mail delivery failed",
+ "type", fmt.Sprintf("%T", rootCause(err)),
+ "err", redactAddress(err.Error(), to))
+ return errors.New("meet: mail delivery failed")
+}
+
+// rootCause is the innermost error in the chain. rastrillo/mail wraps
+// with fmt.Errorf, so %T on what it returns says "*fmt.wrapError" and
+// nothing about what actually went wrong; the cause names the dial
+// failure, the TLS refusal or the SMTP reply type.
+func rootCause(err error) error {
+ for {
+ next := errors.Unwrap(err)
+ if next == nil {
+ return err
+ }
+ err = next
+ }
+}
+
+// redactAddress replaces every occurrence of address in s with
+// "[address]", case-insensitively: an SMTP server routinely echoes a
+// recipient back in a different case from the one it was given.
+func redactAddress(s, address string) string {
+ address = strings.TrimSpace(address)
+ if address == "" {
+ return s
+ }
+ var b strings.Builder
+ lower, target := strings.ToLower(s), strings.ToLower(address)
+ for {
+ i := strings.Index(lower, target)
+ if i < 0 {
+ b.WriteString(s)
+ return b.String()
+ }
+ b.WriteString(s[:i])
+ b.WriteString("[address]")
+ s, lower = s[i+len(target):], lower[i+len(target):]
+ }
+}
diff --git a/internal/meet/mailredact_test.go b/internal/meet/mailredact_test.go
new file mode 100644
index 0000000..81d436c
--- /dev/null
+++ b/internal/meet/mailredact_test.go
@@ -0,0 +1,74 @@
+package meet
+
+import (
+ "bytes"
+ "context"
+ "fmt"
+ "log/slog"
+ "strings"
+ "testing"
+)
+
+type failingSender struct{ err error }
+
+func (f failingSender) Send(ctx context.Context, to, subject, body string) error { return f.err }
+
+// smtpish is a distinct type so the log line's "type" field is worth
+// something: it names what failed without naming who it failed for.
+type smtpish struct{ msg string }
+
+func (e smtpish) Error() string { return e.msg }
+
+func TestMailErrorsCarryNoAddress(t *testing.T) {
+ const address = "Guest@Example.com"
+ inner := failingSender{err: fmt.Errorf("rastrillo/mail: send to %s: %w",
+ address, smtpish{msg: "550 no mailbox for guest@example.com"})}
+
+ var buf bytes.Buffer
+ logger := slog.New(slog.NewTextHandler(&buf, nil))
+
+ err := RedactMailErrors(inner, logger).Send(context.Background(), address, "s", "b")
+ if err == nil {
+ t.Fatal("a failed send answered nil")
+ }
+ if strings.Contains(strings.ToLower(err.Error()), "guest") {
+ t.Fatalf("the returned error carries the address: %q", err)
+ }
+ if err.Error() != "meet: mail delivery failed" {
+ t.Fatalf("returned error is %q", err)
+ }
+ if got := strings.ToLower(buf.String()); strings.Contains(got, "guest") {
+ t.Fatalf("the log line carries the address:\n%s", buf.String())
+ }
+ if !strings.Contains(buf.String(), "[address]") {
+ t.Fatalf("the log line does not say where the address was:\n%s", buf.String())
+ }
+ if !strings.Contains(buf.String(), "meet.smtpish") {
+ t.Fatalf("the log line does not name the error's type:\n%s", buf.String())
+ }
+}
+
+func TestASuccessfulSendIsUntouched(t *testing.T) {
+ var buf bytes.Buffer
+ logger := slog.New(slog.NewTextHandler(&buf, nil))
+ if err := RedactMailErrors(failingSender{}, logger).Send(context.Background(), "a@b.test", "s", "b"); err != nil {
+ t.Fatal(err)
+ }
+ if buf.Len() != 0 {
+ t.Fatalf("a successful send logged:\n%s", buf.String())
+ }
+ if RedactMailErrors(nil, logger) != nil {
+ t.Fatal("a nil mailer was wrapped into a non-nil one, which would defeat the production refusal")
+ }
+}
+
+func TestRedactAddressLeavesTheRestAlone(t *testing.T) {
+ got := redactAddress("send to a@b.test: dial a@b.test refused", "A@B.test")
+ want := "send to [address]: dial [address] refused"
+ if got != want {
+ t.Fatalf("redactAddress = %q, want %q", got, want)
+ }
+ if got := redactAddress("boom", ""); got != "boom" {
+ t.Fatalf("an empty address redacted %q", got)
+ }
+}
diff --git a/internal/meet/store.go b/internal/meet/store.go
index 10469f7..637d279 100644
--- a/internal/meet/store.go
+++ b/internal/meet/store.go
@@ -248,6 +248,41 @@ func (s *store) mintGrant(ctx context.Context, m Meeting, personID int64) (Guest
return g, err
}
+// mintGrantViaLink mints a grant for the link the browser actually
+// followed, re-reading that link inside the grant's own transaction
+// (review C1-3). followLink resolves the link and then mints; between
+// those two steps a member can replace the link, and mintGrant on its
+// own would never notice, because it is handed the meeting rather than
+// the link. Standing checks the grant and not its origin, so such a
+// grant would stay usable for its full two hours.
+//
+// It answers false — no error — when the link has been revoked or its
+// meeting has ended, which the caller turns into the same 404 an unknown
+// token gets (spec §3.2's one 404).
+//
+// SQLite's writer pool is one connection, so the transaction's read and
+// its write cannot be interleaved with another transaction's.
+func (s *store) mintGrantViaLink(ctx context.Context, linkID, personID int64) (GuestGrant, bool, error) {
+ var g GuestGrant
+ live := false
+ err := s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+ ts := &store{g: tx, sealer: s.sealer, now: s.now, logger: s.logger}
+ _, m, ok, err := ts.liveLinkByID(ctx, linkID)
+ if err != nil || !ok {
+ return err
+ }
+ if g, err = ts.mintGrant(ctx, m, personID); err != nil {
+ return err
+ }
+ live = true
+ return nil
+ })
+ if err != nil {
+ return GuestGrant{}, false, err
+ }
+ return g, live, nil
+}
+
func (s *store) fingerprint(address string) (string, error) {
return s.sealer.Fingerprint(seal.PurposePendingAddress, normalizeAddress(address))
}
@@ -301,17 +336,28 @@ func (s *store) deletePending(ctx context.Context, address string) error {
// GET /j/{token} would, mints the grant for a non-member, and deletes
// every pending row for the address, all in one transaction. It returns
// the holding-screen path, or "" when the re-check failed.
+//
+// The row is re-selected inside the transaction and the caller's value
+// is only trusted when it is still the newest unexpired one (review
+// C1-4). latestPending runs before the transaction opens, so by the time
+// we get here the row may have been consumed by a concurrent /continue,
+// or superseded by a newer request from another browser. A stale value
+// must not mint — and must not delete the row that superseded it, so a
+// mismatch deletes nothing at all and the caller falls through to / or
+// /welcome. Expiry needs no separate check: latestPending already
+// excludes expired rows, and the sweeper removes them.
func (s *store) consumePending(ctx context.Context, p Person, pend PendingJoin, member bool) (string, error) {
dest := ""
err := s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
ts := &store{g: tx, sealer: s.sealer, now: s.now, logger: s.logger}
- if err := ts.deletePending(ctx, p.Email); err != nil {
+ current, ok, err := ts.latestPending(ctx, p.Email)
+ if err != nil || !ok || current.ID != pend.ID {
return err
}
- if !pend.ExpiresAt.After(s.now()) {
- return nil
+ if err := ts.deletePending(ctx, p.Email); err != nil {
+ return err
}
- _, m, live, err := ts.liveLinkByID(ctx, pend.LinkID)
+ _, m, live, err := ts.liveLinkByID(ctx, current.LinkID)
if err != nil || !live {
return err
}
@@ -325,3 +371,11 @@ func (s *store) consumePending(ctx context.Context, p Person, pend PendingJoin,
})
return dest, err
}
+
+// sweep deletes pending rows whose fifteen minutes are up. Reads already
+// exclude them; this is about not keeping a record past its life
+// (review C1-7). It returns how many rows went.
+func (s *store) sweep(ctx context.Context, now time.Time) (int64, error) {
+ res := s.g.WithContext(ctx).Where("expires_at <= ?", now.UTC()).Delete(&PendingJoin{})
+ return res.RowsAffected, res.Error
+}
diff --git a/internal/meet/store_test.go b/internal/meet/store_test.go
index ce8f125..04ff94f 100644
--- a/internal/meet/store_test.go
+++ b/internal/meet/store_test.go
@@ -101,6 +101,139 @@ func TestGrantExpiresTwoHoursAfterStartOrCreation(t *testing.T) {
}
}
+// Review C1-3: followLink reads the link, then mints. A member may
+// replace the link in between, and the grant must not be minted through
+// the link that no longer exists.
+func TestAReplacedLinkMintsNoGrant(t *testing.T) {
+ s, _ := testStore(t)
+ ctx := context.Background()
+ m, tok, _ := s.createMeeting(ctx, "x", nil, "person:1")
+ old, _, _, _ := s.liveLink(ctx, tok)
+ if _, err := s.replaceLink(ctx, m.ID, "person:1"); err != nil {
+ t.Fatal(err)
+ }
+ g, live, err := s.mintGrantViaLink(ctx, old.ID, 7)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if live {
+ t.Fatal("a replaced link answered live")
+ }
+ if g.ID != 0 {
+ t.Fatal("a replaced link minted a grant")
+ }
+ if _, ok, _ := s.grantFor(ctx, m.ID, 7); ok {
+ t.Fatal("a grant row exists after minting through a replaced link")
+ }
+
+ // The link that replaced it still mints, once.
+ l2 := MeetingLink{}
+ if err := s.g.Where("meeting_id = ? AND revoked_at IS NULL", m.ID).Take(&l2).Error; err != nil {
+ t.Fatal(err)
+ }
+ g1, live, err := s.mintGrantViaLink(ctx, l2.ID, 7)
+ if err != nil || !live {
+ t.Fatalf("live link: live=%v err=%v", live, err)
+ }
+ g2, _, err := s.mintGrantViaLink(ctx, l2.ID, 7)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if g1.ID != g2.ID {
+ t.Fatal("minting twice through the link made a second grant")
+ }
+}
+
+// Review C1-4: latestPending runs before consumePending's transaction,
+// so the row the caller holds may already be stale. A stale row must
+// never mint, and must not delete the row that superseded it.
+func TestAStalePendingRowMintsNothingAndDeletesNothing(t *testing.T) {
+ s, now := testStore(t)
+ ctx := context.Background()
+ m, tok, _ := s.createMeeting(ctx, "x", nil, "person:1")
+ l, _, _, _ := s.liveLink(ctx, tok)
+ const address = "guest@example.com"
+
+ if err := s.writePending(ctx, address, l); err != nil {
+ t.Fatal(err)
+ }
+ older, ok, err := s.latestPending(ctx, address)
+ if err != nil || !ok {
+ t.Fatalf("first pending row: ok=%v err=%v", ok, err)
+ }
+ *now = now.Add(time.Second)
+ if err := s.writePending(ctx, address, l); err != nil {
+ t.Fatal(err)
+ }
+ newer, _, _ := s.latestPending(ctx, address)
+ if newer.ID == older.ID {
+ t.Fatal("the second write did not make a second row")
+ }
+
+ p := Person{ID: 7, Email: address}
+ dest, err := s.consumePending(ctx, p, older, false)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if dest != "" {
+ t.Fatalf("a stale pending row led to %q", dest)
+ }
+ if _, ok, _ := s.grantFor(ctx, m.ID, p.ID); ok {
+ t.Fatal("a stale pending row minted a grant")
+ }
+ var n int64
+ if err := s.g.Model(&PendingJoin{}).Count(&n).Error; err != nil {
+ t.Fatal(err)
+ }
+ if n != 2 {
+ t.Fatalf("pending rows after a stale consume: %d, want 2", n)
+ }
+
+ // The newest row still works, and takes both rows with it.
+ if dest, err := s.consumePending(ctx, p, newer, false); err != nil || dest != "/m/"+m.Sqid+"/join" {
+ t.Fatalf("the newest row led to %q (err %v)", dest, err)
+ }
+ if err := s.g.Model(&PendingJoin{}).Count(&n).Error; err != nil {
+ t.Fatal(err)
+ }
+ if n != 0 {
+ t.Fatalf("pending rows after consuming: %d, want 0", n)
+ }
+}
+
+// Review C1-7: expired rows were excluded from reads but never deleted.
+func TestSweepDeletesOnlyExpiredPendingRows(t *testing.T) {
+ s, now := testStore(t)
+ ctx := context.Background()
+ _, tok, _ := s.createMeeting(ctx, "x", nil, "person:1")
+ l, _, _, _ := s.liveLink(ctx, tok)
+ if err := s.writePending(ctx, "stale@example.com", l); err != nil {
+ t.Fatal(err)
+ }
+ *now = now.Add(pendingLife + time.Minute)
+ if err := s.writePending(ctx, "live@example.com", l); err != nil {
+ t.Fatal(err)
+ }
+ n, err := s.sweep(ctx, *now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if n != 1 {
+ t.Fatalf("swept %d rows, want 1", n)
+ }
+ var left []PendingJoin
+ if err := s.g.Find(&left).Error; err != nil {
+ t.Fatal(err)
+ }
+ if len(left) != 1 {
+ t.Fatalf("%d rows left, want 1", len(left))
+ }
+ fp, _ := s.fingerprint("live@example.com")
+ if left[0].AddressFingerprint != fp {
+ t.Fatal("the sweep kept the wrong row")
+ }
+}
+
func TestPendingIsKeyedFingerprintAndFollowsItsLink(t *testing.T) {
s, now := testStore(t)
ctx := context.Background()
diff --git a/internal/meettest/holding_test.go b/internal/meettest/holding_test.go
index 99aa4e9..bf7041f 100644
--- a/internal/meettest/holding_test.go
+++ b/internal/meettest/holding_test.go
@@ -4,6 +4,7 @@ import (
"net/http"
"strings"
"testing"
+ "time"
)
func TestHoldingPageShowsTitleStandingAndFloor(t *testing.T) {
@@ -19,12 +20,33 @@ func TestHoldingPageShowsTitleStandingAndFloor(t *testing.T) {
}
}
+// A meeting that exists, a person who is signed in, and no standing
+// toward it. The earlier version of this test asked for a meeting that
+// did not exist, so it passed before standingFor was ever called
+// (review C1-8).
func TestHoldingPageIs404ForNoStanding(t *testing.T) {
- x := newHarness(t) // no team: everyone signs in, nobody is a member
- c := x.browser(t)
- c.signIn("nobody@example.com")
- if rec := c.get("/m/2345678abc/join"); rec.Code != http.StatusNotFound {
- t.Fatalf("no standing: %d", rec.Code)
+ x, first, tok := memberWithMeeting(t)
+
+ // A signed-in non-member: an outsider who followed the first
+ // meeting's link, so they hold a grant for that meeting and none
+ // for any other.
+ g := x.browser(t)
+ g.get("/j/" + tok)
+ if dest := g.signIn("guest@example.com"); dest != "/m/"+first+"/join" {
+ t.Fatalf("outsider landed on %s", dest)
+ }
+
+ member := x.browser(t)
+ member.signIn("paul@example.com")
+ second, _ := createMeeting(t, member, "A meeting they were not invited to", "")
+
+ if rec := g.get("/m/" + second + "/join"); rec.Code != http.StatusNotFound {
+ t.Fatalf("a real meeting with no grant: %d, want 404", rec.Code)
+ }
+ // And the meeting they do hold a grant for, once it has run out.
+ x.advance(2*time.Hour + time.Minute)
+ if rec := g.get("/m/" + first + "/join"); rec.Code != http.StatusNotFound {
+ t.Fatalf("a real meeting with an expired grant: %d, want 404", rec.Code)
}
}
diff --git a/internal/meettest/join_test.go b/internal/meettest/join_test.go
index a674857..212f948 100644
--- a/internal/meettest/join_test.go
+++ b/internal/meettest/join_test.go
@@ -3,6 +3,7 @@ package meettest
import (
"net/http"
"net/url"
+ "strconv"
"strings"
"testing"
"time"
@@ -17,11 +18,36 @@ func memberWithMeeting(t *testing.T) (*harness, string, string) {
return x, sqid, tok
}
+// Spec §3.2's one 404: unknown, revoked and ended are not merely all
+// 404, they are the same 404 — the same status and the same bytes, so
+// the response tells an outsider nothing about which it was.
func TestUnknownRevokedAndEndedLinksAreOne404(t *testing.T) {
- x, _, _ := memberWithMeeting(t)
- rec := x.browser(t).get("/j/" + strings.Repeat("A", 43))
- if rec.Code != http.StatusNotFound {
- t.Fatalf("unknown: %d", rec.Code)
+ x, replaced, revoked := memberWithMeeting(t)
+ c := x.browser(t)
+ c.signIn("paul@example.com")
+ if rec := c.post("/meetings/"+replaced+"/links", nil); rec.Code != http.StatusOK {
+ t.Fatalf("replace link: %d", rec.Code)
+ }
+ ended, endedTok := createMeeting(t, c, "over", "")
+ if rec := c.post("/meetings/"+ended+"/end", nil); rec.Code != http.StatusSeeOther {
+ t.Fatalf("end meeting: %d", rec.Code)
+ }
+
+ bodies := map[string]string{}
+ for _, tc := range []struct{ name, token string }{
+ {"unknown", strings.Repeat("A", 43)},
+ {"revoked", revoked},
+ {"ended", endedTok},
+ } {
+ rec := x.browser(t).get("/j/" + tc.token)
+ if rec.Code != http.StatusNotFound {
+ t.Fatalf("%s: %d, want 404", tc.name, rec.Code)
+ }
+ bodies[tc.name] = rec.Body.String()
+ }
+ if bodies["revoked"] != bodies["unknown"] || bodies["ended"] != bodies["unknown"] {
+ t.Fatalf("the three 404s differ:\nunknown: %q\nrevoked: %q\nended: %q",
+ bodies["unknown"], bodies["revoked"], bodies["ended"])
}
}
@@ -150,6 +176,121 @@ func TestMemberAddressExhaustsTheSharedLimiterWithTheSameAnswer(t *testing.T) {
}
}
+// Review C1-8: the twenty-per-client allowance, not the five-per-address
+// one. rastrillo's limiter is keyed on RemoteAddr, which
+// httptest.NewRequest fixes at 192.0.2.1:1234, so every harness client
+// shares one budget. Twenty-one different member addresses therefore
+// exhaust it even though no address is near its own five.
+func TestSharedClientAllowanceIsExhaustedByOneMemberAddress(t *testing.T) {
+ const clientBudget = 20 // signin.Flow.IPBudget's default
+ members := make([]string, 0, clientBudget+1)
+ for i := 0; i <= clientBudget; i++ {
+ members = append(members, "member"+strconv.Itoa(i)+"@example.com")
+ }
+ x := newHarness(t, members...)
+
+ for i := 0; i < clientBudget; i++ {
+ rec := x.browser(t).post("/signin", url.Values{"address": {members[i]}})
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("attempt %d: %d %s, want 303 /signin?sent=1", i+1, rec.Code, rec.Header().Get("Location"))
+ }
+ }
+ rec := x.browser(t).post("/signin", url.Values{"address": {members[clientBudget]}})
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?err=rate" {
+ t.Fatalf("attempt %d: %d %s, want 303 /signin?err=rate",
+ clientBudget+1, rec.Code, rec.Header().Get("Location"))
+ }
+ if got := x.mail.count(); got != clientBudget {
+ t.Fatalf("mails sent: %d, want %d", got, clientBudget)
+ }
+}
+
+// Spec §3.2: a member who followed a link while signed out reaches the
+// holding screen in whichever browser the emailed link is opened — by
+// the same pending row an outsider uses. The existing member test opens
+// the link in the browser that asked for it (review C1-8).
+func TestSignedOutMemberCompletesInAnotherBrowser(t *testing.T) {
+ x, sqid, tok := memberWithMeeting(t)
+ desktop := x.browser(t)
+ desktop.get("/j/" + tok)
+ rec := desktop.post("/signin", url.Values{"address": {"paul@example.com"}})
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("member's request: %d %s", rec.Code, rec.Header().Get("Location"))
+ }
+ to, link := x.mail.last()
+ if to != "paul@example.com" || link == "" {
+ t.Fatalf("no magic link mailed to the member (last to %q)", to)
+ }
+ phone := x.browser(t) // no session, no pending-join cookie
+ if dest := phone.follow(link); dest != "/m/"+sqid+"/join" {
+ t.Fatalf("other-browser click landed on %s, want the holding page", dest)
+ }
+ if rec := phone.get("/m/" + sqid + "/join"); rec.Code != http.StatusOK {
+ t.Fatalf("holding page in the second browser: %d", rec.Code)
+ }
+}
+
+// Spec §3.2's two refusals: a signed-in person whose pending row fails
+// the /continue re-check gets / or /welcome, and neither path mints a
+// grant. The old test could only observe the redirect; this one asserts
+// the absence of the grant by asking for the holding page (review C1-8).
+func TestReplacedLinkFallbackForSignedInPerson(t *testing.T) {
+ t.Run("member lands on the list", func(t *testing.T) {
+ x, sqid, tok := memberWithMeeting(t)
+ owner := x.browser(t)
+ owner.signIn("paul@example.com")
+
+ // A pending row written after the session exists, from another
+ // browser, so signing in did not already consume it.
+ other := x.browser(t)
+ other.get("/j/" + tok)
+ if rec := other.post("/signin", url.Values{"address": {"paul@example.com"}}); rec.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("member's request: %s", rec.Header().Get("Location"))
+ }
+ if rec := owner.post("/meetings/"+sqid+"/links", nil); rec.Code != http.StatusOK {
+ t.Fatalf("replace link: %d", rec.Code)
+ }
+ rec := owner.get("/continue")
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/" {
+ t.Fatalf("member after a replaced link: %d %s, want 303 /", rec.Code, rec.Header().Get("Location"))
+ }
+ })
+
+ t.Run("non-member lands on welcome with no grant", func(t *testing.T) {
+ x, first, firstTok := memberWithMeeting(t)
+ guest := x.browser(t)
+ guest.get("/j/" + firstTok)
+ guest.signIn("guest@example.com") // signed in, and granted the first meeting
+
+ owner := x.browser(t)
+ owner.signIn("paul@example.com")
+ second, secondTok := createMeeting(t, owner, "second", "")
+
+ // The pending row for the second meeting, from a browser with no
+ // session, so the guest's own session is untouched.
+ other := x.browser(t)
+ other.get("/j/" + secondTok)
+ if rec := other.post("/signin", url.Values{"address": {"guest@example.com"}}); rec.Header().Get("Location") != "/signin?sent=1" {
+ t.Fatalf("outsider's request: %s", rec.Header().Get("Location"))
+ }
+ if rec := owner.post("/meetings/"+second+"/links", nil); rec.Code != http.StatusOK {
+ t.Fatalf("replace link: %d", rec.Code)
+ }
+
+ rec := guest.get("/continue")
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/welcome" {
+ t.Fatalf("non-member after a replaced link: %d %s, want 303 /welcome", rec.Code, rec.Header().Get("Location"))
+ }
+ if rec := guest.get("/m/" + second + "/join"); rec.Code != http.StatusNotFound {
+ t.Fatalf("holding page of the second meeting: %d, want 404 — a grant was minted", rec.Code)
+ }
+ // The grant they legitimately hold is untouched.
+ if rec := guest.get("/m/" + first + "/join"); rec.Code != http.StatusOK {
+ t.Fatalf("holding page of the first meeting: %d, want 200", rec.Code)
+ }
+ })
+}
+
func TestExpiredPendingRowAdmitsNobody(t *testing.T) {
x, _, tok := memberWithMeeting(t)
g := x.browser(t)
diff --git a/internal/meettest/meetings_test.go b/internal/meettest/meetings_test.go
index b783cb1..83dbb77 100644
--- a/internal/meettest/meetings_test.go
+++ b/internal/meettest/meetings_test.go
@@ -111,9 +111,15 @@ func TestTheDatabaseNeverHoldsAPlaintextTitle(t *testing.T) {
grep := func(when string) {
for _, suffix := range []string{"", "-wal", "-shm"} {
b, err := os.ReadFile(x.dbPath + suffix)
- if err != nil {
+ // A missing -wal or -shm is ordinary; any other read error
+ // would let this test pass without reading anything, which
+ // is the one way a privacy gate can lie (review C1-8).
+ if os.IsNotExist(err) {
continue
}
+ if err != nil {
+ t.Fatalf("%s: reading %s: %v", when, filepath.Base(x.dbPath+suffix), err)
+ }
if bytes.Contains(b, []byte(secret)) {
t.Errorf("%s: title in plaintext in %s", when, filepath.Base(x.dbPath+suffix))
}