diff --git a/coderd/workspaceapps/db.go b/coderd/workspaceapps/db.go index 5f15b52d0e..9b196a4b74 100644 --- a/coderd/workspaceapps/db.go +++ b/coderd/workspaceapps/db.go @@ -10,6 +10,7 @@ import ( "strings" "time" + "golang.org/x/exp/slices" "golang.org/x/xerrors" "cdr.dev/slog" @@ -100,7 +101,7 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r * // Lookup workspace app details from DB. dbReq, err := appReq.getDatabase(dangerousSystemCtx, p.Database) if xerrors.Is(err, sql.ErrNoRows) { - WriteWorkspaceApp404(p.Logger, p.DashboardURL, rw, r, &appReq, err.Error()) + WriteWorkspaceApp404(p.Logger, p.DashboardURL, rw, r, &appReq, nil, err.Error()) return nil, "", false } else if err != nil { WriteWorkspaceApp500(p.Logger, p.DashboardURL, rw, r, &appReq, err, "get app details from database") @@ -114,7 +115,7 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r * } // Verify the user has access to the app. - authed, err := p.authorizeRequest(r.Context(), authz, dbReq) + authed, warnings, err := p.authorizeRequest(r.Context(), authz, dbReq) if err != nil { WriteWorkspaceApp500(p.Logger, p.DashboardURL, rw, r, &appReq, err, "verify authz") return nil, "", false @@ -122,7 +123,7 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r * if !authed { if apiKey != nil { // The request has a valid API key but insufficient permissions. - WriteWorkspaceApp404(p.Logger, p.DashboardURL, rw, r, &appReq, "insufficient permissions") + WriteWorkspaceApp404(p.Logger, p.DashboardURL, rw, r, &appReq, warnings, "insufficient permissions") return nil, "", false } @@ -218,7 +219,12 @@ func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r * return &token, tokenStr, true } -func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Authorization, dbReq *databaseRequest) (bool, error) { +// authorizeRequest returns true/false if the request is authorized. The returned []string +// are warnings that aid in debugging. These messages do not prevent authorization, +// but may indicate that the request is not configured correctly. +// If an error is returned, the request should be aborted with a 500 error. +func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Authorization, dbReq *databaseRequest) (bool, []string, error) { + var warnings []string accessMethod := dbReq.AccessMethod if accessMethod == "" { accessMethod = AccessMethodPath @@ -233,6 +239,14 @@ func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Au // Dangerous.AllowPathAppSiteOwnerAccess flag is enabled in the check below. sharingLevel := dbReq.AppSharingLevel if isPathApp && !p.DeploymentValues.Dangerous.AllowPathAppSharing.Value() { + if dbReq.AppSharingLevel != database.AppSharingLevelOwner { + // This is helpful for debugging, and ok to leak to the user. + // This is because the app has the sharing level set to something that + // should be shared, but we are disabling it from a deployment wide + // flag. So the template should be fixed to set the sharing level to + // "owner" instead and this will not appear. + warnings = append(warnings, fmt.Sprintf("unable to use configured sharing level %q because path-based app sharing is disabled (see --dangerous-allow-path-app-sharing), using sharing level \"owner\" instead", sharingLevel)) + } sharingLevel = database.AppSharingLevelOwner } @@ -240,7 +254,7 @@ func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Au if roles == nil { // The user is not authenticated, so they can only access the app if it // is public. - return sharingLevel == database.AppSharingLevelPublic, nil + return sharingLevel == database.AppSharingLevelPublic, warnings, nil } // Block anyone from accessing workspaces they don't own in path-based apps @@ -254,7 +268,13 @@ func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Au sharingLevel == database.AppSharingLevelOwner && dbReq.Workspace.OwnerID.String() != roles.Actor.ID && !p.DeploymentValues.Dangerous.AllowPathAppSiteOwnerAccess.Value() { - return false, nil + // This is not ideal to check for the 'owner' role, but we are only checking + // to determine whether to show a warning for debugging reasons. This does + // not do any authz checks, so it is ok. + if roles != nil && slices.Contains(roles.Actor.Roles.Names(), rbac.RoleOwner()) { + warnings = append(warnings, "path-based apps with \"owner\" share level are only accessible by the workspace owner (see --dangerous-allow-path-app-site-owner-access)") + } + return false, warnings, nil } // Figure out which RBAC resource to check. For terminals we use execution @@ -280,7 +300,7 @@ func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Au // scope allows it). err := p.Authorizer.Authorize(ctx, roles.Actor, rbacAction, rbacResource) if err == nil { - return true, nil + return true, []string{}, nil } switch sharingLevel { @@ -293,15 +313,15 @@ func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *httpmw.Au // to connect to the actor's own workspace. This enforces scopes. err := p.Authorizer.Authorize(ctx, roles.Actor, rbacAction, rbacResourceOwned) if err == nil { - return true, nil + return true, []string{}, nil } case database.AppSharingLevelPublic: // We don't really care about scopes and stuff if it's public anyways. // Someone with a restricted-scope API key could just not submit the API // key cookie in the request and access the page. - return true, nil + return true, []string{}, nil } // No checks were successful. - return false, nil + return false, warnings, nil } diff --git a/coderd/workspaceapps/errors.go b/coderd/workspaceapps/errors.go index 0dd8cc270d..78e5c21262 100644 --- a/coderd/workspaceapps/errors.go +++ b/coderd/workspaceapps/errors.go @@ -10,16 +10,19 @@ import ( // WriteWorkspaceApp404 writes a HTML 404 error page for a workspace app. If // appReq is not nil, it will be used to log the request details at debug level. -func WriteWorkspaceApp404(log slog.Logger, accessURL *url.URL, rw http.ResponseWriter, r *http.Request, appReq *Request, msg string) { +// +// The 'warnings' parameter is sent to the user, 'details' is only shown in the logs. +func WriteWorkspaceApp404(log slog.Logger, accessURL *url.URL, rw http.ResponseWriter, r *http.Request, appReq *Request, warnings []string, details string) { if appReq != nil { slog.Helper() log.Debug(r.Context(), - "workspace app 404: "+msg, + "workspace app 404: "+details, slog.F("username_or_id", appReq.UsernameOrID), slog.F("workspace_and_agent", appReq.WorkspaceAndAgent), slog.F("workspace_name_or_id", appReq.WorkspaceNameOrID), slog.F("agent_name_or_id", appReq.AgentNameOrID), slog.F("app_slug_or_port", appReq.AppSlugOrPort), + slog.F("warnings", warnings), ) } @@ -29,6 +32,7 @@ func WriteWorkspaceApp404(log slog.Logger, accessURL *url.URL, rw http.ResponseW Description: "The application or workspace you are trying to access does not exist or you do not have permission to access it.", RetryEnabled: false, DashboardURL: accessURL.String(), + Warnings: warnings, }) } diff --git a/site/site.go b/site/site.go index 60113e0c71..f434a7b878 100644 --- a/site/site.go +++ b/site/site.go @@ -754,6 +754,7 @@ type ErrorPageData struct { Description string RetryEnabled bool DashboardURL string + Warnings []string } // RenderStaticErrorPage renders the static error page. This is used by app diff --git a/site/static/error.html b/site/static/error.html index 6fa05323b4..46c9a1b9e7 100644 --- a/site/static/error.html +++ b/site/static/error.html @@ -38,7 +38,7 @@ running). */}} text-align: center; } - svg { + .coder-svg { width: 80px; margin-bottom: 24px; } @@ -49,11 +49,18 @@ running). */}} margin-bottom: 8px; } - p { + p, + li { color: #b2bfd7; line-height: 140%; } + .warning li { + text-align: left; + padding-top: 10px; + margin-left: 30px; + } + .button-group { display: flex; align-items: center; @@ -83,11 +90,41 @@ running). */}} .button-group button:hover { border-color: hsl(222, 31%, 40%); } + + .warning { + margin-top: 24px; + border: 1px solid rgb(243, 140, 89); + background: rgb(13, 19, 33); + width: calc(520px + var(--side-padding) * 2); + /* Recenter */ + margin-left: calc(-1 * (100px + var(--side-padding))); + padding: 10px; + } + + .warning-title { + display: inline-flex; + align-self: center; + align-items: center; + } + + .svg-icon svg { + height: 1em; + width: 1em; + } + + .warning-title h3 { + margin-left: 10px; + }
- +

{{ .Error.Description }}

+ {{- if .Error.Warnings }} +
+
+ +

Warnings

+
+ +
+ {{ end }}
{{- if .Error.RetryEnabled }}