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

Implement importable env #3610

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 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
3 changes: 3 additions & 0 deletions src/cloudflare/internal/env.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// Get the current environment, if any
export function getCurrent(): object | undefined;
export function withEnv(newEnv: unknown, fn: () => unknown): unknown;
83 changes: 83 additions & 0 deletions src/cloudflare/workers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,92 @@
// wrapper module that simply re-exports the classes from the built-in module.

import entrypoints from 'cloudflare-internal:workers';
import innerEnv from 'cloudflare-internal:env';

export const WorkerEntrypoint = entrypoints.WorkerEntrypoint;
export const DurableObject = entrypoints.DurableObject;
export const RpcStub = entrypoints.RpcStub;
export const RpcTarget = entrypoints.RpcTarget;
export const WorkflowEntrypoint = entrypoints.WorkflowEntrypoint;

// We turn off these specific linting rules here intentionally as mixing
// typescript and proxies can just be... awkward. The use of any in these
// is intentional.
/* eslint-disable @typescript-eslint/no-unsafe-assignment */
/* eslint-disable @typescript-eslint/no-explicit-any */

export function withEnv(newEnv: unknown, fn: () => unknown): unknown {
return innerEnv.withEnv(newEnv, fn);
}

// A proxy for the workers env/bindings. The proxy is io-context
// aware in that it will only return values when there is an active
// IoContext. Mutations to the env via this proxy propagate to the
// underlying env and do not follow the async context.
export const env: object = new Proxy(
{},
{
get(_: any, prop: string | symbol): unknown {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.get(inner, prop);
}
return undefined;
},

set(_: any, prop: string | symbol, newValue: any): boolean {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.set(inner, prop, newValue);
}
return true;
},

has(_: any, prop: string | symbol): boolean {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.has(inner, prop);
}
return false;
},

ownKeys(_: any): any {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.ownKeys(inner);
}
return [];
},

