fix: safe env var parsing, error handling, localStorage validation, and hook fixes - #29823
fix: safe env var parsing, error handling, localStorage validation, and hook fixes#29823PedroHenrique0713 wants to merge 5 commits into
Conversation
fix(text): truncateOnWord now respects its maxLength parameter instead of using hardcoded 148 fix(CalEventParser): getProviderName handles empty location strings without crashing fix(csvUtils): protect against CSV formula injection (=, +, -, @ prefixes) per OWASP guidance fix(extract-base-email): handle emails missing @ sign without returning 'local@undefined' fix(getBookingResponsesSchema): avoid mutating input array with in-place sort All fixes include tests that fail before the change and pass after.
fix(text): guard against lastIndexOf returning -1 when no space in text truncateOnWord previously called substring(0, -1) which produces empty string when no space character exists within maxLength. fix(getBookingResponsesSchema): compare against sorted copy, not original The previous fix created a copy for sorting but still compared items against the unsorted emails array, potentially missing duplicates. fix(csvUtils.test): remove stale tests from unmerged PR calcom#29783 The \r handling and MIME type tests belonged to PR calcom#29783 which has not been merged. Keep only formula injection tests from this PR.
…y improvements
fix(jsonUtils): validJson now accepts all valid JSON types (string, number,
boolean, null, array), not just objects. Previously rejected valid JSON per
RFC 8259 ("hello", 42, true, null, []) by requiring typeof === "object".
fix(formatPhoneNumber): wrap parsePhoneNumberWithError in try/catch.
libphonenumber-js throws NOT_A_NUMBER error on unrecognized input,
previously uncaught causing unhandled exception.
fix(safeStringify): return placeholder string instead of raw object on
circular reference. Callers expect a string return type; returning the
raw object could cause downstream TypeError on string operations.
fix(contructEmailFromPhoneNumber): add null guard. phoneNumber.replace()
throws TypeError on null/undefined input.
…ixes
fix(hashedLinksUtils): isLinkExpired now checks BOTH expiry time AND usage count.
Previously short-circuited on expiresAt and never reached the usage check.
fix(getIP): wrap JSON.parse and banlist validation in try/catch.
Invalid IP_BANLIST env variable crashed server on every IP check.
fix(intervalTree): sort nodes by start time before building tree.
Unsorted split broke interval tree search invariants (O(n) vs O(log n+k)).
fix(timezone): guard against labels without closing parenthesis.
label.split(') ')[1] was undefined when label had no ')', crashing render.
fix(getSafeRedirectUrl): wrap new URL() in try/catch for config constants.
Misconfigured CONSOLE_URL/WEBAPP_URL/WEBSITE_URL crashed redirect handler.
fix(constants): wrap ALLOWED_HOSTNAMES/RESERVED_SUBDOMAINS JSON.parse in try/catch with CSV fallback. Invalid env vars crashed the app at startup as module-level top-level code. fix(errors): remove redundant condition in handleErrorsJson. response.ok already means status 200-299; the extra check added nothing and the gzip path still bypasses status checks. fix(recentImpersonations): validate localStorage result is Array. Corrupt localStorage returning non-array caused .filter() crash in addRecentImpersonation. fix(getIP): wrap isIpInBanListString JSON.parse in try/catch. Sister function isIpInBanlist was already guarded; this one had no protection against malformed IP_BANLIST env var.
|
Welcome to Cal.diy, @PedroHenrique0713! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughShared utilities now defensively handle malformed environment values, IP ban lists, redirects, JSON, stringification, and missing inputs. Provider-name, email, phone-number, text, timezone, interval-tree, hashed-link, search-parameter, observer, and booking-response behavior were updated. CSV sanitization now prefixes spreadsheet formula values, with tests covering escaping and serialization. New tests cover provider-name parsing, email normalization, CSV handling, and word-boundary truncation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/csvUtils.ts (1)
54-65: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPrevent formula injection before CSV escaping and cover it end to end.
sanitizeValuecurrently returns before applying formula protection for values containing CSV-special characters, and the tests do not exercise those paths.
packages/lib/csvUtils.ts#L54-L65: prefix formula-leading values before quote/comma/newline escaping.packages/lib/csvUtils.test.ts#L29-L34: add formula cases containing commas, newlines, and quotes.packages/lib/csvUtils.test.ts#L43-L51: add anobjectsToCsvintegration case with a formula-leading field containing a CSV-special character.🤖 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 `@packages/lib/csvUtils.ts` around lines 54 - 65, Update sanitizeValue in packages/lib/csvUtils.ts (lines 54-65) to apply formula-prefix protection before CSV-special-character escaping, preserving correct quoting for values containing commas, newlines, or quotes. Add corresponding formula cases in packages/lib/csvUtils.test.ts (lines 29-34) for each special-character path, and add an objectsToCsv integration case at lines 43-51 using a formula-leading field with a CSV-special character.
🧹 Nitpick comments (5)
packages/lib/csvUtils.test.ts (2)
43-51: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover formula sanitization through
objectsToCsv.Add a formula-leading field containing a comma or quote and assert the serialized CSV contains the prefixed single quote. This verifies the mitigation at the export boundary, not only through
sanitizeValue.🤖 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 `@packages/lib/csvUtils.test.ts` around lines 43 - 51, Extend the objectsToCsv test to include a formula-leading field whose value also contains a comma or quote, then assert the serialized CSV prefixes that field with a single quote and applies normal CSV escaping. Keep the existing newline coverage intact and verify sanitization through objectsToCsv rather than only sanitizeValue.
29-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression cases for formula values with CSV-special characters.
The current formula tests all take the direct sanitization path, so they would pass despite the implementation bug. Add cases where formula-leading values also contain commas, newlines, and quotes.
🤖 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 `@packages/lib/csvUtils.test.ts` around lines 29 - 34, Add regression assertions to the existing “prefixes formula-trigger characters” test in csvUtils.test.ts for values beginning with “=”, “+”, “-”, or “@” that also contain commas, newlines, and double quotes. Verify sanitization preserves the CSV-special characters while prefixing the formula value with a single quote and applying the expected CSV escaping.packages/lib/text.test.ts (1)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the word-boundary result, not only its length.
The current assertions would also pass if truncation happened at an arbitrary character. Add a deterministic case that verifies the last complete word is selected.
Suggested test
+ it("should truncate at the last complete word within maxLength", () => { + expect(truncateOnWord("the quick brown fox", 12)).toBe("the quick..."); + });🤖 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 `@packages/lib/text.test.ts` around lines 7 - 15, Add an explicit expected-output assertion to the test around truncateOnWord, using deterministic input that confirms truncation ends at the last complete word boundary rather than merely satisfying the length limits. Preserve the existing maxLength coverage for both 158 and 100.packages/lib/text.ts (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep comments focused on why, not what.
Lines 10 and 13-14 narrate the implementation rather than explaining its rationale. Rephrase them to describe the bounded candidate and the preference for a complete word.
Proposed comment cleanup
- // First split on maxLength chars + // Bound the candidate before adding the optional ellipsis. let truncatedText = text.substring(0, maxLength); - // Then split on the last space, this way we split on the last word, - // which looks just a bit nicer. + // Prefer a complete word when a boundary is available. const lastSpaceIndex = truncatedText.lastIndexOf(" ");🤖 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 `@packages/lib/text.ts` around lines 10 - 17, Update the comments surrounding the truncatedText and lastSpaceIndex logic to explain the rationale rather than narrate the operations: describe limiting the candidate to maxLength and preferring a complete word boundary. Leave the truncation behavior and identifiers unchanged.Source: Coding guidelines
packages/lib/intervalTree.ts (1)
35-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort once instead of re-sorting every subtree.
This fixes the ordering bug, but each recursive call now performs another sort, making construction O(n log² n). Sort the input once in the constructor and let
buildTreeoperate on already-sorted slices.Proposed refactor
constructor(nodes: IntervalNode<T>[]) { - this.root = this.buildTree([...nodes]); + this.root = this.buildTree([...nodes].sort((a, b) => a.start - b.start)); } private buildTree(nodes: IntervalNode<T>[]): IntervalNode<T> | undefined { if (nodes.length === 0) return undefined; - const sorted = [...nodes].sort((a, b) => a.start - b.start); - const mid = Math.floor(sorted.length / 2); - const node = sorted[mid]; + const mid = Math.floor(nodes.length / 2); + const node = nodes[mid]; - const leftNodes = sorted.slice(0, mid); - const rightNodes = sorted.slice(mid + 1); + const leftNodes = nodes.slice(0, mid); + const rightNodes = nodes.slice(mid + 1);🤖 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 `@packages/lib/intervalTree.ts` around lines 35 - 40, Move the interval-node sorting out of the recursive buildTree flow and perform it once in the constructor before the initial build. Update buildTree to consume already-sorted node slices while preserving the existing midpoint selection and left/right partitioning, so recursive construction avoids repeated sorting.
🤖 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 `@packages/lib/constants.ts`:
- Around line 140-150: Update safeParseCsvEnvVar to parse envVar directly,
accept the JSON result only when it is an array containing strings, and
otherwise use the existing comma-separated fallback. Preserve trimming and quote
removal in the fallback while ensuring the returned value always satisfies
string[].
In `@packages/lib/getIP.ts`:
- Around line 40-45: Update the IP banlist initialization around
banlistSchema.parse and both error-handling paths so malformed IP_BANLIST never
produces an empty fail-open ban list. Validate and surface the configuration
error during startup, or reuse a cached last-known-good ban list; avoid
reparsing and logging the same invalid configuration on every invocation.
In `@packages/lib/jsonUtils.ts`:
- Around line 1-7: Update validJson so parse failures cannot be confused with
valid falsy JSON values such as false, 0, or null; use undefined or a tagged
result for failure, or restore the prior object-only contract. Preserve
returning successfully parsed JSON values without treating them as invalid.
In `@packages/lib/recentImpersonations.ts`:
- Around line 23-27: Update the recent impersonation filtering flow around
getRecentImpersonations so malformed array entries such as null cannot cause
item.username to throw. Sanitize the records in the getter or apply a type guard
before the filter, while preserving valid entries and the existing username
comparison behavior.
---
Outside diff comments:
In `@packages/lib/csvUtils.ts`:
- Around line 54-65: Update sanitizeValue in packages/lib/csvUtils.ts (lines
54-65) to apply formula-prefix protection before CSV-special-character escaping,
preserving correct quoting for values containing commas, newlines, or quotes.
Add corresponding formula cases in packages/lib/csvUtils.test.ts (lines 29-34)
for each special-character path, and add an objectsToCsv integration case at
lines 43-51 using a formula-leading field with a CSV-special character.
---
Nitpick comments:
In `@packages/lib/csvUtils.test.ts`:
- Around line 43-51: Extend the objectsToCsv test to include a formula-leading
field whose value also contains a comma or quote, then assert the serialized CSV
prefixes that field with a single quote and applies normal CSV escaping. Keep
the existing newline coverage intact and verify sanitization through
objectsToCsv rather than only sanitizeValue.
- Around line 29-34: Add regression assertions to the existing “prefixes
formula-trigger characters” test in csvUtils.test.ts for values beginning with
“=”, “+”, “-”, or “@” that also contain commas, newlines, and double quotes.
Verify sanitization preserves the CSV-special characters while prefixing the
formula value with a single quote and applying the expected CSV escaping.
In `@packages/lib/intervalTree.ts`:
- Around line 35-40: Move the interval-node sorting out of the recursive
buildTree flow and perform it once in the constructor before the initial build.
Update buildTree to consume already-sorted node slices while preserving the
existing midpoint selection and left/right partitioning, so recursive
construction avoids repeated sorting.
In `@packages/lib/text.test.ts`:
- Around line 7-15: Add an explicit expected-output assertion to the test around
truncateOnWord, using deterministic input that confirms truncation ends at the
last complete word boundary rather than merely satisfying the length limits.
Preserve the existing maxLength coverage for both 158 and 100.
In `@packages/lib/text.ts`:
- Around line 10-17: Update the comments surrounding the truncatedText and
lastSpaceIndex logic to explain the rationale rather than narrate the
operations: describe limiting the candidate to maxLength and preferring a
complete word boundary. Leave the truncation behavior and identifiers unchanged.
🪄 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 Plus
Run ID: 372529ae-3019-4c67-a5ef-c3ca581c4a55
📒 Files selected for processing (23)
packages/features/bookings/lib/getBookingResponsesSchema.tspackages/lib/CalEventParser.test.tspackages/lib/CalEventParser.tspackages/lib/constants.tspackages/lib/contructEmailFromPhoneNumber.tspackages/lib/csvUtils.test.tspackages/lib/csvUtils.tspackages/lib/errors.tspackages/lib/extract-base-email.test.tspackages/lib/extract-base-email.tspackages/lib/formatPhoneNumber.tspackages/lib/getIP.tspackages/lib/getSafeRedirectUrl.tspackages/lib/hashedLinksUtils.tspackages/lib/hooks/useCompatSearchParams.tspackages/lib/hooks/useInViewObserver.tspackages/lib/intervalTree.tspackages/lib/jsonUtils.tspackages/lib/recentImpersonations.tspackages/lib/safeStringify.tspackages/lib/text.test.tspackages/lib/text.tspackages/lib/timezone.ts
| const safeParseCsvEnvVar = (envVar: string | undefined): string[] => { | ||
| if (!envVar) return []; | ||
| try { | ||
| return JSON.parse(`[${envVar}]`); | ||
| } catch { | ||
| return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, "")); | ||
| } | ||
| }; | ||
|
|
||
| export const ALLOWED_HOSTNAMES = safeParseCsvEnvVar(process.env.ALLOWED_HOSTNAMES); | ||
| export const RESERVED_SUBDOMAINS = safeParseCsvEnvVar(process.env.RESERVED_SUBDOMAINS); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse the environment value itself and validate the result.
Wrapping the value in brackets makes valid JSON arrays become nested arrays (["a"] → [["a"]]), while JSON.parse can also return non-string values despite the string[] annotation. Parse envVar directly, accept only arrays of strings, then use the CSV fallback.
Proposed fix
const safeParseCsvEnvVar = (envVar: string | undefined): string[] => {
if (!envVar) return [];
try {
- return JSON.parse(`[${envVar}]`);
+ const parsed: unknown = JSON.parse(envVar);
+ if (Array.isArray(parsed) && parsed.every((value) => typeof value === "string")) {
+ return parsed;
+ }
} catch {
- return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, ""));
}
+ return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, ""));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const safeParseCsvEnvVar = (envVar: string | undefined): string[] => { | |
| if (!envVar) return []; | |
| try { | |
| return JSON.parse(`[${envVar}]`); | |
| } catch { | |
| return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, "")); | |
| } | |
| }; | |
| export const ALLOWED_HOSTNAMES = safeParseCsvEnvVar(process.env.ALLOWED_HOSTNAMES); | |
| export const RESERVED_SUBDOMAINS = safeParseCsvEnvVar(process.env.RESERVED_SUBDOMAINS); | |
| const safeParseCsvEnvVar = (envVar: string | undefined): string[] => { | |
| if (!envVar) return []; | |
| try { | |
| const parsed: unknown = JSON.parse(envVar); | |
| if (Array.isArray(parsed) && parsed.every((value) => typeof value === "string")) { | |
| return parsed; | |
| } | |
| } catch { | |
| } | |
| return envVar.split(",").map((s) => s.trim().replace(/^"|"$/g, "")); | |
| }; | |
| export const ALLOWED_HOSTNAMES = safeParseCsvEnvVar(process.env.ALLOWED_HOSTNAMES); | |
| export const RESERVED_SUBDOMAINS = safeParseCsvEnvVar(process.env.RESERVED_SUBDOMAINS); |
🤖 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 `@packages/lib/constants.ts` around lines 140 - 150, Update safeParseCsvEnvVar
to parse envVar directly, accept the JSON result only when it is an array
containing strings, and otherwise use the existing comma-separated fallback.
Preserve trimming and quote removal in the fallback while ensuring the returned
value always satisfies string[].
| let banList: string[] = []; | ||
| try { | ||
| banList = banlistSchema.parse(JSON.parse(rawBanListJson)); | ||
| } catch { | ||
| logger.error("Invalid IP_BANLIST JSON; treating as empty banlist"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not fail open when IP_BANLIST is malformed.
Both catch blocks leave banList empty, so an invalid security configuration disables all IP bans. The invalid value is also reparsed and logged on every invocation. Validate this configuration at startup and surface the misconfiguration, or retain a last-known-good list instead of silently treating it as no bans.
Also applies to: 55-60
🤖 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 `@packages/lib/getIP.ts` around lines 40 - 45, Update the IP banlist
initialization around banlistSchema.parse and both error-handling paths so
malformed IP_BANLIST never produces an empty fail-open ban list. Validate and
surface the configuration error during startup, or reuse a cached
last-known-good ban list; avoid reparsing and logging the same invalid
configuration on every invocation.
| export const validJson = (jsonString: string) => { | ||
| try { | ||
| const o = JSON.parse(jsonString); | ||
| if (o && typeof o === "object") { | ||
| return o; | ||
| } | ||
| return JSON.parse(jsonString); | ||
| } catch (e) { | ||
| console.log("Invalid JSON:", e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use an unambiguous parse-failure result.
After accepting every JSON type, validJson("false") returns the same false sentinel as malformed JSON; "0" and "null" are also falsy. Return a tagged result or use undefined for failure, or retain the previous object-only contract.
🤖 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 `@packages/lib/jsonUtils.ts` around lines 1 - 7, Update validJson so parse
failures cannot be confused with valid falsy JSON values such as false, 0, or
null; use undefined or a tagged result for failure, or restore the prior
object-only contract. Preserve returning successfully parsed JSON values without
treating them as invalid.
| const recent = getRecentImpersonations(); | ||
| if (!Array.isArray(recent)) return; | ||
| const username = usernameRaw.trim().toLowerCase(); | ||
| if (!username) return; | ||
| const filtered = recent.filter((item) => item.username !== username); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate array entries before filtering.
getRecentImpersonations() returns raw parsed data, so [null] passes the new array check and item.username then throws. The outer catch silently discards the new impersonation. Validate or sanitize each record in the getter, or use a type guard before filtering.
🤖 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 `@packages/lib/recentImpersonations.ts` around lines 23 - 27, Update the recent
impersonation filtering flow around getRecentImpersonations so malformed array
entries such as null cannot cause item.username to throw. Sanitize the records
in the getter or apply a type guard before the filter, while preserving valid
entries and the existing username comparison behavior.
|
Closing this one. I opened a stack of PRs in this repo today and, because each branch was cut from the previous one instead of from the base, they overlap: this PR carries the commits of the earlier ones as well. I am consolidating the work in #29820 and will resubmit the remaining fixes individually, on top of the base branch, once that one has been reviewed. Sorry for the noise. |
1 participant
Summary
Six bug fixes across Cal.diy libraries and hooks.
constants.tsJSON.parseonALLOWED_HOSTNAMES/RESERVED_SUBDOMAINSat module level crashed app at startuperrors.ts!response.ok && (status < 200 || status >= 300)— same as!response.okrecentImpersonations.ts.filter()crashgetIP.tsisIpInBanListStringlacked try/catch that siblingisIpInBanlisthaduseInViewObserver.tsroot: nullwas treated asdocument.bodyinstead of viewportuseCompatSearchParams.ts||replaced empty string""with""(no-op); should use??