Skip to content

Commit 0374af2

Browse files
deansheatherdeansheather
authored
fix(security)!: path-based app sharing changes (#5772)
This commit disables path-based app sharing by default. It is possible for a workspace app on a path (not a subdomain) to make API requests to the Coder API. When accessing your own workspace, this is not much of a problem. When accessing a shared workspace app, the workspace owner could include malicious javascript in the page that makes requests to the Coder API on behalf of the visitor. This vulnerability does not affect subdomain apps. - Disables path-based app sharing by default. Previous behavior can be restored using the `--dangerous-allow-path-app-sharing` flag which is not recommended. - Disables users with the site "owner" role from accessing path-based apps from workspaces they do not own. Previous behavior can be restored using the `--dangerous-allow-path-app-site-owner-access` flag which is not recommended. - Adds a flag `--disable-path-apps` which can be used by security-conscious admins to disable all path-based apps across the entire deployment. This check is enforced at app-access time, not at template-ingest time.
1 parent b42e2ae commit 0374af2

File tree

10 files changed

+506
-78
lines changed

10 files changed

+506
-78
lines changed

‎cli/deployment/config.go

+20
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,26 @@ func newConfig() *codersdk.DeploymentConfig {
500500
Default: "",
501501
},
502502
},
503+
Dangerous: &codersdk.DangerousConfig{
504+
AllowPathAppSharing: &codersdk.DeploymentConfigField[bool]{
505+
Name: "DANGEROUS: Allow Path App Sharing",
506+
Usage: "Allow workspace apps that are not served from subdomains to be shared. Path-based app sharing is DISABLED by default for security purposes. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. Path-based apps can be disabled entirely with --disable-path-apps for further security.",
507+
Flag: "dangerous-allow-path-app-sharing",
508+
Default: false,
509+
},
510+
AllowPathAppSiteOwnerAccess: &codersdk.DeploymentConfigField[bool]{
511+
Name: "DANGEROUS: Allow Site Owners to Access Path Apps",
512+
Usage: "Allow site-owners to access workspace apps from workspaces they do not own. Owners cannot access path-based apps they do not own by default. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. Path-based apps can be disabled entirely with --disable-path-apps for further security.",
513+
Flag: "dangerous-allow-path-app-site-owner-access",
514+
Default: false,
515+
},
516+
},
517+
DisablePathApps: &codersdk.DeploymentConfigField[bool]{
518+
Name: "Disable Path Apps",
519+
Usage: "Disable workspace apps that are not served from subdomains. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. This is recommended for security purposes if a --wildcard-access-url is configured.",
520+
Flag: "disable-path-apps",
521+
Default: false,
522+
},
503523
}
504524
}
505525

‎cli/testdata/coder_server_--help.golden

+30
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,28 @@ Flags:
2929
with systemd.
3030
Consumes $CODER_CACHE_DIRECTORY (default
3131
"/tmp/coder-cli-test-cache")
32+
--dangerous-allow-path-app-sharing Allow workspace apps that are not served
33+
from subdomains to be shared. Path-based
34+
app sharing is DISABLED by default for
35+
security purposes. Path-based apps can
36+
make requests to the Coder API and pose a
37+
security risk when the workspace serves
38+
malicious JavaScript. Path-based apps can
39+
be disabled entirely with
40+
--disable-path-apps for further security.
41+
Consumes
42+
$CODER_DANGEROUS_ALLOW_PATH_APP_SHARING
43+
--dangerous-allow-path-app-site-owner-access Allow site-owners to access workspace
44+
apps from workspaces they do not own.
45+
Owners cannot access path-based apps they
46+
do not own by default. Path-based apps
47+
can make requests to the Coder API and
48+
pose a security risk when the workspace
49+
serves malicious JavaScript. Path-based
50+
apps can be disabled entirely with
51+
--disable-path-apps for further security.
52+
Consumes
53+
$CODER_DANGEROUS_ALLOW_PATH_APP_SITE_OWNER_ACCESS
3254
--dangerous-disable-rate-limits Disables all rate limits. This is not
3355
recommended in production.
3456
Consumes $CODER_RATE_LIMIT_DISABLE_ALL
@@ -61,6 +83,14 @@ Flags:
6183
Consumes
6284
$CODER_DERP_SERVER_STUN_ADDRESSES
6385
(default [stun.l.google.com:19302])
86+
--disable-path-apps Disable workspace apps that are not
87+
served from subdomains. Path-based apps
88+
can make requests to the Coder API and
89+
pose a security risk when the workspace
90+
serves malicious JavaScript. This is
91+
recommended for security purposes if a
92+
--wildcard-access-url is configured.
93+
Consumes $CODER_DISABLE_PATH_APPS
6494
--experiments strings Enable one or more experiments. These are
6595
not ready for production. Separate
6696
multiple experiments with commas, or

