Skip to content

Commit 5fc656a

Browse files
authored
Merge pull request #3778 from cloudflare/kenton/process-env-cleanup
Change process.env implementation to be based on importable env.
2 parents cf02917 + dd9382d commit 5fc656a

File tree

8 files changed

+115
-113
lines changed

8 files changed

+115
-113
lines changed

src/node/internal/process.ts

Lines changed: 98 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,105 @@ export function nextTick(cb: Function, ...args: unknown[]) {
2020
});
2121
}
2222

23-
// Note that there is no process-level environment in workers so the process.env
24-
// object is initially empty. This is different from Node.js where process.env
25-
// picks up values from the operating system environment. The configured bindings
26-
// for the worker are accessible from the env argument passed into the fetch
27-
// handler and have no impact here.
23+
// Decide if a value can round-trip to JSON without losing any information.
24+
function isJsonSerializable(
25+
value: any,
26+
seen: Set<Object> = new Set()
27+
): boolean {
28+
switch (typeof value) {
29+
case 'boolean':
30+
case 'number':
31+
case 'string':
32+
return true;
2833

29-
export const env = new Proxy(utilImpl.getEnvObject(), {
34+
case 'object': {
35+
if (value === null) {
36+
return true;
37+
}
38+
39+
if (seen.has(value)) {
40+
// Don't allow cycles or aliases. (Non-cyclic aliases technically could be OK, but a
41+
// round trip to JSON would lose the fact that they are aliases.)
42+
return false;
43+
}
44+
seen.add(value);
45+
46+
if (typeof value.toJSON === 'function') {
47+
// This type is explicitly designed to be JSON-serialized so we'll accept it.
48+
return true;
49+
}
50+
51+
// We only consider objects to be serializable if they are plain objects or plain arrays.
52+
// Technically, JSON can serialize any subclass of Object (as well as objects with null
53+
// prototypes), but the round trip would lose information about the original type. Hence,
54+
// we assume that any env var containing such things is not intended to be appear as JSON
55+
// in process.env. For example, we wouldn't want a KV namespace to show up in process.env as
56+
// "{}" -- this would be weird.
57+
switch (Object.getPrototypeOf(value)) {
58+
case Object.prototype:
59+
// Note that Object.values() only returns string-keyed values, not symbol-keyed.
60+
return Object.values(value).every((prop) =>
61+
isJsonSerializable(prop, seen)
62+
);
63+
case Array.prototype:
64+
return (<Array<unknown>>value).every((elem) =>
65+
isJsonSerializable(elem, seen)
66+
);
67+
default:
68+
return false;
69+
}
70+
}
71+
72+
default:
73+
return false;
74+
}
75+
}
76+
77+
function getInitialEnv() {
78+
let env: Record<string, string> = {};
79+
for (let [key, value] of Object.entries(utilImpl.getEnvObject())) {
80+
// Workers environment variables can have a variety of types, but process.env vars are
81+
// strictly strings. We want to convert our workers env into process.env, but allowing
82+
// process.env to contain non-strings would probably break Node apps.
83+
//
84+
// As a compromise, we say:
85+
// - Workers env vars that are plain strings are unchanged in process.env.
86+
// - Workers env vars that can be represented as JSON will be JSON-stringified in process.env.
87+
// - Anything else will be omitted.
88+
//
89+
// Note that you might argue that, at the config layer, it's possible to differentiate between
90+
// plain strings and JSON values that evaluated to strings. Wouldn't it be nice if we could
91+
// check which way the binding was originally configured in order to decide whether to
92+
// represent it plain or as JSON here. However, there is no way to tell just by looking at
93+
// the `env` object inside a Worker whether a particular var was originally configured as
94+
// plain text, or as JSON that evaluated to a string. Either way, you just get a string. And
95+
// indeed, the Workers Runtime itself does not necessarily know this. In many cases it does
96+
// know, but in general the abstraction the Runtime intends to provide is that `env` is just
97+
// a JavaScript object, and how exactly the contents were originally represented is not
98+
// intended to be conveyed. This is important because, for example, we could extend dynamic
99+
// dispatch bindings in the future such that the caller can specify `env` directly, and in
100+
// that case the caller would simply specify a JS object, without JSON or any other
101+
// serialization involved. In this case, there would be no way to know if a string var was
102+
// "supposed to be" raw text vs. JSON.
103+
//
104+
// So, we have to do the best we can given just what we know -- the JavaScript object that is
105+
// `env`.
106+
//
107+
// As a consolation, this is consistent with how variables are defined in wrangler.toml: you
108+
// do not explicitly specify whether a variable is text or JSON. If you define a variable with
109+
// a simple string value, it gets configured as a text var. If you specify an object, then it's
110+
// configured as JSON.
111+
112+
if (typeof value === 'string') {
113+
env[key] = value;
114+
} else if (isJsonSerializable(value)) {
115+
env[key] = JSON.stringify(value);
116+
}
117+
}
118+
return env;
119+
}
120+
121+
export const env = new Proxy(getInitialEnv(), {
30122
// Per Node.js rules. process.env values must be coerced to strings.
31123
// When defined using defineProperty, the property descriptor must be writable,
32124
// configurable, and enumerable using just a falsy check. Getters and setters

src/workerd/api/modules.c++

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,8 @@ kj::Maybe<jsg::JsObject> EnvModule::getCurrent(jsg::Lock& js) {
2020
if (FeatureFlags::get(js).getDisableImportableEnv()) return kj::none;
2121

2222
// Otherwise, fallback to provide the stored environment.
23-
return js.getWorkerEnv().map([&](const jsg::Value& val) -> jsg::JsObject {
24-
auto handle = val.getHandle(js);
25-
JSG_REQUIRE(handle->IsObject(), TypeError, "Expected environment to be an object.");
26-
return jsg::JsObject(handle.As<v8::Object>());
23+
return js.getWorkerEnv().map([&](const jsg::V8Ref<v8::Object>& val) -> jsg::JsObject {
24+
return jsg::JsObject(val.getHandle(js));
2725
});
2826
}
2927

src/workerd/api/node/util.c++

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,14 @@ jsg::JsValue UtilModule::getBuiltinModule(jsg::Lock& js, kj::String specifier) {
250250
}
251251

252252
jsg::JsObject UtilModule::getEnvObject(jsg::Lock& js) {
253-
return js.getProcessEnv(true);
253+
if (FeatureFlags::get(js).getPopulateProcessEnv()) {
254+
KJ_IF_SOME(env, js.getWorkerEnv()) {
255+
return jsg::JsObject(env.getHandle(js));
256+
}
257+
}
258+
259+
// Default to empty object.
260+
return js.obj();
254261
}
255262

256263
namespace {

src/workerd/io/worker.c++

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1618,7 +1618,7 @@ Worker::Worker(kj::Own<const Script> scriptParam,
16181618
// Use `env` variable.
16191619
bindingsScope = v8::Object::New(lock.v8Isolate);
16201620
if (!FeatureFlags::get(js).getDisableImportableEnv()) {
1621-
lock.setWorkerEnv(lock.v8Ref(bindingsScope.As<v8::Value>()));
1621+
lock.setWorkerEnv(lock.v8Ref(bindingsScope));
16221622
}
16231623
} else {
16241624
// Use global-scope bindings.

src/workerd/jsg/jsg.h

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2705,18 +2705,11 @@ class Lock {
27052705
// the inspector (if attached), or to KJ_LOG(Info).
27062706
virtual void reportError(const JsValue& value) = 0;
27072707

2708-
// Sets an env value that will be expressed on the process.env
2709-
// if/when nodejs-compat mode is used.
2710-
virtual void setProcessEnvField(const JsValue& name, const JsValue& value) = 0;
2711-
2712-
// Returns the process.env base object.
2713-
virtual JsObject getProcessEnv(bool release = false) = 0;
2714-
27152708
// Store the worker environment.
2716-
virtual void setWorkerEnv(Value value) = 0;
2709+
virtual void setWorkerEnv(V8Ref<v8::Object> value) = 0;
27172710

27182711
// Retrieve the worker environment.
2719-
virtual kj::Maybe<Value> getWorkerEnv() = 0;
2712+
virtual kj::Maybe<V8Ref<v8::Object>> getWorkerEnv() = 0;
27202713

27212714
// Resolve an internal module namespace from the given specifier.
27222715
// This variation can be used only for internal built-ins.

src/workerd/jsg/setup.c++

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,6 @@ void IsolateBase::dropWrappers(kj::FunctionParam<void()> drop) {
428428
// Make sure v8::Globals are destroyed under lock (but not until later).
429429
KJ_DEFER(symbolAsyncDispose.Reset());
430430
KJ_DEFER(opaqueTemplate.Reset());
431-
KJ_DEFER(envObj.Reset());
432431
KJ_DEFER(workerEnvObj.Reset());
433432

434433
// Make sure the TypeWrapper is destroyed under lock by declaring a new copy of the variable

src/workerd/jsg/setup.h

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -284,9 +284,6 @@ class IsolateBase {
284284
// object with 2 internal fields.
285285
v8::Global<v8::FunctionTemplate> opaqueTemplate;
286286

287-
// Object that is used as the underlying target of process.env when nodejs-compat mode is used.
288-
v8::Global<v8::Object> envObj;
289-
290287
// Object used as the underlying storage for a workers environment.
291288
v8::Global<v8::Object> workerEnvObj;
292289

@@ -700,33 +697,13 @@ class Isolate: public IsolateBase {
700697
}
701698
}
702699

703-
// Sets an env value that will be expressed on the process.env
704-
// if/when nodejs-compat mode is used.
705-
void setProcessEnvField(const JsValue& name, const JsValue& value) override {
706-
getProcessEnv().set(*this, name, value);
707-
}
708-
709-
// Returns the env base object.
710-
JsObject getProcessEnv(bool release = false) override {
711-
KJ_DEFER({
712-
if (release) jsgIsolate.envObj.Reset();
713-
});
714-
if (jsgIsolate.envObj.IsEmpty()) {
715-
v8::Local<v8::Object> env = obj();
716-
jsgIsolate.envObj.Reset(v8Isolate, env);
717-
}
718-
return JsObject(jsgIsolate.envObj.Get(v8Isolate));
719-
}
720-
721-
void setWorkerEnv(Value value) override {
722-
auto handle = value.getHandle(*this);
723-
KJ_ASSERT(handle->IsObject());
724-
jsgIsolate.workerEnvObj.Reset(v8Isolate, handle.template As<v8::Object>());
700+
void setWorkerEnv(V8Ref<v8::Object> value) override {
701+
jsgIsolate.workerEnvObj.Reset(v8Isolate, value.getHandle(*this));
725702
}
726703

727-
kj::Maybe<Value> getWorkerEnv() override {
704+
kj::Maybe<V8Ref<v8::Object>> getWorkerEnv() override {
728705
if (jsgIsolate.workerEnvObj.IsEmpty()) return kj::none;
729-
return v8Ref<v8::Value>(jsgIsolate.workerEnvObj.Get(v8Isolate));
706+
return v8Ref<v8::Object>(jsgIsolate.workerEnvObj.Get(v8Isolate));
730707
}
731708

732709
private:

src/workerd/server/workerd-api.c++

Lines changed: 0 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -654,67 +654,6 @@ static v8::Local<v8::Value> createBindingValue(JsgWorkerdIsolate::Lock& lock,
654654
KJ_SWITCH_ONEOF(global.value) {
655655
KJ_CASE_ONEOF(json, Global::Json) {
656656
value = jsg::check(v8::JSON::Parse(context, lock.str(json.text)));
657-
if (featureFlags.getPopulateProcessEnv() && featureFlags.getNodeJsCompat()) {
658-
// Generally speaking, process.env has the TEXT and JSON bindings from env.
659-
// TEXT bindings are simple enough and env.{name} will strictly equal
660-
// process.env.{name}. With JSON bindings it a bit trickier due to architectural
661-
// quirks and history in our runtime.
662-
// If you set the JSON environment variable to a value that parses to a string
663-
// then it will be exposed on process.env and env as that parsed string such
664-
// that env.{name} will strictly equal process.env{name} like with TEXT bindings.
665-
// If, however, the value parses to any type other than a string, the
666-
// process.env.{name} will instead be the raw parseable JSON string and will
667-
// *not* strictly equal env.{name}.
668-
//
669-
// So, for example, given the bindings:
670-
//
671-
// (name = "FOO", json = "{}"),
672-
// (name = "BAR", json = "\"abc\""),
673-
// (name = "BAZ", json = "\"\\\"abc\\\"\"")
674-
//
675-
// In the worker:
676-
//
677-
// env.FOO === process.env.FOO; // false
678-
// console.log(typeof env.FOO); // 'object'
679-
// console.log(typeof process.env.FOO); // 'string'
680-
// console.log(env.FOO); // [object Object]
681-
// console.log(process.env.FOO); // '{}'
682-
// console.log(typeof JSON.parse(process.env.FOO)); // 'object'
683-
//
684-
// env.BAR === process.env.BAR; // true
685-
// console.log(typeof env.BAR); // 'string'
686-
// console.log(typeof process.env.BAR); // 'string'
687-
// console.log(env.BAR); // 'abc'
688-
// console.log(process.env.BAR); // 'abc'
689-
// console.log(typeof JSON.parse(process.env.BAR)); // throws!!
690-
//
691-
// env.BAZ === process.env.BAZ; // true
692-
// console.log(typeof enf.BAZ); // 'string'
693-
// console.log(typeof process.env.BAZ); // 'string'
694-
// console.log(env.BAZ); // '"abc"'
695-
// console.log(process.env.BAZ); // '"abc"
696-
// console.log(typeof JSON.parse(process.env.BAZ)); // 'string'
697-
//
698-
// JSON.parse(process.env.FOO) works because the value is a parseable
699-
// JSON string. JSON.parse(process.env.BAR) throws an error because
700-
// the value is not a parseable JSON string, even tho the binding uses
701-
// type JSON. JSON.parse(process.env.BAZ)` works because the original
702-
// JSON-encoded value was double-escaped and the result of the above
703-
// v8::JSON::Parse is itself a parseable JSON string.
704-
//
705-
// Practically speaking this means that developers can never really
706-
// count on environment variables accessed via `env` always being
707-
// strictly equal to the same environment variable accessed via
708-
// process.env because, despite being defined as JSON bindings,
709-
// the resulting value may or may not be JSON parseable or may be
710-
// parsed in one context (env) and unparsed in another (process.env).
711-
712-
if (value->IsString()) {
713-
lock.setProcessEnvField(lock.str(global.name), jsg::JsValue(value));
714-
} else {
715-
lock.setProcessEnvField(lock.str(global.name), lock.str(json.text));
716-
}
717-
}
718657
}
719658

720659
KJ_CASE_ONEOF(pipeline, Global::Fetcher) {
@@ -800,9 +739,6 @@ static v8::Local<v8::Value> createBindingValue(JsgWorkerdIsolate::Lock& lock,
800739

801740
KJ_CASE_ONEOF(text, kj::String) {
802741
value = lock.wrap(context, kj::mv(text));
803-
if (featureFlags.getPopulateProcessEnv() && featureFlags.getNodeJsCompat()) {
804-
lock.setProcessEnvField(lock.str(global.name), jsg::JsValue(value));
805-
}
806742
}
807743

808744
KJ_CASE_ONEOF(data, kj::Array<byte>) {

0 commit comments

Comments
 (0)