From 345a061d3b15ce245a2398478d15fb89af794cd0 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 31 Jan 2024 21:52:50 -0800 Subject: [PATCH] fix(windows): make `process.env` case-insensitive (#8578) * yay!!!!!! * [autofix.ci] apply automated fixes * ok * do not use reflect here * ok * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- CMakeLists.txt | 2 +- src/bun.js/ConsoleObject.zig | 56 ++++++++++--------- .../bindings/JSEnvironmentVariableMap.cpp | 47 +++++++++++++++- src/js/builtins/ProcessObjectInternals.ts | 44 +++++++++++++++ src/string_immutable.zig | 1 - test/cli/run/env.test.ts | 21 ++++++- test/js/node/env-windows.test.ts | 33 +++++++++++ 7 files changed, 171 insertions(+), 33 deletions(-) create mode 100644 test/js/node/env-windows.test.ts diff --git a/CMakeLists.txt b/CMakeLists.txt index 147cec1fcf..7b3fb3d6dc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.22) cmake_policy(SET CMP0091 NEW) cmake_policy(SET CMP0067 NEW) -set(Bun_VERSION "1.0.26") +set(Bun_VERSION "1.1.0") set(WEBKIT_TAG 9c501b9aa712b7959f80dc99491e8758c151c20e) set(BUN_WORKDIR "${CMAKE_CURRENT_BINARY_DIR}") diff --git a/src/bun.js/ConsoleObject.zig b/src/bun.js/ConsoleObject.zig index a332cd3bc5..840b05d350 100644 --- a/src/bun.js/ConsoleObject.zig +++ b/src/bun.js/ConsoleObject.zig @@ -1107,22 +1107,24 @@ pub const Formatter = struct { return .{ .tag = switch (js_type) { - JSValue.JSType.ErrorInstance => .Error, - JSValue.JSType.NumberObject => .Double, - JSValue.JSType.DerivedArray, JSValue.JSType.Array => .Array, - JSValue.JSType.DerivedStringObject, JSValue.JSType.String, JSValue.JSType.StringObject => .String, - JSValue.JSType.RegExpObject => .String, - JSValue.JSType.Symbol => .Symbol, - JSValue.JSType.BooleanObject => .Boolean, - JSValue.JSType.JSFunction => .Function, - JSValue.JSType.JSWeakMap, JSValue.JSType.JSMap => .Map, - JSValue.JSType.JSMapIterator => .MapIterator, - JSValue.JSType.JSSetIterator => .SetIterator, - JSValue.JSType.JSWeakSet, JSValue.JSType.JSSet => .Set, - JSValue.JSType.JSDate => .JSON, - JSValue.JSType.JSPromise => .Promise, - JSValue.JSType.Object, - JSValue.JSType.FinalObject, + .ErrorInstance => .Error, + .NumberObject => .Double, + .DerivedArray, JSValue.JSType.Array => .Array, + .DerivedStringObject, JSValue.JSType.String, JSValue.JSType.StringObject => .String, + .RegExpObject => .String, + .Symbol => .Symbol, + .BooleanObject => .Boolean, + .JSFunction => .Function, + .JSWeakMap, JSValue.JSType.JSMap => .Map, + .JSMapIterator => .MapIterator, + .JSSetIterator => .SetIterator, + .JSWeakSet, JSValue.JSType.JSSet => .Set, + .JSDate => .JSON, + .JSPromise => .Promise, + + .Object, + .FinalObject, + .ProxyObject, .ModuleNamespaceObject, => .Object, @@ -1132,17 +1134,17 @@ pub const Formatter = struct { .GlobalObject, .ArrayBuffer, - JSValue.JSType.Int8Array, - JSValue.JSType.Uint8Array, - JSValue.JSType.Uint8ClampedArray, - JSValue.JSType.Int16Array, - JSValue.JSType.Uint16Array, - JSValue.JSType.Int32Array, - JSValue.JSType.Uint32Array, - JSValue.JSType.Float32Array, - JSValue.JSType.Float64Array, - JSValue.JSType.BigInt64Array, - JSValue.JSType.BigUint64Array, + .Int8Array, + .Uint8Array, + .Uint8ClampedArray, + .Int16Array, + .Uint16Array, + .Int32Array, + .Uint32Array, + .Float32Array, + .Float64Array, + .BigInt64Array, + .BigUint64Array, .DataView, => .TypedArray, diff --git a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp index b23eac87d0..d0514ae781 100644 --- a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp +++ b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp @@ -5,7 +5,13 @@ #include #include +#include +#include +#include +#include + #include "BunClientData.h" + using namespace JSC; extern "C" size_t Bun__getEnvCount(JSGlobalObject* globalObject, void** list_ptr); @@ -48,7 +54,11 @@ JSC_DEFINE_CUSTOM_SETTER(jsSetterEnvironmentVariable, (JSGlobalObject * globalOb if (!object) return false; - object->putDirect(vm, propertyName, JSValue::decode(value), 0); + auto string = JSValue::decode(value).toString(globalObject); + if (UNLIKELY(!string)) + return false; + + object->putDirect(vm, propertyName, string, 0); return true; } @@ -125,19 +135,30 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) object = constructEmptyObject(globalObject, globalObject->objectPrototype()); } +#if OS(WINDOWS) + JSArray* keyArray = constructEmptyArray(globalObject, nullptr, count); +#endif + static NeverDestroyed TZ = MAKE_STATIC_STRING_IMPL("TZ"); bool hasTZ = false; for (size_t i = 0; i < count; i++) { unsigned char* chars; size_t len = Bun__getEnvKey(list, i, &chars); auto name = String::fromUTF8(chars, len); +#if OS(WINDOWS) + keyArray->putByIndexInline(globalObject, (unsigned)i, jsString(vm, name), false); +#endif if (name == TZ) { hasTZ = true; continue; } ASSERT(len > 0); - - Identifier identifier = Identifier::fromString(vm, name); +#if OS(WINDOWS) + String idName = name.convertToASCIIUppercase(); +#else + String idName = name; +#endif + Identifier identifier = Identifier::fromString(vm, idName); // CustomGetterSetter doesn't support indexed properties yet. // This causes strange issues when the environment variable name is an integer. @@ -165,6 +186,26 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) vm, Identifier::fromString(vm, TZ), JSC::CustomGetterSetter::create(vm, jsTimeZoneEnvironmentVariableGetter, jsTimeZoneEnvironmentVariableSetter), TZAttrs); +#if OS(WINDOWS) + JSC::JSFunction* getSourceEvent = JSC::JSFunction::create(vm, processObjectInternalsWindowsEnvCodeGenerator(vm), globalObject); + RETURN_IF_EXCEPTION(scope, {}); + JSC::MarkedArgumentBuffer args; + args.append(object); + args.append(keyArray); + auto clientData = WebCore::clientData(vm); + JSC::CallData callData = JSC::getCallData(getSourceEvent); + NakedPtr returnedException = nullptr; + auto result = JSC::call(globalObject, getSourceEvent, callData, globalObject->globalThis(), args, returnedException); + RETURN_IF_EXCEPTION(scope, {}); + + if (returnedException) { + throwException(globalObject, scope, returnedException.get()); + return jsUndefined(); + } + + RELEASE_AND_RETURN(scope, result); +#else return object; +#endif } } diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 48495efdeb..2bff5b66e5 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -344,3 +344,47 @@ export function setMainModule(value) { $putByIdDirectPrivate(this, "main", value); return true; } + +type InternalEnvMap = Record; + +export function windowsEnv(internalEnv: InternalEnvMap, envMapList: Array) { + // The use of String(key) here is intentional because Node.js as of v21.5.0 will throw + // on symbol keys as it seems they assume the user uses string keys: + // + // it throws "Cannot convert a Symbol value to a string" + + return new Proxy(internalEnv, { + get(_, p) { + return typeof p === "string" ? Reflect.get(internalEnv, p.toUpperCase()) : undefined; + }, + set(_, p, value) { + var k = String(p).toUpperCase(); + $assert(typeof p === "string"); // proxy is only string and symbol. the symbol would have thrown by now + if (!Reflect.has(internalEnv, k)) { + envMapList.push(p); + } + return Reflect.set(internalEnv, k, String(value)); + }, + has(_, p) { + return typeof p === "string" ? Reflect.has(internalEnv, p.toUpperCase()) : false; + }, + deleteProperty(_, p) { + return typeof p === "string" ? Reflect.deleteProperty(internalEnv, p.toUpperCase()) : true; + }, + defineProperty(_, p, attributes) { + var k = String(p).toUpperCase(); + $assert(typeof p === "string"); // proxy is only string and symbol. the symbol would have thrown by now + if (!Reflect.has(internalEnv, k)) { + envMapList.push(p); + } + return Reflect.defineProperty(internalEnv, k, attributes); + }, + getOwnPropertyDescriptor(target, p) { + return typeof p === "string" ? Reflect.getOwnPropertyDescriptor(target, p.toUpperCase()) : undefined; + }, + ownKeys() { + // .slice() because paranoia that there is a way to call this without the engine cloning it for us + return envMapList.slice(); + }, + }); +} diff --git a/src/string_immutable.zig b/src/string_immutable.zig index 20314a7714..166b49a378 100644 --- a/src/string_immutable.zig +++ b/src/string_immutable.zig @@ -5312,7 +5312,6 @@ pub fn convertUTF16toUTF8InBuffer( input: []const u16, ) ![]const u8 { // See above - if (input.len == 0) return &[_]u8{}; const result = bun.simdutf.convert.utf16.to.utf8.le(input, buf); // switch (result.status) { diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index 645b41cf67..6fcacd6ab9 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -701,8 +701,27 @@ console.log(dynamic().NODE_ENV); }); test("NODE_ENV default is not propogated in bun run", () => { + const getenv = + process.platform !== "win32" ? "env | grep NODE_ENV && exit 1 || true" : "node -e if(process.env.NODE_ENV)throw(1)"; const tmp = tempDirWithFiles("default-node-env", { - "package.json": '{"scripts":{"show-env":"env | grep NODE_ENV && exit 1 || true"}}', + "package.json": '{"scripts":{"show-env":' + JSON.stringify(getenv) + "}}", }); expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); + +const todoOnPosix = process.platform !== "win32" ? test.todo : test; +todoOnPosix("setting process.env coerces the value to a string", () => { + // @ts-expect-error + process.env.SET_TO_TRUE = true; + let did_call = 0; + // @ts-expect-error + process.env.SET_TO_BUN = { + toString() { + did_call++; + return "bun!"; + }, + }; + expect(process.env.SET_TO_TRUE).toBe("true"); + expect(process.env.SET_TO_BUN).toBe("bun!"); + expect(did_call).toBe(1); +}); diff --git a/test/js/node/env-windows.test.ts b/test/js/node/env-windows.test.ts new file mode 100644 index 0000000000..36ddd41d98 --- /dev/null +++ b/test/js/node/env-windows.test.ts @@ -0,0 +1,33 @@ +import { test, expect } from "bun:test"; + +test.if(process.platform === "win32")("process.env is case insensitive on windows", () => { + const keys = Object.keys(process.env); + // this should have at least one character that is lowercase + // it is likely that PATH will be 'Path', and also stuff like 'WindowsLibPath' and so on. + // but not guaranteed, so we just check that there is at least one of each case + expect( + keys + .join("") + .split("") + .some(c => c.toUpperCase() !== c), + ).toBe(true); + expect( + keys + .join("") + .split("") + .some(c => c.toLowerCase() !== c), + ).toBe(true); + expect(process.env.path).toBe(process.env.PATH!); + expect(process.env.pAtH).toBe(process.env.PATH!); + + expect(process.env.doesntexistahahahahaha).toBeUndefined(); + // @ts-expect-error + process.env.doesntExistAHaHaHaHaHa = true; + expect(process.env.doesntexistahahahahaha).toBe("true"); + expect(process.env.doesntexistahahahahaha).toBe("true"); + expect(process.env.doesnteXISTahahahahaha).toBe("true"); + expect(Object.keys(process.env).pop()).toBe("doesntExistAHaHaHaHaHa"); + delete process.env.DOESNTEXISTAHAHAHAHAHA; + expect(process.env.doesntexistahahahahaha).toBeUndefined(); + expect(Object.keys(process.env)).not.toInclude("doesntExistAHaHaHaHaHa"); +});