Architecture review small fixes: FK check, web toggles, doc drift !496

merged merged by cmc on 2026-09-28 22:09 UTC · krz/gitbay:web-audit-fixes into main

10 files changed, +323 −99

Layout: unified · split

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

	@@ -59,8 +59,9 @@ the command wrote diagnostics:
59		59
60	HTTP status maps the exit code: 0→200, 2→400, 3→404, 4→403, else 500.	60	HTTP status maps the exit code: 0→200, 2→400, 3→404, 4→403, else 500.
61	Commands that emit raw text rather than an envelope (=help=, =mr diff=)	61	Commands that emit raw text rather than an envelope (=help=, =mr diff=)
62	come wrapped as ={"output": "..."}=. Git transport commands and the	62	come wrapped as ={"output": "..."}=. Git transport commands are refused
63	token commands are refused by name.	63	by name; the token commands are not — a full-scope token can mint,
		64	list and revoke tokens the same way it can run anything else.
64		65
65	#+begin_src sh	66	#+begin_src sh
66	curl -s -H "Authorization: Bearer $TOKEN" \	67	curl -s -H "Authorization: Bearer $TOKEN" \

.gitbay/wiki/Parity.org +12 −7

 fifth state, so every =state = 'open'= rule still means what it did.
 =mr revisions= lists the heads a merge request has had; the web page
 lists them beside the reviews they staled, and the iOS client has a
