From 245abb92fba35f4d01e111ff3a35b34940cba79e Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Mon, 28 Jul 2025 23:35:46 -0700 Subject: [PATCH] Cleanup some code from recent PRs (#21451) ### What does this PR do? Remove some duplicate code ### How did you verify your code works? Ran the tests --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- cmake/sources/ZigSources.txt | 1 + src/bun.js/api/server/StaticRoute.zig | 120 +++++++------------------- src/bun.js/webcore/Response.zig | 54 ++++++------ src/http.zig | 1 + src/http/ETag.zig | 65 ++++++++++++++ 5 files changed, 124 insertions(+), 117 deletions(-) create mode 100644 src/http/ETag.zig diff --git a/cmake/sources/ZigSources.txt b/cmake/sources/ZigSources.txt index 6c3d102112..628938a3b4 100644 --- a/cmake/sources/ZigSources.txt +++ b/cmake/sources/ZigSources.txt @@ -546,6 +546,7 @@ src/http/AsyncHTTP.zig src/http/CertificateInfo.zig src/http/Decompressor.zig src/http/Encoding.zig +src/http/ETag.zig src/http/FetchRedirect.zig src/http/HeaderBuilder.zig src/http/Headers.zig diff --git a/src/bun.js/api/server/StaticRoute.zig b/src/bun.js/api/server/StaticRoute.zig index ebc6cb0a08..0e504d0ece 100644 --- a/src/bun.js/api/server/StaticRoute.zig +++ b/src/bun.js/api/server/StaticRoute.zig @@ -37,12 +37,9 @@ pub fn initFromAnyBlob(blob: *const AnyBlob, options: InitFromBytesOptions) *Sta // Generate ETag if not already present if (headers.get("etag") == null) { - const slice = blob.slice(); - const hash = std.hash.XxHash64.hash(0, slice); - - var etag_buf: [32]u8 = undefined; - const etag_str = std.fmt.bufPrint(&etag_buf, "\"{x}\"", .{hash}) catch bun.outOfMemory(); - headers.append("ETag", etag_str) catch bun.outOfMemory(); + if (blob.slice().len > 0) { + ETag.appendToHeaders(blob.slice(), &headers) catch bun.outOfMemory(); + } } return bun.new(StaticRoute, .{ @@ -151,16 +148,9 @@ pub fn fromJS(globalThis: *jsc.JSGlobalObject, argument: jsc.JSValue) bun.JSErro // Generate ETag if not already present if (headers.get("etag") == null) { - const slice = blob.slice(); - const hash = std.hash.XxHash64.hash(0, slice); - - var etag_buf: [32]u8 = undefined; - const etag_str = std.fmt.bufPrint(&etag_buf, "\"{x}\"", .{hash}) catch bun.outOfMemory(); - headers.append("ETag", etag_str) catch { - var mutable_blob = blob; - mutable_blob.detach(); - return globalThis.throwOutOfMemory(); - }; + if (blob.slice().len > 0) { + try ETag.appendToHeaders(blob.slice(), &headers); + } } return bun.new(StaticRoute, .{ @@ -181,18 +171,7 @@ pub fn fromJS(globalThis: *jsc.JSGlobalObject, argument: jsc.JSValue) bun.JSErro pub fn onHEADRequest(this: *StaticRoute, req: *uws.Request, resp: AnyResponse) void { // Check If-None-Match for HEAD requests with 200 status if (this.status_code == 200) { - if (evaluateIfNoneMatch(this, req)) { - // Return 304 Not Modified - req.setYield(false); - this.ref(); - if (this.server) |server| { - server.onPendingRequest(); - resp.timeout(server.config().idleTimeout); - } - this.doWriteStatus(304, resp); - this.doWriteHeaders(resp); - resp.endWithoutBody(resp.shouldCloseConnection()); - this.onResponseComplete(resp); + if (this.render304NotModifiedIfNoneMatch(req, resp)) { return; } } @@ -235,18 +214,7 @@ pub fn onRequest(this: *StaticRoute, req: *uws.Request, resp: AnyResponse) void pub fn onGET(this: *StaticRoute, req: *uws.Request, resp: AnyResponse) void { // Check If-None-Match for GET requests with 200 status if (this.status_code == 200) { - if (evaluateIfNoneMatch(this, req)) { - // Return 304 Not Modified - req.setYield(false); - this.ref(); - if (this.server) |server| { - server.onPendingRequest(); - resp.timeout(server.config().idleTimeout); - } - this.doWriteStatus(304, resp); - this.doWriteHeaders(resp); - resp.endWithoutBody(resp.shouldCloseConnection()); - this.onResponseComplete(resp); + if (this.render304NotModifiedIfNoneMatch(req, resp)) { return; } } @@ -292,7 +260,7 @@ fn onResponseComplete(this: *StaticRoute, resp: AnyResponse) void { this.deref(); } -pub fn doRenderBlob(this: *StaticRoute, resp: AnyResponse, did_finish: *bool) void { +fn doRenderBlob(this: *StaticRoute, resp: AnyResponse, did_finish: *bool) void { // We are not corked // The body is small // Faster to do the memcpy than to do the two network calls @@ -305,7 +273,7 @@ pub fn doRenderBlob(this: *StaticRoute, resp: AnyResponse, did_finish: *bool) vo } } -pub fn doRenderBlobCorked(this: *StaticRoute, resp: AnyResponse, did_finish: *bool) void { +fn doRenderBlobCorked(this: *StaticRoute, resp: AnyResponse, did_finish: *bool) void { this.renderMetadata(resp); this.renderBytes(resp, did_finish); } @@ -382,68 +350,42 @@ pub fn onWithMethod(this: *StaticRoute, method: bun.http.Method, resp: AnyRespon } } -/// Parse a single entity tag from a string, returns the tag without quotes and whether it's weak -fn parseEntityTag(tag_str: []const u8) struct { tag: []const u8, is_weak: bool } { - var str = std.mem.trim(u8, tag_str, " \t"); - - // Check for weak indicator - var is_weak = false; - if (std.mem.startsWith(u8, str, "W/")) { - is_weak = true; - str = str[2..]; - str = std.mem.trimLeft(u8, str, " \t"); - } - - // Remove surrounding quotes - if (str.len >= 2 and str[0] == '"' and str[str.len - 1] == '"') { - str = str[1 .. str.len - 1]; - } - - return .{ .tag = str, .is_weak = is_weak }; -} - -/// Perform weak comparison between two entity tags according to RFC 9110 Section 8.8.3.2 -fn weakEntityTagMatch(tag1: []const u8, is_weak1: bool, tag2: []const u8, is_weak2: bool) bool { - _ = is_weak1; - _ = is_weak2; - // For weak comparison, we only compare the opaque tag values, ignoring weak indicators - return std.mem.eql(u8, tag1, tag2); -} - -/// Evaluate If-None-Match condition according to RFC 9110 Section 13.1.2 -fn evaluateIfNoneMatch(this: *StaticRoute, req: *uws.Request) bool { +fn render304NotModifiedIfNoneMatch(this: *StaticRoute, req: *uws.Request, resp: AnyResponse) bool { const if_none_match = req.header("if-none-match") orelse return false; - - // Get the ETag from our headers - const our_etag = this.headers.get("etag") orelse return false; - const our_parsed = parseEntityTag(our_etag); - - // Handle "*" case - if (std.mem.eql(u8, std.mem.trim(u8, if_none_match, " \t"), "*")) { - return true; // Condition is false, so we should return 304 + const etag = this.headers.get("etag") orelse return false; + if (if_none_match.len == 0 or etag.len == 0) { + return false; } - // Parse comma-separated list of entity tags - var iter = std.mem.splitScalar(u8, if_none_match, ','); - while (iter.next()) |tag_str| { - const parsed = parseEntityTag(tag_str); - if (weakEntityTagMatch(our_parsed.tag, our_parsed.is_weak, parsed.tag, parsed.is_weak)) { - return true; // Condition is false, so we should return 304 - } + if (!ETag.ifNoneMatch(etag, if_none_match)) { + return false; } - return false; // Condition is true, continue with normal processing + // Return 304 Not Modified + req.setYield(false); + this.ref(); + if (this.server) |server| { + server.onPendingRequest(); + resp.timeout(server.config().idleTimeout); + } + this.doWriteStatus(304, resp); + this.doWriteHeaders(resp); + resp.endWithoutBody(resp.shouldCloseConnection()); + this.onResponseComplete(resp); + return true; } const std = @import("std"); const bun = @import("bun"); const jsc = bun.jsc; -const Headers = bun.http.Headers; const api = bun.schema.api; const AnyServer = jsc.API.AnyServer; const writeStatus = bun.api.server.writeStatus; const AnyBlob = jsc.WebCore.Blob.Any; +const ETag = bun.http.ETag; +const Headers = bun.http.Headers; + const uws = bun.uws; const AnyResponse = uws.AnyResponse; diff --git a/src/bun.js/webcore/Response.zig b/src/bun.js/webcore/Response.zig index 8e619f8b3d..49076bed2c 100644 --- a/src/bun.js/webcore/Response.zig +++ b/src/bun.js/webcore/Response.zig @@ -426,6 +426,17 @@ pub fn constructJSON( did_succeed = true; return bun.new(Response, response).toJS(globalThis); } + +fn validateRedirectStatusCode(globalThis: *jsc.JSGlobalObject, status_code: i32) bun.JSError!u16 { + switch (status_code) { + 301, 302, 303, 307, 308 => return @intCast(status_code), + else => { + const err = globalThis.createRangeErrorInstance("Failed to execute 'redirect' on 'Response': Invalid status code", .{}); + return globalThis.throwValue(err); + }, + } +} + pub fn constructRedirect( globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame, @@ -464,36 +475,17 @@ pub fn constructRedirect( if (args.nextEat()) |init| { if (init.isUndefinedOrNull()) {} else if (init.isNumber()) { - const status_number = init.toInt32(); - // Validate redirect status codes (301, 302, 303, 307, 308) - if (status_number == 301 or status_number == 302 or status_number == 303 or status_number == 307 or status_number == 308) { - response.init.status_code = @as(u16, @intCast(status_number)); - } else { - const err = globalThis.createRangeErrorInstance("Failed to execute 'redirect' on 'Response': Invalid status code", .{}); - return globalThis.throwValue(err); - } - } else if (init.isObject()) { - // Only process object init values to prevent crash with non-object values - if (Response.Init.init(globalThis, init) catch |err| - if (err == error.JSError) return .zero else null) |_init| - { - // Validate that status code is a valid redirect status if provided - if (_init.status_code != 200) { // 200 is the default, so if it's changed, validate it - if (_init.status_code == 301 or _init.status_code == 302 or _init.status_code == 303 or _init.status_code == 307 or _init.status_code == 308) { - response.init = _init; - } else { - response.init.deinit(bun.default_allocator); - const err = globalThis.createRangeErrorInstance("Failed to execute 'redirect' on 'Response': Invalid status code", .{}); - return globalThis.throwValue(err); - } - } else { - response.init = _init; - response.init.status_code = 302; // Default redirect status - } + response.init.status_code = try validateRedirectStatusCode(globalThis, init.toInt32()); + } else if (try Response.Init.init(globalThis, init)) |_init| { + errdefer response.init.deinit(bun.default_allocator); + response.init = _init; + + if (_init.status_code != 200) { + response.init.status_code = try validateRedirectStatusCode(globalThis, _init.status_code); } } - // Non-object, non-number init values are ignored (like strings, booleans, etc.) } + if (globalThis.hasException()) { return .zero; } @@ -642,7 +634,13 @@ pub const Init = struct { if (!response_init.isCell()) return null; - if (response_init.jsType() == .DOMWrapper) { + const js_type = response_init.jsType(); + + if (!js_type.isObject()) { + return null; + } + + if (js_type == .DOMWrapper) { // fast path: it's a Request object or a Response object // we can skip calling JS getters if (response_init.asDirect(Request)) |req| { diff --git a/src/http.zig b/src/http.zig index 15d0785a8e..d5389563b1 100644 --- a/src/http.zig +++ b/src/http.zig @@ -2424,6 +2424,7 @@ pub const ThreadlocalAsyncHTTP = struct { async_http: AsyncHTTP, }; +pub const ETag = @import("./http/ETag.zig"); pub const Method = @import("./http/Method.zig").Method; pub const Headers = @import("./http/Headers.zig"); pub const MimeType = @import("./http/MimeType.zig"); diff --git a/src/http/ETag.zig b/src/http/ETag.zig new file mode 100644 index 0000000000..416a4e8aff --- /dev/null +++ b/src/http/ETag.zig @@ -0,0 +1,65 @@ +const ETag = @This(); + +/// Parse a single entity tag from a string, returns the tag without quotes and whether it's weak +fn parse(tag_str: []const u8) struct { tag: []const u8, is_weak: bool } { + var str = std.mem.trim(u8, tag_str, " \t"); + + // Check for weak indicator + var is_weak = false; + if (bun.strings.hasPrefix(str, "W/")) { + is_weak = true; + str = str[2..]; + str = std.mem.trimLeft(u8, str, " \t"); + } + + // Remove surrounding quotes + if (str.len >= 2 and str[0] == '"' and str[str.len - 1] == '"') { + str = str[1 .. str.len - 1]; + } + + return .{ .tag = str, .is_weak = is_weak }; +} + +/// Perform weak comparison between two entity tags according to RFC 9110 Section 8.8.3.2 +fn weakMatch(tag1: []const u8, is_weak1: bool, tag2: []const u8, is_weak2: bool) bool { + _ = is_weak1; + _ = is_weak2; + // For weak comparison, we only compare the opaque tag values, ignoring weak indicators + return std.mem.eql(u8, tag1, tag2); +} + +pub fn appendToHeaders(bytes: []const u8, headers: *bun.http.Headers) !void { + const hash = std.hash.XxHash64.hash(0, bytes); + + var etag_buf: [40]u8 = undefined; + const etag_str = std.fmt.bufPrint(&etag_buf, "\"{}\"", .{bun.fmt.hexIntLower(hash)}) catch unreachable; + try headers.append("etag", etag_str); +} + +pub fn ifNoneMatch( + /// "ETag" header + etag: []const u8, + /// "If-None-Match" header + if_none_match: []const u8, +) bool { + const our_parsed = parse(etag); + + // Handle "*" case + if (std.mem.eql(u8, std.mem.trim(u8, if_none_match, " \t"), "*")) { + return true; // Condition is false, so we should return 304 + } + + // Parse comma-separated list of entity tags + var iter = std.mem.splitScalar(u8, if_none_match, ','); + while (iter.next()) |tag_str| { + const parsed = parse(tag_str); + if (weakMatch(our_parsed.tag, our_parsed.is_weak, parsed.tag, parsed.is_weak)) { + return true; // Condition is false, so we should return 304 + } + } + + return false; // Condition is true, continue with normal processing +} + +const bun = @import("bun"); +const std = @import("std");