‎coderd/apidoc/docs.go

+17
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

+17
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/workspaceapps.go

+72-20
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,13 @@ var nonCanonicalHeaders = map[string]string{
6565
"Sec-Websocket-Version": "Sec-WebSocket-Version",
6666
}
6767

68+
type workspaceAppAccessMethod string
69+
70+
const (
71+
workspaceAppAccessMethodPath workspaceAppAccessMethod = "path"
72+
workspaceAppAccessMethodSubdomain workspaceAppAccessMethod = "subdomain"
73+
)
74+
6875
// @Summary Get applications host
6976
// @ID get-applications-host
7077
// @Security CoderSessionToken
@@ -89,6 +96,17 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
8996
workspace := httpmw.WorkspaceParam(r)
9097
agent := httpmw.WorkspaceAgentParam(r)
9198

99+
if api.DeploymentConfig.DisablePathApps.Value {
100+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
101+
Status: http.StatusUnauthorized,
102+
Title: "Unauthorized",
103+
Description: "Path-based applications are disabled on this Coder deployment by the administrator.",
104+
RetryEnabled: false,
105+
DashboardURL: api.AccessURL.String(),
106+
})
107+
return
108+
}
109+
92110
// We do not support port proxying on paths, so lookup the app by slug.
93111
appSlug := chi.URLParam(r, "workspaceapp")
94112
app, ok := api.lookupWorkspaceApp(rw, r, agent.ID, appSlug)
@@ -100,7 +118,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
100118
if app.SharingLevel != "" {
101119
appSharingLevel = app.SharingLevel
102120
}
103-
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspace, appSharingLevel)
121+
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspaceAppAccessMethodPath, workspace, appSharingLevel)
104122
if !ok {
105123
return
106124
}
@@ -127,11 +145,12 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
127145
}
128146

129147
api.proxyWorkspaceApplication(proxyApplication{
130-
Workspace: workspace,
131-
Agent: agent,
132-
App: &app,
133-
Port: 0,
134-
Path: chiPath,
148+
AccessMethod: workspaceAppAccessMethodPath,
149+
Workspace: workspace,
150+
Agent: agent,
151+
App: &app,
152+
Port: 0,
153+
Path: chiPath,
135154
}, rw, r)
136155
}
137156

@@ -238,11 +257,12 @@ func (api *API) handleSubdomainApplications(middlewares ...func(http.Handler) ht
238257
}
239258

240259
api.proxyWorkspaceApplication(proxyApplication{
241-
Workspace: workspace,
242-
Agent: agent,
243-
App: workspaceAppPtr,
244-
Port: app.Port,
245-
Path: r.URL.Path,
260+
AccessMethod: workspaceAppAccessMethodSubdomain,
261+
Workspace: workspace,
262+
Agent: agent,
263+
App: workspaceAppPtr,
264+
Port: app.Port,
265+
Path: r.URL.Path,
246266
}, rw, r)
247267
})).ServeHTTP(rw, r.WithContext(ctx))
248268
})
@@ -411,9 +431,25 @@ func (api *API) lookupWorkspaceApp(rw http.ResponseWriter, r *http.Request, agen
411431
return app, true
412432
}
413433

