fixup! chore: add test for subdomain proxy passthrough

coder · deansheather · Sep 22, 2022 · Sep 19, 2022 · Sep 19, 2022 · Sep 19, 2022
commit 3b875b2cf454508af5d81c49daff9fc87e11d506
diff --git a/coderd/httpapi/url.go b/coderd/httpapi/url.go
@@ -22,15 +22,14 @@ var (
 // SplitSubdomain splits a subdomain from the rest of the hostname. E.g.:
 //   - "foo.bar.com" becomes "foo", "bar.com"
 //   - "foo.bar.baz.com" becomes "foo", "bar.baz.com"
-//
-// An error is returned if the string doesn't contain a period.
-func SplitSubdomain(hostname string) (subdomain string, rest string, err error) {
+//   - "foo" becomes "foo", ""
+func SplitSubdomain(hostname string) (subdomain string, rest string) {
 	toks := strings.SplitN(hostname, ".", 2)
 	if len(toks) < 2 {
-		return "", "", xerrors.New("no subdomain")
+		return toks[0], ""
 	}
 
-	return toks[0], toks[1], nil
+	return toks[0], toks[1]
 }
 
 // ApplicationURL is a parsed application URL hostname.

diff --git a/coderd/httpapi/url_test.go b/coderd/httpapi/url_test.go
@@ -15,49 +15,42 @@ func TestSplitSubdomain(t *testing.T) {
 		Host              string
 		ExpectedSubdomain string
 		ExpectedRest      string
-		ExpectedErr       string
 	}{
 		{
 			Name:              "Empty",
 			Host:              "",
 			ExpectedSubdomain: "",
 			ExpectedRest:      "",
-			ExpectedErr:       "no subdomain",
 		},
 		{
 			Name:              "NoSubdomain",
 			Host:              "com",
-			ExpectedSubdomain: "",
+			ExpectedSubdomain: "com",
 			ExpectedRest:      "",
-			ExpectedErr:       "no subdomain",
 		},
 		{
 			Name:              "Domain",
 			Host:              "coder.com",
 			ExpectedSubdomain: "coder",
 			ExpectedRest:      "com",
-			ExpectedErr:       "",
 		},
 		{
 			Name:              "Subdomain",
 			Host:              "subdomain.coder.com",
 			ExpectedSubdomain: "subdomain",
 			ExpectedRest:      "coder.com",
-			ExpectedErr:       "",
 		},
 		{
 			Name:              "DoubleSubdomain",
 			Host:              "subdomain1.subdomain2.coder.com",
 			ExpectedSubdomain: "subdomain1",
 			ExpectedRest:      "subdomain2.coder.com",
-			ExpectedErr:       "",
 		},
 		{
 			Name:              "WithPort",
 			Host:              "subdomain.coder.com:8080",
 			ExpectedSubdomain: "subdomain",
 			ExpectedRest:      "coder.com:8080",
-			ExpectedErr:       "",
 		},
 	}
 
@@ -66,13 +59,7 @@ func TestSplitSubdomain(t *testing.T) {
 		t.Run(c.Name, func(t *testing.T) {
 			t.Parallel()
 
-			subdomain, rest, err := httpapi.SplitSubdomain(c.Host)
-			if c.ExpectedErr != "" {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), c.ExpectedErr)
-			} else {
-				require.NoError(t, err)
-			}
+			subdomain, rest := httpapi.SplitSubdomain(c.Host)
 			require.Equal(t, c.ExpectedSubdomain, subdomain)
 			require.Equal(t, c.ExpectedRest, rest)
 		})

diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go
@@ -50,6 +50,41 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
 	}, rw, r)
 }
 
+// handleSubdomainApplications handles subdomain-based application proxy
+// requests (aka. DevURLs in Coder V1).
+//
+// There are a lot of paths here:
+//  1. If api.AppHostname is not set then we pass on.
+//  2. If we can't read the request hostname then we return a 400.
+//  3. If the request hostname matches api.AccessURL then we pass on.
+//  5. We split the subdomain into the subdomain and the "rest". If there are no
+//     periods in the hostname then we pass on.
+//  5. We parse the subdomain into a httpapi.ApplicationURL struct. If we
+//     encounter an error:
+//     a. If the "rest" does not match api.AppHostname then we pass on;
+//     b. Otherwise, we return a 400.
+//  6. Finally, we verify that the "rest" matches api.AppHostname, else we
+//     return a 404.
+//
+// Rationales for each of the above steps:
+//  1. We pass on if api.AppHostname is not set to avoid returning any errors if
+//     `--app-hostname` is not configured.
+//  2. Every request should have a valid Host header anyways.
+//  3. We pass on if the request hostname matches api.AccessURL so we can
+//     support having the access URL be at the same level as the application
+//     base hostname.
+//  4. We pass on if there are no periods in the hostname as application URLs
+//     must be a subdomain of a hostname, which implies there must be at least
+//     one period.
+//  5. a. If the request subdomain is not a valid application URL, and the
+//     "rest" does not match api.AppHostname, then it is very unlikely that
+//     the request was intended for this handler. We pass on.
+//     b. If the request subdomain is not a valid application URL, but the
+//     "rest" matches api.AppHostname, then we return a 400 because the
+//     request is probably a typo or something.
+//  6. We finally verify that the "rest" matches api.AppHostname for security
+//     purposes regarding re-authentication and application proxy session
+//     tokens.
 func (api *API) handleSubdomainApplications(middlewares ...func(http.Handler) http.Handler) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
@@ -77,38 +112,52 @@ func (api *API) handleSubdomainApplications(middlewares ...func(http.Handler) ht
 				return
 			}
 
