!496 · krz/gitbay

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

		@@ -59,8 +59,9 @@ the command wrote diagnostics:
59	59
60	60	HTTP status maps the exit code: 0→200, 2→400, 3→404, 4→403, else 500.
61	61	Commands that emit raw text rather than an envelope (=help=, =mr diff=)
62		come wrapped as ={"output": "..."}=. Git transport commands and the
63		token commands are refused by name.
	62	come wrapped as ={"output": "..."}=. Git transport commands are refused
	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	66	#+begin_src sh
66	67	curl -s -H "Authorization: Bearer $TOKEN" \

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

		@@ -80,9 +80,13 @@ does not appear in anyone's review queue. Draft is a flag rather than a
80	80	fifth state, so every =state = 'open'= rule still means what it did.
81	81	=mr revisions= lists the heads a merge request has had; the web page
82	82	lists them beside the reviews they staled, and the iOS client has a
83		Revisions section. =mr range-diff= compares two heads: the iOS client
84		shows it from a revision to the one before, as text; the web has no
85		view. Batched review is not built.
	83	Revisions section. =mr range-diff= compares two heads: the iOS client shows it from a
	84	revision to the one before, as text; the web has no view yet
	85	(krz/gitbay#269). Batched review — draft diff comments held with
	86	=mr comment --pending= and sent together with =--comment=/=--discard=
	87	or a verdict — is built and the web uses it: composing review
	88	comments before publishing them is the same round trip as the CLI's
	89	=--pending= flag.
86	90
87	91	=mr review request --add <user>= asks a particular person, who then
88	92	carries the merge request in their queue and is notified; =--remove=
		@@ -187,7 +191,7 @@ rather than the one the web page shows.
187	191	\| bookmark list \| yes \| yes \| yes \|
188	192	\| bookmarks on your profile \| n/a \| yes \| no \|
189	193	\| watch, unwatch \| yes \| yes \| yes \|
190		\| mute \| yes \| no \| yes \|
	194	\| mute \| yes \| yes \| yes \|
191	195	\| settings, protection \| yes \| yes \| yes \|
192	196	\| default branch \| yes \| yes \| yes \|
193	197	\| merge requests only \| yes \| yes \| yes \|
		@@ -247,9 +251,10 @@ repository — and a repository bookmarked while public and since made
247	251	private drops out of the listing rather than leaking that it exists
248	252	(krz/gitbay#146).
249	253
250		The web's watch and pin controls write the store directly instead of
251		dispatching =repo watch= and =repo pin=. That is why the web cannot
252		mute: its toggle knows watching and default only.
	254	The web's watch and pin controls dispatch =repo pin=/=repo unpin= and
	255	=repo watch=/=repo mute=/=repo unwatch=, the same commands the CLI runs
	256	(krz/gitbay#261). The single watch button cycles default, watching and
	257	muted.
253	258
254	259	Dependency checks are off until a repository's admin turns them on: the
255	260	check tells a public registry what the repository depends on. =repo deps

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

		@@ -17,10 +17,15 @@ matrix and the open gaps are in the [[file:Architecture/00-Overview.org][Archite
17	17	- Serve repository HTML on its own origin as active content. Raw file
18	18	serving is =text/plain= with =nosniff=. Rendered markdown/org is
19	19	sanitized (bluemonday) and served under a CSP that forbids scripts.
20		- Put secrets in argv, URLs, or logs. Import and mirror credentials,
21		registration invites, and API tokens travel on stdin or in request
22		bodies, never as command arguments (visible in =/proc=) or query
23		strings. Tokens are stored only as SHA-256 hashes.
	20	- Put secrets in argv, URLs, or logs, with one documented exception.
	21	Import and mirror credentials, registration invites, and API tokens
	22	travel on stdin or in request bodies, never as command arguments
	23	(visible in =/proc=) or query strings. The one exception is the
	24	emailed login link, =/login?token=...=: single-use, 15-minute expiry,
	25	and the response that consumes it carries =Cache-Control: no-store= so
	26	no intermediary keeps a copy. An operator running gitbay behind a
	27	reverse proxy should configure that proxy to strip the query string
	28	from its own access logs. Tokens are stored only as SHA-256 hashes.
24	29	- Name a mail recipient in the log. A queued mail is logged by its
25	30	queue row id, never by address, and the relay's own error is redacted
26	31	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	98	picked the same way the web page picks one (#268).
99	99	- =repo show='s mirror table truncates =LAST SYNC= to the second, like
100	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	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	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

		@@ -121,6 +121,10 @@ func (s Server) loginSubmit(w http.ResponseWriter, r http.Request) {
121	121	}
122	122
123	123	func (s Server) login(w http.ResponseWriter, r http.Request) {
	124	// token, when present, is a single-use secret in the query string —
	125	// the documented exception to "never in a URL" (Threat-Model). No
	126	// cache may keep a copy of this response.
	127	w.Header().Set("Cache-Control", "no-store")
124	128	token := r.URL.Query().Get("token")
125	129	if token == "" {
126	130	s.renderLogin(w, "", false, s.peekNext(r))
		@@ -240,16 +244,20 @@ func (s Server) newSubmit(w http.ResponseWriter, r http.Request, u store.User)
240	244	http.Redirect(w, r, "/"+owner+"/"+name, http.StatusSeeOther)
241	245	}
242	246
243		// pinToggle pins or unpins the repo for the logged-in viewer.
	247	// pinToggle pins or unpins the repo for the logged-in viewer, through
	248	// repo pin/repo unpin — the same commands the CLI runs — rather than
	249	// writing the store directly (#261).
244	250	func (s Server) pinToggle(w http.ResponseWriter, r http.Request, u store.User) {
245	251	repo, ok := s.repoForUser(w, r, u, policy.CanRead)
246	252	if !ok {
247	253	return
248	254	}
	255	verb := "pin"
249	256	if s.st.IsPinned(u.ID, repo.ID) {
250		s.st.UnpinRepo(u.ID, repo.ID)
251		} else {
252		s.st.PinRepo(u.ID, repo.ID)
	257	verb = "unpin"
	258	}
	259	if _, msg, ok := s.runControl(u, []string{"repo", verb, repo.Path()}); !ok {
	260	s.setFlash(w, msg)
253	261	}
254	262	http.Redirect(w, r, "/"+repo.Path(), http.StatusSeeOther)
255	263	}

internal/httpd/logincookie_test.go +24

		@@ -2,9 +2,11 @@ package httpd
2	2
3	3	import (
4	4	"net/http"
	5	"net/http/httptest"
5	6	"testing"
6	7
7	8	"gitbay.org/gitbay/internal/config"
	9	"gitbay.org/gitbay/internal/store"
8	10	)
9	11
10	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

		@@ -56,17 +56,18 @@ func (s Server) notificationsRead(w http.ResponseWriter, r http.Request, u sto
56	56	http.Redirect(w, r, "/notifications", http.StatusSeeOther)
57	57	}
58	58
59		// watchToggle turns watching a repository on and off from its header,
60		// the way the pin button does.
	59	// watchToggle cycles the viewer's watch state on a repository: default,
	60	// watching, muted, back to default — through repo watch/repo mute/repo
	61	// unwatch, the same commands the CLI runs (#261, #271).
61	62	func (s Server) watchToggle(w http.ResponseWriter, r http.Request, u store.User) {
62	63	repo, ok := s.repoForUser(w, r, u, policy.CanRead)
63	64	if !ok {
64	65	return
65	66	}
66		if s.st.RepoWatchState(repo.ID, u.ID) == "watching" {
67		s.st.ClearRepoWatch(repo.ID, u.ID)
68		} else {
69		s.st.SetRepoWatch(repo.ID, u.ID, "watching")
	67	next := map[string]string{"": "watch", "watching": "mute", "muted": "unwatch"}
	68	verb := next[s.st.RepoWatchState(repo.ID, u.ID)]
	69	if _, msg, ok := s.runControl(u, []string{"repo", verb, repo.Path()}); !ok {
	70	s.setFlash(w, msg)
70	71	}
71	72	http.Redirect(w, r, "/"+repo.Path(), http.StatusSeeOther)
72	73	}

internal/store/store.go +80 −76

		@@ -189,51 +189,26 @@ func (s *Store) migrateTo(target int) error {
189	189	if err != nil {
190	190	return err
191	191	}
192		step := func(sqlText string, newVersion int, fkOff bool) (retErr error) {
193		if !fkOff {
194		tx, err := s.DB.Begin()
195		if err != nil {
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
	192	for cur < target {
	193	m := ms[cur]
	194	if err := s.migrateStep(m.up, m.version, m.upFKOff); err != nil {
	195	return fmt.Errorf("migration %d up: %w", m.version, err)
224	196	}
225		// The connection goes back to the pool when this returns, so every
226		// path out of here has to put foreign keys back on first.
227		restoreFK := func() error {
228		_, err := conn.ExecContext(ctx, "PRAGMA foreign_keys = ON")
229		return err
	197	cur = m.version
	198	}
	199	for cur > target {
	200	m := ms[cur-1]
	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() {
232		if err := restoreFK(); err != nil && retErr == nil {
233		retErr = err
234		}
235		}()
236		tx, err := conn.BeginTx(ctx, nil)
	204	cur = m.version - 1
	205	}
	206	return nil
	207	}
	208
	209	func (s *Store) migrateStep(sqlText string, newVersion int, fkOff bool) (retErr error) {
	210	if !fkOff {
	211	tx, err := s.DB.Begin()
237	212	if err != nil {
238	213	return err
239	214	}
		@@ -244,44 +219,73 @@ func (s *Store) migrateTo(target int) error {
244	219	if _, err := tx.Exec(fmt.Sprintf("PRAGMA user_version = %d", newVersion)); err != nil {
245	220	return err
246	221	}
247		if err := tx.Commit(); err != nil {
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()
	222	return tx.Commit()
269	223	}
270		for cur < target {
271		m := ms[cur]
272		if err := step(m.up, m.version, m.upFKOff); err != nil {
273		return fmt.Errorf("migration %d up: %w", m.version, err)
	224
	225	// A script whose first line is "-- foreign_keys: off" rebuilds a
	226	// table that other tables reference (labels, milestones): with
	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 {
278		m := ms[cur-1]
279		if err := step(m.down, m.version-1, m.downFKOff); err != nil {
280		return fmt.Errorf("migration %d down: %w", m.version, err)
	255	defer tx.Rollback()
	256	if _, err := tx.Exec(sqlText); err != nil {
	257	return 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	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	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	}

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