refactor(auth): one net/url pass for returnUrl safety + canonicalisation

Follow-up to the Copilot review. The previous fix had two helpers parsing
the same URL twice:

  - safeRelativePath: url.Parse(s), reject if scheme/host/user set
  - stripFragment: strings.Cut(s, "#"), throw away the tail

stripFragment was the embarrassing one — net/url already exposes
url.URL.Fragment = "" + url.URL.String(), no string fiddling required.
Collapse both helpers into a single sanitizeRelativePath(s) (string, bool)
that returns the canonical fragment-stripped form alongside the safety
verdict. One Parse, one set of checks, one rebuild.

Test suite merges the two matrices into a single table — including
fragment-stripping cases that the previous split missed (e.g. "/?x=1#frag"
must come back as "/?x=1", not "" + stray strings.Cut output).

Author: AYDEV-FR

internal/auth/oidc.go

@@ -33,15 +33,28 @@ type stateBlob struct {
 	Expiry       int64 // unix seconds
 }
 
-// safeRelativePath enforces "/path[?...][#...]" — no scheme, no host, no
-// protocol-relative "//host/..." trick. `url.Parse` does the heavy lifting,
-// we just inspect the result.
-func safeRelativePath(s string) bool {
+// sanitizeRelativePath does two related jobs in one net/url pass:
+//
+//   - validates that s is a safe relative URL — no scheme, no host, no
+//     userinfo, no protocol-relative ("//host/...") or backslash trick
+//   - returns the canonical form with any user-supplied #fragment dropped,
+//     because the SPA's consumeHashToken() expects the final redirect's
+//     hash to be exclusively "#token=..." (a leftover "/foo#section" would
+//     produce "/foo#section#token=..." which it can't parse)
+//
+// Both concerns are pure net/url plumbing — no string fiddling beyond the
+// "//"/"/\\" prefix sniff that url.Parse can't catch (it parses them as
+// scheme-less but still cross-origin).
+func sanitizeRelativePath(s string) (string, bool) {
 	if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") {
-		return false
+		return "", false
 	}
 	u, err := url.Parse(s)
-	return err == nil && u.Scheme == "" && u.Host == "" && u.User == nil
+	if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil {
+		return "", false
+	}
+	u.Fragment = ""
+	return u.String(), true
 }
 
 // OIDCHandler runs the OAuth2 / OIDC Authorization Code flow with the
