krz/gitbay

pages: a directory redirect cannot leave the site !251

merged merged by cmc on 2026-09-05 00:02 UTC · krz/gitbay:sonar-redirect into main

2 files changed, +86 −2

Layout: unified · split

internal/httpd/pages.go +29 −2
@@ -4,6 +4,7 @@ import (
4 "mime" 4 "mime"
5 "net" 5 "net"
6 "net/http" 6 "net/http"
7 "net/url"
7 "path" 8 "path"
8 "strings" 9 "strings"
9 10
@@ -77,7 +78,7 @@ func (s *Server) servePage(w http.ResponseWriter, r *http.Request, host string)
77 dir := control.RepoDir(s.cfg.Server.Root, repo.OwnerName, repo.Name) 78 dir := control.RepoDir(s.cfg.Server.Root, repo.OwnerName, repo.Name)
78 if _, err := gitutil.ResolveRef(dir, PagesBranch); err == nil { 79 if _, err := gitutil.ResolveRef(dir, PagesBranch); err == nil {
79 if rest == "" && !strings.HasSuffix(r.URL.Path, "/") { 80 if rest == "" && !strings.HasSuffix(r.URL.Path, "/") {
80 http.Redirect(w, r, r.URL.Path+"/", http.StatusMovedPermanently) 81 http.Redirect(w, r, pageRedirectTarget(r.URL.Path), http.StatusMovedPermanently)
81 return 82 return
82 } 83 }
83 s.servePageFile(w, r, repo, rest) 84 s.servePageFile(w, r, repo, rest)
@@ -104,7 +105,7 @@ func (s *Server) servePageFile(w http.ResponseWriter, r *http.Request, repo stor
104 // relative links working. 105 // relative links working.
105 if idx, ierr := gitutil.ReadBlob(dir, PagesBranch, filePath+"/index.html", s.cfg.Limits.MaxBlobBytes); ierr == nil { 106 if idx, ierr := gitutil.ReadBlob(dir, PagesBranch, filePath+"/index.html", s.cfg.Limits.MaxBlobBytes); ierr == nil {
106 if !strings.HasSuffix(r.URL.Path, "/") { 107 if !strings.HasSuffix(r.URL.Path, "/") {
107 http.Redirect(w, r, r.URL.Path+"/", http.StatusMovedPermanently) 108 http.Redirect(w, r, pageRedirectTarget(r.URL.Path), http.StatusMovedPermanently)
108 return 109 return
109 } 110 }
110 data, filePath = idx, filePath+"/index.html" 111 data, filePath = idx, filePath+"/index.html"
@@ -125,3 +126,29 @@ func (s *Server) servePageFile(w http.ResponseWriter, r *http.Request, repo stor
125 } 126 }
126 w.Write(data) 127 w.Write(data)
127} 128}
129
130// pageRedirectTarget is the "add a trailing slash" destination for a
131// directory URL, normalised so it cannot leave the site.
132//
133// The raw request path is not safe to redirect to. net/url keeps a
134// leading "//", and Go emits `Location: //evil.example/` unchanged, which
135// a browser reads as protocol-relative and follows to another origin.
136// Reaching it needed content named like a host under the subdomain's
137// owner — repository names permit dots — so it was narrow rather than
138// impossible (gosecurity:S5146, #153).
139//
140// path.Clean collapses the leading slashes and resolves any "..", and the
141// result is re-rooted, so the destination is always one same-origin
142// absolute path.
143func pageRedirectTarget(reqPath string) string {
144 clean := path.Clean("/" + reqPath)
145 if clean == "/" {
146 return "/"
147 }
148 // Encoding through url.URL rather than concatenating: a backslash is
149 // not a path separator here but browsers following the WHATWG URL
150 // rules treat one as a slash, so "/\\evil.example/" would be another
151 // way to say "//evil.example/". Escaping settles that, and every other
152 // byte a path can hold, without a denylist.
153 return (&url.URL{Path: clean + "/"}).String()
154}
internal/httpd/pagesredirect_test.go added +57
@@ -0,0 +1,57 @@
1package httpd
2
3import (
4 "net/http/httptest"
5 "strings"
6 "testing"
7)
8
9// A redirect target built from the request path must not be able to leave
10// the site. `r.URL.Path` keeps a leading `//` — Go's URL parser does not
11// normalise it — and `Location: //evil.example/` is protocol-relative, so
12// a browser follows it to another origin (gosecurity:S5146, #153).
13//
14// The directory-redirect targets are derived from the cleaned path, so
15// this asserts the property rather than the spelling: whatever the code
16// emits, it must be a same-origin path.
17func TestPageRedirectCannotLeaveTheSite(t *testing.T) {
18 cases := []string{
19 "//evil.example",
20 "//evil.example/deep",
21 "///evil.example",
22 "/\\evil.example",
23 "//evil.example/../..",
24 }
25 for _, p := range cases {
26 got := pageRedirectTarget(p)
27 if got == "" {
28 continue // no redirect for this shape is a fine answer
29 }
30 if strings.HasPrefix(got, "//") || strings.HasPrefix(got, "/\\") {
31 t.Errorf("request %q redirects to %q, which a browser reads as another origin", p, got)
32 }
33 if !strings.HasPrefix(got, "/") {
34 t.Errorf("request %q redirects to %q, which is not an absolute path", p, got)
35 }
36 }
37}
38
39// The ordinary case still behaves: a directory URL without a trailing
40// slash gains one, so relative links inside the page resolve.
41func TestPageRedirectAddsTrailingSlash(t *testing.T) {
42 if got, want := pageRedirectTarget("/guide"), "/guide/"; got != want {
43 t.Errorf("pageRedirectTarget(/guide) = %q, want %q", got, want)
44 }
45 if got, want := pageRedirectTarget("/a/b/c"), "/a/b/c/"; got != want {
46 t.Errorf("pageRedirectTarget(/a/b/c) = %q, want %q", got, want)
47 }
48}
49
50// Guard against a regression in the shape the fix relies on: httptest
51// builds the request the same way the server sees it.
52func TestRawPathKeepsDoubleSlash(t *testing.T) {
53 r := httptest.NewRequest("GET", "http://host//evil.example", nil)
54 if r.URL.Path != "//evil.example" {
55 t.Skipf("net/url normalised the path to %q; the hazard this guards is gone", r.URL.Path)
56 }
57}