Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide mechanism for autopopulating node.js process.env #3311

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 54 additions & 57 deletions src/node/internal/process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,63 +26,60 @@ export function nextTick(cb: Function, ...args: unknown[]) {
// for the worker are accessible from the env argument passed into the fetch
// handler and have no impact here.

export const env = new Proxy(
{},
{
// Per Node.js rules. process.env values must be coerced to strings.
// When defined using defineProperty, the property descriptor must be writable,
// configurable, and enumerable using just a falsy check. Getters and setters
// are not permitted.
set(obj: object, prop: PropertyKey, value: any) {
return Reflect.set(obj, prop, `${value}`);
},
defineProperty(
obj: object,
prop: PropertyKey,
descriptor: PropertyDescriptor
) {
validateObject(descriptor, 'descriptor', {});
if (Reflect.has(descriptor, 'get') || Reflect.has(descriptor, 'set')) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor',
descriptor,
'process.env value must not have getter/setter'
);
}
if (!descriptor.configurable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.configurable',
descriptor,
'process.env value must be configurable'
);
}
if (!descriptor.enumerable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.enumerable',
descriptor,
'process.env value must be enumerable'
);
}
if (!descriptor.writable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.writable',
descriptor,
'process.env value must be writable'
);
}
if (Reflect.has(descriptor, 'value')) {
Reflect.set(descriptor, 'value', `${descriptor.value}`);
} else {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.value',
descriptor,
'process.env value must be specified explicitly'
);
}
return Reflect.defineProperty(obj, prop, descriptor);
},
}
);
export const env = new Proxy(utilImpl.getEnvObject(), {
// Per Node.js rules. process.env values must be coerced to strings.
// When defined using defineProperty, the property descriptor must be writable,
// configurable, and enumerable using just a falsy check. Getters and setters
// are not permitted.
set(obj: object, prop: PropertyKey, value: any) {
return Reflect.set(obj, prop, `${value}`);
},
defineProperty(
obj: object,
prop: PropertyKey,
descriptor: PropertyDescriptor
) {
validateObject(descriptor, 'descriptor', {});
if (Reflect.has(descriptor, 'get') || Reflect.has(descriptor, 'set')) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor',
descriptor,
'process.env value must not have getter/setter'
);
}
if (!descriptor.configurable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.configurable',
descriptor,
'process.env value must be configurable'
);
}
if (!descriptor.enumerable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.enumerable',
descriptor,
'process.env value must be enumerable'
);
}
if (!descriptor.writable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.writable',
descriptor,
'process.env value must be writable'
);
}
if (Reflect.has(descriptor, 'value')) {
Reflect.set(descriptor, 'value', `${descriptor.value}`);
} else {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.value',
descriptor,
'process.env value must be specified explicitly'
);
}
return Reflect.defineProperty(obj, prop, descriptor);
},
});

export function getBuiltinModule(id: string): any {
return utilImpl.getBuiltinModule(id);
Expand Down
1 change: 1 addition & 0 deletions src/node/internal/util.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export function isBoxedPrimitive(
value: unknown
): value is number | string | boolean | bigint | symbol;

export function getEnvObject(): Record<string, string>;
export function getBuiltinModule(id: string): any;
export function getCallSite(frames: number): Record<string, string>[];
export function processExitImpl(code: number): void;
Expand Down
53 changes: 53 additions & 0 deletions src/workerd/api/node/tests/process-nodejs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,56 @@ export const processPlatform = {
assert.ok(['darwin', 'win32', 'linux'].includes(process.platform));
},
};

process.env.QUX = 1;
const pEnv = { ...process.env };

