diff --git a/coderd/coderd.go b/coderd/coderd.go index 0d5f47b8618c7..f1aa14e145eac 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -408,7 +408,6 @@ func New(options *Options) *API { prometheusMW := httpmw.Prometheus(options.PrometheusRegistry) r.Use( - cors, httpmw.Recover(api.Logger), tracing.StatusWriterMiddleware, tracing.Middleware(api.TracerProvider), @@ -419,9 +418,10 @@ 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. api.workspaceAppServer.HandleSubdomain(apiRateLimiter), + 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/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/httpmw/cors_test.go b/coderd/httpmw/cors_test.go new file mode 100644 index 0000000000000..7668771b1e6db --- /dev/null +++ b/coderd/httpmw/cors_test.go @@ -0,0 +1,130 @@ +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) + + methods := []string{ + http.MethodOptions, + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + } + + tests := []struct { + name string + origin string + app httpapi.ApplicationURL + allowed bool + }{ + { + 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", + 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", + app: httpapi.ApplicationURL{ + AppSlugOrPort: "3000", + AgentName: "agent", + WorkspaceName: "ws", + Username: "user", + }, + allowed: true, + }, + { + 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, + }, + } + + 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() + + // Preflight requests need to know what method will be requested. + if method == http.MethodOptions { + 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 { + 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) + } + } + }) + } +} diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 701bf296cc765..331934be6a4d0 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -361,35 +361,34 @@ 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 - } - - // 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, httpmw.WorkspaceAppCors(s.HostnameRegex, app))) 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)) }) 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