fix: add input validation to webhook subscription creation#28770
fix: add input validation to webhook subscription creation#28770pedroccastro wants to merge 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughSSRF (Server-Side Request Forgery) validation is added to webhook subscription creation. The 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/webhooks/lib/addSubscription.test.ts`:
- Around line 7-9: The SSRF module mock in addSubscription.test.ts only provides
validateUrlForSSRF but addSubscription (via scheduleTrigger.ts) also calls
logBlockedSSRFAttempt; update the vi.mock for "@calcom/lib/ssrfProtection" to
include logBlockedSSRFAttempt: vi.fn(), then in both blocked-URL test cases that
expect SSRF blocking (the two tests around the blocked URL assertions) add
assertions that logBlockedSSRFAttempt was called (e.g., toHaveBeenCalled /
toHaveBeenCalledWith the blocked URL/context) so the security logging path is
covered for addSubscription/scheduleTrigger.ts.
In `@packages/features/webhooks/lib/scheduleTrigger.ts`:
- Around line 49-53: The blocked-URL handling in scheduleTrigger.ts swallows the
failure because the thrown plain Error is caught and returns undefined; update
the validation branch around validateUrlForSSRF(subscriberUrl) to log via
logBlockedSSRFAttempt and then throw an ErrorWithCode (not a plain Error)
describing the blocked URL and include validation.error as the message/code, and
ensure you rethrow so callers can distinguish blocked-URL errors; apply the same
change to the other similar block around lines 128-136 (same pattern) so both
use ErrorWithCode and rethrow after logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 590a1c2f-2360-4d2a-a6c2-86ba0693d221
📒 Files selected for processing (2)
packages/features/webhooks/lib/addSubscription.test.tspackages/features/webhooks/lib/scheduleTrigger.ts
| vi.mock("@calcom/lib/ssrfProtection", () => ({ | ||
| validateUrlForSSRF: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that production code imports both SSRF helpers while test mock currently defines only one.
rg -n 'from "@calcom/lib/ssrfProtection"|logBlockedSSRFAttempt|validateUrlForSSRF|vi\.mock\("@calcom/lib/ssrfProtection"' \
packages/features/webhooks/lib/scheduleTrigger.ts \
packages/features/webhooks/lib/addSubscription.test.tsRepository: calcom/cal.com
Length of output: 1455
🏁 Script executed:
cat -n packages/features/webhooks/lib/addSubscription.ts | head -100Repository: calcom/cal.com
Length of output: 139
🏁 Script executed:
fd -i addsubscription packages/features/webhooks/Repository: calcom/cal.com
Length of output: 112
🏁 Script executed:
rg -l "export.*addSubscription" packages/features/webhooks/Repository: calcom/cal.com
Length of output: 107
🏁 Script executed:
cat -n packages/features/webhooks/lib/scheduleTrigger.tsRepository: calcom/cal.com
Length of output: 23813
🏁 Script executed:
cat -n packages/features/webhooks/lib/addSubscription.test.tsRepository: calcom/cal.com
Length of output: 3274
Mock and assert logBlockedSSRFAttempt in blocked-URL test cases.
addSubscription calls both SSRF exports (line 51 in scheduleTrigger.ts), but the test mock only provides validateUrlForSSRF. Add logBlockedSSRFAttempt to the module mock and assert it's called in both blocked URL test cases (lines 60–71 and 73–86) so security logging is actually covered.
🧪 Suggested test update
-import { validateUrlForSSRF } from "@calcom/lib/ssrfProtection";
+import { logBlockedSSRFAttempt, validateUrlForSSRF } from "@calcom/lib/ssrfProtection";
vi.mock("@calcom/lib/ssrfProtection", () => ({
validateUrlForSSRF: vi.fn(),
+ logBlockedSSRFAttempt: vi.fn(),
}));
...
it("does not create webhook when URL is blocked", async () => {
vi.mocked(validateUrlForSSRF).mockResolvedValue({ isValid: false, error: "Blocked hostname" });
const result = await addSubscription({
...VALID_PARAMS,
subscriberUrl: "http://169.254.169.254/latest/meta-data/",
});
expect(validateUrlForSSRF).toHaveBeenCalledWith("http://169.254.169.254/latest/meta-data/");
expect(prisma.webhook.create).not.toHaveBeenCalled();
+ expect(logBlockedSSRFAttempt).toHaveBeenCalledWith(
+ "http://169.254.169.254/latest/meta-data/",
+ "Blocked hostname",
+ { appId: VALID_PARAMS.appId }
+ );
expect(result).toBeUndefined();
});
it("does not create webhook when URL resolves to private IP", async () => {
vi.mocked(validateUrlForSSRF).mockResolvedValue({
isValid: false,
error: "Hostname resolves to private IP",
});
const result = await addSubscription({
...VALID_PARAMS,
subscriberUrl: "https://evil.example.com/",
});
expect(prisma.webhook.create).not.toHaveBeenCalled();
+ expect(logBlockedSSRFAttempt).toHaveBeenCalledWith(
+ "https://evil.example.com/",
+ "Hostname resolves to private IP",
+ { appId: VALID_PARAMS.appId }
+ );
expect(result).toBeUndefined();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/webhooks/lib/addSubscription.test.ts` around lines 7 - 9,
The SSRF module mock in addSubscription.test.ts only provides validateUrlForSSRF
but addSubscription (via scheduleTrigger.ts) also calls logBlockedSSRFAttempt;
update the vi.mock for "@calcom/lib/ssrfProtection" to include
logBlockedSSRFAttempt: vi.fn(), then in both blocked-URL test cases that expect
SSRF blocking (the two tests around the blocked URL assertions) add assertions
that logBlockedSSRFAttempt was called (e.g., toHaveBeenCalled /
toHaveBeenCalledWith the blocked URL/context) so the security logging path is
covered for addSubscription/scheduleTrigger.ts.
| const validation = await validateUrlForSSRF(subscriberUrl); | ||
| if (!validation.isValid) { | ||
| logBlockedSSRFAttempt(subscriberUrl, validation.error ?? "", { appId }); | ||
| throw new Error(`Subscriber URL is not allowed: ${validation.error}`); | ||
| } |
There was a problem hiding this comment.
Blocked-URL failures are currently swallowed instead of propagated.
Line 52 throws, but the outer catch logs and returns undefined, so callers can’t reliably distinguish a blocked URL from other non-success paths. Please rethrow after logging. Also, in this non-tRPC module, use ErrorWithCode instead of a plain Error for the blocked URL case.
🔧 Proposed fix
} catch (error) {
const userId = appApiKey ? appApiKey.userId : account && !account.isTeam ? account.id : null;
const teamId = appApiKey ? appApiKey.teamId : account && account.isTeam ? account.id : null;
log.error(
`Error creating subscription for ${teamId ? `team ${teamId}` : `user ${userId}`}.`,
safeStringify(error)
);
+ throw error;
}As per coding guidelines: Use ErrorWithCode for errors in non-tRPC files (services, repositories, utilities); use TRPCError only in tRPC routers.
Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/webhooks/lib/scheduleTrigger.ts` around lines 49 - 53, The
blocked-URL handling in scheduleTrigger.ts swallows the failure because the
thrown plain Error is caught and returns undefined; update the validation branch
around validateUrlForSSRF(subscriberUrl) to log via logBlockedSSRFAttempt and
then throw an ErrorWithCode (not a plain Error) describing the blocked URL and
include validation.error as the message/code, and ensure you rethrow so callers
can distinguish blocked-URL errors; apply the same change to the other similar
block around lines 128-136 (same pattern) so both use ErrorWithCode and rethrow
after logging.
E2E results are ready! |
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
What does this PR do?
Adds URL validation to the
addSubscriptionfunction inscheduleTrigger.ts, consistent with how the tRPC webhook create/edit handlers already validate subscriber URLs. This centralizes the check so all callers (Zapier, Make, and future integrations) inherit the validation.Changes
prisma.webhook.createinaddSubscriptionHow should this be tested?
Automated
Manual
Mandatory Tasks