From 1a690f0cbbf6b96bea086afad4de6d01d584aaa4 Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 25 May 2023 15:19:59 -0800 Subject: [PATCH 1/7] Allow cross-origin requests between users' own apps --- coderd/coderd.go | 10 ++-- coderd/workspaceapps/proxy.go | 96 +++++++++++++++++++++++++---------- codersdk/deployment.go | 2 +- 3 files changed, 77 insertions(+), 31 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 0d5f47b8618c7..f4204d3e936a5 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -406,9 +406,7 @@ func New(options *Options) *API { derpHandler, api.derpCloseFunc = tailnet.WithWebsocketSupport(api.DERPServer, derpHandler) cors := httpmw.Cors(options.DeploymentValues.Dangerous.AllowAllCors.Value()) prometheusMW := httpmw.Prometheus(options.PrometheusRegistry) - r.Use( - cors, httpmw.Recover(api.Logger), tracing.StatusWriterMiddleware, tracing.Middleware(api.TracerProvider), @@ -419,9 +417,13 @@ func New(options *Options) *API { // SubdomainAppMW checks if the first subdomain is a valid app URL. If // it is, it will serve that application. // - // Workspace apps do their own auth and must be BEFORE the auth - // middleware. + // Workspace apps do their own auth and CORS and must be BEFORE the auth + // and CORS middleware. + // REVIEW: Would it be worth creating httpmw.ExtractWorkspaceApp and using a + // single CORS middleware? api.workspaceAppServer.HandleSubdomain(apiRateLimiter), + // REVIEW: Is it OK that CORS come after the above middleware? + cors, // Build-Version is helpful for debugging. func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 701bf296cc765..811b97592eecc 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -13,6 +13,7 @@ import ( "sync" "github.com/go-chi/chi/v5" + "github.com/go-chi/cors" "github.com/google/uuid" "go.opentelemetry.io/otel/trace" "nhooyr.io/websocket" @@ -361,41 +362,84 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler) return } - if !s.handleAPIKeySmuggling(rw, r, AccessMethodSubdomain) { - return - } - - token, ok := ResolveRequest(rw, r, ResolveRequestOptions{ - Logger: s.Logger, - SignedTokenProvider: s.SignedTokenProvider, - DashboardURL: s.DashboardURL, - PathAppBaseURL: s.AccessURL, - AppHostname: s.Hostname, - AppRequest: Request{ - AccessMethod: AccessMethodSubdomain, - BasePath: "/", - UsernameOrID: app.Username, - WorkspaceNameOrID: app.WorkspaceName, - AgentNameOrID: app.AgentName, - AppSlugOrPort: app.AppSlugOrPort, - }, - AppPath: r.URL.Path, - AppQuery: r.URL.RawQuery, - }) - if !ok { - return + // REVIEW: Like mentioned in coderd.go maybe we should extract the app + // using middleware that way we can do this in a single top-level CORS + // handler? Or just do the URL parsing twice. + var corsmw func(next http.Handler) http.Handler + origin := r.Header.Get("Origin") + if originApp, ok := s.parseOrigin(origin); ok && originApp.Username == app.Username { + corsmw = cors.Handler(cors.Options{ + AllowedOrigins: []string{origin}, + AllowedMethods: []string{ + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + }, + AllowedHeaders: []string{"*"}, + AllowCredentials: true, + }) + } else { + corsmw = cors.Handler(cors.Options{ + AllowedOrigins: []string{""}, // The middleware defaults to *. + AllowedMethods: []string{}, + AllowedHeaders: []string{}, + AllowCredentials: false, + }) } - // Use the passed in app middlewares before passing to the proxy - // app. - mws := chi.Middlewares(middlewares) + // Use the passed in app middlewares before checking authentication and + // passing to the proxy app. + mws := chi.Middlewares(append(middlewares, corsmw)) mws.Handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if !s.handleAPIKeySmuggling(rw, r, AccessMethodSubdomain) { + return + } + + token, ok := ResolveRequest(rw, r, ResolveRequestOptions{ + Logger: s.Logger, + SignedTokenProvider: s.SignedTokenProvider, + DashboardURL: s.DashboardURL, + PathAppBaseURL: s.AccessURL, + AppHostname: s.Hostname, + AppRequest: Request{ + AccessMethod: AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: app.Username, + WorkspaceNameOrID: app.WorkspaceName, + AgentNameOrID: app.AgentName, + AppSlugOrPort: app.AppSlugOrPort, + }, + AppPath: r.URL.Path, + AppQuery: r.URL.RawQuery, + }) + if !ok { + return + } s.proxyWorkspaceApp(rw, r, *token, r.URL.Path) })).ServeHTTP(rw, r.WithContext(ctx)) }) } } +func (s *Server) parseOrigin(rawOrigin string) (httpapi.ApplicationURL, bool) { + origin, err := url.Parse(rawOrigin) + if rawOrigin == "" || origin.Host == "" || err != nil { + return httpapi.ApplicationURL{}, false + } + subdomain, ok := httpapi.ExecuteHostnamePattern(s.HostnameRegex, origin.Host) + if !ok { + return httpapi.ApplicationURL{}, false + } + app, err := httpapi.ParseSubdomainAppURL(subdomain) + if err != nil { + return httpapi.ApplicationURL{}, false + } + return app, true +} + // parseHostname will return if a given request is attempting to access a // workspace app via a subdomain. If it is, the hostname of the request is parsed // into an httpapi.ApplicationURL and true is returned. If the request is not diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 12d84b3c94fe8..2537a3c51549c 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1170,7 +1170,7 @@ when required by your organization's security policy.`, // ☢️ Dangerous settings { Name: "DANGEROUS: Allow all CORs requests", - Description: "For security reasons, CORs requests are blocked. If external requests are required, setting this to true will set all cors headers as '*'. This should never be used in production.", + Description: "For security reasons, CORs requests are blocked except between workspace apps owned by the same user. If external requests are required, setting this to true will set all cors headers as '*'. This should never be used in production.", Flag: "dangerous-allow-cors-requests", Env: "CODER_DANGEROUS_ALLOW_CORS_REQUESTS", Hidden: true, // Hidden, should only be used by yarn dev server From d8a0ba9e0dfbc9a2aaa3c284fe5c6dee30ffe462 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 31 May 2023 13:58:20 -0800 Subject: [PATCH 2/7] Simplify CORS handler with AllowOriginFunc --- coderd/coderd.go | 3 --- coderd/workspaceapps/proxy.go | 45 +++++++++++++---------------------- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index f4204d3e936a5..3abb9ea4eec44 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -419,10 +419,7 @@ func New(options *Options) *API { // // Workspace apps do their own auth and CORS and must be BEFORE the auth // and CORS middleware. - // REVIEW: Would it be worth creating httpmw.ExtractWorkspaceApp and using a - // single CORS middleware? api.workspaceAppServer.HandleSubdomain(apiRateLimiter), - // REVIEW: Is it OK that CORS come after the above middleware? cors, // Build-Version is helpful for debugging. func(next http.Handler) http.Handler { diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 811b97592eecc..4d0d640f7d350 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -362,37 +362,24 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler) return } - // REVIEW: Like mentioned in coderd.go maybe we should extract the app - // using middleware that way we can do this in a single top-level CORS - // handler? Or just do the URL parsing twice. - var corsmw func(next http.Handler) http.Handler - origin := r.Header.Get("Origin") - if originApp, ok := s.parseOrigin(origin); ok && originApp.Username == app.Username { - corsmw = cors.Handler(cors.Options{ - AllowedOrigins: []string{origin}, - AllowedMethods: []string{ - http.MethodHead, - http.MethodGet, - http.MethodPost, - http.MethodPut, - http.MethodPatch, - http.MethodDelete, - }, - AllowedHeaders: []string{"*"}, - AllowCredentials: true, - }) - } else { - corsmw = cors.Handler(cors.Options{ - AllowedOrigins: []string{""}, // The middleware defaults to *. - AllowedMethods: []string{}, - AllowedHeaders: []string{}, - AllowCredentials: false, - }) - } - // Use the passed in app middlewares before checking authentication and // passing to the proxy app. - mws := chi.Middlewares(append(middlewares, corsmw)) + mws := chi.Middlewares(append(middlewares, cors.Handler(cors.Options{ + AllowOriginFunc: func(r *http.Request, origin string) bool { + originApp, ok := s.parseOrigin(origin) + return ok && originApp.Username == app.Username + }, + AllowedMethods: []string{ + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + }, + AllowedHeaders: []string{"*"}, + AllowCredentials: true, + }))) mws.Handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if !s.handleAPIKeySmuggling(rw, r, AccessMethodSubdomain) { return From 45d3565e849380893de81777093f9234158e8864 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 31 May 2023 14:05:08 -0800 Subject: [PATCH 3/7] Break out workspace app cors handler This will make it easier to test. --- coderd/httpmw/cors.go | 34 ++++++++++++++++++++++++++++++++++ coderd/workspaceapps/proxy.go | 34 +--------------------------------- 2 files changed, 35 insertions(+), 33 deletions(-) diff --git a/coderd/httpmw/cors.go b/coderd/httpmw/cors.go index 7c7e89fa53d3d..be11c4a6bfbcd 100644 --- a/coderd/httpmw/cors.go +++ b/coderd/httpmw/cors.go @@ -2,8 +2,12 @@ package httpmw import ( "net/http" + "net/url" + "regexp" "github.com/go-chi/cors" + + "github.com/coder/coder/coderd/httpapi" ) //nolint:revive @@ -25,3 +29,33 @@ func Cors(allowAll bool, origins ...string) func(next http.Handler) http.Handler AllowCredentials: false, }) } + +func WorkspaceAppCors(regex *regexp.Regexp, app httpapi.ApplicationURL) func(next http.Handler) http.Handler { + return cors.Handler(cors.Options{ + AllowOriginFunc: func(r *http.Request, rawOrigin string) bool { + origin, err := url.Parse(rawOrigin) + if rawOrigin == "" || origin.Host == "" || err != nil { + return false + } + subdomain, ok := httpapi.ExecuteHostnamePattern(regex, origin.Host) + if !ok { + return false + } + originApp, err := httpapi.ParseSubdomainAppURL(subdomain) + if err != nil { + return false + } + return ok && originApp.Username == app.Username + }, + AllowedMethods: []string{ + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + }, + AllowedHeaders: []string{"*"}, + AllowCredentials: true, + }) +} diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 4d0d640f7d350..331934be6a4d0 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -13,7 +13,6 @@ import ( "sync" "github.com/go-chi/chi/v5" - "github.com/go-chi/cors" "github.com/google/uuid" "go.opentelemetry.io/otel/trace" "nhooyr.io/websocket" @@ -364,22 +363,7 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler) // Use the passed in app middlewares before checking authentication and // passing to the proxy app. - mws := chi.Middlewares(append(middlewares, cors.Handler(cors.Options{ - AllowOriginFunc: func(r *http.Request, origin string) bool { - originApp, ok := s.parseOrigin(origin) - return ok && originApp.Username == app.Username - }, - AllowedMethods: []string{ - http.MethodHead, - http.MethodGet, - http.MethodPost, - http.MethodPut, - http.MethodPatch, - http.MethodDelete, - }, - AllowedHeaders: []string{"*"}, - AllowCredentials: true, - }))) + mws := chi.Middlewares(append(middlewares, httpmw.WorkspaceAppCors(s.HostnameRegex, app))) mws.Handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if !s.handleAPIKeySmuggling(rw, r, AccessMethodSubdomain) { return @@ -411,22 +395,6 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler) } } -func (s *Server) parseOrigin(rawOrigin string) (httpapi.ApplicationURL, bool) { - origin, err := url.Parse(rawOrigin) - if rawOrigin == "" || origin.Host == "" || err != nil { - return httpapi.ApplicationURL{}, false - } - subdomain, ok := httpapi.ExecuteHostnamePattern(s.HostnameRegex, origin.Host) - if !ok { - return httpapi.ApplicationURL{}, false - } - app, err := httpapi.ParseSubdomainAppURL(subdomain) - if err != nil { - return httpapi.ApplicationURL{}, false - } - return app, true -} - // parseHostname will return if a given request is attempting to access a // workspace app via a subdomain. If it is, the hostname of the request is parsed // into an httpapi.ApplicationURL and true is returned. If the request is not From da1298af0ae861e3effaa32c414b1262b8f96e82 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 31 May 2023 14:58:40 -0800 Subject: [PATCH 4/7] Add tests for workspace app cors --- coderd/httpmw/cors_test.go | 87 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 coderd/httpmw/cors_test.go diff --git a/coderd/httpmw/cors_test.go b/coderd/httpmw/cors_test.go new file mode 100644 index 0000000000000..7123380388ef6 --- /dev/null +++ b/coderd/httpmw/cors_test.go @@ -0,0 +1,87 @@ +package httpmw_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpapi" + "github.com/coder/coder/coderd/httpmw" +) + +func TestWorkspaceAppCors(t *testing.T) { + t.Parallel() + + regex, err := httpapi.CompileHostnamePattern("*--apps.dev.coder.com") + require.NoError(t, err) + + app := httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user", + } + + handler := httpmw.WorkspaceAppCors(regex, app) + methods := []string{ + http.MethodOptions, + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + } + + tests := []struct { + name string + origin string + allowed bool + }{ + { + name: "Self", + origin: "https://3000--agent--ws--user--apps.dev.coder.com", + allowed: true, + }, + { + name: "SameWorkspace", + origin: "https://8000--agent--ws--user--apps.dev.coder.com", + allowed: true, + }, + { + name: "SameUser", + origin: "https://8000--agent2--ws2--user--apps.dev.coder.com", + allowed: true, + }, + { + name: "DifferentUser", + origin: "https://3000--agent--ws--user2--apps.dev.coder.com", + allowed: false, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + for _, method := range methods { + r := httptest.NewRequest(method, "http://localhost", nil) + r.Header.Set("Origin", test.origin) + rw := httptest.NewRecorder() + + handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + })).ServeHTTP(rw, r) + + if test.allowed { + require.Equal(t, test.origin, rw.Header().Get("Access-Control-Allow-Origin")) + } else { + require.Equal(t, "", rw.Header().Get("Access-Control-Allow-Origin")) + } + } + }) + } +} From 1cb650aae4c43ce6c5548dbe547f5375b30eb7da Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 31 May 2023 15:11:58 -0800 Subject: [PATCH 5/7] Test options requests --- coderd/httpmw/cors_test.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/coderd/httpmw/cors_test.go b/coderd/httpmw/cors_test.go index 7123380388ef6..2c18a48433fc3 100644 --- a/coderd/httpmw/cors_test.go +++ b/coderd/httpmw/cors_test.go @@ -24,7 +24,9 @@ func TestWorkspaceAppCors(t *testing.T) { Username: "user", } - handler := httpmw.WorkspaceAppCors(regex, app) + handler := httpmw.WorkspaceAppCors(regex, app)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusNoContent) + })) methods := []string{ http.MethodOptions, http.MethodHead, @@ -72,15 +74,26 @@ func TestWorkspaceAppCors(t *testing.T) { r.Header.Set("Origin", test.origin) rw := httptest.NewRecorder() - handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - rw.WriteHeader(http.StatusOK) - })).ServeHTTP(rw, r) + // Preflight requests need to know what method will be requested. + if method == http.MethodOptions { + r.Header.Set("Access-Control-Request-Method", method) + } + + handler.ServeHTTP(rw, r) if test.allowed { require.Equal(t, test.origin, rw.Header().Get("Access-Control-Allow-Origin")) } else { require.Equal(t, "", rw.Header().Get("Access-Control-Allow-Origin")) } + + // For options we should never get to our handler as the middleware + // short-circuits with a 200. + if method == http.MethodOptions { + require.Equal(t, http.StatusOK, rw.Code) + } else { + require.Equal(t, http.StatusNoContent, rw.Code) + } } }) } From e733304a1be30cbdd41527c6c8298a0653fb9db5 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 7 Jun 2023 09:49:39 -0800 Subject: [PATCH 6/7] Move test app into test cases --- coderd/httpmw/cors_test.go | 66 +++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 18 deletions(-) diff --git a/coderd/httpmw/cors_test.go b/coderd/httpmw/cors_test.go index 2c18a48433fc3..7668771b1e6db 100644 --- a/coderd/httpmw/cors_test.go +++ b/coderd/httpmw/cors_test.go @@ -17,16 +17,6 @@ func TestWorkspaceAppCors(t *testing.T) { regex, err := httpapi.CompileHostnamePattern("*--apps.dev.coder.com") require.NoError(t, err) - app := httpapi.ApplicationURL{ - AppSlugOrPort: "3000", - AgentName: "agent", - WorkspaceName: "ws", - Username: "user", - } - - handler := httpmw.WorkspaceAppCors(regex, app)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - rw.WriteHeader(http.StatusNoContent) - })) methods := []string{ http.MethodOptions, http.MethodHead, @@ -40,26 +30,62 @@ func TestWorkspaceAppCors(t *testing.T) { tests := []struct { name string origin string + app httpapi.ApplicationURL allowed bool }{ { - name: "Self", - origin: "https://3000--agent--ws--user--apps.dev.coder.com", + name: "Self", + origin: "https://3000--agent--ws--user--apps.dev.coder.com", + app: httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user", + }, allowed: true, }, { - name: "SameWorkspace", - origin: "https://8000--agent--ws--user--apps.dev.coder.com", + name: "SameWorkspace", + origin: "https://8000--agent--ws--user--apps.dev.coder.com", + app: httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user", + }, allowed: true, }, { - name: "SameUser", - origin: "https://8000--agent2--ws2--user--apps.dev.coder.com", + name: "SameUser", + origin: "https://8000--agent2--ws2--user--apps.dev.coder.com", + app: httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user", + }, allowed: true, }, { - name: "DifferentUser", - origin: "https://3000--agent--ws--user2--apps.dev.coder.com", + name: "DifferentOriginOwner", + origin: "https://3000--agent--ws--user2--apps.dev.coder.com", + app: httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user", + }, + allowed: false, + }, + { + name: "DifferentHostOwner", + origin: "https://3000--agent--ws--user--apps.dev.coder.com", + app: httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user2", + }, allowed: false, }, } @@ -79,6 +105,10 @@ func TestWorkspaceAppCors(t *testing.T) { r.Header.Set("Access-Control-Request-Method", method) } + handler := httpmw.WorkspaceAppCors(regex, test.app)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusNoContent) + })) + handler.ServeHTTP(rw, r) if test.allowed { From a0f2eb828b0c125c59e9aad1d5a5db3c6d1c8577 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 7 Jun 2023 10:44:54 -0800 Subject: [PATCH 7/7] Recover accidentally removed newline I think it was lost when I was resolving a conflict here. --- coderd/coderd.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/coderd.go b/coderd/coderd.go index 3abb9ea4eec44..f1aa14e145eac 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -406,6 +406,7 @@ func New(options *Options) *API { derpHandler, api.derpCloseFunc = tailnet.WithWebsocketSupport(api.DERPServer, derpHandler) cors := httpmw.Cors(options.DeploymentValues.Dangerous.AllowAllCors.Value()) prometheusMW := httpmw.Prometheus(options.PrometheusRegistry) + r.Use( httpmw.Recover(api.Logger), tracing.StatusWriterMiddleware,