deleteProperty(_: any, prop: string | symbol): boolean {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.deleteProperty(inner, prop);
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return false here ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right. yeah false is more correct here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... actually no. true is correct here. The reason is that this is effectively deleting a non-existent property. If, for instance, you have const n = {}; delete n.abc, that delete will return true because effectively the property is deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I was looking at a MDN snippet:

const docCookies = new Proxy(docCookies, {
// ...
  deleteProperty(target, key) {
    if (!(key in target)) {
      return false;
    }
    return target.removeItem(key);
  },

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some disagreement in the ecosystem about exactly what should be returned in which cases but I tend to err on duplicating what I see v8 doing which is returning true when deleting a non-existent property.

Copy link
Contributor

@vicb vicb Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like returning false might throw in strict mode, which we do not want.

Maybe a simpler impl for all method could be:

// before
      const inner = innerEnv.getCurrent();
      if (inner) {
        return Reflect.deleteProperty(inner, prop);
      }
      return true;
      
// after
return Reflect.deleteProperty(innerEnv.getCurrent() ?? {}, prop);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer the more verbose version as I think it's a bit clearer / easier to read. It will also be a bit faster as it avoids the extraneous property lookup on the empty object.

},

defineProperty(
_: any,
prop: string | symbol,
attr: PropertyDescriptor
): boolean {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.defineProperty(inner, prop, attr);
}
return true;
},

getOwnPropertyDescriptor(
_: any,
prop: string | symbol
): PropertyDescriptor | undefined {
const inner = innerEnv.getCurrent();
if (inner) {
return Reflect.getOwnPropertyDescriptor(inner, prop);
}
return undefined;
},
}
);
18 changes: 17 additions & 1 deletion src/workerd/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ filegroup(
"pyodide/pyodide.c++",
"pyodide/setup-emscripten.c++",
"memory-cache.c++",
"modules.c++",
"r2*.c++",
"rtti.c++",
"url.c++",
Expand Down Expand Up @@ -84,7 +85,10 @@ wd_cc_library(

wd_cc_library(
name = "rtti",
srcs = ["rtti.c++"],
srcs = [
"modules.c++",
"rtti.c++",
],
hdrs = [
"modules.h",
"rtti.h",
Expand Down Expand Up @@ -650,3 +654,15 @@ wd_test(
args = ["--experimental"],
data = ["tests/fetch-test.js"],
)

wd_test(
src = "tests/importable-env-test.wd-test",
args = ["--experimental"],
data = ["tests/importable-env-test.js"],
)

wd_test(
src = "tests/disable-importable-env-test.wd-test",
args = ["--experimental"],
data = ["tests/disable-importable-env-test.js"],
)
36 changes: 36 additions & 0 deletions src/workerd/api/modules.c++
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#include "modules.h"

#include <workerd/jsg/setup.h>

namespace workerd::api {

kj::Maybe<jsg::JsObject> EnvModule::getCurrent(jsg::Lock& js) {
auto& key = jsg::IsolateBase::from(js.v8Isolate).getEnvAsyncContextKey();
KJ_IF_SOME(frame, jsg::AsyncContextFrame::current(js)) {
KJ_IF_SOME(value, frame.get(key)) {
auto handle = value.getHandle(js);
if (handle->IsObject()) {
return jsg::JsObject(handle.As<v8::Object>());
}
}
}
// If the compat flag is set to disable importable env, then this
// will return nothing.
if (FeatureFlags::get(js).getDisableImportableEnv()) return kj::none;

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

jsg::JsValue EnvModule::withEnv(
jsg::Lock& js, jsg::Value newEnv, jsg::Function<jsg::JsValue()> fn) {
auto& key = jsg::IsolateBase::from(js.v8Isolate).getEnvAsyncContextKey();
jsg::AsyncContextFrame::StorageScope storage(js, key, kj::mv(newEnv));
return fn(js);
}

} // namespace workerd::api
17 changes: 17 additions & 0 deletions src/workerd/api/modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,26 @@
#include <workerd/api/sockets.h>
#include <workerd/api/unsafe.h>
#include <workerd/api/worker-rpc.h>
#include <workerd/io/features.h>
#include <workerd/io/worker.h>
#include <workerd/jsg/modules-new.h>

#include <cloudflare/cloudflare.capnp.h>

namespace workerd::api {

class EnvModule final: public jsg::Object {
public:
kj::Maybe<jsg::JsObject> getCurrent(jsg::Lock& js);

jsg::JsValue withEnv(jsg::Lock& js, jsg::Value newEnv, jsg::Function<jsg::JsValue()> fn);

JSG_RESOURCE_TYPE(EnvModule) {
JSG_METHOD(getCurrent);
JSG_METHOD(withEnv);
}
};

template <class Registry>
void registerModules(Registry& registry, auto featureFlags) {
node::registerNodeJsCompatModules(registry, featureFlags);
Expand All @@ -30,6 +43,8 @@ void registerModules(Registry& registry, auto featureFlags) {
registerSocketsModule(registry, featureFlags);
registry.addBuiltinBundle(CLOUDFLARE_BUNDLE);
registerRpcModules(registry, featureFlags);
registry.template addBuiltinModule<EnvModule>(
"cloudflare-internal:env", workerd::jsg::ModuleRegistry::Type::INTERNAL);
}

template <class TypeWrapper>
Expand Down Expand Up @@ -61,6 +76,8 @@ void registerBuiltinModules(jsg::modules::ModuleRegistry::Builder& builder, auto
jsg::modules::ModuleBundle::getBuiltInBundleFromCapnp(builtinsBuilder, CLOUDFLARE_BUNDLE);
builder.add(builtinsBuilder.finish());
}

// TODO(later): Add the internal env module also.
}

} // namespace workerd::api
2 changes: 1 addition & 1 deletion src/workerd/api/node/util.c++
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ jsg::JsValue UtilModule::getBuiltinModule(jsg::Lock& js, kj::String specifier) {
}

jsg::JsObject UtilModule::getEnvObject(jsg::Lock& js) {
return js.getEnv(true);
return js.getProcessEnv(true);
}

namespace {
Expand Down
24 changes: 24 additions & 0 deletions src/workerd/api/tests/disable-importable-env-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { env, withEnv } from 'cloudflare:workers';

// This test runs with the disallow-importable-env flag set, meaning that
// the env imported from cloudflare:workers exists but is not-populated
// with the primary environment. Using withEnv, however, would still work.

import { strictEqual, notDeepStrictEqual } from 'node:assert';

strictEqual(env.FOO, undefined);

export const test = {
async test(_, argEnv) {
strictEqual(env.FOO, undefined);
notDeepStrictEqual(argEnv, env);

// withEnv still works as expected tho
const { env: otherEnv2 } = await withEnv({ BAZ: 1 }, async () => {
await scheduler.wait(0);
return import('child');
});
strictEqual(otherEnv2.FOO, undefined);
strictEqual(otherEnv2.BAZ, 1);
},
};
22 changes: 22 additions & 0 deletions src/workerd/api/tests/disable-importable-env-test.wd-test
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "disable-importable-env-test",
worker = (
modules = [
(name = "worker", esModule = embed "disable-importable-env-test.js"),
(name = "child", esModule = "import {env as live} from 'cloudflare:workers'; export const env = {...live};"),
],
compatibilityDate = "2025-02-01",
compatibilityFlags = [
"nodejs_compat_v2",
"disallow_importable_env",
],
bindings = [
(name = "FOO", text = "BAR"),
],
)
),
],
);
101 changes: 101 additions & 0 deletions src/workerd/api/tests/importable-env-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import {
strictEqual,
deepStrictEqual,
notStrictEqual,
ok,
rejects,
} from 'node:assert';
import { env, withEnv } from 'cloudflare:workers';

import { AsyncLocalStorage } from 'node:async_hooks';
const check = new AsyncLocalStorage();

// The env is populated at the top level scope.
strictEqual(env.FOO, 'BAR');

// Cache exists and is accessible.
ok(env.CACHE);
// But fails when used because we're not in an io-context
await rejects(
env.CACHE.read('hello', async () => {}),
{
message: /Disallowed operation called within global scope./,
}
);

export const importableEnv = {
async test(_, argEnv) {
// Accessing the cache initially at the global scope didn't break anything
const cached = await argEnv.CACHE.read('hello', async () => {
return {
value: 123,
expiration: Date.now() + 10000,
};
});
strictEqual(cached, 123);

// They aren't the same objects...
notStrictEqual(env, argEnv);
// But have all the same stuff...
deepStrictEqual(env, argEnv);

// It is populated inside a request
strictEqual(env.FOO, 'BAR');

// And following async operations.
await scheduler.wait(10);
strictEqual(env.FOO, 'BAR');

// Mutations to the env carry through as expected...
env.BAR = 123;
const { env: otherEnv } = await import('child');
strictEqual(otherEnv.FOO, 'BAR');
strictEqual(otherEnv.BAR, 123);
deepStrictEqual(argEnv, otherEnv);

// Using withEnv to replace the env...
const { env: otherEnv2 } = await withEnv({ BAZ: 1 }, async () => {
await scheduler.wait(0);
return import('child2');
});
strictEqual(otherEnv2.FOO, undefined);
strictEqual(otherEnv2.BAZ, 1);

// Original env is unmodified
strictEqual(env.BAZ, undefined);

// Verify that JSRPC calls appropriately see the environment.
const remote = await argEnv.RPC.rpcTarget(undefined);
await Promise.all([remote.test(), remote.test2()]);
},

get fetch() {
// It's a weird edge case, yes, but let's make sure that the env is
// available in getters when extracting the default handlers.
strictEqual(env.FOO, 'BAR');
return async () => {};
},

// Verifies that the environment is available and correctly propagated
// in custom events like jsrpc.
rpcTarget() {
strictEqual(env.FOO, 'BAR');
return withEnv({ FOO: 'BAZ' }, () => {
// Arguably, returned RPC targets should probably automatically capture
// the current async context and propagate that on subsequent calls,
// but they currently do not... therefore we have to manually capture
// the async context and be sure to enter it ourselves below.
const runInAsyncContext = AsyncLocalStorage.snapshot();
return {
test() {
// This one runs with the modified env
runInAsyncContext(() => strictEqual(env.FOO, 'BAZ'));
},
test2() {
// This one runs with the original env
strictEqual(env.FOO, 'BAR');
},
};
});
},
};
Loading
Loading