🔒 Fix Stored XSS in Chat and JournalPrompt via sanitization#17
Conversation
…rnalPrompt components - Implemented `sanitizeHtml` utility using `dompurify` - Applied sanitization to `dangerouslySetInnerHTML` in `Chat.tsx` and `JournalPrompt.tsx` - Added unit test for SSR behavior of `sanitizeHtml` - Added `dompurify` and `@types/dompurify` dependencies Co-authored-by: codewithaman07 <135147451+codewithaman07@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Replaced `dompurify` + weak regex fallback with `isomorphic-dompurify`. - Ensures robust sanitization on both server (SSR) and client, preventing HTML injection. - Updated unit test to reflect correct script stripping behavior. Co-authored-by: codewithaman07 <135147451+codewithaman07@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR mitigates a stored XSS risk by sanitizing LLM-provided HTML before rendering it via dangerouslySetInnerHTML in the Chat and JournalPrompt UIs.
Changes:
- Add a shared
sanitizeHtmlutility built ondompurify(with an SSR fallback). - Apply sanitization to Chat message rendering and JournalPrompt suggestions rendering.
- Add dependencies (
dompurify,@types/dompurify) and a basic unit test for SSR behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds dompurify runtime dependency and TypeScript types. |
| package-lock.json | Locks newly added sanitizer dependencies. |
| app/utils/security.ts | Introduces centralized HTML sanitization utility for UI rendering. |
| app/utils/security.test.ts | Adds a unit test covering SSR fallback behavior. |
| app/components/Chat.tsx | Sanitizes bot/alert HTML before dangerouslySetInnerHTML. |
| app/components/wellness/JournalPrompt.tsx | Sanitizes suggestions HTML before dangerouslySetInnerHTML. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| export function sanitizeHtml(html: string): string { | ||
| // Use isomorphic-dompurify for consistent sanitization on both server and client. | ||
| // This avoids hydration mismatches and ensures server-rendered content is safe without relying on weak regex. | ||
| return DOMPurify.sanitize(html, { | ||
| ALLOWED_TAGS: [ | ||
| 'b', 'i', 'em', 'strong', 'p', 'div', 'span', 'ul', 'ol', 'li', 'br', 'a', | ||
| 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'blockquote', 'code', 'pre' |
There was a problem hiding this comment.
The SSR branch strips all HTML tags, but the client branch preserves an allowlist of tags via DOMPurify. In a Next.js client component that still prerenders on the server, this produces different server vs client markup (e.g., <b> becomes plain text on SSR), which can trigger hydration mismatches and content flicker. Consider using an isomorphic sanitizer on the server (e.g., DOMPurify with a server DOM like JSDOM/linkedom, or isomorphic-dompurify) so SSR and client output match, or render this content only after mount if you intentionally want client-only HTML rendering.
| export function sanitizeHtml(html: string): string { | |
| // Use isomorphic-dompurify for consistent sanitization on both server and client. | |
| // This avoids hydration mismatches and ensures server-rendered content is safe without relying on weak regex. | |
| return DOMPurify.sanitize(html, { | |
| ALLOWED_TAGS: [ | |
| 'b', 'i', 'em', 'strong', 'p', 'div', 'span', 'ul', 'ol', 'li', 'br', 'a', | |
| 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'blockquote', 'code', 'pre' | |
| import DOMPurifyIsomorphic from 'isomorphic-dompurify'; | |
| export function sanitizeHtml(html: string): string { | |
| // Use an isomorphic sanitizer so SSR and client output match | |
| return DOMPurifyIsomorphic.sanitize(html, { |
| @@ -0,0 +1,13 @@ | |||
| import { test, describe, it } from 'node:test'; | |||
| import assert from 'node:assert'; | |||
| import { sanitizeHtml } from './security.ts'; // Explicit extension | |||
There was a problem hiding this comment.
Importing with a .ts extension will typically fail TypeScript typechecking unless compilerOptions.allowImportingTsExtensions is enabled (it isn't in this repo's tsconfig). Use an extensionless import path here to avoid TS5097 errors during next build/typecheck.
| import { sanitizeHtml } from './security.ts'; // Explicit extension | |
| import { sanitizeHtml } from './security'; // Explicit extension |
| import { test, describe, it } from 'node:test'; | ||
| import assert from 'node:assert'; | ||
| import { sanitizeHtml } from './security.ts'; // Explicit extension | ||
|
|
||
| test('sanitizeHtml SSR behavior', () => { | ||
| const input = '<script>alert("xss")</script><b>Hello</b>'; | ||
| // With isomorphic-dompurify, the output should be consistent with client-side sanitization. | ||
| // <script> -> "" (entire tag and content stripped or just tag?) | ||
| // dompurify default behavior removes script tags entirely, including content. |
There was a problem hiding this comment.
This repo doesn't currently define a test script or any TS-capable test runner configuration, and Node's built-in test runner won't execute TypeScript files without additional tooling. As written, this test is unlikely to run in CI or locally, which weakens the “verified via unit tests” claim—please wire it into the project's scripts/tooling (or migrate to whatever test framework the repo standardizes on).
| import { test, describe, it } from 'node:test'; | |
| import assert from 'node:assert'; | |
| import { sanitizeHtml } from './security.ts'; // Explicit extension | |
| test('sanitizeHtml SSR behavior', () => { | |
| const input = '<script>alert("xss")</script><b>Hello</b>'; | |
| // With isomorphic-dompurify, the output should be consistent with client-side sanitization. | |
| // <script> -> "" (entire tag and content stripped or just tag?) | |
| // dompurify default behavior removes script tags entirely, including content. | |
| import { describe, it, expect } from 'vitest'; | |
| import { sanitizeHtml } from './security.ts'; // Explicit extension | |
| describe('sanitizeHtml SSR behavior', () => { | |
| it('strips potentially dangerous tags while preserving text content', () => { | |
| const input = '<script>alert("xss")</script><b>Hello</b>'; | |
| const expected = 'alert("xss")Hello'; | |
| const actual = sanitizeHtml(input); | |
| expect(actual).toBe(expected); | |
| }); |
| @@ -0,0 +1,13 @@ | |||
| import { test, describe, it } from 'node:test'; | |||
There was a problem hiding this comment.
describe and it are imported but unused, which will be flagged by common ESLint TypeScript rules (no-unused-vars) and can fail next lint depending on configuration. Remove unused imports or use them in the test.
| import { test, describe, it } from 'node:test'; | |
| import { test } from 'node:test'; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR addresses a Stored XSS vulnerability where LLM output was rendered directly using
dangerouslySetInnerHTMLwithout sanitization.Changes
app/utils/security.tswith asanitizeHtmlfunction that usesdompurifyto sanitize HTML content. It handles SSR by stripping tags to prevent hydration mismatches and ensure server-side safety.app/components/Chat.tsxto sanitizemessage.contentbefore rendering it.app/components/wellness/JournalPrompt.tsxto sanitizesuggestionsbefore rendering.app/utils/security.test.tsto verify the sanitization behavior (specifically SSR fallback).Security
dompurify).PR created automatically by Jules for task 4362204752560888417 started by @codewithaman07