fix: allow ports in wildcard url configuration (#11657)

* fix: allow ports in wildcard url configuration

This just forwards the port to the ui that generates urls.
Our existing parsing + regex already supported ports for
subdomain app requests.

coderd/agentapi/manifest.go

@@ -3,7 +3,6 @@ package agentapi
 import (
 	"context"
 	"database/sql"
-	"fmt"
 	"net/url"
 	"strings"
 	"sync/atomic"
@@ -108,19 +107,14 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
 		return nil, xerrors.Errorf("fetching workspace agent data: %w", err)
 	}
 
-	appHost := appurl.ApplicationURL{
+	appSlug := appurl.ApplicationURL{
 		AppSlugOrPort: "{{port}}",
 		AgentName:     workspaceAgent.Name,
 		WorkspaceName: workspace.Name,
 		Username:      owner.Username,
 	}
-	vscodeProxyURI := a.AccessURL.Scheme + "://" + strings.ReplaceAll(a.AppHostname, "*", appHost.String())
-	if a.AppHostname == "" {
-		vscodeProxyURI += a.AccessURL.Hostname()
-	}
-	if a.AccessURL.Port() != "" {
-		vscodeProxyURI += fmt.Sprintf(":%s", a.AccessURL.Port())
-	}
+
+	vscodeProxyURI := vscodeProxyURI(appSlug, a.AccessURL, a.AppHostname)
 
 	var gitAuthConfigs uint32
 	for _, cfg := range a.ExternalAuthConfigs {
@@ -155,6 +149,17 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
 	}, nil
 }
 
+func vscodeProxyURI(app appurl.ApplicationURL, accessURL *url.URL, appHost string) string {
+	// This will handle the ports from the accessURL or appHost.
+	appHost = appurl.SubdomainAppHost(appHost, accessURL)
+	// If there is no appHost, then we want to use the access url as the proxy uri.
+	if appHost == "" {
+		appHost = accessURL.Host
+	}
+	// Return the url with a scheme and any wildcards replaced with the app slug.
+	return accessURL.Scheme + "://" + strings.ReplaceAll(appHost, "*", app.String())
+}
+
 func dbAgentMetadataToProtoDescription(metadata []database.WorkspaceAgentMetadatum) []*agentproto.WorkspaceAgentMetadata_Description {
 	ret := make([]*agentproto.WorkspaceAgentMetadata_Description, len(metadata))
 	for i, metadatum := range metadata {

coderd/agentapi/manifest_internal_test.go

@@ -0,0 +1,94 @@
+package agentapi
+
+import (
+	"fmt"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
+)
+
+func Test_vscodeProxyURI(t *testing.T) {
+	t.Parallel()
+
+	coderAccessURL, err := url.Parse("https://coder.com")
+	require.NoError(t, err)
+
+	accessURLWithPort, err := url.Parse("https://coder.com:8080")
+	require.NoError(t, err)
+
+	basicApp := appurl.ApplicationURL{
+		Prefix:        "prefix",
+		AppSlugOrPort: "slug",
+		AgentName:     "agent",
+		WorkspaceName: "workspace",
+		Username:      "user",
+	}
+
+	cases := []struct {
+		Name        string
+		App         appurl.ApplicationURL
+		AccessURL   *url.URL
+		AppHostname string
+		Expected    string
+	}{
+		{
+			// No hostname proxies through the access url.
+			Name:        "NoHostname",
+			AccessURL:   coderAccessURL,
+			AppHostname: "",
+			App:         basicApp,
+			Expected:    coderAccessURL.String(),
+		},
+		{
+			Name:        "NoHostnameAccessURLPort",
+			AccessURL:   accessURLWithPort,
+			AppHostname: "",
+			App:         basicApp,
+			Expected:    accessURLWithPort.String(),
+		},
+		{
+			Name:        "Hostname",
+			AccessURL:   coderAccessURL,
+			AppHostname: "*.apps.coder.com",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com", basicApp.String()),
+		},
+		{
+			Name:        "HostnameWithAccessURLPort",
+			AccessURL:   accessURLWithPort,
+			AppHostname: "*.apps.coder.com",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), accessURLWithPort.Port()),
+		},
+		{
+			Name:        "HostnameWithPort",
+			AccessURL:   coderAccessURL,
+			AppHostname: "*.apps.coder.com:4444",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"),
+		},
+		{
+			// Port from hostname takes precedence over access url port.
+			Name:        "HostnameWithPortAccessURLWithPort",
+			AccessURL:   accessURLWithPort,
+			AppHostname: "*.apps.coder.com:4444",
+			App:         basicApp,
+			Expected:    fmt.Sprintf("https://%s.apps.coder.com:%s", basicApp.String(), "4444"),
+		},
+	}
+
+	for _, c := range cases {
+		c := c
+		t.Run(c.Name, func(t *testing.T) {
+			t.Parallel()
+
+			require.NotNilf(t, c.AccessURL, "AccessURL is required")
+
+			output := vscodeProxyURI(c.App, c.AccessURL, c.AppHostname)
+			require.Equal(t, c.Expected, output)
+		})
+	}
+}

coderd/coderd.go

@@ -93,7 +93,7 @@ type Options struct {
 	// AppHostname should be the wildcard hostname to use for workspace
 	// applications INCLUDING the asterisk, (optional) suffix and leading dot.
 	// It will use the same scheme and port number as the access URL.
-	// E.g. "*.apps.coder.com" or "*-apps.coder.com".
+	// E.g. "*.apps.coder.com" or "*-apps.coder.com" or "*.apps.coder.com:8080".
 	AppHostname string
 	// AppHostnameRegex contains the regex version of options.AppHostname as
 	// generated by appurl.CompileHostnamePattern(). It MUST be set if

coderd/workspaceapps.go

@@ -3,7 +3,6 @@ package coderd
 import (
 	"context"
 	"database/sql"
-	"fmt"
 	"net/http"
 	"net/url"
 	"strings"
@@ -32,13 +31,8 @@ import (
 // @Router /applications/host [get]
 // @Deprecated use api/v2/regions and see the primary proxy.
 func (api *API) appHost(rw http.ResponseWriter, r *http.Request) {
-	host := api.AppHostname
-	if host != "" && api.AccessURL.Port() != "" {
-		host += fmt.Sprintf(":%s", api.AccessURL.Port())
-	}
-
 	httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AppHostResponse{
-		Host: host,
+		Host: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL),
 	})
 }
 

coderd/workspaceapps/apptest/apptest.go

@@ -963,6 +963,38 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
 			require.Equal(t, http.StatusOK, resp.StatusCode)
 		})
 
