From 3cb260ed15a26785272bb09427504a565010963d Mon Sep 17 00:00:00 2001 From: Felipe Baltor Date: Sun, 30 Jul 2023 10:13:28 -0300 Subject: [PATCH] fix(Deno.serve): accessing .url on cloned request throws (#19869) This PR fixes #19818. The problem was that the new InnerRequest class does not initialize the fields urlList and urlListProcessed that are used during a request clone. The solution aims to be straightforward by simply initializing the missing properties during the clone process. I also implemented a "cache" to the url getter of the new InnerRequest, avoiding the cost of calling op_http_get_request_method_and_url. --- cli/tests/unit/serve_test.ts | 96 ++++++++++++++++++++++++++++++++++++ ext/fetch/23_request.js | 4 +- ext/http/00_serve.js | 16 ++++-- 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/cli/tests/unit/serve_test.ts b/cli/tests/unit/serve_test.ts index fd7cc869ad..abb2562b1b 100644 --- a/cli/tests/unit/serve_test.ts +++ b/cli/tests/unit/serve_test.ts @@ -2789,6 +2789,7 @@ Deno.test( // https://github.com/denoland/deno/issues/15858 Deno.test( + "Clone should work", { permissions: { net: true } }, async function httpServerCanCloneRequest() { const ac = new AbortController(); @@ -2799,6 +2800,20 @@ Deno.test( const cloned = req.clone(); assertEquals(req.headers, cloned.headers); + assertEquals(cloned.url, req.url); + assertEquals(cloned.cache, req.cache); + assertEquals(cloned.destination, req.destination); + assertEquals(cloned.headers, req.headers); + assertEquals(cloned.integrity, req.integrity); + assertEquals(cloned.isHistoryNavigation, req.isHistoryNavigation); + assertEquals(cloned.isReloadNavigation, req.isReloadNavigation); + assertEquals(cloned.keepalive, req.keepalive); + assertEquals(cloned.method, req.method); + assertEquals(cloned.mode, req.mode); + assertEquals(cloned.redirect, req.redirect); + assertEquals(cloned.referrer, req.referrer); + assertEquals(cloned.referrerPolicy, req.referrerPolicy); + // both requests can read body await req.text(); await cloned.json(); @@ -2826,6 +2841,87 @@ Deno.test( }, ); +// https://fetch.spec.whatwg.org/#dom-request-clone +Deno.test( + "Throw if disturbed", + { permissions: { net: true } }, + async function shouldThrowIfBodyIsUnusableDisturbed() { + const ac = new AbortController(); + const listeningPromise = deferred(); + + const server = Deno.serve({ + handler: async (req) => { + await req.text(); + + try { + req.clone(); + } catch (error) { + assert(error instanceof TypeError); + assert( + error.message.endsWith("Body is unusable."), + ); + + ac.abort(); + await server.finished; + } + + return new Response("ok"); + }, + signal: ac.signal, + onListen: ({ port }: { port: number }) => listeningPromise.resolve(port), + }); + + try { + const port = await listeningPromise; + await fetch(`http://localhost:${port}/`, { + headers: { connection: "close" }, + method: "POST", + body: '{"bar":true}', + }); + } catch (e) { + assert(e instanceof TypeError); + } finally { + ac.abort(); + await server.finished; + } + }, +); + +// TODO(fbaltor): As it is this test should throw and fail +// https://fetch.spec.whatwg.org/#dom-request-clone +Deno.test({ + name: "Throw if locked", + ignore: true, + permissions: { net: true }, + fn: async function shouldThrowIfBodyIsUnusableLocked() { + const ac = new AbortController(); + const listeningPromise = deferred(); + + const server = Deno.serve({ + handler: (req) => { + const _reader = req.body?.getReader(); + + req.clone(); + + return new Response("ok"); + }, + signal: ac.signal, + onListen: ({ port }: { port: number }) => listeningPromise.resolve(port), + }); + + const port = await listeningPromise; + const resp = await fetch(`http://localhost:${port}/`, { + headers: { connection: "close" }, + method: "POST", + body: '{"bar":true}', + }); + + ac.abort(); + await resp.body?.cancel(); + await server.finished; + }, +}); + // Checks large streaming response // https://github.com/denoland/deno/issues/16567 Deno.test( diff --git a/ext/fetch/23_request.js b/ext/fetch/23_request.js index daf77a834e..afd3c1c50c 100644 --- a/ext/fetch/23_request.js +++ b/ext/fetch/23_request.js @@ -174,8 +174,8 @@ function cloneInnerRequest(request, skipBody = false) { body, redirectMode: request.redirectMode, redirectCount: request.redirectCount, - urlList: request.urlList, - urlListProcessed: request.urlListProcessed, + urlList: [() => request.url()], + urlListProcessed: [request.url()], clientRid: request.clientRid, blobUrlEntry: request.blobUrlEntry, url() { diff --git a/ext/http/00_serve.js b/ext/http/00_serve.js index e881cca2a3..b51dfee07b 100644 --- a/ext/http/00_serve.js +++ b/ext/http/00_serve.js @@ -129,6 +129,7 @@ class InnerRequest { #streamRid; #body; #upgraded; + #urlValue; constructor(slabId, context) { this.#slabId = slabId; @@ -236,6 +237,10 @@ class InnerRequest { } url() { + if (this.#urlValue !== undefined) { + return this.#urlValue; + } + if (this.#methodAndUri === undefined) { if (this.#slabId === undefined) { throw new TypeError("request closed"); @@ -249,27 +254,28 @@ class InnerRequest { // * is valid for OPTIONS if (path === "*") { - return "*"; + return this.#urlValue = "*"; } // If the path is empty, return the authority (valid for CONNECT) if (path == "") { - return this.#methodAndUri[1]; + return this.#urlValue = this.#methodAndUri[1]; } // CONNECT requires an authority if (this.#methodAndUri[0] == "CONNECT") { - return this.#methodAndUri[1]; + return this.#urlValue = this.#methodAndUri[1]; } const hostname = this.#methodAndUri[1]; if (hostname) { // Construct a URL from the scheme, the hostname, and the path - return this.#context.scheme + hostname + path; + return this.#urlValue = this.#context.scheme + hostname + path; } // Construct a URL from the scheme, the fallback hostname, and the path - return this.#context.scheme + this.#context.fallbackHost + path; + return this.#urlValue = this.#context.scheme + this.#context.fallbackHost + + path; } get remoteAddr() {