From c3455c0ceee6bbe399781819a42fff6cf24792e2 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 13 Sep 2023 13:35:39 +0100 Subject: fix(node/fetch): Make data URL fetch consistent with node (#5126) --- test/js/web/fetch/fetch.test.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index aa44ee76a..4ef5d7bba 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -93,6 +93,30 @@ describe("fetch data urls", () => { expect(blob.type).toBe("text/plain;charset=utf-8"); expect(blob.text()).resolves.toBe("helloworld!"); }); + it("unstrict parsing of invalid URL characters", async () => { + var url = "data:application/json,{%7B%7D}"; + var res = await fetch(url); + expect(res.status).toBe(200); + expect(res.statusText).toBe("OK"); + expect(res.ok).toBe(true); + + var blob = await res.blob(); + expect(blob.size).toBe(4); + expect(blob.type).toBe("application/json;charset=utf-8"); + expect(blob.text()).resolves.toBe("{{}}"); + }); + it("unstrict parsing of double percent characters", async () => { + var url = "data:application/json,{%%7B%7D%%}%%"; + var res = await fetch(url); + expect(res.status).toBe(200); + expect(res.statusText).toBe("OK"); + expect(res.ok).toBe(true); + + var blob = await res.blob(); + expect(blob.size).toBe(9); + expect(blob.type).toBe("application/json;charset=utf-8"); + expect(blob.text()).resolves.toBe("{%{}%%}%%"); + }); it("data url (invalid)", async () => { var url = "data:Hello%2C%20World!"; expect(async () => { -- cgit v1.2.3 From 383d5b55d611b62492178eae4833bf3c134de246 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Sat, 16 Sep 2023 21:55:41 -0700 Subject: fix(fetch) handle 100 continue (#5496) * handle 100 continue * move comment * cleanup * fmt --- src/http_client_async.zig | 10 ++++++- test/js/web/fetch/fetch.test.ts | 60 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/http_client_async.zig b/src/http_client_async.zig index 79fe74899..b3c06df56 100644 --- a/src/http_client_async.zig +++ b/src/http_client_async.zig @@ -2637,6 +2637,14 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u this.state.pending_response = response; var body_buf = to_read[@min(@as(usize, @intCast(response.bytes_read)), to_read.len)..]; + // handle the case where we have a 100 Continue + if (response.status_code == 100) { + // we still can have the 200 OK in the same buffer sometimes + if (body_buf.len > 0) { + this.onData(is_ssl, body_buf, ctx, socket); + } + return; + } var deferred_redirect: ?*URLBufferPool.Node = null; const can_continue = this.handleResponseMetadata( @@ -3399,6 +3407,7 @@ pub fn handleResponseMetadata( this.proxy_tunneling = false; } + // if is no redirect or if is redirect == "manual" just proceed const is_redirect = response.status_code >= 300 and response.status_code <= 399; if (is_redirect) { if (this.redirect_type == FetchRedirect.follow and location.len > 0 and this.remaining_redirect_count > 0) { @@ -3513,7 +3522,6 @@ pub fn handleResponseMetadata( } } - // if is no redirect or if is redirect == "manual" just proceed this.state.response_stage = if (this.state.transfer_encoding == .chunked) .body_chunk else .body; const content_length = this.state.content_length orelse 0; // if no body is expected we should stop processing diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 4ef5d7bba..fc4ce7a18 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -5,6 +5,7 @@ import { mkfifo } from "mkfifo"; import { tmpdir } from "os"; import { join } from "path"; import { gc, withoutAggressiveGC, gcTick } from "harness"; +import net from "net"; const tmp_dir = mkdtempSync(join(realpathSync(tmpdir()), "fetch.test")); @@ -1415,3 +1416,62 @@ it("cloned response headers are independent after accessing", () => { cloned.headers.set("content-type", "text/plain"); expect(response.headers.get("content-type")).toBe("text/html; charset=utf-8"); }); + +it("should work with http 100 continue", async () => { + let server: net.Server | undefined; + try { + server = net.createServer(socket => { + socket.on("data", data => { + const lines = data.toString().split("\r\n"); + for (const line of lines) { + if (line.length == 0) { + socket.write("HTTP/1.1 100 Continue\r\n\r\n"); + socket.write("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 13\r\n\r\nHello, World!"); + break; + } + } + }); + }); + + const { promise: start, resolve } = Promise.withResolvers(); + server.listen(8080, resolve); + + await start; + + const address = server.address() as net.AddressInfo; + const result = await fetch(`http://localhost:${address.port}`).then(r => r.text()); + expect(result).toBe("Hello, World!"); + } finally { + server?.close(); + } +}); + +it("should work with http 100 continue on the same buffer", async () => { + let server: net.Server | undefined; + try { + server = net.createServer(socket => { + socket.on("data", data => { + const lines = data.toString().split("\r\n"); + for (const line of lines) { + if (line.length == 0) { + socket.write( + "HTTP/1.1 100 Continue\r\n\r\nHTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 13\r\n\r\nHello, World!", + ); + break; + } + } + }); + }); + + const { promise: start, resolve } = Promise.withResolvers(); + server.listen(8080, resolve); + + await start; + + const address = server.address() as net.AddressInfo; + const result = await fetch(`http://localhost:${address.port}`).then(r => r.text()); + expect(result).toBe("Hello, World!"); + } finally { + server?.close(); + } +}); -- cgit v1.2.3 From 66d490d10954e449d06efd008a01de5c5dc5d078 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 19 Sep 2023 05:51:05 -0700 Subject: Align fetch() redirect behavior with spec (#5729) Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com> --- src/http_client_async.zig | 61 +++++++++++++------- test/js/web/fetch/fetch.test.ts | 121 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 20 deletions(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/http_client_async.zig b/src/http_client_async.zig index a78358066..00f11626f 100644 --- a/src/http_client_async.zig +++ b/src/http_client_async.zig @@ -3407,12 +3407,16 @@ pub fn handleResponseMetadata( this.proxy_tunneling = false; } + const status_code = response.status_code; + // if is no redirect or if is redirect == "manual" just proceed - const is_redirect = response.status_code >= 300 and response.status_code <= 399; + const is_redirect = status_code >= 300 and status_code <= 399; if (is_redirect) { if (this.redirect_type == FetchRedirect.follow and location.len > 0 and this.remaining_redirect_count > 0) { - switch (response.status_code) { + switch (status_code) { 302, 301, 307, 308, 303 => { + var is_same_origin = true; + if (strings.indexOf(location, "://")) |i| { var url_buf = URLBufferPool.get(default_allocator); @@ -3443,7 +3447,9 @@ pub fn handleResponseMetadata( bun.copy(u8, &url_buf.data, location); } - this.url = URL.parse(url_buf.data[0..url_buf_len]); + const new_url = URL.parse(url_buf.data[0..url_buf_len]); + is_same_origin = strings.eqlCaseInsensitiveASCII(strings.withoutTrailingSlash(new_url.origin), strings.withoutTrailingSlash(this.url.origin), true); + this.url = new_url; this.redirect = url_buf; } else if (strings.hasPrefixComptime(location, "//")) { var url_buf = URLBufferPool.get(default_allocator); @@ -3467,7 +3473,9 @@ pub fn handleResponseMetadata( url_buf_len += "https:".len; } - this.url = URL.parse(url_buf.data[0..url_buf_len]); + const new_url = URL.parse(url_buf.data[0..url_buf_len]); + is_same_origin = strings.eqlCaseInsensitiveASCII(strings.withoutTrailingSlash(new_url.origin), strings.withoutTrailingSlash(this.url.origin), true); + this.url = new_url; this.redirect = url_buf; } else { var url_buf = URLBufferPool.get(default_allocator); @@ -3488,28 +3496,41 @@ pub fn handleResponseMetadata( ) catch return error.RedirectURLTooLong); } + is_same_origin = strings.eqlCaseInsensitiveASCII(strings.withoutTrailingSlash(this.url.origin), strings.withoutTrailingSlash(original_url.origin), true); deferred_redirect.* = this.redirect; this.redirect = url_buf; } - // Note: RFC 1945 and RFC 2068 specify that the client is not allowed to change - // the method on the redirected request. However, most existing user agent - // implementations treat 302 as if it were a 303 response, performing a GET on - // the Location field-value regardless of the original request method. The - // status codes 303 and 307 have been added for servers that wish to make - // unambiguously clear which kind of reaction is expected of the client. - if (response.status_code == 302) { - switch (this.method) { - .GET, .HEAD => {}, - else => { - this.method = .GET; - }, - } + // If one of the following is true + // - internalResponse’s status is 301 or 302 and request’s method is `POST` + // - internalResponse’s status is 303 and request’s method is not `GET` or `HEAD` + // then: + if (((status_code == 301 or status_code == 302) and this.method == .POST) or + (status_code == 303 and this.method != .GET and this.method != .HEAD)) + { + // - Set request’s method to `GET` and request’s body to null. + this.method = .GET; + // - For each headerName of request-body-header name, delete headerName from request’s header list. + this.header_entries.len = 0; } - // https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303 - if (response.status_code == 303 and this.method != .HEAD) { - this.method = .GET; + // https://fetch.spec.whatwg.org/#concept-http-redirect-fetch + // If request’s current URL’s origin is not same origin with + // locationURL’s origin, then for each headerName of CORS + // non-wildcard request-header name, delete headerName from + // request’s header list. + if (!is_same_origin and this.header_entries.len > 0) { + const authorization_header_hash = comptime hashHeaderConst("Authorization"); + for (this.header_entries.items(.name), 0..) |name_ptr, i| { + const name = this.headerStr(name_ptr); + if (name.len == "Authorization".len) { + const hash = hashHeaderName(name); + if (hash == authorization_header_hash) { + this.header_entries.swapRemove(i); + break; + } + } + } } return error.Redirect; diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index fc4ce7a18..23e78ac48 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -1475,3 +1475,124 @@ it("should work with http 100 continue on the same buffer", async () => { server?.close(); } }); + +describe("should strip headers", () => { + it("status code 303", async () => { + const server = Bun.serve({ + port: 0, + async fetch(request: Request) { + if (request.url.endsWith("/redirect")) { + return new Response("hello", { + headers: { + ...request.headers, + "Location": "/redirected", + }, + status: 303, + }); + } + + return new Response("hello", { + headers: request.headers, + }); + }, + }); + + const { headers, url, redirected } = await fetch(`http://${server.hostname}:${server.port}/redirect`, { + method: "POST", + headers: { + "I-Am-Here": "yes", + }, + }); + + expect(headers.get("I-Am-Here")).toBeNull(); + expect(url).toEndWith("/redirected"); + expect(redirected).toBe(true); + server.stop(true); + }); + + it("cross-origin status code 302", async () => { + const server1 = Bun.serve({ + port: 0, + async fetch(request: Request) { + if (request.url.endsWith("/redirect")) { + return new Response("hello", { + headers: { + ...request.headers, + "Location": `http://${server2.hostname}:${server2.port}/redirected`, + }, + status: 302, + }); + } + + return new Response("hello", { + headers: request.headers, + }); + }, + }); + + const server2 = Bun.serve({ + port: 0, + async fetch(request: Request, server) { + if (request.url.endsWith("/redirect")) { + return new Response("hello", { + headers: { + ...request.headers, + "Location": `http://${server.hostname}:${server.port}/redirected`, + }, + status: 302, + }); + } + + return new Response("hello", { + headers: request.headers, + }); + }, + }); + + const { headers, url, redirected } = await fetch(`http://${server1.hostname}:${server1.port}/redirect`, { + method: "GET", + headers: { + "Authorization": "yes", + }, + }); + + expect(headers.get("Authorization")).toBeNull(); + expect(url).toEndWith("/redirected"); + expect(redirected).toBe(true); + server1.stop(true); + server2.stop(true); + }); +}); + +it("same-origin status code 302 should not strip headers", async () => { + const server = Bun.serve({ + port: 0, + async fetch(request: Request, server) { + if (request.url.endsWith("/redirect")) { + return new Response("hello", { + headers: { + ...request.headers, + "Location": `http://${server.hostname}:${server.port}/redirected`, + }, + status: 302, + }); + } + + return new Response("hello", { + headers: request.headers, + }); + }, + }); + + const { headers, url, redirected } = await fetch(`http://${server.hostname}:${server.port}/redirect`, { + method: "GET", + headers: { + "Authorization": "yes", + }, + }); + + expect(headers.get("Authorization")).toEqual("yes"); + expect(url).toEndWith("/redirected"); + expect(redirected).toBe(true); + server.stop(true); +}); -- cgit v1.2.3 From b00588e98cc4ab5254dca1c6e69829de6e1fd993 Mon Sep 17 00:00:00 2001 From: Ai Hoshino Date: Thu, 21 Sep 2023 14:34:00 +0800 Subject: fix(fetch): fix redirect in relative path location. (#5781) * fix(fetch): fix redirect in relative path location. * fix utf-8 encoding * use server.reload * check buf size * add RedirectURLTooLong test --- src/http_client_async.zig | 26 ++++++------ test/js/web/fetch/fetch.test.ts | 94 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 14 deletions(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/http_client_async.zig b/src/http_client_async.zig index 89cedaa1b..3cc044003 100644 --- a/src/http_client_async.zig +++ b/src/http_client_async.zig @@ -3479,22 +3479,20 @@ pub fn handleResponseMetadata( this.redirect = url_buf; } else { var url_buf = URLBufferPool.get(default_allocator); + var fba = std.heap.FixedBufferAllocator.init(&url_buf.data); const original_url = this.url; - const port = original_url.getPortAuto(); - - if (port == original_url.getDefaultPort()) { - this.url = URL.parse(std.fmt.bufPrint( - &url_buf.data, - "{s}://{s}{s}", - .{ original_url.displayProtocol(), original_url.displayHostname(), location }, - ) catch return error.RedirectURLTooLong); - } else { - this.url = URL.parse(std.fmt.bufPrint( - &url_buf.data, - "{s}://{s}:{d}{s}", - .{ original_url.displayProtocol(), original_url.displayHostname(), port, location }, - ) catch return error.RedirectURLTooLong); + + const new_url_ = bun.JSC.URL.join( + bun.String.fromUTF8(original_url.href), + bun.String.fromUTF8(location), + ); + defer new_url_.deref(); + + if (new_url_.utf8ByteLength() > url_buf.data.len) { + return error.RedirectURLTooLong; } + const new_url = new_url_.toUTF8(fba.allocator()); + this.url = URL.parse(new_url.slice()); is_same_origin = strings.eqlCaseInsensitiveASCII(strings.withoutTrailingSlash(this.url.origin), strings.withoutTrailingSlash(original_url.origin), true); deferred_redirect.* = this.redirect; diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 23e78ac48..100f6e79e 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -1596,3 +1596,97 @@ it("same-origin status code 302 should not strip headers", async () => { expect(redirected).toBe(true); server.stop(true); }); + +describe("should handle relative location in the redirect, issue#5635", () => { + var server: Server; + beforeAll(async () => { + server = Bun.serve({ + port: 0, + async fetch(request: Request) { + return new Response("Not Found", { + status: 404, + }); + }, + }); + }); + afterAll(() => { + server.stop(true); + }); + + it.each([ + ["/a/b", "/c", "/c"], + ["/a/b", "c", "/a/c"], + ["/a/b", "/c/d", "/c/d"], + ["/a/b", "c/d", "/a/c/d"], + ["/a/b", "../c", "/c"], + ["/a/b", "../c/d", "/c/d"], + ["/a/b", "../../../c", "/c"], + // slash + ["/a/b/", "/c", "/c"], + ["/a/b/", "c", "/a/b/c"], + ["/a/b/", "/c/d", "/c/d"], + ["/a/b/", "c/d", "/a/b/c/d"], + ["/a/b/", "../c", "/a/c"], + ["/a/b/", "../c/d", "/a/c/d"], + ["/a/b/", "../../../c", "/c"], + ])("('%s', '%s')", async (pathname, location, expected) => { + server.reload({ + async fetch(request: Request) { + const url = new URL(request.url); + if (url.pathname == pathname) { + return new Response("redirecting", { + headers: { + "Location": location, + }, + status: 302, + }); + } else if (url.pathname == expected) { + return new Response("Fine."); + } + return new Response("Not Found", { + status: 404, + }); + }, + }); + + const resp = await fetch(`http://${server.hostname}:${server.port}${pathname}`); + expect(resp.redirected).toBe(true); + expect(new URL(resp.url).pathname).toStrictEqual(expected); + expect(resp.status).toBe(200); + expect(await resp.text()).toBe("Fine."); + }); +}); + +it("should throw RedirectURLTooLong when location is too long", async () => { + const server = Bun.serve({ + port: 0, + async fetch(request: Request) { + gc(); + const url = new URL(request.url); + if (url.pathname == "/redirect") { + return new Response("redirecting", { + headers: { + "Location": "B".repeat(8193), + }, + status: 302, + }); + } + return new Response("Not Found", { + status: 404, + }); + }, + }); + + let err = undefined; + try { + gc(); + const resp = await fetch(`http://${server.hostname}:${server.port}/redirect`); + } catch (error) { + gc(); + err = error; + } + expect(err).not.toBeUndefined(); + expect(err).toBeInstanceOf(Error); + expect(err.code).toStrictEqual("RedirectURLTooLong"); + server.stop(true); +}); -- cgit v1.2.3 From 6514dcf4cbc63cff89f3cac59598872caf776f0b Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 29 Sep 2023 00:03:58 -0700 Subject: Fixes #6053 (#6162) Fixes #6053 Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com> --- src/http_client_async.zig | 48 ++++++++++++++++++++++++++++++++++++++--- test/js/web/fetch/fetch.test.ts | 4 +++- 2 files changed, 48 insertions(+), 4 deletions(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/http_client_async.zig b/src/http_client_async.zig index a8ba8d367..15e29f345 100644 --- a/src/http_client_async.zig +++ b/src/http_client_async.zig @@ -3541,8 +3541,50 @@ pub fn handleResponseMetadata( { // - Set request’s method to `GET` and request’s body to null. this.method = .GET; - // - For each headerName of request-body-header name, delete headerName from request’s header list. - this.header_entries.len = 0; + + // https://github.com/oven-sh/bun/issues/6053 + if (this.header_entries.len > 0) { + // A request-body-header name is a header name that is a byte-case-insensitive match for one of: + // - `Content-Encoding` + // - `Content-Language` + // - `Content-Location` + // - `Content-Type` + const @"request-body-header" = &.{ + "Content-Encoding", + "Content-Language", + "Content-Location", + }; + var i: usize = 0; + + // - For each headerName of request-body-header name, delete headerName from request’s header list. + const names = this.header_entries.items(.name); + var len = names.len; + outer: while (i < len) { + const name = this.headerStr(names[i]); + switch (name.len) { + "Content-Type".len => { + const hash = hashHeaderName(name); + if (hash == comptime hashHeaderConst("Content-Type")) { + _ = this.header_entries.orderedRemove(i); + len = this.header_entries.len; + continue :outer; + } + }, + "Content-Encoding".len => { + const hash = hashHeaderName(name); + inline for (@"request-body-header") |hash_value| { + if (hash == comptime hashHeaderConst(hash_value)) { + _ = this.header_entries.orderedRemove(i); + len = this.header_entries.len; + continue :outer; + } + } + }, + else => {}, + } + i += 1; + } + } } // https://fetch.spec.whatwg.org/#concept-http-redirect-fetch @@ -3557,7 +3599,7 @@ pub fn handleResponseMetadata( if (name.len == "Authorization".len) { const hash = hashHeaderName(name); if (hash == authorization_header_hash) { - this.header_entries.swapRemove(i); + this.header_entries.orderedRemove(i); break; } } diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 100f6e79e..85e2296cd 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -1501,10 +1501,12 @@ describe("should strip headers", () => { method: "POST", headers: { "I-Am-Here": "yes", + "Content-Language": "This should be stripped", }, }); - expect(headers.get("I-Am-Here")).toBeNull(); + expect(headers.get("I-Am-Here")).toBe("yes"); + expect(headers.get("Content-Language")).toBeNull(); expect(url).toEndWith("/redirected"); expect(redirected).toBe(true); server.stop(true); -- cgit v1.2.3 From fa7d7bd1e4a701d1f5d3ec89f287f30a2dd0babb Mon Sep 17 00:00:00 2001 From: Liz Date: Sat, 30 Sep 2023 01:13:49 +0200 Subject: fix: don't set default request method when creating a Request (#6154) In the case of creating a Request with the parameters `(Request, object)`, there was a bug that method and headers are set from the default created by the init rather then the already present value from the request param. This is because for a to me unknown reason the order in which the parameters are processed is reversed. This fixes that by adding a check which stops the defaults from being set, unless they are explicitly passed. Fixes: https://github.com/oven-sh/bun/issues/6144 --- src/bun.js/webcore/request.zig | 27 ++++++++++++++------------- test/js/web/fetch/fetch.test.ts | 10 ++++++++++ 2 files changed, 24 insertions(+), 13 deletions(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/bun.js/webcore/request.zig b/src/bun.js/webcore/request.zig index 381ae2750..7f37bea46 100644 --- a/src/bun.js/webcore/request.zig +++ b/src/bun.js/webcore/request.zig @@ -507,10 +507,9 @@ pub const Request = struct { }; const values_to_try = values_to_try_[0 .. @as(usize, @intFromBool(!is_first_argument_a_url)) + @as(usize, @intFromBool(arguments.len > 1 and arguments[1].isObject()))]; - for (values_to_try) |value| { const value_type = value.jsType(); - + const explicit_check = values_to_try.len == 2 and value_type == .FinalObject and values_to_try[1].jsType() == .DOMWrapper; if (value_type == .DOMWrapper) { if (value.as(Request)) |request| { if (values_to_try.len == 1) { @@ -625,23 +624,25 @@ pub const Request = struct { if (!fields.contains(.method) or !fields.contains(.headers)) { if (Body.Init.init(globalThis.allocator(), globalThis, value) catch null) |init| { - if (!fields.contains(.method)) { - req.method = init.method; - fields.insert(.method); + if (!explicit_check or (explicit_check and value.fastGet(globalThis, .method) != null)) { + if (!fields.contains(.method)) { + req.method = init.method; + fields.insert(.method); + } } - - if (init.headers) |headers| { - if (!fields.contains(.headers)) { - req.headers = headers; - fields.insert(.headers); - } else { - headers.deref(); + if (!explicit_check or (explicit_check and value.fastGet(globalThis, .headers) != null)) { + if (init.headers) |headers| { + if (!fields.contains(.headers)) { + req.headers = headers; + fields.insert(.headers); + } else { + headers.deref(); + } } } } } } - if (req.url.isEmpty()) { globalThis.throw("Failed to construct 'Request': url is required.", .{}); req.finalizeWithoutDeinit(); diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 85e2296cd..4f7b20d3c 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -1241,6 +1241,16 @@ describe("Request", () => { expect(req.signal.aborted).toBe(true); }); + it("copies method (#6144)", () => { + const request = new Request("http://localhost:1337/test", { + method: "POST", + }); + const new_req = new Request(request, { + body: JSON.stringify({ message: "Hello world" }), + }); + expect(new_req.method).toBe("POST"); + }); + it("cloned signal", async () => { gc(); const controller = new AbortController(); -- cgit v1.2.3 From d65fdb6035d2b403e892905a0c296f09193f8343 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 29 Sep 2023 20:45:51 -0700 Subject: Fix hang in `bun install` (#6192) Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com> --- src/bun.zig | 32 ++++++++++++++ src/http_client_async.zig | 51 ++++++++++++++++------- src/install/install.zig | 34 ++++++++++++--- src/libarchive/libarchive.zig | 4 +- src/sys.zig | 13 ++++++ test/js/web/fetch/fetch.test.ts | 92 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 204 insertions(+), 22 deletions(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/bun.zig b/src/bun.zig index 3f09277bb..a93b3c5ae 100644 --- a/src/bun.zig +++ b/src/bun.zig @@ -1906,3 +1906,35 @@ pub inline fn serializableInto(comptime T: type, init: anytype) T { return result.*; } + +/// Like std.fs.Dir.makePath except instead of infinite looping on dangling +/// symlink, it deletes the symlink and tries again. +pub fn makePath(dir: std.fs.Dir, sub_path: []const u8) !void { + var it = try std.fs.path.componentIterator(sub_path); + var component = it.last() orelse return; + while (true) { + dir.makeDir(component.path) catch |err| switch (err) { + error.PathAlreadyExists => { + var path_buf2: [MAX_PATH_BYTES * 2]u8 = undefined; + copy(u8, &path_buf2, component.path); + + path_buf2[component.path.len] = 0; + var path_to_use = path_buf2[0..component.path.len :0]; + const result = sys.lstat(path_to_use); + try result.throw(); + const is_dir = std.os.S.ISDIR(result.result.mode); + // dangling symlink + if (!is_dir) { + dir.deleteTree(component.path) catch {}; + continue; + } + }, + error.FileNotFound => |e| { + component = it.previous() orelse return e; + continue; + }, + else => |e| return e, + }; + component = it.next() orelse return; + } +} diff --git a/src/http_client_async.zig b/src/http_client_async.zig index 15e29f345..65ffbee62 100644 --- a/src/http_client_async.zig +++ b/src/http_client_async.zig @@ -68,6 +68,10 @@ pub const Signals = struct { aborted: ?*std.atomic.Atomic(bool) = null, cert_errors: ?*std.atomic.Atomic(bool) = null, + pub fn isEmpty(this: *const Signals) bool { + return this.aborted == null and this.body_streaming == null and this.header_progress == null and this.cert_errors == null; + } + pub const Store = struct { header_progress: std.atomic.Atomic(bool) = std.atomic.Atomic(bool).init(false), body_streaming: std.atomic.Atomic(bool) = std.atomic.Atomic(bool).init(false), @@ -1339,7 +1343,7 @@ pub const InternalState = struct { return this.transfer_encoding == Encoding.chunked; } - pub fn reset(this: *InternalState, allocator: std.mem.Allocator) void { + pub fn reset(this: *InternalState, buffering: bool, allocator: std.mem.Allocator) void { this.compressed_body.deinit(); this.response_message_buffer.deinit(); @@ -1350,9 +1354,12 @@ pub const InternalState = struct { reader.deinit(); } - // if we are holding a cloned_metadata we need to deinit it - // this should never happen because we should always return the metadata to the user - std.debug.assert(this.cloned_metadata == null); + if (!buffering) { + // if we are holding a cloned_metadata we need to deinit it + // this should never happen because we should always return the metadata to the user + std.debug.assert(this.cloned_metadata == null); + } + // just in case we check and free to avoid leaks if (this.cloned_metadata != null) { this.cloned_metadata.?.deinit(allocator); @@ -2179,7 +2186,7 @@ pub fn doRedirect(this: *HTTPClient) void { this.fail(error.TooManyRedirects); return; } - this.state.reset(this.allocator); + this.state.reset(this.signals.isEmpty(), this.allocator); // also reset proxy to redirect this.proxy_tunneling = false; if (this.proxy_tunnel != null) { @@ -2673,7 +2680,7 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u } var deferred_redirect: ?*URLBufferPool.Node = null; - const can_continue = this.handleResponseMetadata( + const should_continue = this.handleResponseMetadata( &response, // If there are multiple consecutive redirects // and the redirect differs in hostname @@ -2720,8 +2727,7 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u this.state.pending_response = response; } - if (!can_continue) { - log("onData: can_continue is false", .{}); + if (should_continue == .finished) { // this means that the request ended // clone metadata and return the progress at this point this.cloneMetadata(); @@ -2901,7 +2907,7 @@ fn fail(this: *HTTPClient, err: anyerror) void { _ = socket_async_http_abort_tracker.swapRemove(this.async_http_id); } - this.state.reset(this.allocator); + this.state.reset(this.signals.isEmpty(), this.allocator); this.proxy_tunneling = false; this.state.request_stage = .fail; @@ -2987,7 +2993,7 @@ pub fn progressUpdate(this: *HTTPClient, comptime is_ssl: bool, ctx: *NewHTTPCon socket.close(0, null); } - this.state.reset(this.allocator); + this.state.reset(this.signals.isEmpty(), this.allocator); this.state.response_stage = .done; this.state.request_stage = .done; this.state.stage = .done; @@ -3355,13 +3361,19 @@ fn handleResponseBodyChunkedEncodingFromSinglePacket( unreachable; } +const ShouldContinue = enum { + continue_streaming, + finished, +}; + pub fn handleResponseMetadata( this: *HTTPClient, response: *picohttp.Response, deferred_redirect: *?*URLBufferPool.Node, -) !bool { +) !ShouldContinue { var location: string = ""; var pretend_304 = false; + var is_server_sent_events = false; for (response.headers, 0..) |header, header_i| { switch (hashHeaderName(header.name)) { hashHeaderConst("Content-Length") => { @@ -3373,6 +3385,11 @@ pub fn handleResponseMetadata( this.state.content_length = 0; } }, + hashHeaderConst("Content-Type") => { + if (strings.contains(header.value, "text/event-stream")) { + is_server_sent_events = true; + } + }, hashHeaderConst("Content-Encoding") => { if (!this.disable_decompression) { if (strings.eqlComptime(header.value, "gzip")) { @@ -3429,8 +3446,8 @@ pub fn handleResponseMetadata( if (this.proxy_tunneling and this.proxy_tunnel == null) { if (response.status_code == 200) { - //signal to continue the proxing - return true; + // signal to continue the proxing + return ShouldContinue.finished; } //proxy denied connection so return proxy result (407, 403 etc) @@ -3623,6 +3640,10 @@ pub fn handleResponseMetadata( } else { log("handleResponseMetadata: content_length is null and transfer_encoding {}", .{this.state.transfer_encoding}); } - // if no body is expected we should stop processing - return this.method.hasBody() and (content_length == null or content_length.? > 0 or this.state.transfer_encoding == .chunked); + + if (this.method.hasBody() and ((content_length != null and content_length.? > 0) or !this.state.allow_keepalive or this.state.transfer_encoding == .chunked or is_server_sent_events)) { + return ShouldContinue.continue_streaming; + } else { + return ShouldContinue.finished; + } } diff --git a/src/install/install.zig b/src/install/install.zig index 131adb00e..6689b05ac 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -1764,7 +1764,7 @@ pub const PackageManager = struct { pub fn sleep(this: *PackageManager) void { if (this.wait_count.swap(0, .Monotonic) > 0) return; - bun.Mimalloc.mi_collect(false); + Output.flush(); _ = this.waiter.wait() catch 0; } @@ -1829,7 +1829,7 @@ pub const PackageManager = struct { switch (this.options.log_level) { inline else => |log_level| { if (log_level.showProgress()) this.startProgressBarIfNone(); - while (this.pending_tasks > 0) : (this.sleep()) { + while (this.pending_tasks > 0) { this.runTasks( void, {}, @@ -1843,6 +1843,13 @@ pub const PackageManager = struct { ) catch |err| { return .{ .failure = err }; }; + + if (PackageManager.verbose_install and this.pending_tasks > 0) { + Output.prettyErrorln("[PackageManager] waiting for {d} tasks\n", .{this.pending_tasks}); + } + + if (this.pending_tasks > 0) + this.sleep(); } }, } @@ -7297,7 +7304,10 @@ pub const PackageManager = struct { // We use this file descriptor to know where to put it. installer.node_modules_folder = cwd.openIterableDir(node_modules.relative_path, .{}) catch brk: { // Avoid extra mkdir() syscall - try cwd.makePath(bun.span(node_modules.relative_path)); + // + // note: this will recursively delete any dangling symlinks + // in the next.js repo, it encounters a dangling symlink in node_modules/@next/codemod/node_modules/cheerio + try bun.makePath(cwd, bun.span(node_modules.relative_path)); break :brk try cwd.openIterableDir(node_modules.relative_path, .{}); }; @@ -7351,7 +7361,7 @@ pub const PackageManager = struct { if (!installer.options.do.install_packages) return error.InstallFailed; } - while (this.pending_tasks > 0 and installer.options.do.install_packages) : (this.sleep()) { + while (this.pending_tasks > 0 and installer.options.do.install_packages) { try this.runTasks( *PackageInstaller, &installer, @@ -7363,6 +7373,13 @@ pub const PackageManager = struct { }, log_level, ); + + if (PackageManager.verbose_install and this.pending_tasks > 0) { + Output.prettyErrorln("[PackageManager] waiting for {d} tasks\n", .{this.pending_tasks}); + } + + if (this.pending_tasks > 0) + this.sleep(); } if (!installer.options.do.install_packages) return error.InstallFailed; @@ -7733,7 +7750,7 @@ pub const PackageManager = struct { Output.flush(); } - while (manager.pending_tasks > 0) : (manager.sleep()) { + while (manager.pending_tasks > 0) { try manager.runTasks( *PackageManager, manager, @@ -7746,6 +7763,13 @@ pub const PackageManager = struct { }, log_level, ); + + if (PackageManager.verbose_install and manager.pending_tasks > 0) { + Output.prettyErrorln("[PackageManager] waiting for {d} tasks\n", .{manager.pending_tasks}); + } + + if (manager.pending_tasks > 0) + manager.sleep(); } if (comptime log_level.showProgress()) { diff --git a/src/libarchive/libarchive.zig b/src/libarchive/libarchive.zig index c6276b1cc..ff514d72d 100644 --- a/src/libarchive/libarchive.zig +++ b/src/libarchive/libarchive.zig @@ -543,13 +543,13 @@ pub const Archive = struct { if (comptime Environment.isWindows) { std.os.mkdirat(dir_fd, pathname, @as(u32, @intCast(mode))) catch |err| { if (err == error.PathAlreadyExists or err == error.NotDir) break; - try dir.makePath(std.fs.path.dirname(slice) orelse return err); + try bun.makePath(dir, std.fs.path.dirname(slice) orelse return err); try std.os.mkdirat(dir_fd, pathname, 0o777); }; } else { std.os.mkdiratZ(dir_fd, pathname, @as(u32, @intCast(mode))) catch |err| { if (err == error.PathAlreadyExists or err == error.NotDir) break; - try dir.makePath(std.fs.path.dirname(slice) orelse return err); + try bun.makePath(dir, std.fs.path.dirname(slice) orelse return err); try std.os.mkdiratZ(dir_fd, pathname, 0o777); }; } diff --git a/src/sys.zig b/src/sys.zig index 14bb9ddf9..b7542b44f 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -819,6 +819,19 @@ pub fn readlink(in: [:0]const u8, buf: []u8) Maybe(usize) { unreachable; } +pub fn readlinkat(fd: bun.FileDescriptor, in: [:0]const u8, buf: []u8) Maybe(usize) { + while (true) { + const rc = sys.readlinkat(fd, in, buf.ptr, buf.len); + + if (Maybe(usize).errnoSys(rc, .readlink)) |err| { + if (err.getErrno() == .INTR) continue; + return err; + } + return Maybe(usize){ .result = @as(usize, @intCast(rc)) }; + } + unreachable; +} + pub fn ftruncate(fd: fd_t, size: isize) Maybe(void) { if (comptime Environment.isWindows) { if (kernel32.SetFileValidData(bun.fdcast(fd), size) == 0) { diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 4f7b20d3c..8c5e27f88 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -1702,3 +1702,95 @@ it("should throw RedirectURLTooLong when location is too long", async () => { expect(err.code).toStrictEqual("RedirectURLTooLong"); server.stop(true); }); + +it("304 not modified with missing content-length does not cause a request timeout", async () => { + const server = await Bun.listen({ + socket: { + open(socket) { + socket.write("HTTP/1.1 304 Not Modified\r\n\r\n"); + socket.flush(); + setTimeout(() => { + socket.end(); + }, 9999).unref(); + }, + data() {}, + close() {}, + }, + port: 0, + hostname: "localhost", + }); + + const response = await fetch(`http://${server.hostname}:${server.port}/`); + expect(response.status).toBe(304); + expect(await response.arrayBuffer()).toHaveLength(0); + server.stop(true); +}); + +it("304 not modified with missing content-length and connection close does not cause a request timeout", async () => { + const server = await Bun.listen({ + socket: { + open(socket) { + socket.write("HTTP/1.1 304 Not Modified\r\nConnection: close\r\n\r\n"); + socket.flush(); + setTimeout(() => { + socket.end(); + }, 9999).unref(); + }, + data() {}, + close() {}, + }, + port: 0, + hostname: "localhost", + }); + + const response = await fetch(`http://${server.hostname}:${server.port}/`); + expect(response.status).toBe(304); + expect(await response.arrayBuffer()).toHaveLength(0); + server.stop(true); +}); + +it("304 not modified with content-length 0 and connection close does not cause a request timeout", async () => { + const server = await Bun.listen({ + socket: { + open(socket) { + socket.write("HTTP/1.1 304 Not Modified\r\nConnection: close\r\nContent-Length: 0\r\n\r\n"); + socket.flush(); + setTimeout(() => { + socket.end(); + }, 9999).unref(); + }, + data() {}, + close() {}, + }, + port: 0, + hostname: "localhost", + }); + + const response = await fetch(`http://${server.hostname}:${server.port}/`); + expect(response.status).toBe(304); + expect(await response.arrayBuffer()).toHaveLength(0); + server.stop(true); +}); + +it("304 not modified with 0 content-length does not cause a request timeout", async () => { + const server = await Bun.listen({ + socket: { + open(socket) { + socket.write("HTTP/1.1 304 Not Modified\r\nContent-Length: 0\r\n\r\n"); + socket.flush(); + setTimeout(() => { + socket.end(); + }, 9999).unref(); + }, + data() {}, + close() {}, + }, + port: 0, + hostname: "localhost", + }); + + const response = await fetch(`http://${server.hostname}:${server.port}/`); + expect(response.status).toBe(304); + expect(await response.arrayBuffer()).toHaveLength(0); + server.stop(true); +}); -- cgit v1.2.3 From a9b8e3ecc82b01237721ec38afd780407c078d96 Mon Sep 17 00:00:00 2001 From: Liz Date: Tue, 17 Oct 2023 01:11:44 +0200 Subject: fix: don't remove content-encoding header from header table (#5743) Closes #5668 --- src/http_client_async.zig | 5 +++-- test/js/web/fetch/fetch.test.ts | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) (limited to 'test/js/web/fetch/fetch.test.ts') diff --git a/src/http_client_async.zig b/src/http_client_async.zig index a8206e17d..fac496620 100644 --- a/src/http_client_async.zig +++ b/src/http_client_async.zig @@ -1309,6 +1309,7 @@ pub const InternalState = struct { allow_keepalive: bool = true, received_last_chunk: bool = false, + did_set_content_encoding: bool = false, transfer_encoding: Encoding = Encoding.identity, encoding: Encoding = Encoding.identity, content_encoding_i: u8 = std.math.maxInt(u8), @@ -2710,10 +2711,10 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u return; }; - if (this.state.content_encoding_i < response.headers.len) { + if (this.state.content_encoding_i < response.headers.len and !this.state.did_set_content_encoding) { // if it compressed with this header, it is no longer because we will decompress it var mutable_headers = std.ArrayListUnmanaged(picohttp.Header){ .items = response.headers, .capacity = response.headers.len }; - _ = mutable_headers.orderedRemove(this.state.content_encoding_i); + this.state.did_set_content_encoding = true; response.headers = mutable_headers.items; this.state.content_encoding_i = std.math.maxInt(@TypeOf(this.state.content_encoding_i)); // we need to reset the pending response because we removed a header diff --git a/test/js/web/fetch/fetch.test.ts b/test/js/web/fetch/fetch.test.ts index 8c5e27f88..3ed20345b 100644 --- a/test/js/web/fetch/fetch.test.ts +++ b/test/js/web/fetch/fetch.test.ts @@ -3,6 +3,7 @@ import { afterAll, afterEach, beforeAll, describe, expect, it, beforeEach } from import { chmodSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "fs"; import { mkfifo } from "mkfifo"; import { tmpdir } from "os"; +import { gzipSync } from "zlib"; import { join } from "path"; import { gc, withoutAggressiveGC, gcTick } from "harness"; import net from "net"; @@ -346,6 +347,27 @@ describe("Headers", () => { expect(headers.getAll("set-cookie")).toEqual(["foo=bar; Path=/; HttpOnly"]); }); + it("presence of content-encoding header(issue #5668)", async () => { + startServer({ + fetch(req) { + const content = gzipSync(JSON.stringify({ message: "Hello world" })); + return new Response(content, { + status: 200, + headers: { + "content-encoding": "gzip", + "content-type": "application/json", + }, + }); + }, + }); + const result = await fetch(`http://${server.hostname}:${server.port}/`); + const value = result.headers.get("content-encoding"); + const body = await result.json(); + expect(value).toBe("gzip"); + expect(body).toBeDefined(); + expect(body.message).toBe("Hello world"); + }); + it(".getSetCookie() with array", () => { const headers = new Headers([ ["content-length", "123"], -- cgit v1.2.3