-Revisions section. =mr range-diff= compares two heads: the iOS client
+Revisions section. =mr range-diff= compares two heads: the iOS client shows it from a
-shows it from a revision to the one before, as text; the web has no
+revision to the one before, as text; the web has no view yet
-view. Batched review is not built.
+(krz/gitbay#269). Batched review — draft diff comments held with
+=mr comment --pending= and sent together with =--comment=/=--discard=
+or a verdict — is built and the web uses it: composing review
+comments before publishing them is the same round trip as the CLI's
+=--pending= flag.
 =mr review request --add <user>= asks a *particular* person, who then
 carries the merge request in their queue and is notified; =--remove=
 | bookmark list               | yes | yes | yes |
 | bookmarks on your profile   | n/a | yes | no  |
 | watch, unwatch              | yes | yes | yes |
-| mute                        | yes | no  | yes |
+| mute                        | yes | yes | yes |
 | settings, protection        | yes | yes | yes |
 | default branch              | yes | yes | yes |
 | merge requests only         | yes | yes | yes |
 private drops out of the listing rather than leaking that it exists
 (krz/gitbay#146).
-The web's watch and pin controls write the store directly instead of
+The web's watch and pin controls dispatch =repo pin=/=repo unpin= and
-dispatching =repo watch= and =repo pin=. That is why the web cannot
+=repo watch=/=repo mute=/=repo unwatch=, the same commands the CLI runs
-mute: its toggle knows watching and default only.
+(krz/gitbay#261). The single watch button cycles default, watching and
+muted.
 Dependency checks are off until a repository's admin turns them on: the
 check tells a public registry what the repository depends on. =repo deps

.gitbay/wiki/Threat-Model.org +9 −4

 - *Serve repository HTML on its own origin as active content.* Raw file
   serving is =text/plain= with =nosniff=. Rendered markdown/org is
   sanitized (bluemonday) and served under a CSP that forbids scripts.
-- *Put secrets in argv, URLs, or logs.* Import and mirror credentials,
+- *Put secrets in argv, URLs, or logs, with one documented exception.*
-  registration invites, and API tokens travel on stdin or in request
+  Import and mirror credentials, registration invites, and API tokens
-  bodies, never as command arguments (visible in =/proc=) or query
+  travel on stdin or in request bodies, never as command arguments
-  strings. Tokens are stored only as SHA-256 hashes.
+  (visible in =/proc=) or query strings. The one exception is the
+  emailed login link, =/login?token=...=: single-use, 15-minute expiry,
+  and the response that consumes it carries =Cache-Control: no-store= so
+  no intermediary keeps a copy. An operator running gitbay behind a
+  reverse proxy should configure that proxy to strip the query string
+  from its own access logs. Tokens are stored only as SHA-256 hashes.
 - *Name a mail recipient in the log.* A queued mail is logged by its
   queue row id, never by address, and the relay's own error is redacted
   before it is logged because a rejection usually quotes the address it

CHANGELOG.org +15

	@@ -98,6 +98,21 @@ for the eighteen commands whose CLI path differs from the registry's
98	picked the same way the web page picks one (#268).	98	picked the same way the web page picks one (#268).
99	- =repo show='s mirror table truncates =LAST SYNC= to the second, like	99	- =repo show='s mirror table truncates =LAST SYNC= to the second, like
100	every other timestamp in a view (#268).	100	every other timestamp in a view (#268).
		101	- A =-- foreign_keys: off= migration's =foreign_key_check= now runs
		102	inside the migration's own transaction, before commit, so a
		103	violation rolls the migration back instead of leaving the bad
		104	schema and =user_version= already persisted (#261).
		105	- The web pin and watch buttons dispatch through =repo pin=/=unpin=
		106	and =repo watch=/=mute=/=unwatch= instead of writing the store
		107	directly, so a refusal reaches the viewer as a message instead of
		108	being dropped. The watch button now cycles three states — default,
		109	watching, muted — instead of two (#261).
		110	- The response that consumes a login link's =?token== sends
		111	=Cache-Control: no-store=, so no intermediary keeps a copy of the
		112	single-use URL (#261).
		113	- Wiki documentation fixes: API.org clarifies token commands work on the
		114	API, Parity.org documents batched review and web watch/pin dispatch,
		115	Threat-Model.org documents the login-link URL exception (#261).
101		116
102	* v1.36.0 — 2026-09-23	117	* v1.36.0 — 2026-09-23
103		118

internal/httpd/account_test.go +109

	@@ -175,3 +175,112 @@ func TestAccountPageMasksAShortDeviceToken(t *testing.T) {
175	t.Fatalf("the device table has no id column:\n%s", body)	175	t.Fatalf("the device table has no id column:\n%s", body)
176	}	176	}
177	}	177	}
		178
		179	// assertAudited fails the test unless an audit row with the given action
		180	// prefix exists — proof a handler dispatched through the control
		181	// registry rather than writing the store directly, since only Dispatch
		182	// itself calls Store.Audit.
		183	func assertAudited(t testing.T, st store.Store, prefix string) {
		184	t.Helper()
		185	entries, err := st.AuditEntries(store.AuditFilter{ActionPrefix: prefix, Limit: 10})
		186	if err != nil {
		187	t.Fatal(err)
		188	}
		189	if len(entries) == 0 {
		190	t.Fatalf("no audit row with action prefix %q", prefix)
		191	}
		192	}
		193
		194	// Pinning writes through the repo pin command, not the store directly,
		195	// so it carries the same audit trail and write budget as every other
		196	// mutating command (#261).
		197	func TestPinToggleDispatchesRepoPin(t *testing.T) {
		198	st, err := store.Open(":memory:")
		199	if err != nil {
		200	t.Fatal(err)
		201	}
		202	defer st.Close()
		203	if err := st.MigrateUp(); err != nil {
		204	t.Fatal(err)
		205	}
		206	uid, err := st.CreateUser("alice", false)
		207	if err != nil {
		208	t.Fatal(err)
		209	}
		210	u := store.User{ID: uid, Username: "alice"}
		211	if _, err := st.CreateRepo("user", uid, "app", "public"); err != nil {
		212	t.Fatal(err)
		213	}
		214
		215	s := New(config.Default(), st)
		216	req := httptest.NewRequest("POST", "/alice/app/pin", nil)
		217	req.SetPathValue("owner", "alice")
		218	req.SetPathValue("repo", "app")
		219	rr := httptest.NewRecorder()
		220	s.pinToggle(rr, req, u)
		221
		222	repo, err := st.RepoByPath("alice/app")
		223	if err != nil {
		224	t.Fatal(err)
		225	}
		226	if !st.IsPinned(uid, repo.ID) {
		227	t.Fatal("pin did not take effect")
		228	}
		229	assertAudited(t, st, "cmd repo pin")
		230
		231	rr2 := httptest.NewRecorder()
		232	s.pinToggle(rr2, req, u)
		233	if st.IsPinned(uid, repo.ID) {
		234	t.Fatal("second toggle should have unpinned")
		235	}
		236	assertAudited(t, st, "cmd repo unpin")
		237	}
		238
		239	// The watch button cycles default, watching, muted — the three states
		240	// repo watch/repo mute/repo unwatch already support — rather than the
		241	// two the store-writing version offered (#261, #271).
		242	func TestWatchToggleCyclesThroughMuted(t *testing.T) {
		243	st, err := store.Open(":memory:")
		244	if err != nil {
		245	t.Fatal(err)
		246	}
		247	defer st.Close()
		248	if err := st.MigrateUp(); err != nil {
		249	t.Fatal(err)
		250	}
		251	uid, err := st.CreateUser("alice", false)
		252	if err != nil {
		253	t.Fatal(err)
		254	}
		255	u := store.User{ID: uid, Username: "alice"}
		256	if _, err := st.CreateRepo("user", uid, "app", "public"); err != nil {
		257	t.Fatal(err)
		258	}
		259	repo, err := st.RepoByPath("alice/app")
		260	if err != nil {
		261	t.Fatal(err)
		262	}
		263
		264	s := New(config.Default(), st)
		265	req := httptest.NewRequest("POST", "/alice/app/watch", nil)
		266	req.SetPathValue("owner", "alice")
		267	req.SetPathValue("repo", "app")
		268
		269	click := func() string {
		270	rr := httptest.NewRecorder()
		271	s.watchToggle(rr, req, u)
		272	return st.RepoWatchState(repo.ID, uid)
		273	}
		274	if got := click(); got != "watching" {
		275	t.Fatalf("first click: got %q, want watching", got)
		276	}
		277	assertAudited(t, st, "cmd repo watch")
		278	if got := click(); got != "muted" {
		279	t.Fatalf("second click: got %q, want muted", got)
		280	}
		281	assertAudited(t, st, "cmd repo mute")
		282	if got := click(); got != "" {
		283	t.Fatalf("third click: got %q, want default (unwatched)", got)
		284	}
		285	assertAudited(t, st, "cmd repo unwatch")
		286	}

internal/httpd/accounts.go +12 −4

 }
 func (s *Server) login(w http.ResponseWriter, r *http.Request) {
+	// token, when present, is a single-use secret in the query string —
+	// the documented exception to "never in a URL" (Threat-Model). No
+	// cache may keep a copy of this response.
+	w.Header().Set("Cache-Control", "no-store")
 	token := r.URL.Query().Get("token")
 	if token == "" {
 		s.renderLogin(w, "", false, s.peekNext(r))
 	http.Redirect(w, r, "/"+owner+"/"+name, http.StatusSeeOther)
 }
-// pinToggle pins or unpins the repo for the logged-in viewer.
+// pinToggle pins or unpins the repo for the logged-in viewer, through
+// repo pin/repo unpin — the same commands the CLI runs — rather than
+// writing the store directly (#261).
 func (s *Server) pinToggle(w http.ResponseWriter, r *http.Request, u store.User) {
 	repo, ok := s.repoForUser(w, r, u, policy.CanRead)
 	if !ok {
 		return
 	}
+	verb := "pin"
 	if s.st.IsPinned(u.ID, repo.ID) {
-		s.st.UnpinRepo(u.ID, repo.ID)
+		verb = "unpin"
-	} else {
+	}
-		s.st.PinRepo(u.ID, repo.ID)
+	if _, msg, ok := s.runControl(u, []string{"repo", verb, repo.Path()}); !ok {
+		s.setFlash(w, msg)
 	}
 	http.Redirect(w, r, "/"+repo.Path(), http.StatusSeeOther)
 }

internal/httpd/logincookie_test.go +24

	@@ -2,9 +2,11 @@ package httpd
2		2
3	import (	3	import (
4	"net/http"	4	"net/http"
		5	"net/http/httptest"
5	"testing"	6	"testing"
6		7
7	"gitbay.org/gitbay/internal/config"	8	"gitbay.org/gitbay/internal/config"
		9	"gitbay.org/gitbay/internal/store"
8	)	10	)
9		11
10	// The session cookie must be Lax, not Strict. A login link clicked in a mail	12	// The session cookie must be Lax, not Strict. A login link clicked in a mail
	@@ -36,3 +38,25 @@ func TestSessionCookieAttributes(t *testing.T) {
36	}	38	}
37	}	39	}
38	}	40	}
		41
		42	// The login link's token rides in the query string — the one
		43	// documented exception to "never in a URL" — so the response that
		44	// consumes it must never be cached by an intermediary that might log
		45	// or replay the URL (#261).
		46	func TestLoginNoStoreHeader(t *testing.T) {
		47	st, err := store.Open(":memory:")
		48	if err != nil {
		49	t.Fatal(err)
		50	}
		51	defer st.Close()
		52	if err := st.MigrateUp(); err != nil {
		53	t.Fatal(err)
		54	}
		55	s := New(config.Default(), st)
		56	rr := httptest.NewRecorder()
		57	req := httptest.NewRequest("GET", "/login?token=bogus", nil)
		58	s.login(rr, req)
		59	if got := rr.Header().Get("Cache-Control"); got != "no-store" {
		60	t.Errorf("Cache-Control = %q, want no-store", got)
		61	}
		62	}

internal/httpd/notifyweb.go +7 −6

 	http.Redirect(w, r, "/notifications", http.StatusSeeOther)
 }
