Fix/security hardening - #150
Merged
Merged
Conversation
Harden the build CLI's child-process handling, clear all outstanding
dependency advisories, and add the cross-platform and cross-TypeScript
CI that was missing.
Shell injection
---------------
Every spawnSync call in the build path used `shell: true` with
interpolated, unquoted paths, so argument values could be reinterpreted
as shell syntax rather than passed through literally. Path validation
upstream checks shape, not content, and does not make that safe.
Removed `shell: true` from all three call sites. No shell is needed:
`wizer` and `process.execPath` are absolute paths to binaries. Node
flags this exact pattern as DEP0190.
Two details were load-bearing on the shell and had to change with it:
- `-r _start=wizer.resume` was a single argv entry containing a space
and only worked because the shell re-split it; it is now two entries
- `process.execPath` was wrapped in literal quotes for the shell
This also fixes a latent bug: paths were interpolated unquoted, so any
input or output path containing a space failed to build.
Note this closes a gap that could not be fixed from the callers.
FastEdge-mcp-server already spawns `fastedge-build` with `shell: false`,
but that hardening ended at the process boundary; it has to hold here
too.
TypeScript invocation and version compatibility
-----------------------------------------------
The TypeScript used to check a consumer's entrypoint is whichever
version *they* installed, so this has to work across majors that both
invoke and configure differently.
`npx` is no longer used at all. It is a `.cmd` shim on Windows, and
since the fix for CVE-2024-27980 Node refuses to spawn `.bat`/`.cmd`
without `shell: true` - so an `npx.cmd` workaround would have made every
Windows TypeScript build fail, or forced the shell back. Instead the
consumer's `typescript` is resolved directly and run under
`process.execPath`, which needs no shell anywhere. The version now comes
from package.json rather than a second child process, removing a spawn
site entirely.
Resolution goes via `typescript/package.json` and that manifest's own
`bin` field rather than the `typescript/bin/tsc` subpath, because TS 7
ships an "exports" map that does not expose it - asking for that subpath
throws on TS 7 while working on TS 5.
`--moduleResolution node` means node10, which TS 5 deprecated and TS 7
removed outright, so every .ts build failed with TS5108 once a consumer
had TS 7 installed. This was the cause of 4 pre-existing failures in
test:integration on main. Switched to `bundler` (supported from TS 5.0,
and an accurate description of how input is actually consumed, since it
is bundled with esbuild first).
`--ignoreDeprecations` is kept for a consumer's own tsconfig, which may
use options that are deprecated but still functional - on TS 5.0.4,
preserveValueImports, keyofStringsOnly, noImplicitUseStrict and
suppressImplicitAnyIndexErrors are all rejected without it, so dropping
it would break projects that build today. The value tracks the major
that deprecated them: TS 6 rejects "5.0" with TS5107 and demands "6.0".
It is omitted from TS 7 on, where those options are removed and no value
suppresses anything. Our own default flags use no deprecated option, so
they pass none.
CI
--
Nothing validated the build off Linux or against more than one
TypeScript version, and test:integration ran only on a self-hosted
runner inside the release path, so pull requests never exercised it.
- new cross-platform-tests.yaml: ubuntu/windows/macos matrix, each
platform independently typechecks, builds the CLI/libs/types and
runs the full JS -> wasm pipeline. Gates npm_release. workflow_call
only: the runtime wasm comes from build-libs in the same run, which
a manual dispatch could not provide.
- new integration-tests/cross-platform-build.test.js: wasm output,
spaces in input and output paths, literal argv passing, and
per-platform checks that argument values are never interpreted by a
shell. Verified to fail 4 of 5 against the pre-fix code.
- new integration-tests/typescript-versions.test.js: installs real
TypeScript 5.0.4, 5.9.3, 6.0.0-beta and 7.0.2 and asserts per-major
behaviour,
including that a consumer tsconfig using node10 is reported rather
than silently mis-compiled on TS 7, and that suppressible TS 5
deprecations still build. Child installs carry their own timeout,
since spawnSync blocks Jest's event loop and Jest's timeout cannot
interrupt a stalled install.
- the runtime wasm reaches the matrix as an artifact, not the build
cache: actions/cache does not share entries across operating
systems, so Windows and macOS could never restore a Linux-written
cache. The existing cache and its key are left untouched.
- only the two jobs that need Vault/Harbor on gc.onl (build-libs,
prod-invocation) stay self-hosted. code-validation and unit-tests
were reachable from fork pull requests with no internal dependency,
which should not run on a self-hosted runner; the rest move to
GitHub-hosted, which is free for public repositories.
Dependencies
------------
pnpm audit: 1 critical, 24 high, 29 moderate, 4 low -> 0.
All were in workspace packages (github-pages, examples/*) or
devDependencies, so this is dev and CI exposure only.
The published set is types/, bin/fastedge-{assets,build,init}.js,
lib/*.wasm, lib/*.js and README.md. Since bin/*.js and lib/*.js are
esbuild bundles, a dependency they import would be inlined into the
published artifact, so the file list alone does not settle the question.
Verified with esbuild metafiles: lib/create-static-server.js has 9
inputs, all first-party src/; bin/*.js inline 42 npm packages, none of
which appears in the advisory list.
The existing overrides block had gone stale (fast-uri ^3.1.1 no longer
covered the >=3.1.6 advisory) and is refreshed, plus direct bumps of
astro, sharp and hono.
Verified: 432 unit tests, 58 integration tests (4 were failing on main),
lint and typecheck clean, pnpm audit clean.
Full vector details are held for the security advisory rather than
recorded here.
Updated comment to include README.md in the transitive-only advisory fixes. Co-authored-by: Copilot Autofix powered by AI <[email protected]>
fix(security): never spawn build tooling through a shell
…he repo
A manual mock literally named `fastedge::fs.ts` made the repository
impossible to clone on Windows: `:` is not a legal NTFS path character, so
actions/checkout aborts with `invalid path` and exit 128 before any build
step runs.
The file only existed to make `fastedge::fs` resolvable - jest-haste-map
registers anything under a `__mocks__` directory as a project-wide manual
mock, and jest.mock() with a factory still needs the module to resolve.
`fastedge::fs` is provided by the FastEdge runtime and has no on-disk
module, so `{ virtual: true }` is the right mechanism.
…dk-js into fix/security-hardening
Fix/security hardening
prependNodeShebangToFile passed an already-absolute path from path.resolve through `new URL(..., import.meta.url)` and fileURLToPath. That is a no-op on POSIX, where the leading `/` resolves against the file: base, but on Windows an absolute path starts with a drive letter: `D:\a\...` parses as scheme `d:`, so fileURLToPath throws ERR_INVALID_URL_SCHEME and build:cli fails. fs takes filesystem paths, so path.resolve alone is what was needed.
…dk-js into fix/security-hardening
Fix/security hardening
TS 7 (Go rewrite) errors with TS5112 when a file is passed on the command line and a tsconfig.json is present in the CWD. [email protected] (bundled in 2.5.2-beta.1) brings TypeScript 7 in as a transitive dependency, so any consumer project with a tsconfig.json will hit this error without the flag. Our defaultTscBuildFlags already controls all compiler options explicitly, so ignoring any ambient tsconfig is correct behaviour.
…irect file arg Covers the exact trigger the --ignoreConfig fix guards: a .ts input passed directly on the CLI with a tsconfig.json present in the CWD. The prior matrix omitted this combination, so the fix could have been silently dropped without a test failure.
…dk-js into fix/security-hardening
… test to version matrix TS5112 (ambient tsconfig.json + CLI file arg) is triggered by TS 6.0.0-beta as well as TS 7; the previous major >= 7 condition was too narrow. The regression case is moved from fastedge-build.test.js (unpinned npm install) into typescript-versions.test.js so it runs against every pinned major in the matrix. The unpinned variant is removed.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain around cleanup, repeated installs, and the unsupported Undici override.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens child-process execution and Windows path handling while adding TypeScript compatibility coverage and CI/dependency updates.
Changes:
- Replaces shell-based process execution with direct argument invocation.
- Adds TypeScript-version and cross-platform integration tests.
- Updates dependencies, workflows, artifacts, and test fixtures.
File summaries
| File | Summary / final review note |
|---|---|
src/utils/syntax-checker.ts |
Resolves and invokes local TypeScript directly. |
src/server/static-assets/asset-loader/__tests__/inline-asset.test.ts |
Uses a virtual runtime-module mock. |
src/server/static-assets/asset-loader/__mocks__/fastedge::fs.ts |
Removes an invalid Windows path mock. |
src/componentize/componentize.ts |
Removes shell-based Wizer invocation. |
src/componentize/__tests__/componentize.test.ts |
Updates Wizer invocation expectations. |
pnpm-workspace.yaml |
Adds dependency overrides. Moderate (1 vote): avoid forcing undici@7 onto @actions/[email protected] outside its supported range. |
pnpm-lock.yaml |
Refreshes dependency resolutions. |
package.json |
Adds cross-platform testing and updates Hono. |
integration-tests/typescript-versions.test.js |
Adds TypeScript compatibility tests. Moderate (1 vote): reuse installs rather than running npm install for every case. Nit (1 vote): update the testing guide for the new suites. |
integration-tests/cross-platform-build.test.js |
Adds cross-platform build and path-safety coverage. Moderate (2 votes): guard cleanup when setup leaves workDir undefined. |
github-pages/package.json |
Updates Astro dependencies. |
examples/static-assets/package.json |
Updates Hono. |
examples/react-with-hono-server/package.json |
Updates Hono and node-server. |
examples/mcp-server/package.json |
Updates Hono. |
esbuild/cli-binaries.js |
Fixes Windows absolute-path handling. |
context/CHANGELOG.md |
Documents hardening and CI changes. |
.github/workflows/unit-tests.yaml |
Moves tests to hosted runners. |
.github/workflows/pr-tests.yaml |
Restricts workflow permissions. |
.github/workflows/docs.yaml |
Moves documentation jobs to hosted runners. |
.github/workflows/deploy.yaml |
Gates releases on cross-platform tests. |
.github/workflows/cross-platform-tests.yaml |
Adds an operating-system matrix build workflow. |
.github/workflows/code-validation.yaml |
Moves validation to hosted runners. |
.github/workflows/build-libs.yaml |
Publishes runtime artifacts. |
Review details
Suppressed comments (3)
integration-tests/typescript-versions.test.js:63
- This setup runs
npm installonce for every test case:describe.eachhas six tests for each of four versions, so a normal integration run performs 24 network installs sequentially. A registry outage can therefore consume up to roughly 48 minutes from the per-install timeout alone and make the release job unnecessarily slow; install each TypeScript version once perdescribe/sandbox (or otherwise reuse a prepared dependency cache) while resetting only the test files between cases.
const env = await prepareEnvironment();
const install = spawnSync(
'npm',
['install', `typescript@${version}`, '--no-audit', '--no-fund'],
integration-tests/typescript-versions.test.js:1
- These two new integration suites make
context/development/TESTING_GUIDE.mdstale: it still saysintegration-tests/contains three files and lists only the old suites (lines 37-46), omitting this TypeScript-version suite andcross-platform-build.test.js. Please update the guide so the documented test layers and commands match the repository.
/**
pnpm-workspace.yaml:30
- This blanket override forces
undici@7onto@actions/[email protected]; the lockfile shows that unchanged package previously resolved[email protected], so the override is outside its declared major range.@actions/http-clientis pulled into the semantic-release/npm path via@actions/core, and this unsupported major substitution can break publishing at runtime. Keep the override within the parent package's supported range, or upgrade the parent package to one that supports Undici 7 and verify the release workflow.
undici: '^7.29.0'
- Files reviewed: 22/23 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
qrdl
approved these changes
Sep 16, 2026
|
🎉 This PR is included in version 2.5.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This has been released on 2.5.2-beta.2 and tested against example apps and fastedge-test integrations.
Security review by opus/fable
Adds extra CI/CD tests: