From ab4d702445e9569b62e12efc0fb0722015fa0e60 Mon Sep 17 00:00:00 2001 From: Armaan Sandhu Date: Thu, 18 Jun 2026 03:15:31 +0530 Subject: [PATCH] address review: Browser-only download events, Header param parsing, real url/basename, deny default --- src/Notification.zig | 4 +- src/browser/Frame.zig | 126 +++++++++++++++++------------------- src/browser/HttpClient.zig | 1 + src/browser/Session.zig | 7 +- src/cdp/CDP.zig | 2 +- src/cdp/domains/browser.zig | 32 +++++++-- src/cdp/domains/page.zig | 14 ---- src/network/http.zig | 73 +++++++++++++++++++++ 8 files changed, 165 insertions(+), 94 deletions(-) diff --git a/src/Notification.zig b/src/Notification.zig index b710cf855..4a96b2382 100644 --- a/src/Notification.zig +++ b/src/Notification.zig @@ -277,8 +277,8 @@ pub const CookieChanged = struct { // Emitted by Frame when a navigation response is treated as a file download // (Content-Disposition: attachment) under an `allow`/`allowAndName` -// `Browser.setDownloadBehavior`. The CDP page domain turns this into a -// `Page.downloadWillBegin` event. All fields point at Frame-owned memory that +// `Browser.setDownloadBehavior`. The CDP browser domain turns this into a +// `Browser.downloadWillBegin` event. All fields point at Frame-owned memory that // outlives the synchronous dispatch. pub const DownloadWillBegin = struct { frame_id: u32, diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index 77479e87b..86d624e55 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -67,6 +67,7 @@ const MouseEvent = @import("webapi/event/MouseEvent.zig"); const WheelEvent = @import("webapi/event/WheelEvent.zig"); const HttpClient = @import("HttpClient.zig"); +const sys_url = @import("../sys/url.zig"); const timestamp = @import("../datetime.zig").timestamp; const milliTimestamp = @import("../datetime.zig").milliTimestamp; @@ -1214,19 +1215,19 @@ fn maybeStartDownload(self: *Frame, response: HttpClient.Response) !bool { const session = self._session; switch (session.download_behavior) { .allow, .allow_and_name => {}, - .default, .deny => return false, + .deny => return false, } - const disposition = blk: { + const disposition: HttpClient.Header = blk: { var it = response.headerIterator(); while (it.next()) |hdr| { if (std.ascii.eqlIgnoreCase(hdr.name, "content-disposition")) { - break :blk hdr.value; + break :blk hdr; } } return false; }; - if (isAttachment(disposition) == false) { + if (std.ascii.eqlIgnoreCase(disposition.firstValue(), "attachment") == false) { return false; } @@ -1235,11 +1236,13 @@ fn maybeStartDownload(self: *Frame, response: HttpClient.Response) !bool { return false; }; + // `guid` is the CDP "Global Unique Identifier" that ties the + // downloadWillBegin / downloadProgress events to one download. var guid_buf: [36]u8 = undefined; @import("../id.zig").uuidv4(&guid_buf); const guid = try self.arena.dupe(u8, &guid_buf); - const suggested = dispositionFilename(disposition) orelse urlBasename(self.url) orelse guid; + const suggested = dispositionFilename(disposition) orelse (try urlBasename(self.arena, self.url)) orelse guid; const suggested_filename = try self.arena.dupe(u8, suggested); // allowAndName stores the file under its guid; allow uses the suggested name. @@ -1290,59 +1293,32 @@ fn maybeStartDownload(self: *Frame, response: HttpClient.Response) !bool { return true; } -// True when a Content-Disposition value's type token is "attachment". -fn isAttachment(disposition: []const u8) bool { - const semi = std.mem.indexOfScalar(u8, disposition, ';') orelse disposition.len; - const token = std.mem.trim(u8, disposition[0..semi], " \t"); - return std.ascii.eqlIgnoreCase(token, "attachment"); -} - -// Extracts the filename from a Content-Disposition value, handling the quoted, +// Extracts the filename from a Content-Disposition header, handling the quoted, // unquoted, and RFC 5987 (filename*=charset''value) forms. Path components are // stripped so the result is always a bare basename. -fn dispositionFilename(disposition: []const u8) ?[]const u8 { +fn dispositionFilename(disposition: HttpClient.Header) ?[]const u8 { // Prefer the extended filename*= form when present, per RFC 6266. - if (paramValue(disposition, "filename*")) |ext| { + if (disposition.param("filename*")) |ext| { // charset'lang'value — take everything after the second quote. if (std.mem.indexOfScalar(u8, ext, '\'')) |first| { if (std.mem.indexOfScalarPos(u8, ext, first + 1, '\'')) |second| { - return basename(ext[second + 1 ..]); + return sanitizeFilename(ext[second + 1 ..]); } } - return basename(ext); + return sanitizeFilename(ext); } - if (paramValue(disposition, "filename")) |name| { - return basename(name); + if (disposition.param("filename")) |name| { + return sanitizeFilename(name); } return null; } -// Returns the (optionally quoted) value of `key=` within a parameter list. -fn paramValue(disposition: []const u8, key: []const u8) ?[]const u8 { - var rest = disposition; - while (std.mem.indexOfScalar(u8, rest, ';')) |semi| { - const param = std.mem.trim(u8, rest[semi + 1 ..][0 .. (std.mem.indexOfScalarPos(u8, rest, semi + 1, ';') orelse rest.len) - semi - 1], " \t"); - if (std.mem.indexOfScalar(u8, param, '=')) |eq| { - const name = std.mem.trim(u8, param[0..eq], " \t"); - if (std.ascii.eqlIgnoreCase(name, key)) { - var value = std.mem.trim(u8, param[eq + 1 ..], " \t"); - if (value.len >= 2 and value[0] == '"' and value[value.len - 1] == '"') { - value = value[1 .. value.len - 1]; - } - if (value.len > 0) { - return value; - } - } - } - rest = rest[semi + 1 ..]; - } - return null; -} - -// Strips any directory components, guarding against path traversal. -fn basename(name: []const u8) ?[]const u8 { - var out = name; - if (std.mem.lastIndexOfAny(u8, out, "/\\")) |i| { +// Strips any directory components, guarding against path traversal. Content- +// Disposition can carry Windows separators, so backslashes are stripped too, +// regardless of the host platform. +fn sanitizeFilename(name: []const u8) ?[]const u8 { + var out = std.fs.path.basename(name); + if (std.mem.lastIndexOfScalar(u8, out, '\\')) |i| { out = out[i + 1 ..]; } if (out.len == 0 or std.mem.eql(u8, out, ".") or std.mem.eql(u8, out, "..")) { @@ -1351,12 +1327,21 @@ fn basename(name: []const u8) ?[]const u8 { return out; } -// Derives a filename from a URL's last path segment, ignoring query/fragment. -fn urlBasename(url: []const u8) ?[]const u8 { - var path = url; - if (std.mem.indexOfScalar(u8, path, '?')) |i| path = path[0..i]; - if (std.mem.indexOfScalar(u8, path, '#')) |i| path = path[0..i]; - return basename(path); +// Derives a filename from a URL's last path segment. The path is taken from a +// real URL parse (rust-url) so query/fragment and percent-encoding are handled +// the same way the rest of the browser handles URLs. The result is duped into +// `arena`, since the parsed URL is freed before this returns. +fn urlBasename(arena: Allocator, url: []const u8) !?[]const u8 { + var err: i32 = 0; + const u = sys_url.url_parse(url.ptr, url.len, &err) orelse return null; + defer sys_url.url_free(u); + + var ptr: [*]const u8 = undefined; + var len: usize = undefined; + sys_url.url_get_path(u, &ptr, &len); + + const name = sanitizeFilename(ptr[0..len]) orelse return null; + return try arena.dupe(u8, name); } fn frameDataCallback(response: HttpClient.Response, data: []const u8) !void { @@ -1439,6 +1424,11 @@ fn frameDataCallback(response: HttpClient.Response, data: []const u8) !void { .raw, .image => |*buf| try buf.appendSlice(self.arena, data), .download => |*download| { download.file.writeAll(data) catch |err| { + // TODO(#2701 follow-up): surface the write failure properly. We + // can't set `_parse_state = .err` here because the next chunk + // would then hit the `.err => unreachable` branch below, and we + // should also emit a `canceled` downloadProgress and remove the + // partial file on disk. For now we just log and keep going. log.err(.frame, "download write", .{ .err = err, .guid = download.guid }); return; }; @@ -4853,32 +4843,32 @@ fn asUint(comptime string: anytype) std.meta.Int( const testing = @import("../testing.zig"); -test "Frame: isAttachment" { - try testing.expectEqual(true, isAttachment("attachment")); - try testing.expectEqual(true, isAttachment("attachment; filename=\"a.csv\"")); - try testing.expectEqual(true, isAttachment(" ATTACHMENT ; filename=a.csv")); - try testing.expectEqual(false, isAttachment("inline")); - try testing.expectEqual(false, isAttachment("inline; filename=a.csv")); - try testing.expectEqual(false, isAttachment("")); +fn dispositionHeader(value: []const u8) HttpClient.Header { + return .{ .name = "content-disposition", .value = value }; } test "Frame: dispositionFilename" { - try testing.expectEqualSlices(u8, "report.csv", dispositionFilename("attachment; filename=\"report.csv\"").?); - try testing.expectEqualSlices(u8, "report.csv", dispositionFilename("attachment; filename=report.csv").?); - try testing.expectEqualSlices(u8, "r e.csv", dispositionFilename("attachment; filename=\"r e.csv\"").?); + try testing.expectEqualSlices(u8, "report.csv", dispositionFilename(dispositionHeader("attachment; filename=\"report.csv\"")).?); + try testing.expectEqualSlices(u8, "report.csv", dispositionFilename(dispositionHeader("attachment; filename=report.csv")).?); + try testing.expectEqualSlices(u8, "r e.csv", dispositionFilename(dispositionHeader("attachment; filename=\"r e.csv\"")).?); // RFC 5987 extended form is preferred over the plain filename when present // (the value is taken verbatim after the charset'lang' prefix). - try testing.expectEqualSlices(u8, "extended.txt", dispositionFilename("attachment; filename=\"fallback.txt\"; filename*=UTF-8''extended.txt").?); - try testing.expect(dispositionFilename("attachment") == null); + try testing.expectEqualSlices(u8, "extended.txt", dispositionFilename(dispositionHeader("attachment; filename=\"fallback.txt\"; filename*=UTF-8''extended.txt")).?); + try testing.expect(dispositionFilename(dispositionHeader("attachment")) == null); // Path components are stripped to guard against traversal. - try testing.expectEqualSlices(u8, "evil.sh", dispositionFilename("attachment; filename=\"../../evil.sh\"").?); - try testing.expect(dispositionFilename("attachment; filename=\"..\"") == null); + try testing.expectEqualSlices(u8, "evil.sh", dispositionFilename(dispositionHeader("attachment; filename=\"../../evil.sh\"")).?); + try testing.expectEqualSlices(u8, "evil.sh", dispositionFilename(dispositionHeader("attachment; filename=\"..\\..\\evil.sh\"")).?); + try testing.expect(dispositionFilename(dispositionHeader("attachment; filename=\"..\"")) == null); } test "Frame: urlBasename" { - try testing.expectEqualSlices(u8, "report.csv", urlBasename("http://x.com/a/b/report.csv").?); - try testing.expectEqualSlices(u8, "report.csv", urlBasename("http://x.com/report.csv?v=1#x").?); - try testing.expect(urlBasename("http://x.com/") == null); + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + + try testing.expectEqualSlices(u8, "report.csv", (try urlBasename(a, "http://x.com/a/b/report.csv")).?); + try testing.expectEqualSlices(u8, "report.csv", (try urlBasename(a, "http://x.com/report.csv?v=1#x")).?); + try testing.expect((try urlBasename(a, "http://x.com/")) == null); } test "WebApi: Frame" { diff --git a/src/browser/HttpClient.zig b/src/browser/HttpClient.zig index 5334bc263..ecba705eb 100644 --- a/src/browser/HttpClient.zig +++ b/src/browser/HttpClient.zig @@ -38,6 +38,7 @@ const Allocator = std.mem.Allocator; const IS_DEBUG = builtin.mode == .Debug; pub const Method = http.Method; +pub const Header = http.Header; pub const Headers = http.Headers; pub const ResponseHead = http.ResponseHead; pub const HeaderIterator = http.HeaderIterator; diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 6c1ac961c..161c3e092 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -117,16 +117,17 @@ cancel_hook: ?CancelHook = null, // `.allow_and_name`, a navigation whose response carries // `Content-Disposition: attachment` is written to `download_path` instead // of being parsed as a page, and (when `download_events_enabled` is set) -// `Page.downloadWillBegin` / `Browser.downloadProgress` events are emitted. +// `Browser.downloadWillBegin` / `Browser.downloadProgress` events are emitted. // `download_path` is duped into the Session arena. -download_behavior: DownloadBehavior = .default, +download_behavior: DownloadBehavior = .deny, download_path: ?[]const u8 = null, download_events_enabled: bool = false, pub const DownloadBehavior = enum { - default, allow, allow_and_name, + // The CDP `default` behavior is mapped to `deny`: we don't write downloads + // to disk unless the driver explicitly opts in with `allow`/`allowAndName`. deny, }; diff --git a/src/cdp/CDP.zig b/src/cdp/CDP.zig index cd8fd437e..0a4a4d2b8 100644 --- a/src/cdp/CDP.zig +++ b/src/cdp/CDP.zig @@ -823,7 +823,7 @@ pub const BrowserContext = struct { pub fn onDownloadWillBegin(ctx: *anyopaque, msg: *const Notification.DownloadWillBegin) !void { const self: *BrowserContext = @ptrCast(@alignCast(ctx)); - return @import("domains/page.zig").downloadWillBegin(self, msg); + return @import("domains/browser.zig").downloadWillBegin(self, msg); } pub fn onDownloadProgress(ctx: *anyopaque, msg: *const Notification.DownloadProgress) !void { diff --git a/src/cdp/domains/browser.zig b/src/cdp/domains/browser.zig index adcb27657..c7de2ac9f 100644 --- a/src/cdp/domains/browser.zig +++ b/src/cdp/domains/browser.zig @@ -18,6 +18,7 @@ const std = @import("std"); const lp = @import("lightpanda"); +const id = @import("../id.zig"); const CDP = @import("../CDP.zig"); const Session = @import("../../browser/Session.zig"); const Notification = @import("../../Notification.zig"); @@ -84,14 +85,18 @@ fn setDownloadBehavior(cmd: *CDP.Command) !void { eventsEnabled: ?bool = null, })) orelse return error.InvalidParams; + if (params.browserContextId != null) { + log.warn(.not_implemented, "Browser.setDownloadBehavior", .{ .param = "browserContextId" }); + } + + // `default` defers the choice to the browser; we map it to `deny` (no + // download is written unless a driver explicitly opts in). const behavior: Session.DownloadBehavior = if (std.mem.eql(u8, params.behavior, "allow")) .allow else if (std.mem.eql(u8, params.behavior, "allowAndName")) .allow_and_name - else if (std.mem.eql(u8, params.behavior, "deny")) + else if (std.mem.eql(u8, params.behavior, "deny") or std.mem.eql(u8, params.behavior, "default")) .deny - else if (std.mem.eql(u8, params.behavior, "default")) - .default else return error.InvalidParams; @@ -121,6 +126,21 @@ fn setDownloadBehavior(cmd: *CDP.Command) !void { return cmd.sendResult(null, .{ .include_session_id = false }); } +// https://chromedevtools.github.io/devtools-protocol/tot/Browser/#event-downloadWillBegin +// Dispatched by Frame when a navigation response is treated as a file download. +// The opt-in is Browser.setDownloadBehavior, so we emit Browser.* events only +// (not the deprecated Page.downloadWillBegin / Page.downloadProgress). See #2701. +pub fn downloadWillBegin(bc: *CDP.BrowserContext, event: *const Notification.DownloadWillBegin) !void { + const session_id = bc.session_id orelse return; + + return bc.cdp.sendEvent("Browser.downloadWillBegin", .{ + .frameId = &id.toFrameId(event.frame_id), + .guid = event.guid, + .url = event.url, + .suggestedFilename = event.suggested_filename, + }, .{ .session_id = session_id }); +} + // https://chromedevtools.github.io/devtools-protocol/tot/Browser/#event-downloadProgress // Dispatched by Frame as a download is written to disk (see issue #2701). pub fn downloadProgress(bc: *CDP.BrowserContext, msg: *const Notification.DownloadProgress) !void { @@ -306,13 +326,13 @@ test "cdp.browser: setDownloadBehavior stores config on the session" { try testing.expect(bc.session.download_events_enabled); try testing.expect(bc.download_events_registered); - // Flipping back to default tears the registration down again. + // `default` maps to `deny` and tears the registration down again. try ctx.processMessage(.{ .id = 35, .method = "Browser.setDownloadBehavior", .params = .{ .behavior = "default" }, }); - try testing.expectEqual(.default, bc.session.download_behavior); + try testing.expectEqual(.deny, bc.session.download_behavior); try testing.expect(bc.session.download_path == null); try testing.expect(bc.session.download_events_enabled == false); try testing.expect(bc.download_events_registered == false); @@ -378,7 +398,7 @@ test "cdp.browser: setDownloadBehavior writes an attachment to disk and emits ev try runner.wait(.{ .ms = 2000 }); // The guid is random, so it's omitted from these subset matches. - try ctx.expectSentEvent("Page.downloadWillBegin", .{ + try ctx.expectSentEvent("Browser.downloadWillBegin", .{ .url = "http://127.0.0.1:9582/download/report.csv", .suggestedFilename = "report.csv", }, .{ .session_id = "SID-DL" }); diff --git a/src/cdp/domains/page.zig b/src/cdp/domains/page.zig index 8be988d36..2a5808e1d 100644 --- a/src/cdp/domains/page.zig +++ b/src/cdp/domains/page.zig @@ -806,20 +806,6 @@ pub fn frameNetworkAlmostIdle(bc: *CDP.BrowserContext, event: *const Notificatio return sendPageLifecycle(bc, "networkAlmostIdle", event.timestamp, &id.toFrameId(event.frame_id), &id.toLoaderId(event.loader_id)); } -// https://chromedevtools.github.io/devtools-protocol/tot/Page/#event-downloadWillBegin -// Dispatched by Frame when a navigation response is treated as a file download -// (see issue #2701). -pub fn downloadWillBegin(bc: *CDP.BrowserContext, event: *const Notification.DownloadWillBegin) !void { - const session_id = bc.session_id orelse return; - - return bc.cdp.sendEvent("Page.downloadWillBegin", .{ - .frameId = &id.toFrameId(event.frame_id), - .guid = event.guid, - .url = event.url, - .suggestedFilename = event.suggested_filename, - }, .{ .session_id = session_id }); -} - fn sendPageLifecycle(bc: *CDP.BrowserContext, name: []const u8, timestamp: u64, frame_id: []const u8, loader_id: []const u8) !void { // detachTarget could be called, in which case, we still have a frame doing // things, but no session. diff --git a/src/network/http.zig b/src/network/http.zig index 663a5b0dc..718d57b56 100644 --- a/src/network/http.zig +++ b/src/network/http.zig @@ -54,6 +54,59 @@ pub const Method = enum(u8) { pub const Header = struct { name: []const u8, value: []const u8, + + pub const Param = struct { + key: []const u8, + value: []const u8, + }; + + // The header value up to the first ';', trimmed (e.g. "attachment" for a + // Content-Disposition, "text/html" for a Content-Type). + pub fn firstValue(self: Header) []const u8 { + const end = std.mem.indexOfScalar(u8, self.value, ';') orelse self.value.len; + return std.mem.trim(u8, self.value[0..end], " \t"); + } + + // Iterates the `; key=value` parameters that follow the header's first value. + pub fn params(self: Header) ParamIterator { + const start = std.mem.indexOfScalar(u8, self.value, ';') orelse self.value.len; + return .{ .rest = self.value[start..] }; + } + + // Returns the (unquoted) value of the first non-empty `key=` parameter, if any. + pub fn param(self: Header, key: []const u8) ?[]const u8 { + var it = self.params(); + while (it.next()) |p| { + if (p.value.len > 0 and std.ascii.eqlIgnoreCase(p.key, key)) { + return p.value; + } + } + return null; + } + + pub const ParamIterator = struct { + rest: []const u8, + + pub fn next(self: *ParamIterator) ?Param { + while (self.rest.len > 0 and self.rest[0] == ';') { + self.rest = self.rest[1..]; + const end = std.mem.indexOfScalar(u8, self.rest, ';') orelse self.rest.len; + const segment = self.rest[0..end]; + self.rest = self.rest[end..]; + + const eq = std.mem.indexOfScalar(u8, segment, '=') orelse continue; + const key = std.mem.trim(u8, segment[0..eq], " \t"); + if (key.len == 0) continue; + + var value = std.mem.trim(u8, segment[eq + 1 ..], " \t"); + if (value.len >= 2 and value[0] == '"' and value[value.len - 1] == '"') { + value = value[1 .. value.len - 1]; + } + return .{ .key = key, .value = value }; + } + return null; + } + }; }; pub const Headers = struct { @@ -719,6 +772,26 @@ fn makeSockAddrV4(ip: [4]u8) libcurl.CurlSockAddr { const testing = @import("../testing.zig"); +test "Header.firstValue" { + try testing.expectEqualSlices(u8, "attachment", (Header{ .name = "Content-Disposition", .value = "attachment" }).firstValue()); + // firstValue trims but preserves case (callers compare case-insensitively). + try testing.expectEqualSlices(u8, "ATTACHMENT", (Header{ .name = "Content-Disposition", .value = " ATTACHMENT ; filename=a.csv" }).firstValue()); + try testing.expectEqualSlices(u8, "text/html", (Header{ .name = "Content-Type", .value = "text/html; charset=utf-8" }).firstValue()); + try testing.expectEqualSlices(u8, "", (Header{ .name = "X", .value = "" }).firstValue()); +} + +test "Header.param" { + const h: Header = .{ .name = "Content-Disposition", .value = "attachment; filename=\"r e.csv\"; filename*=UTF-8''e.txt" }; + try testing.expectEqualSlices(u8, "r e.csv", h.param("filename").?); + try testing.expectEqualSlices(u8, "r e.csv", h.param("FILENAME").?); + try testing.expectEqualSlices(u8, "UTF-8''e.txt", h.param("filename*").?); + try testing.expect(h.param("missing") == null); + // A bare value with no parameters has nothing to return. + try testing.expect((Header{ .name = "Content-Disposition", .value = "attachment" }).param("filename") == null); + // Empty values are skipped. + try testing.expect((Header{ .name = "Content-Disposition", .value = "attachment; filename=\"\"" }).param("filename") == null); +} + fn findHeader(headers: Headers, name: []const u8) struct { count: usize, value: []const u8 } { var count: usize = 0; var value: []const u8 = "";