Skip to content

fix(twenty-shared): stop decoding URI components in URL normalization#18699

Closed
sonarly[bot] wants to merge 1 commit intomainfrom
sonarly-15564-url-encoded-characters-incorrectly-decoded-in
Closed

fix(twenty-shared): stop decoding URI components in URL normalization#18699
sonarly[bot] wants to merge 1 commit intomainfrom
sonarly-15564-url-encoded-characters-incorrectly-decoded-in

Conversation

@sonarly
Copy link
Copy Markdown
Contributor

@sonarly sonarly bot commented Mar 17, 2026

Automated fix for bug 15564

Severity: high

Summary

The lowercaseUrlOriginAndRemoveTrailingSlash function applies decodeURIComponent to URL pathname and search components, converting intentionally encoded characters like %2F to /, which breaks URLs that depend on encoding (e.g., Google Maps).

User Impact

Any user storing URLs with percent-encoded characters in link fields (via import, direct edit, or API) gets corrupted URLs. Google Maps links, encoded LinkedIn paths, and any URL with meaningful percent-encoding are broken and unclickable.

Root Cause

Proximate cause: In packages/twenty-shared/src/utils/url/lowercaseUrlOriginAndRemoveTrailingSlash.ts (lines 14-15), the function wraps url.pathname and url.search with safeDecodeURIComponent():

const path =
    safeDecodeURIComponent(url.pathname) +
    safeDecodeURIComponent(url.search) +
    url.hash;

safeDecodeURIComponent calls decodeURIComponent(), which converts %2F/, %20 → space, etc. For URLs where encoding is semantically meaningful (like Google Maps' data= parameter containing %2Fg%2F), this decoding produces a broken URL that the target service cannot parse.

This function is invoked in two critical paths:

  1. Server-side on every link field writetransform-links-value.util.ts calls lowercaseUrlOriginAndRemoveTrailingSlash() on both primaryLinkUrl and all secondaryLinks URLs
  2. During spreadsheet/CSV importbuildRecordFromImportedStructuredRow.ts uses it as the transform for primaryLinkUrl

The original function (commit d916ec0af9) used url.pathname + url.search + url.hash without decoding, which correctly preserved percent-encoding.

Triggering cause: Commit 1119e3d77e ("fix(twenty-shared): preserve special characters in URLs", merged 2025-12-15, PR #16312) introduced safeDecodeURIComponent wrapping. The stated intent was to handle special characters like %C3%A9 (accented characters) but the fix over-corrected by decoding ALL percent-encoded characters, including structurally significant ones like %2F (encoded forward slash). The test added in this commit explicitly asserts that %2F is decoded to / (test case: "should handle mixed encoded and non-encoded in same URL"), confirming this was an intentional but incorrect design decision.

Every URL with meaningful percent-encoding stored after 2025-12-15 is affected.

Introduced by: asasin235 on 2025-12-15 in commit 1119e3d

fix(twenty-shared): preserve special characters in URLs (#16312)

Suggested Fix

Removed safeDecodeURIComponent() wrapping from url.pathname and url.search in lowercaseUrlOriginAndRemoveTrailingSlash. This function was added in commit 1119e3d77e to handle accented characters, but new URL() already normalizes non-ASCII characters in pathnames (encoding é as %C3%A9). The decodeURIComponent call was incorrectly decoding structurally significant percent-encoded characters like %2F (encoded slash) back to /, breaking URLs that depend on encoding (e.g., Google Maps data= parameter).

The fix restores the original url.pathname + url.search + url.hash concatenation, which correctly preserves all percent-encoding as provided by the URL constructor. The safeDecodeURIComponent utility itself is kept since it's still used by the IMAP message text extractor service.

Updated test expectations to match correct URL normalization behavior:

  • Percent-encoded characters like %2F, %20, %2520 are preserved as-is
  • Non-ASCII characters are encoded by the URL constructor (e.g., é%C3%A9)
  • Added a Google Maps URL test case to prevent regression

Generated by Sonarly

https://sonarly.com/issue/15564?type=bug

The `lowercaseUrlOriginAndRemoveTrailingSlash` function applies `decodeURIComponent` to URL pathname and search components, converting intentionally encoded characters like `%2F` to `/`, which breaks URLs that depend on encoding (e.g., Google Maps).

Fix: Removed `safeDecodeURIComponent()` wrapping from `url.pathname` and `url.search` in `lowercaseUrlOriginAndRemoveTrailingSlash`. This function was added in commit `1119e3d77e` to handle accented characters, but `new URL()` already normalizes non-ASCII characters in pathnames (encoding `é` as `%C3%A9`). The `decodeURIComponent` call was incorrectly decoding structurally significant percent-encoded characters like `%2F` (encoded slash) back to `/`, breaking URLs that depend on encoding (e.g., Google Maps `data=` parameter).

The fix restores the original `url.pathname + url.search + url.hash` concatenation, which correctly preserves all percent-encoding as provided by the URL constructor. The `safeDecodeURIComponent` utility itself is kept since it's still used by the IMAP message text extractor service.

Updated test expectations to match correct URL normalization behavior:
- Percent-encoded characters like `%2F`, `%20`, `%2520` are preserved as-is
- Non-ASCII characters are encoded by the URL constructor (e.g., `é` → `%C3%A9`)
- Added a Google Maps URL test case to prevent regression
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

@BOHEUS
Copy link
Copy Markdown
Collaborator

BOHEUS commented Mar 18, 2026

Fixes #18698

@charlesBochet
Copy link
Copy Markdown
Member

Fixed by: #18792

Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants