amadan

carlos / movement Public

Tasks 7 and 8: confirmation burns on POST, and the budgets hold

A GET renders a button and burns nothing. Link-scanning gateways fetch
every URL in an arriving message, so confirming on GET would put people on
the list who never clicked — breaking the first half of the one rule with
the mechanism meant to enforce it — and would then hand the actual human a
dead link. A test drives exactly that sequence.

Spent, unknown and expired tokens render one page. Resubmitting a pending
row mints a fresh token and kills the old one, so an old message stops
being a live credential.

The mail budget is enforced end to end: a fourth message to one address
does not go, and the submission that triggered it looks identical to one
that did. A sender that never returns does not block the next submission
and does not roll back the row, which is what send-after-commit buys.

Writing these found a race in the test helper rather than the code — it
returned the last message to an address as soon as any existed, so a
resubmission test could read the previous token and pass without proving
anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell ac2d88061a7b24cbc106e5eec0b14e1cc09d14b9 parent 51fba7a
8 files changed, +473 −11
  • internal/movement/app.go +11 −4
  • internal/movement/config.go +8 −0
  • internal/movement/confirm.go +89 −0
  • internal/movement/render.go +1 −1
  • internal/movement/templates/confirm.html +17 −0
  • internal/movementtest/confirm_test.go +144 −0
  • internal/movementtest/harness_test.go +92 −6
  • internal/movementtest/mail_test.go +111 −0