export const processEnv = {
async test(ctrl, env) {
assert.strictEqual(pEnv.FOO, 'BAR');

// It should be possible to mutate the process.env at runtime.
// All values manually set are coerced to strings.
assert.strictEqual(pEnv.QUX, '1');

// JSON bindings that do not parse to strings come through as
// raw unparsed JSON strings....
assert.strictEqual(pEnv.BAR, '{}');
JSON.parse(pEnv.BAR);

// JSON bindings that parse as strings come through as the
// parsed string value.
assert.strictEqual(pEnv.BAZ, 'abc');

// Throws because althought defined as a JSON binding, the value
// that comes through to process.env in this case is not a JSON
// parseable string because it will already have been parsed.
// This assertion is only true when the JSON bindings value
// happens to parse as a string.
assert.throws(() => JSON.parse(pEnv.BAZ));

// JSON bindings that parse as strings that happen to be double
// escaped might be JSON parseable.
assert.strictEqual(JSON.parse(pEnv.DUB), 'abc');

// Test that imports can see the process.env at the top level
const { FOO } = await import('mod');
jasnell marked this conversation as resolved.
Show resolved Hide resolved
jasnell marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(FOO, 'BAR');

// Mutating the env argument does not change process.env, and
// vis versa
assert.strictEqual(env.QUX, undefined);
env.ZZZ = 'a';
assert.strictEqual(process.env.ZZZ, undefined);

env.FOO = ['just some other value'];
assert.strictEqual(process.env.FOO, 'BAR');

// Other kinds of bindings will be on env but not process.env
assert.strictEqual(pEnv.NON, undefined);
assert.deepStrictEqual(
new Uint8Array(env.NON),
new Uint8Array([97, 98, 99, 100, 101, 102])
);
},
};
17 changes: 14 additions & 3 deletions src/workerd/api/node/tests/process-nodejs-test.wd-test
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,21 @@ const unitTests :Workerd.Config = (
( name = "nodejs-process-test",
worker = (
modules = [
(name = "worker", esModule = embed "process-nodejs-test.js")
(name = "worker", esModule = embed "process-nodejs-test.js"),
(name = "mod", esModule = "export const { FOO } = process.env;")
],
compatibilityDate = "2024-12-28",
compatibilityFlags = [
"nodejs_compat",
"nodejs_compat_populate_process_env"
],
bindings = [
(name = "FOO", text = "BAR"),
(name = "BAR", json = "{}"),
(name = "BAZ", json = "\"abc\""),
(name = "DUB", json = "\"\\\"abc\\\"\""),
(name = "NON", data = "abcdef")
jasnell marked this conversation as resolved.
Show resolved Hide resolved
],
compatibilityDate = "2024-10-11",
compatibilityFlags = ["nodejs_compat"],
)
),
],
Expand Down
4 changes: 4 additions & 0 deletions src/workerd/api/node/util.c++
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ jsg::JsValue UtilModule::getBuiltinModule(jsg::Lock& js, kj::String specifier) {
return js.undefined();
}

jsg::JsObject UtilModule::getEnvObject(jsg::Lock& js) {
return js.getEnv(true);
jasnell marked this conversation as resolved.
Show resolved Hide resolved
}

