Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ and this project adheres to
- Add unread and starred filters in thread panel #581
- Add better filtering and granularity for usage metrics
- Expose `oidc_autojoin` and `identity_sync` flags in provisioning API
- 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.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated


### Changed

Expand Down
7 changes: 6 additions & 1 deletion src/frontend/public/locales/common/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
"Back": "Back",
"Back to your inbox": "Back to your inbox",
"BCC: ": "BCC: ",
"Be careful!": "Be careful!",
"Blind copy: ": "Blind copy: ",
"Calendar invite": "Calendar invite",
"Cancel": "Cancel",
Expand Down Expand Up @@ -432,6 +433,7 @@
"On going": "On going",
"Open {{driveAppName}} preview": "Open {{driveAppName}} preview",
"Open filters": "Open filters",
"Open the link": "Open the link",
"Open the menu": "Open the menu",
"Or": "Or",
"or drag and drop some files": "or drag and drop some files",
Expand All @@ -442,6 +444,7 @@
"Password reset successfully!": "Password reset successfully!",
"Personal mailbox": "Personal mailbox",
"Personal mailboxes cannot be created when identity synchronization is disabled.": "Personal mailboxes cannot be created when identity synchronization is disabled.",
"phishing_notice": "Be careful when clicking links in email, it could be a <1> phishing attempt</1>.",
"Please enter a valid email address.": "Please enter a valid email address.",
"Prefix can only contain letters, numbers, dots, underscores and hyphens.": "Prefix can only contain letters, numbers, dots, underscores and hyphens.",
"Prefix is required.": "Prefix is required.",
Expand Down Expand Up @@ -553,6 +556,7 @@
"The email {{email}} is invalid.": "The email {{email}} is invalid.",
"The email address is invalid.": "The email address is invalid.",
"The forced signature will be the only one usable for new messages.": "The forced signature will be the only one usable for new messages.",
"The link you clicked is probably unsafe :": "The link you clicked is probably unsafe :",
"The message could not be sent.": "The message could not be sent.",
"The message could not be sent. Please try again later.": "The message could not be sent. Please try again later.",
"The personal mailbox <strong>{{mailboxAddress}}</strong> has been created successfully.": "The personal mailbox <1>{{mailboxAddress}}</1> has been created successfully.",
Expand All @@ -567,6 +571,7 @@
"This email prefix is not allowed for personal mailboxes. Please choose a different prefix.": "This email prefix is not allowed for personal mailboxes. Please choose a different prefix.",
"This event has been cancelled": "This event has been cancelled",
"This is the only admin of this mailbox, you cannot therefore modify its access.": "This is the only admin of this mailbox, you cannot therefore modify its access.",
"This links redirects to :": "This links redirects to :",
"This message has {{count}} attachments_one": "This message has one attachment",
"This message has {{count}} attachments_other": "This message has {{count}} attachments",
"This message has a draft": "This message has a draft",
Expand Down Expand Up @@ -645,4 +650,4 @@
"Your email...": "Your email...",
"Your messages have been imported successfully!": "Your messages have been imported successfully!",
"Your session has expired. Please log in again.": "Your session has expired. Please log in again."
}
}
8 changes: 7 additions & 1 deletion src/frontend/public/locales/common/fr-FR.json
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@
"Back": "Précédent",
"Back to your inbox": "Retour à votre messagerie",
"BCC: ": "CCI : ",
"Be careful!": "Attention !",
"Blind copy: ": "Copie cachée : ",
"Calendar invite": "Invitation calendrier",
"Cancel": "Annuler",
Expand Down Expand Up @@ -479,6 +480,7 @@
"On going": "En cours",
"Open {{driveAppName}} preview": "Ouvrir l'aperçu dans {{driveAppName}}",
"Open filters": "Ouvrir les filtres",
"Open the link": "Ouvrir le lien",
"Open the menu": "Ouvrir le menu",
"Or": "Ou",
"or drag and drop some files": "ou glissez-déposez des fichiers",
Expand All @@ -489,6 +491,7 @@
"Password reset successfully!": "Mot de passe réinitialisé avec succès !",
"Personal mailbox": "Boîte personnelle",
"Personal mailboxes cannot be created when identity synchronization is disabled.": "Les boîtes aux lettres personnelles ne peuvent pas être créées lorsque la synchronisation d'identité est désactivée.",
"phishing_notice": "Soyez prudent en cliquant sur les liens dans les emails, il pourrait s'agir d'une <1> tentative de phishing</1>",
"Please enter a valid email address.": "Veuillez saisir une adresse email valide.",
"Prefix can only contain letters, numbers, dots, underscores and hyphens.": "Le préfixe ne peut contenir que des lettres, chiffres, points, tirets bas et tirets.",
"Prefix is required.": "Le préfixe est requis.",
Expand Down Expand Up @@ -603,6 +606,7 @@
"The email {{email}} is invalid.": "Le courriel {{email}} est invalide.",
"The email address is invalid.": "L'adresse email est invalide.",
"The forced signature will be the only one usable for new messages.": "La signature forcée sera la seule utilisable pour les nouveaux messages.",
"The link you clicked is probably unsafe :": "Le lien sur lequel vous avez cliqué est probablement dangereux :",
"The message could not be sent.": "Le message n'a pas pu être envoyé.",
"The message could not be sent. Please try again later.": "Le message n'a pas pu être envoyé. Veuillez réessayer plus tard.",
"The personal mailbox <strong>{{mailboxAddress}}</strong> has been created successfully.": "L'adresse personnelle <strong>{{mailboxAddress}}</strong> a été créée avec succès.",
Expand All @@ -617,6 +621,7 @@
"This email prefix is not allowed for personal mailboxes. Please choose a different prefix.": "Ce préfixe d'adresse n'est pas autorisé pour les boîtes aux lettres personnelles. Veuillez choisir un autre préfixe.",
"This event has been cancelled": "Cet événement a été annulé",
"This is the only admin of this mailbox, you cannot therefore modify its access.": "C'est le seul administrateur de cette boîte aux lettres, vous ne pouvez donc pas modifier son accès.",
"This links redirects to :": "Ce lien redirige vers :",
"This message has {{count}} attachments_one": "Ce message a une pièce jointe",
"This message has {{count}} attachments_many": "Ce message a {{count}} pièces jointes",
"This message has {{count}} attachments_other": "Ce message a {{count}} pièces jointes",
Expand Down Expand Up @@ -688,6 +693,7 @@
"You can now inform the person that their mailbox is ready to be used and communicate the instructions for authentication.": "Vous pouvez désormais prévenir la personne que sa boîte aux lettres est prête à être utilisée et lui communiquer les instructions pour s'authentifier.",
"You cannot delete the last editor of this thread": "Vous ne pouvez pas supprimer le dernier éditeur de cette conversation",
"You cannot modify it.": "Vous ne pouvez pas la modifier.",
"You clicked on the link \"{{linkText}}\" which redirects to :": "Vous avez cliqué sur le lien \"{{linkText}}\" qui redirige vers :",
"You have {{count}} recipients, which exceeds the maximum of {{max}} recipients per message. The message cannot be sent until you reduce the number of recipients._one": "Vous avez {{count}} destinataire, ce qui dépasse le maximum de {{max}} destinataires autorisés par message. Le message ne peut pas être envoyé tant que vous n'avez pas réduit le nombre de destinataires.",
"You have {{count}} recipients, which exceeds the maximum of {{max}} recipients per message. The message cannot be sent until you reduce the number of recipients._many": "Vous avez {{count}} destinataires, ce qui dépasse le maximum de {{max}} destinataires autorisés par message. Le message ne peut pas être envoyé tant que vous n'avez pas réduit le nombre de destinataires.",
"You have {{count}} recipients, which exceeds the maximum of {{max}} recipients per message. The message cannot be sent until you reduce the number of recipients._other": "Vous avez {{count}} destinataires, ce qui dépasse le maximum de {{max}} destinataires autorisés par message. Le message ne peut pas être envoyé tant que vous n'avez pas réduit le nombre de destinataires.",
Expand All @@ -697,4 +703,4 @@
"Your email...": "Renseigner votre email...",
"Your messages have been imported successfully!": "Vos messages ont été importés avec succès !",
"Your session has expired. Please log in again.": "Votre session a expiré. Veuillez vous reconnecter."
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,19 @@
top: 0;
z-index: 2;
}

