Commit `6b5f1f02e9`

6b5f1f02e96e490051195218dabbd9009bd22094

parent: 2727fd25d8

Verified · cmc ci/build: success ci/sonar: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 22:51 UTC

pages: a directory redirect cannot leave the site

Both directory redirects passed the raw request path to http.Redirect
while the sibling reqPath a line above was already cleaned. net/url keeps
a leading "//", and Go emits `Location: //evil.example/` unchanged, which
a browser reads as protocol-relative and follows to another origin. I
confirmed that by calling http.Redirect directly rather than reasoning
about it.

Reaching it needed content named like a host under the subdomain's own
owner — a public repository with a pages branch, and repository names
permit dots, so `evil.com` is a legal one. Narrow rather than impossible,
and the fix is smaller than the argument about how narrow.

The destination is now derived from the cleaned path. Two things the
first attempt got wrong, both caught by the test rather than by me:

A path whose ".." escapes the root cleans to "/", and appending a slash
to that produced "//" — the exact string being defended against.

A backslash is not a path separator here, but a browser following the
WHATWG URL rules treats one as a slash, so "/\evil.example/" is another
spelling of "//evil.example/". Building the destination through url.URL
escapes it, and every other byte a path can hold, without a denylist.

Ref #153

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.
		143	func 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 @@
	1	package httpd
	2
	3	import (
	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.
	17	func 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.
	41	func 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.
	52	func 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	}

Commit 6b5f1f02e9

Commit `6b5f1f02e9`