coder
diff --git a/‎coderd/workspaceapps/proxy.go‎
Lines changed: 35 additions & 13 deletions b/‎coderd/workspaceapps/proxy.go‎
Lines changed: 35 additions & 13 deletions
diff --git a/‎coderd/workspaceapps/proxy_internal_test.go‎
Lines changed: 117 additions & 0 deletions b/‎coderd/workspaceapps/proxy_internal_test.go‎
Lines changed: 117 additions & 0 deletions
@@ -165,6 +165,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.
 //
@@ -265,19 +277,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
 }
 
@@ -615,7 +625,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 != "" {
@@ -624,8 +641,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)
+			})
+		}
+	})
+}