address review: Browser-only download events, Header param parsing, real url/basename, deny default

This commit is contained in:
Armaan Sandhu
2026-06-18 03:15:31 +05:30
committed by Pierre Tachoire
parent 8e8a9b2213
commit ab4d702445
8 changed files with 165 additions and 94 deletions

View File

@@ -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,

View File

@@ -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" {

View File

@@ -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;

View File

@@ -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,
};

View File

@@ -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 {

View File

@@ -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" });

View File

@@ -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.

View File

@@ -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 = "";