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

fix(windows): make process.env case-insensitive #8578

Merged
merged 7 commits into from
Feb 1, 2024
Merged
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
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand Down
56 changes: 29 additions & 27 deletions src/bun.js/ConsoleObject.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand All @@ -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,

Expand Down
47 changes: 44 additions & 3 deletions src/bun.js/bindings/JSEnvironmentVariableMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@

#include <JavaScriptCore/JSObject.h>
#include <JavaScriptCore/ObjectConstructor.h>
#include <JavaScriptCore/JSArray.h>
#include <JavaScriptCore/JSArrayInlines.h>
#include <JavaScriptCore/JSString.h>
#include <JavaScriptCore/JSStringInlines.h>

#include "BunClientData.h"

using namespace JSC;

extern "C" size_t Bun__getEnvCount(JSGlobalObject* globalObject, void** list_ptr);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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<String> 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();
dylan-conway marked this conversation as resolved.
Show resolved Hide resolved
#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.
Expand Down Expand Up @@ -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<JSC::Exception> 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
}
}
44 changes: 44 additions & 0 deletions src/js/builtins/ProcessObjectInternals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,3 +344,47 @@ export function setMainModule(value) {
$putByIdDirectPrivate(this, "main", value);
return true;
}

type InternalEnvMap = Record<string, string>;

export function windowsEnv(internalEnv: InternalEnvMap, envMapList: Array<string>) {
// 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;
paperdave marked this conversation as resolved.
Show resolved Hide resolved
},
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();
},
});
}
1 change: 0 additions & 1 deletion src/string_immutable.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
21 changes: 20 additions & 1 deletion test/cli/run/env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if todoIf would be useful

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);
});
33 changes: 33 additions & 0 deletions test/js/node/env-windows.test.ts
Original file line number Diff line number Diff line change
@@ -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");
});
Loading