Skip to content

fix(fetch) redirects bodies should be handled #8874

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.

Sign up for GitHub

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

Merged
Jarred-Sumner merged 3 commits into from
Feb 13, 2024
Merged

fix(fetch) redirects bodies should be handled #8874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4687d18
redirects bodies should be handled
cirospaciari Feb 13, 2024
2ada637
[autofix.ci] apply automated fixes
autofix-ci[bot] Feb 13, 2024
b7968b7
Update src/http.zig
cirospaciari Feb 13, 2024
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
138 changes: 83 additions & 55 deletions src/http.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,6 +1342,7 @@ pub const InternalState = struct {
allow_keepalive: bool = true,
received_last_chunk: bool = false,
did_set_content_encoding: bool = false,
is_redirect_pending: bool = false,
transfer_encoding: Encoding = Encoding.identity,
encoding: Encoding = Encoding.identity,
content_encoding_i: u8 = std.math.maxInt(u8),
Expand Down Expand Up @@ -1404,6 +1405,7 @@ pub const InternalState = struct {
.original_request_body = .{ .bytes = "" },
.request_body = "",
.certificate_info = null,
.is_redirect_pending = false,
};
}

Expand Down Expand Up @@ -1454,7 +1456,9 @@ pub const InternalState = struct {
try this.decompressBytes(buffer.list.items, body_out_str);
}

pub fn processBodyBuffer(this: *InternalState, buffer: MutableString) !usize {
pub fn processBodyBuffer(this: *InternalState, buffer: MutableString) !bool {
if (this.is_redirect_pending) return false;

var body_out_str = this.body_out_str.?;

switch (this.encoding) {
Expand All @@ -1472,7 +1476,7 @@ pub const InternalState = struct {
},
}

return this.body_out_str.?.list.items.len;
return this.body_out_str.?.list.items.len > 0;
}
};

Expand Down Expand Up @@ -2147,8 +2151,25 @@ pub fn buildRequest(this: *HTTPClient, body_len: usize) picohttp.Request {
};
}

