diff --git a/src/js/node/worker_threads.ts b/src/js/node/worker_threads.ts index acac42253063..6e63f675cd3d 100644 --- a/src/js/node/worker_threads.ts +++ b/src/js/node/worker_threads.ts @@ -1357,7 +1357,11 @@ class Worker extends EventEmitter { // if not the message is the actual error const message = event.message; if (message !== "") { + // The value didn't clone, so rebuild from the text — but keep the `code` + // the native side carried over, which is all that survived of it. + const code = error?.code; error = new Error(message, { cause: event }); + if (typeof code === "string") error.code = code; const stack = event?.stack; if (stack) { error.stack = stack; diff --git a/src/jsc/bindings/webcore/Worker.cpp b/src/jsc/bindings/webcore/Worker.cpp index 75b93e0bb15b..6682985171ce 100644 --- a/src/jsc/bindings/webcore/Worker.cpp +++ b/src/jsc/bindings/webcore/Worker.cpp @@ -33,6 +33,7 @@ #include "Event.h" #include "EventNames.h" #include "StructuredSerializeOptions.h" +#include #include #include #include @@ -516,17 +517,45 @@ void Worker::fireEarlyMessages(Zig::GlobalObject* workerGlobalObject) } } -void Worker::dispatchErrorWithMessage(WTF::String message) +void Worker::dispatchErrorWithMessage(WTF::String message, WTF::String code) { - postTaskToParent([protectedThis = Ref { *this }, message = message.isolatedCopy()](ScriptExecutionContext&) { + postTaskToParent([protectedThis = Ref { *this }, message = message.isolatedCopy(), + code = code.isolatedCopy()](ScriptExecutionContext& context) { ErrorEvent::Init init; init.message = message; + // The worker's value would not clone (Bun's ResolveMessage is not even an + // Error), so the parent rebuilds one from the text; hand `code` over on a + // carrier object or it is lost, unlike every other coded worker error. + if (!code.isNull()) { + auto* globalObject = context.globalObject(); + auto& vm = JSC::getVM(globalObject); + if (auto* carrier = JSC::createError(globalObject, message)) { + carrier->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), JSC::jsString(vm, code)); + init.error = carrier; + } + } auto event = ErrorEvent::create(eventNames().errorEvent, init, EventIsTrusted::Yes); protectedThis->dispatchEvent(event); }); } +// A string `code` off an error-ish value, or null. Reading it can run JS (a +// getter/proxy), so it is done under a top scope and any throw drops the code. +String Worker::errorCodeOf(JSC::JSGlobalObject* globalObject, JSValue value) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); + if (!value.isObject()) + return {}; + JSValue codeValue = value.getObject()->getIfPropertyExists(globalObject, WebCore::builtinNames(vm).codePublicName()); + String code; + if (!scope.exception() && codeValue && codeValue.isString()) + code = codeValue.toWTFString(globalObject); + CLEAR_IF_EXCEPTION(scope); + return code; +} + bool Worker::dispatchErrorWithValue(Zig::GlobalObject* workerGlobalObject, JSValue value) { // This is the top of the stack for the worker's error dispatch: both the @@ -565,13 +594,7 @@ bool Worker::dispatchErrorWithValue(Zig::GlobalObject* workerGlobalObject, JSVal // vendored node tests assert on it (e.g. ERR_TRACE_EVENTS_UNAVAILABLE). // Carry a string `code` across the thread boundary manually. If reading // `code` throws (a throwing getter/proxy), drop the code and proceed. - String errorCode; - if (value.isObject() && !scope.exception()) { - JSValue codeValue = value.getObject()->getIfPropertyExists(workerGlobalObject, WebCore::builtinNames(vm).codePublicName()); - if (!scope.exception() && codeValue && codeValue.isString()) - errorCode = codeValue.toWTFString(workerGlobalObject); - CLEAR_IF_EXCEPTION(scope); - } + String errorCode = errorCodeOf(workerGlobalObject, value); return postTaskToParent([protectedThis = Ref { *this }, serialized, errorCode = WTF::move(errorCode).isolatedCopy()](ScriptExecutionContext& context) { auto* globalObject = context.globalObject(); @@ -759,11 +782,12 @@ extern "C" void WebWorker__dispatchError(Zig::GlobalObject* globalObject, Worker globalObject->globalEventScope->dispatchEvent(ErrorEvent::create(eventNames().errorEvent, init, EventIsTrusted::Yes)); switch (worker->options().kind) { case WorkerOptions::Kind::Web: - return worker->dispatchErrorWithMessage(WTF::move(messageStr)); + return worker->dispatchErrorWithMessage(WTF::move(messageStr), {}); case WorkerOptions::Kind::Node: if (!worker->dispatchErrorWithValue(globalObject, error)) { - // If serialization threw an error, use the string instead - worker->dispatchErrorWithMessage(WTF::move(messageStr)); + // If serialization threw an error, use the string instead — but keep + // `code`, which is all the parent can otherwise recover. + worker->dispatchErrorWithMessage(WTF::move(messageStr), Worker::errorCodeOf(globalObject, error)); } return; } diff --git a/src/jsc/bindings/webcore/Worker.h b/src/jsc/bindings/webcore/Worker.h index 22b9e8ab84e8..38a1382b5aae 100644 --- a/src/jsc/bindings/webcore/Worker.h +++ b/src/jsc/bindings/webcore/Worker.h @@ -133,7 +133,8 @@ class Worker final : public ThreadSafeRefCounted, public EventTargetWith void dispatchOnlineEvent(); void dispatchOnline(Zig::GlobalObject* workerGlobalObject); void fireEarlyMessages(Zig::GlobalObject* workerGlobalObject); - void dispatchErrorWithMessage(WTF::String message); + void dispatchErrorWithMessage(WTF::String message, WTF::String code); + static WTF::String errorCodeOf(JSC::JSGlobalObject*, JSC::JSValue); bool dispatchErrorWithValue(Zig::GlobalObject* workerGlobalObject, JSValue value); bool dispatchExit(int32_t exitCode); diff --git a/test/js/node/test/parallel/test-worker-internal-modules.mjs b/test/js/node/test/parallel/test-worker-internal-modules.mjs new file mode 100644 index 000000000000..607eade85c5d --- /dev/null +++ b/test/js/node/test/parallel/test-worker-internal-modules.mjs @@ -0,0 +1,36 @@ +import '../common/index.mjs'; +import tmpdir from '../common/tmpdir.js'; +import assert from 'node:assert/strict'; +import { once } from 'node:events'; +import fs from 'node:fs/promises'; +import { describe, test, before } from 'node:test'; +import { Worker } from 'node:worker_threads'; + +const accessInternalsSource = ` +import 'node:internal/freelist'; +`; + +function convertScriptSourceToDataUrl(script) { + return new URL(`data:text/javascript,${encodeURIComponent(script)}`); +} + +describe('Worker threads should not be able to access internal modules', () => { + before(() => tmpdir.refresh()); + + test('worker instantiated with module file path', async () => { + const moduleFilepath = tmpdir.resolve('test-worker-internal-modules.mjs'); + await fs.writeFile(moduleFilepath, accessInternalsSource); + const w = new Worker(moduleFilepath); + await assert.rejects(once(w, 'exit'), { code: 'ERR_UNKNOWN_BUILTIN_MODULE' }); + }); + + test('worker instantiated with module source', async () => { + const w = new Worker(accessInternalsSource, { eval: true }); + await assert.rejects(once(w, 'exit'), { code: 'ERR_UNKNOWN_BUILTIN_MODULE' }); + }); + + test('worker instantiated with data: URL', async () => { + const w = new Worker(convertScriptSourceToDataUrl(accessInternalsSource)); + await assert.rejects(once(w, 'exit'), { code: 'ERR_UNKNOWN_BUILTIN_MODULE' }); + }); +});