From 2f879910af7d0017736ea35bfa0f34eef557dd66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?McKayla=20=E3=81=AF=E3=81=AA?= Date: Thu, 23 Jul 2026 11:56:07 -0600 Subject: [PATCH] fix(coderd): harden oauth2 redirect validation (#27274) Closes DEVEX-604 Hardens `redirect` URL handling in the OAuth2/OIDC/external-auth callback flows so redirects are always reduced to a safe, relative path local to the application. Previously a redirect value with an opaque scheme (e.g. `javascript:...`) or a path with multiple leading slashes (e.g. `///evil.com`) could survive sanitization mostly intact. Also de-duplicates the previously copy-pasted `uriFromURL` helper (now exported `httpmw.URIFromURL`) so there's a single implementation shared by `coderd/userauth.go`, `coderd/externalauth.go`, and `coderd/httpmw/oauth2.go`.
Context Addresses a low-severity finding reported via a pentest disclosure: the redirect sanitizer used `url.Parse(...).RequestURI()`, which doesn't reject non-hierarchical (opaque) URLs and doesn't collapse extra leading slashes, so crafted `redirect` values could partially survive sanitization.
This PR was authored by a Coder Agent on behalf of @aslilac. --- coderd/externalauth.go | 12 +-------- coderd/httpapi/redirect.go | 31 +++++++++++++++++++++ coderd/httpapi/redirect_test.go | 48 +++++++++++++++++++++++++++++++++ coderd/httpmw/oauth2.go | 11 +------- coderd/userauth.go | 4 +-- 5 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 coderd/httpapi/redirect.go create mode 100644 coderd/httpapi/redirect_test.go diff --git a/coderd/externalauth.go b/coderd/externalauth.go index 29eb53e679..51b7727c00 100644 --- a/coderd/externalauth.go +++ b/coderd/externalauth.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net/http" - "net/url" "github.com/sqlc-dev/pqtype" "golang.org/x/sync/errgroup" @@ -331,7 +330,7 @@ func (api *API) externalAuthCallback(externalAuthConfig *externalauth.Config) ht // FE know not to enter the authentication loop again, and instead display an error. redirect = fmt.Sprintf("/external-auth/%s?redirected=true", externalAuthConfig.ID) } - redirect = uriFromURL(redirect) + redirect = httpapi.SafeRedirectPath(redirect) http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect) } } @@ -429,12 +428,3 @@ func ExternalAuthConfig(cfg *externalauth.Config) codersdk.ExternalAuthLinkProvi CodeChallengeMethodsSupported: slice.ToStrings(cfg.CodeChallengeMethodsSupported), } } - -func uriFromURL(u string) string { - uri, err := url.Parse(u) - if err != nil { - return "/" - } - - return uri.RequestURI() -} diff --git a/coderd/httpapi/redirect.go b/coderd/httpapi/redirect.go new file mode 100644 index 0000000000..6c1c49e39e --- /dev/null +++ b/coderd/httpapi/redirect.go @@ -0,0 +1,31 @@ +package httpapi + +import ( + "net/url" + "strings" +) + +// SafeRedirectPath reduces a redirect URL down to a safe, relative path. The +// scheme and host are dropped to prevent redirecting to another origin. Opaque +// URLs (e.g. `javascript:`, `data:`) are rejected outright and default to /. +func SafeRedirectPath(u string) string { + uri, err := url.Parse(u) + if err != nil || uri.Opaque != "" { + return "/" + } + + // A path with 2 or more leading slashes (e.g. "//evil.com") is interpreted as + // protocol-relative, so make sure there is exactly one. + path := "/" + strings.TrimLeft(uri.EscapedPath(), "/") + if uri.RawQuery != "" { + path += "?" + uri.RawQuery + } + // We're specifically checking Fragment instead of RawFragment here because + // RawFragment is only populated when the parser needs to preserve a + // non-default escaping, so it is empty for plain-alphanumeric fragments like + // "#wooble". EscapedFragment handles escaping correctly in either case. + if uri.Fragment != "" { + path += "#" + uri.EscapedFragment() + } + return path +} diff --git a/coderd/httpapi/redirect_test.go b/coderd/httpapi/redirect_test.go new file mode 100644 index 0000000000..7bb9b88d99 --- /dev/null +++ b/coderd/httpapi/redirect_test.go @@ -0,0 +1,48 @@ +package httpapi_test + +import ( + "testing" + + "github.com/coder/coder/v2/coderd/httpapi" +) + +func TestSafeRedirectPath(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want string + }{ + {"empty", "", "/"}, + {"simple path", "/foo/bar", "/foo/bar"}, + {"path with query", "/foo/bar?baz=qux", "/foo/bar?baz=qux"}, + {"path with fragment", "/foo/bar#wooble", "/foo/bar#wooble"}, + {"path with query+fragment", "/foo/bar?wibble=wobble#wooble", "/foo/bar?wibble=wobble#wooble"}, + {"no leading slash", "foo/bar", "/foo/bar"}, + {"malformed", "http://[::1]:namedport", "/"}, + // Ensure backslashes aren't a blindspot. + {"backslash after slash", `/\evil.example.com`, "/%5Cevil.example.com"}, + {"leading double backslash", `\\evil.example.com`, "/%5C%5Cevil.example.com"}, + {"backslash then slash", `\/evil.example.com`, "/%5C/evil.example.com"}, + {"mixed slash backslash", `/\/evil.example.com`, "/%5C/evil.example.com"}, + {"scheme with backslash", `https:/\evil.example.com`, "/%5Cevil.example.com"}, + // Cure53 CDM-02-009: triple-slash open redirect. + {"protocol relative triple slash", "///evil.example.com", "/evil.example.com"}, + {"protocol relative double slash", "//evil.example.com", "/"}, + {"absolute url with host", "http://evil.example.com/path", "/path"}, + {"absolute url with host and query", "https://evil.example.com/path?a=b", "/path?a=b"}, + // Cure53 CDM-02-009: javascript: scheme bypassing CSP. + {"javascript scheme", "javascript:alert(origin)", "/"}, + {"nested javascript scheme", "javascript:javascript:javascript:alert(origin)", "/"}, + {"data scheme", "data:text/html,", "/"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := httpapi.SafeRedirectPath(tt.in); got != tt.want { + t.Errorf("SafeRedirectPath(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 71b20a2f28..fccc89642c 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -133,7 +133,7 @@ func ExtractOAuth2(config promoauth.OAuth2Config, client *http.Client, cookieCfg // the host of the AccessURL but ultimately as long as our redirect // url omits a host we're ensuring that we're routing to a path // local to the application. - redirect = uriFromURL(redirect) + redirect = httpapi.SafeRedirectPath(redirect) } // When dynamic redirect URIs are enabled, validate the request Host @@ -532,15 +532,6 @@ func ExtractOAuth2ProviderAppSecret(db database.Store) func(http.Handler) http.H } } -func uriFromURL(u string) string { - uri, err := url.Parse(u) - if err != nil { - return "/" - } - - return uri.RequestURI() -} - // buildDynamicRedirectURI constructs the OIDC redirect_uri from the incoming // request, used when CODER_OIDC_REDIRECT_ALLOWED_HOSTS is configured. // diff --git a/coderd/userauth.go b/coderd/userauth.go index 4babef1be8..aae2854373 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1140,7 +1140,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { http.SetCookie(rw, cookie) } - redirect = uriFromURL(redirect) + redirect = httpapi.SafeRedirectPath(redirect) if api.GithubOAuth2Config.DeviceFlowEnabled { // In the device flow, the redirect is handled client-side. httpapi.Write(ctx, rw, http.StatusOK, codersdk.OAuth2DeviceFlowCallbackResponse{ @@ -1574,7 +1574,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { redirect := state.Redirect // Strip the host if it exists on the URL to prevent // any nefarious redirects. - redirect = uriFromURL(redirect) + redirect = httpapi.SafeRedirectPath(redirect) http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect) }