feat(sdk): webhook receiver hardening — auth, rate limit, provider-shape (#304) - #384
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/sdk/src/cli/serve-webhook.ts">
<violation number="1" location="packages/sdk/src/cli/serve-webhook.ts:93">
P2: When `ratePerSecond` is zero, this emits `Retry-After: Infinity` and `retryAfterMs: null`, neither of which is valid retry metadata. Omit the retry header and field for a non-refilling bucket, or define a finite retry policy.</violation>
</file>
<file name="packages/sdk/src/webhook-signature.ts">
<violation number="1" location="packages/sdk/src/webhook-signature.ts:31">
P1: When `WEBHOOK_SECRET_SLACK` is configured, valid Slack signatures are rejected because `slack.compute` omits Slack’s per-request timestamp. Read `X-Slack-Request-Timestamp` and verify `v0:{timestamp}:{rawBody}` (including replay-age validation), or remove Slack from `SIGNATURE_SCHEMES` until that integration exists.</violation>
</file>
<file name="packages/sdk/src/webhook-rate-limit.ts">
<violation number="1" location="packages/sdk/src/webhook-rate-limit.ts:27">
P2: Every unique route/source key remains in `buckets` forever. Because the receiver admits arbitrary route names before validation, a local client can grow this map without bound; add idle eviction or a bounded key policy.</violation>
<violation number="2" location="packages/sdk/src/webhook-rate-limit.ts:33">
P2: When a caller supplies `NaN` or `Infinity` in `RateLimitConfig`, the constructor accepts it and the bucket can reject every request with a non-finite retry value. Require finite rate and burst values before creating the limiter.</violation>
<violation number="3" location="packages/sdk/src/webhook-rate-limit.ts:68">
P2: When a generic `/raw` request and a `/providers/raw` request share a source, `keyFor` gives them the same bucket. Encode the tuple without using a sentinel string that can also be a provider name.</violation>
</file>
<file name="packages/sdk/tests/webhook-hardening.test.ts">
<violation number="1" location="packages/sdk/tests/webhook-hardening.test.ts:140">
P3: The test 'keys independently per (provider, name, source)' only varies the source address; provider ('github') and name ('pr') are identical across all three consumes, so it does not actually cover provider or name isolation. Vary those dimensions too, or rename the test to reflect that only source is covered.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Slack's v0 scheme is HMAC(SHA256, secret) over `v0:{ts}:{body}`; a | ||
| // deployment that wants Slack signature verification must set the secret | ||
| // to include the timestamp prefix or use provider-specific integration. | ||
| return 'v0=' + createHmac('sha256', secret).update(rawBody).digest('hex'); |
There was a problem hiding this comment.
P1: When WEBHOOK_SECRET_SLACK is configured, valid Slack signatures are rejected because slack.compute omits Slack’s per-request timestamp. Read X-Slack-Request-Timestamp and verify v0:{timestamp}:{rawBody} (including replay-age validation), or remove Slack from SIGNATURE_SCHEMES until that integration exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/webhook-signature.ts, line 31:
<comment>When `WEBHOOK_SECRET_SLACK` is configured, valid Slack signatures are rejected because `slack.compute` omits Slack’s per-request timestamp. Read `X-Slack-Request-Timestamp` and verify `v0:{timestamp}:{rawBody}` (including replay-age validation), or remove Slack from `SIGNATURE_SCHEMES` until that integration exists.</comment>
<file context>
@@ -0,0 +1,74 @@
+ // Slack's v0 scheme is HMAC(SHA256, secret) over `v0:{ts}:{body}`; a
+ // deployment that wants Slack signature verification must set the secret
+ // to include the timestamp prefix or use provider-specific integration.
+ return 'v0=' + createHmac('sha256', secret).update(rawBody).digest('hex');
+ },
+};
</file context>
| const retryAfterSeconds = Math.max(1, Math.ceil(decision.retryAfterMs / 1000)); | ||
| response.setHeader('retry-after', String(retryAfterSeconds)); | ||
| reply(response, 429, { error: 'webhook_rate_limited', retryAfterMs: decision.retryAfterMs }); |
There was a problem hiding this comment.
P2: When ratePerSecond is zero, this emits Retry-After: Infinity and retryAfterMs: null, neither of which is valid retry metadata. Omit the retry header and field for a non-refilling bucket, or define a finite retry policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/cli/serve-webhook.ts, line 93:
<comment>When `ratePerSecond` is zero, this emits `Retry-After: Infinity` and `retryAfterMs: null`, neither of which is valid retry metadata. Omit the retry header and field for a non-refilling bucket, or define a finite retry policy.</comment>
<file context>
@@ -53,6 +81,20 @@ export async function startWebhookServer(dataDir: string, port: number): Promise
+ const decision = limiter.consume(rateKey);
+ if (!decision.allowed) {
+ request.resume();
+ const retryAfterSeconds = Math.max(1, Math.ceil(decision.retryAfterMs / 1000));
+ response.setHeader('retry-after', String(retryAfterSeconds));
+ reply(response, 429, { error: 'webhook_rate_limited', retryAfterMs: decision.retryAfterMs });
</file context>
| const retryAfterSeconds = Math.max(1, Math.ceil(decision.retryAfterMs / 1000)); | |
| response.setHeader('retry-after', String(retryAfterSeconds)); | |
| reply(response, 429, { error: 'webhook_rate_limited', retryAfterMs: decision.retryAfterMs }); | |
| const retryAfterSeconds = Number.isFinite(decision.retryAfterMs) | |
| ? Math.max(1, Math.ceil(decision.retryAfterMs / 1000)) | |
| : undefined; | |
| if (retryAfterSeconds !== undefined) response.setHeader('retry-after', String(retryAfterSeconds)); | |
| reply(response, 429, { error: 'webhook_rate_limited', | |
| ...(retryAfterSeconds === undefined ? {} : { retryAfterMs: decision.retryAfterMs }) }); |
| name: string, | ||
| sourceAddress: string, | ||
| ): string { | ||
| return `${provider ?? 'raw'}:${name}:${sourceAddress}`; |
There was a problem hiding this comment.
P2: When a generic /raw request and a /providers/raw request share a source, keyFor gives them the same bucket. Encode the tuple without using a sentinel string that can also be a provider name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/webhook-rate-limit.ts, line 68:
<comment>When a generic `/raw` request and a `/providers/raw` request share a source, `keyFor` gives them the same bucket. Encode the tuple without using a sentinel string that can also be a provider name.</comment>
<file context>
@@ -0,0 +1,69 @@
+ name: string,
+ sourceAddress: string,
+): string {
+ return `${provider ?? 'raw'}:${name}:${sourceAddress}`;
+}
</file context>
| return `${provider ?? 'raw'}:${name}:${sourceAddress}`; | |
| return JSON.stringify([provider ?? null, name, sourceAddress]); |
| } | ||
|
|
||
| export class TokenBucketLimiter { | ||
| private readonly buckets = new Map<string, Bucket>(); |
There was a problem hiding this comment.
P2: Every unique route/source key remains in buckets forever. Because the receiver admits arbitrary route names before validation, a local client can grow this map without bound; add idle eviction or a bounded key policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/webhook-rate-limit.ts, line 27:
<comment>Every unique route/source key remains in `buckets` forever. Because the receiver admits arbitrary route names before validation, a local client can grow this map without bound; add idle eviction or a bounded key policy.</comment>
<file context>
@@ -0,0 +1,69 @@
+}
+
+export class TokenBucketLimiter {
+ private readonly buckets = new Map<string, Bucket>();
+
+ constructor(
</file context>
| private readonly config: RateLimitConfig, | ||
| private readonly clock: () => number = Date.now, | ||
| ) { | ||
| if (config.burst <= 0 || config.ratePerSecond < 0) { |
There was a problem hiding this comment.
P2: When a caller supplies NaN or Infinity in RateLimitConfig, the constructor accepts it and the bucket can reject every request with a non-finite retry value. Require finite rate and burst values before creating the limiter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/webhook-rate-limit.ts, line 33:
<comment>When a caller supplies `NaN` or `Infinity` in `RateLimitConfig`, the constructor accepts it and the bucket can reject every request with a non-finite retry value. Require finite rate and burst values before creating the limiter.</comment>
<file context>
@@ -0,0 +1,69 @@
+ private readonly config: RateLimitConfig,
+ private readonly clock: () => number = Date.now,
+ ) {
+ if (config.burst <= 0 || config.ratePerSecond < 0) {
+ throw new Error('rate limiter: burst > 0 and ratePerSecond >= 0 required');
+ }
</file context>
|
|
||
| it('keys independently per (provider, name, source)', () => { | ||
| const limiter = new TokenBucketLimiter({ ratePerSecond: 0, burst: 1 }); | ||
| expect(limiter.consume(keyFor('github', 'pr', '10.0.0.1')).allowed).toBe(true); |
There was a problem hiding this comment.
P3: The test 'keys independently per (provider, name, source)' only varies the source address; provider ('github') and name ('pr') are identical across all three consumes, so it does not actually cover provider or name isolation. Vary those dimensions too, or rename the test to reflect that only source is covered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/tests/webhook-hardening.test.ts, line 140:
<comment>The test 'keys independently per (provider, name, source)' only varies the source address; provider ('github') and name ('pr') are identical across all three consumes, so it does not actually cover provider or name isolation. Vary those dimensions too, or rename the test to reflect that only source is covered.</comment>
<file context>
@@ -0,0 +1,144 @@
+
+ it('keys independently per (provider, name, source)', () => {
+ const limiter = new TokenBucketLimiter({ ratePerSecond: 0, burst: 1 });
+ expect(limiter.consume(keyFor('github', 'pr', '10.0.0.1')).allowed).toBe(true);
+ expect(limiter.consume(keyFor('github', 'pr', '10.0.0.1')).allowed).toBe(false);
+ expect(limiter.consume(keyFor('github', 'pr', '10.0.0.2')).allowed).toBe(true);
</file context>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4fe4f6e. Configure here.
| // to include the timestamp prefix or use provider-specific integration. | ||
| return 'v0=' + createHmac('sha256', secret).update(rawBody).digest('hex'); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Slack signature scheme is incorrect
Medium Severity
The slack scheme is registered in SIGNATURE_SCHEMES, so WEBHOOK_SECRET_SLACK selects it, but compute HMACs only the raw body and never reads x-slack-request-timestamp. Slack signs v0:{timestamp}:{body}, so enabling that secret rejects real Slack deliveries. Putting the timestamp into the static secret cannot work because the timestamp changes per request.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4fe4f6e. Configure here.
| const missingTokens = cost - bucket.tokens; | ||
| const retryAfterMs = this.config.ratePerSecond > 0 | ||
| ? Math.ceil((missingTokens / this.config.ratePerSecond) * 1000) | ||
| : Number.POSITIVE_INFINITY; |
There was a problem hiding this comment.
Zero rate emits invalid Retry-After
Low Severity
When ratePerSecond is 0, consume returns retryAfterMs of Infinity. The receiver then sends retry-after: Infinity, which is not a valid delay-seconds value, and JSON.stringify turns retryAfterMs into null. The HTTP rate-limit test uses this configuration, so clients lose a usable retry hint.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4fe4f6e. Configure here.
maintainability lens — FAILPR #384 maintainability reviewLens: could a stranger read this in six months and change it safely? I focused on unclear boundaries, implicit contracts, silent renames, and tests that would not fail if the behavior broke. BlockersB1 — Slack scheme is registered as supported but structurally cannot verify a real Slack signature
B2 — 429 emits
|
history lens — PASSBlockers: none under the HISTORY lens. Reviewed the supplied diff, commit The receiver changes in serve-webhook.ts, lines 108–143 add optional authentication before parsing and retain provider-envelope validation. I found no evidence that they restore a safeguard-removal pattern recorded in DRIVE-LOG. Unsigned operation predates this PR; preserving the loopback development posture is not a newly introduced fail-open regression. The additions remain in the SDK and introduce no kernel step types, provider dependencies inside Rust, or changes to journal authority. I found no new contradiction with a settled RFC decision. The shared-store deferral in webhook-rate-limit.ts, lines 1–7 is explicitly documented and is not a blocker. Concern: webhook-signature.ts, lines 25–44 registers Slack while hashing only Notes: The commit’s “11 tests” statement matches the declaration count. Literal command and captured output: That establishes test count only. I did not execute tests or typechecking, so the PR body’s passing-test assertions remain unverified here, rather than disproven. The older gate brief in REVIEW_PASSED |
structure lens — MISSING |
|
🎯 review-swarm: FAILED (M:fail H:pass S:missing) Lens transcripts posted as sibling comments above. |
…ape (#304) - HMAC signature verification: per-provider `WEBHOOK_SECRET_<UPPER>` env gates authentication; unsigned mode retained for local development. GitHub `x-hub-signature-256` and generic `x-flows-signature-256` schemes ship today. Refusal: `webhook_signature_invalid`. - Rate limiting: in-process token bucket keyed by (provider, name, source). Refusal: `webhook_rate_limited` with `retry-after` header. Cluster mode will need a shared store; taxonomy stays the same. - Provider-shape validation: reuses `providerInboxEvent` but the emitted error is now the canonical `webhook_payload_invalid`. Files: - packages/sdk/src/webhook-signature.ts (new) - packages/sdk/src/webhook-rate-limit.ts (new) - packages/sdk/src/cli/serve-webhook.ts - packages/sdk/tests/webhook-hardening.test.ts (new, 11 tests) - packages/sdk/tsconfig.tests.json Closes #304. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Session-Id: efeda5df-9b7c-48d4-b2ce-957f5bef0a82 Session-Id: efeda5df-9b7c-48d4-b2ce-957f5bef0a82
4fe4f6e to
ad3d3eb
Compare
1 participant
Closes #304. Follow-up to #333 (slice E) + #380 (admission).
Summary
WEBHOOK_SECRET_<PROVIDER_UPPER>gates auth; unsigned mode retained for loopback dev per flows: event triggers via webhook + inbox watcher — SURFACE §1 harness #301. GitHub (x-hub-signature-256) + generic (x-flows-signature-256) schemes ship today.Retry-After. Cluster mode via shared store is a follow-up.webhook_payload_invaliderror kind.Test plan
tests/webhook-hardening.test.ts— signature refuse/accept/missing, unsigned dev mode, rate limit burst + refill + per-key isolation, provider-shape refusewebhook.test.tstests still passNote
Medium Risk
Changes webhook ingress behavior (401/429 paths, error renames) and auth tied to env secrets; impact is mostly loopback dev unless secrets are enabled for public ingress.
Overview
Hardens the local webhook receiver with optional HMAC auth, per-route rate limits, and a stable error for bad provider envelopes.
startWebhookServernow takesWebhookServerOptions(admittedNames, injectableenv, optionalrateLimit). Request handling order is tightened: loaded-flow admission still runs before body read, and rate limiting (default 20/s, burst 60) runs next, keyed by provider, name, and client address via newwebhook-rate-limithelpers. WhenWEBHOOK_SECRET_<PROVIDER>(orWEBHOOK_SECRET_DEFAULTfor raw routes) is set,webhook-signatureverifies GitHub/Slack/generic headers on the raw body and returns 401webhook_signature_invalid; with no secret, loopback dev stays unsigned.Provider route validation failures now use
webhook_payload_invalidinstead ofinvalid_provider_event.tests/webhook-hardening.test.tscovers signatures, 429 +Retry-After, and shape rejection; it is wired intotsconfig.tests.json.Reviewed by Cursor Bugbot for commit ad3d3eb. Bugbot is set up for automated code reviews on this repo. Configure here.