-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*Permission.get_scopes
: don't tolerate unknown actions
#8426
Conversation
With almost all of the `get_scopes` methods, an unknown (action, method) combination will result in an array like `[None]` being returned (sometimes with other elements as well). If that happens, the OPA input will then have `"scope": null`, and so the policy evaluation will fail, unless the user is an admin. Because of this, it's really easy to accidentally make a view admin-only, by forgetting to add/update an entry in `get_scopes` when making changes. `TaskPermission`, `MembershipPermission` and `WebhookPermission` are even worse, because they will just return an empty list of scopes, which will later translate to an empty list of permissions, which means that everyone will be permitted to perform the action. This can lead to vulnerabilities like CVE-2024-45393. Fix this by replacing all `.get` calls with indexing, which will cause a crash if the (action, method) combo is unknown. This breaks one endpoint (`/api/webhooks/events`), which is supposed to be publicly accessible; fix that by disabling authorization for it.
WalkthroughThe pull request introduces changes across multiple files in the CVAT codebase, primarily focusing on the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Permissions
Client->>Server: Request action
Server->>Permissions: Check action validity
alt Action valid
Permissions-->>Server: Return scopes
Server-->>Client: Respond with data
else Action invalid
Permissions-->>Server: Raise KeyError
Server-->>Client: Respond with error
end
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (9)
Additional comments not posted (20)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Quality Gate failedFailed conditions |
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.
LGTM
With almost all of the `get_scopes` methods, an unknown (action, method) combination will result in an array like `[None]` being returned (sometimes with other elements as well). If that happens, the OPA input will then have `"scope": null`, and so the policy evaluation will fail, unless the user is an admin. Because of this, it's really easy to accidentally make a view admin-only, by forgetting to add/update an entry in `get_scopes` when making changes. `TaskPermission`, `MembershipPermission` and `WebhookPermission` are even worse, because they will just return an empty list of scopes, which will later translate to an empty list of permissions, which means that everyone will be permitted to perform the action. This can lead to vulnerabilities like CVE-2024-45393. Fix this by replacing all `.get` calls with indexing, which will cause a crash if the (action, method) combo is unknown. This breaks one endpoint (`/api/webhooks/events`), which is supposed to be publicly accessible; fix that by disabling authorization for it.
Motivation and context
With almost all of the
get_scopes
methods, an unknown (action, method) combination will result in an array like[None]
being returned (sometimes with other elements as well). If that happens, the OPA input will then have"scope": null
, and so the policy evaluation will fail, unless the user is an admin.Because of this, it's really easy to accidentally make a view admin-only, by forgetting to add/update an entry in
get_scopes
when making changes.TaskPermission
,MembershipPermission
andWebhookPermission
are even worse, because they will just return an empty list of scopes, which will later translate to an empty list of permissions, which means that everyone will be permitted to perform the action. This can lead to vulnerabilities like CVE-2024-45393.Fix this by replacing all
.get
calls with indexing, which will cause a crash if the (action, method) combo is unknown. This breaks one endpoint (/api/webhooks/events
), which is supposed to be publicly accessible; fix that by disabling authorization for it.How has this been tested?
Checklist
develop
branch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
events
endpoint to allow unrestricted access by modifying permission requirements.Bug Fixes
Refactor