@@ -203,10 +216,11 @@ func (h *OIDCHandler) consumeState(state string) (*stateBlob, bool) {
 // returnURL is hardened against the protocol-relative open redirect
 // (`//evil.com/x` would otherwise sail past a naive HasPrefix("/") check).
 func (h *OIDCHandler) Login(c *fiber.Ctx) error {
-	returnURL := c.Query("returnUrl", "/")
-	if !safeRelativePath(returnURL) {
-		logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", returnURL)
-		returnURL = "/"
+	returnURL := "/"
+	if clean, ok := sanitizeRelativePath(c.Query("returnUrl", "/")); ok {
+		returnURL = clean
+	} else {
+		logger.Warn("OIDC login: rejected unsafe returnUrl, defaulting to /", "returnUrl", c.Query("returnUrl"))
 	}
 	nonce, err := randomToken()
 	if err != nil {
@@ -301,27 +315,17 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error {
 		logger.Warn("OIDC callback: id_token has no at_hash, skipping access-token binding check")
 	}
 
+	// Re-sanitize on the way out: the blob was signed by us, but defense in
+	// depth is cheap and this is the last chance to drop any fragment that
+	// would otherwise produce a "/foo#frag#token=..." URL the SPA can't read.
 	returnURL := "/"
-	if safeRelativePath(stateData.ReturnURL) {
-		returnURL = stateData.ReturnURL
+	if clean, ok := sanitizeRelativePath(stateData.ReturnURL); ok {
+		returnURL = clean
 	}
-	// Strip any fragment the user smuggled in via returnUrl (e.g. "/foo#section"):
-	// the SPA's consumeHashToken() expects "#token=..." to be the only hash
-	// on the final URL. Without this strip we'd produce "/foo#section#token=...",
-	// which the SPA fails to parse.
-	returnURL = stripFragment(returnURL)
 	logger.Debug("OIDC callback complete", "returnUrl", returnURL, "tokenLength", len(rawIDToken))
 	return c.Redirect(fmt.Sprintf("%s#token=%s", returnURL, rawIDToken), fiber.StatusFound)
 }
 
-// stripFragment returns the input URL without its #fragment, if any. Used at
-// the callback to make sure the only hash on the final SPA redirect URL is
-// the one carrying the token.
-func stripFragment(s string) string {
-	before, _, _ := strings.Cut(s, "#")
-	return before
-}
-
 // Logout currently just bounces the browser home — the SPA clears its
 // localStorage token on this redirect. End-session at the IdP is opt-in
 // (the previous version didn't do it either; add an EndSessionEndpoint

internal/auth/oidc_test.go

@@ -2,54 +2,39 @@ package auth
 
 import "testing"
 
-func TestStripFragment(t *testing.T) {
+// TestSanitizeRelativePath pins both behaviours in one go: the relative-URL
+// safety check (open-redirect surface) and the fragment-stripping canonical
+// form the SPA's consumeHashToken() relies on. Both come out of one
+// net/url.Parse pass — no string fiddling.
+func TestSanitizeRelativePath(t *testing.T) {
 	for _, tc := range []struct {
-		in, want string
+		in     string
+		want   string
+		wantOK bool
 	}{
-		{"/foo", "/foo"},
-		{"/foo?x=1", "/foo?x=1"},
-		{"/foo#frag", "/foo"},
-		{"/foo?x=1#frag", "/foo?x=1"},
-		{"#frag", ""},
-		{"", ""},
-	} {
-		if got := stripFragment(tc.in); got != tc.want {
-			t.Errorf("stripFragment(%q) = %q, want %q", tc.in, got, tc.want)
-		}
-	}
-}
+		// happy paths — exact passthrough or fragment dropped
+		{"/", "/", true},
+		{"/foo", "/foo", true},
+		{"/foo?x=1", "/foo?x=1", true},
+		{"/foo?x=1#frag", "/foo?x=1", true}, // fragment stripped
+		{"/foo#frag", "/foo", true},
+		{"/#frag", "/", true},
 
-// TestSafeRelativePath covers the open-redirect surface a CTF participant is
-// likely to probe through ?returnUrl=. Anything that isn't a plain
-// "/path[?…][#…]" must be rejected before it reaches c.Redirect, where the
-// browser would otherwise treat protocol-relative or backslash-prefixed
-// inputs as cross-origin.
-func TestSafeRelativePath(t *testing.T) {
-	cases := []struct {
-		in   string
-		want bool
-	}{
-		// happy paths
-		{"/", true},
-		{"/foo", true},
-		{"/foo/bar?x=1#frag", true},
-
-		// classic open-redirect tricks — all must be rejected
-		{"//evil.com/x", false},       // protocol-relative URL
-		{"/\\evil.com/x", false},      // backslash-prefixed (some browsers)
-		{"http://evil.com/x", false},  // absolute URL
-		{"https://evil.com/x", false}, // absolute URL https
-		{"javascript:alert(1)", false},
-		{"data:text/html,x", false},
-		{"//user@evil.com/x", false}, // userinfo trick
-		{"", false},                  // empty
-		{"foo/bar", false},           // no leading slash
-		{" /foo", false},             // leading whitespace (Go parser doesn't strip it; we'd happily redirect)
-	}
-	for _, tc := range cases {
-		got := safeRelativePath(tc.in)
-		if got != tc.want {
-			t.Errorf("safeRelativePath(%q) = %v, want %v", tc.in, got, tc.want)
+		// classic open-redirect tricks — all rejected
+		{"//evil.com/x", "", false},       // protocol-relative URL
+		{"/\\evil.com/x", "", false},      // backslash-prefixed (some browsers)
+		{"http://evil.com/x", "", false},  // absolute URL
+		{"https://evil.com/x", "", false}, // absolute URL https
+		{"javascript:alert(1)", "", false},
+		{"data:text/html,x", "", false},
+		{"//user@evil.com/x", "", false}, // userinfo trick
+		{"", "", false},                  // empty
+		{"foo/bar", "", false},           // no leading slash
+		{" /foo", "", false},             // leading whitespace
+	} {
+		got, ok := sanitizeRelativePath(tc.in)
+		if ok != tc.wantOK || got != tc.want {
+			t.Errorf("sanitizeRelativePath(%q) = (%q, %v), want (%q, %v)", tc.in, got, ok, tc.want, tc.wantOK)
 		}
 	}
 }