krz/gitbay

Pin the login disabled guard and the checkOrigin invariant !267

merged merged by cmc on 2026-09-05 18:22 UTC · krz/gitbay:pin-security-guards into main

3 files changed, +123 −1

Layout: unified · split

internal/httpd/checkorigin_test.go added +62
@@ -0,0 +1,62 @@
1package httpd
2
3import (
4 "net/http"
5 "net/http/httptest"
6 "testing"
7
8 "gitbay.org/gitbay/internal/config"
9)
10
11// TestMutatingRoutesRequireCheckOrigin pins the invariant checkOrigin's doc
12// comment rests on (#155, #157): the session cookie is SameSite=Lax, which
13// withholds it from a cross-site POST but not a cross-site top-level GET, so
14// checkOrigin has to be the thing that refuses every other mutating route.
15// checkOrigin is the outermost wrapper, so a cross-site Origin is refused
16// with 403 before any handler logic runs — no session or repository needed.
17//
18// Two routes are exempt on purpose, not by omission:
19var checkOriginAllowlist = map[string]string{
20 // Authenticates only via "Authorization: Bearer ...". A cross-site
21 // browser request cannot attach one, so there is no cookie for
22 // checkOrigin to protect here.
23 "POST /api/v1/cmd": "bearer-token auth, no cookie in play",
24 // Consumes a single-use token from the query string and sets no
25 // cookie on failure; browsers withhold Origin from a cross-site
26 // top-level GET, which is the only way this route is ever reached
27 // cross-site.
28 "GET /login": "single-use token GET, no cookie read",
29}
30
31func TestMutatingRoutesRequireCheckOrigin(t *testing.T) {
32 cfg := config.Default()
33 cfg.Web.Mode = "accounts" // superset of accounts-mode routes
34 cfg.API.Enabled = true
35 // /register is gated on registration.mode != "closed" (routes.go), which
36 // config.Default() leaves at "closed" — the production instance runs
37 // "open", and that is the one deployed value the route table hides its
38 // routes behind if this test's cfg does not open it too. "open" and
39 // "invite" gate the route identically (both are just != "closed"), so
40 // there is no second code path in routes.go for looping over both to
41 // reach; "open" alone matches production and is enough.
42 cfg.Registration.Mode = "open"
43 s := New(cfg, nil)
44
45 for _, r := range s.Routes() {
46 if !r.Mutating {
47 continue
48 }
49 key := r.Method + " " + r.Pattern
50 if _, exempt := checkOriginAllowlist[key]; exempt {
51 continue
52 }
53 req := httptest.NewRequest(r.Method, "http://example.com/", nil)
54 req.Header.Set("Origin", "https://evil.example")
55 rr := httptest.NewRecorder()
56 r.Handler(rr, req)
57 if rr.Code != http.StatusForbidden {
58 t.Errorf("%s: cross-site Origin got status %d, want %d (missing checkOrigin?)",
59 key, rr.Code, http.StatusForbidden)
60 }
61 }
62}
internal/httpd/logindisabled_test.go added +56
@@ -0,0 +1,56 @@
1package httpd
2
3import (
4 "net/http/httptest"
5 "strings"
6 "testing"
7 "time"
8
9 "gitbay.org/gitbay/internal/config"
10 "gitbay.org/gitbay/internal/store"
11)
12
13// login() re-reads the user after ConsumeLoginToken and refuses a disabled
14// account with the same badLoginToken page a bad token gets (#155, #156).
15// SetUserDisabled also deletes login_tokens, which would mask this guard if
16// the test went through it, so the token is inserted directly and the row
17// is disabled with a bare UPDATE — bypassing SetUserDisabled entirely.
18func TestLoginRefusesTokenForDisabledAccount(t *testing.T) {
19 st, err := store.Open(":memory:")
20 if err != nil {
21 t.Fatal(err)
22 }
23 defer st.Close()
24 if err := st.MigrateUp(); err != nil {
25 t.Fatal(err)
26 }
27
28 uid, err := st.CreateUser("alice", false)
29 if err != nil {
30 t.Fatal(err)
31 }
32 tok, hash, err := store.NewToken()
33 if err != nil {
34 t.Fatal(err)
35 }
36 if err := st.CreateLoginToken(uid, hash, time.Hour); err != nil {
37 t.Fatal(err)
38 }
39 if _, err := st.DB.Exec("UPDATE users SET disabled = 1 WHERE id = ?", uid); err != nil {
40 t.Fatal(err)
41 }
42
43 s := New(config.Default(), st)
44 rr := httptest.NewRecorder()
45 req := httptest.NewRequest("GET", "/login?token="+tok, nil)
46 s.login(rr, req)
47
48 for _, c := range rr.Result().Cookies() {
49 if c.Name == sessionCookie {
50 t.Fatalf("login set a session cookie for a disabled account: %+v", c)
51 }
52 }
53 if !strings.Contains(rr.Body.String(), badLoginToken) {
54 t.Errorf("body = %q, want the bad-token page", rr.Body.String())
55 }
56}
internal/httpd/routes_test.go +5 −1
@@ -77,7 +77,11 @@ func TestAccountsModeHasLoginRoute(t *testing.T) {
77// unclaimable username. 77// unclaimable username.
78func TestTopLevelRouteWordsAreReserved(t *testing.T) { 78func TestTopLevelRouteWordsAreReserved(t *testing.T) {
79 cfg := config.Default() 79 cfg := config.Default()
80 cfg.Web.Mode = "accounts" // superset of routes 80 cfg.Web.Mode = "accounts" // superset of accounts-mode routes
81 // /register only registers when registration.mode != "closed"
82 // (config.Default() leaves it "closed"); open it so this walk actually
83 // reaches the route the production instance runs with.
84 cfg.Registration.Mode = "open"
81 s := New(cfg, nil) 85 s := New(cfg, nil)
82 for _, r := range s.Routes() { 86 for _, r := range s.Routes() {
83 seg := strings.TrimPrefix(r.Pattern, "/") 87 seg := strings.TrimPrefix(r.Pattern, "/")