pub fn doRedirect(this: *HTTPClient) void {
std.debug.assert(this.state.cloned_metadata == null);
pub fn doRedirect(this: *HTTPClient, comptime is_ssl: bool, ctx: *NewHTTPContext(is_ssl), socket: NewHTTPContext(is_ssl).HTTPSocket) void {
this.state.response_message_buffer.deinit();
// we need to clean the client reference before closing the socket because we are going to reuse the same ref in a another request
socket.ext(**anyopaque).?.* = bun.cast(
**anyopaque,
NewHTTPContext(is_ssl).ActiveSocket.init(&dead_socket).ptr(),
);
if (this.isKeepAlivePossible()) {
std.debug.assert(this.connected_url.hostname.len > 0);
ctx.releaseSocket(
socket,
this.connected_url.hostname,
this.connected_url.getPortAuto(),
);
} else {
socket.close(0, null);
}

this.connected_url = URL{};
const body_out_str = this.state.body_out_str.?;
this.remaining_redirect_count -|= 1;
std.debug.assert(this.redirect_type == FetchRedirect.follow);
Expand Down Expand Up @@ -2595,8 +2616,27 @@ fn startProxyHandshake(this: *HTTPClient, comptime is_ssl: bool, socket: NewHTTP
this.startProxySendHeaders(is_ssl, socket);
}

inline fn handleShortRead(
this: *HTTPClient,
comptime is_ssl: bool,
incoming_data: []const u8,
socket: NewHTTPContext(is_ssl).HTTPSocket,
needs_move: bool,
) void {
if (needs_move) {
const to_copy = incoming_data;

if (to_copy.len > 0) {
// this one will probably be another chunk, so we leave a little extra room
this.state.response_message_buffer.append(to_copy) catch bun.outOfMemory();
}
}

this.setTimeout(socket, 5);
}
pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u8, ctx: *NewHTTPContext(is_ssl), socket: NewHTTPContext(is_ssl).HTTPSocket) void {
log("onData {}", .{incoming_data.len});

if (this.signals.get(.aborted)) {
this.closeAndAbort(is_ssl, socket);
return;
Expand All @@ -2616,23 +2656,21 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u
// we reset the pending_response each time wich means that on parse error this will be always be empty
this.state.pending_response = picohttp.Response{};

// minimal http/1.1 request size is 16 bytes without headers and 26 with Host header
// if is less than 16 will always be a ShortRead
if (to_read.len < 16) {
this.handleShortRead(is_ssl, incoming_data, socket, needs_move);
return;
}

var response = picohttp.Response.parseParts(
to_read,
&shared_response_headers_buf,
&amount_read,
) catch |err| {
switch (err) {
error.ShortRead => {
if (needs_move) {
const to_copy = incoming_data;

if (to_copy.len > 0) {
// this one will probably be another chunk, so we leave a little extra room
this.state.response_message_buffer.append(to_copy) catch @panic("Out of memory");
}
}

this.setTimeout(socket, 5);
this.handleShortRead(is_ssl, incoming_data, socket, needs_move);
},
else => {
this.closeAndFail(err, is_ssl, socket);
Expand All @@ -2657,29 +2695,6 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u
const should_continue = this.handleResponseMetadata(
&response,
) catch |err| {
if (err == error.Redirect) {
this.state.response_message_buffer.deinit();
// we need to clean the client reference before closing the socket because we are going to reuse the same ref in a another request
socket.ext(**anyopaque).?.* = bun.cast(
**anyopaque,
NewHTTPContext(is_ssl).ActiveSocket.init(&dead_socket).ptr(),
);
if (this.state.allow_keepalive and FeatureFlags.enable_keepalive) {
std.debug.assert(this.connected_url.hostname.len > 0);
ctx.releaseSocket(
socket,
this.connected_url.hostname,
this.connected_url.getPortAuto(),
);
} else {
socket.close(0, null);
}

this.connected_url = URL{};
this.doRedirect();
return;
}

this.closeAndFail(err, is_ssl, socket);
return;
};
Expand All @@ -2695,6 +2710,10 @@ pub fn onData(this: *HTTPClient, comptime is_ssl: bool, incoming_data: []const u
}

if (should_continue == .finished) {
if (this.state.is_redirect_pending) {
this.doRedirect(is_ssl, ctx, socket);
return;
}
// this means that the request ended
// clone metadata and return the progress at this point
this.cloneMetadata();
Expand Down Expand Up @@ -2937,7 +2956,12 @@ pub fn progressUpdate(this: *HTTPClient, comptime is_ssl: bool, ctx: *NewHTTPCon
const body = out_str.*;
const result = this.toResult();
const is_done = !result.has_more;

if (this.state.is_redirect_pending and this.state.fail == error.NoError) {
if (this.state.isDone()) {
this.doRedirect(is_ssl, ctx, socket);
}
return;
}
if (this.signals.aborted != null and is_done) {
_ = socket_async_http_abort_tracker.swapRemove(this.async_http_id);
}
Expand Down Expand Up @@ -3102,6 +3126,15 @@ fn handleResponseBodyFromSinglePacket(this: *HTTPClient, incoming_data: []const
if (!this.state.isChunkedEncoding()) {
this.state.total_body_received += incoming_data.len;
}
defer {
if (this.progress_node) |progress| {
progress.activate();
progress.setCompletedItems(incoming_data.len);
progress.context.maybeRefresh();
}
}
// we can ignore the body data in redirects
if (this.state.is_redirect_pending) return;

if (this.state.encoding.isCompressed()) {
var body_buffer = this.state.body_out_str.?;
Expand All @@ -3125,22 +3158,12 @@ fn handleResponseBodyFromSinglePacket(this: *HTTPClient, incoming_data: []const

this.state.response_message_buffer.deinit();
}

if (this.progress_node) |progress| {
progress.activate();
progress.setCompletedItems(incoming_data.len);
progress.context.maybeRefresh();
}
}

fn handleResponseBodyFromMultiplePackets(this: *HTTPClient, incoming_data: []const u8) !bool {
var buffer = this.state.getBodyBuffer();
const content_length = this.state.content_length;

if (buffer.list.items.len == 0 and incoming_data.len < preallocate_max) {
buffer.list.ensureTotalCapacityPrecise(buffer.allocator, incoming_data.len) catch {};
}

var remainder: []const u8 = undefined;
if (content_length != null) {
const remaining_content_length = content_length.? -| this.state.total_body_received;
Expand All @@ -3149,7 +3172,14 @@ fn handleResponseBodyFromMultiplePackets(this: *HTTPClient, incoming_data: []con
remainder = incoming_data;
}

_ = try buffer.write(remainder);
// we can ignore the body data in redirects
if (!this.state.is_redirect_pending) {
if (buffer.list.items.len == 0 and incoming_data.len < preallocate_max) {
buffer.list.ensureTotalCapacityPrecise(buffer.allocator, incoming_data.len) catch {};
}

_ = try buffer.write(remainder);
}

this.state.total_body_received += remainder.len;

Expand All @@ -3169,7 +3199,7 @@ fn handleResponseBodyFromMultiplePackets(this: *HTTPClient, incoming_data: []con
progress.setCompletedItems(this.state.total_body_received);
progress.context.maybeRefresh();
}
return is_done or processed > 0;
return is_done or processed;
}
return false;
}
Expand Down Expand Up @@ -3227,8 +3257,7 @@ fn handleResponseBodyChunkedEncodingFromMultiplePackets(
}
// streaming chunks
if (this.signals.get(.body_streaming)) {
const processed = try this.state.processBodyBuffer(buffer);
return processed > 0;
return try this.state.processBodyBuffer(buffer);
}

return false;
Expand Down Expand Up @@ -3306,8 +3335,7 @@ fn handleResponseBodyChunkedEncodingFromSinglePacket(

// streaming chunks
if (this.signals.get(.body_streaming)) {
const processed = try this.state.processBodyBuffer(body_buffer.*);
return processed > 0;
return try this.state.processBodyBuffer(body_buffer.*);
}

return false;
Expand Down Expand Up @@ -3655,7 +3683,7 @@ pub fn handleResponseMetadata(
}
}

return error.Redirect;
this.state.is_redirect_pending = true;
},
else => {},
}
Expand Down
11 changes: 6 additions & 5 deletions test/js/web/fetch/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1760,11 +1760,12 @@ it("should allow very long redirect URLS", async () => {
});
},
});

const { url, status } = await fetch(`http://${server.hostname}:${server.port}/redirect`);

expect(url).toBe(`http://${server.hostname}:${server.port}${Location}`);
expect(status).toBe(404);
// run it more times to check Malformed_HTTP_Response errors
for (let i = 0; i < 100; i++) {
const { url, status } = await fetch(`${server.url.origin}/redirect`);
expect(url).toBe(`${server.url.origin}${Location}`);
expect(status).toBe(404);
}
server.stop(true);
});

Expand Down