diff --git a/cmd/meshtender/main.go b/cmd/meshtender/main.go index 95471c4..0caecff 100644 --- a/cmd/meshtender/main.go +++ b/cmd/meshtender/main.go @@ -118,7 +118,10 @@ func run(logger *slog.Logger) error { janitorDone := make(chan struct{}) go func() { defer close(janitorDone) - pruneAuthCodes(bgCtx, st, authCodePruneInterval, logger) + runJanitor(bgCtx, janitorInterval, logger, + janitorSweep{"auth codes", st.PruneAuthCodes}, + janitorSweep{"share links", st.PruneInvites}, + ) }() httpSrv := &http.Server{ @@ -176,41 +179,50 @@ func run(logger *slog.Logger) error { return srvErr } -// authCodePruneInterval is how often expired cross-host handoff codes are swept. -// They live for authCodeTTL (60s), so this bounds a dead row's lifetime at a few -// minutes — the table stays small, which is all that's needed. It matches the -// cadence scs uses for its own session-table cleanup. -const authCodePruneInterval = 5 * time.Minute +// janitorInterval is how often the expired-row sweeps run. It's set by the +// shortest-lived thing they clean — a handoff code (authCodeTTL, 60s) — so this +// bounds a dead code's lifetime at a few minutes, and it matches the cadence scs +// uses for its own session-table cleanup. +// +// Share links are collected on a completely different timescale: they're kept for +// store.ExpiredInviteGrace (30 days) past expiry so their owner can see and remove +// them, so nearly every sweep finds nothing to do. That's fine — the delete is one +// indexed statement, which isn't worth a second ticker to avoid. +const janitorInterval = 5 * time.Minute -// authCodePruner is the slice of the store the janitor needs. Declared at the point -// of use so the loop can be exercised without a database. -type authCodePruner interface { - PruneAuthCodes(ctx context.Context) (int64, error) +// janitorSweep is one periodic cleanup job: a name for the log, and the delete to +// run. Taking a closure rather than an interface keeps the janitor decoupled from +// the store (a store method value satisfies it directly) and lets the loop be +// exercised without a database. +type janitorSweep struct { + name string + prune func(ctx context.Context) (int64, error) } -// pruneAuthCodes deletes expired handoff codes every `every` until ctx is cancelled. -// ConsumeAuthCode only removes codes that are actually redeemed, so abandoned ones -// (a sign-in whose tab was closed mid-handoff) would otherwise accumulate forever -// holding a user_id and login_id. It sweeps once on entry so a restart cleans up -// immediately rather than waiting out the first interval. +// runJanitor runs every sweep once on entry — so a restart cleans up immediately +// instead of waiting out the first interval — then again every `every` until ctx is +// cancelled. // -// This lives here rather than on the analytics rollup ticker — which also prunes, -// on the same cadence — because analytics has no business knowing about auth codes. -// A transient failure is logged and retried on the next tick; a failure caused by -// shutdown cancelling ctx is not an error worth reporting. -func pruneAuthCodes(ctx context.Context, p authCodePruner, every time.Duration, logger *slog.Logger) { - sweep := func() { - n, err := p.PruneAuthCodes(ctx) - switch { - case ctx.Err() != nil: - return // shutting down; not a real failure - case err != nil: - logger.Error("prune auth codes", "err", err) - case n > 0: - logger.Info("pruned expired auth codes", "count", n) +// This lives here rather than on the analytics rollup ticker, which also prunes on +// the same cadence, because analytics has no business knowing about auth codes or +// share links. A transient failure is logged and retried on the next tick, and one +// failing sweep doesn't stop the others; a failure caused by shutdown cancelling ctx +// is not an error worth reporting. +func runJanitor(ctx context.Context, every time.Duration, logger *slog.Logger, sweeps ...janitorSweep) { + runAll := func() { + for _, s := range sweeps { + n, err := s.prune(ctx) + switch { + case ctx.Err() != nil: + return // shutting down; not a real failure + case err != nil: + logger.Error("janitor sweep failed", "sweep", s.name, "err", err) + case n > 0: + logger.Info("janitor removed expired rows", "sweep", s.name, "count", n) + } } } - sweep() + runAll() t := time.NewTicker(every) defer t.Stop() @@ -219,7 +231,7 @@ func pruneAuthCodes(ctx context.Context, p authCodePruner, every time.Duration, case <-ctx.Done(): return case <-t.C: - sweep() + runAll() } } } diff --git a/cmd/meshtender/main_test.go b/cmd/meshtender/main_test.go index 5508895..6fc4494 100644 --- a/cmd/meshtender/main_test.go +++ b/cmd/meshtender/main_test.go @@ -48,38 +48,38 @@ func waitForCalls(t *testing.T, f *fakePruner, n int, within time.Duration) { } } -// TestPruneAuthCodesSweepsImmediately: a restart must clean up without waiting out +// TestJanitorSweepsImmediately: a restart must clean up without waiting out // a full interval, so the loop sweeps once on entry. The interval here is long // enough that a tick can't be what we observed. -func TestPruneAuthCodesSweepsImmediately(t *testing.T) { +func TestJanitorSweepsImmediately(t *testing.T) { t.Parallel() f := newFakePruner(nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - go pruneAuthCodes(ctx, f, time.Hour, discardLogger()) + go runJanitor(ctx, time.Hour, discardLogger(), janitorSweep{"test", f.PruneAuthCodes}) waitForCalls(t, f, 1, 2*time.Second) } -// TestPruneAuthCodesRepeats: the ticker keeps firing, so cleanup is ongoing rather +// TestJanitorRepeats: the ticker keeps firing, so cleanup is ongoing rather // than startup-only. -func TestPruneAuthCodesRepeats(t *testing.T) { +func TestJanitorRepeats(t *testing.T) { t.Parallel() f := newFakePruner(nil) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - go pruneAuthCodes(ctx, f, 2*time.Millisecond, discardLogger()) + go runJanitor(ctx, 2*time.Millisecond, discardLogger(), janitorSweep{"test", f.PruneAuthCodes}) // One immediate sweep plus several ticks. waitForCalls(t, f, 4, 2*time.Second) } -// TestPruneAuthCodesStopsOnCancel is the one that matters for shutdown: main waits +// TestJanitorStopsOnCancel is the one that matters for shutdown: main waits // on <-janitorDone, so a loop that ignored ctx would hang the process forever // instead of exiting. -func TestPruneAuthCodesStopsOnCancel(t *testing.T) { +func TestJanitorStopsOnCancel(t *testing.T) { t.Parallel() f := newFakePruner(nil) ctx, cancel := context.WithCancel(context.Background()) @@ -87,7 +87,7 @@ func TestPruneAuthCodesStopsOnCancel(t *testing.T) { done := make(chan struct{}) go func() { defer close(done) - pruneAuthCodes(ctx, f, time.Millisecond, discardLogger()) + runJanitor(ctx, time.Millisecond, discardLogger(), janitorSweep{"test", f.PruneAuthCodes}) }() waitForCalls(t, f, 1, 2*time.Second) // running @@ -96,35 +96,56 @@ func TestPruneAuthCodesStopsOnCancel(t *testing.T) { select { case <-done: case <-time.After(2 * time.Second): - t.Fatal("pruneAuthCodes did not return after ctx was cancelled — shutdown would hang") + t.Fatal("runJanitor did not return after ctx was cancelled — shutdown would hang") } } -// TestPruneAuthCodesSurvivesErrors: a transient DB failure must not kill the +// TestJanitorSurvivesErrors: a transient DB failure must not kill the // janitor for the life of the process; it logs and tries again next tick. -func TestPruneAuthCodesSurvivesErrors(t *testing.T) { +func TestJanitorSurvivesErrors(t *testing.T) { t.Parallel() f := newFakePruner(errors.New("connection refused")) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - go pruneAuthCodes(ctx, f, 2*time.Millisecond, discardLogger()) + go runJanitor(ctx, 2*time.Millisecond, discardLogger(), janitorSweep{"test", f.PruneAuthCodes}) // Still sweeping well after the first failure. waitForCalls(t, f, 4, 2*time.Second) } -// TestAuthCodePruneIntervalOutlivesCodeTTL: the sweep only has to keep the table +// TestJanitorRunsEverySweep: the janitor drives several cleanups, so each one must +// run on every pass — and a sweep that fails must not stop the sweeps after it, or a +// single sick table would silently stall unrelated cleanup for the whole process +// lifetime. +func TestJanitorRunsEverySweep(t *testing.T) { + t.Parallel() + broken := newFakePruner(errors.New("relation does not exist")) + healthy := newFakePruner(nil) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Broken first, so the healthy one only runs if a failure doesn't abort the pass. + go runJanitor(ctx, 2*time.Millisecond, discardLogger(), + janitorSweep{"broken", broken.PruneAuthCodes}, + janitorSweep{"healthy", healthy.PruneAuthCodes}, + ) + + waitForCalls(t, broken, 2, 2*time.Second) + waitForCalls(t, healthy, 2, 2*time.Second) +} + +// TestJanitorIntervalOutlivesCodeTTL: the sweep only has to keep the table // small, but an interval shorter than the code TTL would mean pointless work, and a // wildly long one would defeat the purpose. Pin it to a sane band so a future edit // has to think about it. -func TestAuthCodePruneIntervalOutlivesCodeTTL(t *testing.T) { +func TestJanitorIntervalOutlivesCodeTTL(t *testing.T) { t.Parallel() - if authCodePruneInterval < time.Minute { - t.Errorf("authCodePruneInterval = %v; shorter than the 60s code TTL means "+ - "sweeping rows that were never expirable", authCodePruneInterval) + if janitorInterval < time.Minute { + t.Errorf("janitorInterval = %v; shorter than the 60s code TTL means "+ + "sweeping rows that were never expirable", janitorInterval) } - if authCodePruneInterval > time.Hour { - t.Errorf("authCodePruneInterval = %v; too long to keep the table small", authCodePruneInterval) + if janitorInterval > time.Hour { + t.Errorf("janitorInterval = %v; too long to keep the table small", janitorInterval) } } diff --git a/internal/core/share_expiry_page_test.go b/internal/core/share_expiry_page_test.go new file mode 100644 index 0000000..3ee8870 --- /dev/null +++ b/internal/core/share_expiry_page_test.go @@ -0,0 +1,157 @@ +package core + +import ( + "io" + "net/http" + "net/http/cookiejar" + "strings" + "testing" + + "github.com/jleight/meshtender/internal/store" +) + +// TestSharePageShowsInviteExpiry checks how the share page presents link expiry to +// the owner: +// - the intro copy states the shelf life, so an owner knows a link is time-boxed +// before they hand it out +// - a live link shows its expiry alongside a copy control +// - an expired link is badged Expired and its copy control is GONE — offering to +// copy a token that can no longer be redeemed just produces a confused recipient +// +// Companion to the store-level tests for audit finding S3. +func TestSharePageShowsInviteExpiry(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("cookiejar: %v", err) + } + owner := seedSession(t, ts, st, ctx, jar, "shareexpiry") + rep, err := st.CreateRepeater(ctx, &store.Repeater{ + OwnerID: owner.ID, Name: "Rep", PublicKeyHex: strings.Repeat("b", 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + cookies := jar.Cookies(mustURL(t, ts.URL)) + + sharePage := func() string { + t.Helper() + resp := do(t, ts, h.app, "/repeaters/"+rep.PublicID+"/share", cookies...) + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("share page = %d, want 200", resp.StatusCode) + } + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + return string(body) + } + + // A live link. + liveToken, err := st.CreateInvite(ctx, rep.ID, "live link", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + + page := sharePage() + if !strings.Contains(page, "expire 7 days") { + t.Error("share page doesn't state how long links last") + } + if !strings.Contains(page, liveToken) { + t.Error("live link's token isn't shown for copying") + } + if !strings.Contains(page, "Expires") { + t.Error("live link doesn't show its expiry date") + } + if strings.Contains(page, `data-testid="invite-expired"`) { + t.Error("a live link is badged Expired") + } + + // Now age it out. + if _, err := st.Pool().Exec(ctx, + `UPDATE repeater_invites SET expires_at = now() - interval '1 minute' WHERE token = $1`, + liveToken); err != nil { + t.Fatalf("expire invite: %v", err) + } + + page = sharePage() + if !strings.Contains(page, `data-testid="invite-expired"`) { + t.Error("expired link isn't badged Expired") + } + if strings.Contains(page, liveToken) { + t.Error("expired link still offers its token to copy — it can't be redeemed") + } + if !strings.Contains(page, "can no longer be accepted") { + t.Error("expired link doesn't explain that it's dead") + } + // The row must still be listed so the owner can tidy it up. + if !strings.Contains(page, `data-testid="invite-row"`) { + t.Error("expired link vanished from the list entirely") + } +} + +// TestInvitePageForDeadLink covers what the *recipient* sees. Every "invalid" path +// renders the same page, and that page names InviteTTLDays — a render payload +// missing the key would put a literal "" in the sentence, which is why +// they all go through renderInviteInvalid. +// +// Also pins that the message stays vague about *why* the link is dead: someone +// holding a token shouldn't learn whether it never existed, was already claimed, +// was revoked, or simply lapsed. +func TestInvitePageForDeadLink(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("cookiejar: %v", err) + } + owner := seedSession(t, ts, st, ctx, jar, "deadlink") + rep, err := st.CreateRepeater(ctx, &store.Repeater{ + // Distinctive so the leak check below can't match navbar chrome ("Repeaters"). + OwnerID: owner.ID, Name: "Zarquon Ridge Relay", PublicKeyHex: strings.Repeat("d", 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + expired, err := st.CreateInvite(ctx, rep.ID, "gone", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + if _, err := st.Pool().Exec(ctx, + `UPDATE repeater_invites SET expires_at = now() - interval '1 minute' WHERE token = $1`, + expired); err != nil { + t.Fatalf("expire invite: %v", err) + } + + // Both a never-existed token and a lapsed one must render the same way. + for _, tc := range []struct{ name, token string }{ + {"expired token", expired}, + {"unknown token", "totally-made-up-token"}, + } { + resp := do(t, ts, h.app, "/invite/"+tc.token) + body, err := io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + t.Fatalf("%s: read body: %v", tc.name, err) + } + page := string(body) + if !strings.Contains(page, "Invalid link") { + t.Errorf("%s: page doesn't show the invalid-link state", tc.name) + } + if strings.Contains(page, "") { + t.Errorf("%s: render payload is missing a key the template names", tc.name) + } + if !strings.Contains(page, "expire after 7 days") { + t.Errorf("%s: message doesn't mention the expiry window", tc.name) + } + if strings.Contains(page, rep.Name) { + t.Errorf("%s: leaks the repeater name for a link that can't be redeemed", tc.name) + } + } +} diff --git a/internal/core/shares.go b/internal/core/shares.go index cd5bf87..475273a 100644 --- a/internal/core/shares.go +++ b/internal/core/shares.go @@ -6,6 +6,7 @@ import ( "net/http" "strconv" "strings" + "time" "github.com/go-chi/chi/v5" @@ -13,6 +14,23 @@ import ( "github.com/jleight/meshtender/internal/web" ) +// inviteTTLDays is store.InviteTTL in whole days, for the owner-facing and +// recipient-facing copy. Derived from the store constant so the stated shelf life +// can't drift from the one actually enforced. +var inviteTTLDays = int(store.InviteTTL / (24 * time.Hour)) + +// renderInviteInvalid renders the dead-link page. It exists so every "invalid" +// path supplies InviteTTLDays — the template names it, and a map missing the key +// would render "" into the copy. The message deliberately doesn't say +// which reason applies (never existed / already claimed / revoked / lapsed), so a +// stranger holding a token learns nothing from it. +func (s *Handlers) renderInviteInvalid(w http.ResponseWriter, r *http.Request) { + s.Render(w, r, "invite.html", map[string]any{ + "State": "invalid", + "InviteTTLDays": inviteTTLDays, + }) +} + // pageShare renders the sharing page for a repeater the user owns: the current // share link (if any) and the list of people who have accepted. func (s *Handlers) pageShare(w http.ResponseWriter, r *http.Request) { @@ -44,7 +62,10 @@ func (s *Handlers) pageShare(w http.ResponseWriter, r *http.Request) { "Invites": invites, "Orgs": orgs, "BaseURL": s.absoluteURL(r, ""), - "Error": r.URL.Query().Get("error"), + // Stated in the copy so an owner knows a link is time-boxed before handing it + // out. Derived from the store's TTL so the two can't disagree. + "InviteTTLDays": inviteTTLDays, + "Error": r.URL.Query().Get("error"), }) } @@ -141,7 +162,7 @@ func (s *Handlers) pageInvite(w http.ResponseWriter, r *http.Request) { // queryUserID drives the Shared flag; 0 when logged out is fine. rep, err := s.Store.RepeaterByInviteToken(r.Context(), uid, token) if errors.Is(err, store.ErrNotFound) { - s.Render(w, r, "invite.html", map[string]any{"State": "invalid"}) + s.renderInviteInvalid(w, r) return } if err != nil { @@ -179,7 +200,7 @@ func (s *Handlers) handleAcceptInvite(w http.ResponseWriter, r *http.Request) { rep, err := s.Store.RepeaterByInviteToken(r.Context(), uid, token) if errors.Is(err, store.ErrNotFound) { - s.Render(w, r, "invite.html", map[string]any{"State": "invalid"}) + s.renderInviteInvalid(w, r) return } if err != nil { @@ -201,7 +222,7 @@ func (s *Handlers) handleAcceptInvite(w http.ResponseWriter, r *http.Request) { // failure can never delete the link without granting access. A row lock inside // makes it the single-use gate against concurrent accepts. if _, err := s.Store.AcceptInvite(r.Context(), token, uid); errors.Is(err, store.ErrNotFound) { - s.Render(w, r, "invite.html", map[string]any{"State": "invalid"}) + s.renderInviteInvalid(w, r) return } else if err != nil { s.ServerError(w, r, "could not accept invite", err) diff --git a/internal/core/templates/invite.html b/internal/core/templates/invite.html index 84e009d..2f4f304 100644 --- a/internal/core/templates/invite.html +++ b/internal/core/templates/invite.html @@ -11,7 +11,11 @@
{{if eq .State "invalid"}}

Invalid link

-

This share link is no longer valid — it's single-use and may have already been used or revoked. Ask the owner for a new one.

+ {{/* Deliberately doesn't say WHICH reason: whoever holds this token + shouldn't learn whether it ever existed, was already claimed, or just + lapsed. Expiry is listed as a possibility so the message stays + accurate now that links have a shelf life. */}} +

This share link is no longer valid — links are single-use and expire after {{.InviteTTLDays}} days, so it may have already been used, been revoked, or timed out. Ask the owner for a new one.

{{template "icon-arrow-left" "me-1"}}Go to dashboard {{else if eq .State "auth_required"}} diff --git a/internal/core/templates/share.html b/internal/core/templates/share.html index 02c2015..43f751d 100644 --- a/internal/core/templates/share.html +++ b/internal/core/templates/share.html @@ -29,24 +29,41 @@

Invite specific people with single-use links: the recipient signs in and accepts to control this repeater from their own browser-connected KISS modem. Each link works once and disappears when it's - used — mint one per person, label it, and choose what they can do. + used — create one per person, label it, and choose what they can do. Links also + expire {{.InviteTTLDays}} days after you create them, so an unused one can't be + redeemed by whoever finds it later.

{{if .Invites}}
{{range .Invites}} -
+
{{if .Description}}{{.Description}}{{else}}(no label){{end}} + {{if .Expired}} + Expired + {{else}} Unused + {{end}}
+ {{if .Expired}} + {{/* No copy control for a dead link — handing out a token that can't be + redeemed just produces a confused recipient. */}} +

+ Expired {{ ts .ExpiresAt "datetime" }} and can no longer be accepted. Create a new link instead. +

+ {{else}}
{{$.BaseURL}}/invite/{{.Token}}
+

Expires {{ ts .ExpiresAt "datetime" }}

+ {{end}}
- +
{{end}} diff --git a/internal/e2e/share_invite_expiry_test.go b/internal/e2e/share_invite_expiry_test.go new file mode 100644 index 0000000..09cc51f --- /dev/null +++ b/internal/e2e/share_invite_expiry_test.go @@ -0,0 +1,95 @@ +//go:build browser + +package e2e + +import ( + "os" + "strings" + "testing" + + cdplog "github.com/chromedp/cdproto/log" + "github.com/chromedp/cdproto/network" + "github.com/chromedp/chromedp" + + "github.com/jleight/meshtender/internal/store" +) + +// TestE2EInviteExpiryBadges renders the share page with one live and one expired +// share link and checks what an owner actually sees: the live link keeps its copy +// control, the expired one is badged and loses it, and the page runs clean under the +// CSP. +// +// Covers the presentation half of audit finding S3 (share links used to never +// expire); the store-level rules are pinned in internal/store. +func TestE2EInviteExpiryBadges(t *testing.T) { + srv := newE2EServer(t) + user, cookie := srv.login(t, "e2einviteexpiry") + + rep, err := srv.store.CreateRepeater(srv.ctx, &store.Repeater{ + OwnerID: user.ID, Name: "Rep", PublicKeyHex: strings.Repeat("c", 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + + if _, err := srv.store.CreateInvite(srv.ctx, rep.ID, "still good", nil); err != nil { + t.Fatalf("create live invite: %v", err) + } + staleToken, err := srv.store.CreateInvite(srv.ctx, rep.ID, "long forgotten", nil) + if err != nil { + t.Fatalf("create stale invite: %v", err) + } + if _, err := srv.store.Pool().Exec(srv.ctx, + `UPDATE repeater_invites SET expires_at = now() - interval '1 minute' WHERE token = $1`, + staleToken); err != nil { + t.Fatalf("expire invite: %v", err) + } + + bctx, cancel, watch := startBrowser(t) + defer cancel() + + var rows, expiredBadges, copyButtons int + var showsStaleToken bool + var shot []byte + if err := chromedp.Run(bctx, + network.Enable(), + cdplog.Enable(), + setSessionCookie(cookie), + chromedp.Navigate(srv.appURL+"/repeaters/"+rep.PublicID+"/share"), + chromedp.WaitVisible(`[data-testid="invite-row"]`, chromedp.ByQuery), + chromedp.Evaluate(`document.querySelectorAll('[data-testid="invite-row"]').length`, &rows), + chromedp.Evaluate(`document.querySelectorAll('[data-testid="invite-expired"]').length`, &expiredBadges), + chromedp.Evaluate(`document.querySelectorAll('[data-copy-target]').length`, ©Buttons), + chromedp.Evaluate(`document.body.innerText.includes(`+jsString(staleToken)+`)`, &showsStaleToken), + chromedp.FullScreenshot(&shot, 80), + ); err != nil { + t.Fatalf("share page: %v", err) + } + watch.assertClean(t) + + if rows != 2 { + t.Errorf("invite rows = %d, want 2 (both links listed so the owner can tidy up)", rows) + } + if expiredBadges != 1 { + t.Errorf("expired badges = %d, want exactly 1", expiredBadges) + } + if copyButtons != 1 { + t.Errorf("copy controls = %d, want 1 — only the live link should be copyable", copyButtons) + } + if showsStaleToken { + t.Error("the expired link's token is still rendered on the page") + } + + // Keep the render for eyeballing when E2E_SHOT_DIR is set; skipped in CI. + if dir := os.Getenv("E2E_SHOT_DIR"); dir != "" && len(shot) > 0 { + if err := os.WriteFile(dir+"/invite-expiry.png", shot, 0o644); err != nil { + t.Logf("write screenshot: %v", err) + } + } +} + +// jsString quotes s for embedding in a JS expression. +func jsString(s string) string { + return `"` + strings.ReplaceAll(s, `"`, `\"`) + `"` +} diff --git a/internal/store/invite_expiry_test.go b/internal/store/invite_expiry_test.go new file mode 100644 index 0000000..dd74a50 --- /dev/null +++ b/internal/store/invite_expiry_test.go @@ -0,0 +1,264 @@ +package store + +import ( + "context" + "errors" + "strings" + "testing" + "time" +) + +// inviteFixture builds an owner + repeater and returns them, for the expiry tests. +func inviteFixture(t *testing.T, st *Store, ctx context.Context, owner string) (*User, *Repeater) { + t.Helper() + u, err := st.CreateUser(ctx, owner, "") + if err != nil { + t.Fatalf("create owner: %v", err) + } + rep, err := st.CreateRepeater(ctx, &Repeater{ + OwnerID: u.ID, Name: "Rep", PublicKeyHex: strings.Repeat("a", 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + return u, rep +} + +// expireInvite backdates a link's expiry by a minute so it reads as just-lapsed, +// without waiting out the real TTL. +func expireInvite(t *testing.T, st *Store, ctx context.Context, token string) { + t.Helper() + setInviteExpiry(t, st, ctx, token, -time.Minute) +} + +// setInviteExpiry moves a link's expiry by offset relative to now (negative = in the +// past), for exercising the prune grace window without waiting weeks. +func setInviteExpiry(t *testing.T, st *Store, ctx context.Context, token string, offset time.Duration) { + t.Helper() + if _, err := st.pool.Exec(ctx, + `UPDATE repeater_invites SET expires_at = now() + $2 WHERE token = $1`, + token, offset); err != nil { + t.Fatalf("set invite expiry: %v", err) + } +} + +// TestCreateInviteSetsExpiry: a new link gets a stored expiry roughly InviteTTL out. +// Storing it (rather than deriving it from created_at at query time) is what makes a +// later change to InviteTTL affect only new links — an outstanding link keeps the +// expiry it was minted with, so lengthening the constant can't revive dead links. +func TestCreateInviteSetsExpiry(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + _, rep := inviteFixture(t, st, ctx, "expiry-owner") + + before := time.Now() + token, err := st.CreateInvite(ctx, rep.ID, "join", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + invites, err := st.ListInvites(ctx, rep.ID) + if err != nil || len(invites) != 1 { + t.Fatalf("ListInvites = %d, %v; want 1", len(invites), err) + } + inv := invites[0] + if inv.Token != token { + t.Fatalf("token = %q, want %q", inv.Token, token) + } + if inv.ExpiresAt.IsZero() { + t.Fatal("ExpiresAt is zero — a link must carry its own expiry") + } + // Allow a generous window for clock skew between the test and the database. + want := before.Add(InviteTTL) + if delta := inv.ExpiresAt.Sub(want); delta < -time.Minute || delta > time.Minute { + t.Errorf("ExpiresAt = %v, want ~%v (InviteTTL out); off by %v", inv.ExpiresAt, want, delta) + } + if inv.Expired() { + t.Error("a freshly minted link reports Expired()") + } +} + +// TestExpiredInviteCannotBeRedeemed is the finding itself: before this change a +// share link stayed valid forever, so one pasted into a chat channel years earlier +// still granted repeater access to whoever found it first. +func TestExpiredInviteCannotBeRedeemed(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + _, rep := inviteFixture(t, st, ctx, "stale-owner") + + token, err := st.CreateInvite(ctx, rep.ID, "join", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + invitee, err := st.CreateUser(ctx, "stale-invitee", "") + if err != nil { + t.Fatalf("create invitee: %v", err) + } + + expireInvite(t, st, ctx, token) + + // The redemption path must refuse it. + if _, err := st.AcceptInvite(ctx, token, invitee.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("AcceptInvite(expired) err = %v, want ErrNotFound", err) + } + // And no share may have been granted as a side effect. + shares, err := st.ListShares(ctx, rep.ID) + if err != nil { + t.Fatalf("list shares: %v", err) + } + for _, sh := range shares { + if sh.UserID == invitee.ID { + t.Fatal("an expired link granted access") + } + } + + // The landing page that previews the link must not resolve it either — otherwise + // a recipient sees a working invite page whose accept then fails. + if _, err := st.RepeaterByInviteToken(ctx, invitee.ID, token); !errors.Is(err, ErrNotFound) { + t.Fatalf("RepeaterByInviteToken(expired) err = %v, want ErrNotFound", err) + } +} + +// TestUnexpiredInviteStillWorks guards the obvious over-correction: the expiry check +// must not break links that are still inside their window. +func TestUnexpiredInviteStillWorks(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + _, rep := inviteFixture(t, st, ctx, "live-owner") + + token, err := st.CreateInvite(ctx, rep.ID, "join", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + invitee, err := st.CreateUser(ctx, "live-invitee", "") + if err != nil { + t.Fatalf("create invitee: %v", err) + } + if _, err := st.RepeaterByInviteToken(ctx, invitee.ID, token); err != nil { + t.Fatalf("RepeaterByInviteToken(live) = %v, want success", err) + } + added, err := st.AcceptInvite(ctx, token, invitee.ID) + if err != nil || !added { + t.Fatalf("AcceptInvite(live) = (%v, %v), want (true, nil)", added, err) + } +} + +// TestListInvitesShowsExpired: an owner needs to see that a link is spent — hiding +// expired rows would leave them wondering where the link went, with no way to tidy +// up. The store returns them; Expired() is how the UI marks them. +func TestListInvitesShowsExpired(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + _, rep := inviteFixture(t, st, ctx, "list-owner") + + token, err := st.CreateInvite(ctx, rep.ID, "join", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + expireInvite(t, st, ctx, token) + + invites, err := st.ListInvites(ctx, rep.ID) + if err != nil || len(invites) != 1 { + t.Fatalf("ListInvites = %d, %v; want the expired link still listed", len(invites), err) + } + if !invites[0].Expired() { + t.Errorf("Expired() = false for a link whose expires_at is %v", invites[0].ExpiresAt) + } +} + +// TestPruneInvitesKeepsRecentlyExpired is the reason the sweep has a grace period: +// the share page lists expired links with an Expired badge and a Remove button, and +// the janitor runs every few minutes. Deleting on expiry would make that state +// unobservable and the button unreachable, so a just-lapsed link must survive a +// sweep. Only links long past their expiry get collected. +func TestPruneInvitesKeepsRecentlyExpired(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + _, rep := inviteFixture(t, st, ctx, "prune-owner") + + live, err := st.CreateInvite(ctx, rep.ID, "live", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + justLapsed, err := st.CreateInvite(ctx, rep.ID, "just lapsed", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + ancient, err := st.CreateInvite(ctx, rep.ID, "ancient", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + // Lapsed a minute ago: the owner should still see it. + expireInvite(t, st, ctx, justLapsed) + // Lapsed well beyond the grace window: collectable. + setInviteExpiry(t, st, ctx, ancient, -(ExpiredInviteGrace + 24*time.Hour)) + + n, err := st.PruneInvites(ctx) + if err != nil { + t.Fatalf("prune: %v", err) + } + if n != 1 { + t.Errorf("prune removed %d, want 1 (only the long-expired link)", n) + } + + invites, err := st.ListInvites(ctx, rep.ID) + if err != nil { + t.Fatalf("list invites: %v", err) + } + kept := map[string]bool{} + for _, inv := range invites { + kept[inv.Token] = true + } + if !kept[live] { + t.Error("the live link was pruned") + } + if !kept[justLapsed] { + t.Error("a just-expired link was pruned — the owner never gets to see or remove it") + } + if kept[ancient] { + t.Error("a link long past the grace window survived the sweep") + } + + // Idempotent: nothing else is collectable yet. + if n, err := st.PruneInvites(ctx); err != nil || n != 0 { + t.Fatalf("second prune = (%d, %v), want (0, nil)", n, err) + } +} + +// TestPruneInvitesGraceBoundary pins the edge: a link that lapsed just inside the +// window stays, one just outside goes. +func TestPruneInvitesGraceBoundary(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + _, rep := inviteFixture(t, st, ctx, "boundary-owner") + + inside, err := st.CreateInvite(ctx, rep.ID, "inside grace", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + outside, err := st.CreateInvite(ctx, rep.ID, "outside grace", nil) + if err != nil { + t.Fatalf("create invite: %v", err) + } + // An hour either side of the boundary, so clock skew can't decide the outcome. + setInviteExpiry(t, st, ctx, inside, -(ExpiredInviteGrace - time.Hour)) + setInviteExpiry(t, st, ctx, outside, -(ExpiredInviteGrace + time.Hour)) + + if _, err := st.PruneInvites(ctx); err != nil { + t.Fatalf("prune: %v", err) + } + invites, err := st.ListInvites(ctx, rep.ID) + if err != nil { + t.Fatalf("list invites: %v", err) + } + kept := map[string]bool{} + for _, inv := range invites { + kept[inv.Token] = true + } + if !kept[inside] { + t.Errorf("a link %v past expiry was pruned; grace is %v", ExpiredInviteGrace-time.Hour, ExpiredInviteGrace) + } + if kept[outside] { + t.Errorf("a link %v past expiry survived; grace is %v", ExpiredInviteGrace+time.Hour, ExpiredInviteGrace) + } +} diff --git a/internal/store/migrations/0040_invite_expiry.sql b/internal/store/migrations/0040_invite_expiry.sql new file mode 100644 index 0000000..8bc9a03 --- /dev/null +++ b/internal/store/migrations/0040_invite_expiry.sql @@ -0,0 +1,26 @@ +-- +goose Up +-- Share links used to be valid forever until redeemed or deleted, so a link pasted +-- into a chat channel or an email years ago still granted repeater access to +-- whoever found it first. Give every link a hard expiry. +-- +-- The expiry is STORED per row rather than computed as created_at + a constant, so +-- each link carries the promise made when it was minted. With a computed expiry, +-- editing the constant would retroactively redefine every outstanding link — and +-- lengthening it would resurrect links that had already expired and that their +-- owners had every reason to consider dead. Same instinct as the append-only +-- org_permission_versions table: a commitment made at creation time stays fixed. +ALTER TABLE repeater_invites ADD COLUMN expires_at TIMESTAMPTZ; + +-- Backfill outstanding links from their own creation time, not from now(): links +-- older than the window are exactly the stale ones this change exists to kill, so +-- they expire immediately rather than getting a fresh lease. +UPDATE repeater_invites SET expires_at = created_at + interval '7 days'; + +ALTER TABLE repeater_invites ALTER COLUMN expires_at SET NOT NULL; + +-- Supports the expiry predicate on every token lookup, and the pruning sweep. +CREATE INDEX repeater_invites_expires_at_idx ON repeater_invites (expires_at); + +-- +goose Down +DROP INDEX repeater_invites_expires_at_idx; +ALTER TABLE repeater_invites DROP COLUMN expires_at; diff --git a/internal/store/shares.go b/internal/store/shares.go index c200c8d..23ec7bd 100644 --- a/internal/store/shares.go +++ b/internal/store/shares.go @@ -177,13 +177,30 @@ type Invite struct { Token string Description string CreatedAt time.Time + ExpiresAt time.Time } +// Expired reports whether the link is past its expiry and can no longer be +// redeemed. The store won't hand it out either way; this is for presentation, so +// the owner can see a spent link before deleting it. +func (i Invite) Expired() bool { return !i.ExpiresAt.IsZero() && time.Now().After(i.ExpiresAt) } + +// InviteTTL is how long a new share link stays redeemable. Share links are +// credentials that travel through chat and email, so they get a hard shelf life +// rather than living until someone remembers to revoke them. +// +// It's applied when a link is minted and stored on the row (see migration 0040), so +// changing this constant affects only future links — outstanding ones keep the +// expiry they were created with. That's deliberate: a computed expiry would mean +// lengthening this value silently resurrects links that already died. +const InviteTTL = 7 * 24 * time.Hour + // CreateInvite mints a new single-use share link for a repeater, returning its // token. description is an owner-facing label (may be empty). commandIDs is the // initial command set the accepter is granted on redemption (may be empty — the // owner can grant more afterwards); it and the invite row are written together so -// the link never exists without its recorded grant. +// the link never exists without its recorded grant. The link expires after +// InviteTTL. func (s *Store) CreateInvite(ctx context.Context, repeaterID int64, description string, commandIDs []int64) (string, error) { token, err := randomToken() if err != nil { @@ -192,8 +209,9 @@ func (s *Store) CreateInvite(ctx context.Context, repeaterID int64, description err = s.inTx(ctx, func(tx pgx.Tx) error { var inviteID int64 if err := tx.QueryRow(ctx, - `INSERT INTO repeater_invites (repeater_id, token, description) VALUES ($1, $2, $3) RETURNING id`, - repeaterID, token, description).Scan(&inviteID); err != nil { + `INSERT INTO repeater_invites (repeater_id, token, description, expires_at) + VALUES ($1, $2, $3, now() + $4) RETURNING id`, + repeaterID, token, description, InviteTTL).Scan(&inviteID); err != nil { return fmt.Errorf("create invite: %w", err) } for _, cid := range commandIDs { @@ -212,9 +230,12 @@ func (s *Store) CreateInvite(ctx context.Context, repeaterID int64, description } // ListInvites returns a repeater's pending (unredeemed) share links, newest first. +// Expired links are included — the owner should see that a link is spent, and be +// able to delete it — so callers must present Invite.Expired() rather than implying +// every row is live. func (s *Store) ListInvites(ctx context.Context, repeaterID int64) ([]Invite, error) { rows, err := s.pool.Query(ctx, ` - SELECT id, token, description, created_at + SELECT id, token, description, created_at, expires_at FROM repeater_invites WHERE repeater_id = $1 ORDER BY created_at DESC`, repeaterID) @@ -223,11 +244,41 @@ func (s *Store) ListInvites(ctx context.Context, repeaterID int64) ([]Invite, er } return collectRows(rows, func(r pgx.Row) (Invite, error) { var inv Invite - err := r.Scan(&inv.ID, &inv.Token, &inv.Description, &inv.CreatedAt) + err := r.Scan(&inv.ID, &inv.Token, &inv.Description, &inv.CreatedAt, &inv.ExpiresAt) return inv, err }) } +// ExpiredInviteGrace is how long a lapsed share link stays in the table before the +// janitor removes it. It exists so the owner can actually SEE that a link went stale +// — the share page lists expired links with an Expired badge and a Remove button, +// and sweeping them immediately would make that state unobservable (the janitor runs +// every few minutes) and the button unreachable. Deleting them silently is the worse +// UX for something the owner handed to a person: the link just vanishes with no +// explanation of why it stopped working. +// +// Holding the row is not holding a credential: every lookup filters on expires_at, +// so a lapsed link can't be redeemed, and the share page stops rendering its token +// once it expires. Growth is bounded by a month of link creation. +const ExpiredInviteGrace = 30 * 24 * time.Hour + +// PruneInvites deletes share links that lapsed more than ExpiredInviteGrace ago, +// returning how many went. Redemption deletes a link and an owner can delete one by +// hand, but an unredeemed link that simply timed out would otherwise sit in the table +// forever. Index-assisted by repeater_invites_expires_at_idx. +func (s *Store) PruneInvites(ctx context.Context) (int64, error) { + // $1 needs the explicit ::interval cast. Without it `now() - $1` is ambiguous — + // Postgres can read it as timestamptz - timestamptz → interval, which then makes + // the comparison `timestamptz < interval` and fails at plan time. (The `now() + $n` + // expressions elsewhere are fine because addition has only one candidate.) + tag, err := s.pool.Exec(ctx, + `DELETE FROM repeater_invites WHERE expires_at < now() - $1::interval`, ExpiredInviteGrace) + if err != nil { + return 0, fmt.Errorf("prune invites: %w", err) + } + return tag.RowsAffected(), nil +} + // DeleteInvite removes an invite by id, scoped to its repeater (so an owner can // only delete links for their own repeater). Idempotent. func (s *Store) DeleteInvite(ctx context.Context, repeaterID, inviteID int64) error { @@ -240,11 +291,13 @@ func (s *Store) DeleteInvite(ctx context.Context, repeaterID, inviteID int64) er } // RepeaterByInviteToken resolves a valid share-link token to its repeater, or -// ErrNotFound if the token is unknown, revoked, or already redeemed (redemption -// deletes the link). queryUserID only populates the Shared flag and owner display. +// ErrNotFound if the token is unknown, revoked, expired, or already redeemed +// (redemption deletes the link). queryUserID only populates the Shared flag and +// owner display. func (s *Store) RepeaterByInviteToken(ctx context.Context, queryUserID int64, token string) (*Repeater, error) { row := s.pool.QueryRow(ctx, repeaterSelect+` - WHERE r.id = (SELECT repeater_id FROM repeater_invites WHERE token = $2)`, + WHERE r.id = (SELECT repeater_id FROM repeater_invites + WHERE token = $2 AND expires_at > now())`, queryUserID, token) r, err := scanRepeater(row) if err != nil { @@ -261,14 +314,17 @@ func (s *Store) RepeaterByInviteToken(ctx context.Context, queryUserID int64, to // without deleting the link, leaving it redeemable again). // // It returns whether a new share was created (false if the user already had one) -// and ErrNotFound if the token is unknown, revoked, or already redeemed. The row -// lock is the single-use gate: concurrent accepts serialize on it, and the winner -// deletes the row, so losers find nothing. +// and ErrNotFound if the token is unknown, revoked, expired, or already redeemed. +// The row lock is the single-use gate: concurrent accepts serialize on it, and the +// winner deletes the row, so losers find nothing. The expiry is checked inside that +// same locked read, so a link can't be redeemed by a request that started before it +// lapsed. func (s *Store) AcceptInvite(ctx context.Context, token string, userID int64) (added bool, err error) { err = s.inTx(ctx, func(tx pgx.Tx) error { var inviteID, repeaterID int64 if err := tx.QueryRow(ctx, - `SELECT id, repeater_id FROM repeater_invites WHERE token = $1 FOR UPDATE`, + `SELECT id, repeater_id FROM repeater_invites + WHERE token = $1 AND expires_at > now() FOR UPDATE`, token).Scan(&inviteID, &repeaterID); err != nil { return notFoundOr(err, "consume invite") }