diff --git a/internal/movement/app.go b/internal/movement/app.go
index b290915..f3c6b9e 100644
--- a/internal/movement/app.go
+++ b/internal/movement/app.go
@@ -55,14 +55,19 @@ func App(d *db.DB, env Env) (*http.ServeMux, error) {
Warn("this build carries unreviewed placeholder copy")
}
+ sender := env.Mailer
+ if sender == nil {
+ sender = mail.FromEnv("CARLOS", slogFor())
+ }
a := &app{
db: d,
env: env,
challengeKey: ChallengeKey(env.InstanceKey),
- // Always passed explicitly. auth.New substitutes mail.Logged for a
- // nil Mailer, which writes the link and the address into carlos logs
- // — the same leak config.go refuses at boot, through another door.
- mailer: BudgetedSender(mail.FromEnv("CARLOS", slogFor()), d.G),
+ // Always passed explicitly wherever a Mailer is wanted. auth.New
+ // substitutes mail.Logged for a nil one, which writes the link and
+ // the address into carlos logs — the same leak config.go refuses at
+ // boot, arriving through another door.
+ mailer: BudgetedSender(sender, d.G),
}
r := chi.NewRouter()
@@ -75,6 +80,8 @@ func App(d *db.DB, env Env) (*http.ServeMux, error) {
// copy has not been through review yet.
r.Get("/s/{stance}", a.stancePage)
r.Post("/signup", a.handleSignup)
+ r.Get("/confirm", a.handleConfirmPage)
+ r.Post("/confirm", a.handleConfirm)
mux := http.NewServeMux()
// The app serves its own static files — the framework never does.
diff --git a/internal/movement/config.go b/internal/movement/config.go
index b6ce1e2..e16dcf0 100644
--- a/internal/movement/config.go
+++ b/internal/movement/config.go
@@ -6,6 +6,8 @@ import (
"net/url"
"os"
"strings"
+
+ "github.com/carlosframework/rastrillo/mail"
)
// CanonicalHost is the one host this app is meant to serve in production.
@@ -20,6 +22,12 @@ type Env struct {
Origin string
InstanceKey string
Admins []string
+
+ // Mailer, when set, replaces the platform SMTP sender. LoadEnv never
+ // fills it: it is the seam main.go could use to wire something else and
+ // that tests use to read what was sent. Left nil, App builds the real one
+ // from CARLOS_SMTP_*.
+ Mailer mail.Sender
}
// loopback reports whether origin is a local development origin.
diff --git a/internal/movement/confirm.go b/internal/movement/confirm.go
new file mode 100644
index 0000000..da44c59
--- /dev/null
+++ b/internal/movement/confirm.go
@@ -0,0 +1,89 @@
+package movement
+
+import (
+ "errors"
+ "net/http"
+ "time"
+
+ "gorm.io/gorm"
+)
+
+// confirmData drives the one confirmation template through its three states.
+type confirmData struct {
+ Token string
+ State string // "offer", "done", "dead"
+}
+
+// handleConfirmPage renders a page with a single Confirm button. It burns
+// nothing.
+//
+// This is the whole reason confirmation is a POST. Link-scanning gateways —
+// Outlook Safe Links, corporate security proxies, some mail clients
+// prefetching — fetch every URL in a message as it arrives. Confirming on GET
+// would put people on the list who never clicked, which breaks the first half
+// of the one rule using the very mechanism meant to enforce it, and would
+// then show the actual human "this link is no longer valid".
+//
+// The lookup here is read-only, so a scanner still cannot change anything. It
+// exists only so somebody arriving with a dead link is told before they press
+// a button that was never going to work.
+func (a *app) handleConfirmPage(w http.ResponseWriter, r *http.Request) {
+ token := r.URL.Query().Get("token")
+
+ if _, err := a.liveRow(token, time.Now()); err != nil {
+ render(w, "confirm", confirmData{State: "dead"})
+ return
+ }
+ render(w, "confirm", confirmData{Token: token, State: "offer"})
+}
+
+// handleConfirm burns the token and puts the address on the list.
+func (a *app) handleConfirm(w http.ResponseWriter, r *http.Request) {
+ now := time.Now()
+ token := r.PostFormValue("token")
+
+ row, err := a.liveRow(token, now)
+ if err != nil {
+ // Unknown, expired and already-burned all render the same page. There
+ // is nothing useful to tell apart here, and plenty to leak by trying.
+ render(w, "confirm", confirmData{State: "dead"})
+ return
+ }
+
+ // Clearing the hash is what makes the link single-use.
+ err = a.db.G.Model(&Signup{}).
+ Where("id = ? AND confirmed = ?", row.ID, false).
+ Select("Confirmed", "ConfirmedAt", "TokenHash", "UpdatedAt").
+ Updates(map[string]any{
+ "confirmed": true,
+ "confirmed_at": now,
+ "token_hash": nil,
+ "updated_at": now,
+ }).Error
+ if err != nil {
+ render(w, "confirm", confirmData{State: "dead"})
+ return
+ }
+
+ render(w, "confirm", confirmData{State: "done"})
+}
+
+// liveRow finds the unconfirmed row a token belongs to, if the token is still
+// good. Only the hash is stored, so the presented token is hashed and matched
+// rather than compared.
+func (a *app) liveRow(token string, now time.Time) (Signup, error) {
+ if token == "" {
+ return Signup{}, errors.New("movement: no token")
+ }
+ var row Signup
+ err := a.db.G.
+ Where("token_hash = ? AND confirmed = ? AND token_expiry > ?", hashToken(token), false, now).
+ First(&row).Error
+ if err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return Signup{}, err
+ }
+ return Signup{}, err
+ }
+ return row, nil
+}
diff --git a/internal/movement/render.go b/internal/movement/render.go
index 0798312..f76a2c2 100644
--- a/internal/movement/render.go
+++ b/internal/movement/render.go
@@ -26,7 +26,7 @@ var assets = rastrillo.NewAssets(appFS)
var pages = map[string]*template.Template{}
func init() {
- for _, name := range []string{"index", "stance", "sent", "errors"} {
+ for _, name := range []string{"index", "stance", "sent", "confirm", "errors"} {
pages[name] = template.Must(
// ui's partials and their helpers first, so every page can
// call one without registering anything. WithIcons points
diff --git a/internal/movement/templates/confirm.html b/internal/movement/templates/confirm.html
new file mode 100644
index 0000000..31621b5
--- /dev/null
+++ b/internal/movement/templates/confirm.html
@@ -0,0 +1,17 @@
+{{define "title"}}{{say "confirm.title"}}{{end}}
+{{define "content"}}
+{{if eq .State "offer"}}
+ <h1 class="rst-page-title">{{say "confirm.title"}}</h1>
+ {{/* A button, not a redirect. Mail scanners fetch every link in an
+ arriving message, and a GET that confirmed would put people on the
+ list who never clicked. */}}
+ <form class="rst-form" method="post" action="/confirm">
+ <input type="hidden" name="token" value="{{.Token}}">
+ <button type="submit">{{say "confirm.button"}}</button>
+ </form>
+{{else if eq .State "done"}}
+ <h1 class="rst-page-title">{{say "confirm.done"}}</h1>
+{{else}}
+ <h1 class="rst-page-title">{{say "confirm.dead"}}</h1>
+{{end}}
+{{end}}
diff --git a/internal/movementtest/confirm_test.go b/internal/movementtest/confirm_test.go
new file mode 100644
index 0000000..9de1dd3
--- /dev/null
+++ b/internal/movementtest/confirm_test.go
@@ -0,0 +1,144 @@
+package movementtest
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "regexp"
+ "strings"
+ "testing"
+
+ movement "amadan.net/carlos/movement/internal/movement"
+)
+
+var confirmLink = regexp.MustCompile(`/confirm\?token=([0-9a-f]{64})`)
+
+// signUpAndTakeToken drives a real signup and returns the token from the
+// message that went out. The token exists nowhere else — only its hash is
+// stored — so reading the mail is the only way to get one, which is the
+// point.
+func signUpAndTakeToken(t *testing.T, h *testApp, email string) string {
+ t.Helper()
+ want := h.Mail.countTo(email) + 1
+ postSignup(t, h, submission(t, email, "curious"))
+
+ body := h.Mail.waitForMailTo(t, email, want)
+ m := confirmLink.FindStringSubmatch(body)
+ if m == nil {
+ t.Fatalf("no confirmation link in the message to %s:\n%s", email, body)
+ }
+ return m[1]
+}
+
+func postConfirm(t *testing.T, h *testApp, token string) *httptest.ResponseRecorder {
+ t.Helper()
+ form := url.Values{"token": {token}}
+ req := httptest.NewRequest(http.MethodPost, "/confirm", strings.NewReader(form.Encode()))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ req.Header.Set("Origin", testOrigin)
+ rec := httptest.NewRecorder()
+ h.ServeHTTP(rec, req)
+ return rec
+}
+
+// The reason confirmation is a POST at all. Link-scanning gateways fetch
+// every URL in a message as it arrives, so a GET that confirmed would put
+// people on the list who never clicked — breaking the first half of the one
+// rule with the very mechanism meant to enforce it — and would then show the
+// actual human a dead link.
+func TestGetDoesNotConfirm(t *testing.T) {
+ h := newApp(t)
+ token := signUpAndTakeToken(t, h, "scanned@example.com")
+
+ rec := get(t, h, "/confirm?token="+token)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("GET /confirm: %d", rec.Code)
+ }
+ if loadSignup(t, h, "scanned@example.com").Confirmed {
+ t.Fatal("a GET confirmed the address; a mail scanner would have joined for them")
+ }
+
+ // And the human's click still works afterwards.
+ postConfirm(t, h, token)
+ if !loadSignup(t, h, "scanned@example.com").Confirmed {
+ t.Fatal("the real click failed after a scanner had fetched the link")
+ }
+}
+
+func TestConfirmPutsTheAddressOnTheList(t *testing.T) {
+ h := newApp(t)
+ token := signUpAndTakeToken(t, h, "joining@example.com")
+
+ rec := postConfirm(t, h, token)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("POST /confirm: %d", rec.Code)
+ }
+
+ row := loadSignup(t, h, "joining@example.com")
+ if !row.Confirmed {
+ t.Fatal("the address is not on the list")
+ }
+ if row.ConfirmedAt == nil {
+ t.Error("ConfirmedAt was not recorded")
+ }
+ if len(row.TokenHash) != 0 {
+ t.Error("the token hash survived; the link is still live")
+ }
+}
+
+// A burned token, an expired one and one that never existed are all the same
+// answer. There is nothing useful to tell apart and plenty to leak by trying.
+func TestSpentUnknownAndExpiredTokensAreIndistinguishable(t *testing.T) {
+ h := newApp(t)
+ token := signUpAndTakeToken(t, h, "once@example.com")
+ postConfirm(t, h, token)
+
+ spent := postConfirm(t, h, token).Body.String()
+ unknown := postConfirm(t, h, strings.Repeat("a", 64)).Body.String()
+ empty := postConfirm(t, h, "").Body.String()
+
+ if spent != unknown || unknown != empty {
+ t.Error("a spent, an unknown and an empty token must render the same page")
+ }
+ if !strings.Contains(spent, movement.T("confirm.dead")) {
+ t.Error("a dead link should say so")
+ }
+}
+
+func TestConfirmingTwiceDoesNotDoubleCount(t *testing.T) {
+ h := newApp(t)
+ token := signUpAndTakeToken(t, h, "twice@example.com")
+
+ postConfirm(t, h, token)
+ first := loadSignup(t, h, "twice@example.com")
+ postConfirm(t, h, token)
+ second := loadSignup(t, h, "twice@example.com")
+
+ if !first.ConfirmedAt.Equal(*second.ConfirmedAt) {
+ t.Error("a second click moved ConfirmedAt; the link is not single-use")
+ }
+}
+
+// A pending row that is submitted again gets a fresh token, and the old one
+// stops working — otherwise every message ever sent to an address stays a
+// live credential for it.
+func TestResubmittingInvalidatesTheOldLink(t *testing.T) {
+ h := newApp(t)
+ const addr = "renewed@example.com"
+
+ old := signUpAndTakeToken(t, h, addr)
+ fresh := signUpAndTakeToken(t, h, addr)
+ if old == fresh {
+ t.Fatal("resubmitting reused the same token")
+ }
+
+ postConfirm(t, h, old)
+ if loadSignup(t, h, addr).Confirmed {
+ t.Error("the superseded link still confirmed the address")
+ }
+
+ postConfirm(t, h, fresh)
+ if !loadSignup(t, h, addr).Confirmed {
+ t.Error("the current link did not work")
+ }
+}
diff --git a/internal/movementtest/harness_test.go b/internal/movementtest/harness_test.go
index 93f4d78..b2c8e0a 100644
--- a/internal/movementtest/harness_test.go
+++ b/internal/movementtest/harness_test.go
@@ -7,6 +7,7 @@
package movementtest
import (
+ "context"
"io"
"log/slog"
"net/http"
@@ -14,10 +15,12 @@ import (
"net/url"
"path/filepath"
"strings"
+ "sync"
"testing"
"time"
"github.com/carlosframework/rastrillo/db"
+ "github.com/carlosframework/rastrillo/mail"
movement "amadan.net/carlos/movement/internal/movement"
)
@@ -26,16 +29,95 @@ import (
// same-origin evidence — the pair a browser form submission would be.
const testOrigin = "http://app.test"
-// newApp builds the whole app per test over a fresh temp database,
-// exactly as main.go does.
-// testApp is the handler plus the database behind it, so a test can assert on
-// what a request actually wrote rather than inferring it from a page.
+// testApp is the handler plus the database and the mailbox behind it, so a
+// test can assert on what a request actually wrote and actually sent rather
+// than inferring either from a page.
type testApp struct {
http.Handler
- DB *db.DB
+ DB *db.DB
+ Mail *recorder
}
+// recorder stands in for the platform's SMTP sender. Confirmation tokens are
+// stored only as hashes, so reading the message is the only way to get one —
+// which is exactly the property the design intends.
+type recorder struct {
+ mu sync.Mutex
+ sent []recorded
+}
+
+type recorded struct{ To, Subject, Body string }
+
+func (r *recorder) Send(ctx context.Context, to, subject, body string) error {
+ r.mu.Lock()
+ defer r.mu.Unlock()
+ r.sent = append(r.sent, recorded{to, subject, body})
+ return nil
+}
+
+func (r *recorder) count() int {
+ r.mu.Lock()
+ defer r.mu.Unlock()
+ return len(r.sent)
+}
+
+func (r *recorder) countTo(addr string) int {
+ r.mu.Lock()
+ defer r.mu.Unlock()
+ n := 0
+ for _, m := range r.sent {
+ if m.To == addr {
+ n++
+ }
+ }
+ return n
+}
+
+// waitForMailTo blocks until at least atLeast messages have gone to addr, and
+// returns the latest one's body.
+//
+// The wait is needed because sends happen after the write transaction
+// commits, in a goroutine, so a message is not necessarily on the record by
+// the time the response has been written. The count matters as much as the
+// wait: returning as soon as any message to the address exists would hand
+// back a stale one, and a test reading yesterday's token would pass while
+// proving nothing.
+func (r *recorder) waitForMailTo(t *testing.T, addr string, atLeast int) string {
+ t.Helper()
+ deadline := time.Now().Add(2 * time.Second)
+ for time.Now().Before(deadline) {
+ r.mu.Lock()
+ var last string
+ n := 0
+ for _, m := range r.sent {
+ if m.To == addr {
+ n++
+ last = m.Body
+ }
+ }
+ r.mu.Unlock()
+ if n >= atLeast {
+ return last
+ }
+ time.Sleep(5 * time.Millisecond)
+ }
+ t.Fatalf("fewer than %d messages reached %s", atLeast, addr)
+ return ""
+}
+
+// newApp builds the whole app per test over a fresh temp database, exactly as
+// main.go does, with a recording mailer in place of the platform's SMTP.
func newApp(t *testing.T) *testApp {
+ t.Helper()
+ box := &recorder{}
+ app := newAppWith(t, box)
+ app.Mail = box
+ return app
+}
+
+// newAppWith is newApp with a mailer of the test's choosing — for the cases
+// that need a sender which fails, or one that never returns at all.
+func newAppWith(t *testing.T, sender mail.Sender) *testApp {
t.Helper()
logger := slog.New(slog.NewTextHandler(io.Discard, nil))
d, err := db.Open(filepath.Join(t.TempDir(), "app.db"), logger)
@@ -44,7 +126,11 @@ func newApp(t *testing.T) *testApp {
}
t.Cleanup(func() { d.Close() })
movement.SetLogger(logger)
- mux, err := movement.App(d, testEnv())
+
+ env := testEnv()
+ env.Mailer = sender
+
+ mux, err := movement.App(d, env)
if err != nil {
t.Fatalf("App: %v", err)
}
diff --git a/internal/movementtest/mail_test.go b/internal/movementtest/mail_test.go
new file mode 100644
index 0000000..1f63a9f
--- /dev/null
+++ b/internal/movementtest/mail_test.go
@@ -0,0 +1,111 @@
+package movementtest
+
+import (
+ "context"
+ "testing"
+ "time"
+
+ movement "amadan.net/carlos/movement/internal/movement"
+)
+
+// The budget is what actually stands between a stranger's inbox and a flood
+// from our own verified sending identity. Proof of work is a speed bump —
+// 2^18 SHA-256 is under a millisecond on a commodity GPU — so this is the
+// defence, and it is persisted because the process hibernates and an
+// in-memory counter would forget on the next wake.
+func TestAnAddressCannotBeMailedPastItsAllowance(t *testing.T) {
+ h := newApp(t)
+ const addr = "flooded@example.com"
+
+ for i := 0; i < movement.PerAddressPerDay+3; i++ {
+ postSignup(t, h, submission(t, addr, "curious"))
+ }
+
+ // Give any in-flight sends a moment to land before counting.
+ time.Sleep(100 * time.Millisecond)
+
+ if got := h.Mail.countTo(addr); got != movement.PerAddressPerDay {
+ t.Errorf("%d messages reached %s, want the budget of %d",
+ got, addr, movement.PerAddressPerDay)
+ }
+}
+
+// A suppressed send is invisible from the outside. If it were not, the budget
+// itself would become the oracle the one rule exists to prevent.
+func TestSuppressedSendRendersTheSamePage(t *testing.T) {
+ h := newApp(t)
+ const addr = "quiet@example.com"
+
+ first := postSignup(t, h, submission(t, addr, "curious")).Body.String()
+ for i := 0; i < movement.PerAddressPerDay; i++ {
+ postSignup(t, h, submission(t, addr, "curious"))
+ }
+ suppressed := postSignup(t, h, submission(t, addr, "curious")).Body.String()
+
+ if suppressed != first {
+ t.Error("a submission whose message was suppressed looked different from one that was sent")
+ }
+}
+
+// blocking never returns. mail.Sender.Send discards the context it is given
+// and smtp.SendMail dials with no deadline, so this is not a hypothetical
+// sender — it is what a hung SES connection looks like from in here.
+type blocking struct{ entered chan struct{} }
+
+func (b *blocking) Send(ctx context.Context, to, subject, body string) error {
+ select {
+ case b.entered <- struct{}{}:
+ default:
+ }
+ <-ctx.Done() // held until the app's own deadline fires
+ return ctx.Err()
+}
+
+// The send happens after the transaction commits, in a goroutine. Inside the
+// transaction it would hold the single SQLite writer and stall every other
+// signup and the scheduled tick behind it.
+func TestAHungSenderDoesNotBlockTheNextSubmission(t *testing.T) {
+ hung := &blocking{entered: make(chan struct{}, 1)}
+ h := newAppWith(t, hung)
+
+ postSignup(t, h, submission(t, "first@example.com", "curious"))
+
+ select {
+ case <-hung.entered:
+ case <-time.After(2 * time.Second):
+ t.Fatal("the sender was never called")
+ }
+
+ // With the first send still hanging, a second submission must complete.
+ done := make(chan struct{})
+ go func() {
+ postSignup(t, h, submission(t, "second@example.com", "curious"))
+ close(done)
+ }()
+
+ select {
+ case <-done:
+ case <-time.After(5 * time.Second):
+ t.Fatal("a hung send blocked the next submission; it is holding the writer")
+ }
+
+ if got := countSignups(t, h, "second@example.com"); got != 1 {
+ t.Errorf("the second submission wrote %d rows, want 1", got)
+ }
+}
+
+// The row is committed before the message is attempted, so a send that never
+// succeeds still leaves something to re-submit against.
+func TestRowSurvivesASendThatNeverCompletes(t *testing.T) {
+ hung := &blocking{entered: make(chan struct{}, 1)}
+ h := newAppWith(t, hung)
+
+ postSignup(t, h, submission(t, "stranded@example.com", "curious"))
+
+ if got := countSignups(t, h, "stranded@example.com"); got != 1 {
+ t.Fatal("the signup was rolled back when its message hung")
+ }
+ if row := loadSignup(t, h, "stranded@example.com"); len(row.TokenHash) == 0 {
+ t.Error("no token was stored, so no future link could ever work")
+ }
+}