Skip to content

Commit 84af5fb

Browse files
committed
fix(auth): close encoded-backslash open-redirect + tidy callback findings
Address the latest Copilot review on PR #39: - sanitizeRelativePath: reject a backslash in the decoded path. A percent-encoded backslash ("/%5cevil.com") survived the literal "/\\" prefix check, and user agents that normalize "\" to "/" would read the decoded "//evil.com" as a protocol-relative redirect. - Callback: return 400 (not 502) on nonce mismatch — a client/session issue or attack, not an upstream failure, mirroring the state path. - Fix the package comment: the flow now carries four flow cookies (state/verifier/nonce/returnUrl), not three. - Pin the bypass with percent-encoded backslash test cases. Claude-Session: https://claude.ai/code/session_01M4Dd5oASBGHwr6ts6wTnYn
1 parent ebbd794 commit 84af5fb

2 files changed

Lines changed: 14 additions & 2 deletions

File tree

internal/auth/oidc.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// the canonical Go OIDC pattern used by Kubernetes, Argo CD and the official
66
// coreos/go-oidc README example: go-oidc for discovery + ID token
77
// verification, golang.org/x/oauth2 for the Authorization Code + PKCE flow,
8-
// and three short-lived HttpOnly cookies to carry state/verifier/returnUrl
8+
// and four short-lived HttpOnly cookies to carry state/verifier/nonce/returnUrl
99
// across the browser bounce. No framework, no signed-cookie key management,
1010
// nothing dploy-specific beyond the optional split-horizon issuer support
1111
// and the SPA's "#token=..." hand-off.
@@ -162,6 +162,13 @@ func sanitizeRelativePath(s string) (string, bool) {
162162
if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil {
163163
return "", false
164164
}
165+
// A percent-encoded backslash (e.g. "/%5cevil.com") survives the literal
166+
// "/\\" prefix check above but decodes into u.Path; user agents that
167+
// normalize "\" to "/" would then read "//evil.com" as protocol-relative.
168+
// Reject any backslash in the decoded path to close that bypass.
169+
if strings.Contains(u.Path, "\\") {
170+
return "", false
171+
}
165172
u.Fragment = ""
166173
return u.String(), true
167174
}
@@ -293,7 +300,9 @@ func (h *OIDCHandler) Callback(c *fiber.Ctx) error {
293300
// token carrying the attacker's nonce, not ours. Constant-time compare
294301
// because the nonce stays a one-shot secret until the cookie clears.
295302
if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(nonce)) != 1 {
296-
return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "nonce mismatch"})
303+
// Client-side/session issue or an attack, not an upstream failure —
304+
// mirror the state-mismatch path with a 400 rather than a 502.
305+
return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "nonce mismatch"})
297306
}
298307

299308
// Re-sanitize the returnUrl from the cookie as defense-in-depth, even

internal/auth/oidc_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ func TestSanitizeRelativePath(t *testing.T) {
2323
// classic open-redirect tricks — all rejected
2424
{"//evil.com/x", "", false}, // protocol-relative URL
2525
{"/\\evil.com/x", "", false}, // backslash-prefixed (some browsers)
26+
{"/%5cevil.com/x", "", false}, // percent-encoded backslash
27+
{"/%5Cevil.com/x", "", false}, // percent-encoded backslash (upper)
28+
{"/foo/%5cbar", "", false}, // encoded backslash mid-path
2629
{"http://evil.com/x", "", false}, // absolute URL
2730
{"https://evil.com/x", "", false}, // absolute URL https
2831
{"javascript:alert(1)", "", false},

0 commit comments

Comments
 (0)