fix(api): improve ICS URL validation edge cases#29412
Conversation
|
Welcome to Cal.diy, @abhinavlevi! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughThis pull request adds SSRF-protected calendar feed URL validation. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/api/v2/src/platform/calendars/input/create-ics.input.ts (1)
20-28: ⚡ Quick winAvoid duplicating URL-security logic in
IsICSUrlConstraint.Line 20-28 duplicates the new calendar URL rules instead of reusing the shared validator. This creates drift risk for future security fixes. Prefer composing with
IsCalendarUrlConstraint/IsCalendarUrland keep one source of truth.Refactor sketch
-import { - ... - Validate, - ValidatorConstraint, - ValidatorConstraintInterface, - isURL, - ... -} from "class-validator"; +import { ..., Validate, ValidatorConstraint, ValidatorConstraintInterface, ... } from "class-validator"; +import { IsCalendarUrlConstraint } from "../validators/is-calendar-url"; ... validate(url: unknown) { - if (typeof url !== "string") return false; - return isURL(url, { - protocols: ["http", "https"], - require_protocol: true, - require_valid_protocol: true, - require_tld: process.env.NODE_ENV === "production", - host_blacklist: ["localhost", "127.0.0.1", "::1"], - }); + return new IsCalendarUrlConstraint().validate(url); }Also applies to: 50-50
🤖 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/v2/src/platform/calendars/input/create-ics.input.ts` around lines 20 - 28, The IsICSUrlConstraint duplicates the calendar URL validation rules; replace the inline isURL(...) logic with a call or composition that reuses the existing IsCalendarUrlConstraint / IsCalendarUrl validator to ensure one source of truth. Specifically, import IsCalendarUrlConstraint or IsCalendarUrl and either delegate validation inside IsICSUrlConstraint to that constraint's validate method or apply the IsCalendarUrl decorator where IsICSUrl is used, removing the duplicated protocols/require_tld/host_blacklist options so future security fixes only need to be made in IsCalendarUrlConstraint.apps/api/v2/src/platform/calendars/validators/is-calendar-url.ts (1)
1-7: ⚡ Quick winUse
import typefor type-only class-validator imports.
ValidationOptionsandValidatorConstraintInterfaceare type-only and should be imported withimport typeto match project standards.As per coding guidelines, "Use
import type { X }for TypeScript type imports".🤖 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/v2/src/platform/calendars/validators/is-calendar-url.ts` around lines 1 - 7, The import statement currently brings in both runtime and type-only symbols; change it so type-only identifiers are imported with "import type" — specifically import ValidationOptions and ValidatorConstraintInterface using "import type { ValidationOptions, ValidatorConstraintInterface }" and keep registerDecorator, ValidatorConstraint, and isURL as regular imports; update the import lines near the top of is-calendar-url.ts to reflect this split so type-only symbols follow project standards.apps/api/v2/src/platform/calendars/input/create-ics.input.test.ts (1)
28-33: ⚡ Quick winAdd SSRF edge-case tests for numeric/private hosts.
Current security test only checks
localhost. Add cases for127.0.0.1,[::1], and at least one private IPv4 host (e.g.,192.168.1.10) so regressions in host filtering are caught early.🤖 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/v2/src/platform/calendars/input/create-ics.input.test.ts` around lines 28 - 33, Extend the existing SSRF test in apps/api/v2/src/platform/calendars/input/create-ics.input.test.ts that uses CreateIcsFeedInputDto and validate: add additional it() cases (or expand the current case) to assert validation rejects URLs with numeric/private hosts — include "http://127.0.0.1/fake-calendar", "http://[::1]/fake-calendar" and a private IPv4 like "http://192.168.1.10/fake-calendar" — each should construct a CreateIcsFeedInputDto, assign input.urls, call await validate(input), and expect errors.length to be > 0 so the host filtering covers these SSRF edge cases.
🤖 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.
Inline comments:
In `@apps/api/v2/src/platform/calendars/validators/is-calendar-url.ts`:
- Around line 15-21: Change the two imports ValidationOptions and
ValidatorConstraintInterface to type-only imports (import type {
ValidationOptions, ValidatorConstraintInterface }) so they are erased at
runtime, and replace the brittle host_blacklist-based SSRF check around the
isURL(...) call with an explicit private/loopback IP check: after parsing the
URL/hostname (used with isURL) resolve or validate the hostname to an IP and
reject if net.isIP indicates an address in 127.0.0.0/8, 10.0.0.0/8,
172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16, IPv6 ::1, fc00::/7 or fe80::/10;
keep hostname blacklist for literal names like localhost/::1 but rely on the
IP-range check to block other loopback/private addresses before accepting the
URL.
---
Nitpick comments:
In `@apps/api/v2/src/platform/calendars/input/create-ics.input.test.ts`:
- Around line 28-33: Extend the existing SSRF test in
apps/api/v2/src/platform/calendars/input/create-ics.input.test.ts that uses
CreateIcsFeedInputDto and validate: add additional it() cases (or expand the
current case) to assert validation rejects URLs with numeric/private hosts —
include "http://127.0.0.1/fake-calendar", "http://[::1]/fake-calendar" and a
private IPv4 like "http://192.168.1.10/fake-calendar" — each should construct a
CreateIcsFeedInputDto, assign input.urls, call await validate(input), and expect
errors.length to be > 0 so the host filtering covers these SSRF edge cases.
In `@apps/api/v2/src/platform/calendars/input/create-ics.input.ts`:
- Around line 20-28: The IsICSUrlConstraint duplicates the calendar URL
validation rules; replace the inline isURL(...) logic with a call or composition
that reuses the existing IsCalendarUrlConstraint / IsCalendarUrl validator to
ensure one source of truth. Specifically, import IsCalendarUrlConstraint or
IsCalendarUrl and either delegate validation inside IsICSUrlConstraint to that
constraint's validate method or apply the IsCalendarUrl decorator where IsICSUrl
is used, removing the duplicated protocols/require_tld/host_blacklist options so
future security fixes only need to be made in IsCalendarUrlConstraint.
In `@apps/api/v2/src/platform/calendars/validators/is-calendar-url.ts`:
- Around line 1-7: The import statement currently brings in both runtime and
type-only symbols; change it so type-only identifiers are imported with "import
type" — specifically import ValidationOptions and ValidatorConstraintInterface
using "import type { ValidationOptions, ValidatorConstraintInterface }" and keep
registerDecorator, ValidatorConstraint, and isURL as regular imports; update the
import lines near the top of is-calendar-url.ts to reflect this split so
type-only symbols follow project standards.
🪄 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: d5fc65de-c628-4fa5-bc19-42151bcff88d
📒 Files selected for processing (3)
apps/api/v2/src/platform/calendars/input/create-ics.input.test.tsapps/api/v2/src/platform/calendars/input/create-ics.input.tsapps/api/v2/src/platform/calendars/validators/is-calendar-url.ts
abhinavlevi
force-pushed
the
fix/ics-validation-clean-v5
branch
from
942b014 to
f9660ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/v2/src/platform/calendars/validators/is-calendar-url.ts (1)
62-62: 💤 Low valueRemove developer debug comment.
The comment "THIS fixes your failing test" is test-specific and not appropriate for production code. Either remove it or replace with a meaningful explanation of the SSRF protection purpose.
- // DNS SSRF check (THIS fixes your failing test) + // Resolve hostname and block if any IP is private/internal (SSRF protection)🤖 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/v2/src/platform/calendars/validators/is-calendar-url.ts` at line 62, Remove the ephemeral debug text "THIS fixes your failing test" from the inline comment next to the "DNS SSRF check" and either delete the comment or replace it with a concise, production-ready note explaining the SSRF protection (e.g., why DNS validation is performed and what threat it mitigates) in the isCalendarUrl validator.
🤖 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.
Inline comments:
In `@apps/api/v2/src/platform/calendars/validators/is-calendar-url.ts`:
- Line 19: The IPv6 regex in is-calendar-url.ts currently uses an alternation
that only matches exact strings (so prefixes like "fe80::1" or "fd00::1" slip
through); update the regex used in that validator to match IPv6
private/link-local prefixes rather than exact full-string values — specifically
change the pattern referenced in is-calendar-url.ts so it anchors the start (^)
and checks for prefixes like "::1", "fe80:", and the unique-local ranges
starting with "fc" or "fd" (matching the prefix rather than requiring an exact
match) and ensure case-insensitive flag remains; replace the existing
/^(::1|fe80:|fc|fd|::)$/i pattern with a prefix-aware alternative and run tests
for addresses like fe80::1 and fd00::1 to confirm they are blocked.
---
Nitpick comments:
In `@apps/api/v2/src/platform/calendars/validators/is-calendar-url.ts`:
- Line 62: Remove the ephemeral debug text "THIS fixes your failing test" from
the inline comment next to the "DNS SSRF check" and either delete the comment or
replace it with a concise, production-ready note explaining the SSRF protection
(e.g., why DNS validation is performed and what threat it mitigates) in the
isCalendarUrl validator.
🪄 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: 520efda8-ad3e-4ddf-94a6-44148175a0e4
📒 Files selected for processing (3)
apps/api/v2/src/platform/calendars/input/create-ics.input.test.tsapps/api/v2/src/platform/calendars/input/create-ics.input.tsapps/api/v2/src/platform/calendars/validators/is-calendar-url.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/v2/src/platform/calendars/input/create-ics.input.test.ts
- apps/api/v2/src/platform/calendars/input/create-ics.input.ts
| /^192\.168\./.test(ip) || | ||
| /^172\.(1[6-9]|2[0-9]|3[0-1])\./.test(ip) || | ||
| /^169\.254\./.test(ip) || | ||
| /^(::1|fe80:|fc|fd|::)$/i.test(ip) |
There was a problem hiding this comment.
Critical: IPv6 regex does not block private/link-local addresses.
The regex /^(::1|fe80:|fc|fd|::)$/i requires an exact match due to the $ anchor being outside the alternation group. Real IPv6 addresses like fe80::1 (link-local) or fd00::1 (unique-local) will not match, allowing SSRF bypass via IPv6 private addresses.
fe80::1234:5678.match(regex) →null(should block, doesn't)fd12:3456::1.match(regex) →null(should block, doesn't)
Proposed fix
private isPrivateIp(ip: string): boolean {
+ const normalizedIp = ip.toLowerCase();
return (
/^127\./.test(ip) ||
/^10\./.test(ip) ||
/^192\.168\./.test(ip) ||
/^172\.(1[6-9]|2[0-9]|3[0-1])\./.test(ip) ||
/^169\.254\./.test(ip) ||
- /^(::1|fe80:|fc|fd|::)$/i.test(ip)
+ normalizedIp === "::1" ||
+ normalizedIp === "::" ||
+ /^fe80:/i.test(ip) ||
+ /^f[cd]/i.test(ip)
);
}🤖 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/v2/src/platform/calendars/validators/is-calendar-url.ts` at line 19,
The IPv6 regex in is-calendar-url.ts currently uses an alternation that only
matches exact strings (so prefixes like "fe80::1" or "fd00::1" slip through);
update the regex used in that validator to match IPv6 private/link-local
prefixes rather than exact full-string values — specifically change the pattern
referenced in is-calendar-url.ts so it anchors the start (^) and checks for
prefixes like "::1", "fe80:", and the unique-local ranges starting with "fc" or
"fd" (matching the prefix rather than requiring an exact match) and ensure
case-insensitive flag remains; replace the existing /^(::1|fe80:|fc|fd|::)$/i
pattern with a prefix-aware alternative and run tests for addresses like fe80::1
and fd00::1 to confirm they are blocked.
|
|
|
This seems to be a duplicate of #29307 |
Thanks for pointing that out. While both PRs address the same user-facing issue, this implementation takes a broader validation-focused approach instead of only removing the In addition to accepting RFC-compliant non-
So the overlap is mainly in the symptom being fixed, while the implementation scope and validation behavior differ. |
|
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. |
3 participants
Summary
Fixes ICS feed validation rejecting standards-compliant calendar feeds when URLs do not end with
.ics.The previous validation relied on URL suffix matching instead of validating the actual calendar feed endpoint structure.
This PR:
.icssuffix enforcementIsCalendarUrlvalidator.icscalendar feed URLsExample Previously Rejected
This endpoint returns:
Content-Type: text/calendarBEGIN:VCALENDARbut was rejected only because the URL did not end with
.ics.Why
RFC 5545 and RFC 4791 do not require calendar feed URLs to end with
.ics.Validation should focus on:
rather than filename suffixes.