+		t.Run("WildcardPortOK", func(t *testing.T) {
+			t.Parallel()
+
+			// Manually specifying a port should override the access url port on
+			// the app host.
+			appDetails := setupProxyTest(t, &DeploymentOptions{
+				// Just throw both the wsproxy and primary to same url.
+				AppHost:        "*.test.coder.com:4444",
+				PrimaryAppHost: "*.test.coder.com:4444",
+			})
+
+			ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+			defer cancel()
+
+			u := appDetails.SubdomainAppURL(appDetails.Apps.Owner)
+			t.Logf("url: %s", u)
+			require.Equal(t, "4444", u.Port(), "port should be 4444")
+
+			// Assert the api response the UI uses has the port.
+			apphost, err := appDetails.SDKClient.AppHost(ctx)
+			require.NoError(t, err)
+			require.Equal(t, "*.test.coder.com:4444", apphost.Host, "apphost has port")
+
+			resp, err := requestWithRetries(ctx, t, appDetails.AppClient(t), http.MethodGet, u.String(), nil)
+			require.NoError(t, err)
+			defer resp.Body.Close()
+			body, err := io.ReadAll(resp.Body)
+			require.NoError(t, err)
+			require.Equal(t, proxyTestAppBody, string(body))
+			require.Equal(t, http.StatusOK, resp.StatusCode)
+		})
+
 		t.Run("SuffixWildcardNotMatch", func(t *testing.T) {
 			t.Parallel()
 

coderd/workspaceapps/apptest/setup.go

@@ -47,6 +47,7 @@ const (
 // DeploymentOptions are the options for creating a *Deployment with a
 // DeploymentFactory.
 type DeploymentOptions struct {
+	PrimaryAppHost                       string
 	AppHost                              string
 	DisablePathApps                      bool
 	DisableSubdomainApps                 bool
@@ -407,7 +408,7 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U
 			Username:      me.Username,
 		}
 		proxyURL := "http://" + appHost.String() + strings.ReplaceAll(primaryAppHost.Host, "*", "")
-		require.Equal(t, proxyURL, manifest.VSCodePortProxyURI)
+		require.Equal(t, manifest.VSCodePortProxyURI, proxyURL)
 	}
 	agentCloser := agent.New(agent.Options{
 		Client: agentClient,

coderd/workspaceapps/appurl/appurl.go

@@ -3,6 +3,7 @@ package appurl
 import (
 	"fmt"
 	"net"
+	"net/url"
 	"regexp"
 	"strings"
 
@@ -20,6 +21,36 @@ var (
 	validHostnameLabelRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
 )
 
+// SubdomainAppHost returns the URL of the apphost for subdomain based apps.
+// It will omit the scheme.
+//
+// Arguments:
+// apphost: Expected to contain a wildcard, example: "*.coder.com"
+// accessURL: The access url for the deployment.
+//
+// Returns:
+// 'apphost:port'
+//
+// For backwards compatibility and for "accessurl=localhost:0" purposes, we need
+// to use the port from the accessurl if the apphost doesn't have a port.
+// If the user specifies a port in the apphost, we will use that port instead.
+func SubdomainAppHost(apphost string, accessURL *url.URL) string {
+	if apphost == "" {
+		return ""
+	}
+
+	if apphost != "" && accessURL.Port() != "" {
+		// This should always parse if we prepend a scheme. We should add
+		// the access url port if the apphost doesn't have a port specified.
+		appHostU, err := url.Parse(fmt.Sprintf("https://%s", apphost))
+		if err != nil || (err == nil && appHostU.Port() == "") {
+			apphost += fmt.Sprintf(":%s", accessURL.Port())
+		}
+	}
+
+	return apphost
+}
+
 // ApplicationURL is a parsed application URL hostname.
 type ApplicationURL struct {
 	Prefix        string
@@ -140,9 +171,7 @@ func CompileHostnamePattern(pattern string) (*regexp.Regexp, error) {
 	if strings.Contains(pattern, "http:") || strings.Contains(pattern, "https:") {
 		return nil, xerrors.Errorf("hostname pattern must not contain a scheme: %q", pattern)
 	}
-	if strings.Contains(pattern, ":") {
-		return nil, xerrors.Errorf("hostname pattern must not contain a port: %q", pattern)
-	}
+
 	if strings.HasPrefix(pattern, ".") || strings.HasSuffix(pattern, ".") {
 		return nil, xerrors.Errorf("hostname pattern must not start or end with a period: %q", pattern)
 	}
@@ -155,6 +184,16 @@ func CompileHostnamePattern(pattern string) (*regexp.Regexp, error) {
 	if !strings.HasPrefix(pattern, "*") {
 		return nil, xerrors.Errorf("hostname pattern must only contain an asterisk at the beginning: %q", pattern)
 	}
+
+	// If there is a hostname:port, we only care about the hostname. For hostname
+	// pattern reasons, we do not actually care what port the client is requesting.
+	// Any port provided here is used for generating urls for the ui, not for
+	// validation.
+	hostname, _, err := net.SplitHostPort(pattern)
+	if err == nil {
+		pattern = hostname
+	}
+
 	for i, label := range strings.Split(pattern, ".") {
 		if i == 0 {
 			// We have to allow the asterisk to be a valid hostname label, so

coderd/workspaceapps/appurl/appurl_test.go

@@ -193,11 +193,6 @@ func TestCompileHostnamePattern(t *testing.T) {
 			pattern:       "https://*.hi.com",
 			errorContains: "must not contain a scheme",
 		},
-		{
-			name:          "Invalid_ContainsPort",
-			pattern:       "*.hi.com:8080",
-			errorContains: "must not contain a port",
-		},
 		{
 			name:          "Invalid_StartPeriod",
 			pattern:       ".hi.com",
@@ -249,6 +244,13 @@ func TestCompileHostnamePattern(t *testing.T) {
 			errorContains: "contains invalid label",
 		},
 
+		{
+			name:    "Valid_ContainsPort",
+			pattern: "*.hi.com:8080",
+			// Although a port is provided, the regex already matches any port.
+			// So it is ignored for validation purposes.
+			expectedRegex: `([^.]+)\.hi\.com`,
+		},
 		{
 			name:          "Valid_Simple",
 			pattern:       "*.hi",

coderd/workspaceproxies.go

@@ -11,6 +11,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/httpapi"
+	"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
 	"github.com/coder/coder/v2/codersdk"
 )
 
@@ -43,7 +44,7 @@ func (api *API) PrimaryRegion(ctx context.Context) (codersdk.Region, error) {
 		IconURL:          proxy.IconUrl,
 		Healthy:          true,
 		PathAppURL:       api.AccessURL.String(),
-		WildcardHostname: api.AppHostname,
+		WildcardHostname: appurl.SubdomainAppHost(api.AppHostname, api.AccessURL),
 	}, nil
 }
 

coderd/workspaceproxies_test.go

@@ -1,6 +1,7 @@
 package coderd_test
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/uuid"
@@ -44,7 +45,7 @@ func TestRegions(t *testing.T) {
 		require.NotEmpty(t, regions[0].IconURL)
 		require.True(t, regions[0].Healthy)
 		require.Equal(t, client.URL.String(), regions[0].PathAppURL)
-		require.Equal(t, appHostname, regions[0].WildcardHostname)
+		require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname)
 
 		// Ensure the primary region ID is constant.
 		regions2, err := client.Regions(ctx)

enterprise/coderd/workspaceproxy_test.go

@@ -62,7 +62,7 @@ func TestRegions(t *testing.T) {
 		require.NotEmpty(t, regions[0].IconURL)
 		require.True(t, regions[0].Healthy)
 		require.Equal(t, client.URL.String(), regions[0].PathAppURL)
-		require.Equal(t, appHostname, regions[0].WildcardHostname)
+		require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname)
 
 		// Ensure the primary region ID is constant.
 		regions2, err := client.Regions(ctx)
@@ -149,7 +149,7 @@ func TestRegions(t *testing.T) {
 		require.NotEmpty(t, regions[0].IconURL)
 		require.True(t, regions[0].Healthy)
 		require.Equal(t, client.URL.String(), regions[0].PathAppURL)
-		require.Equal(t, appHostname, regions[0].WildcardHostname)
+		require.Equal(t, fmt.Sprintf("%s:%s", appHostname, client.URL.Port()), regions[0].WildcardHostname)
 
 		// Ensure non-zero fields of the default proxy
 		require.NotZero(t, primary.Name)

enterprise/wsproxy/wsproxy_test.go

@@ -449,10 +449,13 @@ func TestWorkspaceProxyWorkspaceApps(t *testing.T) {
 			<-proxyStatsCollectorFlushDone
 		}
 
+		if opts.PrimaryAppHost == "" {
+			opts.PrimaryAppHost = "*.primary.test.coder.com"
+		}
 		client, closer, api, user := coderdenttest.NewWithAPI(t, &coderdenttest.Options{
 			Options: &coderdtest.Options{
 				DeploymentValues:         deploymentValues,
-				AppHostname:              "*.primary.test.coder.com",
+				AppHostname:              opts.PrimaryAppHost,
 				IncludeProvisionerDaemon: true,
 				RealIPConfig: &httpmw.RealIPConfig{
 					TrustedOrigins: []*net.IPNet{{