| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…ng the workspace via symlinks A schematic/migration write can escape the workspace root via a symlinked directory inside the workspace: ScopedHost's containment is lexical and does not resolve symlinks. WorkspaceRootHost resolves the real (symlink-collapsed) path and rejects any write/delete/rename whose real location is outside the workspace root, mirroring the MCP host's realpath-based restriction.
There was a problem hiding this comment.
This pull request introduces WorkspaceRootHost, a custom ScopedHost that prevents schematic operations (write, delete, rename) from escaping the workspace root via symlinks by resolving real paths. The review feedback highlights a potential crash during initial workspace creation (e.g., ng new) when the root directory does not exist yet, as realpathSync will throw an ENOENT error. To resolve this, the reviewer suggests extracting a robust resolveRealPath helper that walks up to the first existing ancestor, and importing basename to support this helper.
Sorry, something went wrong.
| /** | ||
| * A {@link virtualFs.ScopedHost} that additionally rejects any write/delete/rename whose real | ||
| * (symlink-resolved) location escapes the workspace root. | ||
| * | ||
| * The lexical containment of `ScopedHost` (and the schematics `Tree`, which rejects `..`) does not | ||
| * resolve symlinks, so a workspace that contains a symlinked directory could otherwise route a | ||
| * schematic/migration write to a file outside the workspace. This mirrors the realpath-based root | ||
| * restriction already used by the MCP host (`createRootRestrictedHost`). | ||
| */ | ||
| class WorkspaceRootHost<T extends object> extends virtualFs.ScopedHost<T> { | ||
| private readonly _systemRoot: string; | ||
|
|
||
| constructor(delegate: virtualFs.Host<T>, root: Path) { | ||
| super(delegate, root); | ||
| this._systemRoot = realpathSync(getSystemPath(root)); | ||
| } | ||
|
|
||
| private _assertWithinRoot(path: Path): void { | ||
| // Resolve the real path, walking up to the first existing ancestor for not-yet-created files. | ||
| let current = resolveSystemPath(getSystemPath(this._resolve(path))); | ||
| let real: string; | ||
| for (;;) { | ||
| try { | ||
| real = realpathSync(current); | ||
| break; | ||
| } catch (e) { | ||
| if ((e as NodeJS.ErrnoException).code !== 'ENOENT') { | ||
| throw e; | ||
| } | ||
| const parent = dirname(current); | ||
| if (parent === current) { | ||
| throw e; | ||
| } | ||
| current = parent; | ||
| } | ||
| } | ||
|
|
||
| const rel = relative(this._systemRoot, real); | ||
| if (rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel)) { | ||
| throw new Error( | ||
| `Schematic attempted to access a path outside of the workspace root: ` + | ||
| getSystemPath(this._resolve(path)), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| override write(path: Path, content: virtualFs.FileBuffer): Observable<void> { | ||
| this._assertWithinRoot(path); | ||
|
|
||
| return super.write(path, content); | ||
| } | ||
|
|
||
| override delete(path: Path): Observable<void> { | ||
| this._assertWithinRoot(path); | ||
|
|
||
| return super.delete(path); | ||
| } | ||
|
|
||
| override rename(from: Path, to: Path): Observable<void> { | ||
| this._assertWithinRoot(from); | ||
| this._assertWithinRoot(to); | ||
|
|
||
| return super.rename(from, to); | ||
| } | ||
| } |
There was a problem hiding this comment.
If the workspace root directory does not exist yet (for example, during a schematic execution like ng new that creates the workspace directory), realpathSync(getSystemPath(root)) in the constructor will throw an ENOENT error and crash the process.
To make this robust, we can extract a helper function resolveRealPath that resolves the real path of any path (including the workspace root and target files) by walking up to the first existing ancestor, resolving its real path, and then appending the remaining non-existent segments. This also simplifies the _assertWithinRoot method by removing duplicate loop logic.
/**
* Resolves the real path of a system path, walking up to the first existing ancestor
* if the path or its descendants do not exist, and preserving the non-existent segments.
*/
function resolveRealPath(systemPath: string): string {
let current = resolveSystemPath(systemPath);
const segments: string[] = [];
for (;;) {
try {
const real = realpathSync(current);
return resolveSystemPath(real, ...segments.reverse());
} catch (e) {
if ((e as NodeJS.ErrnoException).code !== 'ENOENT') {
throw e;
}
const parent = dirname(current);
if (parent === current) {
throw e;
}
segments.push(basename(current));
current = parent;
}
}
}
/**
* A {@link virtualFs.ScopedHost} that additionally rejects any write/delete/rename whose real
* (symlink-resolved) location escapes the workspace root.
*
* The lexical containment of `ScopedHost` (and the schematics `Tree`, which rejects `..`) does not
* resolve symlinks, so a workspace that contains a symlinked directory could otherwise route a
* schematic/migration write to a file outside the workspace. This mirrors the realpath-based root
* restriction already used by the MCP host (`createRootRestrictedHost`).
*/
class WorkspaceRootHost<T extends object> extends virtualFs.ScopedHost<T> {
private readonly _systemRoot: string;
constructor(delegate: virtualFs.Host<T>, root: Path) {
super(delegate, root);
this._systemRoot = resolveRealPath(getSystemPath(root));
}
private _assertWithinRoot(path: Path): void {
const systemPath = resolveSystemPath(getSystemPath(this._resolve(path)));
const real = resolveRealPath(systemPath);
const rel = relative(this._systemRoot, real);
if (rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel)) {
throw new Error(
`Schematic attempted to access a path outside of the workspace root: ` +
getSystemPath(this._resolve(path)),
);
}
}
override write(path: Path, content: virtualFs.FileBuffer): Observable<void> {
this._assertWithinRoot(path);
return super.write(path, content);
}
override delete(path: Path): Observable<void> {
this._assertWithinRoot(path);
return super.delete(path);
}
override rename(from: Path, to: Path): Observable<void> {
this._assertWithinRoot(from);
this._assertWithinRoot(to);
return super.rename(from, to);
}
}
Sorry, something went wrong.
| 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, sep } from 'node:path'; |
There was a problem hiding this comment.
Import basename from node:path to support extracting path segments when resolving the real path of non-existent directories.
| import { dirname, isAbsolute, relative, resolve as resolveSystemPath, sep } from 'node:path'; | |
| import { basename, dirname, isAbsolute, relative, resolve as resolveSystemPath, sep } from 'node:path'; |
Sorry, something went wrong.
…ce-root containment The previous WorkspaceRootHost resolved the workspace root with realpathSync(getSystemPath(root)) in the constructor, which throws ENOENT when the root directory does not exist yet — e.g. during `ng new`, which creates the workspace — crashing the workflow. Extract a resolveRealPath helper that walks up to the first existing ancestor, resolves its real path, and re-appends the remaining non-existent segments. Use it for both the workspace root and the asserted target path, so containment works for not-yet-created files and a not-yet-created root while still rejecting symlink escapes.
|
Good catch — fixed. realpathSync on the root in the constructor would indeed throw ENOENT during ng new (root not created yet). I extracted a resolveRealPath helper that walks up to the first existing ancestor and re-appends the non-existent trailing segments, and use it for both the workspace root and the asserted target path. Symlink escapes are still rejected (a symlinked existing ancestor resolves to its real outside-root location before the segments are re-appended), and not-yet-created files/roots no longer crash. |
Sorry, something went wrong.
|
Gentle ping on this — it's been open about a month with green CI and the CLA signed, and only automated review so far. It's a focused, single-file change: the schematics NodeWorkflow host now rejects write / delete / rename whose real (symlink-resolved) path escapes the workspace root, mirroring the realpathSync containment the CLI already uses in the MCP host (createRootRestrictedHost). Validated against the published packages — a real use-application-builder migration through a symlinked karmaConfig is now rejected, while the same in-workspace migration still applies unchanged. Happy to add a unit test if someone can point me at the preferred fixture style for a symlinked-workspace case (the existing node-workflow_spec.ts is minimal). Would appreciate a review whenever someone has a moment — thanks! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Reopens #33325 (the previous PR was accidentally closed; this is the same single-file fix rebased cleanly onto current main).
A schematic / migration write can escape the workspace root via a symlinked directory inside the workspace. The schematics NodeWorkflow writes through virtualFs.ScopedHost, whose containment is lexical (it joins the path under the root but does not resolve symlinks), and the schematics Tree only rejects ... So a workspace that contains a symlinked directory can route a write/delete/rename to a file outside the workspace.
Fix
WorkspaceRootHost extends virtualFs.ScopedHost resolves the real (symlink-collapsed) path with realpathSync (walking up to the first existing ancestor for not-yet-created files) and rejects any write/delete/rename whose real location is outside the workspace root (rel === ".." || rel.startsWith(".." + sep) || isAbsolute(rel)). This mirrors the realpath-based root restriction already used by the MCP host (createRootRestrictedHost). No change for legitimate in-workspace paths.
Single file changed: packages/angular_devkit/schematics/tools/workflow/node-workflow.ts (+70 −1).