amadan
fix: redaction without message text, sweeper lifecycle and platform wake, review follow-ups
Closes the internal review of 5fdddcf and Codex round 2 (codex-code-r2.md): - Internal 1 / C2-1: mail-error logging drops err.Error() entirely, logging only the root cause's type and an address_seen bool computed with strings.Contains over lowered strings (no slicing, no rune trap). redactAddress and its byte-offset substitution are gone. - Internal 2 / Internal 3: every mailer is wrapped inside resolve, not at the main.go call site; RedactMailErrors forwards Delivers() and SendList when the inner sender has them. New production-path wiring test drives a real POST /signin over a failing Config.Mailer and checks a captured Config.Logger for any trace of the address. - Internal 4 / Internal 5 / C2-3: build takes a ctx and bounds the boot sweep to 30s; the hourly ticker is started only by App and exits on ctx.Done(); NewForTest exposes SweepForTest(now time.Time), driving store.sweep and auth.Sweep from one given clock instead of two. - C2-2: store.nextExpiry backs a new Runtime.NextDue that App returns alongside the mux; cmd/meet/main.go wires it to rastrillo's Options.NextDue so a platform-hibernated instance is woken before an unclicked link or pending row would otherwise sit unswept. - Bounded sweep: store.sweep deletes at most 1000 pending_joins rows a call via a subselect on ids; the hourly pass loops while a call returns exactly that many. - Internal 6 / Internal 7 / C2-4: cross-meeting stale-pending coverage, the negative grantFor lookup's error, and the primary-database-must- exist tightening on the plaintext-title test. The shared-limiter rename and the signed-in-non-member-with-an-expired-row /continue case were already in the working tree from an interrupted pass and are kept as found. - docs/superpowers/reviews/2026-09-09-codex-code-r1.md: appended a Round 2 section with the verbatim Codex text and a C2-1..C2-4 dispositions table; C2-4's transaction-interleaving and concurrent- budget coverage is recorded as not done, not claimed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
15 files changed,
+807
−102
cmd/meet/main.go+14 −6docs/superpowers/reviews/2026-09-09-codex-code-r1.md+135 −0internal/meet/app.go+130 −25internal/meet/auth_test.go+3 −3internal/meet/config.go+8 −2internal/meet/mailredact.go+89 −28internal/meet/mailredact_test.go+130 −19internal/meet/store.go+30 −4internal/meet/store_test.go+94 −2internal/meettest/config_test.go+4 −3internal/meettest/harness_test.go+26 −4internal/meettest/join_test.go+77 −1internal/meettest/meetings_test.go+7 −4internal/meettest/schema_test.go+2 −1internal/meettest/signin_test.go+58 −0
CI passed — run details
diff --git a/cmd/meet/main.go b/cmd/meet/main.go| index 7ff70f5..a2f1ef6 100644 |
| --- a/cmd/meet/main.go |
| +++ b/cmd/meet/main.go |
| @@ -1,6 +1,7 @@ |
| package main |
| import ( |
| + "context" |
| "fmt" |
| "log/slog" |
| "os" |
| @@ -42,11 +43,10 @@ func main() { |
| } |
| defer d.Close() |
| - // 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) |
| + // Raw: meet.resolve wraps every mailer in RedactMailErrors, so the |
| + // invariant holds for whatever a caller passes, not only for what |
| + // this main happens to build (review C1-2). |
| + mailer := newMailer(logger) |
| cfg := meet.Config{ |
| Env: *env, |
| @@ -60,7 +60,10 @@ func main() { |
| Logger: logger, |
| } |
| - mux, err := meet.App(d, cfg) |
| + // context.Background(): the hourly sweep this starts runs for the |
| + // life of the process, and nothing here ever needs to cancel it |
| + // early (review C2-3 — "acceptable" per the brief). |
| + mux, rt, err := meet.App(context.Background(), d, cfg) |
| if err != nil { |
| logger.Error("build app", "err", err) |
| os.Exit(1) |
| @@ -68,6 +71,11 @@ func main() { |
| opts.Mux = mux |
| opts.DBPath = "" |
| + // NextDue answers the platform's scheduled-wake poll: a magic link |
| + // or pending-join row can outlive an instance's hibernation, and |
| + // without this the platform has no way to know it should wake the |
| + // app to clear one (review C2-2). |
| + opts.NextDue = rt.NextDue |
| if err := rastrillo.Serve(opts); err != nil { |
| logger.Error("serve failed", "err", err) |
| os.Exit(1) |
diff --git a/docs/superpowers/reviews/2026-09-09-codex-code-r1.md b/docs/superpowers/reviews/2026-09-09-codex-code-r1.md| index 6a43c28..f371613 100644 |
| --- a/docs/superpowers/reviews/2026-09-09-codex-code-r1.md |
| +++ b/docs/superpowers/reviews/2026-09-09-codex-code-r1.md |
| @@ -135,3 +135,138 @@ 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. |
| + |
| + |
| +## Round 2 |
| + |
| +Reviewer: GPT-6 (Codex), read-only sandbox. Pinned to `3c105b5` (the |
| +`fix/codex-code-r1` branch after round 1's commit, 5fdddcf, plus an |
| +unrelated planning-doc commit). Verdict: request changes. Verbatim below. |
| + |
| +### The review, verbatim |
| + |
| +**Verdict: request changes.** Reviewed `3c105b50bac55197dba2840429e4078054b0ea86`. No files modified; no network used. Go tests were inspected, not executed in the read-only sandbox. |
| + |
| +**Round-1 findings, in order** |
| + |
| +| Finding | Status | Code assessment | |
| +|---|---|---| |
| +| C1-1 | **Half-closed; explicitly deferred** | `store.go:55` still writes plaintext email/name; `auth.go:23` still leaves `SubjectFor` unset. The controller resolved its scope for this wave, not the storage concern. Not an acceptance blocker here. | |
| +| C1-2 | **Half-closed** | The returned mail error is safe, and GORM parameter interpolation is disabled. Meet’s own redacted log can still disclose the address: C2-1. Captured GORM-log coverage is missing. | |
| +| C1-3 | **Closed in code** | `store.go:268` opens the transaction; the transaction’s store rereads the link and meeting before inserting the grant. `followLink` calls this method and checks both error and liveness. The resolver preserves transaction connections, so these reads do not escape to the reader pool. | |
| +| C1-4 | **Closed in code** | `store.go:353` reselects the newest unexpired pending row inside the transaction. Missing/mismatched rows return before deletion. The subsequent link lookup uses `current.LinkID`, not the caller’s stale value. | |
| +| C1-5 | **Closed in code** | `main.go:24` extracts the app flag and updates `os.Args` before `Resolve`. The competing global flag parser is gone. Executable-invocation regression tests remain absent. | |
| +| C1-6 | **Closed in code** | `main.go:84` implements per-setting CARLOS fallbacks, trims HOST/FROM and preserves credential whitespace. Missing HOST/FROM returns nil, preserving the production startup refusal. | |
| +| C1-7 | **Half-closed** | Pending deletion and `auth.Sweep` now run at boot and hourly while alive. Hibernation, cancellation and bounded execution remain unresolved: C2-2/C2-3. | |
| +| C1-8 | **Half-closed** | Several previously misleading tests now exercise the intended state. Required coverage remains incomplete, and some assertions still permit false confidence: C2-4 and the test assessment below. | |
| + |
| +**C2-1 — P1 — Unicode lowercasing can leave the complete recipient in the error log.** |
| + |
| +Location: [internal/meet/mailredact.go:79](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/mailredact.go:79), emitted at line 52. |
| + |
| +`strings.ToLower` can change UTF-8 byte lengths. The code finds an offset in the lowercase string, then uses that offset and the lowercase target length to slice the original string. |
| + |
| +A concrete error is: |
| + |
| +```text |
| +rastrillo/mail: send to guest@example.com: 550 KKKKKKKKK guest@example.com |
| +``` |
| + |
| +Each Kelvin sign occupies three bytes but lowercases to one-byte `k`. The first address is removed correctly. For the second occurrence, the calculated offset is eighteen bytes early; the replacement cuts through the preceding Unicode text and leaves the complete `guest@example.com` in the output. A read-only Python reproduction of these byte operations confirmed this; Go’s local `strings.ToLower` source confirms the relevant Unicode mapping behavior. |
| + |
| +The downstream path is otherwise sound: `Send` returns a fresh `errors.New`, without `Unwrap`; signin returns it directly; rastrillo’s `auth/handlers.go:51` logs that safe error. Neither `%v` of that returned error nor `%T` of the original root cause exposes the original message. **The remaining leak is Meet’s own line 52.** |
| + |
| +Correction: stop logging arbitrary SMTP error text, retaining a safe error type/status, or use matching that returns offsets into the original string. Add this Unicode regression and capture both Meet’s and rastrillo’s actual log lines. |
| + |
| +**C2-2 — P2 — Expiry cleanup has no scheduled wake under hibernation.** |
| + |
| +Locations: [internal/meet/app.go:124](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/app.go:124), `cmd/meet/main.go:69`. |
| + |
| +The only recurring trigger is an in-process ticker. Main configures neither `Options.NextDue` nor scheduled cleanup. Rastrillo’s `serve.go:122` explicitly says an unset `NextDue` means the activator sees no schedule. |
| + |
| +A link can be issued, the app can hibernate before its fifteen-minute expiry, and its plaintext `auth_links.address` can remain indefinitely without another activation. Boot cleanup removes it on the next launch, but cannot impose a retention bound while asleep. |
| + |
| +Correction: arrange a platform wake for cleanup, with a bounded cleanup pass on activation and a recurring trigger for standalone serving. Test an unclicked link across shutdown, expiry and scheduled activation. |
| + |
| +**C2-3 — P3 — Each app construction starts an immortal, uncancellable sweeper.** |
| + |
| +Location: [internal/meet/app.go:67](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/app.go:67). |
| + |
| +`sweepForever` has no exit condition. Its deferred `t.Stop()` is unreachable during ordinary application teardown. Every `build`, including every test harness, retains the app, logger, database and ticker after callers close the database. An hourly tick subsequently queries those closed pools. |
| + |
| +The pass also lacks an execution deadline. Pending deletion receives `context.Background`; rastrillo’s `Auth.Sweep` uses context-free `Exec`. Failures are logged and retried next hour, which is useful, but a blocked pass has no application cancellation path. The deletions also process all expired rows in one pass. |
| + |
| +Correction: give background work an explicit owner, cancellation and completion wait; stop it before closing the database. Bound cleanup execution and large backlogs. Test shutdown and cleanup-error behavior. |
| + |
| +**C2-4 — P2 — The regression tests do not yet establish the requested closure.** |
| + |
| +Locations: [internal/meet/store_test.go:150](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/store_test.go:150), `internal/meet/mailredact_test.go:22`, `internal/meettest/join_test.go:294`, `internal/meettest/meetings_test.go:117`. |
| + |
| +Material gaps remain: |
| + |
| +- The link and pending tests are sequential. Moving their rechecks outside the transaction would still pass them. The pending test uses one meeting and never retries an already-consumed row after another meeting’s continuation. |
| +- No captured-log test exercises slow/failed person queries. The mail test calls the wrapper directly, so removing its production wiring would leave it green. |
| +- The sweep test only calls `store.sweep`. Removing `auth.Sweep` entirely would leave the new tests green. |
| +- An already-signed-in non-member with an expired pending row still lacks a `/continue` test. The existing expiry test stops at `Authorize`. |
| +- The privacy test skips `IsNotExist` for the primary database as well as optional sidecars. A wrong database path can therefore still produce a vacuous pass. |
| + |
| +Correction: add controlled transaction interleavings, cross-meeting consumed-row coverage, captured production-path logs, auth cleanup and authenticated expired-pending tests. Require the primary database file to exist; ignore missing files only for optional sidecars. Concurrent grant and budget tests requested in round 1 also remain outstanding. |
| + |
| +**GORM logging assessment** |
| + |
| +The `ParameterizedQueries` change is effective for the inspected request paths. GORM calls `ParamsFilter` before `Dialector.Explain`; the new logger removes the values. Transaction stores inherit that logger. Auth links and sessions use `database/sql` directly, without query tracing. |
| + |
| +GORM still formats database errors with `%v`, and Meet logs returned store errors. `ParameterizedQueries` does not sanitize those messages. I found no concrete address-bearing database-error path in the current queries and schema, so this is not a separate finding. The missing captured-log regression prevents claiming tested closure. |
| + |
| +**Each new or changed test** |
| + |
| +| Test | What it actually establishes | |
| +|---|---| |
| +| `TestMailErrorsCarryNoAddress` | Correctly checks repeated, mixed-case ASCII addresses in a wrapped error, returned error and captured wrapper log. Misses C2-1 and framework/production wiring. | |
| +| `TestASuccessfulSendIsUntouched` | Checks nil return, no logging and nil-sender preservation. Does not verify that context, recipient, subject and body reach the inner sender unchanged. | |
| +| `TestRedactAddressLeavesTheRestAlone` | Checks repeated ASCII matching and empty-address handling. Does not establish Unicode-safe preservation. | |
| +| `TestAuthorizeAdmitsMembersAndLivePendingRowsOnly` | Directly exercises unknown/member/live-pending/revoked cases. Does not cover pending expiry or meeting end independently. | |
| +| `TestStandingTable` | Sound six-case member/guest/absent/expired table; setup errors are checked. | |
| +| `TestAReplacedLinkMintsNoGrant` | Proves sequential rejection and sequential idempotency. Does not prove transactional placement or concurrent replacement. Its negative grant lookup discards errors. | |
| +| `TestAStalePendingRowMintsNothingAndDeletesNothing` | Proves a superseded ID is rejected and preserves both pending rows. Does not cover the original cross-meeting, already-consumed sequence; the successful path does not directly assert the grant row. | |
| +| `TestSweepDeletesOnlyExpiredPendingRows` | Proves deletion of one expired pending row and preservation of one live row. Does not cover exact expiry, auth records, scheduling or failures. | |
| +| `TestHoldingPageIs404ForNoStanding` | Now exercises an authenticated guest against a real unrelated meeting, then an expired grant. The original nonexistent-meeting false pass is fixed. | |
| +| `TestUnknownRevokedAndEndedLinksAreOne404` | Proves status/body equality for the three HTTP scenarios. Ending also revokes the token, so it cannot independently detect removal of the meeting-ended check. | |
| +| `TestSharedClientAllowanceIsExhaustedByOneMemberAddress` | Correctly isolates the shared client limit using **21 different addresses**. Its name and the specified repeated-single-member scenario remain inaccurate/uncovered. | |
| +| `TestSignedOutMemberCompletesInAnotherBrowser` | Establishes the intended cross-browser member journey and successful holding-page access. | |
| +| `TestReplacedLinkFallbackForSignedInPerson` | Establishes both fallbacks and denied access to the guest’s second meeting while preserving access to the first. The member case never checks grant absence; the guest check establishes no usable grant, not direct row absence. | |
| +| `TestTheDatabaseNeverHoldsAPlaintextTitle` | Now fails ordinary read errors and scans before/after checkpoint. Still skips a missing primary database. | |
| + |
| +The transaction fixes are sound on inspection. Address redaction and hibernating cleanup still need correction before acceptance. |
| + |
| +### Dispositions, C2-1 to C2-4 |
| + |
| +| Finding | Disposition | |
| +|---|---| |
| +| C2-1 — `strings.ToLower` offsets can leak a fragment of the address | **Closed.** `internal/meet/mailredact.go` no longer touches the message text at all beyond testing it: `redact` logs only the root cause's `%T` and a bool, `address_seen`, from `addressSeen(err.Error(), to)` — `strings.Contains(strings.ToLower(msg), strings.ToLower(address))`, no offsets and no slicing. `redactAddress` and its "\[address\]"-substitution approach are gone entirely. `TestNoFragmentOfAnAddressReachesTheLog` (`internal/meet/mailredact_test.go`) puts `Kelvin@example.com` (plain ASCII K) and `İnes@example.com` (LATIN CAPITAL LETTER I WITH DOT ABOVE, U+0130) in one error message alongside a run of ordinary `KKK`s, and asserts the captured log holds no `@` and no fragment of either address — plus no U+FFFD, in case anything were still cutting mid-rune. | |
| +| C2-2 — no scheduled wake for expiry cleanup under hibernation | **Closed.** `store.nextExpiry(ctx)` (`internal/meet/store.go`) answers the earliest `pending_joins.expires_at`, or the zero time when none exists. `App`'s signature changed to `App(ctx, d, cfg) (*http.ServeMux, *Runtime, error)`; `Runtime.NextDue` is `a.nextDue`, which answers the earlier of `nextExpiry` and one hour from now, or the zero time when nothing is pending. `cmd/meet/main.go` sets `opts.NextDue = rt.NextDue`, matching `serve.go`'s contract: zero means nothing scheduled, and the returned time is read by the platform's `GET /api/next-due` poll. `TestNextExpiryIsTheEarliestExpiryNotTheLatestRow` (`internal/meet/store_test.go`) covers the store half by construction (a row written last, at the higher id, with an earlier expiry than the one before it) — nothing exercises the platform's own poll route, which is rastrillo's surface, not Meet's. | |
| +| C2-3 — an immortal, uncancellable sweeper per app construction | **Closed.** `build(ctx, d, cfg)` takes a context and bounds the boot sweep to it with a 30-second timeout (`context.WithTimeout`); the hourly ticker (`sweepForever`) is started only by `App`, never by `NewForTest`, and exits on `ctx.Done()`. `App`'s caller (`cmd/meet/main.go`) passes `context.Background()` — acceptable per the brief, since nothing in this app ever needs to cancel the sweep early — but the exit path exists for a caller that does. `NewForTest` returns a `*TestHandle` whose `SweepForTest(now time.Time)` runs one bounded pass (30-second timeout) with the given time for both `store.sweep` and `auth.Sweep`, so a test drives one clock, not two. `store.sweep` is now bounded to `sweepLimit` (1000) rows a call via a subselect on ids; the hourly pass (`sweepHourly`) loops while a call returns exactly that many, so a backlog larger than one page still clears within the hour rather than only ever removing its first 1000 rows. | |
| +| C2-4 — regression tests do not establish the requested closure | **Half-closed**, itemized: | |
| + |
| +| C2-4 sub-item | Now | |
| +|---|---| |
| +| Transaction fixes proven only sequentially, not against interleaving | **Not done.** No racing test was added for `mintGrantViaLink` or `consumePending`. Named as uncovered below, as round 1 also left it. | |
| +| Pending test uses one meeting; no cross-meeting retry of an already-consumed row | **Closed.** `TestConsumingOneMeetingsPendingRowLeavesAnothersIDStale` (`internal/meet/store_test.go`): pending rows for meetings A then B for one address; consuming B's (the newer) mints B's grant and deletes every pending row for the address, A's included; a subsequent `consumePending` with A's now-stale id answers no grant and no error. | |
| +| No captured-log test for the production wiring (mail test calls the wrapper directly) | **Closed.** `TestSigninOverAFailingMailerLeavesNoAddressInTheLog` (`internal/meettest/signin_test.go`) builds the app with `meet.App` — the same entry point `cmd/meet/main.go` calls — over a `Config.Mailer` that fails and a real `*slog.Logger` passed as `Config.Logger`, drives a real `POST /signin`, and asserts the captured log carries no fragment of the address. This is the check that `resolve` wraps every mailer regardless of call site (Internal 3), not just that the wrapper itself is safe. | |
| +| Sweep test only calls `store.sweep`; removing `auth.Sweep` would leave it green | **Closed.** `TestSweepClearsUnclickedMagicLinks` (`internal/meettest/join_test.go`) asserts `auth_links` directly: a sign-in request leaves one row, a sweep at +11 minutes (inside `auth_links`' fifteen-minute life) leaves it, and a sweep at +16 removes it. This exercises `auth.Sweep` specifically — deleting it would fail the +16 assertion. | |
| +| No `/continue` test for an already-signed-in non-member with an expired pending row | **Closed**, by the substitute construction the brief itself allowed for: `TestReplacedLinkFallbackForSignedInPerson`'s `"non-member with an expired row lands on welcome with no grant"` subtest (`internal/meettest/join_test.go`) signs the outsider in through meeting A, then has them `POST /signin` again for meeting B's link while already signed in (still writes a pending row), advances the clock sixteen minutes, and confirms `/continue` lands on `/welcome`, B's holding page 404s, and A's still 200s. | |
| +| Privacy test skips `IsNotExist` for the primary database too | **Closed.** `TestTheDatabaseNeverHoldsAPlaintextTitle` (`internal/meettest/meetings_test.go`) now only tolerates `os.IsNotExist` for the `-wal`/`-shm` suffixes; a missing primary file fails the test. | |
| +| `TestAReplacedLinkMintsNoGrant`'s negative lookup discards its error | **Closed** — `internal/meet/store_test.go` now checks `err` alongside `ok` on the `grantFor` call. | |
| +| Shared-limiter test's name and scenario | **Closed** (round 1's own residue, named again in C1-8's table): renamed to `TestSharedClientAllowanceIsExhaustedByOneClient`, with a comment that behind the platform edge `RemoteAddr` is the proxy, so this is a real, filed gap (the shared budget is exhaustible by any twenty visitors of any team), not a quirk of the harness. | |
| + |
| +Not covered, named rather than claimed: C2-4's core ask — controlled |
| +transaction interleavings for `mintGrantViaLink` and `consumePending`, |
| +and concurrent budget consumption — remains untested. Every invariant |
| +those paths rely on is enforced by the transaction boundary plus |
| +SQLite's single writer connection (`SetMaxOpenConns(1)`), not by a |
| +racing Go test; a test that actually raced two goroutines against it |
| +would be exercising the sqlite driver and the OS scheduler, not Meet's |
| +logic. This is the same position round 1 took and round 2 flagged as |
| +still open; it is being named again rather than resolved, because doing |
| +it properly needs a harness for deterministic interleaving (e.g. hooks |
| +around the transaction, or a mock DB) that does not exist yet. |
diff --git a/internal/meet/app.go b/internal/meet/app.go| index 827de3c..974002f 100644 |
| --- a/internal/meet/app.go |
| +++ b/internal/meet/app.go |
| @@ -30,9 +30,24 @@ type app struct { |
| outsiderBudget *budget |
| } |
| -// SetClockForTest replaces the store's clock; the harness uses it to |
| -// expire grants and pending rows without sleeping. |
| -func (a *app) SetClockForTest(now func() time.Time) { a.store.now = now } |
| +// TestHandle is the test-only control surface NewForTest hands back: |
| +// the app's clock, and the sweep that would otherwise only run on a |
| +// timer. Tests only; nothing in production holds one. |
| +type TestHandle struct{ a *app } |
| + |
| +// SetClock replaces the store's clock, so a test can expire grants, |
| +// pending rows and magic links without sleeping. |
| +func (h *TestHandle) SetClock(now func() time.Time) { h.a.store.now = now } |
| + |
| +// SweepForTest runs one bounded pass of what the hourly ticker runs, |
| +// using now for both store.sweep and auth.Sweep — Internal 5's one |
| +// clock for the whole pass, given explicitly rather than read back off |
| +// the store, so a test does not have to keep the two in step by hand. |
| +func (h *TestHandle) SweepForTest(now time.Time) { |
| + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
| + defer cancel() |
| + h.a.sweepOnce(ctx, 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 |
| @@ -49,33 +64,79 @@ func (w gormWriter) Printf(format string, args ...any) { |
| 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 { |
| +// auth links and sessions (review C1-7), one bounded call to |
| +// store.sweep — not looped, so a caller with a deadline (the boot sweep, |
| +// SweepForTest) gets one page of work done and returns. 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. |
| +// |
| +// now drives both halves: one clock for the whole pass (Internal 5), so |
| +// a test that advances Meet's clock expires rastrillo's auth_links rows |
| +// too. In production the caller passes the store's own clock, so it is |
| +// simply time.Now. |
| +func (a *app) sweepOnce(ctx context.Context, now time.Time) { |
| + if n, err := a.store.sweep(ctx, 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 { |
| + if err := a.auth.Sweep(now); err != nil { |
| + a.logger.Error("meet: sweep auth links and sessions", "err", err) |
| + } |
| +} |
| + |
| +// sweepHourly is what the ticker runs: store.sweep is bounded to |
| +// sweepLimit rows a call (review C2-3), so a backlog bigger than that |
| +// needs more than one call to actually clear it — the loop the boot |
| +// sweep and SweepForTest deliberately do not do, because a deadline- |
| +// bound or test-driven caller wants one page of work, not "until done". |
| +func (a *app) sweepHourly(ctx context.Context) { |
| + now := a.store.now() |
| + for { |
| + n, err := a.store.sweep(ctx, now) |
| + if err != nil { |
| + a.logger.Error("meet: sweep pending joins", "err", err) |
| + break |
| + } |
| + if n > 0 { |
| + a.logger.Info("meet: swept pending joins", "deleted", n) |
| + } |
| + if n < sweepLimit { |
| + break |
| + } |
| + } |
| + if err := a.auth.Sweep(now); err != nil { |
| a.logger.Error("meet: sweep auth links and sessions", "err", err) |
| } |
| } |
| -func (a *app) sweepForever() { |
| +// sweepForever is started by App and not by NewForTest: a test builds an |
| +// app per case, and an hourly ticker per case is a goroutine per case |
| +// that outlives it holding the store and the database open. It exits on |
| +// ctx.Done, so App's caller can stop it by cancelling the context it |
| +// passed in. |
| +func (a *app) sweepForever(ctx context.Context) { |
| t := time.NewTicker(sweepEvery) |
| defer t.Stop() |
| - for range t.C { |
| - a.sweepOnce(context.Background()) |
| + for { |
| + select { |
| + case <-ctx.Done(): |
| + return |
| + case <-t.C: |
| + a.sweepHourly(ctx) |
| + } |
| } |
| } |
| // 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. |
| -func build(d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| +// |
| +// ctx bounds the sweep at boot. The hourly ticker is not started here: |
| +// App starts it, NewForTest does not, so a test suite does not leak a |
| +// goroutine per app it builds. |
| +func build(ctx context.Context, d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| rc, err := resolve(cfg) |
| if err != nil { |
| return nil, nil, err |
| @@ -120,8 +181,12 @@ func build(d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| } |
| a.auth = authPlugin |
| - a.sweepOnce(context.Background()) |
| - go a.sweepForever() |
| + // Bounded, not open-ended: a boot sweep runs on the caller's ctx but |
| + // must still finish, so an app never fails to start (or hangs a |
| + // test) because of a stuck sweep (review C2-3). |
| + bootCtx, cancel := context.WithTimeout(ctx, 30*time.Second) |
| + a.sweepOnce(bootCtx, a.store.now()) |
| + cancel() |
| r := chi.NewRouter() |
| r.Use(csrf.Protect(rc.Origin)) |
| @@ -158,20 +223,60 @@ func build(d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| return mux, a, nil |
| } |
| +// Runtime is what App hands main beyond the mux: the platform's |
| +// scheduled-wake function (review C2-2). Nothing else lives here yet; |
| +// it is a struct rather than a bare func so a later addition (the |
| +// sidecar pass, a shutdown func) does not change App's signature again. |
| +type Runtime struct { |
| + // NextDue answers rastrillo.Options.NextDue: the next moment this |
| + // app has work, or the zero time when nothing is scheduled. |
| + NextDue func() time.Time |
| +} |
| + |
| +// nextDue is Runtime.NextDue's implementation: the earliest of the next |
| +// pending_joins.expires_at and one hour from now — the hourly sweep's |
| +// own interval, so the platform never lets the app sleep past a sweep |
| +// it would otherwise have run — or the zero time when no pending row |
| +// exists at all, meaning there is nothing to wake early for. |
| +func (a *app) nextDue() time.Time { |
| + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
| + defer cancel() |
| + exp, err := a.store.nextExpiry(ctx) |
| + if err != nil { |
| + a.logger.Error("meet: next due: read next expiry", "err", err) |
| + return time.Time{} |
| + } |
| + if exp.IsZero() { |
| + return time.Time{} |
| + } |
| + if hourly := a.store.now().Add(sweepEvery); hourly.Before(exp) { |
| + return hourly |
| + } |
| + return exp |
| +} |
| + |
| // App wires the whole app: refusals, schema, router, static files. It |
| // returns a *http.ServeMux because rastrillo.Options.Mux is typed that |
| -// way; the chi router mounts inside it. |
| -func App(d *db.DB, cfg Config) (*http.ServeMux, error) { |
| - mux, _, err := build(d, cfg) |
| - return mux, err |
| +// way; the chi router mounts inside it, and a *Runtime carrying the |
| +// platform wake function main.go wires to Options.NextDue. It also |
| +// starts the hourly sweep on ctx, which runs until ctx is cancelled or |
| +// the process exits. |
| +func App(ctx context.Context, d *db.DB, cfg Config) (*http.ServeMux, *Runtime, error) { |
| + mux, a, err := build(ctx, d, cfg) |
| + if err != nil { |
| + return nil, nil, err |
| + } |
| + go a.sweepForever(ctx) |
| + return mux, &Runtime{NextDue: a.nextDue}, nil |
| } |
| -// NewForTest builds the app and returns the handler and a clock setter. |
| -// Tests only; production uses App. |
| -func NewForTest(d *db.DB, cfg Config) (*http.ServeMux, func(func() time.Time), error) { |
| - mux, a, err := build(d, cfg) |
| +// NewForTest builds the app and returns the handler and its test |
| +// controls. No sweep ticker is started: see build. Tests only; |
| +// production uses App. |
| +func NewForTest(d *db.DB, cfg Config) (*http.ServeMux, *TestHandle, error) { |
| + mux, a, err := build(context.Background(), d, cfg) |
| if err != nil { |
| return nil, nil, err |
| } |
| - return mux, a.SetClockForTest, nil |
| + return mux, &TestHandle{a: a}, nil |
| } |
diff --git a/internal/meet/auth_test.go b/internal/meet/auth_test.go| index 8c66dc4..fc65421 100644 |
| --- a/internal/meet/auth_test.go |
| +++ b/internal/meet/auth_test.go |
| @@ -23,7 +23,7 @@ func testApp(t *testing.T, members ...string) *app { |
| t.Fatal(err) |
| } |
| t.Cleanup(func() { d.Close() }) |
| - _, a, err := build(d, Config{ |
| + _, a, err := build(context.Background(), d, Config{ |
| Env: EnvDevelopment, Origin: "http://app.test", |
| AllowedAddresses: members, Logger: logger, |
| }) |
| @@ -31,7 +31,7 @@ func testApp(t *testing.T, members ...string) *app { |
| t.Fatal(err) |
| } |
| now := time.Date(2026, 9, 8, 10, 0, 0, 0, time.UTC) |
| - a.SetClockForTest(func() time.Time { return now }) |
| + a.store.now = func() time.Time { return now } |
| return a |
| } |
| @@ -111,7 +111,7 @@ func TestStandingTable(t *testing.T) { |
| } |
| if tc.expired { |
| after := a.store.now().Add(grantLife + time.Minute) |
| - a.SetClockForTest(func() time.Time { return after }) |
| + a.store.now = func() time.Time { return after } |
| } |
| got, err := a.standingFor(ctx, p, m) |
| if err != nil { |
diff --git a/internal/meet/config.go b/internal/meet/config.go| index 7ae39f7..c1e7c20 100644 |
| --- a/internal/meet/config.go |
| +++ b/internal/meet/config.go |
| @@ -105,11 +105,17 @@ func resolve(cfg Config) (resolved, error) { |
| out.dek = b |
| } |
| + // Wrapped here rather than at the call site: a delivery error names |
| + // the recipient, and rastrillo/auth logs it (review C1-2). Doing it |
| + // in resolve makes that unbypassable — every mailer the app ever |
| + // holds has been through RedactMailErrors, including the one a test |
| + // or another main passes in. The nil check above the wrap, not |
| + // below: it is what refuses to start without a relay. |
| switch { |
| case cfg.Mailer != nil: |
| - out.mailer = cfg.Mailer |
| + out.mailer = RedactMailErrors(cfg.Mailer, cfg.Logger) |
| case cfg.development(): |
| - out.mailer = mail.Logged(cfg.Logger) |
| + out.mailer = RedactMailErrors(mail.Logged(cfg.Logger), cfg.Logger) |
| default: |
| return out, fmt.Errorf("meet: mail is not configured (set $MEET_SMTP_HOST and $MEET_SMTP_FROM) and Env is %q, not %q; a sign-in that cannot send its link is not sign-in", cfg.Env, EnvDevelopment) |
| } |
diff --git a/internal/meet/mailredact.go b/internal/meet/mailredact.go| index cc0aeb9..d2aa0ab 100644 |
| --- a/internal/meet/mailredact.go |
| +++ b/internal/meet/mailredact.go |
| @@ -11,7 +11,8 @@ import ( |
| ) |
| // RedactMailErrors wraps a mail sender so a delivery failure never |
| -// carries the recipient's address out of this package (review C1-2). |
| +// carries the recipient's address out of this package (review C1-2, |
| +// C2-1). |
| // |
| // rastrillo/mail wraps every SMTP failure as "send to <address>: <err>" |
| // (mail/mail.go:163) and rastrillo/auth logs that error verbatim |
| @@ -19,14 +20,36 @@ import ( |
| // 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. |
| +// does. So the wrapper logs the failure itself 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. |
| +// What the log line keeps is the root cause's *type* and a boolean |
| +// saying whether the address appeared in the message. It does not keep |
| +// the message, and never calls err.Error() for logging purposes — only |
| +// to test it for the address, and that result is thrown away, kept only |
| +// as a bool. That is a real loss — an SMTP reply's text is the most |
| +// useful thing about it — and it is the deliberate trade: a remote |
| +// server's error text is arbitrary bytes that may carry the address in a |
| +// shape no scrubber anticipated (the local part alone, the domain alone, |
| +// re-encoded, split across a fold). A type plus a yes/no is small, and |
| +// it cannot leak. |
| +// |
| +// The first version of this file tried to do better: replace every |
| +// occurrence of the address in the message with "[address]" and log the |
| +// result. That slices the message at byte offsets found in a |
| +// strings.ToLower copy, and ToLower can change a string's byte length |
| +// (the Kelvin sign U+212A is three bytes and lowercases to one-byte "k"; |
| +// capital dotted I U+0130 is two bytes and lowercases to two-byte "i̇"), |
| +// so an offset found in the lowered copy does not address the same bytes |
| +// in the original — an address containing either could leave a fragment |
| +// of itself in the "redacted" text (codex round 2, C2-1). Logging |
| +// nothing derived from the message at all, only a boolean computed with |
| +// no slicing, has no such trap: strings.Contains never needs its |
| +// operands to line up byte-for-byte with anything else. |
| +// |
| +// The wrapper forwards Delivers and SendList when the inner sender has |
| +// them, so mail.IsConfigured still sees through it (mail/mail.go:59) and |
| +// a ListSender does not silently lose its list headers. |
| func RedactMailErrors(inner mail.Sender, logger *slog.Logger) mail.Sender { |
| if inner == nil { |
| return nil |
| @@ -34,7 +57,18 @@ func RedactMailErrors(inner mail.Sender, logger *slog.Logger) mail.Sender { |
| if logger == nil { |
| logger = slog.Default() |
| } |
| - return &redactingSender{inner: inner, logger: logger} |
| + s := &redactingSender{inner: inner, logger: logger} |
| + list, isList := inner.(mail.ListSender) |
| + deliverer, isDeliverer := inner.(mail.Deliverer) |
| + switch { |
| + case isList && isDeliverer: |
| + return &redactingListDeliverer{redactingListSender{*s, list}, deliverer} |
| + case isList: |
| + return &redactingListSender{*s, list} |
| + case isDeliverer: |
| + return &redactingDeliverer{*s, deliverer} |
| + } |
| + return s |
| } |
| type redactingSender struct { |
| @@ -43,16 +77,48 @@ type redactingSender struct { |
| } |
| func (s *redactingSender) Send(ctx context.Context, to, subject, body string) error { |
| - err := s.inner.Send(ctx, to, subject, body) |
| + return s.redact(to, s.inner.Send(ctx, to, subject, body)) |
| +} |
| + |
| +// redact logs a delivery failure and returns the only error this package |
| +// lets out. It never returns anything derived from err. |
| +func (s *redactingSender) redact(to string, err error) error { |
| if err == nil { |
| return nil |
| } |
| s.logger.Error("meet: mail delivery failed", |
| "type", fmt.Sprintf("%T", rootCause(err)), |
| - "err", redactAddress(err.Error(), to)) |
| + "address_seen", addressSeen(err.Error(), to)) |
| return errors.New("meet: mail delivery failed") |
| } |
| +// redactingListSender forwards SendList; redactingDeliverer forwards |
| +// Delivers; redactingListDeliverer forwards both. Three types rather |
| +// than one with nil checks, so the interface a caller type-asserts for |
| +// is the interface the inner sender actually had. |
| +type redactingListSender struct { |
| + redactingSender |
| + list mail.ListSender |
| +} |
| + |
| +func (s *redactingListSender) SendList(ctx context.Context, m mail.ListMessage) error { |
| + return s.redact(m.To, s.list.SendList(ctx, m)) |
| +} |
| + |
| +type redactingDeliverer struct { |
| + redactingSender |
| + deliverer mail.Deliverer |
| +} |
| + |
| +func (s *redactingDeliverer) Delivers() bool { return s.deliverer.Delivers() } |
| + |
| +type redactingListDeliverer struct { |
| + redactingListSender |
| + deliverer mail.Deliverer |
| +} |
| + |
| +func (s *redactingListDeliverer) Delivers() bool { return s.deliverer.Delivers() } |
| + |
| // 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 |
| @@ -67,24 +133,19 @@ func rootCause(err error) error { |
| } |
| } |
| -// 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 { |
| +// addressSeen reports whether the recipient's address appears in an |
| +// error message, case-insensitively — the one bit of the message worth |
| +// keeping, and the only thing this package computes from it. No |
| +// slicing: strings.Contains over two independently-lowered strings never |
| +// needs an offset from one to mean anything in the other, so the byte- |
| +// length change strings.ToLower can produce (the Kelvin sign U+212A, |
| +// capital dotted I U+0130) cannot desynchronise anything the way it did |
| +// when the first version of this file used lowered offsets to slice the |
| +// original message. |
| +func addressSeen(msg, address string) bool { |
| 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):] |
| + return false |
| } |
| + return strings.Contains(strings.ToLower(msg), strings.ToLower(address)) |
| } |
diff --git a/internal/meet/mailredact_test.go b/internal/meet/mailredact_test.go| index 81d436c..e0bca8a 100644 |
| --- a/internal/meet/mailredact_test.go |
| +++ b/internal/meet/mailredact_test.go |
| @@ -7,6 +7,8 @@ import ( |
| "log/slog" |
| "strings" |
| "testing" |
| + |
| + "amadan.net/rastrillo/rastrillo/mail" |
| ) |
| type failingSender struct{ err error } |
| @@ -19,38 +21,98 @@ type smtpish struct{ msg string } |
| func (e smtpish) Error() string { return e.msg } |
| +func capture() (*slog.Logger, *bytes.Buffer) { |
| + var buf bytes.Buffer |
| + return slog.New(slog.NewTextHandler(&buf, nil)), &buf |
| +} |
| + |
| 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)) |
| + logger, buf := capture() |
| err := RedactMailErrors(inner, logger).Send(context.Background(), address, "s", "b") |
| if err == nil { |
| t.Fatal("a failed send answered nil") |
| } |
| + if err.Error() != "meet: mail delivery failed" { |
| + t.Fatalf("returned error is %q; it must be derived from nothing", err) |
| + } |
| if strings.Contains(strings.ToLower(err.Error()), "guest") { |
| t.Fatalf("the returned error carries the address: %q", err) |
| } |
| + log := buf.String() |
| + if strings.Contains(strings.ToLower(log), "guest") { |
| + t.Fatalf("the log line carries the address:\n%s", log) |
| + } |
| + if !strings.Contains(log, "meet.smtpish") { |
| + t.Fatalf("the log line does not name the error's type:\n%s", log) |
| + } |
| + if !strings.Contains(log, "address_seen=true") { |
| + t.Fatalf("the log line does not say the address was in the message:\n%s", log) |
| + } |
| +} |
| + |
| +// The rune trap the first version of this file walked into (codex round |
| +// 2, C2-1): it replaced the address in the message with "[address]" by |
| +// finding an offset in a strings.ToLower copy and slicing the original |
| +// at that offset. strings.ToLower can change a string's byte length — |
| +// KELVIN SIGN (U+212A, three bytes) folds to one-byte "k"; capital |
| +// dotted I (U+0130, two bytes) folds to two-byte "i̇" — so an offset |
| +// found in the lowered copy does not address the same bytes in the |
| +// original, and applying it could leave a suffix of the address in the |
| +// "redacted" text or cut a rune in half. |
| +// |
| +// This version logs nothing derived from the message — only a bool, via |
| +// strings.Contains over two independently-lowered strings, no slicing — |
| +// so there is nothing left to desynchronise. This test is the guard |
| +// that keeps it that way: two addresses carrying both trap runes, inside |
| +// one error message that also contains a run of ordinary ASCII "KKK"s |
| +// (an SMTP server rejecting a batch tends to shout), asserting the log |
| +// holds no "@" and no fragment of either address. |
| +func TestNoFragmentOfAnAddressReachesTheLog(t *testing.T) { |
| + const ( |
| + kelvin = "Kelvin@example.com" // ordinary ASCII K — not the Kelvin sign |
| + ines = "\u0130nes@example.com" // LATIN CAPITAL LETTER I WITH DOT ABOVE |
| + ) |
| + inner := failingSender{err: fmt.Errorf("rastrillo/mail: send to %s: %w", kelvin, |
| + smtpish{msg: "550 KKKKKKKKK unknown recipients " + kelvin + ", " + ines})} |
| + logger, buf := capture() |
| + |
| + err := RedactMailErrors(inner, logger).Send(context.Background(), kelvin, "s", "b") |
| + if err == nil { |
| + t.Fatal("a failed send answered nil") |
| + } |
| if err.Error() != "meet: mail delivery failed" { |
| - t.Fatalf("returned error is %q", err) |
| + t.Fatalf("returned error is %q, want exactly the fixed sentence", err) |
| } |
| - if got := strings.ToLower(buf.String()); strings.Contains(got, "guest") { |
| - t.Fatalf("the log line carries the address:\n%s", buf.String()) |
| + |
| + log := buf.String() |
| + if strings.Contains(log, "@") { |
| + t.Fatalf("the log contains an @:\n%s", log) |
| + } |
| + for _, fragment := range []string{ |
| + kelvin, ines, |
| + strings.ToLower(kelvin), strings.ToLower(ines), |
| + "elvin", "nes", // the local parts, minus the leading trap character |
| + "example.com", |
| + "\u0130", // the trap rune itself |
| + } { |
| + if strings.Contains(log, fragment) { |
| + t.Errorf("the log carries a fragment of an address, %q:\n%s", fragment, log) |
| + } |
| } |
| - 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(log, "address_seen=true") { |
| + t.Errorf("the address was in the message and the log does not say so:\n%s", log) |
| } |
| - if !strings.Contains(buf.String(), "meet.smtpish") { |
| - t.Fatalf("the log line does not name the error's type:\n%s", buf.String()) |
| + if strings.ContainsRune(log, '\uFFFD') { |
| + t.Errorf("the log holds a replacement rune, so something was cut mid-rune:\n%s", log) |
| } |
| } |
| func TestASuccessfulSendIsUntouched(t *testing.T) { |
| - var buf bytes.Buffer |
| - logger := slog.New(slog.NewTextHandler(&buf, nil)) |
| + logger, buf := capture() |
| if err := RedactMailErrors(failingSender{}, logger).Send(context.Background(), "a@b.test", "s", "b"); err != nil { |
| t.Fatal(err) |
| } |
| @@ -62,13 +124,62 @@ func TestASuccessfulSendIsUntouched(t *testing.T) { |
| } |
| } |
| -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) |
| +func TestAddressSeenIsCaseInsensitiveAndNeedsNoSlicing(t *testing.T) { |
| + for _, tc := range []struct { |
| + msg, address string |
| + want bool |
| + }{ |
| + {"send to a@b.test: refused", "A@B.test", true}, // the recipient, folded |
| + {"send to a@b.test: refused", "c@d.test", false}, |
| + {"dial tcp 10.0.0.1:25: connection refused", "a@b.test", false}, |
| + {"connection reset", "", false}, |
| + {"550 " + "\u212A" + "elvin@example.com not found", "\u212A" + "elvin@example.com", true}, |
| + } { |
| + if got := addressSeen(tc.msg, tc.address); got != tc.want { |
| + t.Errorf("addressSeen(%q, %q) = %v, want %v", tc.msg, tc.address, got, tc.want) |
| + } |
| } |
| - if got := redactAddress("boom", ""); got != "boom" { |
| - t.Fatalf("an empty address redacted %q", got) |
| +} |
| + |
| +// mail.IsConfigured asks a Sender whether it really delivers. The |
| +// wrapper has to forward that, or wrapping a logging mailer would make |
| +// it look like a relay (mail/mail.go:59). |
| +func TestTheWrapperForwardsDeliversAndSendList(t *testing.T) { |
| + logger, buf := capture() |
| + |
| + logged := RedactMailErrors(mail.Logged(logger), logger) |
| + if mail.IsConfigured(logged) { |
| + t.Fatal("a wrapped logging mailer claims to deliver") |
| + } |
| + if _, ok := logged.(mail.ListSender); !ok { |
| + t.Fatal("a wrapped ListSender lost SendList") |
| + } |
| + if err := logged.(mail.ListSender).SendList(context.Background(), |
| + mail.ListMessage{To: "a@b.test", Subject: "s", Body: "b"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + buf.Reset() |
| + |
| + plain := RedactMailErrors(failingSender{}, logger) |
| + if !mail.IsConfigured(plain) { |
| + t.Fatal("a wrapped plain Sender is assumed not to deliver") |
| + } |
| + if _, ok := plain.(mail.ListSender); ok { |
| + t.Fatal("a wrapped plain Sender gained a SendList it cannot honour") |
| + } |
| + |
| + // A failing SendList redacts exactly as Send does. |
| + failing := RedactMailErrors(failingListSender{err: fmt.Errorf("send to a@b.test: %w", smtpish{"550"})}, logger) |
| + err := failing.(mail.ListSender).SendList(context.Background(), mail.ListMessage{To: "a@b.test"}) |
| + if err == nil || err.Error() != "meet: mail delivery failed" { |
| + t.Fatalf("SendList returned %v", err) |
| + } |
| + if strings.Contains(buf.String(), "a@b.test") { |
| + t.Fatalf("SendList's log line carries the address:\n%s", buf.String()) |
| } |
| } |
| + |
| +type failingListSender struct{ err error } |
| + |
| +func (f failingListSender) Send(ctx context.Context, to, subject, body string) error { return f.err } |
| +func (f failingListSender) SendList(ctx context.Context, m mail.ListMessage) error { return f.err } |
diff --git a/internal/meet/store.go b/internal/meet/store.go| index 637d279..54806b5 100644 |
| --- a/internal/meet/store.go |
| +++ b/internal/meet/store.go |
| @@ -372,10 +372,36 @@ 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. |
| +// sweepLimit bounds one sweep call to a page of rows, so a large |
| +// backlog cannot hold SQLite's one write connection for an unbounded |
| +// delete (review C2-3). The caller loops while the count it gets back |
| +// is exactly this many. |
| +const sweepLimit = 1000 |
| + |
| +// sweep deletes pending rows whose fifteen minutes are up, at most |
| +// sweepLimit per call via a subselect on ids (a bare DELETE has no |
| +// portable LIMIT). Reads already exclude expired rows; 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{}) |
| + res := s.g.WithContext(ctx).Where( |
| + "id IN (SELECT id FROM pending_joins WHERE expires_at <= ? LIMIT ?)", |
| + now.UTC(), sweepLimit, |
| + ).Delete(&PendingJoin{}) |
| return res.RowsAffected, res.Error |
| } |
| + |
| +// nextExpiry answers the earliest expires_at among pending_joins, or the |
| +// zero time when there is no pending row at all. It is the platform |
| +// wake-up input for cmd/meet/main.go's Options.NextDue (review C2-2). |
| +func (s *store) nextExpiry(ctx context.Context) (time.Time, error) { |
| + var p PendingJoin |
| + err := s.g.WithContext(ctx).Order("expires_at ASC").Take(&p).Error |
| + if errors.Is(err, gorm.ErrRecordNotFound) { |
| + return time.Time{}, nil |
| + } |
| + if err != nil { |
| + return time.Time{}, err |
| + } |
| + return p.ExpiresAt, nil |
| +} |
diff --git a/internal/meet/store_test.go b/internal/meet/store_test.go| index 04ff94f..b25afa0 100644 |
| --- a/internal/meet/store_test.go |
| +++ b/internal/meet/store_test.go |
| @@ -122,8 +122,8 @@ func TestAReplacedLinkMintsNoGrant(t *testing.T) { |
| 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") |
| + if _, ok, err := s.grantFor(ctx, m.ID, 7); ok || err != nil { |
| + t.Fatalf("a grant row exists after minting through a replaced link: ok=%v err=%v", ok, err) |
| } |
| // The link that replaced it still mints, once. |
| @@ -234,6 +234,98 @@ func TestSweepDeletesOnlyExpiredPendingRows(t *testing.T) { |
| } |
| } |
| +// Codex round 2, C2-4: TestAStalePendingRowMintsNothingAndDeletesNothing |
| +// covers one meeting only. The cross-meeting case is the one |
| +// deletePending's address-wide (not id-scoped) delete actually protects |
| +// against: two pending rows for the same address across two different |
| +// meetings, A then B. Consuming B (the newer) mints B's grant and takes |
| +// every pending row for the address, A's included. The caller who still |
| +// holds A's stale id must then get no grant and no error — not an |
| +// attempt to resurrect a row that is already gone. |
| +func TestConsumingOneMeetingsPendingRowLeavesAnothersIDStale(t *testing.T) { |
| + s, now := testStore(t) |
| + ctx := context.Background() |
| + const address = "guest@example.com" |
| + |
| + mA, tokA, _ := s.createMeeting(ctx, "a", nil, "person:1") |
| + lA, _, _, _ := s.liveLink(ctx, tokA) |
| + if err := s.writePending(ctx, address, lA); err != nil { |
| + t.Fatal(err) |
| + } |
| + pendA, ok, err := s.latestPending(ctx, address) |
| + if err != nil || !ok { |
| + t.Fatalf("meeting A's pending row: ok=%v err=%v", ok, err) |
| + } |
| + |
| + *now = now.Add(time.Second) |
| + mB, tokB, _ := s.createMeeting(ctx, "b", nil, "person:1") |
| + lB, _, _, _ := s.liveLink(ctx, tokB) |
| + if err := s.writePending(ctx, address, lB); err != nil { |
| + t.Fatal(err) |
| + } |
| + pendB, ok, err := s.latestPending(ctx, address) |
| + if err != nil || !ok || pendB.ID == pendA.ID { |
| + t.Fatalf("meeting B's pending row: ok=%v err=%v same-id=%v", ok, err, pendB.ID == pendA.ID) |
| + } |
| + |
| + p := Person{ID: 9, Email: address} |
| + dest, err := s.consumePending(ctx, p, pendB, false) |
| + if err != nil || dest != "/m/"+mB.Sqid+"/join" { |
| + t.Fatalf("consuming B's row: dest=%q err=%v", dest, err) |
| + } |
| + if _, ok, err := s.grantFor(ctx, mB.ID, p.ID); !ok || err != nil { |
| + t.Fatalf("no grant for B after consuming its row: ok=%v err=%v", ok, err) |
| + } |
| + |
| + dest, err = s.consumePending(ctx, p, pendA, false) |
| + if err != nil { |
| + t.Fatalf("consuming A's now-stale row answered an error: %v", err) |
| + } |
| + if dest != "" { |
| + t.Fatalf("consuming A's now-stale row led to %q", dest) |
| + } |
| + if _, ok, err := s.grantFor(ctx, mA.ID, p.ID); ok || err != nil { |
| + t.Fatalf("a grant exists for A after its row was already consumed via B: ok=%v err=%v", ok, err) |
| + } |
| +} |
| + |
| +// Review C2-2: main.go's Options.NextDue reads this. The ordering is on |
| +// expires_at, deliberately not on id or created_at (which is what |
| +// latestPending sorts on, the other way): the row written last is not |
| +// necessarily the one that expires first. |
| +func TestNextExpiryIsTheEarliestExpiryNotTheLatestRow(t *testing.T) { |
| + s, now := testStore(t) |
| + ctx := context.Background() |
| + if got, err := s.nextExpiry(ctx); err != nil || !got.IsZero() { |
| + t.Fatalf("nextExpiry with no pending rows: %v (err %v), want the zero time", got, err) |
| + } |
| + |
| + _, tok, _ := s.createMeeting(ctx, "x", nil, "person:1") |
| + l, _, _, _ := s.liveLink(ctx, tok) |
| + base := *now |
| + |
| + *now = base.Add(20 * time.Minute) |
| + if err := s.writePending(ctx, "later@example.com", l); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Written last (the higher id), but its expiry is earlier than the |
| + // row above — the only way this test can fail is if nextExpiry |
| + // sorted on something other than expires_at. |
| + *now = base.Add(5 * time.Minute) |
| + if err := s.writePending(ctx, "sooner@example.com", l); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + got, err := s.nextExpiry(ctx) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + want := base.Add(5 * time.Minute).Add(pendingLife).UTC() |
| + if !got.Equal(want) { |
| + t.Fatalf("nextExpiry = %v, want %v (the sooner row's expiry)", got, want) |
| + } |
| +} |
| + |
| func TestPendingIsKeyedFingerprintAndFollowsItsLink(t *testing.T) { |
| s, now := testStore(t) |
| ctx := context.Background() |
diff --git a/internal/meettest/config_test.go b/internal/meettest/config_test.go| index 06eb888..2a48a10 100644 |
| --- a/internal/meettest/config_test.go |
| +++ b/internal/meettest/config_test.go |
| @@ -1,6 +1,7 @@ |
| package meettest |
| import ( |
| + "context" |
| "io" |
| "log/slog" |
| "path/filepath" |
| @@ -35,20 +36,20 @@ func TestProductionRefusesToStartWithoutSecrets(t *testing.T) { |
| "no allowlist": {Env: "production", Origin: "https://x.test", InstanceKey: "k", DEK: validDEK, Mailer: nopMailer{}}, |
| } |
| for name, cfg := range cases { |
| - if _, err := meet.App(openDB(t), cfg); err == nil { |
| + if _, _, err := meet.App(context.Background(), openDB(t), cfg); err == nil { |
| t.Errorf("%s: production started without it", name) |
| } |
| } |
| } |
| func TestDevelopmentStartsWithNothing(t *testing.T) { |
| - if _, err := meet.App(openDB(t), meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| + if _, _, err := meet.App(context.Background(), openDB(t), meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| t.Fatalf("development refused: %v", err) |
| } |
| } |
| func TestBadDEKIsRefusedEvenInDevelopment(t *testing.T) { |
| - _, err := meet.App(openDB(t), meet.Config{Env: meet.EnvDevelopment, DEK: "not base64!"}) |
| + _, _, err := meet.App(context.Background(), openDB(t), meet.Config{Env: meet.EnvDevelopment, DEK: "not base64!"}) |
| if err == nil || !strings.Contains(err.Error(), "base64") { |
| t.Fatalf("want a base64 refusal, got %v", err) |
| } |
diff --git a/internal/meettest/harness_test.go b/internal/meettest/harness_test.go| index 5f80966..f775a21 100644 |
| --- a/internal/meettest/harness_test.go |
| +++ b/internal/meettest/harness_test.go |
| @@ -38,7 +38,7 @@ type harness struct { |
| db *db.DB |
| dbPath string |
| mail *recordingMailer |
| - clock func(func() time.Time) |
| + ctl *meet.TestHandle |
| now time.Time |
| } |
| @@ -47,7 +47,25 @@ func (x *harness) sqlDB() *sql.DB { s, _ := x.db.G.DB(); return s } |
| func (x *harness) advance(d time.Duration) { |
| x.now = x.now.Add(d) |
| now := x.now |
| - x.clock(func() time.Time { return now }) |
| + x.ctl.SetClock(func() time.Time { return now }) |
| +} |
| + |
| +// sweep runs one bounded pass of what App runs hourly, at the harness's |
| +// own current time — the same clock advance already set, given |
| +// explicitly rather than read back off the app (Internal 5's one clock |
| +// for the whole pass). |
| +func (x *harness) sweep() { x.ctl.SweepForTest(x.now) } |
| + |
| +// authLinks counts rastrillo's unclicked magic links. It is the app's |
| +// own table, not Meet's, which is the point: nothing deletes a row there |
| +// but presenting the token or sweeping. |
| +func (x *harness) authLinks(t *testing.T) int { |
| + t.Helper() |
| + var n int |
| + if err := x.db.G.Raw("SELECT count(*) FROM auth_links").Scan(&n).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + return n |
| } |
| type recordingMailer struct { |
| @@ -91,13 +109,17 @@ func newHarness(t *testing.T, members ...string) *harness { |
| } |
| t.Cleanup(func() { d.Close() }) |
| rec := &recordingMailer{} |
| - mux, setClock, err := meet.NewForTest(d, meet.Config{ |
| + mux, ctl, err := meet.NewForTest(d, meet.Config{ |
| Env: meet.EnvDevelopment, Origin: testOrigin, AllowedAddresses: members, Mailer: rec, Logger: logger, |
| }) |
| if err != nil { |
| t.Fatal(err) |
| } |
| - x := &harness{h: mux, db: d, dbPath: dbPath, mail: rec, clock: setClock, now: time.Date(2026, 9, 8, 10, 0, 0, 0, time.UTC)} |
| + // The base is now, not a fixed date. Meet's clock is the only one a |
| + // test can move; rastrillo stamps its auth_links rows from the real |
| + // one. A fixed base put the two most of a day apart, so nothing that |
| + // compares them could mean anything — see the auth_links sweep test. |
| + x := &harness{h: mux, db: d, dbPath: dbPath, mail: rec, ctl: ctl, now: time.Now().UTC()} |
| x.advance(0) |
| return x |
| } |
diff --git a/internal/meettest/join_test.go b/internal/meettest/join_test.go| index 212f948..cba8471 100644 |
| --- a/internal/meettest/join_test.go |
| +++ b/internal/meettest/join_test.go |
| @@ -181,7 +181,15 @@ func TestMemberAddressExhaustsTheSharedLimiterWithTheSameAnswer(t *testing.T) { |
| // 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) { |
| +// |
| +// This is spec §3.2's filed residue, not a quirk of the harness. Behind |
| +// the platform edge, RemoteAddr is the proxy, so every visitor of every |
| +// team shares this one budget: twenty sign-in attempts per fifteen |
| +// minutes for the whole world, exhaustible on purpose by anyone, and |
| +// nothing in front of Begin can tell one client from another. The fix is |
| +// a trusted client address from the edge or a per-app limiter keyed on |
| +// something better; the question is filed on the platform. |
| +func TestSharedClientAllowanceIsExhaustedByOneClient(t *testing.T) { |
| const clientBudget = 20 // signin.Flow.IPBudget's default |
| members := make([]string, 0, clientBudget+1) |
| for i := 0; i <= clientBudget; i++ { |
| @@ -289,6 +297,74 @@ func TestReplacedLinkFallbackForSignedInPerson(t *testing.T) { |
| t.Fatalf("holding page of the first meeting: %d, want 200", rec.Code) |
| } |
| }) |
| + |
| + // Spec §3.2's third continuation case for a non-member: the pending |
| + // row is not replaced but simply expired. An outsider whose row |
| + // expires before they click is refused at Authorize with a 403 and |
| + // never reaches /continue (TestExpiredPendingRowAdmitsNobody), so the |
| + // only way to exercise this branch is with a session that already |
| + // exists — which is the gate the review found missing (C1-8). |
| + t.Run("non-member with an expired row 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") |
| + |
| + owner := x.browser(t) |
| + owner.signIn("paul@example.com") |
| + second, secondTok := createMeeting(t, owner, "second", "") |
| + |
| + 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")) |
| + } |
| + |
| + // The link stays live; only the row runs out (fifteen minutes). |
| + x.advance(16 * time.Minute) |
| + |
| + rec := guest.get("/continue") |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/welcome" { |
| + t.Fatalf("non-member with an expired row: %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) |
| + } |
| + if rec := guest.get("/m/" + first + "/join"); rec.Code != http.StatusOK { |
| + t.Fatalf("holding page of the first meeting: %d, want 200", rec.Code) |
| + } |
| + }) |
| +} |
| + |
| +// Review follow-up to C1-7: the point of the sweep is that an unclicked |
| +// magic link does not keep a plaintext address in rastrillo's auth_links |
| +// for the life of the instance. Asserted on the row itself. |
| +// |
| +// Eleven minutes is not enough: signin.Flow.LinkTTL defaults to fifteen |
| +// (keymaildev/signin@v0.1.1/flow.go:186) and rastrillo sets no override, |
| +// so a sweep at +11 must keep the row and one at +16 must take it. Both |
| +// halves are asserted, because a sweep that deleted everything would |
| +// pass the second on its own. |
| +func TestSweepClearsUnclickedMagicLinks(t *testing.T) { |
| + x := newHarness(t, "paul@example.com") |
| + if rec := x.browser(t).post("/signin", url.Values{"address": {"paul@example.com"}}); rec.Header().Get("Location") != "/signin?sent=1" { |
| + t.Fatalf("sign-in request: %s", rec.Header().Get("Location")) |
| + } |
| + if x.authLinks(t) != 1 { |
| + t.Fatalf("auth_links rows after a sign-in request: %d, want 1", x.authLinks(t)) |
| + } |
| + |
| + x.advance(11 * time.Minute) |
| + x.sweep() |
| + if n := x.authLinks(t); n != 1 { |
| + t.Fatalf("auth_links rows after a sweep inside the link's life: %d, want 1", n) |
| + } |
| + |
| + x.advance(5 * time.Minute) |
| + x.sweep() |
| + if n := x.authLinks(t); n != 0 { |
| + t.Fatalf("auth_links rows after the link expired and was swept: %d, want 0", n) |
| + } |
| } |
| func TestExpiredPendingRowAdmitsNobody(t *testing.T) { |
diff --git a/internal/meettest/meetings_test.go b/internal/meettest/meetings_test.go| index 83dbb77..5744df5 100644 |
| --- a/internal/meettest/meetings_test.go |
| +++ b/internal/meettest/meetings_test.go |
| @@ -111,10 +111,13 @@ func TestTheDatabaseNeverHoldsAPlaintextTitle(t *testing.T) { |
| grep := func(when string) { |
| for _, suffix := range []string{"", "-wal", "-shm"} { |
| b, err := os.ReadFile(x.dbPath + suffix) |
| - // 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) { |
| + // A missing -wal or -shm is ordinary — SQLite only keeps |
| + // them while there is unchecked WAL content. The primary |
| + // file missing is not ordinary: a wrong path would read |
| + // nothing and this test would pass having grepped nothing |
| + // at all, which is the one way a privacy gate can lie |
| + // (review C1-8, codex round 2 C2-4). |
| + if suffix != "" && os.IsNotExist(err) { |
| continue |
| } |
| if err != nil { |
diff --git a/internal/meettest/schema_test.go b/internal/meettest/schema_test.go| index 52fa6b7..8d1097a 100644 |
| --- a/internal/meettest/schema_test.go |
| +++ b/internal/meettest/schema_test.go |
| @@ -1,6 +1,7 @@ |
| package meettest |
| import ( |
| + "context" |
| "testing" |
| "amadan.net/oficina/meet/internal/meet" |
| @@ -8,7 +9,7 @@ import ( |
| func TestBootCreatesEveryTable(t *testing.T) { |
| d := openDB(t) |
| - if _, err := meet.App(d, meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| + if _, _, err := meet.App(context.Background(), d, meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| t.Fatal(err) |
| } |
| for _, table := range []string{"persons", "meetings", "meeting_links", "guest_grants", "pending_joins", "sessions", "auth_links"} { |
diff --git a/internal/meettest/signin_test.go b/internal/meettest/signin_test.go| index 6cd38a5..270c2cd 100644 |
| --- a/internal/meettest/signin_test.go |
| +++ b/internal/meettest/signin_test.go |
| @@ -1,10 +1,20 @@ |
| package meettest |
| import ( |
| + "bytes" |
| + "context" |
| + "errors" |
| + "fmt" |
| + "log/slog" |
| "net/http" |
| "net/url" |
| + "path/filepath" |
| "strings" |
| "testing" |
| + |
| + "amadan.net/rastrillo/rastrillo/db" |
| + |
| + meet "amadan.net/oficina/meet/internal/meet" |
| ) |
| func TestMemberSignsInAndLandsOnTheList(t *testing.T) { |
| @@ -57,3 +67,51 @@ func TestNonMemberSessionSeesWelcomeNotTheList(t *testing.T) { |
| t.Fatal("welcome page does not name the address") |
| } |
| } |
| + |
| +// Codex round 2, C2-4: mailredact_test.go proves the wrapper is safe by |
| +// calling it directly, which would stay green even if cmd/meet/main.go |
| +// stopped wiring it up. This is the production-path check instead: an |
| +// app built with meet.App — the same entry point main.go calls, over a |
| +// mailer that fails and a real *slog.Logger — answers a real |
| +// POST /signin, and nothing that logger captured names the address. |
| +// (Internal 3 / review C1-2's resolve-wraps-everything design is what |
| +// this is actually testing: no call site can opt out of the wrap.) |
| +func TestSigninOverAFailingMailerLeavesNoAddressInTheLog(t *testing.T) { |
| + const address = "paul@example.com" |
| + var buf bytes.Buffer |
| + logger := slog.New(slog.NewTextHandler(&buf, nil)) |
| + d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Cleanup(func() { d.Close() }) |
| + |
| + failing := failingMailer{err: fmt.Errorf("rastrillo/mail: send to %s: %w", address, errFakeSMTPFailure)} |
| + mux, _, err := meet.App(context.Background(), d, meet.Config{ |
| + Env: meet.EnvDevelopment, Origin: testOrigin, AllowedAddresses: []string{address}, |
| + Mailer: failing, Logger: logger, |
| + }) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + rec := post(t, mux, "/signin", url.Values{"address": {address}}) |
| + if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?sent=1" { |
| + t.Fatalf("POST /signin over a failing mailer: %d %s, want 303 /signin?sent=1 (the same answer as success)", |
| + rec.Code, rec.Header().Get("Location")) |
| + } |
| + |
| + log := buf.String() |
| + if strings.Contains(strings.ToLower(log), strings.ToLower(address)) { |
| + t.Fatalf("the log carries the address after a failing send:\n%s", log) |
| + } |
| + if !strings.Contains(log, "meet: mail delivery failed") { |
| + t.Fatalf("no redacted delivery failure was logged at all — the wrapper may not be wired:\n%s", log) |
| + } |
| +} |
| + |
| +var errFakeSMTPFailure = errors.New("550 mailbox unavailable") |
| + |
| +type failingMailer struct{ err error } |
| + |
| +func (f failingMailer) Send(ctx context.Context, to, subject, body string) error { return f.err } |