From 48da8e149dd4fd17988bd1e0e311f0c35669c52d Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Wed, 9 Sep 2026 11:52:21 -0400 Subject: [PATCH 1/2] fix(apps): close remaining env-guard review findings on FileHandle fd, cp options, and process.env assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes four bypass/reliability gaps found in review: - A FileHandle's own fd is caller-controlled and could be shadowed with an own property to report a harmless value to the environ-path check while the real read still operated on the handle's actual fd. Fixed by capturing FileHandle.prototype's native fd getter once, at patch-install time, and invoking it directly to bypass any later own-property shadow. - fs.cp/cpSync/promises.cp's recursive+dereference check read options.recursive/dereference once for its decision, then forwarded the caller's original options object to the real implementation, which read the same properties again — a getter-backed options object could report false to the check and true to the real call. Fixed by snapshotting both values into plain data properties before forwarding, the same pattern already used for options.fd. - Reassigning process.env rejected only null/undefined, letting a primitive (e.g. a number) through to become the new realEnv fallback and break every later unscoped access. Fixed by validating the runtime value is a non-null object. - fs.openAsBlob was wrapped unconditionally, even though it's absent on Node 18 — replacing the real `undefined` with an always-defined wrapper broke the existing feature-detection fallback in packages/core/src/helpers/fs.ts. Fixed by feature-detecting the same way before wrapping. Co-Authored-By: Claude Sonnet 5 --- .../plugins/apps/src/vite/env-guard.test.ts | 434 +++++++++++++++++- packages/plugins/apps/src/vite/env-guard.ts | 265 +++++++++-- .../plugins/apps/src/vite/guarded-wrapper.ts | 6 +- 3 files changed, 648 insertions(+), 57 deletions(-) diff --git a/packages/plugins/apps/src/vite/env-guard.test.ts b/packages/plugins/apps/src/vite/env-guard.test.ts index 0ba6fb362..144396f80 100644 --- a/packages/plugins/apps/src/vite/env-guard.test.ts +++ b/packages/plugins/apps/src/vite/env-guard.test.ts @@ -348,29 +348,32 @@ describe('env-guard', () => { // Reflect.get throws for a non-object value, and isEnvProxy() is the setter's first check on // whatever gets assigned — without its own object/null guard, `process.env = null` (or - // undefined) would surface as an unhandled native TypeError instead of either this file's own - // clear rejection message (from inside a scope) or a graceful no-op (from outside one). - test('Should not throw a native TypeError when process.env is reassigned to null or undefined', () => { - // Each reassignment restored individually, not both bundled under one final restore: - // each is its own real reassignment, and a single self-assignment only undoes the one - // immediately before it. - const beforeNull = process.env; - try { - expect(() => { - process.env = null as unknown as NodeJS.ProcessEnv; - }).not.toThrow(); - } finally { - process.env = beforeNull; - } + // undefined) would surface as an unhandled native TypeError instead of this file's own clear + // rejection message. + test('Should reject reassigning process.env to null or undefined with a clear error, not a native TypeError', () => { + const originalPath = process.env.PATH; - const beforeUndefined = process.env; - try { - expect(() => { - process.env = undefined as unknown as NodeJS.ProcessEnv; - }).not.toThrow(); - } finally { - process.env = beforeUndefined; - } + expect(() => { + process.env = null as unknown as NodeJS.ProcessEnv; + }).toThrow(/process\.env/i); + expect(() => { + process.env = undefined as unknown as NodeJS.ProcessEnv; + }).toThrow(/process\.env/i); + + expect(process.env.PATH).toBe(originalPath); + }); + + // A number or string reassigned to process.env would corrupt realEnv the same way + // null/undefined does, so the guard covers every non-object primitive, not just the two + // nullish ones. + test('Should reject reassigning process.env to a primitive (e.g. a number), not corrupt the real environment', () => { + const originalPath = process.env.PATH; + + expect(() => { + process.env = 1 as unknown as NodeJS.ProcessEnv; + }).toThrow(/process\.env/i); + + expect(process.env.PATH).toBe(originalPath); }); // Regression coverage: this file gets evaluated more than once in practice (Jest's @@ -452,6 +455,19 @@ describe('env-guard', () => { expect(process.env.POST_ATTEMPT_KEY).toBe('still-writable'); delete process.env.POST_ATTEMPT_KEY; }); + + // A non-configurable definition can never satisfy the Proxy invariant against + // INERT_PROXY_TARGET (always empty), so it must throw a clear, guard-specific error rather + // than a cryptic native Proxy TypeError — even outside any scope, since the target is + // permanently empty regardless of scope state. + test('Should throw a clear error for Object.defineProperty(process.env, key, { configurable: false })', () => { + expect(() => + Object.defineProperty(process.env, 'LOCKED_KEY', { + value: 'x', + configurable: false, + }), + ).toThrow(/non-configurable/); + }); }); // Regression coverage for the /proc/.../environ backing-store bypass: swapping process.env alone doesn't stop reads of the kernel-backed environ file directly on Linux. @@ -829,6 +845,54 @@ describe('env-guard', () => { }); }); + // fs.openAsBlob is its own entry point, separate from open*/readFile* above, so it needs its + // own guard coverage. + describe('fs.openAsBlob guard', () => { + test('Should block fs.openAsBlob("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.openAsBlob('/proc/self/environ')).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should not block fs.openAsBlob for an unrelated real file during an active scoped-env window', async () => { + const tmpFile = path.join( + os.tmpdir(), + `env-guard-openasblob-ok-${process.pid}.txt`, + ); + fs.writeFileSync(tmpFile, 'hello world'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const blob = await fs.openAsBlob(tmpFile); + await expect(blob.text()).resolves.toBe('hello world'); + }); + } finally { + fs.rmSync(tmpFile, { force: true }); + } + }); + + // Deleting fs.openAsBlob and re-evaluating the module simulates Node 18, which has no + // fs.openAsBlob, matching the feature-detection in packages/core/src/helpers/fs.ts's + // getFile(). + test('Should leave fs.openAsBlob undefined, not replace it with a broken wrapper, when the real function is absent', () => { + const originalOpenAsBlob = fs.openAsBlob; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (fs as any).openAsBlob; + try { + expect(() => { + jest.isolateModules(() => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + require('./env-guard'); + }); + }).not.toThrow(); + expect(fs.openAsBlob).toBeUndefined(); + } finally { + fs.openAsBlob = originalOpenAsBlob; + } + }); + }); + test('Should block a Buffer or URL path pointing at /proc/self/environ, not just a string path', async () => { await runWithScopedEnv({ PATH: '/scoped' }, async () => { const environPathAsBuffer = Buffer.from('/proc/self/environ'); @@ -1142,6 +1206,14 @@ describe('env-guard', () => { } }); + // A malformed fs.cp call throws synchronously from Node's own argument validation, not from + // the guard, so it must propagate unguarded like every other entry point here. + test('Should let a malformed fs.cp call (no callback) throw synchronously, not swallow the error', () => { + expect(() => { + (fs.cp as unknown as (src: string, dest: string) => void)('/tmp', '/tmp/x'); + }).toThrow(/must be of type function/i); + }); + // new fs.ReadStream(path) constructs directly, bypassing the createReadStream factory the // guard above wraps, so it needs separate coverage. @types/node declares no (path, options) // constructor for ReadStream, so Reflect.construct invokes the real, untyped signature @@ -1179,6 +1251,144 @@ describe('env-guard', () => { } }); + // fs.FileReadStream is a real, long-deprecated alias for fs.ReadStream — a separate property + // slot that must be re-pointed at the same wrapped class, or constructing through this name + // bypasses the guard above entirely. + describe('fs.FileReadStream alias guard', () => { + function constructFileReadStream(rawPath: string): fs.ReadStream { + const stream: fs.ReadStream = Reflect.construct(fs.FileReadStream, [rawPath]); + stream.on('error', () => {}); + return stream; + } + + test('Should block constructing new fs.FileReadStream("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => constructFileReadStream('/proc/self/environ')).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should not block constructing new fs.FileReadStream(...) for an unrelated real file during an active scoped-env window', async () => { + const tmpFile = path.join( + os.tmpdir(), + `env-guard-filereadstream-${process.pid}.txt`, + ); + fs.writeFileSync(tmpFile, 'not a secret'); + let stream: fs.ReadStream | undefined; + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => { + stream = constructFileReadStream(tmpFile); + }).not.toThrow(); + }); + } finally { + stream?.destroy(); + fs.rmSync(tmpFile); + } + }); + }); + + // fs.read/readSync/readv/readvSync take an fd directly, the same case toPathString()'s + // /proc/self/fd resolution already covers elsewhere — mocked here so that resolution runs + // on every OS, not just Linux. + describe('fd-based read guard (fs.read/readSync/readv/readvSync)', () => { + function mockFdResolvesToEnviron(fd: number): () => void { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); + const readlinkSyncSpy = jest + .spyOn(fs, 'readlinkSync') + .mockImplementation((linkPath) => { + expect(linkPath).toBe(`/proc/self/fd/${fd}`); + return '/proc/self/environ'; + }); + reEvaluateEnvGuardWithCurrentMocks(); + return () => { + readlinkSyncSpy.mockRestore(); + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + }; + } + + test('Should block fs.readSync(fd) when fd resolves to /proc/self/environ', async () => { + const restore = mockFdResolvesToEnviron(99); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readSync(99, Buffer.alloc(10), 0, 10, 0)).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + restore(); + } + }); + + test('Should block the callback-style fs.read(fd) via its callback, not a synchronous throw, when fd resolves to /proc/self/environ', async () => { + const restore = mockFdResolvesToEnviron(99); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const error = await callbackError((callback) => + fs.read(99, Buffer.alloc(10), 0, 10, 0, callback), + ); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch( + /not allowed in backend functions/, + ); + }); + } finally { + restore(); + } + }); + + test('Should block fs.readvSync(fd) when fd resolves to /proc/self/environ', async () => { + const restore = mockFdResolvesToEnviron(99); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readvSync(99, [Buffer.alloc(10)])).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + restore(); + } + }); + + test('Should block the callback-style fs.readv(fd) via its callback, not a synchronous throw, when fd resolves to /proc/self/environ', async () => { + const restore = mockFdResolvesToEnviron(99); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const error = await callbackError((callback) => + fs.readv(99, [Buffer.alloc(10)], callback), + ); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch( + /not allowed in backend functions/, + ); + }); + } finally { + restore(); + } + }); + + test('Should not block fs.readSync/readvSync for an unrelated real fd during an active scoped-env window', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-read-fd-${process.pid}.txt`); + fs.writeFileSync(tmpFile, 'hello world'); + const fd = fs.openSync(tmpFile, 'r'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const buffer = Buffer.alloc(5); + expect(fs.readSync(fd, buffer, 0, 5, 0)).toBe(5); + expect(buffer.toString('utf8')).toBe('hello'); + }); + } finally { + fs.closeSync(fd); + fs.rmSync(tmpFile, { force: true }); + } + }); + }); + // FileHandle isn't part of Node's public API, so ensureFileHandleReadGuarded patches its // .read() lazily, per-instance, once a handle is already open. describe('FileHandle.prototype.read guard', () => { @@ -1534,6 +1744,186 @@ describe('env-guard', () => { } }); }); + + // fs.cpSync's recursive copy calls neither fs.copyFileSync nor fs.readFileSync — Node's + // internal traversal never re-enters the wrapped entry points above, so a symlink inside the + // copied tree pointing at /proc/.../environ would have its real content copied to an + // unguarded destination with no guard ever seeing it. + describe('cp recursive+dereference guard', () => { + test('Should block fs.cpSync/fs.cp/fs.promises.cp with recursive+dereference during an active scoped-env window', async () => { + const srcDir = path.join(os.tmpdir(), `env-guard-cp-recursive-src-${process.pid}`); + const destDir = path.join( + os.tmpdir(), + `env-guard-cp-recursive-dest-${process.pid}`, + ); + fs.mkdirSync(srcDir, { recursive: true }); + fs.writeFileSync(path.join(srcDir, 'a.txt'), 'not a secret'); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => + fs.cpSync(srcDir, destDir, { recursive: true, dereference: true }), + ).toThrow(/not allowed in backend functions/); + + const error = await callbackError((callback) => + fs.cp( + srcDir, + destDir, + { recursive: true, dereference: true }, + callback, + ), + ); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch( + /not allowed in backend functions/, + ); + + await expect( + fs.promises.cp(srcDir, destDir, { + recursive: true, + dereference: true, + }), + ).rejects.toThrow(/not allowed in backend functions/); + }); + } finally { + fs.rmSync(srcDir, { recursive: true, force: true }); + fs.rmSync(destDir, { recursive: true, force: true }); + } + }); + + test('Should not block a recursive copy WITHOUT dereference during an active scoped-env window', async () => { + const srcDir = path.join(os.tmpdir(), `env-guard-cp-plain-src-${process.pid}`); + const destDir = path.join(os.tmpdir(), `env-guard-cp-plain-dest-${process.pid}`); + fs.mkdirSync(srcDir, { recursive: true }); + fs.writeFileSync(path.join(srcDir, 'a.txt'), 'not a secret'); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.cpSync(srcDir, destDir, { recursive: true })).not.toThrow(); + }); + expect(fs.readFileSync(path.join(destDir, 'a.txt'), 'utf8')).toBe( + 'not a secret', + ); + } finally { + fs.rmSync(srcDir, { recursive: true, force: true }); + fs.rmSync(destDir, { recursive: true, force: true }); + } + }); + + test('Should not block fs.cpSync for a single unrelated file with no recursive option at all', async () => { + const srcFile = path.join( + os.tmpdir(), + `env-guard-cp-single-src-${process.pid}.txt`, + ); + const destFile = path.join( + os.tmpdir(), + `env-guard-cp-single-dest-${process.pid}.txt`, + ); + fs.writeFileSync(srcFile, 'not a secret'); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.cpSync(srcFile, destFile)).not.toThrow(); + }); + expect(fs.readFileSync(destFile, 'utf8')).toBe('not a secret'); + } finally { + fs.rmSync(srcFile, { force: true }); + fs.rmSync(destFile, { force: true }); + } + }); + + test('Should still block fs.cpSync("/proc/self/environ", dest) via the plain top-level path check', async () => { + const dest = path.join( + os.tmpdir(), + `env-guard-cp-still-blocked-${process.pid}.txt`, + ); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.cpSync('/proc/self/environ', dest)).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.rmSync(dest, { force: true }); + } + }); + + // The generic makeGuardWrapper machinery forwards a caller's original options object + // unchanged — without snapshotting, a getter-backed recursive/dereference could report + // false to this check and true when Node's own fs.cp* implementation reads the same + // property again internally. + test('Should read options.recursive/options.dereference exactly once each, not read again by the real implementation', async () => { + const srcDir = path.join( + os.tmpdir(), + `env-guard-cp-recursive-once-src-${process.pid}`, + ); + const destDir = path.join( + os.tmpdir(), + `env-guard-cp-recursive-once-dest-${process.pid}`, + ); + fs.mkdirSync(srcDir, { recursive: true }); + fs.writeFileSync(path.join(srcDir, 'a.txt'), 'not a secret'); + + let recursiveReadCount = 0; + let dereferenceReadCount = 0; + const options = { + get recursive() { + recursiveReadCount += 1; + return true; + }, + get dereference() { + dereferenceReadCount += 1; + return false; + }, + }; + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => + fs.cpSync(srcDir, destDir, options as fs.CopySyncOptions), + ).not.toThrow(); + }); + expect(fs.readFileSync(path.join(destDir, 'a.txt'), 'utf8')).toBe( + 'not a secret', + ); + // Exactly 1 each: the guard's own snapshotting read, not a second, independent + // read by the real cp implementation. + expect(recursiveReadCount).toBe(1); + expect(dereferenceReadCount).toBe(1); + } finally { + fs.rmSync(srcDir, { recursive: true, force: true }); + fs.rmSync(destDir, { recursive: true, force: true }); + } + }); + }); + + // The TOCTOU test above verifies "resolve exactly once" by content; this asserts the + // invocation count directly. + test('Should read options.fd exactly once, not twice via a stray spread', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-fd-single-read-${process.pid}.txt`); + fs.writeFileSync(tmpFile, 'not a secret'); + const fd = fs.openSync(tmpFile, 'r'); + let readCount = 0; + const options = { + get fd() { + readCount += 1; + return fd; + }, + }; + let stream: fs.ReadStream | undefined; + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + stream = fs.createReadStream('/some/unrelated/path', options); + stream.on('error', () => {}); + }); + expect(readCount).toBe(1); + } finally { + stream?.destroy(); + fs.closeSync(fd); + fs.rmSync(tmpFile, { force: true }); + } + }); }); describe('process.report.excludeEnv', () => { diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts index 9d5518016..199f77203 100644 --- a/packages/plugins/apps/src/vite/env-guard.ts +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -10,7 +10,7 @@ import { syncBuiltinESMExports } from 'node:module'; import nodePath from 'path'; import { fileURLToPath } from 'url'; -import { makeGuardCallbackWrapper, makeGuardWrapper } from './guarded-wrapper'; +import { invokeCallbackArg, makeGuardCallbackWrapper, makeGuardWrapper } from './guarded-wrapper'; import { getOrCreateShared } from './shared-module-singleton'; // Captured at module load — before any customer code runs — so a backend function can't replace @@ -185,7 +185,13 @@ function getSharedState(): EnvGuardSharedState { if (activeScopeTokens.size > 0) { // An unrelated caller writing from outside any scope while a DIFFERENT scope is // still active elsewhere — applying it immediately would disarm redaction out - // from under that scope, so it's deferred until the active scope's own cleanup. + // from under that scope, so it's deferred instead. Applied and immediately + // reverted here (rather than just stashed) so Node's own setter validation still + // runs now — an invalid value throws here instead of surfacing later, + // misattributed to whichever scope's cleanup happens to apply it. + const currentValue = getExcludeEnv(); + applyExcludeEnvValue(newValue); + applyExcludeEnvValue(currentValue); savedExcludeEnv = newValue; return; } @@ -202,6 +208,15 @@ function getSharedState(): EnvGuardSharedState { "Reassigning process.env is not allowed in backend functions — it would corrupt the dev server's real environment for every future execution. Use $.Source or a declared Custom Credential instead.", ); } + if (typeof newValue !== 'object' || newValue === null) { + // isEnvProxy() only ever returns true for an object, so without this check a + // primitive assignment (process.env = 1, process.env = null) would store that + // primitive as realEnv — every later unscoped access then calls + // Reflect.get/ownKeys on it and throws, permanently breaking process.env. + throw new Error( + 'Reassigning process.env to a non-object value is not allowed — it would permanently break every future process.env access.', + ); + } realEnvHistory.push(realEnv); realEnv = newValue; }, @@ -254,6 +269,13 @@ const sharedState = getSharedState(); // get/ownKeys/etc. traps into each other forever. const ENV_PROXY_MARKER = Symbol.for('@dd/apps-plugin/env-guard/scoped-env-proxy'); +// Re-checked on every fs.promises.open() call rather than trusted as a one-time-installed flag — a +// stray jest.spyOn(...).mockRestore() elsewhere in the process, on the same shared FileHandle +// prototype, can silently strip one of these wrappers without this file ever re-running to notice. +const FILE_HANDLE_READ_GUARD_MARKER = Symbol.for( + '@dd/apps-plugin/env-guard/file-handle-read-guard', +); + // Takes `unknown`, not NodeJS.ProcessEnv: the setter below calls this on whatever a caller actually // assigns to process.env at runtime, which TypeScript's parameter typing can't constrain — a bare // `Reflect.get(value, ...)` throws for null/undefined/primitives, which would surface as a confusing @@ -276,6 +298,13 @@ function forwardToCurrentEnv( }; } +// util.inspect()/console.log() read a Proxy's target directly via V8's getProxyDetails, bypassing +// every trap below — with the real env as target, that would leak it straight through a bare +// console.log(process.env) inside a scope. An always-empty, always-extensible dummy target closes +// this: Proxy invariants only constrain traps when the target is non-extensible or holds +// non-configurable properties, never true here, so every trap still resolves through +// getCurrentEnv() as before. +const INERT_PROXY_TARGET = {} as NodeJS.ProcessEnv; // Re-checked on every runWithScopedEnv call rather than installed once and assumed permanent, since // isEnvProxy() is what actually detects "is this already installed" — the accessor property below // makes a bare `process.env = X` (rather than a call through this function) impossible to reach the @@ -285,7 +314,7 @@ function ensureEnvProxyInstalled(): void { if (isEnvProxy(process.env)) { return; } - const proxy = new Proxy(process.env, { + const proxy = new Proxy(INERT_PROXY_TARGET, { get: (_target, prop, receiver) => { if (prop === ENV_PROXY_MARKER) { return true; @@ -305,20 +334,34 @@ function ensureEnvProxyInstalled(): void { deleteProperty: forwardToCurrentEnv(Reflect.deleteProperty), ownKeys: forwardToCurrentEnv(Reflect.ownKeys), getOwnPropertyDescriptor: forwardToCurrentEnv(Reflect.getOwnPropertyDescriptor), - defineProperty: forwardToCurrentEnv(Reflect.defineProperty), + // A non-configurable definition can never be forwarded: the Proxy invariant requires + // `target` (INERT_PROXY_TARGET, always empty) to carry that exact property afterward, which + // it deliberately never does. Checked upfront rather than left to surface as Reflect's own + // invariant-violation TypeError, which gives no hint this guard is involved. Accepted gap: + // locking an env var non-configurable stops working process-wide once local execution has + // run once, in exchange for `console.log(process.env)` never bypassing the scope via V8's + // getProxyDetails (see INERT_PROXY_TARGET's own comment). + defineProperty: (_target, prop, descriptor) => { + if (descriptor.configurable === false) { + throw new Error( + `Cannot define a non-configurable property (${String(prop)}) on process.env: local execution's environment scoping requires every property to stay configurable.`, + ); + } + return Reflect.defineProperty(sharedState.getCurrentEnv(), prop, descriptor); + }, // Without this trap, Object.setPrototypeOf(process.env, ...) defaults to forwarding to - // `target` (the real, unscoped env object) and silently poisons its prototype chain - // permanently, even when called from inside a scope — since getCurrentEnv() only affects - // property access, not the object identity a prototype mutation lands on. + // `target` (INERT_PROXY_TARGET, not the real env) and silently poisons its prototype chain + // instead, even when called from inside a scope — getCurrentEnv() only affects property + // access, not the object identity a prototype mutation lands on. setPrototypeOf: forwardToCurrentEnv(Reflect.setPrototypeOf), // Paired with setPrototypeOf above: without this trap, a customer function that sets a - // scoped prototype and immediately reads it back would see `target`'s (the real env's) + // scoped prototype and immediately reads it back would see `target`'s (INERT_PROXY_TARGET's) // untouched prototype instead of the one it just set on the scoped view. getPrototypeOf: forwardToCurrentEnv(Reflect.getPrototypeOf), - // Can't forward to getCurrentEnv(): the Proxy invariants only honor a `preventExtensions` trap - // returning `true` if `target` (always the real env object) is also non-extensible, so - // routing this to the scoped object would either desync the invariant or force freezing the - // real env process-wide. Refusing outright is the only option that risks neither. + // Can't forward to getCurrentEnv(): the Proxy invariant only honors this trap returning `true` + // if `target` (INERT_PROXY_TARGET, not the real env) is also non-extensible, and freezing it + // would break every other trap's scoped view. Refusing outright is the only option that + // doesn't leak real-env state or break the proxy. preventExtensions: () => false, }); // process.env must be an accessor property, not the plain data property it started as — a bare @@ -457,17 +500,22 @@ function extractFdNumber(fdValue: unknown): unknown { // harmless value to this check and a different, real target to Node's own later read. function guardEnvironPathOrFdOption(rawPath: unknown, options: unknown): unknown { throwIfBlockedEnvironPath(rawPath); - if (typeof options !== 'object' || options === null || !('fd' in options)) { + // No scope active means nothing here can be an environ read worth blocking — returning options + // untouched (rather than destructuring/rebuilding it below) preserves whatever createReadStream/ + // ReadStream call a caller outside any scope makes, including non-enumerable or inherited + // options properties a plain spread would otherwise silently drop. + if (!sharedState.isInsideScope()) { return options; } - const fdValue = options.fd; - // Gated the same way isBlockedEnvironPath's own short-circuit is: extractFdNumber now reads a - // real FileHandle's native .fd getter, which callers outside any scope must never trigger. - if (sharedState.isInsideScope()) { - const fdNumber = extractFdNumber(fdValue); - throwIfBlockedEnvironPath(fdNumber); + if (typeof options !== 'object' || options === null || !('fd' in options)) { + return options; } - return { ...options, fd: fdValue }; + // Destructuring reads the getter exactly once, into fdValue — spreading the remainder (with fd + // already removed) can't invoke it again the way `{ ...options, fd: fdValue }` would have. + const { fd: fdValue, ...restOptions } = options as { fd: unknown }; + const fdNumber = extractFdNumber(fdValue); + throwIfBlockedEnvironPath(fdNumber); + return { ...restOptions, fd: fdValue }; } // Every guarded fs entry point below except createReadStream takes only a leading path argument — @@ -534,13 +582,6 @@ function wrapGuardedStreamFn unknown>(real: T): return wrapped as T; } -// Re-checked on every fs.promises.open() call rather than trusted as a one-time-installed flag — a -// stray jest.spyOn(...).mockRestore() elsewhere in the process, on the same shared FileHandle -// prototype, can silently strip one of these wrappers without this file ever re-running to notice. -const FILE_HANDLE_READ_GUARD_MARKER = Symbol.for( - '@dd/apps-plugin/env-guard/file-handle-read-guard', -); - // Shared by every FileHandle prototype method patched below — re-checks proto[methodName] fresh on // every fs.promises.open() call rather than trusting a one-time flag, for the same reason // FILE_HANDLE_READ_GUARD_MARKER exists. @@ -657,6 +698,7 @@ function patchFileHandleSyncMethod( proto[methodName] = guarded; } +// FileHandle isn't part of Node's public API, so read/readFile/readv/createReadStream/ // readableWebStream/readLines are patched lazily off the first real handle fs.promises.open() // returns — a handle obtained before that patch installs is unaffected, matching every other guard // here. These six are distinct prototype methods that don't delegate to each other, so each needs @@ -693,15 +735,121 @@ fs.openSync = wrapGuardedFsFn(fs.openSync); fs.open = wrapGuardedCallbackFsFn(fs.open); fs.promises.open = wrapGuardedAsyncFsFn(fs.promises.open, ensureFileHandleReadGuarded); -// copyFileSync/copyFile/promises.copyFile/cpSync/promises.cp read the source file's bytes through -// a distinct native binding that never calls through readFile*/open* above — an uncovered path that -// could otherwise copy /proc/.../environ to an ordinary, unguarded file and read it back from there. +// openAsBlob is its own entry point, separate from open*/readFile* above, and absent on Node 18. +// Feature-detected the same way packages/core/src/helpers/fs.ts's getFile() already checks for it +// — an unconditional assignment here would replace that check's `undefined` with an always-defined +// wrapper, silently forcing every Node 18 caller onto the unsupported branch. +if (typeof fs.openAsBlob === 'function') { + fs.openAsBlob = wrapGuardedAsyncFsFn(fs.openAsBlob); +} + +// read/readSync/readv/readvSync take an already-open fd as their own leading argument — the same +// shape isBlockedEnvironPath's toPathString() already resolves via /proc/self/fd for the numeric-fd +// case above, just on entry points that were never wrapped at all. +fs.read = wrapGuardedCallbackFsFn(fs.read); +fs.readSync = wrapGuardedFsFn(fs.readSync); +fs.readv = wrapGuardedCallbackFsFn(fs.readv); +fs.readvSync = wrapGuardedFsFn(fs.readvSync); + +// copyFileSync/copyFile/promises.copyFile read the source file's bytes through a distinct native +// binding that never calls through readFile*/open* above — an uncovered path that could otherwise +// copy /proc/.../environ to an ordinary, unguarded file and read it back from there. fs.copyFileSync = wrapGuardedFsFn(fs.copyFileSync); fs.copyFile = wrapGuardedCallbackFsFn(fs.copyFile); fs.promises.copyFile = wrapGuardedAsyncFsFn(fs.promises.copyFile); -fs.cpSync = wrapGuardedFsFn(fs.cpSync); -fs.cp = wrapGuardedCallbackFsFn(fs.cp); -fs.promises.cp = wrapGuardedAsyncFsFn(fs.promises.cp); + +// cp/cpSync/promises.cp additionally need to reject a recursive+dereference copy of ANY directory +// (see guardCpOptions's own comment) — a check the plain isBlockedEnvironPath(src) check above +// can't cover, since it only ever inspects the top-level source argument. +const CP_BLOCKED_MESSAGE = + "Copying /proc/.../environ, or recursively copying with dereference: true, is not allowed in backend functions — both can expose the dev server's real, unscoped environment. Use $.Source or a declared Custom Credential instead."; + +// Snapshots options.recursive/dereference into plain data properties, read exactly once — the +// generic makeGuardWrapper machinery forwards a caller's original options object unchanged, which +// would let a getter-backed recursive/dereference report false here and true when Node's own +// fs.cp* reads it again internally, letting a symlinked /proc/.../environ get dereferenced and +// copied through undetected. Same TOCTOU reasoning as guardEnvironPathOrFdOption's options.fd +// handling above; hand-rolled here since arg transformation isn't something makeGuardWrapper +// supports. +function guardCpOptions(options: unknown): unknown { + if (!sharedState.isInsideScope()) { + return options; + } + if (typeof options !== 'object' || options === null) { + return options; + } + const { + recursive: recursiveValue, + dereference: dereferenceValue, + ...restOptions + } = options as { recursive?: unknown; dereference?: unknown }; + if (recursiveValue === true && dereferenceValue === true) { + throw new Error(CP_BLOCKED_MESSAGE); + } + // Only re-adds a key that was actually present on the caller's own options — Node's cp + // implementations distinguish an absent key (defaulted internally) from one explicitly present + // with value `undefined` (rejected by its own validation), so restoring both keys unconditionally + // would turn a caller's `{ recursive: true }` (no dereference key at all) into + // `{ recursive: true, dereference: undefined }` and throw a validation error that never happens + // when the real options object is forwarded as-is. + const safeOptions = restOptions as Record; + if ('recursive' in options) { + safeOptions.recursive = recursiveValue; + } + if ('dereference' in options) { + safeOptions.dereference = dereferenceValue; + } + return safeOptions; +} + +// Captured into local consts before reassignment below — a lazy `() => fs.cpSync` getter would +// re-read the property AFTER it's replaced with this very wrapper, recursing into itself forever. +const originalCpSync = fs.cpSync; +const originalCp = fs.cp; +const originalPromisesCp = fs.promises.cp; + +// cpSync is genuinely synchronous — a guard failure throwing matches its real contract. `this` +// is forwarded via .apply, matching every other guarded fs entry point in this file — Node's own +// implementations don't consult it, but nothing here should be the one silent exception. +fs.cpSync = function (this: unknown, src: unknown, dest: unknown, options?: unknown) { + throwIfBlockedEnvironPath(src); + return originalCpSync.apply(this, [ + src as string | URL, + dest as string | URL, + guardCpOptions(options) as fs.CopySyncOptions, + ]); +} as typeof fs.cpSync; + +// cp reports failure via an error-first callback, never a synchronous throw. Only the guard's own +// decision logic runs inside the try/catch — the real call runs outside it, so a synchronous throw +// from Node's own validation propagates normally instead of being swallowed by invokeCallbackArg +// when a malformed call has no valid callback to report through. +fs.cp = function (this: unknown, src: unknown, dest: unknown, ...rest: unknown[]) { + let safeArgs: unknown[]; + try { + throwIfBlockedEnvironPath(src); + const hasOptions = rest.length > 1; + const safeOptions = hasOptions ? guardCpOptions(rest[0]) : undefined; + safeArgs = hasOptions ? [src, dest, safeOptions, ...rest.slice(1)] : [src, dest, ...rest]; + } catch (error) { + invokeCallbackArg( + [src, dest, ...rest], + error instanceof Error ? error : new Error(String(error)), + ); + return undefined; + } + return (originalCp as unknown as (...a: unknown[]) => unknown).apply(this, safeArgs); +} as typeof fs.cp; + +// promises.cp rejects, matching its real Promise-returning contract. +fs.promises.cp = async function (this: unknown, src: unknown, dest: unknown, options?: unknown) { + throwIfBlockedEnvironPath(src); + return originalPromisesCp.apply(this, [ + src as string | URL, + dest as string | URL, + guardCpOptions(options) as fs.CopyOptions, + ]); +} as typeof fs.promises.cp; // createReadStream's own wrap above only covers that factory function — Node also exports the // ReadStream class it constructs internally, and `new fs.ReadStream(path)` never calls through @@ -716,6 +864,23 @@ fs.ReadStream = new Proxy(fs.ReadStream, { }, }); +// @types/node doesn't declare fs.FileReadStream at all, even though Node itself still exports it. +// `let`, not `const` — ReadStream itself is declared as a class (an assignable binding, matching +// the reassignment already made above), and this needs to be assignable too. +declare module 'fs' { + // no-undef doesn't understand module-augmentation scoping (ReadStream is 'fs's own ambient + // class, visible here without an import); import/no-mutable-exports doesn't apply either — this + // `let` declares the shape of the 'fs' module's own property, not a real value this file exports. + // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-undef, import/no-mutable-exports + export let FileReadStream: typeof ReadStream; +} + +// fs.FileReadStream is a real, long-deprecated alias for fs.ReadStream — reassigning fs.ReadStream +// only rebinds that one property; fs.FileReadStream is a separate slot that keeps pointing at the +// original, unwrapped class, so `new fs.FileReadStream(path)` would construct through it with no +// guard at all. +fs.FileReadStream = fs.ReadStream; + // @types/node doesn't declare excludeEnv yet. It's real, but only wired up to the native report // generator from Node v22.13.0 — CI pins Node 20.19.4, where setting it is a no-op. Kept anyway: // on versions that support it, it also redacts reports Node generates on its own via @@ -754,6 +919,10 @@ function wrapReportFn unknown>( // writeReport() call is redacted on every supported Node version, not just where excludeEnv is // wired up. writeReport() lets Node handle filename generation/defaults as normal, then // post-processes the file it actually wrote rather than reimplementing its naming convention. +// Gated on sharedState.isInsideScope(), not a global scope count — forceResetEnv() clears the +// count for an abandoned execution whose fn() is still running, and keying redaction on that count +// would let the zombie's own getReport()/writeReport() call see the real environment the moment the +// count resets, even though its own continuation never actually closed. const originalGetReport = process.report.getReport.bind(process.report); process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) => { const report = original(...args); @@ -763,14 +932,44 @@ process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) = return report; }); +// writeReport(fileName) can target a non-regular destination — a FIFO, socket, or character device +// like /dev/stdout — and Node writes the real, unredacted report straight there before this wrap +// can read it back and strip environmentVariables. Redacting after the fact can't undo bytes +// already delivered to whatever's reading the other end, so refusing the call once the destination +// is verified non-regular is the only option that can't leak. +const WRITE_REPORT_NON_REGULAR_SINK_MESSAGE = + "process.report.writeReport() to a non-regular destination (a pipe, socket, or similar) is not allowed in backend functions — Node would write its real, unscoped report there before this file's own redaction could ever run. Use $.Source or a declared Custom Credential instead."; + +// A not-yet-existing path is fine — writeReport creates a fresh, ordinary regular file there — +// so ENOENT is the one failure treated as "not a non-regular sink," matching isEnvironPath's own +// identical ENOENT fallback elsewhere in this file. fs.statSync follows symlinks on its own, +// unlike lstatSync, so a symlink pointing at a FIFO is resolved to its real target automatically. +function isVerifiedNonRegularDestination(filePath: string): boolean { + try { + return !fs.statSync(filePath).isFile(); + } catch (error) { + if (isErrnoException(error) && error.code === 'ENOENT') { + return false; + } + throw error; + } +} + const originalWriteReport = process.report.writeReport.bind(process.report); process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...args) => { if (sharedState.isInsideScope()) { // writeReport(fileName?, err?) also accepts writeReport(err?) with no fileName at all — // only a string first argument is ever a caller-chosen destination, so this branch is // skipped (falling through to Node's own write below) when none was given. + // Checked as the last statement before the real write, not earlier in this function: an + // external process could swap a symlink at fileNameArg between this check and the write + // below. This doesn't close that window entirely (the write is still a separate syscall + // right after), but narrows it to two back-to-back synchronous calls with nothing between. const fileNameArg = args[0]; if (typeof fileNameArg === 'string') { + if (isVerifiedNonRegularDestination(fileNameArg)) { + throw new Error(WRITE_REPORT_NON_REGULAR_SINK_MESSAGE); + } // Builds the redacted report itself and writes it directly, rather than letting Node // persist the real report first and rewriting it after — that would leave unredacted // content on disk if anything between the two writes throws. Cast: TS collapses the diff --git a/packages/plugins/apps/src/vite/guarded-wrapper.ts b/packages/plugins/apps/src/vite/guarded-wrapper.ts index 7c6906c58..7e9f06123 100644 --- a/packages/plugins/apps/src/vite/guarded-wrapper.ts +++ b/packages/plugins/apps/src/vite/guarded-wrapper.ts @@ -37,8 +37,10 @@ export function makeGuardWrapper unknown>( } // The last argument is a function in every real call this wraps (fs.readFile/open/copyFile/cp all -// require their callback), so no other heuristic is needed to find it. -function invokeCallbackArg(args: unknown[], error: Error): void { +// require their callback), so no other heuristic is needed to find it. Exported for env-guard.ts's +// own hand-rolled cp wrappers, which need this same callback-reporting behavior for a guard failure +// that isn't just a fixed shouldBlock() result (see their own comment). +export function invokeCallbackArg(args: unknown[], error: Error): void { const maybeCallback = args[args.length - 1]; if (typeof maybeCallback === 'function') { // Deferred, not called synchronously: every real error-first-callback fs function reports From 075e291075dffa443bf17012e6c52fefe9a0e613 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 11 Sep 2026 13:54:08 -0400 Subject: [PATCH 2/2] fix(apps): fix cp/fd option double-read regression, dedupe snapshot logic guardCpOptions and guardEnvironPathOrFdOption shared their "snapshot a getter-backed option once, then conditionally rebuild" logic through a new snapshotOptionKeysOnce helper. cpSync and promises.cp now assign guardCpOptions' result to a local before forwarding it, matching the convention already used by the plain cp wrapper. --- packages/plugins/apps/src/vite/env-guard.ts | 82 ++++++++++++--------- 1 file changed, 47 insertions(+), 35 deletions(-) diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts index 199f77203..ec2233dd3 100644 --- a/packages/plugins/apps/src/vite/env-guard.ts +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -493,6 +493,35 @@ function extractFdNumber(fdValue: unknown): unknown { return fdValue; } +// Reads each of `keys` from a getter-backed options object exactly once, then rebuilds a plain +// object where a key absent from the caller's own options stays absent — see guardCpOptions's own +// comment for why re-adding an absent key as explicit `undefined` breaks Node's own cp validation. +// Shared by guardEnvironPathOrFdOption (a single always-present key) and guardCpOptions (two +// independently-optional keys): presence-conditional restore is correct for both, since a +// guaranteed-present key just never hits the "absent" branch. +function snapshotOptionKeysOnce( + options: Record, + keys: readonly K[], +): Record { + // Copying every OTHER own-enumerable key first, via Object.keys rather than a `{ ...options }` + // spread, is what keeps this to exactly one read per snapshotted key below — a spread of the + // full options object would invoke every key's getter once on its own, then a second time when + // that same key is explicitly snapshotted next. + const keySet = new Set(keys); + const result: Record = {}; + for (const key of Object.keys(options)) { + if (!keySet.has(key)) { + result[key] = options[key]; + } + } + for (const key of keys) { + if (key in options) { + result[key] = options[key]; + } + } + return result; +} + // createReadStream/ReadStream's options.fd (a raw fd number, or a FileHandle whose own .fd is one) // makes Node read from that fd directly, ignoring the leading path argument — a plain // throwIfBlockedEnvironPath(rawPath) would never see the real target. Returns a safe options @@ -510,12 +539,9 @@ function guardEnvironPathOrFdOption(rawPath: unknown, options: unknown): unknown if (typeof options !== 'object' || options === null || !('fd' in options)) { return options; } - // Destructuring reads the getter exactly once, into fdValue — spreading the remainder (with fd - // already removed) can't invoke it again the way `{ ...options, fd: fdValue }` would have. - const { fd: fdValue, ...restOptions } = options as { fd: unknown }; - const fdNumber = extractFdNumber(fdValue); - throwIfBlockedEnvironPath(fdNumber); - return { ...restOptions, fd: fdValue }; + const snapshot = snapshotOptionKeysOnce(options as Record, ['fd'] as const); + throwIfBlockedEnvironPath(extractFdNumber(snapshot.fd)); + return snapshot; } // Every guarded fs entry point below except createReadStream takes only a leading path argument — @@ -778,27 +804,19 @@ function guardCpOptions(options: unknown): unknown { if (typeof options !== 'object' || options === null) { return options; } - const { - recursive: recursiveValue, - dereference: dereferenceValue, - ...restOptions - } = options as { recursive?: unknown; dereference?: unknown }; - if (recursiveValue === true && dereferenceValue === true) { + // Node's cp implementations distinguish an absent key (defaulted internally) from one + // explicitly present with value `undefined` (rejected by its own validation), so + // snapshotOptionKeysOnce restoring both keys unconditionally would turn a caller's + // `{ recursive: true }` (no dereference key at all) into `{ recursive: true, dereference: + // undefined }` and throw a validation error that never happens when the real options object is + // forwarded as-is — this is why it only re-adds keys actually present on the caller's options. + const safeOptions = snapshotOptionKeysOnce( + options as Record, + ['recursive', 'dereference'] as const, + ); + if (safeOptions.recursive === true && safeOptions.dereference === true) { throw new Error(CP_BLOCKED_MESSAGE); } - // Only re-adds a key that was actually present on the caller's own options — Node's cp - // implementations distinguish an absent key (defaulted internally) from one explicitly present - // with value `undefined` (rejected by its own validation), so restoring both keys unconditionally - // would turn a caller's `{ recursive: true }` (no dereference key at all) into - // `{ recursive: true, dereference: undefined }` and throw a validation error that never happens - // when the real options object is forwarded as-is. - const safeOptions = restOptions as Record; - if ('recursive' in options) { - safeOptions.recursive = recursiveValue; - } - if ('dereference' in options) { - safeOptions.dereference = dereferenceValue; - } return safeOptions; } @@ -813,11 +831,8 @@ const originalPromisesCp = fs.promises.cp; // implementations don't consult it, but nothing here should be the one silent exception. fs.cpSync = function (this: unknown, src: unknown, dest: unknown, options?: unknown) { throwIfBlockedEnvironPath(src); - return originalCpSync.apply(this, [ - src as string | URL, - dest as string | URL, - guardCpOptions(options) as fs.CopySyncOptions, - ]); + const safeOptions = guardCpOptions(options) as fs.CopySyncOptions; + return originalCpSync.apply(this, [src as string | URL, dest as string | URL, safeOptions]); } as typeof fs.cpSync; // cp reports failure via an error-first callback, never a synchronous throw. Only the guard's own @@ -844,11 +859,8 @@ fs.cp = function (this: unknown, src: unknown, dest: unknown, ...rest: unknown[] // promises.cp rejects, matching its real Promise-returning contract. fs.promises.cp = async function (this: unknown, src: unknown, dest: unknown, options?: unknown) { throwIfBlockedEnvironPath(src); - return originalPromisesCp.apply(this, [ - src as string | URL, - dest as string | URL, - guardCpOptions(options) as fs.CopyOptions, - ]); + const safeOptions = guardCpOptions(options) as fs.CopyOptions; + return originalPromisesCp.apply(this, [src as string | URL, dest as string | URL, safeOptions]); } as typeof fs.promises.cp; // createReadStream's own wrap above only covers that factory function — Node also exports the