3922170 fix(web): exempt only exact login paths from the session gate · ermite

M cmd/ermite/web_session_test.go +13 -2


@@ -171,9 +171,15 @@ func TestWebLoginEndToEnd(t *testing.T) {
171	171		assert.Equal(t, "no-store", g.hdr.Get("Cache-Control"))
172	172		assert.Contains(t, g.body, "login link not valid")
173	173		r := b.do("POST", p, map[string]string{"Origin": publicOrigin}, "")
174		-	assert.Equal(t, 404, r.status, p)
175		-	assert.Equal(t, g.body, r.body, "no detail tells why")
176	174		assert.Nil(t, r.cookie(sessionCookie))
	175	+	if validNonceShape(p) {
	176	+	assert.Equal(t, 404, r.status, p)
	177	+	assert.Equal(t, g.body, r.body, "no detail tells why")
	178	+	} else {
	179	+	// Not even the shape of a link: the gate refuses it, as it does
	180	+	// any POST without a session.
	181	+	assert.Equal(t, 403, r.status, p)
	182	+	}
177	183		}
178	184
179	185		// Signed in: private repositories show on every page, headers say private.

@@ -271,6 +277,11 @@ func TestWebLoginEndToEnd(t *testing.T) {
271	277		assert.Equal(t, 404, b.get("/secret", as(token)).status)
272	278		}
273	279
	280	+	// validNonceShape tells whether a login path ends in 43 characters of base64url.
	281	+	func validNonceShape(p string) bool {
	282	+	return len(strings.TrimPrefix(p, "/-/login/")) == 43
	283	+	}
	284	+
274	285		// cookieInterceptor makes a Connect client send the session cookie, as a
275	286		// browser-made call would, to show the API pays no attention to it.
276	287		func cookieInterceptor(token string) connect.Interceptor {

 		assert.Equal(t, "no-store", g.hdr.Get("Cache-Control"))
 		assert.Contains(t, g.body, "login link not valid")
 		r := b.do("POST", p, map[string]string{"Origin": publicOrigin}, "")
-		assert.Equal(t, 404, r.status, p)
-		assert.Equal(t, g.body, r.body, "no detail tells why")
 		assert.Nil(t, r.cookie(sessionCookie))
+		if validNonceShape(p) {
+			assert.Equal(t, 404, r.status, p)
+			assert.Equal(t, g.body, r.body, "no detail tells why")
+		} else {
+			// Not even the shape of a link: the gate refuses it, as it does
+			// any POST without a session.
+			assert.Equal(t, 403, r.status, p)
+		}
 	}
 	// Signed in: private repositories show on every page, headers say private.
 	assert.Equal(t, 404, b.get("/secret", as(token)).status)
 }
