Commit `f08b7b5e1a`

f08b7b5e1ac10272d2be4062b0582b4c28142d92

parent: b81a25efa5

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-20 11:09 UTC

control: gate push on [push] enabled

EnqueuePush ran whether or not the instance could deliver. With
enabled = false nothing starts the drainer, and the retention sweep
only collects rows carrying sent_at or failed_at, so every notice to
an account with a registered device left a row that is never read and
never collected. Gate it beside the sendMail check, which already
does this job for the other optional route.

device add refuses there for the same reason: it reported "device
registered" for a registration that can never deliver, while
notifications settings show still said push was on.

Ref #89

.gitbay/wiki/Users.org +3 −2

		@@ -705,8 +705,9 @@ truncated); =notifications device remove <id>= drops one by hand, from
705	705	the CLI or the account page. A device is also dropped on its own the
706	706	moment Apple reports the token dead, so an app deleted from a phone
707	707	stops costing anything without you having to notice. Push only works
708		when the instance operator has configured it — a self-hosted instance
709		with no APNs key configured never sends any.
	708	when the instance operator has configured it: on an instance with
	709	=[push] enabled = false=, =notifications device add= refuses rather
	710	than registering a device nothing can deliver to.
710	711
711	712	Push notifications carry the notice in full: a private repository's
712	713	name and the issue or merge request number reach Apple and can appear

internal/control/notifications.go +20 −4

		@@ -87,10 +87,16 @@ func notify(c *Ctx, userIDs []int64, n notice) {
87	87	return
88	88	}
89	89	sendMail := c.Cfg.Mail.SMTPHost != ""
	90	// Nothing drains push_queue unless the daemon started the deliverer,
	91	// and the retention sweep only collects rows that were sent or
	92	// dead-lettered, so a row written here would sit there forever.
	93	sendPush := c.Cfg.Push.Enabled
