Skip to content

Commit f0c5201

Browse files
code-ashercode-asher
authored
feat: allow cross-origin requests between users' own apps (coder#7688)
1 parent 125e9ef commit f0c5201

File tree

5 files changed

+195
-32
lines changed

5 files changed

+195
-32
lines changed

‎coderd/coderd.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,6 @@ func New(options *Options) *API {
408408
prometheusMW := httpmw.Prometheus(options.PrometheusRegistry)
409409

410410
r.Use(
411-
cors,
412411
httpmw.Recover(api.Logger),
413412
tracing.StatusWriterMiddleware,
414413
tracing.Middleware(api.TracerProvider),
@@ -419,9 +418,10 @@ func New(options *Options) *API {
419418
// SubdomainAppMW checks if the first subdomain is a valid app URL. If
420419
// it is, it will serve that application.
421420
//
422-
// Workspace apps do their own auth and must be BEFORE the auth
423-
// middleware.
421+
// Workspace apps do their own auth and CORS and must be BEFORE the auth
422+
// and CORS middleware.
424423
api.workspaceAppServer.HandleSubdomain(apiRateLimiter),
424+
cors,
425425
// Build-Version is helpful for debugging.
426426
func(next http.Handler) http.Handler {
427427
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

‎coderd/httpmw/cors.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,12 @@ package httpmw
22

33
import (
44
"net/http"
5+
"net/url"
6+
"regexp"
57

68
"github.com/go-chi/cors"
9+
10+
"github.com/coder/coder/coderd/httpapi"
711
)
812

913
//nolint:revive
@@ -25,3 +29,33 @@ func Cors(allowAll bool, origins ...string) func(next http.Handler) http.Handler
2529
AllowCredentials: false,
2630
})
2731
}
32+
33+
func WorkspaceAppCors(regex *regexp.Regexp, app httpapi.ApplicationURL) func(next http.Handler) http.Handler {
34+
return cors.Handler(cors.Options{
35+
AllowOriginFunc: func(r *http.Request, rawOrigin string) bool {
36+
origin, err := url.Parse(rawOrigin)
37+
if rawOrigin == "" || origin.Host == "" || err != nil {
38+
return false
39+
}
40+
subdomain, ok := httpapi.ExecuteHostnamePattern(regex, origin.Host)
41+
if !ok {
42+
return false
43+
}
44+
originApp, err := httpapi.ParseSubdomainAppURL(subdomain)
45+
if err != nil {
46+
return false
47+
}
48+
return ok && originApp.Username == app.Username
49+
},
50+
AllowedMethods: []string{
51+
http.MethodHead,
52+
http.MethodGet,
53+
http.MethodPost,
54+
http.MethodPut,
55+
http.MethodPatch,
56+
http.MethodDelete,
57+
},
58+
AllowedHeaders: []string{"*"},
59+
AllowCredentials: true,
60+
})
61+
}

‎coderd/httpmw/cors_test.go

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package httpmw_test
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"testing"
7+
8+
"github.com/stretchr/testify/require"
9+
10+
"github.com/coder/coder/coderd/httpapi"
11+
"github.com/coder/coder/coderd/httpmw"
12+
)
13+
14+
func TestWorkspaceAppCors(t *testing.T) {
15+
t.Parallel()
16+
17+
regex, err := httpapi.CompileHostnamePattern("*--apps.dev.coder.com")
18+
require.NoError(t, err)
19+
20+
methods := []string{
21+
http.MethodOptions,
22+
http.MethodHead,
23+
http.MethodGet,
24+
http.MethodPost,
25+
http.MethodPut,
26+
http.MethodPatch,
27+
http.MethodDelete,
28+
}
29+
30+
tests := []struct {
31+
name string
32+
origin string
33+
app httpapi.ApplicationURL
34+
allowed bool
35+
}{
36+
{
37+
name: "Self",
38+
origin: "https://3000--agent--ws--user--apps.dev.coder.com",
39+
app: httpapi.ApplicationURL{
40+
AppSlugOrPort: "3000",
41+
AgentName: "agent",
42+
WorkspaceName: "ws",
43+
Username: "user",
44+
},
45+
allowed: true,
46+
},
47+
{
48+
name: "SameWorkspace",
49+
origin: "https://8000--agent--ws--user--apps.dev.coder.com",
50+
app: httpapi.ApplicationURL{
51+
AppSlugOrPort: "3000",
52+
AgentName: "agent",
53+
WorkspaceName: "ws",
54+
Username: "user",
55+
},
56+
allowed: true,
57+
},
58+
{
59+
name: "SameUser",
60+
origin: "https://8000--agent2--ws2--user--apps.dev.coder.com",
61+
app: httpapi.ApplicationURL{
62+
AppSlugOrPort: "3000",
63+
AgentName: "agent",
64+
WorkspaceName: "ws",
65+
Username: "user",
66+
},
67+
allowed: true,
68+
},
69+
{
70+
name: "DifferentOriginOwner",
71+
origin: "https://3000--agent--ws--user2--apps.dev.coder.com",
72+
app: httpapi.ApplicationURL{
73+
AppSlugOrPort: "3000",
74+
AgentName: "agent",
75+
WorkspaceName: "ws",
76+
Username: "user",
77+
},
78+
allowed: false,
79+
},
80+
{
81+
name: "DifferentHostOwner",
82+
origin: "https://3000--agent--ws--user--apps.dev.coder.com",
83+
app: httpapi.ApplicationURL{
84+
AppSlugOrPort: "3000",
85+
AgentName: "agent",
86+
WorkspaceName: "ws",
87+
Username: "user2",
88+
},
89+
allowed: false,
90+
},
91+
}
92+
93+
for _, test := range tests {
94+
test := test
95+
t.Run(test.name, func(t *testing.T) {
96+
t.Parallel()
97+
98+
for _, method := range methods {
99+
r := httptest.NewRequest(method, "http://localhost", nil)
100+
r.Header.Set("Origin", test.origin)
101+
rw := httptest.NewRecorder()
102+
103+
// Preflight requests need to know what method will be requested.
104+
if method == http.MethodOptions {
105+
r.Header.Set("Access-Control-Request-Method", method)
106+
}
107+
108+
handler := httpmw.WorkspaceAppCors(regex, test.app)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
109+
rw.WriteHeader(http.StatusNoContent)
110+
}))
111+
112+
handler.ServeHTTP(rw, r)
113+
114+
if test.allowed {
115+
require.Equal(t, test.origin, rw.Header().Get("Access-Control-Allow-Origin"))
116+
} else {
117+
require.Equal(t, "", rw.Header().Get("Access-Control-Allow-Origin"))
118+
}
119+
120+
// For options we should never get to our handler as the middleware
121+
// short-circuits with a 200.
122+
if method == http.MethodOptions {
123+
require.Equal(t, http.StatusOK, rw.Code)
124+
} else {
125+
require.Equal(t, http.StatusNoContent, rw.Code)
126+
}
127+
}
128+
})
129+
}
130+
}