+// validNonceShape tells whether a login path ends in 43 characters of base64url.
+func validNonceShape(p string) bool {
+	return len(strings.TrimPrefix(p, "/-/login/")) == 43
+}
 // cookieInterceptor makes a Connect client send the session cookie, as a
 // browser-made call would, to show the API pays no attention to it.
 func cookieInterceptor(token string) connect.Interceptor {

M internal/adapters/httpserver/middleware.go +5 -2


@@ -4,6 +4,7 @@ package httpserver
4	4
5	5		import (
6	6		"net/http"
	7	+	"path"
7	8		"strings"
8	9		"time"
9	10

@@ -65,9 +66,11 @@ func (s *statusRecorder) Unwrap() http.ResponseWriter { return s.ResponseWriter
65	66		// the path is a secret.
66	67		const loginPathPrefix = "/-/login/"
67	68
68		-	// loggedPath is the path of a request as the log may show it.
	69	+	// loggedPath is the path of a request as the log may show it. The check is
	70	+	// wide on purpose: a path that only reaches the link after cleaning
	71	+	// ("/x/../-/login/…", "//-/login/…") or holds the prefix further in is cut too.
69	72		func loggedPath(p string) string {
70		-	if strings.HasPrefix(p, loginPathPrefix) {
	73	+	if strings.HasPrefix(path.Clean(p), loginPathPrefix) \|\| strings.Contains(p, loginPathPrefix) {
71	74		return loginPathPrefix + "…"
72	75		}
73	76		return p

 import (
 	"net/http"
+	"path"
 	"strings"
 	"time"
 // the path is a secret.
 const loginPathPrefix = "/-/login/"
-// loggedPath is the path of a request as the log may show it.
+// loggedPath is the path of a request as the log may show it. The check is
+// wide on purpose: a path that only reaches the link after cleaning
+// ("/x/../-/login/…", "//-/login/…") or holds the prefix further in is cut too.
 func loggedPath(p string) string {
-	if strings.HasPrefix(p, loginPathPrefix) {
+	if strings.HasPrefix(path.Clean(p), loginPathPrefix) || strings.Contains(p, loginPathPrefix) {
 		return loginPathPrefix + "…"
 	}
 	return p

M internal/adapters/httpserver/server_test.go +13 -0


@@ -141,6 +141,19 @@ func TestRequestLog(t *testing.T) {
141	141		assert.Empty(t, quiet.String())
142	142		}
143	143
	144	+	// The secret of a login link never reaches the request log, whatever shape
	145	+	// the path takes.
	146	+	func TestRequestLogCutsLoginLinks(t *testing.T) {
	147	+	var buf syncBuffer
	148	+	ctx := zerowrap.WithCtx(context.Background(), zerowrap.New(zerowrap.Config{Level: "debug", Format: "json", Output: &buf}))
	149	+	h := httpserver.New(httpserver.Config{}).Handler()
	150	+	for _, target := range []string{"/-/login/sEcReT", "//-/login/sEcReT", "/x/../-/login/sEcReT", "/-/login/sEcReT/more", "/-/./login/sEcReT"} {
	151	+	h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(ctx, "POST", target, nil))
	152	+	}
	153	+	assert.NotContains(t, buf.String(), "sEcReT")
	154	+	assert.Contains(t, buf.String(), "/-/login/…")
	155	+	}
	156	+
144	157		func TestDirWritable(t *testing.T) {
145	158		dir := t.TempDir()
146	159		check := httpserver.DirWritable(dir)


@@ -141,6 +141,19 @@ func TestRequestLog(t *testing.T) {
141	assert.Empty(t, quiet.String())	141		assert.Empty(t, quiet.String())
142	}	142		}
143		143
		144	+	// The secret of a login link never reaches the request log, whatever shape
		145	+	// the path takes.
		146	+	func TestRequestLogCutsLoginLinks(t *testing.T) {
		147	+	var buf syncBuffer
		148	+	ctx := zerowrap.WithCtx(context.Background(), zerowrap.New(zerowrap.Config{Level: "debug", Format: "json", Output: &buf}))
		149	+	h := httpserver.New(httpserver.Config{}).Handler()
		150	+	for _, target := range []string{"/-/login/sEcReT", "//-/login/sEcReT", "/x/../-/login/sEcReT", "/-/login/sEcReT/more", "/-/./login/sEcReT"} {
		151	+	h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(ctx, "POST", target, nil))
		152	+	}
		153	+	assert.NotContains(t, buf.String(), "sEcReT")
		154	+	assert.Contains(t, buf.String(), "/-/login/…")
		155	+	}
		156	+
144	func TestDirWritable(t *testing.T) {	157		func TestDirWritable(t *testing.T) {
145	dir := t.TempDir()	158		dir := t.TempDir()
146	check := httpserver.DirWritable(dir)	159		check := httpserver.DirWritable(dir)

M internal/adapters/web/session.go +17 -4


@@ -6,6 +6,8 @@ import (
6	6		"context"
7	7		"errors"
8	8		"net/http"
	9	+	"net/url"
	10	+	"path"
9	11		"strconv"
10	12		"strings"
11	13		"time"

@@ -88,9 +90,11 @@ func cacheHeaders(ctx context.Context, h http.Header, public, private string) {
88	90		}
89	91
90	92		// logPath is the path of a request as the log may show it: a login link is
91		-	// a secret.
	93	+	// a secret. The decoded path is given, so the check is wide on purpose: a
	94	+	// path that only reaches the link after cleaning ("/x/../-/login/…",
	95	+	// "//-/login/…") or that holds the prefix further in is cut too.
92	96		func logPath(p string) string {
93		-	if strings.HasPrefix(p, loginPrefix) {
	97	+	if strings.HasPrefix(path.Clean(p), loginPrefix) \|\| strings.Contains(p, loginPrefix) {
94	98		return loginPrefix + "…"
95	99		}
96	100		return p

@@ -100,7 +104,7 @@ func logPath(p string) string {
100	104		// makes the request the owner's. A cookie that is not valid is cleared. A
101	105		// request without a cookie is untouched.
102	106		func (s Server) withSession(w http.ResponseWriter, r http.Request) *http.Request {
103		-	if s.cfg.Sessions == nil \|\| strings.HasPrefix(r.URL.Path, StaticPrefix) {
	107	+	if s.cfg.Sessions == nil \|\| strings.HasPrefix(r.URL.EscapedPath(), StaticPrefix) {
104	108		return r
105	109		}
106	110		c, err := r.Cookie(CookieName)

@@ -160,13 +164,22 @@ func (s Server) gate(w http.ResponseWriter, r http.Request) bool {
160	164		denyForbidden(w)
161	165		return false
162	166		}
163		-	if !SignedIn(r.Context()) && !strings.HasPrefix(r.URL.Path, loginPrefix) {
	167	+	if !SignedIn(r.Context()) && (r.Method != http.MethodPost \|\| !isLoginPath(r.URL)) {
164	168		denyForbidden(w)
165	169		return false
166	170		}
167	171		return true
168	172		}
169	173
	174	+	// isLoginPath tells whether u is exactly a login link: loginPrefix, a nonce of
	175	+	// the right shape and nothing after it. It looks at the escaped path, which is
	176	+	// what the router matches: the decoded path would let "/-%2Flogin%2Fx/..." pass
	177	+	// for a link while the router sends it to another route.
	178	+	func isLoginPath(u *url.URL) bool {
	179	+	nonce, ok := strings.CutPrefix(u.EscapedPath(), loginPrefix)
	180	+	return ok && validNonce(nonce)
	181	+	}
	182	+
170	183		func denyForbidden(w http.ResponseWriter) {
171	184		w.Header().Set("Cache-Control", "no-store")
172	185		http.Error(w, "forbidden", http.StatusForbidden)

 	"context"
 	"errors"
 	"net/http"
+	"net/url"
+	"path"
 	"strconv"
 	"strings"
 	"time"
 }
 // logPath is the path of a request as the log may show it: a login link is
-// a secret.
+// a secret. The decoded path is given, so the check is wide on purpose: a
+// path that only reaches the link after cleaning ("/x/../-/login/…",
+// "//-/login/…") or that holds the prefix further in is cut too.
 func logPath(p string) string {
-	if strings.HasPrefix(p, loginPrefix) {
+	if strings.HasPrefix(path.Clean(p), loginPrefix) || strings.Contains(p, loginPrefix) {
 		return loginPrefix + "…"
 	}
 	return p
 // makes the request the owner's. A cookie that is not valid is cleared. A
 // request without a cookie is untouched.
 func (s *Server) withSession(w http.ResponseWriter, r *http.Request) *http.Request {
-	if s.cfg.Sessions == nil || strings.HasPrefix(r.URL.Path, StaticPrefix) {
+	if s.cfg.Sessions == nil || strings.HasPrefix(r.URL.EscapedPath(), StaticPrefix) {
 		return r
 	}
 	c, err := r.Cookie(CookieName)
 		denyForbidden(w)
 		return false
 	}
-	if !SignedIn(r.Context()) && !strings.HasPrefix(r.URL.Path, loginPrefix) {
+	if !SignedIn(r.Context()) && (r.Method != http.MethodPost || !isLoginPath(r.URL)) {
 		denyForbidden(w)
 		return false
 	}
 	return true
 }
+// isLoginPath tells whether u is exactly a login link: loginPrefix, a nonce of
+// the right shape and nothing after it. It looks at the escaped path, which is
+// what the router matches: the decoded path would let "/-%2Flogin%2Fx/..." pass
+// for a link while the router sends it to another route.
+func isLoginPath(u *url.URL) bool {
+	nonce, ok := strings.CutPrefix(u.EscapedPath(), loginPrefix)
+	return ok && validNonce(nonce)
+}
 func denyForbidden(w http.ResponseWriter) {
 	w.Header().Set("Cache-Control", "no-store")
 	http.Error(w, "forbidden", http.StatusForbidden)

M internal/adapters/web/session_test.go +51 -2


@@ -115,8 +115,54 @@ func TestGate(t *testing.T) {
115	115		assert.Equal(t, 303, do(link, "https://forge.example", ""), "login needs the link, not a session")
116	116		assert.Equal(t, 404, do("/-/login/"+strings.Repeat("b", 43), "https://forge.example", ""))
117	117		assert.Equal(t, 1, lim.fails, "one failed login counted")
118		-	assert.Equal(t, 404, do("/-/login/x", "https://forge.example", ""))
119		-	assert.Equal(t, 2, lim.fails, "a malformed link counts too")
	118	+	assert.Equal(t, 403, do("/-/login/x", "https://forge.example", ""), "a malformed link is not a login")
	119	+	}
	120	+
	121	+	// GO-001: the exemption of the login POST is decided on the escaped path, the
	122	+	// one the router matches, and on exactly loginPrefix + nonce. An encoded slash
	123	+	// must not turn another route into a "login".
	124	+	func TestGateLoginExemptionIsExact(t *testing.T) {
	125	+	sess := &fakeSessions{valid: "tok"}
	126	+	lim := &countingLimiter{}
	127	+	srv, err := New(Config{
	128	+	Repos: failing{err: errors.New("x")}, Git: ermiteconnect.NewGitServiceClient(http.DefaultClient, "http://unused.invalid"),
	129	+	MergeRequests: ermiteconnect.NewMergeRequestServiceClient(http.DefaultClient, "http://unused.invalid"),
	130	+	PublicURL: "https://forge.example", Sessions: sess, Limiter: lim,
	131	+	})
	132	+	require.NoError(t, err)
	133	+	post := func(target, cookie string) int {
	134	+	req := httptest.NewRequest(http.MethodPost, target, nil)
	135	+	req.Header.Set("Origin", "https://forge.example")
	136	+	if cookie != "" {
	137	+	req.AddCookie(&http.Cookie{Name: CookieName, Value: cookie})
	138	+	}
	139	+	rec := httptest.NewRecorder()
	140	+	srv.ServeHTTP(rec, req)
	141	+	return rec.Code
	142	+	}
	143	+	nonce := strings.Repeat("a", 43)
	144	+	for _, target := range []string{
	145	+	"/-%2Flogin%2Fx/merge-requests/1/close",
	146	+	"/-%2flogin%2fx/merge-requests/1/close",
	147	+	"/-%2Flogin%2F" + nonce + "/merge-requests/1/close",
	148	+	"/-/login%2F" + nonce,
	149	+	"/-/login/" + nonce + "/merge-requests/1/close",
	150	+	"/-/login/" + nonce + "/",
	151	+	"/-/login/" + nonce + "%2Fx",
	152	+	"/-/login/" + nonce + "%2F..%2Fx",
	153	+	"/-/login/%2e%2e/x/merge-requests/1/close",
	154	+	"/-/login/x/../../x/merge-requests/1/close",
	155	+	"/-/login/",
	156	+	"/-/login/%61" + nonce[1:],
	157	+	"/-/login//" + nonce,
	158	+	"/%2D/login/" + nonce,
	159	+	} {
	160	+	assert.Equal(t, http.StatusForbidden, post(target, ""), target)
	161	+	}
	162	+	assert.Empty(t, sess.used, "no link was used")
	163	+
	164	+	assert.Equal(t, http.StatusSeeOther, post("/-/login/"+nonce, ""), "a valid link still logs in")
	165	+	assert.Equal(t, []string{nonce}, sess.used)
120	166		}
121	167
122	168		// A session check that cannot be made fails closed (anonymous) and keeps the

@@ -165,4 +211,7 @@ func TestNoSessionsNoRoutes(t *testing.T) {
165	211		func TestLogPathHidesLinks(t *testing.T) {
166	212		assert.Equal(t, "/-/login/…", logPath("/-/login/"+strings.Repeat("a", 43)))
167	213		assert.Equal(t, "/demo/tree", logPath("/demo/tree"))
	214	+	for _, p := range []string{"//-/login/secret", "/x/../-/login/secret", "/-/login/secret/more", "/-/./login/secret"} {
	215	+	assert.Equal(t, "/-/login/…", logPath(p), p)
	216	+	}
168	217		}


@@ -115,8 +115,54 @@ func TestGate(t *testing.T) {
115		assert.Equal(t, 303, do(link, "https://forge.example", ""), "login needs the link, not a session")	115		assert.Equal(t, 303, do(link, "https://forge.example", ""), "login needs the link, not a session")
116		assert.Equal(t, 404, do("/-/login/"+strings.Repeat("b", 43), "https://forge.example", ""))	116		assert.Equal(t, 404, do("/-/login/"+strings.Repeat("b", 43), "https://forge.example", ""))
117		assert.Equal(t, 1, lim.fails, "one failed login counted")	117		assert.Equal(t, 1, lim.fails, "one failed login counted")
118	-	assert.Equal(t, 404, do("/-/login/x", "https://forge.example", ""))	118	+	assert.Equal(t, 403, do("/-/login/x", "https://forge.example", ""), "a malformed link is not a login")
119	-	assert.Equal(t, 2, lim.fails, "a malformed link counts too")	119	+	}
			120	+
			121	+	// GO-001: the exemption of the login POST is decided on the escaped path, the
			122	+	// one the router matches, and on exactly loginPrefix + nonce. An encoded slash
			123	+	// must not turn another route into a "login".
			124	+	func TestGateLoginExemptionIsExact(t *testing.T) {
			125	+	sess := &fakeSessions{valid: "tok"}
			126	+	lim := &countingLimiter{}
			127	+	srv, err := New(Config{
			128	+	Repos: failing{err: errors.New("x")}, Git: ermiteconnect.NewGitServiceClient(http.DefaultClient, "http://unused.invalid"),
			129	+	MergeRequests: ermiteconnect.NewMergeRequestServiceClient(http.DefaultClient, "http://unused.invalid"),
			130	+	PublicURL: "https://forge.example", Sessions: sess, Limiter: lim,
			131	+	})
			132	+	require.NoError(t, err)
			133	+	post := func(target, cookie string) int {
			134	+	req := httptest.NewRequest(http.MethodPost, target, nil)
			135	+	req.Header.Set("Origin", "https://forge.example")
			136	+	if cookie != "" {
			137	+	req.AddCookie(&http.Cookie{Name: CookieName, Value: cookie})
			138	+	}
			139	+	rec := httptest.NewRecorder()
			140	+	srv.ServeHTTP(rec, req)
			141	+	return rec.Code
			142	+	}
			143	+	nonce := strings.Repeat("a", 43)
			144	+	for _, target := range []string{
			145	+	"/-%2Flogin%2Fx/merge-requests/1/close",
			146	+	"/-%2flogin%2fx/merge-requests/1/close",
			147	+	"/-%2Flogin%2F" + nonce + "/merge-requests/1/close",
			148	+	"/-/login%2F" + nonce,
			149	+	"/-/login/" + nonce + "/merge-requests/1/close",
			150	+	"/-/login/" + nonce + "/",
			151	+	"/-/login/" + nonce + "%2Fx",
			152	+	"/-/login/" + nonce + "%2F..%2Fx",
			153	+	"/-/login/%2e%2e/x/merge-requests/1/close",
			154	+	"/-/login/x/../../x/merge-requests/1/close",
			155	+	"/-/login/",
			156	+	"/-/login/%61" + nonce[1:],
			157	+	"/-/login//" + nonce,
			158	+	"/%2D/login/" + nonce,
			159	+	} {
			160	+	assert.Equal(t, http.StatusForbidden, post(target, ""), target)
			161	+	}
			162	+	assert.Empty(t, sess.used, "no link was used")
			163	+
			164	+	assert.Equal(t, http.StatusSeeOther, post("/-/login/"+nonce, ""), "a valid link still logs in")
			165	+	assert.Equal(t, []string{nonce}, sess.used)
120		}	166		}
121			167
122		// A session check that cannot be made fails closed (anonymous) and keeps the	168		// A session check that cannot be made fails closed (anonymous) and keeps the

@@ -165,4 +211,7 @@ func TestNoSessionsNoRoutes(t *testing.T) {
165		func TestLogPathHidesLinks(t *testing.T) {	211		func TestLogPathHidesLinks(t *testing.T) {
166		assert.Equal(t, "/-/login/…", logPath("/-/login/"+strings.Repeat("a", 43)))	212		assert.Equal(t, "/-/login/…", logPath("/-/login/"+strings.Repeat("a", 43)))
167		assert.Equal(t, "/demo/tree", logPath("/demo/tree"))	213		assert.Equal(t, "/demo/tree", logPath("/demo/tree"))
			214	+	for _, p := range []string{"//-/login/secret", "/x/../-/login/secret", "/-/login/secret/more", "/-/./login/secret"} {
			215	+	assert.Equal(t, "/-/login/…", logPath(p), p)
			216	+	}
168		}	217		}