fix(web): redirect /<owner>/<rest> unless a repository is named like the owner

The redirect no longer depends on the route the path would match: a
repository named like a page (actions, commits...) is reachable under the
owner prefix. Adds a table test covering open-redirect attempts, methods,
query and git smart HTTP.

commit: 43dcef47fabdfa753c40dd70bfd436e346f80448
author: brice · 9d ago · 2026-10-13 00:08 UTC
parent: 3690ef9
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +157 -7

A internal/adapters/web/ownerredirect_test.go +119 -0

@@ -0,0 +1,119 @@
	1	+	// SPDX-License-Identifier: AGPL-3.0-or-later
	2	+
	3	+	package web
	4	+
	5	+	import (
	6	+	"errors"
	7	+	"net/http"
	8	+	"net/http/httptest"
	9	+	"testing"
	10	+
	11	+	"github.com/stretchr/testify/assert"
	12	+	"github.com/stretchr/testify/mock"
	13	+	"github.com/stretchr/testify/require"
	14	+
	15	+	"bnema.dev/ermite/internal/domain"
	16	+	"bnema.dev/ermite/internal/ports/mocks"
	17	+	)
	18	+
	19	+	const testOwner = "owner"
	20	+
	21	+	// ownerRedirect sends /<owner>/<rest> to /<rest> whenever no repository named
	22	+	// like the owner is visible, and only for a clean repository segment.
	23	+	func TestOwnerRedirect(t *testing.T) {
	24	+	t.Parallel()
	25	+	none := reposServing(t)
	26	+	tests := []struct {
	27	+	name string
	28	+	method string
	29	+	target string
	30	+	repos func(t testing.T) mocks.RepoService
	31	+	want string // "" : not redirected
	32	+	}{
	33	+	{"a repository page", http.MethodGet, "/owner/demo", nil, "/demo"},
	34	+	{"a repository page with a slash", http.MethodGet, "/owner/demo/", nil, "/demo/"},
	35	+	{"a sub-route", http.MethodGet, "/owner/demo/commit/abc123", nil, "/demo/commit/abc123"},
	36	+	{"head", http.MethodHead, "/owner/demo", nil, "/demo"},
	37	+	{"post is not redirected", http.MethodPost, "/owner/demo", nil, ""},
	38	+	{"query is kept", http.MethodGet, "/owner/demo/commits/main?a=1&b=x%20y", nil, "/demo/commits/main?a=1&b=x%20y"},
	39	+	{"a repository named like a route", http.MethodGet, "/owner/actions", nil, "/actions"},
	40	+	{"a repository named like a route, with a path", http.MethodGet, "/owner/actions/actions/runs/1", nil, "/actions/actions/runs/1"},
	41	+	{"git smart HTTP", http.MethodGet, "/owner/repo.git/info/refs?service=git-upload-pack", nil, "/repo.git/info/refs?service=git-upload-pack"},
	42	+	{"the owner alone", http.MethodGet, "/owner", nil, ""},
	43	+	{"the owner with a slash", http.MethodGet, "/owner/", nil, ""},
	44	+	{"another first segment", http.MethodGet, "/other/demo", nil, ""},
	45	+	{"a prefix of the owner", http.MethodGet, "/owners/demo", nil, ""},
	46	+	{"a double slash host", http.MethodGet, "//evil.com", nil, ""},
	47	+	{"a double slash host after the owner", http.MethodGet, "/owner//evil.com", nil, ""},
	48	+	{"an encoded slash", http.MethodGet, "/owner/%2Fevil.com", nil, ""},
	49	+	{"an encoded slash after the repository", http.MethodGet, "/owner/demo%2F/x", nil, ""},
	50	+	{"an encoded backslash", http.MethodGet, "/owner/%5Cevil.com", nil, ""},
	51	+	{"a backslash", http.MethodGet, `/owner/\evil.com`, nil, ""},
	52	+	{"dot dot", http.MethodGet, "/owner/..", nil, ""},
	53	+	{"dot dot after the repository", http.MethodGet, "/owner/../etc", nil, ""},
	54	+	{"encoded dot dot", http.MethodGet, "/owner/%2e%2e/etc", nil, ""},
	55	+	{"a dot", http.MethodGet, "/owner/.", nil, ""},
	56	+	{"an uppercase name", http.MethodGet, "/owner/Demo", nil, ""},
	57	+	{"the /-/ routes", http.MethodGet, "/owner/-/events", nil, ""},
	58	+	{"a bare .git", http.MethodGet, "/owner/.git/info/refs", nil, ""},
	59	+	{"a repository really named like the owner", http.MethodGet, "/owner/demo", func(t testing.T) mocks.RepoService {
	60	+	return reposServing(t, publicRepo(t, testOwner))
	61	+	}, ""},
	62	+	{"a route of the repository named like the owner", http.MethodGet, "/owner/actions", func(t testing.T) mocks.RepoService {
	63	+	return reposServing(t, publicRepo(t, testOwner), publicRepo(t, "actions"))
	64	+	}, ""},
	65	+	{"a private repository named like the owner is a missing one", http.MethodGet, "/owner/demo", func(t testing.T) mocks.RepoService {
	66	+	m := mocks.NewRepoService(t)
	67	+	m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).Return(domain.Repo{}, domain.ErrNotFound)
	68	+	return m
	69	+	}, "/demo"},
	70	+	{"a failing lookup serves the request as it is", http.MethodGet, "/owner/demo", func(t testing.T) mocks.RepoService {
	71	+	return reposFailing(t, errors.New("store down"))
	72	+	}, ""},
	73	+	}
	74	+	for _, tt := range tests {
	75	+	t.Run(tt.name, func(t *testing.T) {
	76	+	t.Parallel()
	77	+	repos := none
	78	+	if tt.repos != nil {
	79	+	repos = tt.repos(t)
	80	+	}
	81	+	srv := newTestServer(t, Config{Repos: repos, Browse: mocks.NewBrowseService(t), Owner: testOwner})
	82	+	req := httptest.NewRequest(tt.method, tt.target, nil)
	83	+	to, ok := srv.ownerRedirect(req)
	84	+	assert.Equal(t, tt.want != "", ok)
	85	+	assert.Equal(t, tt.want, to)
	86	+	if !ok {
	87	+	return
	88	+	}
	89	+	// Whatever the target is, it stays on this origin.
	90	+	require.True(t, to[0] == '/' && (len(to) == 1 \|\| (to[1] != '/' && to[1] != '\\')), "target %q", to)
	91	+	})
	92	+	}
	93	+	}
	94	+
	95	+	// Through ServeHTTP the redirect is a 308 with the Location, and the paths
	96	+	// that are not redirected reach the router.
	97	+	func TestOwnerRedirectServed(t *testing.T) {
	98	+	t.Parallel()
	99	+	srv := newTestServer(t, Config{Repos: reposServing(t), Browse: mocks.NewBrowseService(t), Owner: testOwner})
	100	+	rec := httptest.NewRecorder()
	101	+	srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/owner/actions?x=1", nil))
	102	+	assert.Equal(t, http.StatusPermanentRedirect, rec.Code)
	103	+	assert.Equal(t, "/actions?x=1", rec.Header().Get("Location"))
	104	+
	105	+	rec = httptest.NewRecorder()
	106	+	srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "//evil.com", nil))
	107	+	// The mux cleans the path itself and answers with a local redirect: the
	108	+	// owner redirect adds none, and nothing leads off this origin.
	109	+	assert.NotEqual(t, http.StatusPermanentRedirect, rec.Code)
	110	+	assert.Equal(t, "/evil.com", rec.Header().Get("Location"))
	111	+	}
	112	+
	113	+	// Without an owner there is no redirect.
	114	+	func TestOwnerRedirectOff(t *testing.T) {
	115	+	t.Parallel()
	116	+	srv := newTestServer(t, Config{Repos: reposServing(t), Browse: mocks.NewBrowseService(t)})
	117	+	_, ok := srv.ownerRedirect(httptest.NewRequest(http.MethodGet, "/owner/demo", nil))
	118	+	assert.False(t, ok)
	119	+	}

