[PR #106] [MERGED] fix: close security gaps in init.ts shell exec and app-dev-server external proxy #313

New issue

Closed

opened 2026-05-06 12:39:08 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 12:39:08 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/cloudflare/vinext/pull/106
Author: @threepointone
Created: 2/26/2026
Status: ✅ Merged
Merged: 2/26/2026
Merged by: @threepointone

Base: main ← Head: fix/security-gaps-1-2

📝 Commits (3)

c6d10c1 fix: close security gaps in init.ts shell exec and app-dev-server external proxy
4da89c5 fix: unify isExternalUrl to detect all URL schemes and protocol-relative URLs
858f165 fix: add shell: true to execFileSync for Windows .cmd file compat

📊 Changes

10 files changed (+4259 additions, -17 deletions)

View changed files

📝 packages/vinext/src/config/config-matchers.ts (+4 -3)
📝 packages/vinext/src/init.ts (+3 -2)
📝 packages/vinext/src/server/app-dev-server.ts (+15 -3)
📝 packages/vinext/src/server/dev-server.ts (+4 -3)
📝 packages/vinext/src/shims/navigation.ts (+2 -2)
📝 packages/vinext/src/shims/router.ts (+2 -2)
📝 tests/app-router.test.ts (+63 -0)
📝 tests/fixtures/app-basic/next.config.ts (+5 -0)
➕ tests/fixtures/pages-basic/dist/server/entry.js (+4140 -0)
📝 tests/shims.test.ts (+21 -2)

📄 Description

Summary

Closes two remaining security gaps identified during the vulnerability audit, fixes a missed backslash normalization path, and unifies isExternalUrl across all copies.

Gap 1 — GHSA-w7vc-6jjg-498h (shell injection in init.ts)

init.ts:224 used execSync with string concatenation. All inputs are hardcoded package names so practical risk is near zero, but deploy.ts and cli.ts were already migrated to execFileSync — init.ts was missed.

Fix: execSync → execFileSync with cmd.split(" ") argument array. The _exec test callback interface is unchanged.

Gap 2 — GHSA-pf96-3m7r-pv3j (credential leak on external proxy)

app-dev-server.ts:1118-1137 has an inline __proxyExternalRequest in the generated RSC entry that forwarded Cookie, Authorization, and x-api-key headers to external rewrite targets. The fixed version in config-matchers.ts strips all of these plus adds a 30s timeout — the dev-server copy was not updated.

Fix: Strip cookie, authorization, x-api-key, proxy-authorization, and all x-middleware-* headers. Add 30-second AbortSignal.timeout. Matches the hardened proxyExternalRequest in config-matchers.ts.

Bonus — GHSA-79rw-r8hc-qg8v (backslash bypass, missed GSP path)

The GSSP redirect path in dev-server.ts was already fixed in PR #105, but the GSP redirect path (line ~520) was missed. Now both paths normalize backslashes.

isExternalUrl unification (defense-in-depth)

Four copies of isExternalUrl existed with inconsistent checks:

config-matchers.ts and app-dev-server.ts only checked http:// and https:// (missing //)
router.ts and navigation.ts checked http://, https://, and // but missed exotic schemes

All four now use the same RFC 3986 scheme regex:

/^[a-z][a-z0-9+.-]*:/i.test(url) || url.startsWith("//")

This detects data:, javascript:, blob:, ftp:, and any valid URL scheme, plus protocol-relative URLs.

Tests

Integration test for proxy credential stripping (mock HTTP server + conditional external rewrite)
isExternalUrl tests expanded: exotic schemes, protocol-relative, hash-only, bare strings

Files changed

File	Change
`init.ts`	`execSync` → `execFileSync`
`app-dev-server.ts`	Inline proxy: strip credentials + 30s timeout; `__isExternalUrl` unified
`dev-server.ts`	Backslash normalization in GSP redirect path
`config-matchers.ts`	`isExternalUrl`: add `//` + exotic scheme detection
`router.ts`	`isExternalUrl`: RFC 3986 regex
`navigation.ts`	`isExternalUrl`: RFC 3986 regex
`app-router.test.ts`	Integration test for proxy credential stripping
`next.config.ts` (fixture)	Conditional external rewrite for test
`shims.test.ts`	`isExternalUrl` exotic scheme + protocol-relative tests
`dist/server/entry.js` (fixture)	Pre-built entry updated

All 1981 tests pass.

_{🔄 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/106 **Author:** [@threepointone](https://github.com/threepointone) **Created:** 2/26/2026 **Status:** ✅ Merged **Merged:** 2/26/2026 **Merged by:** [@threepointone](https://github.com/threepointone) **Base:** `main` ← **Head:** `fix/security-gaps-1-2` --- ### 📝 Commits (3) - [`c6d10c1`](https://github.com/cloudflare/vinext/commit/c6d10c1baff3c4066b5beb1baea6c72dadb528ad) fix: close security gaps in init.ts shell exec and app-dev-server external proxy - [`4da89c5`](https://github.com/cloudflare/vinext/commit/4da89c5ee1d03742a81254d8201f7c717d1ccd41) fix: unify isExternalUrl to detect all URL schemes and protocol-relative URLs - [`858f165`](https://github.com/cloudflare/vinext/commit/858f1659d5b7920cc611d6b53c2c9cab9664e588) fix: add shell: true to execFileSync for Windows .cmd file compat ### 📊 Changes **10 files changed** (+4259 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `packages/vinext/src/config/config-matchers.ts` (+4 -3) 📝 `packages/vinext/src/init.ts` (+3 -2) 📝 `packages/vinext/src/server/app-dev-server.ts` (+15 -3) 📝 `packages/vinext/src/server/dev-server.ts` (+4 -3) 📝 `packages/vinext/src/shims/navigation.ts` (+2 -2) 📝 `packages/vinext/src/shims/router.ts` (+2 -2) 📝 `tests/app-router.test.ts` (+63 -0) 📝 `tests/fixtures/app-basic/next.config.ts` (+5 -0) ➕ `tests/fixtures/pages-basic/dist/server/entry.js` (+4140 -0) 📝 `tests/shims.test.ts` (+21 -2) </details> ### 📄 Description ## Summary Closes two remaining security gaps identified during the vulnerability audit, fixes a missed backslash normalization path, and unifies `isExternalUrl` across all copies. ### Gap 1 — GHSA-w7vc-6jjg-498h (shell injection in init.ts) `init.ts:224` used `execSync` with string concatenation. All inputs are hardcoded package names so practical risk is near zero, but `deploy.ts` and `cli.ts` were already migrated to `execFileSync` — `init.ts` was missed. **Fix:** `execSync` → `execFileSync` with `cmd.split(" ")` argument array. The `_exec` test callback interface is unchanged. ### Gap 2 — GHSA-pf96-3m7r-pv3j (credential leak on external proxy) `app-dev-server.ts:1118-1137` has an inline `__proxyExternalRequest` in the generated RSC entry that forwarded Cookie, Authorization, and x-api-key headers to external rewrite targets. The fixed version in `config-matchers.ts` strips all of these plus adds a 30s timeout — the dev-server copy was not updated. **Fix:** Strip `cookie`, `authorization`, `x-api-key`, `proxy-authorization`, and all `x-middleware-*` headers. Add 30-second `AbortSignal.timeout`. Matches the hardened `proxyExternalRequest` in `config-matchers.ts`. ### Bonus — GHSA-79rw-r8hc-qg8v (backslash bypass, missed GSP path) The GSSP redirect path in `dev-server.ts` was already fixed in PR #105, but the GSP redirect path (line ~520) was missed. Now both paths normalize backslashes. ### isExternalUrl unification (defense-in-depth) Four copies of `isExternalUrl` existed with inconsistent checks: - `config-matchers.ts` and `app-dev-server.ts` only checked `http://` and `https://` (missing `//`) - `router.ts` and `navigation.ts` checked `http://`, `https://`, and `//` but missed exotic schemes All four now use the same RFC 3986 scheme regex: ```js /^[a-z][a-z0-9+.-]*:/i.test(url) || url.startsWith("//") ``` This detects `data:`, `javascript:`, `blob:`, `ftp:`, and any valid URL scheme, plus protocol-relative URLs. ### Tests - Integration test for proxy credential stripping (mock HTTP server + conditional external rewrite) - `isExternalUrl` tests expanded: exotic schemes, protocol-relative, hash-only, bare strings ### Files changed | File | Change | |------|--------| | `init.ts` | `execSync` → `execFileSync` | | `app-dev-server.ts` | Inline proxy: strip credentials + 30s timeout; `__isExternalUrl` unified | | `dev-server.ts` | Backslash normalization in GSP redirect path | | `config-matchers.ts` | `isExternalUrl`: add `//` + exotic scheme detection | | `router.ts` | `isExternalUrl`: RFC 3986 regex | | `navigation.ts` | `isExternalUrl`: RFC 3986 regex | | `app-router.test.ts` | Integration test for proxy credential stripping | | `next.config.ts` (fixture) | Conditional external rewrite for test | | `shims.test.ts` | `isExternalUrl` exotic scheme + protocol-relative tests | | `dist/server/entry.js` (fixture) | Pre-built entry updated | **All 1981 tests pass.** --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>