From b930937325a9b4ab6ad751f9bfdd33059ef9a5cd Mon Sep 17 00:00:00 2001 From: maxmcd Date: Tue, 30 Apr 2024 21:57:16 -0400 Subject: [PATCH] Cleanup and fix file permissions for a script import --- deno-bootstrap/index.ts | 5 +- src/DenoHTTPWorker.test.ts | 46 ++++------- src/DenoHTTPWorker.ts | 152 ++++++++++++++----------------------- 3 files changed, 75 insertions(+), 128 deletions(-) diff --git a/deno-bootstrap/index.ts b/deno-bootstrap/index.ts index 21f8e8d..0960d39 100644 --- a/deno-bootstrap/index.ts +++ b/deno-bootstrap/index.ts @@ -15,10 +15,13 @@ if (typeof handler.default !== "function") { throw new Error("Default export is not a function."); } -Deno.serve({ path: socketFile }, (req: Request) => { +// Use an empty onListen callback to prevent Deno from logging +Deno.serve({ path: socketFile, onListen: () => {} }, (req: Request) => { const url = new URL(req.url); url.host = req.headers.get("X-Deno-Worker-Host") || url.host; url.port = req.headers.get("X-Deno-Worker-Port") || url.port; + // Setting url.protocol did not replace the protocol correctly for a unix + // socket. Replacing the href value seems to work well. url.href = url.href.replace( /^http\+unix:/, req.headers.get("X-Deno-Worker-Protocol") || url.protocol diff --git a/src/DenoHTTPWorker.test.ts b/src/DenoHTTPWorker.test.ts index 866cb69..2ffe6b9 100644 --- a/src/DenoHTTPWorker.test.ts +++ b/src/DenoHTTPWorker.test.ts @@ -24,7 +24,7 @@ describe("DenoHTTPWorker", { timeout: 1000 }, () => { return Response.json({ ok: req.url, headers: headers }) } `, - { printCommandAndArguments: true, printOutput: true } + { printOutput: true } ); for (let i = 0; i < 10; i++) { let json = await worker.client @@ -41,45 +41,27 @@ describe("DenoHTTPWorker", { timeout: 1000 }, () => { worker.terminate(); }); - it("deny-net not always allowed", async () => { - expect( - newDenoHTTPWorker(echoScript, { - runFlags: [`--deny-net`], - printOutput: true, - }) - ).rejects.toThrowError("not supported"); - expect( - newDenoHTTPWorker(echoScript, { - runFlags: [`--deny-net=0.0.0.0:0`], - printOutput: true, + it("should be able to import script", async () => { + const file = path.resolve(__dirname, "./test/echo-request.ts"); + const url = new URL(`file://${file}`); + let worker = await newDenoHTTPWorker(url, { + printOutput: true, + printCommandAndArguments: true, + }); + + let resp: any = await worker.client + .get("https://localhost/", { + headers: { "User-Agent": "some value" }, }) - ).rejects.toThrowError("with the address"); + .json(); + await worker.terminate(); }); - // it("should be able to import script", async () => { - // const file = path.resolve(__dirname, "./test/echo-request.ts"); - // const url = new URL(`file://${file}`); - // let worker = await newDenoHTTPWorker(url, { - // runFlags: [`--allow-read=${file}`], - // printOutput: true, - // }); - - // let resp: any = await worker.client - // .get("https://localhost/", { - // headers: { "User-Agent": "some value" }, - // }) - // .json(); - // await worker.terminate(); - // }); - it("user agent is not overwritten", async () => { - console.log(echoFile); let worker = await newDenoHTTPWorker(echoScript, { - printCommandAndArguments: true, printOutput: true, runFlags: [`--unstable-http`], }); - console.log("making request"); let resp: any = await worker.client .get("https://localhost/", { headers: { "User-Agent": "some value" }, diff --git a/src/DenoHTTPWorker.ts b/src/DenoHTTPWorker.ts index ea0c2f2..85c491e 100644 --- a/src/DenoHTTPWorker.ts +++ b/src/DenoHTTPWorker.ts @@ -22,10 +22,9 @@ const DEFAULT_DENO_BOOTSTRAP_SCRIPT_PATH = __dirname.endsWith("src") export interface DenoWorkerOptions { /** * The path to the executable that should be use when spawning the subprocess. - * Defaults to "deno". You can pass an array here if you want to invoke Deno - * with multiple arguments, like `sandbox run deno`. + * Defaults to "deno". */ - denoExecutable: string | string[]; + denoExecutable: string; /** * The path to the script that should be used to bootstrap the worker @@ -68,49 +67,52 @@ export const newDenoHTTPWorker = async ( script: string | URL, options?: Partial ): Promise => { - const _options: DenoWorkerOptions = Object.assign( - { - denoExecutable: "deno", - denoBootstrapScriptPath: DEFAULT_DENO_BOOTSTRAP_SCRIPT_PATH, - runFlags: [], - printCommandAndArguments: false, - spawnOptions: {}, - printOutput: false, - }, - options || {} - ); - - let networkingIsOk = false; + const _options: DenoWorkerOptions = { + denoExecutable: "deno", + denoBootstrapScriptPath: DEFAULT_DENO_BOOTSTRAP_SCRIPT_PATH, + runFlags: [], + printCommandAndArguments: false, + spawnOptions: {}, + printOutput: false, + ...options, + }; + + let scriptArgs: string[]; + + // Create the socket location that we'll use to communicate with Deno. + const socketFile = `${crypto.randomUUID()}-deno-http.sock`; + + // If we have a file import, make sure we allow read access to the file. + const allowReadFlagValue = + typeof script === "string" + ? socketFile + : `${socketFile},${script.href.replace("file://", "")}`; + + let allowReadFound = false; + let allowWriteFound = false; _options.runFlags = _options.runFlags.map((flag) => { - if (flag === "--allow-net" || flag === "--allow-all") { - networkingIsOk = true; + if (flag === "--allow-read" || flag === "--allow-all") { + allowReadFound = true; } - if (flag === "--deny-net") { - throw new Error( - "Using --deny-net without specifying specific addresses is not supported" - ); + if (flag === "--allow-write" || flag === "--allow-all") { + allowWriteFound = true; } - if (flag.startsWith("--deny-net") && flag.includes(LISTENING_HOSTPORT)) { - throw new Error( - `Using --deny-net with the address ${LISTENING_HOSTPORT} is not supported` - ); + if (flag.startsWith("--allow-read=")) { + allowReadFound = true; + return (flag += "," + allowReadFlagValue); } - if (flag.startsWith("--allow-net=")) { - networkingIsOk = true; - return (flag += "," + LISTENING_HOSTPORT); + if (flag.startsWith("--allow-write=")) { + allowReadFound = true; + return (flag += "," + socketFile); } return flag; }); - if (!networkingIsOk) { - _options.runFlags.push("--allow-net=" + LISTENING_HOSTPORT); + if (!allowReadFound) { + _options.runFlags.push("--allow-read=" + allowReadFlagValue); + } + if (!allowWriteFound) { + _options.runFlags.push("--allow-write=" + socketFile); } - - let scriptArgs: string[]; - - let socketFile = `${Math.random()}-deno-http.sock`.substring(2); - - _options.runFlags.push(`--allow-read=${socketFile}`); - _options.runFlags.push(`--allow-write=${socketFile}`); if (typeof script === "string") { scriptArgs = [socketFile, "script", script]; @@ -118,21 +120,7 @@ export const newDenoHTTPWorker = async ( scriptArgs = [socketFile, "import", script.href]; } - let command = "deno"; - if (typeof _options.denoExecutable === "string") { - command = _options.denoExecutable; - } - - if (Array.isArray(_options.denoExecutable)) { - if (_options.denoExecutable.length === 0) - throw new Error("denoExecutable must not be empty"); - - command = _options.denoExecutable[0]!; - _options.runFlags = [ - ..._options.denoExecutable.slice(1), - ..._options.runFlags, - ]; - } + const command = _options.denoExecutable; return new Promise(async (resolve, reject) => { const args = [ @@ -149,7 +137,7 @@ export const newDenoHTTPWorker = async ( let exited = false; let worker: DenoHTTPWorker; process.on("exit", (code: number, signal: string) => { - console.log("EXIT"); + exited = true; if (!running) { let stderr = process.stderr?.read()?.toString(); reject( @@ -158,7 +146,7 @@ export const newDenoHTTPWorker = async ( (stderr ? `\n${stderr}` : "") ) ); - exited = true; + fs.rm(socketFile); } else { worker.terminate(code, signal); } @@ -176,6 +164,7 @@ export const newDenoHTTPWorker = async ( }); } + // Wait for the socket file to be created by the Deno process. while (true) { if (exited) { break; @@ -185,7 +174,7 @@ export const newDenoHTTPWorker = async ( // File exists break; } catch (err) { - await new Promise((resolve) => setTimeout(resolve, 100)); + await new Promise((resolve) => setTimeout(resolve, 50)); } } @@ -200,7 +189,6 @@ export const newDenoHTTPWorker = async ( throw err; } }); - _httpSession.on("connect", () => { const _got = got.extend({ hooks: { @@ -210,7 +198,7 @@ export const newDenoHTTPWorker = async ( options.h2session = _httpSession; options.http2 = true; - // We follow got's example here: + // We follow Got's example here: // https://github.com/sindresorhus/got/blob/88e623a0d8140e02eef44d784f8d0327118548bc/documentation/examples/h2c.js#L32-L34 // But, this still surfaces a type error for various // differences between the implementation. Ignoring for now. @@ -218,7 +206,7 @@ export const newDenoHTTPWorker = async ( // @ts-ignore options.request = http2.request; - // Ensure the got user-agent string is never present. If a + // Ensure the Got user-agent string is never present. If a // value is passed by the user it will override got's // default value. if ( @@ -252,7 +240,7 @@ export const newDenoHTTPWorker = async ( worker = new DenoHTTPWorker( _httpSession, - 0, + socketFile, _got, process, stdout, @@ -268,7 +256,7 @@ export type { DenoHTTPWorker }; class DenoHTTPWorker { #httpSession: http2.ClientHttp2Session; #got: Got; - #denoListeningPort: number; + #socketFile: string; #process: ChildProcess; #stdout: Readable; #stderr: Readable; @@ -276,14 +264,14 @@ class DenoHTTPWorker { constructor( httpSession: http2.ClientHttp2Session, - denoListeningPort: number, + socketFile: string, got: Got, process: ChildProcess, stdout: Readable, stderr: Readable ) { this.#httpSession = httpSession; - this.#denoListeningPort = denoListeningPort; + this.#socketFile = socketFile; this.#got = got; this.#process = process; this.#stdout = stdout; @@ -293,16 +281,19 @@ class DenoHTTPWorker { get client(): Got { return this.#got; } + terminate(code?: number, signal?: string) { if (this.#terminated) { return; } - this.onexit(code || this.#process.exitCode || 0, signal || ""); this.#terminated = true; + this.onexit(code || this.#process.exitCode || 0, signal || ""); if (this.#process && this.#process.exitCode === null) { - // TODO: is this preventing listening on SIGINT for cleanup? Do we care? + // TODO: do we need to SIGINT first to make sure we allow the process to do + // any cleanup? forceKill(this.#process.pid!); } + fs.rm(this.#socketFile); this.#httpSession.close(); } @@ -314,9 +305,6 @@ class DenoHTTPWorker { return this.#stderr; } - get denoListeningPort(): number { - return this.#denoListeningPort; - } /** * Represents an event handler for the "exit" event. That is, a function to be * called when the Deno worker process is terminated. @@ -324,38 +312,12 @@ class DenoHTTPWorker { onexit: (code: number, signal: string) => void = () => {}; } -function addOption( - list: string[], - name: string, - option: boolean | string[] | undefined -) { - if (option === true) { - list.push(`${name}`); - } else if (Array.isArray(option)) { - let values = option.join(","); - list.push(`${name}=${values}`); - } -} - /** * Forcefully kills the process with the given ID. * On Linux/Unix, this means sending the process the SIGKILL signal. - * On Windows, this means using the taskkill executable to kill the process. - * @param pid The ID of the process to kill. */ export function forceKill(pid: number) { - // TODO: do we need to SIGINT first to make sure we allow the process to do - // any cleanup? - const isWindows = /^win/.test(process.platform); - if (isWindows) { - return killWindows(pid); - } else { - return killUnix(pid); - } -} - -function killWindows(pid: number) { - execSync(`taskkill /PID ${pid} /T /F`); + return killUnix(pid); } function killUnix(pid: number) {