Skip to content

Commit

Permalink
Merge commit from fork
Browse files Browse the repository at this point in the history
Due to how `WebhookPermission.get_scopes` is written, any unhandled endpoint
will result in an empty list being returned, and thus no access control
being performed. This is the right thing for `/api/webhooks/events`, but not
so much for every other endpoint.
  • Loading branch information
SpecLad authored Sep 10, 2024
1 parent 88ce0a4 commit 0fafb79
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Security

- Fixed a missing authorization vulnerability in webhook delivery endpoints
(<https://github.com/cvat-ai/cvat/security/advisories/GHSA-p3c9-m7jr-jxxj>)
4 changes: 4 additions & 0 deletions cvat/apps/webhooks/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ def get_scopes(request, view, obj):
('update', 'PUT'): Scopes.UPDATE,
('list', 'GET'): Scopes.LIST,
('retrieve', 'GET'): Scopes.VIEW,
('ping', 'POST'): Scopes.UPDATE,
('deliveries', 'GET'): Scopes.VIEW,
('retrieve_delivery', 'GET'): Scopes.VIEW,
('redelivery', 'POST'): Scopes.UPDATE,
}.get((view.action, request.method))

scopes = []
Expand Down
64 changes: 64 additions & 0 deletions tests/python/rest_api/test_webhooks_sender.py
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,34 @@ def test_webhook_create_and_delete_comment(self, issues, jobs, tasks):
)


@pytest.mark.usefixtures("restore_db_per_class")
class TestGetWebhookDeliveries:
def test_not_project_staff_cannot_get_webhook(self, projects, users):
user, project = next(
(user, project)
for user in users
if "user" in user["groups"]
for project in projects
if project["owner"]["id"] != user["id"]
)

webhook = create_webhook(["create:task"], "project", project_id=project["id"])
owner = next(user for user in users if user["id"] == project["owner"]["id"])

response = post_method(owner["username"], f"webhooks/{webhook['id']}/ping", {})
assert response.status_code == HTTPStatus.OK

delivery_id = response.json()["id"]

response = get_method(user["username"], f"webhooks/{webhook['id']}/deliveries")
assert response.status_code == HTTPStatus.FORBIDDEN

response = get_method(
user["username"], f"webhooks/{webhook['id']}/deliveries/{delivery_id}"
)
assert response.status_code == HTTPStatus.FORBIDDEN


@pytest.mark.usefixtures("restore_db_per_function")
class TestWebhookPing:
def test_ping_webhook(self, projects):
Expand All @@ -680,6 +708,20 @@ def test_ping_webhook(self, projects):
== {}
)

def test_not_project_staff_cannot_ping(self, projects, users):
user, project = next(
(user, project)
for user in users
if "user" in user["groups"]
for project in projects
if project["owner"]["id"] != user["id"]
)

webhook = create_webhook(["create:task"], "project", project_id=project["id"])

response = post_method(user["username"], f"webhooks/{webhook['id']}/ping", {})
assert response.status_code == HTTPStatus.FORBIDDEN


@pytest.mark.usefixtures("restore_db_per_function")
class TestWebhookRedelivery:
Expand Down Expand Up @@ -727,3 +769,25 @@ def test_webhook_redelivery(self, projects):
)
== {}
)

def test_not_project_staff_cannot_redeliver(self, projects, users):
user, project = next(
(user, project)
for user in users
if "user" in user["groups"]
for project in projects
if project["owner"]["id"] != user["id"]
)

webhook = create_webhook(["create:task"], "project", project_id=project["id"])
owner = next(user for user in users if user["id"] == project["owner"]["id"])

response = post_method(owner["username"], f"webhooks/{webhook['id']}/ping", {})
assert response.status_code == HTTPStatus.OK

delivery_id = response.json()["id"]

response = post_method(
user["username"], f"webhooks/{webhook['id']}/deliveries/{delivery_id}/redelivery", {}
)
assert response.status_code == HTTPStatus.FORBIDDEN

0 comments on commit 0fafb79

Please sign in to comment.