diff --git a/src/js/internal/fs/streams.ts b/src/js/internal/fs/streams.ts index 365c24987f53..7652ad1537fd 100644 --- a/src/js/internal/fs/streams.ts +++ b/src/js/internal/fs/streams.ts @@ -34,6 +34,7 @@ type FSStream = import("node:fs").ReadStream & type FD = number; const { validateInteger, validateInt32, validateFunction } = require("internal/validators"); +const { isURL } = require("internal/url"); const kIsPerformingIO = Symbol("kIsPerformingIO"); const kIoDone = Symbol("kIoDone"); @@ -88,7 +89,7 @@ function streamFileHandleClose(this: FileHandle, fd: FD, cb: (err?: any) => void } function getValidatedPath(p: any) { - if (p instanceof URL) return Bun.fileURLToPath(p as URL); + if (isURL(p)) return Bun.fileURLToPath(p); if (typeof p !== "string") throw $ERR_INVALID_ARG_TYPE("path", "string or URL", p); return require("node:path").resolve(p); } diff --git a/src/js/internal/fs/watch.ts b/src/js/internal/fs/watch.ts index 9698ed78ec74..bbfc6e405955 100644 --- a/src/js/internal/fs/watch.ts +++ b/src/js/internal/fs/watch.ts @@ -1,6 +1,7 @@ // fs.watch is lazily loaded so the FSWatcher class is only set up when it is used. const EventEmitter = require("node:events"); const { basename } = require("node:path"); +const { isURL } = require("internal/url"); // The native `node:fs` binding, shared via `internal/fs/binding`. const fs = require("internal/fs/binding"); @@ -138,7 +139,7 @@ class FSWatcher extends EventEmitter { constructor(path, options, listener) { super(); - if (path instanceof URL) { + if (isURL(path)) { path = Bun.fileURLToPath(path); } else if (typeof path === "string" && path.startsWith("file:")) { path = Bun.fileURLToPath(path); diff --git a/src/js/internal/url.ts b/src/js/internal/url.ts index c29a0e4241e8..578423f7e874 100644 --- a/src/js/internal/url.ts +++ b/src/js/internal/url.ts @@ -1,3 +1,8 @@ +// Node's isURL (lib/internal/url.js). Legacy url.parse() objects have auth and path. +function isURL(self) { + return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined); +} + function urlToHttpOptions(url) { const options = { ...url, @@ -23,5 +28,6 @@ function urlToHttpOptions(url) { } export default { + isURL, urlToHttpOptions, }; diff --git a/src/js/internal/validators.ts b/src/js/internal/validators.ts index 92b3f2eb5efb..11f7eca1a511 100644 --- a/src/js/internal/validators.ts +++ b/src/js/internal/validators.ts @@ -1,7 +1,10 @@ const { hideFromStack } = require("internal/shared"); +const { isURL } = require("internal/url"); +const { isUint8Array } = require("node:util/types"); const RegExpPrototypeExec = RegExp.prototype.exec; const ArrayIsArray = Array.isArray; +const Uint8ArrayPrototypeIncludes = Uint8Array.prototype.includes; const tokenRegExp = /^[\^_`a-zA-Z\-0-9!#$%&'*+.|~]+$/; /** @@ -87,7 +90,7 @@ function validateBoolean(value, name) { /** Validate a string-or-URL path and return it resolved to an absolute path string. */ function getValidatedPath(p: any) { - if (p instanceof URL) return Bun.fileURLToPath(p as URL); + if (isURL(p)) return Bun.fileURLToPath(p); if (typeof p !== "string") throw $ERR_INVALID_ARG_TYPE("path", "string or URL", p); if (p.startsWith("file:")) return Bun.fileURLToPath(p); return require("node:path").resolve(p); @@ -99,21 +102,17 @@ function throwIfNullBytesInFileName(filename: string) { } } -/** - * node's fs getValidatedPath (lib/internal/fs/utils.js): converts URL - * *instances* via fileURLToPath, accepts strings and Buffers as-is (no - * path.resolve, no "file:"-prefix string sniffing), and rejects null bytes. - */ +/** node's fs getValidatedPath (lib/internal/fs/utils.js): a URL becomes a path, strings and Buffers pass as-is. */ function getValidatedFsPath(p: any, propName: string = "path") { - if (p instanceof URL) p = Bun.fileURLToPath(p); + if (isURL(p)) p = Bun.fileURLToPath(p); if (typeof p === "string") { if (p.indexOf("\u0000") !== -1) { throw $ERR_INVALID_ARG_VALUE(propName, p, "must be a string, Uint8Array, or URL without null bytes"); } return p; } - if (p instanceof Uint8Array) { - if (p.indexOf(0) !== -1) { + if (isUint8Array(p)) { + if (Uint8ArrayPrototypeIncludes.$call(p, 0)) { throw $ERR_INVALID_ARG_VALUE(propName, p, "must be a string, Uint8Array, or URL without null bytes"); } return p; @@ -171,7 +170,7 @@ export default { validateBuffer: $newCppFunction("NodeValidator.cpp", "jsFunction_validateBuffer", 0), /** `(value, name, oneOf)` */ validateOneOf: $newCppFunction("NodeValidator.cpp", "jsFunction_validateOneOf", 0), - isUint8Array: value => value instanceof Uint8Array, + isUint8Array, /** `(path)` — accepts a string or file URL, returns it resolved to an absolute path string */ getValidatedPath, getValidatedFsPath, diff --git a/src/js/node/_http_client.ts b/src/js/node/_http_client.ts index 8404735a4de0..d7efd00340bb 100644 --- a/src/js/node/_http_client.ts +++ b/src/js/node/_http_client.ts @@ -16,7 +16,7 @@ const { } = require("node:_http_common"); const { kUniqueHeaders, parseUniqueHeadersOption, OutgoingMessage } = require("node:_http_outgoing"); const Agent = require("node:_http_agent"); -const { urlToHttpOptions } = require("internal/url"); +const { isURL, urlToHttpOptions } = require("internal/url"); const { kOutHeaders, kNeedDrain, kProxyConfig, checkShouldUseProxy } = require("internal/http"); const { validateInteger, validateBoolean, validateString, validateOneOf } = require("internal/validators"); const { getTimerDuration } = require("internal/timers"); @@ -95,10 +95,6 @@ function closeRequest(req) { req.emit("close"); } -function isURLInstance(input) { - return input != null && typeof input === "object" && input instanceof URL; -} - // When proxying a HTTP request, the following needs to be done: // https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2 // 1. Rewrite the request path to absolute-form. @@ -169,7 +165,7 @@ function ClientRequest(input, options, cb) { if (typeof input === "string") { const urlStr = input; input = urlToHttpOptions(new URL(urlStr)); - } else if (isURLInstance(input)) { + } else if (isURL(input)) { // url.URL instance input = urlToHttpOptions(input); } else { diff --git a/src/js/node/child_process.ts b/src/js/node/child_process.ts index 29eee5b7b3f2..b57baee2f342 100644 --- a/src/js/node/child_process.ts +++ b/src/js/node/child_process.ts @@ -9,6 +9,7 @@ const { validateArray, validateObject, validateOneOf, + isUint8Array, } = require("internal/validators"); var NetModule; @@ -1925,10 +1926,6 @@ function getValidatedPath(fileURLOrPath, propName = "path") { return path; } -function isUint8Array(value) { - return typeof value === "object" && value !== null && value instanceof Uint8Array; -} - //------------------------------------------------------------------------------ // Section 6. Random utilities //------------------------------------------------------------------------------ diff --git a/src/js/node/https.ts b/src/js/node/https.ts index 414742fe2955..aa18ef29a70b 100644 --- a/src/js/node/https.ts +++ b/src/js/node/https.ts @@ -3,7 +3,7 @@ // https://github.com/nodejs/node/blob/v26.3.0/lib/https.js const http = require("node:http"); const { isIP } = require("internal/net/isIP"); -const { urlToHttpOptions } = require("internal/url"); +const { isURL, urlToHttpOptions } = require("internal/url"); const { kEmptyObject, once } = require("internal/shared"); const { validateObject } = require("internal/validators"); const { kProxyConfig, checkShouldUseProxy, kWaitForProxyTunnel } = require("internal/http"); @@ -20,7 +20,7 @@ function request(...args) { if (typeof args[0] === "string") { const urlStr = ArrayPrototypeShift.$call(args); options = urlToHttpOptions(new URL(urlStr)); - } else if (args[0] instanceof URL) { + } else if (isURL(args[0])) { options = urlToHttpOptions(ArrayPrototypeShift.$call(args)); } diff --git a/src/js/node/url.ts b/src/js/node/url.ts index 8b82062b6364..d086f0405313 100644 --- a/src/js/node/url.ts +++ b/src/js/node/url.ts @@ -27,7 +27,7 @@ const { URL, URLSearchParams, URLPattern } = globalThis; const [domainToASCII, domainToUnicode, idnaToASCII] = $cpp("NodeURL.cpp", "Bun::createNodeURLBinding"); -const { urlToHttpOptions } = require("internal/url"); +const { isURL, urlToHttpOptions } = require("internal/url"); const { validateString, validateObject } = require("internal/validators"); const ObjectSetPrototypeOf = Object.setPrototypeOf; @@ -1330,14 +1330,6 @@ function hexByteToNumber(byte: number): number { return -1; } -// Node's isURL (lib/internal/url.js): a duck-type check rather than -// `instanceof`, so cross-realm URLs and compatible foreign implementations -// are accepted; `auth`/`path` must be absent to exclude legacy `url.parse` -// objects, which carry both. -function isURL(self: any): boolean { - return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined); -} - function fileURLToPathBuffer(path: unknown, options?: { windows?: boolean }): Buffer { const windows = options?.windows; if (typeof path === "string") { diff --git a/src/js/node/worker_threads.ts b/src/js/node/worker_threads.ts index 1658bd7cb4eb..edc2fddd3cc8 100644 --- a/src/js/node/worker_threads.ts +++ b/src/js/node/worker_threads.ts @@ -23,14 +23,15 @@ function normalizeWorkerName(rawName) { } const { isAbsolute: pathIsAbsolute } = require("node:path"); +const { isURL } = require("internal/url"); // node's filename validation for non-eval workers: absolute or "./"/"../"-relative // paths and file: URL objects; bare specifiers and string URLs are rejected. function validateWorkerFilename(filename) { - if (filename instanceof URL) { - if (filename.protocol === "data:") return `${filename}`; + if (isURL(filename)) { + if (filename.protocol === "data:") return filename.href; // throws ERR_INVALID_URL_SCHEME (TypeError) for non-file: URLs - return Bun.fileURLToPath(filename); + return Bun.fileURLToPath(filename.href); } if (typeof filename !== "string") { // Not a string or URL: defer to the native Worker constructor, which diff --git a/test/js/node/child_process/child_process.test.ts b/test/js/node/child_process/child_process.test.ts index dfc6dd2f5f6e..275e9663f6e1 100644 --- a/test/js/node/child_process/child_process.test.ts +++ b/test/js/node/child_process/child_process.test.ts @@ -17,6 +17,7 @@ import { ChildProcess, exec, execFile, execFileSync, execSync, fork, spawn, spaw import { getEventListeners, once, setMaxListeners } from "node:events"; import os from "node:os"; import { promisify } from "node:util"; +import vm from "node:vm"; import path from "path"; const debug = process.env.DEBUG ? console.log : () => {}; @@ -744,6 +745,25 @@ describe("spawnSync()", () => { signal: null, }); }); + + it("validates a cwd Uint8Array from another realm like a same-realm one", () => { + // A typed array from a node:vm context has that realm's prototype, so + // `instanceof Uint8Array` is false for it. node checks the cell type + // (util.types.isUint8Array), so both inputs take the same route. A bare + // Uint8Array cwd does not spawn in either runtime (only a Buffer + // stringifies to a path), so compare the outcomes instead of asserting one. + const bytes = [...Buffer.from(tmpdirSync())]; + const sameRealm = new Uint8Array(bytes); + const otherRealm = vm.runInNewContext(`new Uint8Array(${JSON.stringify(bytes)})`); + expect(otherRealm instanceof Uint8Array).toBe(false); + + const run = (cwd: Uint8Array) => { + const { status, error } = spawnSync(bunExe(), ["-e", ""], { cwd: cwd as any, env: bunEnv }); + return { status, code: (error as any)?.code }; + }; + // Before: ERR_INVALID_ARG_TYPE for options.cwd "Received an instance of Uint8Array". + expect(run(otherRealm)).toEqual(run(sameRealm)); + }); }); describe("execFileSync()", () => { diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index c555aa1b5576..39439490abc2 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -29,6 +29,7 @@ import { tmpdir } from "node:os"; import * as path from "node:path"; import { Duplex, duplexPair, PassThrough, Writable } from "node:stream"; import { connect as tlsConnect } from "node:tls"; +import vm from "node:vm"; import tunnel from "tunnel"; import { run as runHTTPProxyTest } from "./node-http-proxy.js"; const { describe, expect, it, beforeAll, afterAll, createDoneDotAll, mock, test } = createTest(import.meta.path); @@ -1079,6 +1080,153 @@ describe("node:http", () => { req.end(); }); }); + + // node decides "is this a URL?" structurally (lib/internal/url.js isURL: + // truthy href and protocol, no legacy url.parse() `auth`/`path`), so a + // WHATWG URL from another implementation (jsdom, whatwg-url) counts. A + // plain object holding a URL's fields stands in for one here. + function urlLikeOf(href: string) { + const url = new URL(href); + const urlLike = Object.create({ + toString() { + return this.href; + }, + }); + for (const key of [ + "href", + "origin", + "protocol", + "username", + "password", + "host", + "hostname", + "port", + "pathname", + "search", + "hash", + ]) { + urlLike[key] = url[key]; + } + return urlLike; + } + + function requestJSON(...args: any[]) { + return new Promise((resolve, reject) => { + const req = request(...(args as [any]), (res: IncomingMessage) => { + let body = ""; + res.setEncoding("utf8"); + res.on("data", chunk => (body += chunk)); + res.on("end", () => resolve(JSON.parse(body))); + }); + req.on("error", reject); + req.end(); + }); + } + + it("request() and get() read a URL-like object that is not a URL instance as a URL", async () => { + const server = createServer((req, res) => { + res.end(JSON.stringify({ method: req.method, url: req.url, host: req.headers.host })); + }); + server.listen(0); + await once(server, "listening"); + try { + const { port } = server.address() as AddressInfo; + const urlLike = urlLikeOf(`http://localhost:${port}/some/path?q=1#frag`); + expect(urlLike instanceof URL).toBe(false); + const expected = { method: "GET", url: "/some/path?q=1", host: `localhost:${port}` }; + + // An `instanceof URL` check took this object for an options bag. It has + // no `path` key, so the request silently went to "/". + expect(await requestJSON(urlLike)).toEqual(expected); + expect(await requestJSON(urlLike, { method: "POST" })).toEqual({ ...expected, method: "POST" }); + expect(await requestJSON(urlLike, { headers: { "x-extra": "1" } })).toEqual(expected); + + const viaGet = await new Promise((resolve, reject) => { + get(urlLike, res => { + let body = ""; + res.setEncoding("utf8"); + res.on("data", chunk => (body += chunk)); + res.on("end", () => resolve(JSON.parse(body))); + }).on("error", reject); + }); + expect(viaGet).toEqual(expected); + } finally { + server.close(); + } + }); + + it("request() accepts a URL instance after globalThis.URL was replaced", async () => { + // @happy-dom/global-registrator installs its own URL class on globalThis. + // A URL made before that, or by node:url, is then not `instanceof URL`. + // Before the fix it was taken for an options bag, and a URL instance has + // no own properties to copy, so the request went to localhost:80. + const NativeURL = URL; + const server = createServer((req, res) => { + res.end(JSON.stringify({ method: req.method, url: req.url, host: req.headers.host })); + }); + server.listen(0); + await once(server, "listening"); + const { port } = server.address() as AddressInfo; + const url = new NativeURL(`http://localhost:${port}/some/path?q=1`); + globalThis.URL = class URL extends NativeURL {}; + try { + expect(url instanceof URL).toBe(false); + expect(await requestJSON(url)).toEqual({ method: "GET", url: "/some/path?q=1", host: `localhost:${port}` }); + } finally { + globalThis.URL = NativeURL; + server.close(); + } + }); + + it("https.request() reads a URL-like object that is not a URL instance as a URL", () => { + const urlLike = urlLikeOf("https://localhost:1/some/path?q=1"); + const req = https.request(urlLike); + // Nothing listens on port 1; the connect error is expected and ignored. + req.on("error", () => {}); + try { + expect({ protocol: req.protocol, host: req.host, path: req.path }).toEqual({ + protocol: "https:", + host: "localhost", + path: "/some/path?q=1", + }); + } finally { + req.destroy(); + } + }); + + it("req.write() and req.end() accept a Uint8Array from another realm", async () => { + // A typed array created in a node:vm context has that realm's + // Uint8Array.prototype, so `instanceof Uint8Array` is false for it. The + // chunk check has to look at the cell type, like util.types.isUint8Array. + const chunk = vm.runInNewContext("new Uint8Array([104, 105])"); + expect(chunk instanceof Uint8Array).toBe(false); + + const server = createServer((req, res) => { + let body = ""; + req.setEncoding("utf8"); + req.on("data", part => (body += part)); + req.on("end", () => res.end(body)); + }); + server.listen(0); + await once(server, "listening"); + try { + const { port } = server.address() as AddressInfo; + const echoed = await new Promise((resolve, reject) => { + const req = request({ port, method: "POST" }, res => { + let body = ""; + res.setEncoding("utf8"); + res.on("data", part => (body += part)); + res.on("end", () => resolve(body)); + }); + req.on("error", reject); + req.write(chunk); + req.end(chunk); + }); + expect(echoed).toBe("hihi"); + } finally { + server.close(); + } + }); }); describe("https.request with custom tls options", () => { diff --git a/test/js/node/url/url-is-url.test.js b/test/js/node/url/url-is-url.test.js index be2f331eea78..38223debb5b5 100644 --- a/test/js/node/url/url-is-url.test.js +++ b/test/js/node/url/url-is-url.test.js @@ -1,5 +1,6 @@ // Flags: --expose-internals -import { describe, test } from "bun:test"; +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; import assert from "node:assert"; import { URL, parse } from "node:url"; @@ -19,4 +20,107 @@ describe("internal/url", () => { false, ); }); + + // The consumers of isURL, driven with URL instances after globalThis.URL was + // replaced. @happy-dom/global-registrator (the DOM testing setup in + // docs/test/dom.mdx) installs its own URL class there, so a URL made by + // node:url or Bun.pathToFileURL, or before the registration, is no longer + // `instanceof URL`. Node detects URLs structurally and is not affected. + test("URL instances are still URLs after globalThis.URL is replaced", async () => { + using dir = tempDir("is-url-consumers", { + "a.txt": "hello", + "worker.js": `require("node:worker_threads").parentPort.postMessage("from worker");`, + "main.js": ` + const NativeURL = URL; + globalThis.URL = class URL extends NativeURL {}; + const fs = require("node:fs"); + const http = require("node:http"); + const { once } = require("node:events"); + const { Worker } = require("node:worker_threads"); + const { pathToFileURL } = require("node:url"); + const fileURL = name => new NativeURL(pathToFileURL(name).href); + + const out = {}; + const probe = async (name, fn) => { + try { + out[name] = await fn(); + } catch (e) { + out[name] = e.code ?? e.message; + } + }; + out.instanceOfURL = fileURL("a.txt") instanceof URL; + + await probe("createReadStream", async () => { + let body = ""; + for await (const chunk of fs.createReadStream(fileURL("a.txt"), "utf8")) body += chunk; + return body; + }); + await probe("createWriteStream", async () => { + const stream = fs.createWriteStream(fileURL("w.txt")); + stream.end("written"); + await once(stream, "finish"); + return fs.readFileSync("w.txt", "utf8"); + }); + await probe("watch", () => { + fs.watch(fileURL("a.txt")).close(); + return "ok"; + }); + await probe("watchFile", () => { + fs.watchFile(fileURL("a.txt"), () => {}); + fs.unwatchFile(fileURL("a.txt")); + return "ok"; + }); + await probe("cpSync", () => { + fs.cpSync(fileURL("a.txt"), "b.txt"); + return fs.readFileSync("b.txt", "utf8"); + }); + await probe("cp", async () => { + await fs.promises.cp(fileURL("a.txt"), "c.txt"); + return fs.readFileSync("c.txt", "utf8"); + }); + await probe("Worker", async () => { + const worker = new Worker(fileURL("worker.js")); + const [message] = await once(worker, "message"); + await worker.terminate(); + return message; + }); + await probe("http.get", async () => { + const server = http.createServer((req, res) => res.end(req.url)); + server.listen(0); + await once(server, "listening"); + try { + const url = new NativeURL("http://127.0.0.1:" + server.address().port + "/some/path?q=1"); + const [res] = await once(http.get(url), "response"); + let body = ""; + for await (const chunk of res.setEncoding("utf8")) body += chunk; + return body; + } finally { + server.close(); + } + }); + console.log(JSON.stringify(out)); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.js"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual({ + instanceOfURL: false, + createReadStream: "hello", + createWriteStream: "written", + watch: "ok", + watchFile: "ok", + cpSync: "hello", + cp: "hello", + Worker: "from worker", + "http.get": "/some/path?q=1", + }); + expect(exitCode).toBe(0); + }); }); diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index f4a126a6dd19..3c672a2f2af7 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -4,6 +4,7 @@ import { once } from "node:events"; import fs from "node:fs"; import { join, relative, resolve } from "node:path"; import { Readable } from "node:stream"; +import { pathToFileURL } from "node:url"; import wt, { BroadcastChannel, getEnvironmentData, @@ -315,6 +316,39 @@ test("support worker eval that throws", async () => { await worker.terminate(); }); +test("filename accepts a URL-like object that is not a URL instance", async () => { + using dir = tempDir("worker-url-like", { + "worker.js": `require("node:worker_threads").parentPort.postMessage("from url-like");`, + }); + const url = pathToFileURL(join(String(dir), "worker.js")); + // node decides "is this a URL?" structurally (lib/internal/url.js isURL), so + // a WHATWG URL from another implementation (jsdom, whatwg-url) counts. A + // plain object with a URL's fields stands in for one here. + const urlLike = { href: url.href, protocol: url.protocol, hostname: url.hostname, pathname: url.pathname }; + const worker = new Worker(urlLike as any); + const [message] = await once(worker, "message"); + expect(message).toBe("from url-like"); + await worker.terminate(); + + // A data: URL-like object runs its href, not its string form. + const dataHref = `data:text/javascript,postMessage("from data url")`; + const dataWorker = new Worker({ href: dataHref, protocol: "data:", pathname: dataHref.slice(5) } as any); + const [dataMessage] = await once(dataWorker, "message"); + expect(dataMessage).toBe("from data url"); + await dataWorker.terminate(); + + // Like node, a URL-like object goes through fileURLToPath, so a non-file + // scheme fails on the scheme and not on the argument type. + expect(() => new Worker({ ...urlLike, href: "http://example.com/worker.js", protocol: "http:" } as any)).toThrow( + expect.objectContaining({ code: "ERR_INVALID_URL_SCHEME" }), + ); + // A legacy url.parse() object has `auth` and `path` (null, not undefined), + // so it is not a URL. + expect(() => new Worker({ ...urlLike, auth: null, path: url.pathname } as any)).toThrow( + expect.objectContaining({ code: "ERR_INVALID_ARG_TYPE" }), + ); +}); + describe("execArgv option", async () => { // this needs to be a subprocess to ensure that the parent's execArgv is not empty // otherwise we could not distinguish between the worker inheriting the parent's execArgv