Commit b0fba1c090
b0fba1c090202ac599e31fe580739070e2a4d750
parent: ca4d0e9517
Verified · cmc ci/build: success ci/sonar: success ci/test: success ci/vuln: success
cmc <hello@cleberg.net> · 2026-09-04 23:13 UTC
web: clearing a cookie carries the attributes that set it
Logout and flash consumption expired their cookies with a bare
Set-Cookie while the setting calls specify HttpOnly, SameSite and a
Secure that follows TLS.
Deletion works either way — a cookie is identified by name, domain and
path, not by its flags — so this is consistency rather than a live bug.
It is worth doing because a reviewer comparing the two paths should not
have to work out whether the difference is deliberate, and because four
scanner findings resolve to one two-line change.
Secure still follows TLS rather than being forced on: an instance
serving plain HTTP is a supported deployment, and a Secure cookie there
would be one the browser refuses to send back, which for a clearing
cookie means one it never expires.
Ref #153
Layout: unified · split
internal/httpd/accounts.go
+1 −1
| @@ -103,7 +103,7 @@ func (s *Server) logout(w http.ResponseWriter, r *http.Request) { |
| 103 | 103 | if ck, err := r.Cookie(sessionCookie); err == nil { |
| 104 | 104 | s.st.DeleteWebSession(store.HashToken(ck.Value)) |
| 105 | 105 | } |
| 106 | | http.SetCookie(w, &http.Cookie{Name: sessionCookie, Value: "", Path: "/", MaxAge: -1}) |
| 106 | http.SetCookie(w, s.clearCookie(sessionCookie, http.SameSiteStrictMode)) |
| 107 | 107 | http.Redirect(w, r, "/", http.StatusSeeOther) |
| 108 | 108 | } |
| 109 | 109 | |
internal/httpd/cookieclear_test.go
added
+39
| @@ -0,0 +1,39 @@ |
| 1 | package httpd |
| 2 | |
| 3 | import ( |
| 4 | "net/http" |
| 5 | "testing" |
| 6 | |
| 7 | "gitbay.org/gitbay/internal/config" |
| 8 | ) |
| 9 | |
| 10 | // A cookie that clears a session should carry the attributes the one that |
| 11 | // set it carried. Deletion works without them, so this is consistency — |
| 12 | // but a reviewer comparing the two paths should not have to work out |
| 13 | // whether the difference is deliberate (go:S2092, go:S3330, #153). |
| 14 | func TestClearCookieMirrorsTheSettingCall(t *testing.T) { |
| 15 | for _, tls := range []string{"acme", "off"} { |
| 16 | s := &Server{cfg: config.Config{}} |
| 17 | s.cfg.HTTP.TLS = tls |
| 18 | c := s.clearCookie(sessionCookie, http.SameSiteStrictMode) |
| 19 | |
| 20 | if c.Value != "" || c.MaxAge >= 0 { |
| 21 | t.Errorf("tls=%s: not an expiring cookie: value=%q maxage=%d", tls, c.Value, c.MaxAge) |
| 22 | } |
| 23 | if !c.HttpOnly { |
| 24 | t.Errorf("tls=%s: clearing cookie is not HttpOnly", tls) |
| 25 | } |
| 26 | if c.SameSite != http.SameSiteStrictMode { |
| 27 | t.Errorf("tls=%s: SameSite = %v, want Strict", tls, c.SameSite) |
| 28 | } |
| 29 | if c.Path != "/" { |
| 30 | t.Errorf("tls=%s: Path = %q, want /", tls, c.Path) |
| 31 | } |
| 32 | // Secure follows TLS exactly as the setting calls do: forcing it |
| 33 | // on would make the cookie undeletable over plain HTTP, which is |
| 34 | // a supported deployment. |
| 35 | if want := tls != "off"; c.Secure != want { |
| 36 | t.Errorf("tls=%s: Secure = %v, want %v", tls, c.Secure, want) |
| 37 | } |
| 38 | } |
| 39 | } |
internal/httpd/flash.go
+19 −1
| @@ -35,10 +35,28 @@ func (s *Server) takeFlash(w http.ResponseWriter, r *http.Request) string { |
| 35 | 35 | if err != nil || c.Value == "" { |
| 36 | 36 | return "" |
| 37 | 37 | } |
| 38 | | http.SetCookie(w, &http.Cookie{Name: flashCookie, Value: "", Path: "/", MaxAge: -1}) |
| 38 | http.SetCookie(w, s.clearCookie(flashCookie, http.SameSiteLaxMode)) |
| 39 | 39 | msg, err := url.QueryUnescape(c.Value) |
| 40 | 40 | if err != nil { |
| 41 | 41 | return "" |
| 42 | 42 | } |
| 43 | 43 | return msg |
| 44 | 44 | } |
| 45 | |
| 46 | // clearCookie is the expiring twin of a Set-Cookie, carrying the same |
| 47 | // attributes the setting call used. |
| 48 | // |
| 49 | // Deletion works without them — a cookie is identified by name, domain |
| 50 | // and path, not by its flags — so this is consistency rather than a live |
| 51 | // bug. It is worth having because a reviewer comparing the set and clear |
| 52 | // paths should not have to work out whether the difference is deliberate, |
| 53 | // and because a scanner will otherwise flag the bare form every time |
| 54 | // (go:S2092, go:S3330, #153). |
| 55 | func (s *Server) clearCookie(name string, sameSite http.SameSite) *http.Cookie { |
| 56 | return &http.Cookie{ |
| 57 | Name: name, Value: "", Path: "/", |
| 58 | HttpOnly: true, SameSite: sameSite, |
| 59 | Secure: s.cfg.HTTP.TLS != "off", |
| 60 | MaxAge: -1, |
| 61 | } |
| 62 | } |