fix: add URL validation to calendar integration endpoints#28768
fix: add URL validation to calendar integration endpoints#28768pedroccastro wants to merge 4 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds SSRF URL validation to multiple calendar integration API endpoints and introduces Zod request-body schemas for CalDAV and ICS Feed. Handlers for CalDAV, Exchange 2013, Exchange 2016, and the Exchange classic path call 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
🧹 Nitpick comments (1)
packages/app-store/caldavcalendar/api/add.schema.ts (1)
3-7: Tighten credential fields to reject empty input early.
z.string()permits empty strings forusername/password. Consider enforcing non-empty values at schema level to fail earlier and avoid unnecessary downstream calls.Suggested change
export const caldavAddBodySchema = z.object({ - username: z.string(), - password: z.string(), + username: z.string().min(1), + password: z.string().min(1), url: z.string().url(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-store/caldavcalendar/api/add.schema.ts` around lines 3 - 7, The caldavAddBodySchema currently allows empty strings for username and password; update caldavAddBodySchema to enforce non-empty values for those fields (e.g., change username: z.string() and password: z.string() to use z.string().nonempty() or z.string().min(1) so validation fails early). Keep url as z.string().url() and run unit/validation tests for any callers that construct request bodies.
🤖 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/app-store/ics-feedcalendar/api/add.schema.ts`:
- Around line 3-5: The icsFeedAddBodySchema currently allows an empty urls
array; update the schema for icsFeedAddBodySchema so the urls array requires at
least one element (use z.array(...).min(1)) and include a clear validation
message if desired; target the urls definition in icsFeedAddBodySchema to
enforce .min(1) on the array of z.string().url().
In `@packages/app-store/tests/calendar-ssrf-validation.test.ts`:
- Around line 84-180: Add SSRF unit tests for the two modified handlers to
mirror the existing Exchange 2013 cases: import the handler from
"exchange2016calendar/api/add" and the POST handler from
"exchangecalendar/api/_postAdd" (similar to how postHandler is derived in the
Exchange 2013 describe block), then write tests that (1) mockBlockedUrl/error
and assert a 400 response when a blocked URL is supplied, and (2) mockValidUrl
and assert a non-400 (allowed) response for a valid Exchange URL; also verify
that validateUrlForSSRF is called as expected when applicable. Ensure you reuse
the same helper patterns shown (createReqRes, CREDENTIAL_BODY, mockBlockedUrl,
mockValidUrl, validateUrlForSSRF) and assert status codes consistently with the
Exchange 2013 tests.
---
Nitpick comments:
In `@packages/app-store/caldavcalendar/api/add.schema.ts`:
- Around line 3-7: The caldavAddBodySchema currently allows empty strings for
username and password; update caldavAddBodySchema to enforce non-empty values
for those fields (e.g., change username: z.string() and password: z.string() to
use z.string().nonempty() or z.string().min(1) so validation fails early). Keep
url as z.string().url() and run unit/validation tests for any callers that
construct request bodies.
🪄 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: 6881a801-6c7b-4711-b762-0e119409b6e6
📒 Files selected for processing (8)
packages/app-store/caldavcalendar/api/add.schema.tspackages/app-store/caldavcalendar/api/add.tspackages/app-store/exchange2013calendar/api/add.tspackages/app-store/exchange2016calendar/api/add.tspackages/app-store/exchangecalendar/api/_postAdd.tspackages/app-store/ics-feedcalendar/api/add.schema.tspackages/app-store/ics-feedcalendar/api/add.tspackages/app-store/tests/calendar-ssrf-validation.test.ts
… for Exchange 2016 and Classic
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 calendar integration setup endpoints, consistent with how webhook endpoints already validate URLs.
Changes
How should this be tested?
Automated
Manual
Mandatory Tasks