✨(frontend) Add a link preview modal#651
✨(frontend) Add a link preview modal#651ValentinRegnault wants to merge 3 commits intosuitenumerique:mainfrom
Conversation
Instead of opening the link directly when clicked on in an email, a modal now shows the true URL and asks to confirm. Signed-off-by: Valentin Regnault <valentinregnault22@gmail.com>
📝 WalkthroughWalkthroughAdds a link-confirmation flow: clicking email links opens a modal that shows the resolved/true URL and requests user confirmation before navigating. Includes i18n entries, styling, a new modal component + hook, and iframe link interception integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Iframe as Message iframe
participant Thread as ThreadMessageBody
participant Modal as LinkPreviewModal / Hook
participant Browser as Browser (window/tab)
Iframe->>Thread: user clicks anchor
Thread->>Thread: preventDefault, compute href & visible text
Thread->>Modal: askConfirmation(href, hardWarning?, linkText)
Modal-->>Thread: await user decision (promise)
alt user confirms
Thread->>Browser: window.open(href, _blank)
else user cancels or closes
Thread-->>Thread: no navigation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/public/locales/common/en-US.json (1)
559-574:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the grammar in the new warning copy.
The English security prompt still reads awkwardly:
This links redirects to :andunsafe :both look like mistakes in the UI. Tightening that wording will make the warning feel more trustworthy.✏️ Suggested copy update
- "The link you clicked is probably unsafe :": "The link you clicked is probably unsafe :", + "The link you clicked is probably unsafe :": "The link you clicked is probably unsafe:", ... - "This links redirects to :": "This links redirects to :", + "This links redirects to :": "This link redirects to:",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/public/locales/common/en-US.json` around lines 559 - 574, Update the two awkward English strings in the locale file: change the value for the key "The link you clicked is probably unsafe :" to a cleaner sentence like "The link you clicked may be unsafe." and change the value for "This links redirects to :" to "This link redirects to:" (fixing grammar and removing the stray space before the colon). Edit those exact JSON keys in src/frontend/public/locales/common/en-US.json so the UI displays the corrected copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 21: The changelog entry "Add a link preview modal : instead of opening
the link directly when clicked on in an email, a modal now shows the true URL
and asks to confirm." was added under the released "0.5.0" section; move this
bullet up into the "[Unreleased]" section instead so it appears in unreleased
changes rather than the published 0.5.0 release.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx`:
- Around line 333-349: Remove the conditional that skips the confirmation for
links whose label equals the href and always intercept clicks: in the click
handler attached in the useEffect (the doc.querySelectorAll('a').forEach block)
always call e.preventDefault() and invoke handleLinkClick; update
handleLinkClick to pass a dynamic hardWarning flag to askLinkConfirmation (e.g.,
askLinkConfirmation(url, text !== url, text)) so disguised links (where text !==
url) trigger the stronger phishing-warning path instead of hardcoding false.
- Around line 348-349: The window.open call in thread-message-body.tsx currently
uses window.open(url, '_blank') which is vulnerable to tabnabbing; update the
call inside the decision branch (where you check if (decision)) to include the
noopener and noreferrer flags (use window.open with the feature string
"noopener,noreferrer") so the newly opened page cannot access window.opener and
referrer information.
---
Outside diff comments:
In `@src/frontend/public/locales/common/en-US.json`:
- Around line 559-574: Update the two awkward English strings in the locale
file: change the value for the key "The link you clicked is probably unsafe :"
to a cleaner sentence like "The link you clicked may be unsafe." and change the
value for "This links redirects to :" to "This link redirects to:" (fixing
grammar and removing the stray space before the colon). Edit those exact JSON
keys in src/frontend/public/locales/common/en-US.json so the UI displays the
corrected copy.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 11e8febb-7f82-4f81-a96c-534ad3c0481b
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/link-preview-modal.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx
| doc.querySelectorAll('a').forEach(link => { | ||
| link.addEventListener('click', (e) => { | ||
| if (link.href != link.textContent) { | ||
| e.preventDefault(); | ||
| handleLinkClick(link.href, link.textContent); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
| onLoad?.(); | ||
| }, [onLoad, resizeIframe]); | ||
|
|
||
| const handleLinkClick = async (url: string, text: string) => { | ||
| const lang = i18n.language.split('-')[0]; // Get base language for documentation link | ||
| const decision = await askLinkConfirmation(url, false, text); | ||
| if (decision) { | ||
| window.open(url, '_blank'); |
There was a problem hiding this comment.
Gate every email link behind the confirmation flow.
if (link.href != link.textContent) means links whose label already matches the URL still open directly from the iframe, which bypasses the new protection entirely. On top of that, askLinkConfirmation(url, false, text) hardcodes hardWarning to false, so the phishing-warning path never appears for disguised links either.
🛡️ Suggested fix
// Handle link clicks
doc.querySelectorAll('a').forEach(link => {
link.addEventListener('click', (e) => {
- if (link.href != link.textContent) {
- e.preventDefault();
- handleLinkClick(link.href, link.textContent);
- }
+ e.preventDefault();
+ const visibleText = link.textContent?.trim() ?? '';
+ const isMaskedLink = visibleText !== '' && visibleText !== link.href;
+ void handleLinkClick(link.href, visibleText, isMaskedLink);
});
});
}
onLoad?.();
}, [onLoad, resizeIframe]);
- const handleLinkClick = async (url: string, text: string) => {
- const lang = i18n.language.split('-')[0]; // Get base language for documentation link
- const decision = await askLinkConfirmation(url, false, text);
+ const handleLinkClick = async (url: string, text: string, hardWarning: boolean) => {
+ const decision = await askLinkConfirmation(url, hardWarning, text);
if (decision) {
window.open(url, '_blank');
}
}📝 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.
| doc.querySelectorAll('a').forEach(link => { | |
| link.addEventListener('click', (e) => { | |
| if (link.href != link.textContent) { | |
| e.preventDefault(); | |
| handleLinkClick(link.href, link.textContent); | |
| } | |
| }); | |
| }); | |
| } | |
| onLoad?.(); | |
| }, [onLoad, resizeIframe]); | |
| const handleLinkClick = async (url: string, text: string) => { | |
| const lang = i18n.language.split('-')[0]; // Get base language for documentation link | |
| const decision = await askLinkConfirmation(url, false, text); | |
| if (decision) { | |
| window.open(url, '_blank'); | |
| doc.querySelectorAll('a').forEach(link => { | |
| link.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| const visibleText = link.textContent?.trim() ?? ''; | |
| const isMaskedLink = visibleText !== '' && visibleText !== link.href; | |
| void handleLinkClick(link.href, visibleText, isMaskedLink); | |
| }); | |
| }); | |
| } | |
| onLoad?.(); | |
| }, [onLoad, resizeIframe]); | |
| const handleLinkClick = async (url: string, text: string, hardWarning: boolean) => { | |
| const lang = i18n.language.split('-')[0]; // Get base language for documentation link | |
| const decision = await askLinkConfirmation(url, hardWarning, text); | |
| if (decision) { | |
| window.open(url, '_blank'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx`
around lines 333 - 349, Remove the conditional that skips the confirmation for
links whose label equals the href and always intercept clicks: in the click
handler attached in the useEffect (the doc.querySelectorAll('a').forEach block)
always call e.preventDefault() and invoke handleLinkClick; update
handleLinkClick to pass a dynamic hardWarning flag to askLinkConfirmation (e.g.,
askLinkConfirmation(url, text !== url, text)) so disguised links (where text !==
url) trigger the stronger phishing-warning path instead of hardcoding false.
There was a problem hiding this comment.
if (link.href !== link.textContent): This is expected: if the user clicks on https://example.com, then he already knows he is opening example.com and doesn't need a modal, whereas if he clicks on https://example.com or on I'll take advantage of it!, then the modal is useful.
hardWarning: the modal has two flavors, normal ask for confirmation modal and hard warning "this is probably malicious". This second flavour could be used when Quad9 DNS filtering blocks the domain, but that's not implemented yet.
There was a problem hiding this comment.
@ValentinRegnault, understood — both points are clear now:
-
link.href !== link.textContentguard — The conditional is intentional. When the visible text already matches the URL, the user is aware of the destination and the modal adds no value. The confirmation flow is specifically meant for masked links (where visible text ≠ actual href), which is where the phishing risk lies. -
hardWarninghardcoded tofalse— Also intentional. ThehardWarningpath is a placeholder for the future Quad9 DNS-filtering integration; since that feature isn't implemented yet, it should stayfalsefor now.
My concerns are withdrawn. Thanks for the context!
✏️ Learnings added
Learnt from: ValentinRegnault
Repo: suitenumerique/messages PR: 651
File: src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx:333-349
Timestamp: 2026-05-01T09:44:59.483Z
Learning: In `src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx`, the `if (link.href !== link.textContent)` guard before showing the link-preview confirmation modal is intentional: when the link's visible text already matches the href, no modal is shown because the user already knows the destination. The modal is only triggered for "masked" links where the visible text differs from the actual URL.
Learnt from: ValentinRegnault
Repo: suitenumerique/messages PR: 651
File: src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx:333-349
Timestamp: 2026-05-01T09:44:59.483Z
Learning: In `src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx`, `askLinkConfirmation(url, false, text)` hardcodes `hardWarning` to `false` intentionally. The `hardWarning: true` flavor of the link-preview modal is reserved for a future Quad9 DNS filtering integration that will flag probably-unsafe domains; it should not be set to `true` until that feature is implemented.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jbpenrath
Repo: suitenumerique/messages PR: 639
File: src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx:237-279
Timestamp: 2026-04-23T13:28:38.440Z
Learning: `src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx` is a fork of `gouvfr-lasuite/ui-kit` v0.20.0's `ShareModal` component. The known reference-equality bug in `pendingInvitationUsers.includes(user)` already exists in the upstream ui-kit implementation and should be fixed there, not in the fork. Do not flag this issue in future reviews of this file; it is intentionally left to be addressed when the changes are upstreamed to ui-kit.
Learnt from: jbpenrath
Repo: suitenumerique/messages PR: 464
File: src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-mailbox-signature/index.tsx:65-71
Timestamp: 2026-01-12T13:16:20.666Z
Learning: In Zod 4, replace the deprecated message option with the error parameter for error customization. Use syntax like z.string().min(1, { error: "message" }) instead of { message: "message" }. Also, error maps can return plain strings rather than { message: string } objects. Apply this guidance to all TSX files that use Zod validation in this codebase (src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-mailbox-signature/index.tsx and similar).
Learnt from: jbpenrath
Repo: suitenumerique/messages PR: 512
File: src/frontend/src/features/forms/hooks/use-attachments.tsx:161-192
Timestamp: 2026-02-09T18:20:49.098Z
Learning: In TSX files that use the centralized fetchAPI wrapper, do not add explicit HTTP status checks (e.g., if (response.status >= 400)) after API calls, since non-ok responses throw an APIError which should be handled in existing catch blocks. Ensure downstream error handling relies on catching APIError and presenting user-friendly messages, rather than manual status checks.
Learnt from: jbpenrath
Repo: suitenumerique/messages PR: 561
File: src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsx:32-35
Timestamp: 2026-03-11T17:06:41.542Z
Learning: In the suitenumerique/messages frontend, React Query is configured with a global error handler that shows an error toast for mutation failures. Therefore, individual call sites (e.g., thread-message-actions.tsx) don't need per-call try/catch blocks for error handling. Rely on the global handler for feedback on failures. Do still handle success states locally if needed and avoid duplicating error UI logic. This guideline applies to all TSX files in the frontend where React Query mutations are used.
| if (decision) { | ||
| window.open(url, '_blank'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the file exists and examine the specific lines
if [ -f "src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx" ]; then
echo "File exists"
wc -l "src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx"
echo "--- Lines 340-360 ---"
sed -n '340,360p' "src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx"
else
echo "File not found"
fiRepository: suitenumerique/messages
Length of output: 991
🏁 Script executed:
# Search for all window.open calls in the codebase to identify similar patterns
rg "window\.open\s*\(" --type tsx --type ts -nRepository: suitenumerique/messages
Length of output: 95
🌐 Web query:
window.open noopener noreferrer security tabnabbing
💡 Result:
For tabnabbing protection when opening a new tab/window with JavaScript, the key issue is whether the newly opened page gets access to the opener via window.opener. 1) The vulnerability (tabnabbing / reverse tabnabbing) - If window.opener is available, the opened (possibly malicious) page can navigate the original page (the opener), e.g. by changing opener.location, which enables phishing-style attacks. - OWASP describes reverse tabnabbing as being possible when the source page opens another page (e.g., via target="_blank" without preventative measures), allowing the linked page to rewrite the target page. 2) How to prevent it with window.open (not just links) A) Use the noopener window feature in window.open - Modern browsers support a “noopener” feature in the third argument of window.open. - MDN states: if noopener is set, the new window will not have access to the originating window via Window.opener and it returns null. Example: const w = window.open('https://example.com', '_blank', 'noopener'); B) Use noreferrer if you also want to suppress the Referer header - MDN states: if “noreferrer” is set in window.open features, the browser omits the Referer header and also sets noopener to true. Example: window.open('https://example.com', '_blank', 'noreferrer'); C) Fallback / defense-in-depth: explicitly clear opener on the returned window - Even with feature support differences, a common fallback is to set the opened window’s opener to null. - (Note: this may have popup-timing/behavior quirks in some browsers; but it’s the conceptual mitigation.) Example pattern: const w = window.open('https://example.com', '_blank', ''); w.opener = null; 3) For HTML links (contrast) - For elements, rel="noopener"/rel="noreferrer" prevents the opened page from accessing the opener (window.opener becomes null). - MDN explains that rel="noopener" prevents access to the document that opened it by not setting Window.opener. Practical recommendation - For external/untrusted destinations opened via window.open, pass 'noopener' (or 'noreferrer' if you want to avoid sending the Referer header). - For maximum robustness on older/edge environments, consider additionally clearing w.opener = null right after opening.
Citations:
- 1: http://bit.ly/1Mk2Rtw
- 2: https://owasp.org/www-community/attacks/Reverse_Tabnabbing
- 3: https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/noopener
- 4: https://developer.mozilla.org/en-US/docs/DOM/window.open
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Window/open
- 6: https://wiki.owasp.org/index.php/Reverse_Tabnabbing
- 7: https://stackoverflow.com/questions/40593632/use-window-open-but-block-use-of-window-opener
🏁 Script executed:
# Search for window.open calls without file type restrictions (rg will auto-detect)
rg "window\.open\s*\(" -A 1 -B 1 --no-type-listRepository: suitenumerique/messages
Length of output: 511
🏁 Script executed:
# Alternatively, search in the frontend directory more specifically
find src/frontend -name "*.ts" -o -name "*.tsx" | xargs grep -n "window.open" 2>/dev/null | head -20Repository: suitenumerique/messages
Length of output: 450
🏁 Script executed:
# Let's examine the context around the use-print.tsx window.open call
sed -n '55,75p' "src/frontend/src/features/message/use-print.tsx"Repository: suitenumerique/messages
Length of output: 770
🏁 Script executed:
# Let's also check if there are any other window.open calls we might have missed
rg "window\.open" -nRepository: suitenumerique/messages
Length of output: 450
Prevent tabnabbing by using noopener when opening external URLs.
window.open(url, '_blank') without noopener allows the destination page to access window.opener, enabling tabnabbing attacks where a malicious page redirects your original tab. Add the noopener,noreferrer flags to block this.
Suggested fix
- window.open(url, '_blank');
+ window.open(url, '_blank', 'noopener,noreferrer');📝 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.
| if (decision) { | |
| window.open(url, '_blank'); | |
| if (decision) { | |
| window.open(url, '_blank', 'noopener,noreferrer'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx`
around lines 348 - 349, The window.open call in thread-message-body.tsx
currently uses window.open(url, '_blank') which is vulnerable to tabnabbing;
update the call inside the decision branch (where you check if (decision)) to
include the noopener and noreferrer flags (use window.open with the feature
string "noopener,noreferrer") so the newly opened page cannot access
window.opener and referrer information.
…tion Signed-off-by: Valentin Regnault <valentinregnault22@gmail.com>
…nabbing Signed-off-by: Valentin Regnault <valentinregnault22@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx (1)
349-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse valid
window.openfeature flags on Line 349.
'rel=noopener'is not a validwindowFeaturestoken, so opener protection is not reliably applied.Suggested fix
- window.open(url, '_blank', 'rel=noopener'); + window.open(url, '_blank', 'noopener,noreferrer');According to MDN documentation for Window.open(), which values are valid in the third `windowFeatures` argument to prevent `window.opener` access, and is `rel=noopener` valid there?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx` at line 349, The call window.open(url, '_blank', 'rel=noopener') uses an invalid windowFeatures token; update the code in the ThreadMessageBody component (where window.open is called) to use valid feature flags such as 'noopener' (and optionally 'noreferrer'), e.g. window.open(url, '_blank', 'noopener,noreferrer'), or alternatively open via an <a> element with target="_blank" and rel="noopener noreferrer"; ensure the change is made in the function that performs the link open in thread-message-body.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx`:
- Line 349: The call window.open(url, '_blank', 'rel=noopener') uses an invalid
windowFeatures token; update the code in the ThreadMessageBody component (where
window.open is called) to use valid feature flags such as 'noopener' (and
optionally 'noreferrer'), e.g. window.open(url, '_blank',
'noopener,noreferrer'), or alternatively open via an <a> element with
target="_blank" and rel="noopener noreferrer"; ensure the change is made in the
function that performs the link open in thread-message-body.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 397b2188-67a6-4bcc-a676-a5905fedf3d9
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-body.tsx
1 participant
#627 Instead of opening links directly from an email, a modal now displays the actual destination URL and asks for confirmation.
Purpose
To protect users against phishing attacks that involve disguising a malicious link within an
<a>HTML tag (link masking).Proposal
Implement a modal that reveals the true URL to which the user is being redirected.
Future work:
Summary by CodeRabbit
New Features
Localization
Style
Documentation