Skip to content

fix: security audit findings from issue #741 #757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LukePercy wants to merge 1 commit into from
Closed

fix: security audit findings from issue #741 #757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions .github/workflows/nextjs-tracker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ jobs:
id: commits
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
SINCE_HOURS: ${{ github.event.inputs.since_hours || '24' }}
run: |
HOURS="${{ github.event.inputs.since_hours || '24' }}"
HOURS="$SINCE_HOURS"
SINCE=$(date -u -d "${HOURS} hours ago" +%Y-%m-%dT%H:%M:%SZ)

echo "Fetching Next.js canary commits since $SINCE"
Expand Down Expand Up @@ -70,8 +71,10 @@ jobs:
- name: Build tracker prompt
id: build_prompt
if: steps.commits.outputs.has_commits == 'true'
env:
DRY_RUN_INPUT: ${{ github.event.inputs.dry_run || 'false' }}
run: |
DRY_RUN="${{ github.event.inputs.dry_run || 'false' }}"
DRY_RUN="$DRY_RUN_INPUT"
COMMITS=$(cat /tmp/nextjs-commits.json)

if [ "$DRY_RUN" = "true" ]; then
Expand Down Expand Up @@ -134,4 +137,6 @@ jobs:

- name: Skip (no commits)
if: steps.commits.outputs.has_commits == 'false'
run: echo "No Next.js canary commits in the last ${{ github.event.inputs.since_hours || '24' }} hours. Nothing to do."
env:
SINCE_HOURS: ${{ github.event.inputs.since_hours || '24' }}
run: echo "No Next.js canary commits in the last $SINCE_HOURS hours. Nothing to do."
4 changes: 3 additions & 1 deletion .github/workflows/publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,15 @@ jobs:
- name: Bump version
id: version
working-directory: packages/vinext
env:
BUMP: ${{ inputs.bump }}
run: |
# vp wraps package-manager subcommands, but versioning still needs the real npm CLI
# because we are mutating package.json in place before the publish step.
LATEST=$(npm view vinext version 2>/dev/null || echo "0.0.0")
IFS='.' read -r MAJOR MINOR PATCH <<< "$LATEST"

case "${{ inputs.bump }}" in
case "$BUMP" in
major)
MAJOR=$((MAJOR + 1))
MINOR=0
Expand Down
3 changes: 2 additions & 1 deletion examples/hackernews/components/comment.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use client';

import { useState } from 'react';
import DOMPurify from 'dompurify';
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 3, 2026

Choose a reason for hiding this comment

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

Bug: DOMPurify.sanitize() is a silent no-op during SSR.

This component has 'use client', but in vinext's App Router, client components are still SSR'd via renderToReadableStream in the SSR environment. During SSR there is no window/document, so DOMPurify sets isSupported = false and sanitize() returns the input string unchanged (see purify.ts L1541-1543).

This means the initial HTML response sent to the browser contains the raw, unsanitized HN API HTML. Sanitization only kicks in after client-side hydration triggers a re-render. A malicious HN comment could execute in the initial page load before React hydrates.

Two options to fix:

  1. Use isomorphic-dompurify — it bundles jsdom for server-side use, so sanitization works in both environments.
  2. Sanitize in the server component (app/item/[id]/(comments)/page.tsx) before passing text as a prop. You could use a server-compatible sanitizer there (e.g., sanitize-html or isomorphic-dompurify), which would protect both SSR and client renders.

Option 2 is arguably better because it sanitizes once at the data boundary rather than on every render.


import timeAgo from '../lib/time-ago';