‎coderd/workspaceapps/proxy.go

Lines changed: 27 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -361,35 +361,34 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler)
361361
return
362362
}
363363

364-
if !s.handleAPIKeySmuggling(rw, r, AccessMethodSubdomain) {
365-
return
366-
}
367-
368-
token, ok := ResolveRequest(rw, r, ResolveRequestOptions{
369-
Logger: s.Logger,
370-
SignedTokenProvider: s.SignedTokenProvider,
371-
DashboardURL: s.DashboardURL,
372-
PathAppBaseURL: s.AccessURL,
373-
AppHostname: s.Hostname,
374-
AppRequest: Request{
375-
AccessMethod: AccessMethodSubdomain,
376-
BasePath: "/",
377-
UsernameOrID: app.Username,
378-
WorkspaceNameOrID: app.WorkspaceName,
379-
AgentNameOrID: app.AgentName,
380-
AppSlugOrPort: app.AppSlugOrPort,
381-
},
382-
AppPath: r.URL.Path,
383-
AppQuery: r.URL.RawQuery,
384-
})
385-
if !ok {
386-
return
387-
}
388-
389-
// Use the passed in app middlewares before passing to the proxy
390-
// app.
391-
mws := chi.Middlewares(middlewares)
364+
// Use the passed in app middlewares before checking authentication and
365+
// passing to the proxy app.
366+
mws := chi.Middlewares(append(middlewares, httpmw.WorkspaceAppCors(s.HostnameRegex, app)))
392367
mws.Handler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
368+
if !s.handleAPIKeySmuggling(rw, r, AccessMethodSubdomain) {
369+
return
370+
}
371+
372+
token, ok := ResolveRequest(rw, r, ResolveRequestOptions{
373+
Logger: s.Logger,
374+
SignedTokenProvider: s.SignedTokenProvider,
375+
DashboardURL: s.DashboardURL,
376+
PathAppBaseURL: s.AccessURL,
377+
AppHostname: s.Hostname,
378+
AppRequest: Request{
379+
AccessMethod: AccessMethodSubdomain,
380+
BasePath: "/",
381+
UsernameOrID: app.Username,
382+
WorkspaceNameOrID: app.WorkspaceName,
383+
AgentNameOrID: app.AgentName,
384+
AppSlugOrPort: app.AppSlugOrPort,
385+
},
386+
AppPath: r.URL.Path,
387+
AppQuery: r.URL.RawQuery,
388+
})
389+
if !ok {
390+
return
391+
}
393392
s.proxyWorkspaceApp(rw, r, *token, r.URL.Path)
394393
})).ServeHTTP(rw, r.WithContext(ctx))
395394
})

‎codersdk/deployment.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1170,7 +1170,7 @@ when required by your organization's security policy.`,
11701170
// ☢️ Dangerous settings
11711171
{
11721172
Name: "DANGEROUS: Allow all CORs requests",
1173-
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.",
1173+
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.",
11741174
Flag: "dangerous-allow-cors-requests",
11751175
Env: "CODER_DANGEROUS_ALLOW_CORS_REQUESTS",
11761176
Hidden: true, // Hidden, should only be used by yarn dev server

0 commit comments

Comments
 (0)