.c__modal__title-icon .material-icons {
font-size: var(--icon-size);
}

.link-preview__children {
display: flex;
flex-direction: column;
gap: var(--c--globals--spacings--xs);
}

.link-preview__phishing-notice {
font-size: var(--c--globals--font--sizes--xs);
color: var(--c--contextuals--content--semantic--neutral--secondary);
text-align: left;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import { useState, useCallback, useRef } from "react";
import i18n from "@/features/i18n/initI18n";
import { Button, Modal, ModalSize, Alert, VariantType, iconFromType } from "@gouvfr-lasuite/cunningham-react";
import classNames from "classnames";
import { Trans, useTranslation } from "react-i18next";
/**
* Modal component to show a preview of a link.
* <a href={url}>{linkText}</a>
*
* @param isOpen - Whether the modal is open
* @param onClose - Function to call when the modal is closed
* @param url - The URL to preview
* @param linkText - The text of the link (optional)
* @param hardWarning - Whether to show a more prominent warning
* @param decision - Function to call with the user's confirmation choice
*/
type LinkPreviewModalProps = {
isOpen: boolean;
url: string;
hardWarning?: boolean;
decision: (choice: boolean) => void;
}

/**
* Confirmation modal before redirecting to an external link.
* It alerts the user about potential risks (phishing, etc.).
*/
export const LinkPreviewModal = ({ isOpen, url, hardWarning, decision }: LinkPreviewModalProps) => {
const { t } = useTranslation();
return (
<Modal
isOpen={isOpen}
size={ModalSize.SMALL}
title={(
<span className="c__modal__text--centered">{
hardWarning
? i18n.t('Be careful!')
: i18n.t('This links redirects to :')
}</span>
)}
titleIcon={hardWarning && (
<span
className="material-icons modal-message-error-icon"
>
{iconFromType(VariantType.WARNING)}
</span>
)}
hideCloseButton={true}
actions={[
<Button
key="cancel"
variant={hardWarning ? "primary" : "tertiary"}
onClick={() => decision(false)}
>
{t("Cancel")}
</Button>,
<Button
key="confirm"
variant={hardWarning ? "tertiary" : "primary"}
color={hardWarning ? "error" : "neutral"}
onClick={() => decision(true)}
>
{t("Open the link")}
</Button>
]}
onClose={() => decision(false)}
closeOnClickOutside={true}
>
<div className="link-preview__children">
{hardWarning && i18n.t('The link you clicked is probably unsafe :')}
<Alert type={hardWarning ? VariantType.WARNING : VariantType.NEUTRAL}>{url}</Alert>
<p className="link-preview__phishing-notice">
<Trans i18nKey="phishing_notice">
Be careful when clicking links in email, it could be a
<a href={`https://www.service-public.gouv.fr/particuliers/vosdroits/F34800`}>
phishing attempt
</a>.
</Trans>
</p>
</div>
</Modal >
)
}

/**
* Hook to manage the state and logic of the link preview modal.
* Exposes an asynchronous `askConfirmation` function that waits for user action.
*
* @returns An object containing:
* - `askConfirmation`: an async function that opens the modal and returns a boolean (`true` if confirmed)
* - `modal`: the React node of the modal to be injected into the component tree
*/
export const useLinkPreviewModal = () => {
const [isOpen, setIsOpen] = useState(false);
const [url, setUrl] = useState('');
const [hardWarning, setHardWarning] = useState(false);
const resolverRef = useRef<((choice: boolean) => void) | null>(null);

const askConfirmation = useCallback((urlToPreview: string, isHardWarning: boolean = false, textToPreview?: string) => {
setUrl(urlToPreview);
setHardWarning(isHardWarning);
setIsOpen(true);

return new Promise<boolean>((resolve) => {
resolverRef.current = resolve;
});
}, []);

const decision = useCallback((choice: boolean) => {
setIsOpen(false);
if (resolverRef.current) {
resolverRef.current(choice);
resolverRef.current = null;
}
}, []);

const modal = isOpen ? (
<LinkPreviewModal
isOpen={isOpen}
url={url}
hardWarning={hardWarning}
decision={decision}
/>
) : null;

return { askConfirmation, modal };
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { renderToStaticMarkup } from "react-dom/server";
import { getRequestUrl, getApiOrigin } from "@/features/api/utils";
import { getBlobDownloadRetrieveUrl } from "@/features/api/gen/blob/blob";
import { UnquoteMessage } from '@/features/utils/unquote-message';
import { useTranslation } from "react-i18next";
import { Trans, useTranslation } from "react-i18next";
import { tokens } from '@/styles/cunningham-tokens'
import { useTheme } from "@/features/providers/theme";
import { useConfig } from "@/features/providers/config";
Expand All @@ -14,6 +14,11 @@ import { getMailboxesImageProxyListUrl } from "@/features/api/gen/mailboxes/mail
import { EXTERNAL_IMAGES_CONSENT_KEY } from "@/features/config/constants";
import { renderBodyParts } from "./renderers";
import { ThreadMessageBodyProps } from "./types";
import { LinkPreviewModal, useLinkPreviewModal } from "./link-preview-modal";
import { Alert, Button, iconFromType, VariantType } from "@gouvfr-lasuite/cunningham-react";
import { handle } from "@/features/utils/errors";
import { set } from "zod";
import Link from "next/link";

const CSP = [
// Allow images from our domain, data URIs, and API endpoints
Expand Down Expand Up @@ -44,7 +49,7 @@ const CSP = [
].join('; ');

const ThreadMessageBody = ({ bodyParts, attachments = [], isHidden = false, messageId, onLoad }: ThreadMessageBodyProps) => {
const { t } = useTranslation();
const { t, i18n } = useTranslation();
const iframeRef = useRef<HTMLIFrameElement>(null);
const { cunninghamTheme, variant } = useTheme();
const { selectedMailbox } = useMailboxContext();
Expand All @@ -64,6 +69,8 @@ const ThreadMessageBody = ({ bodyParts, attachments = [], isHidden = false, mess
setDisplayExternalImages(true);
};

const { askConfirmation: askLinkConfirmation, modal: linkConfirmationModal } = useLinkPreviewModal();

// Build CID to blob URL mapping for inline image resolution
const cidToBlobUrlMap = useMemo(() => {
const map = new Map<string, string>();
Expand Down Expand Up @@ -321,10 +328,28 @@ const ThreadMessageBody = ({ bodyParts, attachments = [], isHidden = false, mess
doc.querySelectorAll('details.email-quoted-content').forEach(node => {
node.addEventListener('toggle', resizeIframe);
});

// Handle link clicks
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');
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@ValentinRegnault, understood — both points are clear now:

  1. link.href !== link.textContent guard — 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.

  2. hardWarning hardcoded to false — Also intentional. The hardWarning path is a placeholder for the future Quad9 DNS-filtering integration; since that feature isn't implemented yet, it should stay false for 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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"
fi

Repository: 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 -n

Repository: 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:


🏁 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-list

Repository: 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 -20

Repository: 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" -n

Repository: 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.

Suggested change
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.

}
}

useEffect(() => {
const handleMessage = (event: MessageEvent) => {
if (event.data === 'iframe-loaded') {
Expand Down Expand Up @@ -380,6 +405,7 @@ const ThreadMessageBody = ({ bodyParts, attachments = [], isHidden = false, mess
sandbox="allow-same-origin allow-popups allow-popups-to-escape-sandbox allow-top-navigation-by-user-activation"
onLoad={handleIframeLoad}
/>
{linkConfirmationModal}
</>
)
}
Expand Down