414-
func (api *API) authorizeWorkspaceApp(r *http.Request, sharingLevel database.AppSharingLevel, workspace database.Workspace) (bool, error) {
434+
//nolint:revive
435+
func (api *API) authorizeWorkspaceApp(r *http.Request, accessMethod workspaceAppAccessMethod, sharingLevel database.AppSharingLevel, workspace database.Workspace) (bool, error) {
415436
ctx := r.Context()
416437

438+
if accessMethod == "" {
439+
accessMethod = workspaceAppAccessMethodPath
440+
}
441+
isPathApp := accessMethod == workspaceAppAccessMethodPath
442+
443+
// If path-based app sharing is disabled (which is the default), we can
444+
// force the sharing level to be "owner" so that the user can only access
445+
// their own apps.
446+
//
447+
// Site owners are blocked from accessing path-based apps unless the
448+
// Dangerous.AllowPathAppSiteOwnerAccess flag is enabled in the check below.
449+
if isPathApp && !api.DeploymentConfig.Dangerous.AllowPathAppSharing.Value {
450+
sharingLevel = database.AppSharingLevelOwner
451+
}
452+
417453
// Short circuit if not authenticated.
418454
roles, ok := httpmw.UserAuthorizationOptional(r)
419455
if !ok {
@@ -422,6 +458,21 @@ func (api *API) authorizeWorkspaceApp(r *http.Request, sharingLevel database.App
422458
return sharingLevel == database.AppSharingLevelPublic, nil
423459
}
424460

461+
// Block anyone from accessing workspaces they don't own in path-based apps
462+
// unless the admin disables this security feature. This blocks site-owners
463+
// from accessing any apps from any user's workspaces.
464+
//
465+
// When the Dangerous.AllowPathAppSharing flag is not enabled, the sharing
466+
// level will be forced to "owner", so this check will always be true for
467+
// workspaces owned by different users.
468+
if isPathApp &&
469+
sharingLevel == database.AppSharingLevelOwner &&
470+
workspace.OwnerID != roles.ID &&
471+
!api.DeploymentConfig.Dangerous.AllowPathAppSiteOwnerAccess.Value {
472+
473+
return false, nil
474+
}
475+
425476
// Do a standard RBAC check. This accounts for share level "owner" and any
426477
// other RBAC rules that may be in place.
427478
//
@@ -463,8 +514,8 @@ func (api *API) authorizeWorkspaceApp(r *http.Request, sharingLevel database.App
463514
// for a given app share level in the given workspace. The user's authorization
464515
// status is returned. If a server error occurs, a HTML error page is rendered
465516
// and false is returned so the caller can return early.
466-
func (api *API) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, workspace database.Workspace, appSharingLevel database.AppSharingLevel) (authed bool, ok bool) {
467-
ok, err := api.authorizeWorkspaceApp(r, appSharingLevel, workspace)
517+
func (api *API) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, accessMethod workspaceAppAccessMethod, workspace database.Workspace, appSharingLevel database.AppSharingLevel) (authed bool, ok bool) {
518+
ok, err := api.authorizeWorkspaceApp(r, accessMethod, appSharingLevel, workspace)
468519
if err != nil {
469520
api.Logger.Error(r.Context(), "authorize workspace app", slog.Error(err))
470521
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
@@ -484,8 +535,8 @@ func (api *API) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Re
484535
// for a given app share level in the given workspace. If the user is not
485536
// authorized or a server error occurs, a discrete HTML error page is rendered
486537
// and false is returned so the caller can return early.
487-
func (api *API) checkWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, workspace database.Workspace, appSharingLevel database.AppSharingLevel) bool {
488-
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspace, appSharingLevel)
538+
func (api *API) checkWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, accessMethod workspaceAppAccessMethod, workspace database.Workspace, appSharingLevel database.AppSharingLevel) bool {
539+
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, accessMethod, workspace, appSharingLevel)
489540
if !ok {
490541
return false
491542
}
@@ -502,7 +553,7 @@ func (api *API) checkWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Re
502553
// they will be redirected to the route below. If the user does have a session
503554
// key but insufficient permissions a static error page will be rendered.
504555
func (api *API) verifyWorkspaceApplicationSubdomainAuth(rw http.ResponseWriter, r *http.Request, host string, workspace database.Workspace, appSharingLevel database.AppSharingLevel) bool {
505-
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspace, appSharingLevel)
556+
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspaceAppAccessMethodSubdomain, workspace, appSharingLevel)
506557
if !ok {
507558
return false
508559
}
@@ -731,8 +782,9 @@ func (api *API) workspaceApplicationAuth(rw http.ResponseWriter, r *http.Request
731782

732783
// proxyApplication are the required fields to proxy a workspace application.
733784
type proxyApplication struct {
734-
Workspace database.Workspace
735-
Agent database.WorkspaceAgent
785+
AccessMethod workspaceAppAccessMethod
786+
Workspace database.Workspace
787+
Agent database.WorkspaceAgent
736788

737789
// Either App or Port must be set, but not both.
738790
App *database.WorkspaceApp
@@ -752,7 +804,7 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
752804
if proxyApp.App != nil && proxyApp.App.SharingLevel != "" {
753805
sharingLevel = proxyApp.App.SharingLevel
754806
}
755-
if !api.checkWorkspaceApplicationAuth(rw, r, proxyApp.Workspace, sharingLevel) {
807+
if !api.checkWorkspaceApplicationAuth(rw, r, proxyApp.AccessMethod, proxyApp.Workspace, sharingLevel) {
756808
return
757809
}
758810

0 commit comments

Comments
 (0)