notify: log dead-lettered mail by queue row, not recipient !284

merged merged by cmc on 2026-09-06 18:22 UTC · krz/gitbay:fix-173-dead-letter-recipient into main

5 files changed, +70 −3

Layout: unified · split

.gitbay/wiki/Admin.org +6

		@@ -252,6 +252,12 @@ no =queues= key at all.
252	252	In accounts mode the same read renders at =/admin=, linked from the rail
253	253	for admins. Anyone else gets a 404 there.
254	254
	255	A dead-lettered mail is logged as =notification dead-lettered mail=<id>=,
	256	with the address redacted out of the relay's error. The id is the queue
	257	row: find it in the Mail table on =/admin=, or in =dashboard --json=,
	258	where the recipient and the unredacted error are. That is deliberate —
	259	see the Threat-Model page.
	260
255	261	* Maintenance
256	262
257	263	#+begin_src sh

.gitbay/wiki/Threat-Model.org +7

		@@ -20,6 +20,13 @@ log and hardening notes in [[Admin]].
20	20	registration invites, and API tokens travel on stdin or in request
21	21	bodies, never as command arguments (visible in =/proc=) or query
22	22	strings. Tokens are stored only as SHA-256 hashes.
	23	- Name a mail recipient in the log. A queued mail is logged by its
	24	queue row id, never by address, and the relay's own error is redacted
	25	before it is logged because a rejection usually quotes the address it
	26	rejected. The unredacted error and the address stay on the row, which
	27	an instance admin reads on =/admin=: the database holds who, the log
	28	holds which. One rule for every mail type — notifications, dependency
	29	reports and login links all drain the same queue (krz/gitbay#173).
23	30	- Confirm the existence of private repositories. Every surface answers
24	31	"not found" identically for a private repo and a nonexistent one — web
25	32	pages, git transport, control commands, release asset downloads.

internal/notify/notify.go +16 −1

		@@ -6,6 +6,7 @@ package notify
6	6	import (
7	7	"context"
8	8	"log/slog"
	9	"regexp"
9	10	"time"
10	11
11	12	"gitbay.org/gitbay/internal/config"
		@@ -53,7 +54,8 @@ func (m *Mailer) Run(ctx context.Context) {
53	54	attempt := q.Attempts + 1
54	55	if attempt >= m.MaxAttempts {
55	56	m.St.MarkMailFailed(q.ID, err.Error(), nil)
56		slog.Warn("notification dead-lettered", "recipient", q.Recipient, "err", err)
	57	slog.Warn("notification dead-lettered",
	58	"mail", q.ID, "attempts", attempt, "err", redactAddresses(err.Error()))
57	59	} else {
58	60	next := time.Now().Add(m.RetryBase << (attempt - 1))
59	61	m.St.MarkMailFailed(q.ID, err.Error(), &next)
		@@ -65,3 +67,16 @@ func (m *Mailer) Run(ctx context.Context) {
65	67	}
66	68	}
67	69	}
	70
	71	// A relay's rejection usually quotes the address it rejected — "550 5.1.1
	72	// <x@y>: Recipient address rejected" — so dropping the recipient field
	73	// alone would not keep an address out of the log.
	74	var addressPat = regexp.MustCompile(`[^\s<>@,;:"]+@[^\s<>@,;:"]+`)
	75
	76	// redactAddresses removes mail addresses from text bound for the log. The
	77	// unredacted error is still recorded on the queue row, where an instance
	78	// admin reads it on /admin: the database holds the address, the log does
	79	// not (#173).
	80	func redactAddresses(s string) string {
	81	return addressPat.ReplaceAllString(s, "<address>")
	82	}

internal/notify/redact_test.go added +39

		@@ -0,0 +1,39 @@
	1	package notify
	2
	3	import "testing"
	4
	5	// The log names the queue row, not the person. A relay's rejection quotes
	6	// the address it rejected, so the error text is redacted too — otherwise
	7	// dropping the recipient field would move the leak rather than close it
	8	// (#173).
	9	func TestRedactAddresses(t *testing.T) {
	10	cases := []struct{ in, want string }{
	11	{"550 5.1.1 <alice@example.test>: Recipient address rejected",
	12	"550 5.1.1 <<address>>: Recipient address rejected"},
	13	{"dial tcp 10.0.0.1:587: connect: connection refused",
	14	"dial tcp 10.0.0.1:587: connect: connection refused"},
	15	{"554 alice@a.test, bob@b.test both unknown",
	16	"554 <address>, <address> both unknown"},
	17	{"x509: certificate signed by unknown authority", "x509: certificate signed by unknown authority"},
	18	}
	19	for _, c := range cases {
	20	if got := redactAddresses(c.in); got != c.want {
	21	t.Errorf("redactAddresses(%q) = %q, want %q", c.in, got, c.want)
	22	}
	23	}
	24	}
	25
	26	// Whatever the relay says, no @ survives into the log line.
	27	func TestRedactLeavesNoAddress(t *testing.T) {
	28	for _, s := range []string{
	29	"550 <a.b+tag@sub.example.co.uk> over quota",
	30	`smtp: 553 "weird name"@host.test refused`,
	31	"relay said: alice@example.test; bob@example.test",
	32	} {
	33	if got := redactAddresses(s); containsAddress(got) {
	34	t.Errorf("address survived redaction: %q -> %q", s, got)
	35	}
	36	}
	37	}
	38
	39	func containsAddress(s string) bool { return addressPat.MatchString(s) }

internal/web/templates/admin.html +2 −2

		@@ -14,8 +14,8 @@
14	14	{{with .Queues.Mail}}
15	15	<h2>Mail <span class="count">{{.Pending}}</span></h2>
16	16	<p class="meta">{{.Pending}} pending · {{.Retrying}} retrying · {{.Failed}} failed{{if .OldestPending}} · oldest pending {{when .OldestPending}}{{end}}</p>
17		{{if .Items}}<div class="tablewrap"><table class="keys"><thead><tr class="cols"><th>Recipient</th><th>Subject</th><th>Attempts</th><th>State</th><th>Last error</th></tr></thead><tbody>
18		{{range .Items}}<tr><td>{{.Recipient}}</td><td>{{.Subject}}</td><td>{{.Attempts}}</td><td>{{if .FailedAt}}failed {{when .FailedAt}}{{else}}retrying{{end}}</td><td>{{.LastError}}</td></tr>
	17	{{if .Items}}<div class="tablewrap"><table class="keys"><thead><tr class="cols"><th>Mail</th><th>Recipient</th><th>Subject</th><th>Attempts</th><th>State</th><th>Last error</th></tr></thead><tbody>
	18	{{range .Items}}<tr><td class="mono">{{.ID}}</td><td>{{.Recipient}}</td><td>{{.Subject}}</td><td>{{.Attempts}}</td><td>{{if .FailedAt}}failed {{when .FailedAt}}{{else}}retrying{{end}}</td><td>{{.LastError}}</td></tr>
19	19	{{end}}</tbody></table></div>{{else}}<p class="none">Nothing retrying or failed</p>{{end}}
20	20	{{end}}
21	21