Expand All @@ -26,7 +27,7 @@ export default function Comment({ user, text, date, comments, commentsCount }) {
<div
key="text"
className={styles.text}
dangerouslySetInnerHTML={{ __html: text }}
dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(text) }}
/>,
<div key="children" className={styles.children}>
{comments.map((comment) => (
Expand Down
50 changes: 26 additions & 24 deletions examples/hackernews/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
{
"name": "vinext-hackernews",
"type": "module",
"private": true,
"scripts": {
"dev": "vp dev",
"build": "vp build",
"preview": "vp preview"
},
"dependencies": {
"@vitejs/plugin-react": "catalog:",
"ms": "catalog:",
"react": "catalog:",
"react-dom": "catalog:",
"server-only": "catalog:",
"vite": "catalog:",
"vinext": "workspace:*",
"@vitejs/plugin-rsc": "catalog:",
"react-server-dom-webpack": "catalog:",
"@cloudflare/vite-plugin": "catalog:",
"wrangler": "catalog:"
},
"devDependencies": {
"vite-plus": "catalog:"
}
"name": "vinext-hackernews",
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 3, 2026

Choose a reason for hiding this comment

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

Nit: This file was reformatted from 2-space to 4-space indentation. Every other package.json in the repo (including all other examples) uses 2-space indent. This creates unnecessary diff noise and inconsistency.

Please revert the whitespace-only changes and keep only the meaningful additions (dompurify in dependencies, @cloudflare/workers-types in devDependencies).

"type": "module",
"private": true,
"scripts": {
"dev": "vp dev",
"build": "vp build",
"preview": "vp preview"
},
"dependencies": {
"@vitejs/plugin-react": "catalog:",
"dompurify": "catalog:",
"ms": "catalog:",
"react": "catalog:",
"react-dom": "catalog:",
"server-only": "catalog:",
"vite": "catalog:",
"vinext": "workspace:*",
"@vitejs/plugin-rsc": "catalog:",
"react-server-dom-webpack": "catalog:",
"@cloudflare/vite-plugin": "catalog:",
"wrangler": "catalog:"
},
"devDependencies": {
"@cloudflare/workers-types": "catalog:",
"vite-plus": "catalog:"
}
}
15 changes: 11 additions & 4 deletions examples/hackernews/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,15 @@
"esModuleInterop": true,
"skipLibCheck": true,
"allowJs": true,
"baseUrl": ".",
"types": ["@cloudflare/workers-types"]
"noEmit": true,
"types": [
"@cloudflare/workers-types"
]
},
"include": ["app", "components", "lib", "worker"]
}
"include": [
"app",
"components",
"lib",
"worker"
]
}
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 3, 2026

Choose a reason for hiding this comment

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

Nit: Missing trailing newline. The diff shows No newline at end of file. Most editors and linters expect a final newline — please add one.

4 changes: 3 additions & 1 deletion packages/vinext/src/shims/head.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ export function escapeAttr(s: string): string {
* context but prevents the HTML parser from seeing a closing tag.
*/
export function escapeInlineContent(content: string, tag: string): string {
// Build a pattern like `<\/script` or `<\/style`, case-insensitive
// Build a pattern like `<\/script` or `<\/style`, case-insensitive.
// `tag` is always a literal developer-controlled value ("script" or "style")
// guarded by the RAW_CONTENT_TAGS.has(tag) check at all call sites — never user input.
const pattern = new RegExp(`<\\/(${tag})`, "gi");
return content.replace(pattern, "<\\/$1");
}
Expand Down
5 changes: 5 additions & 0 deletions packages/vinext/src/shims/script.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,11 @@ function Script(props: ScriptProps): React.ReactElement | null {
}

if (dangerouslySetInnerHTML?.__html) {
// Intentional: mirrors the Next.js <Script> API where dangerouslySetInnerHTML
// is developer-supplied inline script content (not user input). The prop name
// itself signals developer awareness of the XSS risk, consistent with React's
// design. User-supplied data must never flow into this prop.
// eslint-disable-next-line no-unsanitized/property
el.innerHTML = dangerouslySetInnerHTML.__html as string;
} else if (children && typeof children === "string") {
el.textContent = children;
Expand Down
22 changes: 22 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pnpm-workspace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ catalog:
clsx: ^2.1.1
codehike: 1.0.7
date-fns: 4.1.0
dompurify: ^3.2.6
"@types/dompurify": ^3.0.5
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 3, 2026

Choose a reason for hiding this comment

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

Unused catalog entry. @types/dompurify is added to the workspace catalog here but is never referenced by any package.json in the repo. It's also absent from the lockfile, confirming nothing installed it.

Either add it to examples/hackernews/package.json devDependencies (so TypeScript can type-check DOMPurify.sanitize()) or remove this line. If you switch to isomorphic-dompurify, this entry would need to change too.

fumadocs-core: 16.6.17
fumadocs-mdx: 14.2.10
fumadocs-ui: 16.6.17
Expand Down
2 changes: 1 addition & 1 deletion tests/vite-hmr-websocket.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function readUpgradeResponse(
"Connection: Upgrade",
"Upgrade: websocket",
"Sec-WebSocket-Version: 13",
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==",
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==", // gitleaks:allow — RFC 6455 example nonce ("the sample nonce"), not a real credential
"Sec-WebSocket-Protocol: vite-hmr",
];
if (origin) lines.push(`Origin: ${origin}`);
Expand Down
Loading