-// watchToggle turns watching a repository on and off from its header,
+// watchToggle cycles the viewer's watch state on a repository: default,
-// the way the pin button does.
+// watching, muted, back to default — through repo watch/repo mute/repo
+// unwatch, the same commands the CLI runs (#261, #271).
 func (s *Server) watchToggle(w http.ResponseWriter, r *http.Request, u store.User) {
 	repo, ok := s.repoForUser(w, r, u, policy.CanRead)
 	if !ok {
 		return
 	}
-	if s.st.RepoWatchState(repo.ID, u.ID) == "watching" {
+	next := map[string]string{"": "watch", "watching": "mute", "muted": "unwatch"}
-		s.st.ClearRepoWatch(repo.ID, u.ID)
+	verb := next[s.st.RepoWatchState(repo.ID, u.ID)]
-	} else {
+	if _, msg, ok := s.runControl(u, []string{"repo", verb, repo.Path()}); !ok {
-		s.st.SetRepoWatch(repo.ID, u.ID, "watching")
+		s.setFlash(w, msg)
 	}
 	http.Redirect(w, r, "/"+repo.Path(), http.StatusSeeOther)
 }

internal/store/store.go +80 −76

	@@ -189,51 +189,26 @@ func (s *Store) migrateTo(target int) error {
189	if err != nil {	189	if err != nil {
190	return err	190	return err
191	}	191	}
192	step := func(sqlText string, newVersion int, fkOff bool) (retErr error) {	192	for cur < target {
193	if !fkOff {	193	m := ms[cur]
194	tx, err := s.DB.Begin()	194	if err := s.migrateStep(m.up, m.version, m.upFKOff); err != nil {
195	if err != nil {	195	return fmt.Errorf("migration %d up: %w", m.version, err)
196	return err
197	}
198	defer tx.Rollback()
199	if _, err := tx.Exec(sqlText); err != nil {
200	return err
201	}
202	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
203	return err
204	}
205	return tx.Commit()
206	}
207
208	// A script whose first line is "-- foreign_keys: off" rebuilds a
209	// table that other tables reference (labels, milestones): with
210	// foreign keys on, the rebuild-by-rename loses the children's
211	// rows. PRAGMA foreign_keys is a no-op inside a transaction, and
212	// the pool gives no guarantee that a pragma set on one connection
213	// is seen by the connection Begin() draws next, so the whole step
214	// — pragma off, transaction, pragma on, foreign_key_check — runs
215	// on a single pinned connection.
216	ctx := context.Background()
217	conn, err := s.DB.Conn(ctx)
218	if err != nil {
219	return err
220	}
221	defer conn.Close()
222	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = OFF"); err != nil {
223	return err
224	}	196	}
225	// The connection goes back to the pool when this returns, so every	197	cur = m.version
226	// path out of here has to put foreign keys back on first.	198	}
227	restoreFK := func() error {	199	for cur > target {
228	_, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = ON")	200	m := ms[cur-1]
229	return err	201	if err := s.migrateStep(m.down, m.version-1, m.downFKOff); err != nil {
		202	return fmt.Errorf("migration %d down: %w", m.version, err)
230	}	203	}
231	defer func() {	204	cur = m.version - 1
232	if err := restoreFK(); err != nil && retErr == nil {	205	}
233	retErr = err	206	return nil
234	}	207	}
235	}()	208
236	tx, err := conn.BeginTx(ctx, nil)	209	func (s *Store) migrateStep(sqlText string, newVersion int, fkOff bool) (retErr error) {
		210	if !fkOff {
		211	tx, err := s.DB.Begin()
237	if err != nil {	212	if err != nil {
238	return err	213	return err
239	}	214	}
	@@ -244,44 +219,73 @@ func (s *Store) migrateTo(target int) error {
244	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {	219	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
245	return err	220	return err
246	}	221	}
247	if err := tx.Commit(); err != nil {	222	return tx.Commit()
248	return err
249	}
250	if err := restoreFK(); err != nil {
251	return err
252	}
253	rows, err := conn.QueryContext(ctx, "PRAGMA foreign_key_check")
254	if err != nil {
255	return err
256	}
257	defer rows.Close()
258	if rows.Next() {
259	var table string
260	var rowid sql.NullInt64
261	var referredTable string
262	var fkid int
263	if err := rows.Scan(&table, &rowid, &referredTable, &fkid); err != nil {
264	return err
265	}
266	return fmt.Errorf("foreign_key_check failed after migration: %s", table)
267	}
268	return rows.Err()
269	}	223	}
270	for cur < target {	224
271	m := ms[cur]	225	// A script whose first line is "-- foreign_keys: off" rebuilds a
272	if err := step(m.up, m.version, m.upFKOff); err != nil {	226	// table that other tables reference (labels, milestones): with
273	return fmt.Errorf("migration %d up: %w", m.version, err)	227	// foreign keys on, the rebuild-by-rename loses the children's
		228	// rows. PRAGMA foreign_keys is a no-op inside a transaction, and
		229	// the pool gives no guarantee that a pragma set on one connection
		230	// is seen by the connection Begin() draws next, so the whole step
		231	// — pragma off, transaction, foreign_key_check, commit, pragma on —
		232	// runs on a single pinned connection. The check runs before commit:
		233	// checking after would report a violation once the bad schema and
		234	// user_version were already persisted.
		235	ctx := context.Background()
		236	conn, err := s.DB.Conn(ctx)
		237	if err != nil {
		238	return err
		239	}
		240	defer conn.Close()
		241	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = OFF"); err != nil {
		242	return err
		243	}
		244	// The connection goes back to the pool when this returns, so every
		245	// path out of here has to put foreign keys back on first.
		246	defer func() {
		247	if _, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil && retErr == nil {
		248	retErr = err
274	}	249	}
275	cur = m.version	250	}()
		251	tx, err := conn.BeginTx(ctx, nil)
		252	if err != nil {
		253	return err
276	}	254	}
277	for cur > target {	255	defer tx.Rollback()
278	m := ms[cur-1]	256	if _, err := tx.Exec(sqlText); err != nil {
279	if err := step(m.down, m.version-1, m.downFKOff); err != nil {	257	return err
280	return fmt.Errorf("migration %d down: %w", m.version, err)	258	}
		259	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
		260	return err
		261	}
		262	// foreign_key_check works with enforcement off: it inspects the data
		263	// directly rather than consulting the pragma. Running it here, inside
		264	// the transaction, means a violation rolls back the whole rebuild
		265	// (the deferred tx.Rollback fires) instead of leaving the bad schema
		266	// and version committed.
		267	rows, err := tx.QueryContext(ctx, "PRAGMA foreign_key_check")
		268	if err != nil {
		269	return err
		270	}
		271	if rows.Next() {
		272	var table string
		273	var rowid sql.NullInt64
		274	var referredTable string
		275	var fkid int
		276	if err := rows.Scan(&table, &rowid, &referredTable, &fkid); err != nil {
		277	rows.Close()
		278	return err
281	}	279	}
282	cur = m.version - 1	280	rows.Close()
		281	return fmt.Errorf("foreign_key_check failed after migration: %s row %v", table, rowid)
283	}	282	}
284	return nil	283	if err := rows.Err(); err != nil {
		284	rows.Close()
		285	return err
		286	}
		287	rows.Close()
		288	return tx.Commit()
285	}	289	}
286		290
287	// IsInternal reports whether err is the database or the I/O beneath it	291	// IsInternal reports whether err is the database or the I/O beneath it

internal/store/store_test.go +52

	@@ -255,3 +255,55 @@ func TestMigrationForeignKeysDirective(t *testing.T) {
255	t.Fatalf("foreign_keys after MigrateUp: %d, want 1", fk)	255	t.Fatalf("foreign_keys after MigrateUp: %d, want 1", fk)
256	}	256	}
257	}	257	}
		258
		259	// A migration marked "-- foreign_keys: off" must have its
		260	// foreign_key_check run before the transaction commits, not after —
		261	// otherwise a violation is reported once the bad schema and
		262	// user_version are already persisted (#261).
		263	func TestFKOffMigrationChecksBeforeCommit(t *testing.T) {
		264	s := open(t)
		265	if err := s.MigrateUp(); err != nil {
		266	t.Fatal(err)
		267	}
		268	versionBefore, err := s.Version()
		269	if err != nil {
		270	t.Fatal(err)
		271	}
		272
		273	// Insert a row a fkOff rebuild would have to preserve or complain
		274	// about: a milestone with no matching repo_id (the deliberately
		275	// impossible case a corrupt migration would produce).
		276	if _, err := s.DB.Exec("PRAGMA foreign_keys = OFF"); err != nil {
		277	t.Fatal(err)
		278	}
		279	if _, err := s.DB.Exec(
		280	"INSERT INTO milestones (repo_id, title, state, created_at) VALUES (99999, 'orphan', 'open', datetime('now'))"); err != nil {
		281	t.Fatal(err)
		282	}
		283	if _, err := s.DB.Exec("PRAGMA foreign_keys = ON"); err != nil {
		284	t.Fatal(err)
		285	}
		286
		287	// A no-op fkOff step (rewriting milestones to itself) must now
		288	// refuse — before it commits, not after — because the orphan row
		289	// fails foreign_key_check.
		290	err = s.migrateStep(
		291	"UPDATE sqlite_master SET name = name WHERE 0", versionBefore+1, true)
		292	if err == nil {
		293	t.Fatal("expected foreign_key_check to refuse the orphaned row")
		294	}
		295	after, err := s.Version()
		296	if err != nil {
		297	t.Fatal(err)
		298	}
		299	if after != versionBefore {
		300	t.Fatalf("user_version changed to %d despite the refused check (should stay %d)", after, versionBefore)
		301	}
		302	var fk int
		303	if err := s.DB.QueryRow("PRAGMA foreign_keys").Scan(&fk); err != nil {
		304	t.Fatal(err)
		305	}
		306	if fk != 1 {
		307	t.Fatalf("foreign_keys after refused fkOff step: %d, want 1", fk)
		308	}
		309	}