Sanitize notification HTML to prevent XSS by IshaanXCoder · Pull Request #3243 · intelowlproject/IntelOwl

IshaanXCoder · 2026-01-28T05:48:10Z

Sanitize notification HTML to prevent XSS

Description

This PR fixes a potential XSS vulnerability in the NotificationsList component by replacing dangerouslySetInnerHTML with react-markdown

Changes

Replaced dangerouslySetInnerHTML with <ReactMarkdown> in frontend/src/components/jobs/notification/NotificationsList.jsx
Configured rel="noopener noreferrer" for external links to improve security.
Note: Modifying src/components/common/markdownToHtml.jsx was avoided because it is being used across multiple components, thus I added the snippet to NotificationsList.jsx itself. Let me know if I should be folllowign an alternative approach.
Tested in frontend/tests/components/jobs/notification/NotificationsList.test.jsx

Type of change

Bug fix (non-breaking change which fixes an issue).

Checklist

I have read and understood the rules about how to Contribute to this project
The pull request is for the branch develop
Please avoid adding new libraries as requirements whenever it is possible. Use new libraries only if strictly needed to solve the issue you are working for. In case of doubt, ask a maintainer permission to use a specific library.
[N/A] Linters (Black, Flake, Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
I have added tests for the feature/bug I solved (see tests folder). All the tests (new and old ones) gave 0 errors.
If the GUI has been modified:
- I have a provided a screenshot of the result in the PR.
- I have created new frontend tests for the new component or updated existing ones.

Copilot

Pull request overview

Sanitizes notification rendering to mitigate XSS by replacing raw HTML injection with Markdown rendering.

Changes:

Replaced dangerouslySetInnerHTML usage in notifications with react-markdown.
Added custom markdown renderers for emphasis and links (including target="_blank" and rel="noopener noreferrer").
Added frontend tests focused on XSS payloads and basic markdown behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
frontend/src/components/jobs/notification/NotificationsList.jsx	Switches notification body rendering to `ReactMarkdown` with custom link handling.
frontend/tests/components/jobs/notification/NotificationsList.test.jsx	Adds tests to ensure injected HTML/scripts don’t produce DOM elements and validates markdown rendering behaviors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-28T05:51:26Z

frontend/src/components/jobs/notification/NotificationsList.jsx

+const markdownComponents = {
+  // eslint-disable-next-line id-length
+  em: ({ node: _node, ...props }) => <i className="text-code" {...props} />,
+  // eslint-disable-next-line id-length
+  a: ({ node: _node, href, children, ...props }) => {
+    // eslint-disable-next-line no-script-url
+    if (href && href.startsWith("javascript:")) {
+      return null;


The javascript: URL check is easy to bypass because it’s case-sensitive and doesn’t normalize/trim the URL (e.g., JaVaScRiPt:..., leading whitespace/newlines). It also only covers javascript: and not other dangerous schemes (e.g., data:, vbscript:). Prefer a scheme allowlist via react-markdown’s URL transform hook or by parsing the URL and validating protocol after normalization, and render the link text safely when disallowed.

Suggested change

const markdownComponents = {

// eslint-disable-next-line id-length

em: ({ node: _node, ...props }) => <i className="text-code" {...props} />,

// eslint-disable-next-line id-length

a: ({ node: _node, href, children, ...props }) => {

// eslint-disable-next-line no-script-url

if (href && href.startsWith("javascript:")) {

return null;

const SAFE_URL_PROTOCOLS = ["http:", "https:", "mailto:", "tel:"];

const isSafeHref = (href) => {

if (!href) {

return false;

}

const trimmedHref = href.trim();

// Allow relative links and in-page anchors.

if (trimmedHref.startsWith("/") || trimmedHref.startsWith("#")) {

return true;

}

try {

const url = new URL(trimmedHref, window.location.origin);

return SAFE_URL_PROTOCOLS.includes(url.protocol);

} catch (_e) {

// If parsing fails, treat as unsafe.

return false;

}

};

const markdownComponents = {

// eslint-disable-next-line id-length

em: ({ node: _node, ...props }) => <i className="text-code" {...props} />,

// eslint-disable-next-line id-length

a: ({ node: _node, href, children, ...props }) => {

if (!isSafeHref(href)) {

// Render the text but do not make it a clickable link.

return (

<span className="link-primary" {...props}>

{children}

</span>

);

Copilot · 2026-01-28T05:51:26Z

frontend/src/components/jobs/notification/NotificationsList.jsx

+const markdownComponents = {
+  // eslint-disable-next-line id-length
+  em: ({ node: _node, ...props }) => <i className="text-code" {...props} />,
+  // eslint-disable-next-line id-length
+  a: ({ node: _node, href, children, ...props }) => {
+    // eslint-disable-next-line no-script-url
+    if (href && href.startsWith("javascript:")) {
+      return null;
+    }
+    return (
+      <a
+        href={href}
+        target="_blank"
+        rel="noopener noreferrer"
+        className="link-primary"
+        {...props}
+      >
+        {children}
+      </a>
+    );
+  },
+};


This file introduces a custom markdownComponents implementation, but there is already a shared helper at frontend/src/components/common/markdownToHtml.jsx that wraps react-markdown and is used in multiple places (e.g., components/common/form/pluginsMultiSelectDropdownInput.jsx, components/jobs/result/pluginReportTables.jsx). Consider moving the security/link-hardening changes into the shared helper (or extracting shared markdown components) and reusing it here to avoid duplicated/inconsistent markdown behavior across the UI.

Already explained in the description.

Copilot · 2026-01-28T05:51:26Z

frontend/tests/components/jobs/notification/NotificationsList.test.jsx

+    test("prevents javascript: protocol in links", () => {
+      const notifications = [
+        {
+          id: 4,
+          title: "JS Protocol Test",
+          body: '[Click me](javascript:alert("XSS"))',
+          read: false,
+          created_at: "2023-01-01T00:00:00Z",
+        },
+      ];
+
+      render(
+        <NotificationsList
+          notifications={notifications}
+          refetchFn={mockRefetch}
+        />,
+      );
+
+      const link = screen.queryByRole("link");
+      if (link) {
+        const href = link.getAttribute("href");
+        if (href) {
+          expect(href).not.toContain("javascript:");
+        }
+      }
+    });


The javascript: protocol test can produce false positives because it passes if no link is rendered at all (e.g., parsing changes), which wouldn’t actually validate sanitization. Make the assertion deterministic by first asserting the link text is present and then asserting either (a) no anchor is rendered for it, or (b) the rendered anchor’s href is transformed to a safe value.

Copilot · 2026-01-28T05:51:26Z

frontend/tests/components/jobs/notification/NotificationsList.test.jsx

+      const link = screen.getByRole("link", { name: "Google" });
+      expect(link).toHaveAttribute("href", "https://google.com");
+      expect(link).toHaveAttribute("target", "_blank");
+      expect(link).toHaveClass("link-primary");
+    });


The link rendering test asserts target="_blank" but doesn’t verify the security requirement from the PR description (rel="noopener noreferrer"). Add an assertion for the rel attribute so regressions don’t reintroduce tabnabbing risk.

IshaanXCoder · 2026-01-28T06:09:30Z

Hey @fgibertoni @mlodic PTAL at this :)

mlodic · 2026-01-28T14:17:40Z

can you also show us a brief video that this actually works in the GUI?

IshaanXCoder · 2026-01-29T12:37:45Z

can you also show us a brief video that this actually works in the GUI?

Hey @mlodic apologies for the delay, hope this video explains it clearly. (Before then after)

Screen.Recording.2026-01-29.at.5.58.56.PM.mov

mlodic · 2026-01-29T14:03:54Z

thanks, very helpful!

fix/sanitize-notification-HTML-to-prevent-XSS

aaa3042

Copilot AI review requested due to automatic review settings January 28, 2026 05:48

Copilot started reviewing on behalf of IshaanXCoder January 28, 2026 05:48 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

fix : copilot recommendations

91c0204

mlodic approved these changes Jan 29, 2026

View reviewed changes

mlodic merged commit c2bd833 into intelowlproject:develop Jan 29, 2026
9 checks passed

mlodic mentioned this pull request Jan 29, 2026

Bug : xss notifications. #3123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sanitize notification HTML to prevent XSS#3243

Sanitize notification HTML to prevent XSS#3243
mlodic merged 2 commits intointelowlproject:developfrom
IshaanXCoder:fix/sanitize-notification

IshaanXCoder commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

IshaanXCoder Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

IshaanXCoder commented Jan 28, 2026

Uh oh!

mlodic commented Jan 28, 2026

Uh oh!

IshaanXCoder commented Jan 29, 2026

Uh oh!

Uh oh!

mlodic commented Jan 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-const markdownComponents = {
-  // eslint-disable-next-line id-length
-  em: ({ node: _node, ...props }) => <i className="text-code" {...props} />,
-  // eslint-disable-next-line id-length
-  a: ({ node: _node, href, children, ...props }) => {
-    // eslint-disable-next-line no-script-url
-    if (href && href.startsWith("javascript:")) {
-      return null;
+const SAFE_URL_PROTOCOLS = ["http:", "https:", "mailto:", "tel:"];
+const isSafeHref = (href) => {
+  if (!href) {
+    return false;
+  }
+  const trimmedHref = href.trim();
+  // Allow relative links and in-page anchors.
+  if (trimmedHref.startsWith("/") || trimmedHref.startsWith("#")) {
+    return true;
+  }
+  try {
+    const url = new URL(trimmedHref, window.location.origin);
+    return SAFE_URL_PROTOCOLS.includes(url.protocol);
+  } catch (_e) {
+    // If parsing fails, treat as unsafe.
+    return false;
+  }
+};
+const markdownComponents = {
+  // eslint-disable-next-line id-length
+  em: ({ node: _node, ...props }) => <i className="text-code" {...props} />,
+  // eslint-disable-next-line id-length
+  a: ({ node: _node, href, children, ...props }) => {
+    if (!isSafeHref(href)) {
+      // Render the text but do not make it a clickable link.
+      return (
+        <span className="link-primary" {...props}>
+          {children}
+        </span>
+      );

Uh oh!

Conversation

IshaanXCoder commented Jan 28, 2026

Description

Changes

Type of change

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

IshaanXCoder Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

IshaanXCoder commented Jan 28, 2026

Uh oh!

mlodic commented Jan 28, 2026

Uh oh!

IshaanXCoder commented Jan 29, 2026

Uh oh!

Uh oh!

mlodic commented Jan 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants