From 87ae0be9a34a16de5f7d5291bafc8f07bb2b07b1 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 3 Apr 2024 17:55:44 +0000 Subject: [PATCH 01/10] feat: add s suffix to use HTTPS for ports --- coderd/workspaceapps/request.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index 0f3eddf6cbd9a..1866b3bb4ca89 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -291,7 +291,18 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR appURL string appSharingLevel database.AppSharingLevel portUint, portUintErr = strconv.ParseUint(r.AppSlugOrPort, 10, 16) + protocol = "http" ) + // If we fail to parse the port, see if it's a port with a trailing "s" for + // HTTPS. + if portUintErr != nil && strings.HasSuffix(r.AppSlugOrPort, "s") { + appSlugOrPort := strings.TrimRight(r.AppSlugOrPort, "s") + portUint, portUintErr = strconv.ParseUint(appSlugOrPort, 10, 16) + if portUintErr == nil { + protocol = "https" + } + } + if portUintErr == nil { if r.AccessMethod != AccessMethodSubdomain { // TODO(@deansheather): this should return a 400 instead of a 500. @@ -312,7 +323,7 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR // "anonymous app". We only support HTTP for port-based URLs. // // This is only supported for subdomain-based applications. - appURL = fmt.Sprintf("http://127.0.0.1:%d", portUint) + appURL = fmt.Sprintf("%s://127.0.0.1:%d", protocol, portUint) appSharingLevel = database.AppSharingLevelOwner // Port sharing authorization From e8d0fae650cda0c32ed878cf7f425e7f847c8da8 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Fri, 5 Apr 2024 15:31:18 +0000 Subject: [PATCH 02/10] add valudate tests --- coderd/workspaceapps/request.go | 6 +--- coderd/workspaceapps/request_test.go | 20 ++++++++++++ coderd/workspaceapps/token_test.go | 48 ++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index 1866b3bb4ca89..9a2bd5d225f6c 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -320,7 +320,7 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR } // If the app slug is a port number, then route to the port as an - // "anonymous app". We only support HTTP for port-based URLs. + // "anonymous app". // // This is only supported for subdomain-based applications. appURL = fmt.Sprintf("%s://127.0.0.1:%d", protocol, portUint) @@ -353,10 +353,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR } // No port share found, so we keep default to owner. } else { - if ps.Protocol == database.PortShareProtocolHttps { - // Apply HTTPS protocol if specified. - appURL = fmt.Sprintf("https://127.0.0.1:%d", portUint) - } appSharingLevel = ps.ShareLevel } } else { diff --git a/coderd/workspaceapps/request_test.go b/coderd/workspaceapps/request_test.go index 7240937a06d9f..b6e4bb7a2e65f 100644 --- a/coderd/workspaceapps/request_test.go +++ b/coderd/workspaceapps/request_test.go @@ -57,6 +57,26 @@ func Test_RequestValidate(t *testing.T) { AppSlugOrPort: "baz", }, }, + { + name: "OK5", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AppSlugOrPort: "8080", + }, + }, + { + name: "OK6", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AppSlugOrPort: "8080s", + }, + }, { name: "NoAccessMethod", req: workspaceapps.Request{ diff --git a/coderd/workspaceapps/token_test.go b/coderd/workspaceapps/token_test.go index 06ab8a2acd4b2..db9a063d8e40c 100644 --- a/coderd/workspaceapps/token_test.go +++ b/coderd/workspaceapps/token_test.go @@ -222,6 +222,54 @@ func Test_TokenMatchesRequest(t *testing.T) { }, want: false, }, + { + name: "PortPortocolHTTP", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + Prefix: "yolo--", + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "8080", + }, + token: workspaceapps.SignedToken{ + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + Prefix: "yolo--", + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "8080", + }, + }, + want: true, + }, + { + name: "PortPortocolHTTPS", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + Prefix: "yolo--", + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "8080s", + }, + token: workspaceapps.SignedToken{ + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + Prefix: "yolo--", + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "8080", + }, + }, + want: true, + }, } for _, c := range cases { From 3fe678722376b97a747e9fa7a53becb085668057 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Fri, 5 Apr 2024 15:46:45 +0000 Subject: [PATCH 03/10] add more tests --- .vscode/settings.json | 3 ++- coderd/workspaceapps/token_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 1f47d7ecf99d4..8d18daa6d02a6 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -222,5 +222,6 @@ "go.testFlags": ["-short", "-coverpkg=./..."], // We often use a version of TypeScript that's ahead of the version shipped // with VS Code. - "typescript.tsdk": "./site/node_modules/typescript/lib" + "typescript.tsdk": "./site/node_modules/typescript/lib", + "codeQL.githubDatabase.download": "never" } diff --git a/coderd/workspaceapps/token_test.go b/coderd/workspaceapps/token_test.go index db9a063d8e40c..c656ae2ab77b8 100644 --- a/coderd/workspaceapps/token_test.go +++ b/coderd/workspaceapps/token_test.go @@ -265,7 +265,7 @@ func Test_TokenMatchesRequest(t *testing.T) { UsernameOrID: "foo", WorkspaceNameOrID: "bar", AgentNameOrID: "baz", - AppSlugOrPort: "8080", + AppSlugOrPort: "8080s", }, }, want: true, From 0ab922a12ecfe8db869de8eced1afa3fcd1635f6 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 8 Apr 2024 16:34:40 +0000 Subject: [PATCH 04/10] reverse parse order --- coderd/workspaceapps/request.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index 9a2bd5d225f6c..6bfc5f48df92d 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -290,20 +290,17 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR agentNameOrID = r.AgentNameOrID appURL string appSharingLevel database.AppSharingLevel - portUint, portUintErr = strconv.ParseUint(r.AppSlugOrPort, 10, 16) - protocol = "http" + potentialPortStr = strings.TrimSuffix(r.AppSlugOrPort, "s") + portUint, portUintErr = strconv.ParseUint(potentialPortStr, 10, 16) ) - // If we fail to parse the port, see if it's a port with a trailing "s" for - // HTTPS. - if portUintErr != nil && strings.HasSuffix(r.AppSlugOrPort, "s") { - appSlugOrPort := strings.TrimRight(r.AppSlugOrPort, "s") - portUint, portUintErr = strconv.ParseUint(appSlugOrPort, 10, 16) - if portUintErr == nil { + if portUintErr == nil { + // If we fail to parse the port, see if it's a port with a trailing "s" for + // HTTPS. + protocol := "http" + if strings.HasSuffix(r.AppSlugOrPort, "s") { protocol = "https" } - } - if portUintErr == nil { if r.AccessMethod != AccessMethodSubdomain { // TODO(@deansheather): this should return a 400 instead of a 500. return nil, xerrors.New("port-based URLs are only supported for subdomain-based applications") From b3d60a4d04ac85846766039470c0362fcbff7c1f Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 8 Apr 2024 16:55:18 +0000 Subject: [PATCH 05/10] add tests --- coderd/workspaceapps/db_test.go | 63 +++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/coderd/workspaceapps/db_test.go b/coderd/workspaceapps/db_test.go index eccc96d0080b4..a14dc2c78c233 100644 --- a/coderd/workspaceapps/db_test.go +++ b/coderd/workspaceapps/db_test.go @@ -40,6 +40,7 @@ func Test_ResolveRequest(t *testing.T) { // Users can access unhealthy and initializing apps (as of 2024-02). appNameUnhealthy = "app-unhealthy" appNameInitializing = "app-initializing" + appNameEndsInS = "app-ends-in-s" // This agent will never connect, so it will never become "connected". // Users cannot access unhealthy agents. @@ -166,6 +167,12 @@ func Test_ResolveRequest(t *testing.T) { Threshold: 1000, }, }, + { + Slug: appNameEndsInS, + DisplayName: appNameEndsInS, + SharingLevel: proto.AppSharingLevel_OWNER, + Url: appURL, + }, }, }, { @@ -644,6 +651,62 @@ func Test_ResolveRequest(t *testing.T) { require.Equal(t, "http://127.0.0.1:9090", token.AppURL) }) + t.Run("PortSubdomainHTTPSS", func(t *testing.T) { + t.Parallel() + + req := (workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: me.Username, + WorkspaceNameOrID: workspace.Name, + AgentNameOrID: agentName, + AppSlugOrPort: "9090ss", + }).Normalize() + + rw := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) + + _, ok := workspaceapps.ResolveRequest(rw, r, workspaceapps.ResolveRequestOptions{ + Logger: api.Logger, + SignedTokenProvider: api.WorkspaceAppsProvider, + DashboardURL: api.AccessURL, + PathAppBaseURL: api.AccessURL, + AppHostname: api.AppHostname, + AppRequest: req, + }) + // should parse as app and fail to find app "9090ss" + require.False(t, ok) + }) + + t.Run("SubdomainEndsInS", func(t *testing.T) { + t.Parallel() + + req := (workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: me.Username, + WorkspaceNameOrID: workspace.Name, + AgentNameOrID: agentName, + AppSlugOrPort: appNameEndsInS, + }).Normalize() + + rw := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) + + token, ok := workspaceapps.ResolveRequest(rw, r, workspaceapps.ResolveRequestOptions{ + Logger: api.Logger, + SignedTokenProvider: api.WorkspaceAppsProvider, + DashboardURL: api.AccessURL, + PathAppBaseURL: api.AccessURL, + AppHostname: api.AppHostname, + AppRequest: req, + }) + require.True(t, ok) + require.Equal(t, req.AppSlugOrPort, token.AppSlugOrPort) + }) + t.Run("Terminal", func(t *testing.T) { t.Parallel() From 755d1efdcb5fc69b77371e0ff7b420306e56501f Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 8 Apr 2024 16:56:19 +0000 Subject: [PATCH 06/10] fix vscode file --- .vscode/settings.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 8d18daa6d02a6..1f47d7ecf99d4 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -222,6 +222,5 @@ "go.testFlags": ["-short", "-coverpkg=./..."], // We often use a version of TypeScript that's ahead of the version shipped // with VS Code. - "typescript.tsdk": "./site/node_modules/typescript/lib", - "codeQL.githubDatabase.download": "never" + "typescript.tsdk": "./site/node_modules/typescript/lib" } From 816660c4f0b22e2ee989da8e8798d81aeeb8515f Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 8 Apr 2024 17:15:35 +0000 Subject: [PATCH 07/10] let shared port protocol call shots for now --- coderd/workspaceapps/request.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index 6bfc5f48df92d..d0c810f5e62d2 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -287,15 +287,14 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR // whether the app is a slug or a port and whether there are multiple agents // in the workspace or not. var ( - agentNameOrID = r.AgentNameOrID - appURL string - appSharingLevel database.AppSharingLevel + agentNameOrID = r.AgentNameOrID + appURL string + appSharingLevel database.AppSharingLevel + // First check if it's a port-based URL with an optional "s" suffix for HTTPS. potentialPortStr = strings.TrimSuffix(r.AppSlugOrPort, "s") portUint, portUintErr = strconv.ParseUint(potentialPortStr, 10, 16) ) if portUintErr == nil { - // If we fail to parse the port, see if it's a port with a trailing "s" for - // HTTPS. protocol := "http" if strings.HasSuffix(r.AppSlugOrPort, "s") { protocol = "https" @@ -350,6 +349,10 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR } // No port share found, so we keep default to owner. } else { + if ps.Protocol == database.PortShareProtocolHttps { + // Apply HTTPS protocol if specified. + appURL = fmt.Sprintf("https://127.0.0.1:%d", portUint) + } appSharingLevel = ps.ShareLevel } } else { From f090196613dd961cd2e3482f46085adc51d0098d Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 8 Apr 2024 17:24:32 +0000 Subject: [PATCH 08/10] lint --- coderd/prometheusmetrics/prometheusmetrics_test.go | 3 ++- coderd/workspaceapps/request.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/coderd/prometheusmetrics/prometheusmetrics_test.go b/coderd/prometheusmetrics/prometheusmetrics_test.go index 0ca7884cfbd8c..a6aa075a6044a 100644 --- a/coderd/prometheusmetrics/prometheusmetrics_test.go +++ b/coderd/prometheusmetrics/prometheusmetrics_test.go @@ -11,13 +11,14 @@ import ( "testing" "time" - "github.com/coder/coder/v2/cryptorand" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" + "github.com/coder/coder/v2/cryptorand" + "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index d0c810f5e62d2..d0fba4256cf03 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -294,6 +294,7 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR potentialPortStr = strings.TrimSuffix(r.AppSlugOrPort, "s") portUint, portUintErr = strconv.ParseUint(potentialPortStr, 10, 16) ) + //nolint:nestif if portUintErr == nil { protocol := "http" if strings.HasSuffix(r.AppSlugOrPort, "s") { From 5ee05b8573c0f4175ef6aa7cd3baf51c047727a3 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 8 Apr 2024 17:25:09 +0000 Subject: [PATCH 09/10] random lint --- coderd/prometheusmetrics/prometheusmetrics_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/coderd/prometheusmetrics/prometheusmetrics_test.go b/coderd/prometheusmetrics/prometheusmetrics_test.go index a6aa075a6044a..0ca7884cfbd8c 100644 --- a/coderd/prometheusmetrics/prometheusmetrics_test.go +++ b/coderd/prometheusmetrics/prometheusmetrics_test.go @@ -11,14 +11,13 @@ import ( "testing" "time" + "github.com/coder/coder/v2/cryptorand" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" - "github.com/coder/coder/v2/cryptorand" - "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" From 3c9790e510cb3166a1298efef45258fba6532bfd Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Tue, 9 Apr 2024 15:29:59 +0000 Subject: [PATCH 10/10] check string in body --- coderd/workspaceapps/db_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/workspaceapps/db_test.go b/coderd/workspaceapps/db_test.go index a14dc2c78c233..e8c7464f88ff1 100644 --- a/coderd/workspaceapps/db_test.go +++ b/coderd/workspaceapps/db_test.go @@ -677,6 +677,11 @@ func Test_ResolveRequest(t *testing.T) { }) // should parse as app and fail to find app "9090ss" require.False(t, ok) + w := rw.Result() + _ = w.Body.Close() + b, err := io.ReadAll(w.Body) + require.NoError(t, err) + require.Contains(t, string(b), "404 - Application Not Found") }) t.Run("SubdomainEndsInS", func(t *testing.T) {