Enhance email body rendering by auto-linking URLs and email addresses #16449
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/activities/emails/components/EmailThreadMessageBody.tsx">
<violation number="1" location="packages/twenty-front/src/modules/activities/emails/components/EmailThreadMessageBody.tsx:46">
P0: Critical XSS vulnerability: Email body content is rendered via `dangerouslySetInnerHTML` without sanitization. The `autoLink` function only adds anchor tags but doesn't sanitize the input. Consider using DOMPurify (already used in the server-side code) to sanitize the HTML before rendering:
```typescript
import DOMPurify from 'dompurify';
// ...
const sanitized = DOMPurify.sanitize(autoLink(body));
```</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| <AnimatedEaseInOut isOpen={isDisplayed} duration="fast"> | ||
| <StyledThreadMessageBody>{body}</StyledThreadMessageBody> | ||
| <StyledThreadMessageBody | ||
| dangerouslySetInnerHTML={{ __html: autoLink(body) }} |
There was a problem hiding this comment.
P0: Critical XSS vulnerability: Email body content is rendered via dangerouslySetInnerHTML without sanitization. The autoLink function only adds anchor tags but doesn't sanitize the input. Consider using DOMPurify (already used in the server-side code) to sanitize the HTML before rendering:
import DOMPurify from 'dompurify';
// ...
const sanitized = DOMPurify.sanitize(autoLink(body));Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/activities/emails/components/EmailThreadMessageBody.tsx, line 46:
<comment>Critical XSS vulnerability: Email body content is rendered via `dangerouslySetInnerHTML` without sanitization. The `autoLink` function only adds anchor tags but doesn't sanitize the input. Consider using DOMPurify (already used in the server-side code) to sanitize the HTML before rendering:
```typescript
import DOMPurify from 'dompurify';
// ...
const sanitized = DOMPurify.sanitize(autoLink(body));
```</comment>
<file context>
@@ -15,14 +15,36 @@ type EmailThreadMessageBodyProps = {
<AnimatedEaseInOut isOpen={isDisplayed} duration="fast">
- <StyledThreadMessageBody>{body}</StyledThreadMessageBody>
+ <StyledThreadMessageBody
+ dangerouslySetInnerHTML={{ __html: autoLink(body) }}
+ />
</AnimatedEaseInOut>
</file context>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances email body rendering by automatically converting plain text URLs and email addresses into clickable hyperlinks using regex pattern matching. The implementation adds an autoLink utility function that processes email body text before rendering.
Key Changes:
- Added
autoLinkfunction with regex patterns to detect and linkify URLs and email addresses - Modified
EmailThreadMessageBodyto usedangerouslySetInnerHTMLfor rendering auto-linked content
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
+47
| const autoLink = (text: string): string => { | ||
| const urlRegex = | ||
| /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi; | ||
| const emailRegex = /([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})/gi; | ||
|
|
||
| let linked = text.replace(urlRegex, (url) => { | ||
| let href = url; | ||
| if (!href.startsWith('http')) { | ||
| href = 'https://' + href; | ||
| } | ||
| return `<a href="${href}" target="_blank" rel="noopener noreferrer">${url}</a>`; | ||
| }); | ||
|
|
||
| linked = linked.replace(emailRegex, (email) => { | ||
| if (linked.includes(`>${email}<`)) return email; | ||
| return `<a href="mailto:${email}">${email}</a>`; | ||
| }); | ||
|
|
||
| return linked; | ||
| }; | ||
|
|
||
| export const EmailThreadMessageBody = ({ | ||
| body, | ||
| isDisplayed, | ||
| }: EmailThreadMessageBodyProps) => { | ||
| return ( | ||
| <AnimatedEaseInOut isOpen={isDisplayed} duration="fast"> | ||
| <StyledThreadMessageBody>{body}</StyledThreadMessageBody> | ||
| <StyledThreadMessageBody | ||
| dangerouslySetInnerHTML={{ __html: autoLink(body) }} | ||
| /> |
There was a problem hiding this comment.
Critical XSS Vulnerability: Using dangerouslySetInnerHTML with unsanitized user input creates a Cross-Site Scripting (XSS) vulnerability. The email body could contain malicious HTML/JavaScript that will be executed in the browser.
For example, if the email body contains:
<script>alert('XSS')</script>
<img src=x onerror="alert('XSS')">
These scripts would execute when the email is displayed.
Recommendation: Sanitize the HTML before rendering using a library like DOMPurify:
import DOMPurify from 'dompurify';
const autoLink = (text: string): string => {
// ... existing autoLink logic ...
// Sanitize the final HTML to prevent XSS
return DOMPurify.sanitize(linked, {
ALLOWED_TAGS: ['a'],
ALLOWED_ATTR: ['href', 'target', 'rel']
});
};Or alternatively, escape HTML entities in the input text before auto-linking:
const escapeHtml = (text: string): string => {
return text
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
.replace(/'/g, ''');
};
const autoLink = (text: string): string => {
const escaped = escapeHtml(text);
// ... then apply regex replacements on escaped text ...
};
Comment on lines
+19
to
+36
| const urlRegex = | ||
| /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi; | ||
| const emailRegex = /([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})/gi; | ||
|
|
||
| let linked = text.replace(urlRegex, (url) => { | ||
| let href = url; | ||
| if (!href.startsWith('http')) { | ||
| href = 'https://' + href; | ||
| } | ||
| return `<a href="${href}" target="_blank" rel="noopener noreferrer">${url}</a>`; | ||
| }); | ||
|
|
||
| linked = linked.replace(emailRegex, (email) => { | ||
| if (linked.includes(`>${email}<`)) return email; | ||
| return `<a href="mailto:${email}">${email}</a>`; | ||
| }); | ||
|
|
||
| return linked; |
There was a problem hiding this comment.
The method used to avoid double-linking emails is inefficient and unreliable. The check if (linked.includes(\>${email}<`))` performs a simple string search which:
- Can fail if the email appears in a URL that was already linked (e.g.,
https://[email protected]) - Doesn't account for emails that might be in attributes rather than just between tags
- Is a performance issue as it searches the entire string for each email match
Recommendation: Track positions or use a more robust approach such as:
- Parse and process the text in a single pass
- Use negative lookahead/lookbehind in the regex to avoid already-linked text
- Or track already-replaced positions to skip them
Example with tracking positions:
const autoLink = (text: string): string => {
const replacements: Array<{start: number, end: number, replacement: string}> = [];
// Collect URL replacements
let match;
const urlRegex = /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi;
while ((match = urlRegex.exec(text)) !== null) {
let href = match[0];
if (!href.startsWith('http')) {
href = 'https://' + href;
}
replacements.push({
start: match.index,
end: match.index + match[0].length,
replacement: \`<a href="\${href}" target="_blank" rel="noopener noreferrer">\${match[0]}</a>\`
});
}
// Collect email replacements (skip overlapping positions)
const emailRegex = /([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})/gi;
while ((match = emailRegex.exec(text)) !== null) {
const isOverlapping = replacements.some(r =>
match.index >= r.start && match.index < r.end
);
if (!isOverlapping) {
replacements.push({
start: match.index,
end: match.index + match[0].length,
replacement: \`<a href="mailto:\${match[0]}">\${match[0]}</a>\`
});
}
}
// Apply replacements in reverse order
replacements.sort((a, b) => b.start - a.start);
let result = text;
for (const r of replacements) {
result = result.slice(0, r.start) + r.replacement + result.slice(r.end);
}
return result;
};
Suggested change
| const urlRegex = | |
| /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi; | |
| const emailRegex = /([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})/gi; | |
| let linked = text.replace(urlRegex, (url) => { | |
| let href = url; | |
| if (!href.startsWith('http')) { | |
| href = 'https://' + href; | |
| } | |
| return `<a href="${href}" target="_blank" rel="noopener noreferrer">${url}</a>`; | |
| }); | |
| linked = linked.replace(emailRegex, (email) => { | |
| if (linked.includes(`>${email}<`)) return email; | |
| return `<a href="mailto:${email}">${email}</a>`; | |
| }); | |
| return linked; | |
| const replacements: Array<{start: number, end: number, replacement: string}> = []; | |
| let match; | |
| const urlRegex = /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi; | |
| const emailRegex = /([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})/gi; | |
| // Collect URL replacements | |
| while ((match = urlRegex.exec(text)) !== null) { | |
| let href = match[0]; | |
| if (!href.startsWith('http')) { | |
| href = 'https://' + href; | |
| } | |
| replacements.push({ | |
| start: match.index, | |
| end: match.index + match[0].length, | |
| replacement: `<a href="${href}" target="_blank" rel="noopener noreferrer">${match[0]}</a>` | |
| }); | |
| } | |
| // Collect email replacements (skip overlapping positions) | |
| while ((match = emailRegex.exec(text)) !== null) { | |
| const isOverlapping = replacements.some(r => | |
| match.index < r.end && (match.index + match[0].length) > r.start | |
| ); | |
| if (!isOverlapping) { | |
| replacements.push({ | |
| start: match.index, | |
| end: match.index + match[0].length, | |
| replacement: `<a href="mailto:${match[0]}">${match[0]}</a>` | |
| }); | |
| } | |
| } | |
| // Apply replacements in reverse order | |
| replacements.sort((a, b) => b.start - a.start); | |
| let result = text; | |
| for (const r of replacements) { | |
| result = result.slice(0, r.start) + r.replacement + result.slice(r.end); | |
| } | |
| return result; |
Comment on lines
+19
to
+20
| const urlRegex = | ||
| /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi; |
There was a problem hiding this comment.
The URL regex pattern may not correctly handle all valid URL formats. Specific issues:
- Missing protocol-less URLs: The pattern requires either
https?://orwww., so URLs likeexample.com/pathwon't be matched - Query parameters with special characters: Characters like
()are common in URLs but not included in the character class - Fragment identifiers: The
#is only allowed in the middle but valid URLs can have fragments like#section
Recommendation: Consider using a more comprehensive URL regex or a dedicated URL parsing library. For example:
const urlRegex = /\b(?:https?:\/\/|www\.)[^\s<>"{}|\^`\[\]]+/gi;Or for a more robust solution, use a library that properly validates and parses URLs to avoid false positives.
Suggested change
| const urlRegex = | |
| /((https?:\/\/|www\.)[\w-]+(\.[\w-]+)+([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?)/gi; | |
| // Improved URL regex: matches http(s), www, and protocol-less domain URLs, allows more special characters | |
| const urlRegex = | |
| /\b((?:https?:\/\/|www\.)[^\s<>"{}|\^`\[\]]+|(?:[a-zA-Z0-9-]+\.)+[a-zA-Z]{2,}(?:\/[^\s<>"{}|\^`\[\]]*)?)/gi; |
packages/twenty-front/src/modules/activities/emails/components/EmailThreadMessageBody.tsx
Show resolved
Hide resolved
Contributor
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:13112 This environment will automatically shut down when the PR is closed or after 5 hours. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes Issue #16396
Uses regex to fix the rendering