Commit `5063e69c97`

5063e69c970eba5409380bc6c85417b24f9fdab3

parent: 452ae0d87e

Verified · cmc

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

web: confirm a device removal on its id

Masking the token column left the token itself on the page three more
times: prefix8 returns anything under nine characters unchanged, and
that value went into a hidden tokenprefix input, the confirm field's
aria-label and its placeholder. The row was also asking for a string
it never showed, since the column renders through ShortToken while
the confirmation wanted prefix8.

Confirm on the device id instead. It is not device-identifying, the
form already posts it, and it is now a column, so what has to be
typed is on screen. want is derived from the same value the removal
dispatches on, as key-remove and pgp-remove derive theirs — the old
code read it from a separate hidden field, so a post that simply
omitted tokenprefix left want empty and an empty confirm matched.

prefix8 is unchanged: it still derives the typed-confirmation strings
for key, PGP and email removal.

Ref #89

internal/httpd/account.go +10 −8

		@@ -6,6 +6,7 @@ import (
6	6	"io"
7	7	"net/http"
8	8	"net/url"
	9	"strconv"
9	10	"strings"
10	11
11	12	"gitbay.org/gitbay/internal/control"
		@@ -32,16 +33,16 @@ type accountPGP struct {
32	33	}
33	34
34	35	// accountDevice is one registered APNs device as the settings page shows
35		// it. The full token is device-identifying and never reaches the page.
	36	// it. No form of the token reaches the page but the masked column:
	37	// removal confirms on the id, which is not device-identifying.
36	38	type accountDevice struct {
37	39	ID int64
38	40	Label string
39	41	// Token is rendered by control.ShortToken, the same renderer
40		// notifications device list uses: prefix8 returns anything under nine
41		// characters unchanged, and device add enforces no minimum length.
	42	// notifications device list uses.
42	43	Token string
43	44	LastSeenAt string
44		Confirm string // the token's first 8 characters, typed back to confirm removal
	45	Confirm string // the id as text, typed back to confirm removal
45	46	}
46	47
47	48	// accountForm renders the account's own settings: keys, addresses, and the
		@@ -84,7 +85,8 @@ func (s Server) accountPage(w http.ResponseWriter, r http.Request, u store.Use
84	85	if list, err := s.st.PushDevices(u.ID); err == nil {
85	86	for _, d := range list {
86	87	devices = append(devices, accountDevice{ID: d.ID, Label: d.Label,
87		Token: control.ShortToken(d.Token), LastSeenAt: d.LastSeenAt, Confirm: prefix8(d.Token)})
	88	Token: control.ShortToken(d.Token), LastSeenAt: d.LastSeenAt,
	89	Confirm: strconv.FormatInt(d.ID, 10)})
88	90	}
89	91	}
90	92
		@@ -279,12 +281,12 @@ func (s Server) accountSubmit(w http.ResponseWriter, r http.Request, u store.U
279	281	}
280	282	back("", "notification preferences saved")
281	283	case "device-remove":
282		want := r.FormValue("tokenprefix")
283		if ok, msg := confirmed(r, want); !ok {
	284	id := r.FormValue("id")
	285	if ok, msg := confirmed(r, id); !ok {
284	286	back(msg, "")
285	287	return
286	288	}
287		if _, msg, ok := s.runControl(u, []string{"notifications", "device", "remove", r.FormValue("id")}); !ok {
	289	if _, msg, ok := s.runControl(u, []string{"notifications", "device", "remove", id}); !ok {
288	290	back(msg, "")
289	291	return
290	292	}

internal/httpd/account_test.go +23 −13

		@@ -94,9 +94,10 @@ func TestAccountSubmitNotifyPush(t *testing.T) {
94	94	}
95	95	}
96	96
97		// Removing a device requires the typed confirmation, the same guard
98		// key-remove and pgp-remove carry, and then dispatches to notifications
99		// device remove, scoped to the caller's own account.
	97	// Removing a device requires the device id typed back, and then
	98	// dispatches to notifications device remove, scoped to the caller's own
	99	// account. The id is what the form dispatches on, so the guard is
	100	// derived server-side the way key-remove derives its own.
100	101	func TestAccountSubmitDeviceRemove(t *testing.T) {
101	102	st, err := store.Open(":memory:")
102	103	if err != nil {
		@@ -118,16 +119,15 @@ func TestAccountSubmitDeviceRemove(t *testing.T) {
118	119	}
119	120	s := New(config.Default(), st)
120	121
121		want := prefix8(token)
122	122	idStr := strconv.FormatInt(id, 10)
123	123
124	124	// Without the typed confirmation, the device survives.
125		submitAccountForm(t, s, u, url.Values{"field": {"device-remove"}, "id": {idStr}, "tokenprefix": {want}})
	125	submitAccountForm(t, s, u, url.Values{"field": {"device-remove"}, "id": {idStr}})
126	126	if devices, _ := st.PushDevices(uid); len(devices) != 1 {
127	127	t.Fatalf("device removed without confirmation: %v", devices)
128	128	}
129	129
130		rr := submitAccountForm(t, s, u, url.Values{"field": {"device-remove"}, "id": {idStr}, "tokenprefix": {want}, "confirm": {want}})
	130	rr := submitAccountForm(t, s, u, url.Values{"field": {"device-remove"}, "id": {idStr}, "confirm": {idStr}})
131	131	if rr.Code != http.StatusSeeOther {
132	132	t.Fatalf("status %d, body %s", rr.Code, rr.Body.String())
133	133	}
		@@ -136,10 +136,10 @@ func TestAccountSubmitDeviceRemove(t *testing.T) {
136	136	}
137	137	}
138	138
139		// A token at or under the truncation length is masked rather than shown
140		// whole, as notifications device list masks it. prefix8 returns anything
141		// shorter than nine characters unchanged, and device add enforces no
142		// minimum length, so the short token is a value that reaches the page.
	139	// A short token reaches no part of the page — not the visible column,
	140	// and not a hidden input, aria-label or placeholder either. Device add
	141	// enforces no minimum length, so a token this short is a value the store
	142	// can hold, and it is device-identifying whatever its length.
143	143	func TestAccountPageMasksAShortDeviceToken(t *testing.T) {
144	144	st, err := store.Open(":memory:")
145	145	if err != nil {
		@@ -153,7 +153,8 @@ func TestAccountPageMasksAShortDeviceToken(t *testing.T) {
153	153	if err != nil {
154	154	t.Fatal(err)
155	155	}
156		if _, err := st.AddPushDevice(uid, "abc123", "iphone"); err != nil {
	156	id, err := st.AddPushDevice(uid, "abc123", "iphone")
	157	if err != nil {
157	158	t.Fatal(err)
158	159	}
159	160
		@@ -161,7 +162,16 @@ func TestAccountPageMasksAShortDeviceToken(t *testing.T) {
161	162	rr := httptest.NewRecorder()
162	163	s.accountPage(rr, httptest.NewRequest("GET", "/settings", nil), store.User{ID: uid, Username: "alice"})
163	164
164		if strings.Contains(rr.Body.String(), `class="mono">abc123<`) {
165		t.Fatalf("the page printed the short token verbatim:\n%s", rr.Body.String())
	165	body := rr.Body.String()
	166	if strings.Contains(body, "abc123") {
	167	t.Fatalf("the short token reached the page:\n%s", body)
	168	}
	169	// What the removal asks for has to be on screen to be typed back.
	170	idStr := strconv.FormatInt(id, 10)
	171	if !strings.Contains(body, `aria-label="Type `+idStr+` to confirm"`) {
	172	t.Fatalf("removal does not confirm on the device id:\n%s", body)
	173	}
	174	if !strings.Contains(body, `<th scope="col">id</th>`) {
	175	t.Fatalf("the device table has no id column:\n%s", body)
166	176	}
167	177	}

internal/web/templates/account.html +3 −2

		@@ -153,12 +153,13 @@ account and where notifications go.</p>
153	153	<p class="meta">Notification text is sent in full, including for private repositories, so a repository name and item number reach Apple and appear on a lock screen.</p>
154	154	<h3>Devices</h3>
155	155	{{if .Devices}}<div class="tablewrap"><table class="keys nowrap">
156		<tr class="cols"><th scope="col">label</th><th scope="col">token</th><th scope="col">last seen</th><th scope="col"><span class="vh">actions</span></th></tr>
	156	<tr class="cols"><th scope="col">id</th><th scope="col">label</th><th scope="col">token</th><th scope="col">last seen</th><th scope="col"><span class="vh">actions</span></th></tr>
157	157	{{range .Devices}}<tr>
	158	<td class="mono">{{.ID}}</td>
158	159	<td>{{.Label}}</td>
159	160	<td class="mono">{{.Token}}</td>
160	161	<td>{{if .LastSeenAt}}{{when .LastSeenAt}}{{else}}never{{end}}</td>
161		<td class="act"><form method="post" action="/settings"><input type="hidden" name="field" value="device-remove"><input type="hidden" name="id" value="{{.ID}}"><input type="hidden" name="tokenprefix" value="{{.Confirm}}">{{template "confirmfield" .Confirm}} <button type="submit" class="danger">Remove</button></form></td>
	162	<td class="act"><form method="post" action="/settings"><input type="hidden" name="field" value="device-remove"><input type="hidden" name="id" value="{{.ID}}">{{template "confirmfield" .Confirm}} <button type="submit" class="danger">Remove</button></form></td>
162	163	</tr>
163	164	{{end}}</table></div>
164	165	{{else}}<p class="none">No registered devices.</p>{{end}}

Commit 5063e69c97

Commit `5063e69c97`