krz/gitbay

A CLI-first git forge. cli forge git self-hosted https://gitbay.org

Commit 3f7a91e284

3f7a91e28444fe0ea1c42d105fd7e508f518e4cc

parent: 090dc2eb20

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-05 03:44 UTC 

web: close the login-link timing side channel, cover the username path

RequestLoginLink sent mail synchronously, so a hit cost a full SMTP round
trip to the relay while every miss returned after one local query --
separable over the network in a handful of samples, leaking exactly what
the response body was built not to. Mail now goes out from a goroutine so
every case returns on the same DB-bound path.

Also: cover the username branch (untested until now), pin the throttle
test to the exact budget instead of an upper bound that a broken endpoint
would also satisfy, and check the throttled response against the first
rather than assuming the code already does the right thing there.

Ref #155

Layout: unified · split

e2e/emaillogin_test.go +46 −3
@@ -5,6 +5,7 @@ import (
5 "net/url" 5 "net/url"
6 "strings" 6 "strings"
7 "testing" 7 "testing"
8 "time"
8) 9)
9 10
10// A person with no SSH key can still get into the web UI: they ask for a 11// A person with no SSH key can still get into the web UI: they ask for a
@@ -53,6 +54,21 @@ func TestEmailLogin(t *testing.T) {
53 if _, body := browserGet(t, second, inst.base()+"/settings"); strings.Contains(body, "dana@example.test") { 54 if _, body := browserGet(t, second, inst.base()+"/settings"); strings.Contains(body, "dana@example.test") {
54 t.Error("login link worked twice") 55 t.Error("login link worked twice")
55 } 56 }
57
58 // The identifier can also be a bare username; it resolves to the
59 // account's verified address the same way an email address does.
60 status, body = browserPost(t, browser, inst.base()+"/login",
61 url.Values{"identifier": {"dana"}})
62 if status != 200 || !strings.Contains(body, "on its way") {
63 t.Fatalf("POST /login by username: %d", status)
64 }
65 deadline := time.Now().Add(2 * time.Second)
66 for time.Now().Before(deadline) && len(smtp.mailTo("dana@example.test")) < 2 {
67 time.Sleep(25 * time.Millisecond)
68 }
69 if n := len(smtp.mailTo("dana@example.test")); n != 2 {
70 t.Fatalf("login by username did not mail a second link: got %d mails, want 2", n)
71 }
56} 72}
57 73
58// The response must not say whether an account exists. A different status, 74// The response must not say whether an account exists. A different status,
@@ -77,6 +93,8 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
77 url.Values{"identifier": {"eve@example.test"}}) 93 url.Values{"identifier": {"eve@example.test"}})
78 empty, bodyEmpty := browserPost(t, browser, inst.base()+"/login", 94 empty, bodyEmpty := browserPost(t, browser, inst.base()+"/login",
79 url.Values{"identifier": {""}}) 95 url.Values{"identifier": {""}})
96 absentUser, bodyAbsentUser := browserPost(t, browser, inst.base()+"/login",
97 url.Values{"identifier": {"nosuchuser"}})
80 98
81 for _, c := range []struct { 99 for _, c := range []struct {
82 name string 100 name string
@@ -86,6 +104,7 @@ func TestEmailLoginDoesNotEnumerate(t *testing.T) {
86 {"absent", absent, bodyAbsent}, 104 {"absent", absent, bodyAbsent},
87 {"unverified", unver, bodyUnver}, 105 {"unverified", unver, bodyUnver},
88 {"empty", empty, bodyEmpty}, 106 {"empty", empty, bodyEmpty},
107 {"absent-username", absentUser, bodyAbsentUser},
89 } { 108 } {
90 if c.status != real1 || c.body != bodyReal { 109 if c.status != real1 || c.body != bodyReal {
91 t.Errorf("%s differs from a real address: status %d vs %d", c.name, c.status, real1) 110 t.Errorf("%s differs from a real address: status %d vs %d", c.name, c.status, real1)
@@ -110,11 +129,35 @@ func TestEmailLoginThrottled(t *testing.T) {
110 "--email", "dana@example.test", "--verified") 129 "--email", "dana@example.test", "--verified")
111 130
112 browser := newBrowser(t) 131 browser := newBrowser(t)
132 var first, sixth string
113 for i := 0; i < 6; i++ { 133 for i := 0; i < 6; i++ {
114 browserPost(t, browser, inst.base()+"/login", 134 _, body := browserPost(t, browser, inst.base()+"/login",
115 url.Values{"identifier": {"dana@example.test"}}) 135 url.Values{"identifier": {"dana@example.test"}})
136 switch i {
137 case 0:
138 first = body
139 case 5:
140 sixth = body
141 }
142 }
143 // Being over the throttle is one more class whose response must not
144 // differ from an ordinary request.
145 if sixth != first {
146 t.Error("the throttled response differs from the first")
147 }
148
149 // Mail goes out from a goroutine, not on the request path, so give the
150 // last permitted one time to land before counting.
151 var n int
152 deadline := time.Now().Add(2 * time.Second)
153 for time.Now().Before(deadline) {
154 n = len(smtp.mailTo("dana@example.test"))
155 if n >= 5 {
156 break
157 }
158 time.Sleep(25 * time.Millisecond)
116 } 159 }
117 if n := len(smtp.mailTo("dana@example.test")); n > 5 { 160 if n != 5 {
118 t.Fatalf("sent %d login mails in an hour, want at most 5", n) 161 t.Fatalf("sent %d login mails in an hour, want exactly 5", n)
119 } 162 }
120} 163}
internal/control/loginlink.go +19 −2
@@ -2,6 +2,7 @@ package control
2 2
3import ( 3import (
4 "fmt" 4 "fmt"
5 "log/slog"
5 "strings" 6 "strings"
6 "time" 7 "time"
7 8
@@ -12,7 +13,11 @@ import (
12 13
13// maxLoginLinksPerHour bounds what one account's address can be made to 14// maxLoginLinksPerHour bounds what one account's address can be made to
14// receive. It matches maxEmailAddsPerHour: enough for a person who mistypes 15// receive. It matches maxEmailAddsPerHour: enough for a person who mistypes
15// and retries, nothing for a script. 16// and retries, nothing for a script. The counter is shared with SSH-minted
17// links, not just these: CountLoginTokensSince counts every row in
18// login_tokens, and "web login" over SSH inserts into that same table
19// without consulting this bound, so five "ssh git@host web login" calls in
20// an hour also spend an account's budget here.
16const maxLoginLinksPerHour = 5 21const maxLoginLinksPerHour = 5
17 22
18// loginLinkTTL is longer than the five minutes an SSH-minted link gets. 23// loginLinkTTL is longer than the five minutes an SSH-minted link gets.
@@ -82,5 +87,17 @@ func RequestLoginLink(cfg config.Config, st *store.Store, identifier string) err
82 "Open this link within 15 minutes. It works once:\n\n %s/login?token=%s\n\n"+ 87 "Open this link within 15 minutes. It works once:\n\n %s/login?token=%s\n\n"+
83 "If this wasn't you, ignore this mail. Nothing has changed on the account.\n", 88 "If this wasn't you, ignore this mail. Nothing has changed on the account.\n",
84 host, strings.TrimSuffix(cfg.Server.SiteURL, "/"), token) 89 host, strings.TrimSuffix(cfg.Server.SiteURL, "/"), token)
85 return mail.Send(cfg, address, "log in to "+host, body) 90 subject := "log in to " + host
91
92 // Sent in the background: mail.Send is a synchronous SMTP round trip to
93 // the relay, tens to hundreds of milliseconds against the sub-millisecond
94 // a miss takes to answer. Returning before it completes keeps every case
95 // — hit, miss, unverified, throttled — on the same DB-bound path, so
96 // response time cannot answer what the response body is built not to.
97 go func() {
98 if err := mail.Send(cfg, address, subject, body); err != nil {
99 slog.Error("login link mail", "address", address, "err", err)
100 }
101 }()
102 return nil
86} 103}