feat(api): manage workspace webhooks via the public token API#9398
Open
clwluvw wants to merge 1 commit into
Open
feat(api): manage workspace webhooks via the public token API#9398clwluvw wants to merge 1 commit into
clwluvw wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds workspace webhook CRUD API endpoints, a serializer with shared SSRF and domain validation, workspace-owner authorization, secret-key response filtering, conflict handling, routing, and contract tests covering delivery signatures. ChangesWorkspace webhook management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookAPIEndpoint
participant WebhookSerializer
participant Webhook
Client->>WebhookAPIEndpoint: Submit workspace webhook request
WebhookAPIEndpoint->>WebhookSerializer: Validate request data
WebhookSerializer->>Webhook: Create or update webhook
Webhook-->>WebhookAPIEndpoint: Return persisted webhook
WebhookAPIEndpoint-->>Client: Return filtered API response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/api/plane/tests/contract/api/test_webhooks.py (1)
90-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood create coverage — consider adding a test for
is_internal/versionprotection.The create tests thoroughly cover secret handling, SSRF, scheme validation, conflicts, and authorization. However, there's no test asserting that
is_internalandversionare read-only on create. Given thatsecret_keyis tested for read-only enforcement (line 116-123),is_internalandversiondeserve the same coverage — especially since they're currently writable (see the major issue flagged on the view).🧪 Suggested test
+ `@pytest.mark.django_db` + def test_create_ignores_is_internal_and_version(self, api_key_client, workspace, webhook_data): + """is_internal and version are server-managed, not client-settable.""" + payload = {**webhook_data, "is_internal": True, "version": "v2"} + response = api_key_client.post(_webhooks_url(workspace.slug), payload, format="json") + assert response.status_code == status.HTTP_201_CREATED + webhook = Webhook.objects.get(id=response.data["id"]) + assert webhook.is_internal is False + assert webhook.version == "v1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/contract/api/test_webhooks.py` around lines 90 - 159, Add a create test to TestWebhookCreateAPIEndpoint verifying that client-supplied is_internal and version values are ignored and the response/persisted webhook retains server-controlled defaults. Follow the existing test_create_ignores_client_supplied_secret pattern and assert both fields are protected.apps/api/plane/api/views/webhook.py (1)
104-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOperation ID
list_webhooksis used for both list and retrieve.The
getmethod handles both list (nopk) and retrieve (withpk) operations, but the@extend_schemaoperation_id is"list_webhooks"for both. drf-spectacular may auto-suffix duplicates, but the naming is misleading and could cause issues with OpenAPI client generators that expect unique operation IDs.Consider splitting into two operations or using a more generic operation_id like
list_or_retrieve_webhooks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/api/views/webhook.py` around lines 104 - 127, Update the `get` endpoint schema so its `operation_id` accurately represents both the list and retrieve branches, or split the schema definitions to assign distinct operation IDs for each operation. Ensure the generated OpenAPI document uses unique, semantically correct IDs without changing the existing response behavior.apps/api/plane/utils/webhook.py (1)
56-65: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisallowed-domain / allowed-host matching is case-sensitive against raw config.
hostnameis normalized to lowercase (Line 50), but the comparisons againstsettings.WEBHOOK_ALLOWED_HOSTS(Line 56) andsettings.WEBHOOK_DISALLOWED_DOMAINS(Line 64) use the raw config values. If an operator configures a disallowed domain with any uppercase (e.g.Internal.Corp), it will silently fail to matchinternal.corpand the block is bypassed. Normalizing the config values makes the guard robust to casing.🛡️ Suggested normalization
- if hostname in settings.WEBHOOK_ALLOWED_HOSTS: + allowed_hosts = {h.rstrip(".").lower() for h in settings.WEBHOOK_ALLOWED_HOSTS} + if hostname in allowed_hosts: return - disallowed_domains = list(settings.WEBHOOK_DISALLOWED_DOMAINS) + disallowed_domains = [d.rstrip(".").lower() for d in settings.WEBHOOK_DISALLOWED_DOMAINS] if request: request_host = request.get_host().split(":")[0].rstrip(".").lower() disallowed_domains.append(request_host)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/utils/webhook.py` around lines 56 - 65, Normalize WEBHOOK_ALLOWED_HOSTS and WEBHOOK_DISALLOWED_DOMAINS entries to lowercase before comparing them with the already-normalized hostname in the webhook validation logic. Preserve the existing exact-host and subdomain matching behavior while ensuring mixed-case configuration values are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/plane/api/views/webhook.py`:
- Around line 104-127: Update the `get` endpoint schema so its `operation_id`
accurately represents both the list and retrieve branches, or split the schema
definitions to assign distinct operation IDs for each operation. Ensure the
generated OpenAPI document uses unique, semantically correct IDs without
changing the existing response behavior.
In `@apps/api/plane/tests/contract/api/test_webhooks.py`:
- Around line 90-159: Add a create test to TestWebhookCreateAPIEndpoint
verifying that client-supplied is_internal and version values are ignored and
the response/persisted webhook retains server-controlled defaults. Follow the
existing test_create_ignores_client_supplied_secret pattern and assert both
fields are protected.
In `@apps/api/plane/utils/webhook.py`:
- Around line 56-65: Normalize WEBHOOK_ALLOWED_HOSTS and
WEBHOOK_DISALLOWED_DOMAINS entries to lowercase before comparing them with the
already-normalized hostname in the webhook validation logic. Preserve the
existing exact-host and subdomain matching behavior while ensuring mixed-case
configuration values are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bde45a2d-257f-4f65-ac93-e6b17e981214
📒 Files selected for processing (9)
apps/api/plane/api/serializers/__init__.pyapps/api/plane/api/serializers/webhook.pyapps/api/plane/api/urls/__init__.pyapps/api/plane/api/urls/webhook.pyapps/api/plane/api/views/__init__.pyapps/api/plane/api/views/webhook.pyapps/api/plane/app/serializers/webhook.pyapps/api/plane/tests/contract/api/test_webhooks.pyapps/api/plane/utils/webhook.py
Contributor
There was a problem hiding this comment.
Pull request overview
Adds public token-API (X-Api-Key) endpoints to manage workspace webhooks under /api/v1/workspaces/{slug}/webhooks/, mirroring the internal app API behavior while centralizing webhook URL SSRF/disallowed-domain validation in a shared utility.
Changes:
- Introduces token-API CRUD endpoints for workspace webhooks (create/list/retrieve/update/delete), with secret_key returned only on create.
- Extracts webhook URL validation (SSRF + disallowed-domain + loop-back guard) into
plane.utils.webhook.validate_webhook_urland reuses it from both app API and token API serializers. - Adds contract tests covering CRUD behavior, SSRF rejection, secret_key visibility rules, and delivery signature correctness.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/utils/webhook.py | New shared webhook URL validation helper used by both API surfaces. |
| apps/api/plane/tests/contract/api/test_webhooks.py | New contract tests for token-API webhook CRUD + secret/signature guarantees. |
| apps/api/plane/app/serializers/webhook.py | Refactors internal app serializer to call the shared validation helper. |
| apps/api/plane/api/views/webhook.py | Adds token-API webhook endpoint implementation + OpenAPI annotations. |
| apps/api/plane/api/views/init.py | Exposes the new webhook API view from the token-API views package. |
| apps/api/plane/api/urls/webhook.py | Adds URL routes for list/create and detail operations. |
| apps/api/plane/api/urls/init.py | Registers webhook URL patterns into the token-API URL set. |
| apps/api/plane/api/serializers/webhook.py | Adds token-API webhook serializer (incl. shared URL validation). |
| apps/api/plane/api/serializers/init.py | Exports the new token-API webhook serializer. |
Add GET/POST/PATCH/DELETE /api/v1/workspaces/{slug}/webhooks/ to the
public (X-Api-Key) token API, mirroring the internal app-API webhook
endpoint so webhooks can be provisioned programmatically instead of only
through the UI.
- New WebhookAPIEndpoint (workspace-admin only, via WorkspaceOwnerPermission)
reusing the shared Webhook model.
- New public WebhookSerializer: generates secret_key server-side (returned
only on create, withheld on list/retrieve), accepts url + the entity
toggles (issue, issue_comment, cycle, module, project), and enforces the
schema/domain validators plus the SSRF / WEBHOOK_ALLOWED_IPS guard.
- Extract the SSRF/disallowed-domain webhook-URL validation into a shared
plane.utils.webhook.validate_webhook_url helper used by BOTH the app and
public serializers, so the guard can never drift between the two surfaces.
The app serializer keeps its _validate_webhook_url adapter (behaviour
unchanged) and delegates to the shared helper.
- Generic HMAC delivery (bgtasks/webhook_task.py) is unchanged:
X-Plane-Signature stays HMAC-SHA256 over the JSON payload with secret_key.
- Contract tests: create returns a usable server-generated secret, the
secret is withheld on reads, the SSRF/non-http guards reject
loopback/private/scheme-invalid targets, duplicate URLs 409, management is
admin-only, and a real delivery signs the payload correctly with the
API-issued secret.
Signed-off-by: Seena Fallah <seenafallah@gmail.com>
clwluvw
force-pushed
the
webhook-api-auth
branch
from
14dbb17 to
e34e886
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add GET/POST/PATCH/DELETE /api/v1/workspaces/{slug}/webhooks/ to the public (X-Api-Key) token API, mirroring the internal app-API webhook endpoint so webhooks can be provisioned programmatically instead of only through the UI.
Type of Change
Summary by CodeRabbit
New Features
X-Plane-Signature(HMAC) so receivers can verify payload integrity.Bug Fixes
Tests