fix: multiple utility bug fixes and hardening across lib packages - #29820
fix: multiple utility bug fixes and hardening across lib packages#29820PedroHenrique0713 wants to merge 4 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.
|
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! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes update five independent behaviors: multiemail duplicate detection avoids mutating parsed input, provider parsing handles missing integration names, CSV sanitization prefixes spreadsheet formula values, base email extraction handles strings without an at-sign, and word-aware truncation respects the supplied maximum length. New tests cover these changes along with CSV escaping, object conversion, provider parsing, email normalization, and text truncation. Merge Risk: ⚪ Minimal · up to This PR makes localized utility bug fixes and hardening changes with targeted tests; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![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 (2)
packages/features/bookings/lib/getBookingResponsesSchema.ts (1)
203-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompare against the sorted copy when detecting duplicates.
The code sorts a copy but compares against the original
emailsarray, so unsorted duplicate inputs can be missed. Store the sorted copy and use it for both values:Proposed fix
- [...emails].sort().some((item, i) => { - if (item === emails[i + 1]) { + const sortedEmails = [...emails].sort(); + sortedEmails.some((item, i) => { + if (item === sortedEmails[i + 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/features/bookings/lib/getBookingResponsesSchema.ts` around lines 203 - 208, Update the duplicate detection in the email validation logic to store the sorted copy of emails and compare each item with the next value from that same sorted array. Keep the existing duplicate_email issue behavior and early termination unchanged.packages/lib/text.ts (1)
11-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve text when no preceding space exists.
When
lastIndexOf(" ")returns-1,substring(0, -1)produces an empty string, sotruncateOnWord("a".repeat(200), 100)returns only"...". Fall back to themaxLengthsubstring when no space is found.Proposed fix
let truncatedText = text.substring(0, maxLength); - truncatedText = truncatedText.substring(0, Math.min(truncatedText.length, truncatedText.lastIndexOf(" "))); + const lastSpaceIndex = truncatedText.lastIndexOf(" "); + if (lastSpaceIndex !== -1) { + truncatedText = truncatedText.substring(0, lastSpaceIndex); + }🤖 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 11 - 15, Update truncateOnWord so it only splits at the last space when one exists; when lastIndexOf(" ") returns -1, retain the initial text.substring(0, maxLength) result instead of producing an empty string.
🧹 Nitpick comments (1)
packages/lib/csvUtils.test.ts (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove assertion-restating comments.
These comments repeat the expectations immediately below; retain only the neighboring rationale about CSV field versus record delimiters.
As per coding guidelines, “Only add code comments that explain why, not what.”
Also applies to: 77-77
🤖 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` at line 59, Remove the assertion-restating comments near the CSV line-count and related assertions in the test cases, including both occurrences. Retain only the neighboring rationale explaining CSV field versus record delimiters.Source: Coding guidelines
🤖 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/csvUtils.test.ts`:
- Around line 21-23: Update sanitizeValue to quote values containing carriage
returns by including \r in its CSV quoting condition alongside the existing
newline check. Preserve current behavior for other values and ensure row
serialization assertions pass.
- Around line 103-105: Update downloadAsCsv to create its Blob with the MIME
type text/csv;charset=utf-8 so it matches the existing createObjectURLSpy
assertion. Preserve the current CSV download behavior and avoid changing the
test expectation unless plain text is explicitly intended.
In `@packages/lib/csvUtils.ts`:
- Around line 60-65: Update the value-sanitization flow in the CSV utility so
formula-triggering prefixes are added before comma/quote CSV escaping, ensuring
values such as =SUM(1,1) remain text after import. Preserve existing escaping
behavior for non-formula values, and add a regression case covering a formula
containing a comma or quote.
In `@packages/lib/text.test.ts`:
- Around line 7-10: Update the test around truncateOnWord to use a 200-character
input containing spaces, then assert the expected truncated content for
maxLength values 158 and 100 rather than only checking length bounds. Ensure the
assertions distinguish honoring each supplied maxLength and prevent the no-space
fallback from passing with just "...".
---
Outside diff comments:
In `@packages/features/bookings/lib/getBookingResponsesSchema.ts`:
- Around line 203-208: Update the duplicate detection in the email validation
logic to store the sorted copy of emails and compare each item with the next
value from that same sorted array. Keep the existing duplicate_email issue
behavior and early termination unchanged.
In `@packages/lib/text.ts`:
- Around line 11-15: Update truncateOnWord so it only splits at the last space
when one exists; when lastIndexOf(" ") returns -1, retain the initial
text.substring(0, maxLength) result instead of producing an empty string.
---
Nitpick comments:
In `@packages/lib/csvUtils.test.ts`:
- Line 59: Remove the assertion-restating comments near the CSV line-count and
related assertions in the test cases, including both occurrences. Retain only
the neighboring rationale explaining CSV field versus record delimiters.
🪄 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: 1dcc24e7-7574-4434-86fb-4094426712d9
📒 Files selected for processing (9)
packages/features/bookings/lib/getBookingResponsesSchema.tspackages/lib/CalEventParser.test.tspackages/lib/CalEventParser.tspackages/lib/csvUtils.test.tspackages/lib/csvUtils.tspackages/lib/extract-base-email.test.tspackages/lib/extract-base-email.tspackages/lib/text.test.tspackages/lib/text.ts
| it("wraps values with carriage returns in quotes", () => { | ||
| expect(sanitizeValue("hello\rworld")).toBe('"hello\rworld"'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle lone carriage returns in the implementation.
This test currently fails because sanitizeValue checks \n, but not \r, and returns hello\rworld unquoted. Include \r in the CSV quoting condition; this also fixes the row assertion at lines 52-65.
🤖 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 21 - 23, Update sanitizeValue to
quote values containing carriage returns by including \r in its CSV quoting
condition alongside the existing newline check. Preserve current behavior for
other values and ensure row serialization assertions pass.
| expect(createObjectURLSpy).toHaveBeenCalledTimes(1); | ||
| const blob = createObjectURLSpy.mock.calls[0][0] as Blob; | ||
| expect(blob.type).toBe("text/csv;charset=utf-8"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the Blob MIME type with this assertion.
downloadAsCsv currently creates a text/plain Blob, so this assertion fails. Update it to text/csv;charset=utf-8, or change the expectation if plain text is intentional.
🤖 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 103 - 105, Update downloadAsCsv
to create its Blob with the MIME type text/csv;charset=utf-8 so it matches the
existing createObjectURLSpy assertion. Preserve the current CSV download
behavior and avoid changing the test expectation unless plain text is explicitly
intended.
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.
There was a problem hiding this comment.
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.test.ts (1)
29-34: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCover formula values that also require CSV quoting.
These tests only cover formula triggers without commas, newlines, or quotes. In
packages/lib/csvUtils.ts, CSV quoting runs before formula-prefix protection, so values such as=HYPERLINK("http://evil.com")or=SUM(A1,A10)can still parse as formulas. Add regression cases and apply the apostrophe before CSV escaping.🤖 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, Update sanitizeValue in csvUtils.ts to apply the apostrophe formula-protection prefix before CSV escaping. Add regression coverage for formula values containing commas, newlines, and embedded quotes, such as hyperlink and SUM expressions, and verify the resulting values remain safely quoted while retaining the prefix.
🤖 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.
Outside diff comments:
In `@packages/lib/csvUtils.test.ts`:
- Around line 29-34: Update sanitizeValue in csvUtils.ts to apply the apostrophe
formula-protection prefix before CSV escaping. Add regression coverage for
formula values containing commas, newlines, and embedded quotes, such as
hyperlink and SUM expressions, and verify the resulting values remain safely
quoted while retaining the prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61ba866e-0e9f-4a34-8d04-f884eb85dd15
📒 Files selected for processing (4)
packages/features/bookings/lib/getBookingResponsesSchema.tspackages/lib/csvUtils.test.tspackages/lib/text.test.tspackages/lib/text.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/lib/text.ts
- packages/features/bookings/lib/getBookingResponsesSchema.ts
- packages/lib/text.test.ts
|
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. |
|
Still active. State verified today:
The five fixes are independent of each other (19 test cases across 4 files):
Two things I can act on:
|
|
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. |
The =/+/-/@ guard ran after the quoting branches, so a formula that also contains a comma or a quote was returned quoted but without the ' prefix and is still evaluated by Excel/Sheets. CSV quoting is a delimiter rule, not an escape the spreadsheet honours. Adds a test covering =SUM(1,1), a quoted HYPERLINK payload and a formula containing a newline. It fails on the previous implementation.
|
Just pushed the fix for the incomplete formula-injection guard CodeRabbit flagged. The The rest of the PR is unchanged and still applies to
State: Worth knowing before merge: this PR and #29783 both add |
|
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. |
csvUtils.ts was being edited by this PR and by calcom#29783 at the same time, and the two conflict on sanitizeValue. calcom#29783 is the PR dedicated to that file, so the formula-injection guard and its tests now live there and this PR keeps the four unrelated fixes. This also retires the two review comments on csvUtils.test.ts: the file is no longer part of this PR.
sanitizeValue was being changed by two open PRs at once (calcom#29783 and calcom#29820), which conflict on this exact block. Consolidating both csvUtils changes here so one PR owns the function: line breaks (LF/CR/CRLF) and the Blob MIME type, plus the OWASP formula-injection prefix that calcom#29820 carried. The prefix runs before quoting on purpose: quoting is a CSV delimiter, not an escape a spreadsheet honours, so a formula that also contains a comma would come back merely quoted and still be evaluated.
|
Removed the csvUtils changes from this PR and moved them to #29783, which is the PR dedicated to that file. The two were editing That also retires the two open review comments on What remains here is the four unrelated fixes: |
1 participant
Summary
Five bug fixes across utility libraries, each with a test that fails before the fix and passes after.
1.
truncateOnWordignoresmaxLengthparameter (packages/lib/text.ts)The function accepted a
maxLengthparameter but used a hardcoded148instead. Callers passingmaxLength=158(e.g., OpenGraph meta description truncation) were silently getting 148-character truncation. Fix: usemaxLengthinstead of148.2.
getProviderNamecrashes on empty location (packages/lib/CalEventParser.ts)When location was
"integrations:""(nothing after the colon),location.split(":")[1]returned""and""[0].toUpperCase()threw TypeError. Fix: return empty string when provider name is empty.3. CSV formula injection (
packages/lib/csvUtils.ts)Values starting with
=,+,-,@that contained no commas/line breaks were passed through unquoted. When opened in Excel/Sheets, these execute as formulas (OWASP CSV Injection). Fix: prefix formula-trigger characters with single quote to force text rendering.4.
extractBaseEmailproduces"local@undefined"(packages/lib/extract-base-email.ts)Without an
@sign,email.split("@")produceddomain = undefined, which template literal converted to the string"undefined". This function is called from 21 call sites. Fix: guard for missing@and return input unchanged.5.
sort()mutates input array in validation (packages/features/bookings/lib/getBookingResponsesSchema.ts:203)emails.sort()was called in-place on the parsed data inside a ZodsuperRefine, mutating the input array during validation. Fix: copy before sorting with[...emails].sort().Testing
npx vitest run packages/lib/text.test.ts packages/lib/csvUtils.test.ts packages/lib/CalEventParser.test.ts packages/lib/extract-base-email.test.ts— 30/30 passing