-			// Check if the hostname matches the access URL.
+			// Check if the hostname matches the access URL. If it does, the
+			// user was definitely trying to connect to the dashboard/API.
 			if httpapi.HostnamesMatch(api.AccessURL.Hostname(), host) {
-				// The user was definitely trying to connect to the
-				// dashboard/API.
 				next.ServeHTTP(rw, r)
 				return
 			}
 
-			// Split the subdomain and verify it matches the configured app
-			// hostname.
-			subdomain, rest, err := httpapi.SplitSubdomain(host)
-			if err != nil {
-				httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
-					Message: fmt.Sprintf("Could not split request Host header %q.", host),
-					Detail:  err.Error(),
-				})
-				return
-			}
-			if !httpapi.HostnamesMatch(api.AppHostname, rest) {
-				httpapi.ResourceNotFound(rw)
+			// Split the subdomain so we can parse the application details and
+			// verify it matches the configured app hostname later.
+			subdomain, rest := httpapi.SplitSubdomain(host)
+			if rest == "" {
+				// If there are no periods in the hostname, then it can't be a
+				// valid application URL.
+				next.ServeHTTP(rw, r)
 				return
 			}
+			matchingBaseHostname := httpapi.HostnamesMatch(api.AppHostname, rest)
 
+			// Parse the application URL from the subdomain.
 			app, err := httpapi.ParseSubdomainAppURL(subdomain)
 			if err != nil {
+				// If it isn't a valid app URL and the base domain doesn't match
+				// the configured app hostname, this request was probably
+				// destined for the dashboard/API router.
+				if !matchingBaseHostname {
+					next.ServeHTTP(rw, r)
+					return
+				}
+
 				httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
 					Message: "Could not parse subdomain application URL.",
 					Detail:  err.Error(),
 				})
 				return
 			}
 
+			// At this point we've verified that the subdomain looks like a
+			// valid application URL, so the base hostname should match the
+			// configured app hostname.
+			if !matchingBaseHostname {
+				httpapi.Write(rw, http.StatusNotFound, codersdk.Response{
+					Message: "The server does not accept application requests on this hostname.",
+				})
+				return
+			}
+
 			workspaceAgentKey := fmt.Sprintf("%s.%s", app.WorkspaceName, app.AgentName)
 			chiCtx := chi.RouteContext(ctx)
 			chiCtx.URLParams.Add("workspace_and_agent", workspaceAgentKey)

diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go
@@ -232,7 +232,10 @@ func TestWorkspaceAppsProxyPath(t *testing.T) {
 func TestWorkspaceAppsProxySubdomainPassthrough(t *testing.T) {
 	t.Parallel()
 
-	client := coderdtest.New(t, nil)
+	// No AppHostname set.
+	client := coderdtest.New(t, &coderdtest.Options{
+		AppHostname: "",
+	})
 	firstUser := coderdtest.CreateFirstUser(t, client)
 
 	// Configure the HTTP client to always route all requests to the coder test
@@ -261,6 +264,73 @@ func TestWorkspaceAppsProxySubdomainPassthrough(t *testing.T) {
 	require.Equal(t, firstUser.UserID, user.ID)
 }
 
+// This test ensures that the subdomain handler blocks the request if it looks
+// like a workspace app request but the configured app hostname differs from the
+// request, or the request is not a valid app subdomain but the hostname
+// matches.
+func TestWorkspaceAppsProxySubdomainBlocked(t *testing.T) {
+	t.Parallel()
+
+	setup := func(t *testing.T, appHostname string) *codersdk.Client {
+		client := coderdtest.New(t, &coderdtest.Options{
+			AppHostname: appHostname,
+		})
+		_ = coderdtest.CreateFirstUser(t, client)
+
+		// Configure the HTTP client to always route all requests to the coder test
+		// server.
+		defaultTransport, ok := http.DefaultTransport.(*http.Transport)
+		require.True(t, ok)
+		transport := defaultTransport.Clone()
+		transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+			return (&net.Dialer{}).DialContext(ctx, network, client.URL.Host)
+		}
+		client.HTTPClient.Transport = transport
+
+		return client
+	}
+
+	t.Run("NotMatchingHostname", func(t *testing.T) {
+		t.Parallel()
+		client := setup(t, "test."+proxyTestSubdomain)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		uri := fmt.Sprintf("http://app--agent--workspace--username.%s/api/v2/users/me", proxyTestSubdomain)
+		resp, err := client.Request(ctx, http.MethodGet, uri, nil)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		// Should have an error response.
+		require.Equal(t, http.StatusNotFound, resp.StatusCode)
+		var resBody codersdk.Response
+		err = json.NewDecoder(resp.Body).Decode(&resBody)
+		require.NoError(t, err)
+		require.Contains(t, resBody.Message, "does not accept application requests on this hostname")
+	})
+
+	t.Run("InvalidSubdomain", func(t *testing.T) {
+		t.Parallel()
+		client := setup(t, proxyTestSubdomain)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		uri := fmt.Sprintf("http://not-an-app-subdomain.%s/api/v2/users/me", proxyTestSubdomain)
+		resp, err := client.Request(ctx, http.MethodGet, uri, nil)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+
+		// Should have an error response.
+		require.Equal(t, http.StatusBadRequest, resp.StatusCode)
+		var resBody codersdk.Response
+		err = json.NewDecoder(resp.Body).Decode(&resBody)
+		require.NoError(t, err)
+		require.Contains(t, resBody.Message, "Could not parse subdomain application URL")
+	})
+}
+
 func TestWorkspaceAppsProxySubdomain(t *testing.T) {
 	t.Parallel()
 	client, orgID, workspace, port := setupProxyTest(t)