fix: prevent open redirect in proxy authentication flow (#26647)

2026-09-24 15:04:27 +08:00 · 2026-06-25 11:17:23 -05:00
parent 11efcc0656
commit 7e7a6b4f18
3 changed files with 291 additions and 58 deletions
@@ -166,6 +166,18 @@ func (s *Server) Attach(r chi.Router) {
 	r.Get("/api/v2/workspaceagents/{workspaceagent}/pty", s.workspaceAgentPTY)
 }

+// originLocalURL returns p as a relative URL rooted at the current origin,
+// safe to use as a redirect Location. p (typically r.URL.Path) is
+// attacker-controlled and already percent-decoded, so a leading "//" or "/\"
+// run would otherwise be parsed by http.Redirect or a browser as a
+// scheme-relative URL pointing at another host, turning a redirect back to the
+// same path into an open redirect. Collapsing the leading slash and backslash
+// run to a single "/" keeps the result same-origin, and url.URL.String()
+// percent-encodes any control characters in the path.
+func originLocalURL(p string) *url.URL {
+	return &url.URL{Path: "/" + strings.TrimLeft(p, `/\`)}
+}
+
 // handleAPIKeySmuggling is called by the proxy path and subdomain handlers to
 // process any "smuggled" API keys in the query parameters.
 //
@@ -266,19 +278,17 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request,
 		HttpOnly: true,
 	}))

-	// Strip the query parameter.
-	path := r.URL.Path
-	if path == "" {
-		path = "/"
-	}
+	// Strip the smuggled API key query parameter and redirect back to the same
+	// path. r.URL.Path is attacker-controlled and can smuggle a separate host
+	// (e.g. "//evil.com"); originLocalURL keeps the redirect on the current
+	// origin.
+	redirectURL := originLocalURL(r.URL.Path)
+
 	q := r.URL.Query()
 	q.Del(SubdomainProxyAPIKeyParam)
-	rawQuery := q.Encode()
-	if rawQuery != "" {
-		path += "?" + q.Encode()
-	}
+	redirectURL.RawQuery = q.Encode()

-	http.Redirect(rw, r, path, http.StatusSeeOther)
+	http.Redirect(rw, r, redirectURL.String(), http.StatusSeeOther)
 	return false
 }

@@ -616,7 +626,14 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT
 		// Web applications typically request paths relative to the
 		// root URL. This allows for routing behind a proxy or subpath.
 		// See https://github.com/coder/code-server/issues/241 for examples.
-		http.Redirect(rw, r, r.URL.Path+"/", http.StatusTemporaryRedirect)
+		//
+		// r.URL.Path is attacker-controlled, so sanitize it before redirecting
+		// to avoid an off-origin "//host" Location (see originLocalURL).
+		redirectURL := originLocalURL(r.URL.Path)
+		if !strings.HasSuffix(redirectURL.Path, "/") {
+			redirectURL.Path += "/"
+		}
+		http.Redirect(rw, r, redirectURL.String(), http.StatusTemporaryRedirect)
 		return
 	}
 	if path == "/" && r.URL.RawQuery == "" && appURL.RawQuery != "" {
@@ -625,8 +642,13 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT
 		// query parameters for server-side requests, but sometimes
 		// client-side applications require the query parameters to render
 		// properly. With code-server, this is the "folder" param.
-		r.URL.RawQuery = appURL.RawQuery
-		http.Redirect(rw, r, r.URL.String(), http.StatusTemporaryRedirect)
+		//
+		// r.URL.Path is attacker-controlled, so build the Location from a
+		// sanitized same-origin path instead of r.URL directly (see
+		// originLocalURL).
+		redirectURL := originLocalURL(r.URL.Path)
+		redirectURL.RawQuery = appURL.RawQuery
+		http.Redirect(rw, r, redirectURL.String(), http.StatusTemporaryRedirect)
 		return
 	}

@@ -0,0 +1,117 @@
+package workspaceapps
+
+import (
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// Test_originLocalURL checks that originLocalURL produces a redirect target that
+// stays on the current origin.
+func Test_originLocalURL(t *testing.T) {
+	t.Parallel()
+
+	t.Run("RejectsOffOrigin", func(t *testing.T) {
+		t.Parallel()
+
+		// Each path models an already-percent-decoded r.URL.Path that tries to
+		// smuggle a separate host into the redirect.
+		cases := []struct {
+			name string
+			path string
+		}{
+			{name: "DoubleSlash", path: "//evil.com/phish"},
+			{name: "TripleSlash", path: "///evil.com/phish"},
+			{name: "SlashBackslash", path: "/\\evil.com/phish"},
+			{name: "SlashBackslashSlash", path: "/\\/evil.com/phish"},
+			{name: "DoubleBackslash", path: "\\\\evil.com/phish"},
+			{name: "SlashTab", path: "/\t/evil.com/phish"},
+			{name: "SlashTabBackslash", path: "/\t\\evil.com/phish"},
+			{name: "SlashNewline", path: "/\n/evil.com/phish"},
+			{name: "SlashCarriageReturn", path: "/\r/evil.com/phish"},
+			{name: "SlashTabDoubleSlash", path: "/\t//evil.com/phish"},
+		}
+
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+
+				loc := originLocalURL(tc.path).String()
+
+				// The Location must parse as a relative, same-origin reference.
+				require.Falsef(t, strings.HasPrefix(loc, "//"),
+					"path %q produced scheme-relative Location %q", tc.path, loc)
+				parsed, err := url.Parse(loc)
+				require.NoErrorf(t, err, "path %q produced unparseable Location %q", tc.path, loc)
+				require.Emptyf(t, parsed.Scheme, "path %q produced Location %q with a scheme", tc.path, loc)
+				require.Emptyf(t, parsed.Host, "path %q produced Location %q with a host", tc.path, loc)
+
+				// It must also be free of raw bytes a browser would normalize back
+				// into an authority before resolving (a backslash becomes "/", and
+				// tab/newline/CR are stripped, either of which could re-form
+				// "//host"). url.URL.String() guarantees this by percent-encoding
+				// them; we assert it here rather than reproducing browser
+				// normalization in the code.
+				for _, raw := range []string{`\`, "\t", "\n", "\r"} {
+					require.NotContainsf(t, loc, raw,
+						"path %q produced Location %q containing a raw %q", tc.path, loc, raw)
+				}
+			})
+		}
+	})
+
+	t.Run("EscapesControlCharacters", func(t *testing.T) {
+		t.Parallel()
+
+		// A redirect built from a path containing a raw control character is an
+		// open redirect: http.Redirect emits it verbatim (url.Parse rejects the
+		// control byte and skips cleaning) and browsers strip tab/newline/CR
+		// before resolving, re-forming "//evil.com". originLocalURL percent-encodes
+		// each one. Assert every class is escaped so a future change that breaks
+		// encoding for only one class is caught.
+		cases := []struct {
+			name string
+			in   string
+			want string
+		}{
+			{name: "Tab", in: "/\t/evil.com", want: "/%09/evil.com"},
+			{name: "Newline", in: "/\n/evil.com", want: "/%0A/evil.com"},
+			{name: "CarriageReturn", in: "/\r/evil.com", want: "/%0D/evil.com"},
+		}
+
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+
+				require.Equalf(t, tc.want, originLocalURL(tc.in).String(),
+					"originLocalURL(%q) must percent-encode the control character", tc.in)
+			})
+		}
+	})
+
+	t.Run("PreservesLegitPaths", func(t *testing.T) {
+		t.Parallel()
+
+		cases := []struct {
+			name string
+			in   string
+			want string
+		}{
+			{name: "Empty", in: "", want: "/"},
+			{name: "Root", in: "/", want: "/"},
+			{name: "Simple", in: "/test", want: "/test"},
+			{name: "Nested", in: "/app/sub/page", want: "/app/sub/page"},
+			{name: "PathApp", in: "/@user/ws/apps/app", want: "/@user/ws/apps/app"},
+		}
+
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+
+				require.Equalf(t, tc.want, originLocalURL(tc.in).String(), "originLocalURL(%q)", tc.in)
+			})
+		}
+	})
+}
@@ -8,12 +8,15 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
+	"time"

 	"github.com/stretchr/testify/require"

 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/httpmw"
+	"github.com/coder/coder/v2/coderd/jwtutils"
 	"github.com/coder/coder/v2/coderd/workspaceapps"
 	"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
 	"github.com/coder/coder/v2/testutil"
@@ -34,57 +37,148 @@ func (s *fakeSignedTokenProvider) Issue(_ context.Context, _ http.ResponseWriter
 	return nil, "", false
 }

-func TestHandleSubdomain_IgnoresUntrustedForwardedHost(t *testing.T) {
+func TestHandleSubdomain(t *testing.T) {
 	t.Parallel()

-	hostnamePattern := "*--apps.test.coder.com"
-	hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern)
-	require.NoError(t, err)
+	t.Run("IgnoresUntrustedForwardedHost", func(t *testing.T) {
+		t.Parallel()

-	dashboardURL, err := url.Parse("https://dashboard.test.coder.com")
-	require.NoError(t, err)
+		hostnamePattern := "*--apps.test.coder.com"
+		hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern)
+		require.NoError(t, err)

-	provider := &fakeSignedTokenProvider{}
-	srv := workspaceapps.NewServer(workspaceapps.ServerOptions{
-		Logger:        testutil.Logger(t),
-		DashboardURL:  dashboardURL,
-		AccessURL:     dashboardURL,
-		Hostname:      hostnamePattern,
-		HostnameRegex: hostnameRegex,
-		RealIPConfig: &httpmw.RealIPConfig{
-			TrustedOrigins: []*net.IPNet{{
-				IP:   net.ParseIP("10.0.0.1"),
-				Mask: net.CIDRMask(32, 32),
-			}},
-		},
-		SignedTokenProvider: provider,
+		dashboardURL, err := url.Parse("https://dashboard.test.coder.com")
+		require.NoError(t, err)
+
+		provider := &fakeSignedTokenProvider{}
+		srv := workspaceapps.NewServer(workspaceapps.ServerOptions{
+			Logger:        testutil.Logger(t),
+			DashboardURL:  dashboardURL,
+			AccessURL:     dashboardURL,
+			Hostname:      hostnamePattern,
+			HostnameRegex: hostnameRegex,
+			RealIPConfig: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{{
+					IP:   net.ParseIP("10.0.0.1"),
+					Mask: net.CIDRMask(32, 32),
+				}},
+			},
+			SignedTokenProvider: provider,
+		})
+
+		forgedHost := appurl.ApplicationURL{
+			AppSlugOrPort: "app",
+			WorkspaceName: "workspace",
+			Username:      "victim",
+		}.String() + "--apps.test.coder.com"
+
+		nextCalled := false
+		next := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
+			nextCalled = true
+		})
+
+		// Given: a request with a forged X-Forwarded-Host set to a valid
+		// app hostname, and an immediate peer outside the trusted proxy
+		// config.
+		req := httptest.NewRequest(http.MethodGet, "https://dashboard.test.coder.com/", nil)
+		req.Header.Set(httpapi.XForwardedHostHeader, forgedHost)
+		req.RemoteAddr = "17.18.19.20:1234"
+
+		// When: HandleSubdomain runs.
+		srv.HandleSubdomain()(next).ServeHTTP(httptest.NewRecorder(), req)
+
+		// Then: it ignores untrusted X-Forwarded-Host, so the received
+		// dashboard host is used, the request falls through to the next
+		// handler, and the signed app token provider is never called.
+		require.True(t, nextCalled)
+		require.Zero(t, provider.fromRequestCalls)
+		require.Zero(t, provider.issueCalls)
 	})

-	forgedHost := appurl.ApplicationURL{
-		AppSlugOrPort: "app",
-		WorkspaceName: "workspace",
-		Username:      "victim",
-	}.String() + "--apps.test.coder.com"
+	// After consuming a smuggled API key, the handler redirects to strip the key.
+	// A path that smuggles a separate host (e.g. "//evil.com") must redirect back
+	// to the current origin, not off-site. The full path-sanitization matrix lives
+	// in Test_originLocalURL.
+	t.Run("APIKeySmugglingStaysOnOrigin", func(t *testing.T) {
+		t.Parallel()

-	nextCalled := false
-	next := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
-		nextCalled = true
+		ctx := testutil.Context(t, testutil.WaitShort)
+
+		hostnamePattern := "*--apps.test.coder.com"
+		hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern)
+		require.NoError(t, err)
+
+		dashboardURL, err := url.Parse("https://dashboard.test.coder.com")
+		require.NoError(t, err)
+
+		// StaticKey lets us mint a smuggled key the handler can decrypt.
+		// A256GCMKW needs a 32-byte key.
+		keycache := jwtutils.StaticKey{ID: "test", Key: generateSecret(t, 32)}
+		payload := workspaceapps.EncryptedAPIKeyPayload{APIKey: "fake-api-key"}
+		payload.Fill(time.Now())
+		encryptedAPIKey, err := jwtutils.Encrypt(ctx, keycache, payload)
+		require.NoError(t, err)
+
+		srv := workspaceapps.NewServer(workspaceapps.ServerOptions{
+			Logger:        testutil.Logger(t),
+			DashboardURL:  dashboardURL,
+			AccessURL:     dashboardURL,
+			Hostname:      hostnamePattern,
+			HostnameRegex: hostnameRegex,
+			RealIPConfig: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{{
+					IP:   net.ParseIP("10.0.0.1"),
+					Mask: net.CIDRMask(32, 32),
+				}},
+			},
+			SignedTokenProvider:      &fakeSignedTokenProvider{},
+			APIKeyEncryptionKeycache: keycache,
+		})
+
+		host := appurl.ApplicationURL{
+			AppSlugOrPort: "app",
+			WorkspaceName: "workspace",
+			Username:      "user",
+		}.String() + "--apps.test.coder.com"
+
+		// Set r.URL.Path directly: the slash-collapsing middleware writes to chi's
+		// RoutePath, not r.URL.Path, so the raw "//evil.com" survives to the handler.
+		// "x=1" rides along to confirm unrelated params are preserved.
+		req := httptest.NewRequest(http.MethodGet, "https://"+host+"/", nil)
+		req.Host = host
+		req.RemoteAddr = "10.0.0.1:1234"
+		req.URL.Path = "//evil.com/phish"
+		req.URL.RawQuery = url.Values{
+			workspaceapps.SubdomainProxyAPIKeyParam: {encryptedAPIKey},
+			"x":                                     {"1"},
+		}.Encode()
+
+		rec := httptest.NewRecorder()
+		nextCalled := false
+		next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
+			nextCalled = true
+		})
+
+		srv.HandleSubdomain()(next).ServeHTTP(rec, req)
+
+		res := rec.Result()
+		defer res.Body.Close()
+
+		// The key is consumed and a redirect is issued instead of proxying.
+		require.Equal(t, http.StatusSeeOther, res.StatusCode)
+		require.False(t, nextCalled)
+
+		loc := res.Header.Get("Location")
+		require.NotEmpty(t, loc)
+		require.False(t, strings.HasPrefix(loc, "//"), "redirect %q must stay same-origin", loc)
+
+		parsed, err := url.Parse(loc)
+		require.NoError(t, err)
+		require.Empty(t, parsed.Scheme, "redirect %q must not carry a scheme", loc)
+		require.Empty(t, parsed.Host, "redirect %q must not carry a host", loc)
+
+		// The smuggled key is stripped and unrelated query params survive.
+		require.Empty(t, parsed.Query().Get(workspaceapps.SubdomainProxyAPIKeyParam))
+		require.Equal(t, "1", parsed.Query().Get("x"))
 	})
-
-	// Given: a request with a forged X-Forwarded-Host set to a valid
-	// app hostname, and an immediate peer outside the trusted proxy
-	// config.
-	req := httptest.NewRequest(http.MethodGet, "https://dashboard.test.coder.com/", nil)
-	req.Header.Set(httpapi.XForwardedHostHeader, forgedHost)
-	req.RemoteAddr = "17.18.19.20:1234"
-
-	// When: HandleSubdomain runs.
-	srv.HandleSubdomain()(next).ServeHTTP(httptest.NewRecorder(), req)
-
-	// Then: it ignores untrusted X-Forwarded-Host, so the received
-	// dashboard host is used, the request falls through to the next
-	// handler, and the signed app token provider is never called.
-	require.True(t, nextCalled)
-	require.Zero(t, provider.fromRequestCalls)
-	require.Zero(t, provider.issueCalls)
 }