fix(@angular-devkit/schematics): prevent schematic writes from escaping the workspace via symlinks#33325
Conversation
…ng the workspace via symlinks The schematics `Tree` and `ScopedHost` confine writes to the workspace only lexically: `_normalizePath` rejects `..` escapes, and `ScopedHost._resolve` joins paths to the workspace root. But the real-filesystem commit (`NodeJsSyncHost.write`/`delete`/`rename`) uses `writeFileSync`/`rmSync`/ `renameSync`, which follow symlinks, with no realpath check. So if a workspace contains a symlinked directory pointing outside it (e.g. from a cloned repo), a built-in schematic or `ng update` migration writing a lexically in-workspace path can create/overwrite/delete a file outside the workspace. This wraps the NodeWorkflow's host so write/delete/rename assert that the real (symlink-resolved) path stays within the workspace root, mirroring the realpath-based restriction already used by the MCP host (`createRootRestrictedHost`). In-workspace operations are unaffected. Verified against the published packages: a real `use-application-builder` migration whose `karmaConfig` resolves through a symlinked directory no longer overwrites the outside target, while the same migration on an in-workspace config still applies.
There was a problem hiding this comment.
Code Review
This pull request introduces WorkspaceRootHost, a custom host extending virtualFs.ScopedHost that prevents schematic operations (write, delete, rename) from escaping the workspace root via symlinks by resolving real paths. The review feedback correctly identifies a potential bug in the path containment check where rel.startsWith('..') could trigger false positives for paths starting with double dots (e.g., ..foo), and suggests using sep from node:path to safely validate path boundaries.
| import { Path, getSystemPath, normalize, schema, virtualFs } from '@angular-devkit/core'; | ||
| import { NodeJsSyncHost } from '@angular-devkit/core/node'; | ||
| import { realpathSync } from 'node:fs'; | ||
| import { dirname, isAbsolute, relative, resolve as resolveSystemPath } from 'node:path'; |
There was a problem hiding this comment.
Import sep from node:path to safely check for path boundaries when validating if a path escapes the workspace root.
| import { dirname, isAbsolute, relative, resolve as resolveSystemPath } from 'node:path'; | |
| import { dirname, isAbsolute, relative, resolve as resolveSystemPath, sep } from 'node:path'; |
| } | ||
|
|
||
| const rel = relative(this._systemRoot, real); | ||
| if (rel.startsWith('..') || isAbsolute(rel)) { |
There was a problem hiding this comment.
Using rel.startsWith('..') can cause false positives if a file or directory inside the workspace starts with two dots (e.g., ..foo or ...bar). In such cases, relative will return ..foo, which starts with .. but is actually a valid path inside the workspace.
To prevent this, check if the relative path is exactly '..' or starts with '..' + sep.
| if (rel.startsWith('..') || isAbsolute(rel)) { | |
| if (rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel)) { |
The schematics
TreeandScopedHostconfine writes to the workspace lexically only:_normalizePathrejects..escapes andScopedHost._resolvejoins paths to the workspace root. But the real-filesystem commit (NodeJsSyncHost.write/delete/rename) useswriteFileSync/rmSync/renameSync, which follow symlinks, with no realpath check (grep lstat|realpathSync|O_NOFOLLOWacrosscore/node+core/src/virtual-fsreturns nothing).So if a workspace contains a symlinked directory pointing outside it (e.g. shipped by a cloned repository), a built-in schematic or
ng updatemigration writing a lexically in-workspace path can create/overwrite/delete a file outside the workspace.Fix
Wrap the
NodeWorkflowhost sowrite/delete/renameassert that the real (symlink-resolved) path stays within the workspace root, walking up to the first existing ancestor for not-yet-created files. This mirrors the realpath-based root restriction the CLI already implements in the MCP host (createRootRestrictedHost). In-workspace operations are unaffected.Validation (against the published packages)
use-application-buildermigration whosekarmaConfigresolves through a symlinked directory (src/symdir -> ../../outside) no longer overwrites the outside target (the write is rejected); without the fix it overwrote the outside file.(Happy to add a dedicated spec — the existing
node-workflow_spec.tsis minimal; pointing me at the preferred test style for a symlinked-workspace fixture would help.)