- Notifications
You must be signed in to change notification settings - Fork1k
fix(security)!: path-based app sharing changes#5772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This commit disables path-based app sharing by default. It is possiblefor a workspace app on a path (not a subdomain) to make API requests tothe Coder API. When accessing your own workspace, this is not much of aproblem. When accessing a shared workspace app, the workspace ownercould include malicious javascript in the page that makes requests tothe 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.
cli/deployment/config.go Outdated
| }, | ||
| DisablePathApps:&codersdk.DeploymentConfigField[bool]{ | ||
| Name:"Disable Path Apps", | ||
| 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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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 maliciousJavascript. This is recommended for security purposes if a --wildcard-access-url is configured.", | |
| 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 maliciousJavaScript. This is recommended for security purposes if a --wildcard-access-url is configured.", |
Should this be default if--wildcard-access-url is enabled? I think it'd be preferable if we were secure-by-default in most configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we could make that change in the future, but the other changes in this PR of disabling sharing for path-based apps will mitigate this problem in almost all cases.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I approve the way we interact with authz.
Unfortunately to do this entirely in authz RBAC checks would require dynamic permissions , scopes, or roles.
Dynamic roles is not something we should really get into prematurely, so doing a static check if theworkspace.OwnerID == roles.ID is just something we have to live with right now.
Another opther option looked into was making a new resource for Path based apps, but still has the issue of making dynamic permissions based on mutable flags 😢. If we did this, we could implement the permissions such that there is no security vulnerability (by not giving perms for path based apps), but then users cannot share path based apps which is a great feature for small deployments and demos 🤷.
UX vs security is always a fun battle.
👍
Uh oh!
There was an error while loading.Please reload this page.
| typeDangerousConfigstruct { | ||
| AllowPathAppSharing*DeploymentConfigField[bool]`json:"allow_path_app_sharing" typescript:",notnull"` | ||
| AllowPathAppSiteOwnerAccess*DeploymentConfigField[bool]`json:"allow_path_app_site_owner_access" typescript:",notnull"` | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it would be nice to include the dangerous ratelimits flag in this config.
Uh oh!
There was an error while loading.Please reload this page.
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-sharingflag 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-accessflag which is not recommended.Adds a flag
--disable-path-appswhich 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.Note: After this is merged we will immediately release a minor release.
Updating the tests required a lot of massaging, since the workspaces were being made by the site owner in almost all of them.
https://github.com/coder/security/issues/3