[WEB-8103] fix: stop leaking webhook HMAC secret_key on reads (GHSA-83rj)#9382
[WEB-8103] fix: stop leaking webhook HMAC secret_key on reads (GHSA-83rj)#9382mguptahub wants to merge 1 commit into
Conversation
…3rj) WebhookEndpoint list/retrieve/patch pass a fields= allowlist that excludes secret_key, but DynamicBaseSerializer.__init__ discards the caller allowlist (fields = self.expand). With WebhookSerializer using fields="__all__" and secret_key only in read_only_fields (read-only is still serialized), the HMAC signing secret leaked on every webhook read (GHSA-83rj-4282-x39v; admin-only). Rather than secret_key = CharField(write_only=True) — which would let a client inject their own secret on create/patch and break the intended one-time reveal — hide it by default and reveal only where intended: - WebhookSerializer.to_representation drops secret_key unless the show_secret_key context flag is set (secure by default). secret_key stays server-generated (default=generate_token) and non-writable. - POST create and WebhookSecretRegenerateEndpoint pass show_secret_key so the secret is still returned once for the caller to configure their receiver; list/retrieve/patch no longer emit it. Add contract regression tests (fail-before verified). Follow-up: the DynamicBaseSerializer.__init__ allowlist bug affects other serializers — tracked separately. Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
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 (3)
📝 WalkthroughWalkthroughWebhookSerializer now overrides to_representation to remove secret_key from output unless the serializer context sets show_secret_key. WebhookEndpoint and WebhookSecretRegenerateEndpoint pass this context flag on create and regenerate responses. A new regression test module validates the exposure rules across endpoints and serializer directly. ChangesWebhook secret_key scoping
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookEndpoint
participant WebhookSerializer
Client->>WebhookEndpoint: POST create/regenerate webhook
WebhookEndpoint->>WebhookSerializer: instantiate with context show_secret_key True
WebhookSerializer->>WebhookSerializer: to_representation checks context
WebhookSerializer-->>WebhookEndpoint: data including secret_key
WebhookEndpoint-->>Client: response with secret_key
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a security issue where webhook HMAC secret_key was being serialized and returned on webhook list/retrieve/patch responses due to a serializer field-allowlist being silently dropped. The change makes secret_key hidden by default in WebhookSerializer and only revealed when explicitly opted-in via serializer context, preserving the intended one-time reveal behavior on create/regenerate.
Changes:
- Hide
secret_keyby default inWebhookSerializer.to_representation, only returning it whencontext["show_secret_key"]is true. - Opt-in to secret reveal for webhook creation and secret regeneration endpoints via
show_secret_key=True. - Add contract regression tests ensuring list/retrieve/patch never leak
secret_key, while create/regenerate still reveal it.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/api/plane/app/serializers/webhook.py | Filters secret_key out of serialized output by default; supports explicit reveal via serializer context flag. |
| apps/api/plane/app/views/webhook/base.py | Passes show_secret_key=True only on create and secret regeneration responses. |
| apps/api/plane/tests/contract/app/test_webhook_secret_key_scope_app.py | Adds regression tests covering both non-leak and reveal-once cases. |
Fixes GHSA-83rj-4282-x39v (MED, admin-only).
Problem
WebhookEndpointlist/retrieve/patch pass afields=(...)allowlist that deliberately excludessecret_key, butDynamicBaseSerializer.__init__discards the caller allowlist (fields = self.expand). Combined withWebhookSerializerusingfields="__all__"andsecret_keyonly inread_only_fields(read-only fields are still serialized), the HMAC signingsecret_keywas returned on every webhook read. All handlers are@allow_permission([ADMIN], level="WORKSPACE"), so exposure is workspace-admin-only.Fix
The advisory suggests
secret_key = serializers.CharField(write_only=True), but that would (a) let a client inject their own secret on create/patch (a downgrade — the secret is server-generated viadefault=generate_token) and (b) break the intended one-time reveal. Instead:WebhookSerializer.to_representationdropssecret_keyunless ashow_secret_keycontext flag is set — secure by default.secret_keystays server-generated and non-writable.POST createandWebhookSecretRegenerateEndpointpassshow_secret_key=True, so the secret is still returned once for the caller to configure their receiver.WebhookSerializeris only used inapp/views/webhook/base.py(no external API / EE consumer), so the blast radius is contained.Tests
tests/contract/app/test_webhook_secret_key_scope_app.py— 6 tests: list/retrieve/patch do not leak, create + regenerate still reveal, and a serializer-level check of both branches of the flag. Fail-before verified (the four no-leak tests returnsecret_keybefore the fix).ruff+manage.py checkgreen.Follow-up
The root
DynamicBaseSerializer.__init__bug (silently discarding thefields=allowlist) also affects other serializers (workspace/project member over-exposure) — tracked as a separate audit.Summary by CodeRabbit