namespace {
[[noreturn]] void handleProcessExit(jsg::Lock& js, int code) {
// There are a few things happening here. First, we abort the current IoContext
Expand Down
5 changes: 5 additions & 0 deletions src/workerd/api/node/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ class UtilModule final: public jsg::Object {
return processPlatform;
}

jsg::JsObject getEnvObject(jsg::Lock& js);

JSG_RESOURCE_TYPE(UtilModule) {
JSG_NESTED_TYPE(MIMEType);
JSG_NESTED_TYPE(MIMEParams);
Expand All @@ -258,6 +260,9 @@ class UtilModule final: public jsg::Object {
JSG_METHOD(previewEntries);
JSG_METHOD(getConstructorName);
JSG_METHOD(getCallSite);
// TODO(cleanup): It might be about time to separate some of these out
// to a different module.
JSG_METHOD(getEnvObject);
jasnell marked this conversation as resolved.
Show resolved Hide resolved

#define V(Type) JSG_METHOD(is##Type);
JS_UTIL_IS_TYPES(V)
Expand Down
6 changes: 6 additions & 0 deletions src/workerd/io/compatibility-date.capnp
Original file line number Diff line number Diff line change
Expand Up @@ -679,4 +679,10 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef {
$compatDisableFlag("cache_no_cache_disabled")
$experimental;
# Enables the use of cache: no-cache in the fetch api.

populateProcessEnv @71 :Bool
$compatEnableFlag("nodejs_compat_populate_process_env")
$compatDisableFlag("nodejs_compat_do_not_populate_process_env");
# Automatically populate process.env from text bindings only
# when nodejs_compat is being used.
}
7 changes: 7 additions & 0 deletions src/workerd/jsg/jsg.h
Original file line number Diff line number Diff line change
Expand Up @@ -2675,6 +2675,13 @@ class Lock {
// the inspector (if attached), or to KJ_LOG(Info).
virtual void reportError(const JsValue& value) = 0;

// Sets an env value that will be expressed on the process.env
// if/when nodejs-compat mode is used.
virtual void setEnvField(const JsValue& name, const JsValue& value) = 0;

// Returns the env base object.
virtual JsObject getEnv(bool release = false) = 0;

private:
// Mark the jsg::Lock as being disallowed from being passed as a parameter into
// a kj promise coroutine. Note that this only blocks directly passing the Lock
Expand Down
1 change: 1 addition & 0 deletions src/workerd/jsg/setup.c++
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ void IsolateBase::dropWrappers(kj::FunctionParam<void()> drop) {
// Make sure v8::Globals are destroyed under lock (but not until later).
KJ_DEFER(symbolAsyncDispose.Reset());
KJ_DEFER(opaqueTemplate.Reset());
KJ_DEFER(envObj.Reset());

// Make sure the TypeWrapper is destroyed under lock by declaring a new copy of the variable
// that is destroyed before the lock is released.
Expand Down
21 changes: 21 additions & 0 deletions src/workerd/jsg/setup.h
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ class IsolateBase {
// object with 2 internal fields.
v8::Global<v8::FunctionTemplate> opaqueTemplate;

// Object that is used as the underlying target of process.env when nodejs-compat mode is used.
v8::Global<v8::Object> envObj;

// Polyfilled Symbol.asyncDispose.
v8::Global<v8::Symbol> symbolAsyncDispose;

Expand Down Expand Up @@ -665,6 +668,24 @@ class Isolate: public IsolateBase {
}
}

// Sets an env value that will be expressed on the process.env
jasnell marked this conversation as resolved.
Show resolved Hide resolved
// if/when nodejs-compat mode is used.
void setEnvField(const JsValue& name, const JsValue& value) override {
getEnv().set(*this, name, value);
}

// Returns the env base object.
JsObject getEnv(bool release = false) override {
KJ_DEFER({
if (release) jsgIsolate.envObj.Reset();
});
if (jsgIsolate.envObj.IsEmpty()) {
v8::Local<v8::Object> env = obj();
jsgIsolate.envObj.Reset(v8Isolate, env);
}
return JsObject(jsgIsolate.envObj.Get(v8Isolate));
}

private:
Isolate& jsgIsolate;

Expand Down
67 changes: 65 additions & 2 deletions src/workerd/server/workerd-api.c++
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,68 @@ static v8::Local<v8::Value> createBindingValue(JsgWorkerdIsolate::Lock& lock,

KJ_SWITCH_ONEOF(global.value) {
KJ_CASE_ONEOF(json, Global::Json) {
v8::Local<v8::String> string = lock.wrap(context, kj::mv(json.text));
value = jsg::check(v8::JSON::Parse(context, string));
value = jsg::check(v8::JSON::Parse(context, lock.str(json.text)));
if (featureFlags.getPopulateProcessEnv() && featureFlags.getNodeJsCompat()) {
// Generally speaking, process.env has the TEXT and JSON bindings from env.
// TEXT bindings are simple enough and env.{name} will strictly equal
// process.env.{name}. With JSON bindings it a bit trickier due to architectural
// quirks and history in our runtime.
// If you set the JSON environment variable to a value that parses to a string
// then it will be exposed on process.env and env as that parsed string such
// that env.{name} will strictly equal process.env{name} like with TEXT bindings.
// If, however, the value parses to any type other than a string, the
// process.env.{name} will instead be the raw parseable JSON string and will
// *not* strictly equal env.{name}.
//
// So, for example, given the bindings:
//
// (name = "FOO", json = "{}"),
// (name = "BAR", json = "\"abc\""),
// (name = "BAZ", json = "\"\\\"abc\\\"\"")
//
// In the worker:
//
// env.FOO === process.env.FOO; // false
// console.log(typeof env.FOO); // 'object'
// console.log(typeof process.env.FOO); // 'string'
// console.log(env.FOO); // [object Object]
// console.log(process.env.FOO); // '{}'
// console.log(typeof JSON.parse(process.env.FOO)); // 'object'
//
// env.BAR === process.env.BAR; // true
// console.log(typeof env.BAR); // 'string'
// console.log(typeof process.env.BAR); // 'string'
// console.log(env.BAR); // 'abc'
// console.log(process.env.BAR); // 'abc'
// console.log(typeof JSON.parse(process.env.BAR)); // throws!!
//
// env.BAZ === process.env.BAZ; // true
// console.log(typeof enf.BAZ); // 'string'
// console.log(typeof process.env.BAZ); // 'string'
// console.log(env.BAZ); // '"abc"'
// console.log(process.env.BAZ); // '"abc"
// console.log(typeof JSON.parse(process.env.BAZ)); // 'string'
//
// JSON.parse(process.env.FOO) works because the value is a parseable
// JSON string. JSON.parse(process.env.BAR) throws an error because
// the value is not a parseable JSON string, even tho the binding uses
// type JSON. JSON.parse(process.env.BAZ) works because the original
// JSON-encoded value was double-escaped and the result of the above
// v8::JSON::Parse is itself a parseable JSON string.
//
// Practically speaking this means that developers can never really
// count on environment variables accessed via `env` always being
// strictly equal to the same environment variable accessed via
// process.env because, despite being defined as JSON bindings,
// the resulting value may or may not be JSON parseable or may be
// parsed in one context (env) and unparsed in another (process.env).

if (value->IsString()) {
lock.setEnvField(lock.str(global.name), jsg::JsValue(value));
} else {
lock.setEnvField(lock.str(global.name), lock.str(json.text));
}
}
}

KJ_CASE_ONEOF(pipeline, Global::Fetcher) {
Expand Down Expand Up @@ -712,6 +772,9 @@ static v8::Local<v8::Value> createBindingValue(JsgWorkerdIsolate::Lock& lock,

KJ_CASE_ONEOF(text, kj::String) {
value = lock.wrap(context, kj::mv(text));
if (featureFlags.getPopulateProcessEnv() && featureFlags.getNodeJsCompat()) {
lock.setEnvField(lock.str(global.name), jsg::JsValue(value));
jasnell marked this conversation as resolved.
Show resolved Hide resolved
}
}

KJ_CASE_ONEOF(data, kj::Array<byte>) {
Expand Down
Loading