@@ -0,0 +1,119 @@
	1	+	// SPDX-License-Identifier: AGPL-3.0-or-later
	2	+
	3	+	package web
	4	+
	5	+	import (
	6	+	"errors"
	7	+	"net/http"
	8	+	"net/http/httptest"
	9	+	"testing"
	10	+
	11	+	"github.com/stretchr/testify/assert"
	12	+	"github.com/stretchr/testify/mock"
	13	+	"github.com/stretchr/testify/require"
	14	+
	15	+	"bnema.dev/ermite/internal/domain"
	16	+	"bnema.dev/ermite/internal/ports/mocks"
	17	+	)
	18	+
	19	+	const testOwner = "owner"
	20	+
	21	+	// ownerRedirect sends /<owner>/<rest> to /<rest> whenever no repository named
	22	+	// like the owner is visible, and only for a clean repository segment.
	23	+	func TestOwnerRedirect(t *testing.T) {
	24	+	t.Parallel()
	25	+	none := reposServing(t)
	26	+	tests := []struct {
	27	+	name string
	28	+	method string
	29	+	target string
	30	+	repos func(t testing.T) mocks.RepoService
	31	+	want string // "" : not redirected
	32	+	}{
	33	+	{"a repository page", http.MethodGet, "/owner/demo", nil, "/demo"},
	34	+	{"a repository page with a slash", http.MethodGet, "/owner/demo/", nil, "/demo/"},
	35	+	{"a sub-route", http.MethodGet, "/owner/demo/commit/abc123", nil, "/demo/commit/abc123"},
	36	+	{"head", http.MethodHead, "/owner/demo", nil, "/demo"},
	37	+	{"post is not redirected", http.MethodPost, "/owner/demo", nil, ""},
	38	+	{"query is kept", http.MethodGet, "/owner/demo/commits/main?a=1&b=x%20y", nil, "/demo/commits/main?a=1&b=x%20y"},
	39	+	{"a repository named like a route", http.MethodGet, "/owner/actions", nil, "/actions"},
	40	+	{"a repository named like a route, with a path", http.MethodGet, "/owner/actions/actions/runs/1", nil, "/actions/actions/runs/1"},
	41	+	{"git smart HTTP", http.MethodGet, "/owner/repo.git/info/refs?service=git-upload-pack", nil, "/repo.git/info/refs?service=git-upload-pack"},
	42	+	{"the owner alone", http.MethodGet, "/owner", nil, ""},
	43	+	{"the owner with a slash", http.MethodGet, "/owner/", nil, ""},
	44	+	{"another first segment", http.MethodGet, "/other/demo", nil, ""},
	45	+	{"a prefix of the owner", http.MethodGet, "/owners/demo", nil, ""},
	46	+	{"a double slash host", http.MethodGet, "//evil.com", nil, ""},
	47	+	{"a double slash host after the owner", http.MethodGet, "/owner//evil.com", nil, ""},
	48	+	{"an encoded slash", http.MethodGet, "/owner/%2Fevil.com", nil, ""},
	49	+	{"an encoded slash after the repository", http.MethodGet, "/owner/demo%2F/x", nil, ""},
	50	+	{"an encoded backslash", http.MethodGet, "/owner/%5Cevil.com", nil, ""},
	51	+	{"a backslash", http.MethodGet, `/owner/\evil.com`, nil, ""},
	52	+	{"dot dot", http.MethodGet, "/owner/..", nil, ""},
	53	+	{"dot dot after the repository", http.MethodGet, "/owner/../etc", nil, ""},
	54	+	{"encoded dot dot", http.MethodGet, "/owner/%2e%2e/etc", nil, ""},
	55	+	{"a dot", http.MethodGet, "/owner/.", nil, ""},
	56	+	{"an uppercase name", http.MethodGet, "/owner/Demo", nil, ""},
	57	+	{"the /-/ routes", http.MethodGet, "/owner/-/events", nil, ""},
	58	+	{"a bare .git", http.MethodGet, "/owner/.git/info/refs", nil, ""},
	59	+	{"a repository really named like the owner", http.MethodGet, "/owner/demo", func(t testing.T) mocks.RepoService {
	60	+	return reposServing(t, publicRepo(t, testOwner))
	61	+	}, ""},
	62	+	{"a route of the repository named like the owner", http.MethodGet, "/owner/actions", func(t testing.T) mocks.RepoService {
	63	+	return reposServing(t, publicRepo(t, testOwner), publicRepo(t, "actions"))
	64	+	}, ""},
	65	+	{"a private repository named like the owner is a missing one", http.MethodGet, "/owner/demo", func(t testing.T) mocks.RepoService {
	66	+	m := mocks.NewRepoService(t)
	67	+	m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything).Return(domain.Repo{}, domain.ErrNotFound)
	68	+	return m
	69	+	}, "/demo"},
	70	+	{"a failing lookup serves the request as it is", http.MethodGet, "/owner/demo", func(t testing.T) mocks.RepoService {
	71	+	return reposFailing(t, errors.New("store down"))
	72	+	}, ""},
	73	+	}
	74	+	for _, tt := range tests {
	75	+	t.Run(tt.name, func(t *testing.T) {
	76	+	t.Parallel()
	77	+	repos := none
	78	+	if tt.repos != nil {
	79	+	repos = tt.repos(t)
	80	+	}
	81	+	srv := newTestServer(t, Config{Repos: repos, Browse: mocks.NewBrowseService(t), Owner: testOwner})
	82	+	req := httptest.NewRequest(tt.method, tt.target, nil)
	83	+	to, ok := srv.ownerRedirect(req)
	84	+	assert.Equal(t, tt.want != "", ok)
	85	+	assert.Equal(t, tt.want, to)
	86	+	if !ok {
	87	+	return
	88	+	}
	89	+	// Whatever the target is, it stays on this origin.
	90	+	require.True(t, to[0] == '/' && (len(to) == 1 \|\| (to[1] != '/' && to[1] != '\\')), "target %q", to)
	91	+	})
	92	+	}
	93	+	}
	94	+
	95	+	// Through ServeHTTP the redirect is a 308 with the Location, and the paths
	96	+	// that are not redirected reach the router.
	97	+	func TestOwnerRedirectServed(t *testing.T) {
	98	+	t.Parallel()
	99	+	srv := newTestServer(t, Config{Repos: reposServing(t), Browse: mocks.NewBrowseService(t), Owner: testOwner})
	100	+	rec := httptest.NewRecorder()
	101	+	srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/owner/actions?x=1", nil))
	102	+	assert.Equal(t, http.StatusPermanentRedirect, rec.Code)
	103	+	assert.Equal(t, "/actions?x=1", rec.Header().Get("Location"))
	104	+
	105	+	rec = httptest.NewRecorder()
	106	+	srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "//evil.com", nil))
	107	+	// The mux cleans the path itself and answers with a local redirect: the
	108	+	// owner redirect adds none, and nothing leads off this origin.
	109	+	assert.NotEqual(t, http.StatusPermanentRedirect, rec.Code)
	110	+	assert.Equal(t, "/evil.com", rec.Header().Get("Location"))
	111	+	}
	112	+
	113	+	// Without an owner there is no redirect.
	114	+	func TestOwnerRedirectOff(t *testing.T) {
	115	+	t.Parallel()
	116	+	srv := newTestServer(t, Config{Repos: reposServing(t), Browse: mocks.NewBrowseService(t)})
	117	+	_, ok := srv.ownerRedirect(httptest.NewRequest(http.MethodGet, "/owner/demo", nil))
	118	+	assert.False(t, ok)
	119	+	}

M internal/adapters/web/web.go +38 -7


@@ -66,8 +66,9 @@ type Config struct {
66	66		// DefaultHeartbeat when zero.
67	67		Heartbeat time.Duration
68	68		// Owner is the owner login the GitHub-shaped URLs of CI show
69		-	// (<server>/<owner>/<repo>/...). A GET of such a URL that is not a page
70		-	// by itself is redirected to /<repo>/.... Empty: no redirect.
	69	+	// (<server>/<owner>/<repo>/...). A GET or HEAD of /<owner>/<rest> is
	70	+	// redirected to /<rest> unless a repository named like the owner is
	71	+	// visible to the caller. Empty: no redirect.
71	72		Owner string
72	73		// PublicURL is the base URL of the instance, without a trailing slash:
73	74		// the clone URLs are built on it, and its host is the instance name.

@@ -213,9 +214,18 @@ const catchAll = "GET /{repo}/{rest...}"
213	214
214	215		// ownerRedirect maps /<owner>/<repo>[.git][/rest] to /<repo>[.git][/rest]: the
215	216		// URLs that the github context of CI shows (html_url, commit and compare
216		-	// URLs, clone_url) have the owner of the instance in front, like GitHub's. A
217		-	// path is only redirected when it would be a 404 otherwise, so that a
218		-	// repository named like the owner keeps its own pages.
	217	+	// URLs, clone_url) have the owner of the instance in front, like GitHub's.
	218	+	//
	219	+	// The rule is by repository, not by route: when no repository named like the
	220	+	// owner is visible to the caller, every GET or HEAD of /<owner>/<rest> is
	221	+	// redirected, so /<owner>/actions reaches the repository named "actions". A
	222	+	// repository named like the owner keeps its own pages. A private one is seen
	223	+	// as missing by an anonymous caller (the use case answers ErrNotFound), so the
	224	+	// redirect is the same as for a name nobody has and tells nothing.
	225	+	//
	226	+	// The target is built from a segment that parses as a repository name, which
	227	+	// has no slash, backslash, percent sign or dot-dot: it can only be a path on
	228	+	// this origin, never a "//host" one.
219	229		func (s Server) ownerRedirect(r http.Request) (string, bool) {
220	230		if s.cfg.Owner == "" \|\| (r.Method != http.MethodGet && r.Method != http.MethodHead) {
221	231		return "", false

@@ -225,10 +235,10 @@ func (s Server) ownerRedirect(r http.Request) (string, bool) {
225	235		return "", false
226	236		}
227	237		repo, _, _ := strings.Cut(rest, "/")
228		-	if _, err := domain.ParseRepoName(strings.TrimSuffix(repo, ".git")); err != nil {
	238	+	if _, err := domain.ParseRepoPath(repo); err != nil {
229	239		return "", false
230	240		}
231		-	if _, pattern := s.mux.Handler(r); pattern != catchAll {
	241	+	if s.ownerNamesARepo(r) {
232	242		return "", false
233	243		}
234	244		to := "/" + rest

@@ -238,6 +248,27 @@ func (s Server) ownerRedirect(r http.Request) (string, bool) {
238	248		return to, true
239	249		}
240	250
	251	+	// ownerNamesARepo tells whether the caller can see a repository named like
	252	+	// the owner. A lookup that fails otherwise counts as yes: the request is then
	253	+	// served as it was before, without a redirect.
	254	+	func (s Server) ownerNamesARepo(r http.Request) bool {
	255	+	name, err := domain.ParseRepoName(s.cfg.Owner)
	256	+	if err != nil {
	257	+	return false
	258	+	}
	259	+	ctx := r.Context()
	260	+	_, err = s.cfg.Repos.Get(ctx, principal(ctx), name)
	261	+	switch {
	262	+	case err == nil:
	263	+	return true
	264	+	case errors.Is(err, domain.ErrNotFound):
	265	+	return false
	266	+	}
	267	+	log := zerowrap.FromCtx(ctx)
	268	+	log.Warn().Err(err).Msg("owner redirect: the lookup of the repository named like the owner failed")
	269	+	return true
	270	+	}
	271	+
241	272		// handler builds a page. It returns the component and its status, or an
242	273		// error, which becomes the 404 or 500 page.
243	274		type handler func(ctx context.Context, r *http.Request) (templ.Component, int, error)

 	// DefaultHeartbeat when zero.
 	Heartbeat time.Duration
 	// Owner is the owner login the GitHub-shaped URLs of CI show
-	// (<server>/<owner>/<repo>/...). A GET of such a URL that is not a page
+	// (<server>/<owner>/<repo>/...). A GET or HEAD of /<owner>/<rest> is
-	// by itself is redirected to /<repo>/.... Empty: no redirect.
+	// redirected to /<rest> unless a repository named like the owner is
+	// visible to the caller. Empty: no redirect.
 	Owner string
 	// PublicURL is the base URL of the instance, without a trailing slash:
 	// the clone URLs are built on it, and its host is the instance name.
 // ownerRedirect maps /<owner>/<repo>[.git][/rest] to /<repo>[.git][/rest]: the
 // URLs that the github context of CI shows (html_url, commit and compare
-// URLs, clone_url) have the owner of the instance in front, like GitHub's. A
+// URLs, clone_url) have the owner of the instance in front, like GitHub's.
-// path is only redirected when it would be a 404 otherwise, so that a
+//
-// repository named like the owner keeps its own pages.
+// The rule is by repository, not by route: when no repository named like the
+// owner is visible to the caller, every GET or HEAD of /<owner>/<rest> is
+// redirected, so /<owner>/actions reaches the repository named "actions". A
+// repository named like the owner keeps its own pages. A private one is seen
+// as missing by an anonymous caller (the use case answers ErrNotFound), so the
+// redirect is the same as for a name nobody has and tells nothing.
+//
+// The target is built from a segment that parses as a repository name, which
+// has no slash, backslash, percent sign or dot-dot: it can only be a path on
+// this origin, never a "//host" one.
 func (s *Server) ownerRedirect(r *http.Request) (string, bool) {
 	if s.cfg.Owner == "" || (r.Method != http.MethodGet && r.Method != http.MethodHead) {
 		return "", false
 		return "", false
 	}
 	repo, _, _ := strings.Cut(rest, "/")
-	if _, err := domain.ParseRepoName(strings.TrimSuffix(repo, ".git")); err != nil {
+	if _, err := domain.ParseRepoPath(repo); err != nil {
 		return "", false
 	}
-	if _, pattern := s.mux.Handler(r); pattern != catchAll {
+	if s.ownerNamesARepo(r) {
 		return "", false
 	}
 	to := "/" + rest
 	return to, true
 }
+// ownerNamesARepo tells whether the caller can see a repository named like
+// the owner. A lookup that fails otherwise counts as yes: the request is then
+// served as it was before, without a redirect.
+func (s *Server) ownerNamesARepo(r *http.Request) bool {
+	name, err := domain.ParseRepoName(s.cfg.Owner)
+	if err != nil {
+		return false
+	}
+	ctx := r.Context()
+	_, err = s.cfg.Repos.Get(ctx, principal(ctx), name)
+	switch {
+	case err == nil:
+		return true
+	case errors.Is(err, domain.ErrNotFound):
+		return false
+	}
+	log := zerowrap.FromCtx(ctx)
+	log.Warn().Err(err).Msg("owner redirect: the lookup of the repository named like the owner failed")
+	return true
+}
 // handler builds a page. It returns the component and its status, or an
 // error, which becomes the 404 or 500 page.
 type handler func(ctx context.Context, r *http.Request) (templ.Component, int, error)