90	94	body := noticeBody(c, n)
91	95	for _, id := range recipients {
92	96	c.Store.AddNotice(id, n.repo.ID, n.kind, c.User.Username, n.action, n.path)
93		c.Store.EnqueuePush(id, pushTitle(n), pushBody(c.User.Username, n), n.path)
	97	if sendPush {
	98	c.Store.EnqueuePush(id, pushTitle(n), pushBody(c.User.Username, n), n.path)
	99	}
94	100	if !sendMail {
95	101	continue
96	102	}
		@@ -250,6 +256,13 @@ func runNotificationsDeviceAdd(c *Ctx, args []string) int {
250	256	if len(f.Pos) != 0 {
251	257	return c.usage()
252	258	}
	259	// The registration itself would succeed and then deliver nothing,
	260	// while notifications settings show still reported push on. Say what
	261	// is actually wrong instead.
	262	if !c.Cfg.Push.Enabled {
	263	return c.fail(protocol.ExitFailure,
	264	"this instance does not send push notifications ([push] enabled = false); ask an admin")
	265	}
253	266	raw, err := io.ReadAll(io.LimitReader(c.Stdin, maxDeviceTokenBytes+1))
254	267	if err != nil {
255	268	return c.fail(protocol.ExitFailure, "reading stdin: %v", err)
		@@ -286,7 +299,7 @@ func runNotificationsDeviceList(c *Ctx, args []string) int {
286	299	rows := make([]row, 0, len(devices))
287	300	for _, d := range devices {
288	301	rows = append(rows, row{ID: d.ID, Label: d.Label,
289		Token: shortToken(d.Token), Added: d.CreatedAt})
	302	Token: ShortToken(d.Token), Added: d.CreatedAt})
290	303	}
291	304	return c.emit(rows, func(w io.Writer) {
292	305	for _, r := range rows {
		@@ -295,12 +308,15 @@ func runNotificationsDeviceList(c *Ctx, args []string) int {
295	308	})
296	309	}
297	310
298		// shortToken renders a device token as its first eight characters. Enough
	311	// ShortToken renders a device token as its first eight characters. Enough
299	312	// to tell two devices apart in a list, not enough to push to one. A real
300	313	// APNs token is 64 hex characters, so anything at or under the cut length
301	314	// is not a token worth showing part of — it is masked outright rather
302	315	// than echoed whole, which "abc…" would imply is a truncation.
303		func shortToken(t string) string {
	316	//
	317	// Exported because the account page lists the same devices: one renderer,
	318	// so the two surfaces cannot come to disagree about what they print.
	319	func ShortToken(t string) string {
304	320	if len(t) > 8 {
305	321	return t[:8] + "…"
306	322	}

internal/control/notifications_test.go +53 −4

		@@ -5,6 +5,8 @@ import (
5	5	"strings"
6	6	"testing"
7	7
	8	"gitbay.org/gitbay/internal/config"
	9	"gitbay.org/gitbay/internal/protocol"
8	10	"gitbay.org/gitbay/internal/store"
9	11	)
10	12
		@@ -27,9 +29,12 @@ func notifTestCtx(t testing.T, username string) Ctx {
27	29	}
28	30	var out bytes.Buffer
29	31	return &Ctx{
30		User: store.User{ID: uid, Username: username},
31		Scope: "full",
32		Store: st,
	32	User: store.User{ID: uid, Username: username},
	33	Scope: "full",
	34	Store: st,
	35	// Push enabled is the instance state the push tests assume; the
	36	// disabled case sets it back to false explicitly.
	37	Cfg: config.Config{Push: config.Push{Enabled: true}},
33	38	Stdin: strings.NewReader(""),
34	39	Stdout: &out,
35	40	Stderr: &out,
		@@ -106,6 +111,50 @@ func TestNotifyQueuesNoPushForTheActor(t *testing.T) {
106	111	}
107	112	}
108	113
	114	// TestNotifyQueuesNoPushWhenDisabled: on an instance with [push]
	115	// enabled = false nothing drains the queue, and the retention sweep only
	116	// collects rows that were sent or dead-lettered, so a row written here is
	117	// never collected. The mail half already gates on the instance having
	118	// SMTP; push gates the same way.
	119	func TestNotifyQueuesNoPushWhenDisabled(t *testing.T) {
	120	c, repo, bob := testRepoWithWatcher(t)
	121	c.Cfg.Push.Enabled = false
	122	c.Store.AddPushDevice(bob, "tok-b", "iphone")
	123
	124	notify(c, []int64{bob}, notice{repo: repo, kind: "issue",
	125	subject: "s", action: "opened issue #1", path: "alice/app/issues/1"})
	126
	127	if due, _ := c.Store.DuePush(20); len(due) != 0 {
	128	t.Fatalf("queued %d pushes on a push-disabled instance", len(due))
	129	}
	130	// The inbox row is still filed: push is the optional half, not the
	131	// notice.
	132	if n := c.Store.UnreadNotices(bob); n != 1 {
	133	t.Fatalf("unread notices = %d, want 1", n)
	134	}
	135	}
	136
	137	// TestNotificationsDeviceAddRefusedWhenPushDisabled: registering a device
	138	// on an instance that cannot deliver would report success and then never
	139	// push, with notifications settings show still saying push is on.
	140	func TestNotificationsDeviceAddRefusedWhenPushDisabled(t *testing.T) {
	141	c := notifTestCtx(t, "alice")
	142	c.Cfg.Push.Enabled = false
	143	c.Stdin = strings.NewReader("DEVTOKEN\n")
	144	var out bytes.Buffer
	145	c.Stdout, c.Stderr = &out, &out
	146
	147	if code := runNotificationsDeviceAdd(c, nil); code != protocol.ExitFailure {
	148	t.Fatalf("exit %d, want %d", code, protocol.ExitFailure)
	149	}
	150	if devices, _ := c.Store.PushDevices(c.User.ID); len(devices) != 0 {
	151	t.Fatalf("device registered anyway: %+v", devices)
	152	}
	153	if !strings.Contains(out.String(), "[push] enabled = false") {
	154	t.Fatalf("message does not name the instance setting: %q", out.String())
	155	}
	156	}
	157
109	158	func TestNotificationsDeviceAddReadsStdin(t *testing.T) {
110	159	c := notifTestCtx(t, "alice")
111	160	c.Stdin = strings.NewReader("DEVTOKEN\n")
		@@ -154,7 +203,7 @@ func TestNotificationsDeviceListTruncatesTheTokenJSON(t *testing.T) {
154	203	}
155	204
156	205	// TestNotificationsDeviceListMasksAShortToken: a token at or under the
157		// truncation cut length is not returned unchanged. shortToken's short
	206	// truncation cut length is not returned unchanged. ShortToken's short
158	207	// path used to return the token verbatim, a full echo of anything eight
159	208	// characters or fewer; runNotificationsDeviceAdd enforces no minimum
160	209	// length, so a short token is a value the command will store.

Commit f08b7b5e1a

Commit `f08b7b5e1a`