From e25d2da0c0d97bc1816540e204d30a0f5c205434 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 31 Aug 2026 14:36:59 +0800 Subject: [PATCH 1/2] chore: Simplify making HTTP requests. If you look at https://github.com/lightpanda-io/browser/pull/3293, you'll see a relatively contained change that has to touch over 20 files. The issue is that every HttpClient.newRequest needs to provide a lot of data. But `newRequest` takes a 2nd parameter: the HttpClient.Owner. If we make that Owner a little smarter, we can start to remove some of the individual fields needed in newRequest. For example, we can still allow a callsite to pass frame_id but, by default, we can use the owner's frame_id (which is what we want in most cases). --- src/Notification.zig | 4 +- src/browser/Frame.zig | 115 ++----- src/browser/ScriptManager.zig | 15 - src/browser/ScriptManagerBase.zig | 33 -- src/browser/frame/resource_load.zig | 5 - src/browser/js/Execution.zig | 18 +- src/browser/webapi/Document.zig | 24 +- .../webapi/SharedWorkerGlobalScope.zig | 5 - src/browser/webapi/Worker.zig | 3 - src/browser/webapi/WorkerGlobalScope.zig | 29 +- src/browser/webapi/net/EventSource.zig | 8 +- src/browser/webapi/net/Fetch.zig | 18 +- src/browser/webapi/net/XMLHttpRequest.zig | 8 +- src/browser/webapi/storage/CookieStore.zig | 16 +- src/network/HttpClient.zig | 293 +++++++++++------- src/network/RobotsGate.zig | 6 +- src/network/SingleFlight.zig | 5 - src/network/WebBotAuth.zig | 5 - src/server/cdp/CDP.zig | 5 - src/server/cdp/domains/network.zig | 10 +- 20 files changed, 258 insertions(+), 367 deletions(-) diff --git a/src/Notification.zig b/src/Notification.zig index 48bc53737..b7021dc2c 100644 --- a/src/Notification.zig +++ b/src/Notification.zig @@ -131,7 +131,7 @@ const Events = union(enum) { download_will_begin: *const DownloadWillBegin, download_progress: *const DownloadProgress, }; -const EventType = std.meta.FieldEnum(Events); +pub const EventType = std.meta.FieldEnum(Events); pub const FrameRemove = struct {}; @@ -483,7 +483,7 @@ pub fn dispatch(self: *Notification, comptime event: EventType, data: ArgType(ev } // Given an event type enum, returns the type of arg the event emits -fn ArgType(comptime event: Notification.EventType) type { +pub fn ArgType(comptime event: Notification.EventType) type { inline for (std.meta.fields(Notification.Events)) |f| { if (std.mem.eql(u8, f.name, @tagName(event))) { return f.type; diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index a1d2ef4bd..354627235 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -410,7 +410,17 @@ pub fn init(self: *Frame, frame_id: u32, page: *Page, opts: InitOpts) !void { ._http_owner = undefined, }; self._queued_events = &self._queued_events_1; - self._http_owner = .init(&page.blob_urls, &self.origin); + self._http_owner = .{ + .blob_urls = &page.blob_urls, + .origin = &self.origin, + .url = &self.url, + .parent = if (parent) |p| &p._http_owner else null, + .frame_id = frame_id, + .document_frame_id = frame_id, + .loader_id = self._loader_id, + .cookie_jar = &session.cookie_jar, + .notification = session.notification, + }; var screen: *Screen = undefined; var visual_viewport: *VisualViewport = undefined; @@ -612,19 +622,7 @@ fn referrerSource(self: *const Frame) [:0]const u8 { // RFC 6265bis "site for cookies" for SameSite checks on requests this frame does. pub fn siteForCookies(self: *const Frame) Cookie.SiteForCookies { - const own_url = self.referrerSource(); - const own_host = URL.getHostname(own_url); - var frame: *const Frame = self; - while (frame.parent) |parent| : (frame = parent) { - const parent_host = URL.getHostname(parent.url); - if (parent_host.len == 0) { - continue; - } - if (Cookie.areHostsSameSite(parent_host, own_host) == false) { - return .none; - } - } - return .{ .url = own_url }; + return self._http_owner.siteForCookies(); } pub fn getTitle(self: *Frame) !?[]const u8 { @@ -850,18 +848,14 @@ pub fn navigate(self: *Frame, request_url: [:0]const u8, opts: NavigateOpts) !vo const transfer = try http_client.newRequest(.{ .ctx = self, .url = self.url, - .frame_id = self._frame_id, - .loader_id = self._loader_id, .method = opts.method, .body = opts.body, // don't cache top-level pages, most cases won't revisit this, and, if they // do, they probably don't want the cached version. .skip_cache = self.parent == null, .throttle = self.parent == null, - .cookie_jar = &session.cookie_jar, - .cookie_origin = opts.initiator_url orelse .{ .url = self.url }, + .cookie_origin = opts.initiator_url, .resource_type = .document, - .notification = self._session.notification, .header_callback = frameHeaderDoneCallback, .data_callback = frameDataCallback, .done_callback = frameDoneCallback, @@ -1058,20 +1052,12 @@ fn scheduleNavigationWithArena(originator: *Frame, arena: *lp.Arena, request_url nav_opts.referrer_policy = originator.referrer_policy; } } - if (nav_opts.initiator_url == null) { - if (target.parent) |parent| { - // A subframe navigation's SameSite initiator is the frame's whole - // ancestor chain, not the document that triggered the navigation. - if (std.mem.startsWith(u8, parent.referrerSource(), "http")) { - nav_opts.initiator_url = switch (parent.siteForCookies()) { - .none => .none, - .url => |u| .{ .url = try arena.dupeZ(u8, u) }, - }; - } - } else if (std.mem.startsWith(u8, referrer_source, "http")) { - // Top-level navigation. - nav_opts.initiator_url = .{ .url = try arena.dupeZ(u8, referrer_source) }; - } + // A subframe navigation's SameSite initiator is the frame's whole ancestor + // chain, not the document that triggered the navigation; the request gets + // that from its owner. Only a top-level navigation's initiator is another + // document. + if (nav_opts.initiator_url == null and target.parent == null and std.mem.startsWith(u8, referrer_source, "http")) { + nav_opts.initiator_url = .{ .url = try arena.dupeZ(u8, referrer_source) }; } if (nav_opts.initiator_origin == null) { if (originator.origin) |o| { @@ -1140,9 +1126,7 @@ pub fn makeRequest(self: *Frame, req: HttpClient.Request) !void { // Two-phase variant; see HttpClient.newRequest for the ownership contract. pub fn newRequest(self: *Frame, req: HttpClient.Request) !*HttpClient.Transfer { - var r = req; - r.document_frame_id = self._frame_id; - return self._session.browser.http_client.newRequest(r, &self._http_owner); + return self._session.browser.http_client.newRequest(req, &self._http_owner); } // Synchronously abort every transfer and WebSocket owned by this frame @@ -2011,15 +1995,12 @@ pub fn iframeAddedCallback(self: *Frame, iframe: *IFrame) !void { self.child_frames_sorted = false; // Iframe's initial src request carries the parent's URL as Referer - // (subject to the parent's Referrer-Policy). The SameSite initiator is - // the parent's ancestor chain. + // (subject to the parent's Referrer-Policy). const referrer_source = self.referrerSource(); - const initiator_url: ?Cookie.SiteForCookies = if (std.mem.startsWith(u8, referrer_source, "http")) self.siteForCookies() else null; new_frame.navigate(url, .{ .reason = .initialFrameNavigation, .referer = try referrer.compute(self.call_arena, self.referrer_policy, referrer_source, url), .referrer_policy = self.referrer_policy, - .initiator_url = initiator_url, .initiator_origin = self.origin, }) catch |err| { // extra defensive..maybe navigate added a new frame, and the index it @@ -2371,12 +2352,7 @@ pub fn loadExternalStylesheet(self: *Frame, link: *Element.Html.Link, href: []co const transfer = http_client.newRequest(.{ .url = resolved, .method = .GET, - .frame_id = self._frame_id, - .loader_id = self._loader_id, - .cookie_jar = &session.cookie_jar, - .cookie_origin = self.siteForCookies(), .resource_type = .stylesheet, - .notification = session.notification, .shutdown_callback = HttpClient.noopShutdown, // syncRequest installs its own }, &self._http_owner) catch |err| { log.warn(.http, "external stylesheet fetch", .{ .err = err, .url = resolved }); @@ -3390,13 +3366,13 @@ pub const NavigateOpts = struct { // can recompute the header. null (e.g. a CDP-supplied referrer) leaves // the Referer untouched across redirects. referrer_policy: ?referrer.Policy = null, - // The "site for cookies" of the document that initiated this navigation, - // used when computing SameSite: for a subframe, the whole ancestor - // chain's site (or .none when that chain crosses sites); for a top-level - // navigation, the initiating document's URL. Distinct from `referer` + // The "site for cookies" of the document that initiated a top-level + // navigation, used when computing SameSite. Distinct from `referer` // because a Referrer-Policy can suppress the Referer header without // affecting SameSite (which always considers the real initiator). null - // means browser-initiated, which cookie lookup treats as same-site. + // leaves it to the navigated frame's owner: its own site for a top-level + // navigation (browser-initiated, treated as same-site), its ancestor + // chain's for a subframe. initiator_url: ?Cookie.SiteForCookies = null, initiator_origin: ?[]const u8 = null, force: bool = false, @@ -3832,45 +3808,6 @@ test "Page: isSameOrigin" { try testing.expectEqual(false, frame.isSameOrigin("//origin.com/foo")); } -test "Frame: siteForCookies" { - var top: Frame = undefined; - top.parent = null; - top.url = "http://attacker.example/attacker-nested"; - - var middle: Frame = undefined; - middle.parent = ⊤ - middle.url = "http://victim.example/nested-middle"; - - var inner: Frame = undefined; - inner.parent = &middle; - inner.url = "http://victim.example/inner"; - - // A top-level document is its own site. - try testing.expectEqual("http://attacker.example/attacker-nested", top.siteForCookies().url); - - // Cross-site with the top-level document: no site for cookies — for the - // directly-embedded frame and for the same-site-with-parent frame nested - // under it alike. - try testing.expectEqual(true, middle.siteForCookies() == .none); - try testing.expectEqual(true, inner.siteForCookies() == .none); - - // A fully same-site chain (subdomains included) keeps its site. - top.url = "http://victim.example/"; - middle.url = "http://sub.victim.example/nested-middle"; - try testing.expectEqual("http://victim.example/inner", inner.siteForCookies().url); - try testing.expectEqual("http://sub.victim.example/nested-middle", middle.siteForCookies().url); - - // about: documents inherit their creator's origin: transparent as an - // ancestor, and judged through their nearest real ancestor themselves. - middle.url = "about:blank"; - try testing.expectEqual("http://victim.example/inner", inner.siteForCookies().url); - try testing.expectEqual("http://victim.example/", middle.siteForCookies().url); - - top.url = "http://attacker.example/"; - try testing.expectEqual(true, inner.siteForCookies() == .none); - try testing.expectEqual("http://attacker.example/", middle.siteForCookies().url); -} - test "Frame: static immediate meta refresh navigates" { const page = try testing.pageTest("fixtures/meta_refresh.html", .{}); defer page.close(); diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig index 4345ca392..98a245a8a 100644 --- a/src/browser/ScriptManager.zig +++ b/src/browser/ScriptManager.zig @@ -134,12 +134,7 @@ pub fn preloadScript(self: *ScriptManager, element: ?*Element.Html, url: []const .ctx = script, .url = owned_url, .method = .GET, - .frame_id = frame._frame_id, - .loader_id = frame._loader_id, - .cookie_jar = &frame._session.cookie_jar, - .cookie_origin = frame.siteForCookies(), .resource_type = .script, - .notification = frame._session.notification, .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, .header_callback = Script.headerCallback, .data_callback = Script.dataCallback, @@ -350,12 +345,7 @@ pub fn addFromElement(self: *ScriptManager, comptime from_parser: bool, script_e const transfer = try self.base.client.newRequest(.{ .url = remote_url, .method = .GET, - .frame_id = frame._frame_id, - .loader_id = frame._loader_id, - .cookie_jar = &frame._session.cookie_jar, - .cookie_origin = frame.siteForCookies(), .resource_type = .script, - .notification = frame._session.notification, .shutdown_callback = HttpClient.noopShutdown, // syncRequest installs its own }, &frame._http_owner); { @@ -399,12 +389,7 @@ pub fn addFromElement(self: *ScriptManager, comptime from_parser: bool, script_e .ctx = script, .url = remote_url, .method = .GET, - .frame_id = frame._frame_id, - .loader_id = frame._loader_id, - .cookie_jar = &frame._session.cookie_jar, - .cookie_origin = frame.siteForCookies(), .resource_type = .script, - .notification = frame._session.notification, .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, .header_callback = Script.headerCallback, .data_callback = Script.dataCallback, diff --git a/src/browser/ScriptManagerBase.zig b/src/browser/ScriptManagerBase.zig index 9bb1731d5..1e90c2fb5 100644 --- a/src/browser/ScriptManagerBase.zig +++ b/src/browser/ScriptManagerBase.zig @@ -27,7 +27,6 @@ const Session = @import("Session.zig"); const Frame = @import("Frame.zig"); const ImportMap = @import("ImportMap.zig"); const WorkerGlobalScope = @import("webapi/WorkerGlobalScope.zig"); -const Cookie = @import("webapi/storage/Cookie.zig"); const Element = @import("webapi/Element.zig"); @@ -50,18 +49,6 @@ pub const Owner = union(enum) { }; } - pub fn frameId(self: Owner) u32 { - return switch (self) { - inline else => |g| g._frame_id, - }; - } - - pub fn loaderId(self: Owner) u32 { - return switch (self) { - inline else => |g| g._loader_id, - }; - } - pub fn session(self: Owner) *Session { return switch (self) { inline else => |g| g._session, @@ -79,14 +66,6 @@ pub const Owner = union(enum) { inline else => |g| g.makeRequest(req), }; } - - // `Execution.siteForCookies` ditto. - pub fn siteForCookies(self: Owner) Cookie.SiteForCookies { - return switch (self) { - .frame => |frame| frame.siteForCookies(), - .worker => |worker| worker.site_for_cookies, - }; - } }; owner: Owner, @@ -266,17 +245,11 @@ pub fn preloadImport(self: *ScriptManagerBase, url: [:0]const u8, referrer: []co self.async_scripts.append(&script.node); const owner = self.owner; - const session = owner.session(); owner.makeRequest(.{ .ctx = script, .url = url, .method = .GET, - .frame_id = owner.frameId(), - .loader_id = owner.loaderId(), - .cookie_jar = &session.cookie_jar, - .cookie_origin = owner.siteForCookies(), .resource_type = .script, - .notification = session.notification, .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, .header_callback = Script.headerCallback, .data_callback = Script.dataCallback, @@ -459,18 +432,12 @@ pub fn getAsyncImport(self: *ScriptManagerBase, url: [:0]const u8, cb: ImportAsy defer self.endEvaluationWindow(was_evaluating); const owner = self.owner; - const session = self.owner.session(); self.async_scripts.append(&script.node); owner.makeRequest(.{ .ctx = script, .url = url, .method = .GET, - .frame_id = owner.frameId(), - .loader_id = owner.loaderId(), .resource_type = .script, - .cookie_jar = &session.cookie_jar, - .cookie_origin = owner.siteForCookies(), - .notification = session.notification, .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, .header_callback = Script.headerCallback, .data_callback = Script.dataCallback, diff --git a/src/browser/frame/resource_load.zig b/src/browser/frame/resource_load.zig index c9a161a00..941daae25 100644 --- a/src/browser/frame/resource_load.zig +++ b/src/browser/frame/resource_load.zig @@ -79,12 +79,7 @@ pub fn image(frame: *Frame, img: *Element.Html.Image, src: []const u8) !void { .ctx = load, .url = resolved, .method = .GET, - .frame_id = frame._frame_id, - .loader_id = frame._loader_id, - .cookie_jar = &session.cookie_jar, - .cookie_origin = frame.siteForCookies(), .resource_type = .image, - .notification = session.notification, .headers_only = true, .header_callback = ImageLoad.headerCallback, .data_callback = ImageLoad.dataCallback, diff --git a/src/browser/js/Execution.zig b/src/browser/js/Execution.zig index 118583214..8862f8c5f 100644 --- a/src/browser/js/Execution.zig +++ b/src/browser/js/Execution.zig @@ -133,12 +133,8 @@ pub fn origin(self: *const Execution) ?[]const u8 { }; } -// a Worker inherits its creating document's; `Frame` walks its ancestor chain. pub fn siteForCookies(self: *const Execution) Cookie.SiteForCookies { - return switch (self.js.global) { - .frame => |frame| frame.siteForCookies(), - .worker => |worker| worker.site_for_cookies, - }; + return self.httpOwner().siteForCookies(); } // HttpClient.Owner of the current global (Frame or WGS). Used by code @@ -181,15 +177,3 @@ pub fn console(self: *const Execution) *Console { .worker => |worker| worker.getConsole(), }; } - -pub fn frameId(self: *const Execution) u32 { - return switch (self.js.global) { - inline else => |g| g._frame_id, - }; -} - -pub fn loaderId(self: *const Execution) u32 { - return switch (self.js.global) { - inline else => |g| g._loader_id, - }; -} diff --git a/src/browser/webapi/Document.zig b/src/browser/webapi/Document.zig index 290177129..a94d308a5 100644 --- a/src/browser/webapi/Document.zig +++ b/src/browser/webapi/Document.zig @@ -19,14 +19,15 @@ const std = @import("std"); const lp = @import("lightpanda"); -const js = @import("../js/js.zig"); -const Frame = @import("../Frame.zig"); -const Window = @import("Window.zig"); -const URL = @import("../URL.zig"); const idna = @import("../../sys/idna.zig"); const public_suffix_list = @import("../../data/public_suffix_list.zig"); +const URL = @import("../URL.zig"); +const js = @import("../js/js.zig"); +const Frame = @import("../Frame.zig"); + const Node = @import("Node.zig"); +const Window = @import("Window.zig"); const Element = @import("Element.zig"); const Location = @import("Location.zig"); const Parser = @import("../parser/Parser.zig"); @@ -1638,6 +1639,8 @@ pub const JsApi = struct { }; const testing = @import("../../testing.zig"); +const HttpClient = @import("../../network/HttpClient.zig"); + test "WebApi: Document" { try testing.htmlRunner("document", .{}); } @@ -1655,12 +1658,13 @@ test "Document: cookie access from a cross-site frame" { // victim.example embedded by attacker.example: the ancestor chain is // cross-site, so the frame has no site for cookies. - var top: Frame = undefined; + var top_url: [:0]const u8 = "https://attacker.example/"; + var top: HttpClient.Owner = undefined; + top.url = &top_url; top.parent = null; - top.url = "https://attacker.example/"; frame.url = "https://victim.example/inner"; - frame.parent = ⊤ - defer frame.parent = null; + frame._http_owner.parent = ⊤ + defer frame._http_owner.parent = null; try jar.populateFromResponse("https://victim.example/", "strict=1; SameSite=Strict"); try jar.populateFromResponse("https://victim.example/", "lax=2; SameSite=Lax"); @@ -1681,7 +1685,7 @@ test "Document: cookie access from a cross-site frame" { // The same jar seen from a same-site chain: everything applies, and the // dropped writes really were dropped rather than hidden. - top.url = "https://victim.example/"; + top_url = "https://victim.example/"; try testing.expectEqual("strict=1; lax=2; default=3; none=4; set_none=8", try doc.getCookie(frame)); _ = try doc.setCookie("set_strict=5; SameSite=Strict", frame); _ = try doc.setCookie("set_default=7", frame); @@ -1689,7 +1693,7 @@ test "Document: cookie access from a cross-site frame" { // Back in the cross-site context, the newly written cookies obey the // same visibility rules. - top.url = "https://attacker.example/"; + top_url = "https://attacker.example/"; try testing.expectEqual("none=4; set_none=8", try doc.getCookie(frame)); } diff --git a/src/browser/webapi/SharedWorkerGlobalScope.zig b/src/browser/webapi/SharedWorkerGlobalScope.zig index 2a38832dc..69710022e 100644 --- a/src/browser/webapi/SharedWorkerGlobalScope.zig +++ b/src/browser/webapi/SharedWorkerGlobalScope.zig @@ -104,12 +104,7 @@ pub fn init(frame: *Frame, url: [:0]const u8, name: []const u8, worker_type: Wor .ctx = self, .method = .GET, .url = owned_url, - .frame_id = self._frame_id, - .loader_id = self._loader_id, .resource_type = .script, - .cookie_jar = &session.cookie_jar, - .cookie_origin = proto.site_for_cookies, - .notification = session.notification, .header_callback = httpHeaderCallback, .data_callback = httpDataCallback, .done_callback = httpDoneCallback, diff --git a/src/browser/webapi/Worker.zig b/src/browser/webapi/Worker.zig index 85440d747..49ac6b1d5 100644 --- a/src/browser/webapi/Worker.zig +++ b/src/browser/webapi/Worker.zig @@ -106,9 +106,6 @@ pub fn init(url: []const u8, options: ?WorkerOptions, frame: *Frame) !*Worker { .frame_id = self._frame_id, .loader_id = self._loader_id, .resource_type = .script, - .cookie_jar = &session.cookie_jar, - .cookie_origin = frame.siteForCookies(), - .notification = session.notification, .header_callback = httpHeaderCallback, .data_callback = httpDataCallback, .done_callback = httpDoneCallback, diff --git a/src/browser/webapi/WorkerGlobalScope.zig b/src/browser/webapi/WorkerGlobalScope.zig index 330b16c19..dde06509d 100644 --- a/src/browser/webapi/WorkerGlobalScope.zig +++ b/src/browser/webapi/WorkerGlobalScope.zig @@ -46,7 +46,6 @@ const ErrorEvent = @import("event/ErrorEvent.zig"); const Fetch = @import("net/Fetch.zig"); const idb = @import("storage/idb/idb.zig"); const CookieStore = @import("storage/CookieStore.zig"); -const Cookie = @import("storage/Cookie.zig"); const MessagePort = @import("MessagePort.zig"); const SharedWorkerGlobalScope = @import("SharedWorkerGlobalScope.zig"); const DedicatedWorkerGlobalScope = @import("DedicatedWorkerGlobalScope.zig"); @@ -79,8 +78,6 @@ local_arena: Allocator, url: [:0]const u8, // Same-origin constraint: a worker's origin is inherited from its parent frame. origin: ?[]const u8 = null, -// Inherited from the creating frame, like origin. -site_for_cookies: Cookie.SiteForCookies, buf: [1024]u8 = undefined, // same size as frame.buf // Document charset (matches Page.charset). Workers default to UTF-8. charset: []const u8 = "UTF-8", @@ -151,10 +148,6 @@ pub fn init( .url = url, .arena = arena, .origin = frame.origin, - .site_for_cookies = switch (frame.siteForCookies()) { - .none => .none, - .url => |u| .{ .url = try arena.dupeZ(u8, u) }, - }, .js = undefined, ._call_arena = call_arena, ._local_arena = local_arena, @@ -181,7 +174,17 @@ pub fn init( const self = leaf._proto; self._type = @unionInit(Type, @tagName(tag), leaf); - self._http_owner = .init(&frame._page.blob_urls, &self.origin); + self._http_owner = .{ + .blob_urls = &frame._page.blob_urls, + .origin = &self.origin, + .url = null, + .parent = &frame._http_owner, + .frame_id = frame_id, + .document_frame_id = frame._frame_id, + .loader_id = loader_id, + .cookie_jar = &session.cookie_jar, + .notification = session.notification, + }; self._script_manager = ScriptManagerBase.init( arena, @@ -282,9 +285,7 @@ pub fn makeRequest(self: *WorkerGlobalScope, req: HttpClient.Request) !void { // Two-phase variant; see HttpClient.newRequest for the ownership contract. pub fn newRequest(self: *WorkerGlobalScope, req: HttpClient.Request) !*HttpClient.Transfer { - var r = req; - r.document_frame_id = self._frame._frame_id; - return self._session.browser.http_client.newRequest(r, &self._http_owner); + return self._session.browser.http_client.newRequest(req, &self._http_owner); } pub fn getSelf(self: *WorkerGlobalScope) *WorkerGlobalScope { @@ -426,13 +427,7 @@ fn importScript(self: *WorkerGlobalScope, arena: Allocator, url: [:0]const u8) ! const transfer = http_client.newRequest(.{ .url = resolved_url, .method = .GET, - .frame_id = self._frame_id, - .document_frame_id = self._frame._frame_id, - .loader_id = self._loader_id, - .cookie_jar = &session.cookie_jar, - .cookie_origin = self.site_for_cookies, .resource_type = .script, - .notification = session.notification, .shutdown_callback = HttpClient.noopShutdown, // syncRequest installs its own }, &self._http_owner) catch |err| { log.warn(.http, "importScript", .{ .url = resolved_url, .err = err }); diff --git a/src/browser/webapi/net/EventSource.zig b/src/browser/webapi/net/EventSource.zig index 02b7926c4..8b13019d4 100644 --- a/src/browser/webapi/net/EventSource.zig +++ b/src/browser/webapi/net/EventSource.zig @@ -162,8 +162,6 @@ fn asEventTarget(self: *EventSource) *EventTarget { fn connect(self: *EventSource) !void { const exec = self._exec; - const session = exec.session; - self._skip_lf = false; self._bom_checked = false; self._line_buf.clearRetainingCapacity(); @@ -180,13 +178,9 @@ fn connect(self: *EventSource) !void { .ctx = self, .url = self._url, .method = .GET, - .frame_id = exec.frameId(), - .loader_id = exec.loaderId(), - .cookie_jar = if (cookie_support) &session.cookie_jar else null, - .cookie_origin = exec.siteForCookies(), + .cookies = cookie_support, .resource_type = .eventsource, .streaming = true, - .notification = session.notification, .header_callback = httpHeaderDoneCallback, .data_callback = httpDataCallback, .done_callback = httpDoneCallback, diff --git a/src/browser/webapi/net/Fetch.zig b/src/browser/webapi/net/Fetch.zig index 25c469dc4..df2adb8f1 100644 --- a/src/browser/webapi/net/Fetch.zig +++ b/src/browser/webapi/net/Fetch.zig @@ -83,34 +83,26 @@ pub fn init(input: Input, options: ?InitOpts, exec: *const Execution) !js.Promis ._manual_redirect = request._redirect == .manual, }; - const session = exec.session; - if (comptime lp.IS_DEBUG) { log.debug(.http, "fetch", .{ .url = request._url }); } - const cookie_jar = switch (request._credentials) { - .omit => null, - .include => &session.cookie_jar, - .@"same-origin" => if (exec.isSameOrigin(request._url)) &session.cookie_jar else null, - }; - const transfer = exec.newRequest(.{ .ctx = fetch, .url = request._url, .method = request._method, - .frame_id = exec.frameId(), - .loader_id = exec.loaderId(), .body = request._body, .resource_type = .fetch, - .cookie_jar = cookie_jar, - .cookie_origin = exec.siteForCookies(), + .cookies = switch (request._credentials) { + .omit => false, + .include => true, + .@"same-origin" => exec.isSameOrigin(request._url), + }, .redirect = switch (request._redirect) { .follow => .follow, .manual => .manual, .@"error" => .@"error", }, - .notification = session.notification, .header_callback = httpHeaderDoneCallback, .data_callback = httpDataCallback, .done_callback = httpDoneCallback, diff --git a/src/browser/webapi/net/XMLHttpRequest.zig b/src/browser/webapi/net/XMLHttpRequest.zig index 21063d416..d21aee839 100644 --- a/src/browser/webapi/net/XMLHttpRequest.zig +++ b/src/browser/webapi/net/XMLHttpRequest.zig @@ -303,8 +303,6 @@ pub fn send(self: *XMLHttpRequest, body_: ?BodyInit, exec_: *const Execution) !v const exec = self._exec; - const session = exec.session; - // Only add cookies for same-origin or when withCredentials is true const cookie_support = self._with_credentials or exec.isSameOrigin(self._url); @@ -316,14 +314,10 @@ pub fn send(self: *XMLHttpRequest, body_: ?BodyInit, exec_: *const Execution) !v .ctx = self, .url = self._url, .method = self._method, - .frame_id = exec.frameId(), - .loader_id = exec.loaderId(), .body = self._request_body, - .cookie_jar = if (cookie_support) &session.cookie_jar else null, - .cookie_origin = exec.siteForCookies(), + .cookies = cookie_support, .resource_type = .xhr, .timeout_ms = self._timeout, - .notification = session.notification, .header_callback = httpHeaderDoneCallback, .data_callback = httpDataCallback, .done_callback = httpDoneCallback, diff --git a/src/browser/webapi/storage/CookieStore.zig b/src/browser/webapi/storage/CookieStore.zig index a1c9b4ea6..96918721f 100644 --- a/src/browser/webapi/storage/CookieStore.zig +++ b/src/browser/webapi/storage/CookieStore.zig @@ -23,7 +23,6 @@ const js = @import("../../js/js.zig"); const URL = @import("../../URL.zig"); const Notification = @import("../../../Notification.zig"); -const Frame = @import("../../Frame.zig"); const Cookie = @import("Cookie.zig"); const EventTarget = @import("../EventTarget.zig"); const CookieChangeEvent = @import("../event/CookieChangeEvent.zig"); @@ -616,6 +615,8 @@ pub const CookieListItem = struct { }; const testing = @import("../../../testing.zig"); +const HttpClient = @import("../../../network/HttpClient.zig"); + test "WebApi: CookieStore" { try testing.htmlRunner("cookie_store.html", .{}); } @@ -629,12 +630,13 @@ test "CookieStore: cross-site frame" { // victim.example embedded by attacker.example: the ancestor chain is // cross-site, so the frame has no site for cookies. - var top: Frame = undefined; + var top_url: [:0]const u8 = "https://attacker.example/"; + var top: HttpClient.Owner = undefined; + top.url = &top_url; top.parent = null; - top.url = "https://attacker.example/"; frame.url = "https://victim.example/inner"; - frame.parent = ⊤ - defer frame.parent = null; + frame._http_owner.parent = ⊤ + defer frame._http_owner.parent = null; try jar.populateFromResponse("https://victim.example/", "strict=1; SameSite=Strict"); try jar.populateFromResponse("https://victim.example/", "lax=2; SameSite=Lax"); @@ -662,7 +664,7 @@ test "CookieStore: cross-site frame" { // The same jar seen from a same-site chain: everything applies, and the // rejected writes really were rejected rather than hidden. - top.url = "https://victim.example/"; + top_url = "https://victim.example/"; { const items = try matchCookies(exec, null, null, false); try testing.expectEqual(4, items.len); @@ -687,7 +689,7 @@ test "CookieStore: cross-site frame" { // Back in the cross-site context, the Strict cookie written same-site is // hidden again. - top.url = "https://attacker.example/"; + top_url = "https://attacker.example/"; { const items = try matchCookies(exec, null, null, false); try testing.expectEqual(1, items.len); diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index 953de939c..71cd2fd84 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -606,7 +606,17 @@ pub fn newRequest(self: *Client, req: Request, owner: ?*Owner) anyerror!*Transfe // These are all small, so duping them into the transfer's arena is // cheap and can solve some nasty UAF. owned.url = try arena.dupeZ(u8, req.url); - if (req.cookie_origin) |cookie_origin| { + + var cookie_jar: ?*CookieJar = null; + if (owner) |o| { + if (owned.frame_id == 0) owned.frame_id = o.frame_id; + if (owned.loader_id == 0) owned.loader_id = o.loader_id; + if (owned.document_frame_id == null) owned.document_frame_id = o.document_frame_id; + if (owned.notification == null) owned.notification = o.notification; + if (owned.cookie_origin == null) owned.cookie_origin = o.siteForCookies(); + if (req.cookies) cookie_jar = o.cookie_jar; + } + if (owned.cookie_origin) |cookie_origin| { owned.cookie_origin = switch (cookie_origin) { .none => .none, .url => |url| .{ .url = try arena.dupeZ(u8, url) }, @@ -628,6 +638,7 @@ pub fn newRequest(self: *Client, req: Request, owner: ?*Owner) anyerror!*Transfe const t = try arena.create(Transfer); t.* = .{ .req = owned, + .cookie_jar = cookie_jar, .client = self, .arena = arena, .id = self.incrReqId(), @@ -951,10 +962,10 @@ fn pipeline(self: *Client, transfer: *Transfer, from: SubmitFrom) !void { if (self.serve_mode) { transfer._notify_cdp = true; - transfer.req.notification.dispatch(.http_request_start, &.{ .transfer = transfer }); + transfer.notify(.http_request_start, &.{ .transfer = transfer }); var wait_for_interception = false; - transfer.req.notification.dispatch(.http_request_intercept, &.{ + transfer.notify(.http_request_intercept, &.{ .transfer = transfer, .wait_for_interception = &wait_for_interception, }); @@ -1528,7 +1539,7 @@ fn processOneMessage(self: *Client, msg: http.Handles.MultiMessage, transfer: *T // TODO give a way to configure the number of auth retries. if (transfer._auth_challenge != null and transfer._tries < 10) { var wait_for_interception = false; - transfer.req.notification.dispatch( + transfer.notify( .http_request_auth_required, &.{ .transfer = transfer, .wait_for_interception = &wait_for_interception }, ); @@ -1583,7 +1594,7 @@ fn processOneMessage(self: *Client, msg: http.Handles.MultiMessage, transfer: *T // Chromium announces each redirect hop before pausing it // for Fetch interception. Playwright uses redirectResponse // to pair the new pause with a new Request. - transfer.req.notification.dispatch(.http_request_start, &.{ + transfer.notify(.http_request_start, &.{ .transfer = transfer, .redirect_response = true, }); @@ -1599,7 +1610,7 @@ fn processOneMessage(self: *Client, msg: http.Handles.MultiMessage, transfer: *T if (self.serve_mode) { // e.g. cdp var wait_for_interception = false; - transfer.req.notification.dispatch(.http_request_intercept, &.{ + transfer.notify(.http_request_intercept, &.{ .transfer = transfer, .wait_for_interception = &wait_for_interception, }); @@ -1774,18 +1785,13 @@ pub const Request = struct { // ten segments, versus a TCP handshake plus a TLS one. const HEADERS_ONLY_DRAIN_MAX: usize = 16 * 1024; - frame_id: u32, - loader_id: u32, method: Method, url: [:0]const u8, body: ?[]const u8 = null, - cookie_jar: ?*CookieJar, - cookie_origin: ?Cookie.SiteForCookies, resource_type: ResourceType, redirect: RedirectMode = .follow, referrer_policy: ?referrer.Policy = null, credentials: ?[:0]const u8 = null, - notification: *Notification, timeout_ms: u32 = 0, skip_cache: bool = false, @@ -1798,9 +1804,22 @@ pub const Request = struct { // fires. headers_only: bool = false, - // The document frame this request belongs to, for CDP attribution. - // This will be different than frame_id for Workers. + // Should only be set when they need to differ from the owner's. + frame_id: u32 = 0, + loader_id: u32 = 0, + // The document frame this request belongs to. Differs from frame_id for + // Workers. document_frame_id: ?u32 = null, + notification: ?*Notification = null, + + // Send the owner's cookies and honour Set-Cookie. Off for a credential-less + // fetch / XHR / EventSource. Meaningless without an owner: there is no jar. + cookies: bool = true, + + // The site for SameSite checks. null = the owner's (Owner.siteForCookies). + // Frame.navigate is the one caller with a reason to override it: the + // initiator of a top-level navigation isn't the frame being navigated. + cookie_origin: ?Cookie.SiteForCookies = null, // Requests that are internal to the browser and skip various layers, // these do not need to be deferred and do not obey robots.txt. @@ -1837,22 +1856,6 @@ pub const Request = struct { // every caller decides — pass `HttpClient.noopShutdown` to opt out, // knowingly. shutdown_callback: ShutdownCallback, - - pub fn getCookieString(self: *Request, arena: Allocator) !?[:0]const u8 { - const jar = self.cookie_jar orelse return null; - var aw: std.Io.Writer.Allocating = .init(arena); - try jar.forRequest(self.url, &aw.writer, .{ - .is_http = true, - .origin_url = self.cookie_origin, - .is_navigation = self.resource_type == .document, - }); - if (aw.written().len == 0) { - return null; - } - try aw.writer.writeByte(0); - const written = aw.written(); - return written.ptr[0 .. written.len - 1 :0]; - } }; pub const SyncResponse = struct { @@ -1946,7 +1949,7 @@ fn fulfillRedirect( errdefer |err| transfer.abortPipelineError(err); // retrieve cookies from the fulfilled response's headers. - if (transfer.req.cookie_jar) |jar| { + if (transfer.cookie_jar) |jar| { for (headers) |hdr| { if (std.ascii.eqlIgnoreCase(hdr.name, "set-cookie")) { try jar.populateFromResponse(transfer.req.url, hdr.value); @@ -2005,13 +2008,47 @@ pub const Owner = struct { // it can change during navigation. origin: *const ?[]const u8, + // The owning Frame's URL slot, a pointer for the same reason. A worker + // has none: its site for cookies is its creating document's. + url: ?*const [:0]const u8, + + // The parent frame's Owner; for a worker, its creating frame's. Outlives + // this Owner: child frames are torn down before their parent, a worker + // before its frame. + parent: ?*const Owner, + + // Copied onto every request made through this owner, see Request. + frame_id: u32, + document_frame_id: u32, + loader_id: u32, + cookie_jar: *CookieJar, + notification: *Notification, + const Blob = @import("../browser/webapi/Blob.zig"); - pub fn init(blob_urls: *const Blob.UrlMap, origin: *const ?[]const u8) Owner { - return .{ - .blob_urls = blob_urls, - .origin = origin, - }; + // RFC 6265bis "site for cookies" + pub fn siteForCookies(self: *const Owner) Cookie.SiteForCookies { + var source = self; + while (source.parent) |parent| : (source = parent) { + const url = source.url orelse continue; + if (!std.mem.startsWith(u8, url.*, "about:")) break; + } + // Only a worker has no url, and a worker always hangs off a Frame. + const own_url = source.url.?.*; + const own_host = URL.getHostname(own_url); + + var owner = source; + while (owner.parent) |parent| : (owner = parent) { + const parent_url = parent.url orelse continue; + const parent_host = URL.getHostname(parent_url.*); + if (parent_host.len == 0) { + continue; + } + if (Cookie.areHostsSameSite(parent_host, own_host) == false) { + return .none; + } + } + return .{ .url = own_url }; } pub fn addTransfer(self: *Owner, t: *Transfer) void { @@ -2057,6 +2094,9 @@ pub const Transfer = struct { res: Response = .{}, client: *Client, + // The owner's jar, unless the request opted out of cookies. + cookie_jar: ?*CookieJar = null, + req_headers: std.ArrayList(RequestHeader) = .empty, start_time: u64, @@ -2444,6 +2484,30 @@ pub const Transfer = struct { self.failAsync(err); } + pub fn getCookieString(self: *Transfer, arena: Allocator) !?[:0]const u8 { + const jar = self.cookie_jar orelse return null; + const req = &self.req; + var aw: std.Io.Writer.Allocating = .init(arena); + try jar.forRequest(req.url, &aw.writer, .{ + .is_http = true, + .origin_url = req.cookie_origin, + .is_navigation = req.resource_type == .document, + }); + if (aw.written().len == 0) { + return null; + } + try aw.writer.writeByte(0); + const written = aw.written(); + return written.ptr[0 .. written.len - 1 :0]; + } + + // Mirrors the transfer to whoever the owner reports to (CDP). A request + // without attribution has nobody listening. + fn notify(self: *Transfer, comptime event: Notification.EventType, data: Notification.ArgType(event)) void { + const notification = self.req.notification orelse return; + notification.dispatch(event, data); + } + // Owner-driven teardown: fires shutdown_callback (not error_callback) // and otherwise behaves like abort. Called by Client.abortOwner / // abortRequests when a Frame / WGS is being torn down. Any buffered, @@ -2451,7 +2515,7 @@ pub const Transfer = struct { fn kill(self: *Transfer) void { if (self._notify_cdp and !self._notified_fail) { self._notified_fail = true; - self.req.notification.dispatch(.http_request_fail, &.{ + self.notify(.http_request_fail, &.{ .transfer = self, .err = error.Shutdown, }); @@ -2533,7 +2597,7 @@ pub const Transfer = struct { } if (self._notify_cdp) { - self.req.notification.dispatch(.http_request_fail, &.{ + self.notify(.http_request_fail, &.{ .transfer = self, .err = err, .blocked_reason = if (err == error.UrlBlocked) .inspector else null, @@ -2706,7 +2770,7 @@ pub const Transfer = struct { const headers = try it.collect(arena.allocator()); self.res.headers = headers.items; - if (self.req.cookie_jar) |jar| { + if (self.cookie_jar) |jar| { for (self.res.headers) |hdr| { if (std.ascii.eqlIgnoreCase(hdr.name, "set-cookie")) { jar.populateFromResponse(self.req.url, hdr.value) catch |err| { @@ -2771,7 +2835,7 @@ pub const Transfer = struct { try conn.commitHeaders(); // Add cookies from cookie jar. - if (try self.req.getCookieString(self.arena.allocator())) |cookies| { + if (try self.getCookieString(self.arena.allocator())) |cookies| { try conn.setCookies(@ptrCast(cookies.ptr)); } @@ -3307,7 +3371,7 @@ pub const Transfer = struct { const req = &transfer.req; if (transfer._from_cache) { - req.notification.dispatch( + transfer.notify( .http_request_served_from_cache, &.{ .transfer = transfer }, ); @@ -3336,7 +3400,7 @@ pub const Transfer = struct { }, .header => { if (transfer._notify_cdp) { - req.notification.dispatch(.http_response_header_done, &.{ + transfer.notify(.http_response_header_done, &.{ .transfer = transfer, }); } @@ -3349,7 +3413,7 @@ pub const Transfer = struct { }, .data => |chunk| { if (transfer._notify_cdp) { - req.notification.dispatch(.http_response_data, &.{ + transfer.notify(.http_response_data, &.{ .data = chunk, .transfer = transfer, }); @@ -3371,7 +3435,7 @@ pub const Transfer = struct { std.mem.swap(std.ArrayList(u8), &res.buffer, &res.stream.spare); const chunk = res.stream.spare.items; if (transfer._notify_cdp) { - req.notification.dispatch(.http_response_data, &.{ + transfer.notify(.http_response_data, &.{ .data = chunk, .transfer = transfer, }); @@ -3384,7 +3448,7 @@ pub const Transfer = struct { .done => { terminal = true; if (transfer._notify_cdp) { - req.notification.dispatch(.http_request_done, &.{ + transfer.notify(.http_request_done, &.{ .transfer = transfer, .content_length = transfer._content_length, }); @@ -3546,6 +3610,22 @@ const Synthetic = struct { }; const testing = @import("../testing.zig"); + +// Only the transfer list matters to the tests using it: they build their +// transfers by hand and never go through newRequest. +fn testOwner() Owner { + return .{ + .blob_urls = undefined, + .origin = undefined, + .url = null, + .parent = null, + .frame_id = 0, + .document_frame_id = 0, + .loader_id = 0, + .cookie_jar = undefined, + .notification = undefined, + }; +} const AdBlocker = @import("adblock/AdBlocker.zig"); // The Network every test client points at: only the fields a test actually @@ -3801,14 +3881,9 @@ fn testTransfer(arena: *lp.Arena) Transfer { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, }, .client = undefined, @@ -3942,6 +4017,61 @@ test "HttpClient: Fetch header overrides restore after one hop" { try testing.expectEqual(2, transfer.req_headers.items.len); } +test "HttpClient: Owner.siteForCookies" { + var top_url: [:0]const u8 = "http://attacker.example/attacker-nested"; + var top = testOwner(); + top.url = &top_url; + + var middle_url: [:0]const u8 = "http://victim.example/nested-middle"; + var middle = testOwner(); + middle.url = &middle_url; + middle.parent = ⊤ + + var inner_url: [:0]const u8 = "http://victim.example/inner"; + var inner = testOwner(); + inner.url = &inner_url; + inner.parent = &middle; + + // A worker has no site of its own; it takes its creating document's. + var worker = testOwner(); + worker.parent = &inner; + + // A top-level document is its own site. + try testing.expectEqual("http://attacker.example/attacker-nested", top.siteForCookies().url); + + // Cross-site with the top-level document: no site for cookies — for the + // directly-embedded frame and for the same-site-with-parent frame nested + // under it alike. + try testing.expectEqual(true, middle.siteForCookies() == .none); + try testing.expectEqual(true, inner.siteForCookies() == .none); + try testing.expectEqual(true, worker.siteForCookies() == .none); + + // A fully same-site chain (subdomains included) keeps its site. + top_url = "http://victim.example/"; + middle_url = "http://sub.victim.example/nested-middle"; + try testing.expectEqual("http://victim.example/inner", inner.siteForCookies().url); + try testing.expectEqual("http://sub.victim.example/nested-middle", middle.siteForCookies().url); + try testing.expectEqual("http://victim.example/inner", worker.siteForCookies().url); + + // about: documents inherit their creator's origin: transparent as an + // ancestor, and judged through their nearest real ancestor themselves. + middle_url = "about:blank"; + try testing.expectEqual("http://victim.example/inner", inner.siteForCookies().url); + try testing.expectEqual("http://victim.example/", middle.siteForCookies().url); + + top_url = "http://attacker.example/"; + try testing.expectEqual(true, inner.siteForCookies() == .none); + try testing.expectEqual("http://attacker.example/", middle.siteForCookies().url); + + // A worker under an about: document reads through it too. + worker.parent = &middle; + try testing.expectEqual("http://attacker.example/", worker.siteForCookies().url); + + // A top-level about:blank has nothing to read through. + top_url = "about:blank"; + try testing.expectEqual("about:blank", top.siteForCookies().url); +} + test "HttpClient: fulfillIntercepted survives a done_callback that tears down the owner" { // Regression: the fulfilled response's done_callback runs JS which // navigates / closes the page, re-entrantly killing the transfer @@ -3956,7 +4086,7 @@ test "HttpClient: fulfillIntercepted survives a done_callback that tears down th defer client.processGraveyard(); defer client.transfers.deinit(testing.allocator); - var owner: Owner = .init(undefined, undefined); + var owner = testOwner(); const Ctx = struct { client: *Client, @@ -3979,14 +4109,9 @@ test "HttpClient: fulfillIntercepted survives a done_callback that tears down th .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = &ctx, .done_callback = Ctx.doneCallback, @@ -4031,7 +4156,7 @@ test "HttpClient: kill during done_callback does not also fire shutdown_callback defer client.processGraveyard(); defer client.transfers.deinit(testing.allocator); - var owner: Owner = .init(undefined, undefined); + var owner = testOwner(); const Ctx = struct { client: *Client, @@ -4064,14 +4189,9 @@ test "HttpClient: kill during done_callback does not also fire shutdown_callback .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .xhr, - .notification = undefined, .shutdown_callback = Ctx.shutdownCallback, .ctx = &ctx, .done_callback = Ctx.doneCallback, @@ -4113,7 +4233,7 @@ test "HttpClient: kill during a non-terminal callback defers shutdown_callback" defer client.processGraveyard(); defer client.transfers.deinit(testing.allocator); - var owner: Owner = .init(undefined, undefined); + var owner = testOwner(); const Ctx = struct { client: *Client, @@ -4150,14 +4270,9 @@ test "HttpClient: kill during a non-terminal callback defers shutdown_callback" .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .xhr, - .notification = undefined, .shutdown_callback = Ctx.shutdownCallback, .ctx = &ctx, .header_callback = Ctx.headerCallback, @@ -4211,14 +4326,9 @@ test "HttpClient: aborting a robots-parked transfer unlinks it from the gate" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, }, .client = &client, @@ -4278,15 +4388,10 @@ test "HttpClient: fulfillIntercepted follows a 3xx redirect" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .POST, .url = "http://example.com/start", .body = "payload", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = undefined, }, @@ -4322,15 +4427,10 @@ test "HttpClient: fulfillIntercepted follows a 3xx redirect" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .POST, .url = "http://example.com/start", .body = "payload", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = undefined, }, @@ -4391,14 +4491,9 @@ test "HttpClient: fulfillIntercepted delivers a 3xx without a Location as the re .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = &ctx, .header_callback = Ctx.headerCallback, @@ -4438,7 +4533,7 @@ test "HttpClient: abortParked survives an error_callback that tears down the own defer client.processGraveyard(); defer client.transfers.deinit(testing.allocator); - var owner: Owner = .init(undefined, undefined); + var owner = testOwner(); const Ctx = struct { client: *Client, @@ -4459,14 +4554,9 @@ test "HttpClient: abortParked survives an error_callback that tears down the own .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = &ctx, .error_callback = Ctx.errorCallback, @@ -4514,7 +4604,7 @@ test "HttpClient: abort survives an error_callback that tears down the owner" { defer client.processGraveyard(); defer client.transfers.deinit(testing.allocator); - var owner: Owner = .init(undefined, undefined); + var owner = testOwner(); const Ctx = struct { client: *Client, @@ -4537,14 +4627,9 @@ test "HttpClient: abort survives an error_callback that tears down the owner" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .xhr, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = &ctx, .error_callback = Ctx.errorCallback, @@ -4574,14 +4659,9 @@ test "HttpClient: abort survives an error_callback that tears down the owner" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .xhr, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = &ctx, .error_callback = Ctx.errorCallback, @@ -4638,14 +4718,9 @@ test "HttpClient: throttled navigations wait for their per-host slot" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = url, - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = noopShutdown, .ctx = undefined, .throttle = throttle, diff --git a/src/network/RobotsGate.zig b/src/network/RobotsGate.zig index e1ad1784b..19a737ad9 100644 --- a/src/network/RobotsGate.zig +++ b/src/network/RobotsGate.zig @@ -105,8 +105,8 @@ fn fetchThenResume(self: *RobotsGate, robots_url: [:0]const u8, transfer: *Trans log.debug(.browser, "fetching robots.txt", .{ .robots_url = owned_url }); - // Only the parent's frame/loader ids (CDP correlation) and notification - // carry over — no cookies, credentials, headers, or timeout. + // Ownerless: no cookies, credentials, headers, or timeout. We attribute to + // the parent for CDP correlation const fetch_transfer = try client.newRequest(.{ .url = owned_url, .method = .GET, @@ -116,8 +116,6 @@ fn fetchThenResume(self: *RobotsGate, robots_url: [:0]const u8, transfer: *Trans .document_frame_id = transfer.req.document_frame_id, .loader_id = transfer.req.loader_id, .notification = transfer.req.notification, - .cookie_jar = null, - .cookie_origin = .{ .url = owned_url }, .ctx = robots_ctx, .header_callback = RobotsContext.headerCallback, .data_callback = RobotsContext.dataCallback, diff --git a/src/network/SingleFlight.zig b/src/network/SingleFlight.zig index d753cba7a..76deabb30 100644 --- a/src/network/SingleFlight.zig +++ b/src/network/SingleFlight.zig @@ -104,14 +104,9 @@ fn makeTestTransfer(arena: *lp.Arena, client: *HttpClient, id: u32) !*Transfer { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = HttpClient.noopShutdown, }, .client = client, diff --git a/src/network/WebBotAuth.zig b/src/network/WebBotAuth.zig index dabe7df77..9fc80cc89 100644 --- a/src/network/WebBotAuth.zig +++ b/src/network/WebBotAuth.zig @@ -242,14 +242,9 @@ test "signRequest: adds headers with correct names" { .arena = arena, .owner = null, .req = .{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "https://example.com/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .document, - .notification = undefined, .shutdown_callback = @import("HttpClient.zig").noopShutdown, }, .client = undefined, diff --git a/src/server/cdp/CDP.zig b/src/server/cdp/CDP.zig index 510ec3e67..65816abb6 100644 --- a/src/server/cdp/CDP.zig +++ b/src/server/cdp/CDP.zig @@ -1536,14 +1536,9 @@ test "cdp: syncRequest short-circuits after disconnect" { // installed. The latch check runs before any req field is read, so the // rest are placeholders. const transfer = try client.newRequest(.{ - .frame_id = 0, - .loader_id = 0, .method = .GET, .url = "http://127.0.0.1:9582/", - .cookie_jar = null, - .cookie_origin = .none, .resource_type = .fetch, - .notification = undefined, .shutdown_callback = HttpClient.noopShutdown, }, null); try testing.expectError(error.ClientDisconnected, transfer.submitSync()); diff --git a/src/server/cdp/domains/network.zig b/src/server/cdp/domains/network.zig index b1d83797c..80e2a22ad 100644 --- a/src/server/cdp/domains/network.zig +++ b/src/server/cdp/domains/network.zig @@ -565,7 +565,7 @@ pub const RequestWriter = struct { try SafeString.writeObjectField(jws, hdr.name); try jws.write(SafeString.wrap(hdr.value)); } - if (try request.getCookieString(transfer.arena.allocator())) |cookies| { + if (try transfer.getCookieString(transfer.arena.allocator())) |cookies| { try jws.objectField("Cookie"); try jws.write(cookies[0 .. cookies.len - 1]); } @@ -1173,8 +1173,6 @@ test "cdp.Network: setBlockedURLs blocks requests with inspector reason" { .loader_id = 1, .method = .GET, .url = "https://blocked.test/script.js", - .cookie_jar = null, - .cookie_origin = .{ .url = "https://blocked.test/" }, .resource_type = .script, .notification = bc.session.notification, .ctx = &error_context, @@ -1199,8 +1197,6 @@ test "cdp.Network: setBlockedURLs blocks requests with inspector reason" { .loader_id = 1, .method = .GET, .url = "http://127.0.0.1:9582/redirect-no-fragment", - .cookie_jar = null, - .cookie_origin = .{ .url = "http://127.0.0.1:9582/" }, .resource_type = .script, .notification = bc.session.notification, .ctx = &error_context, @@ -1240,8 +1236,6 @@ test "cdp.Network: POST body exposed as postData" { .method = .POST, .url = "http://127.0.0.1:9582/echo_body", .body = body, - .cookie_jar = null, - .cookie_origin = .{ .url = "http://127.0.0.1:9582/" }, .resource_type = .fetch, .notification = bc.session.notification, .shutdown_callback = HttpClient.noopShutdown, @@ -1505,8 +1499,6 @@ test "cdp.Network: redirect hop precedes Fetch pause and carries redirectRespons .loader_id = 7, .method = .GET, .url = start_url, - .cookie_jar = null, - .cookie_origin = .{ .url = start_url }, .resource_type = .script, .notification = bc.session.notification, .ctx = &callback_context, From ed88bd763f16c6aa5215411e229b7a64e6e9a673 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 2 Sep 2026 08:17:15 +0800 Subject: [PATCH 2/2] complete merge --- src/browser/webapi/storage/Cookie.zig | 23 +++++++++++------------ src/network/HttpClient.zig | 19 +++++++++++-------- src/server/cdp/domains/network.zig | 2 -- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/browser/webapi/storage/Cookie.zig b/src/browser/webapi/storage/Cookie.zig index 0bcc72ed5..d102646d2 100644 --- a/src/browser/webapi/storage/Cookie.zig +++ b/src/browser/webapi/storage/Cookie.zig @@ -451,8 +451,7 @@ pub fn appliesTo(self: *const Cookie, url: *const PreparedUri, same_site: bool, return true; } -// Kept distinct from a plain URL so "the initiator has no site for cookies" -// (.none) can't be confused with "there is no initiating document". +// RFC 6265bis "site for cookies" of a request's initiator. pub const SiteForCookies = union(enum) { // Matches no site, used for a frame whose ancestor chain contains a cross-site document. none, @@ -584,7 +583,7 @@ pub const Jar = struct { request_time: ?u64 = null, is_navigation: bool = true, prefix: ?[]const u8 = null, - origin_url: ?SiteForCookies = null, + origin_url: SiteForCookies, }; pub fn forRequest(self: *Jar, target_url: [:0]const u8, writer: anytype, opts: LookupOpts) !void { const target = PreparedUri.init(target_url); @@ -650,12 +649,11 @@ fn areCookiesEqual(a: *const Cookie, b: *const Cookie) bool { return true; } -pub fn areSameSite(maybe_origin_url: ?SiteForCookies, target_host: []const u8) bool { - const origin_url = switch (maybe_origin_url orelse return true) { - .none => return false, - .url => |url| url, +pub fn areSameSite(origin_url: SiteForCookies, target_host: []const u8) bool { + return switch (origin_url) { + .none => false, + .url => |url| areHostsSameSite(URL.getHostname(url), target_host), }; - return areHostsSameSite(URL.getHostname(origin_url), target_host); } pub fn areHostsSameSite(target_host: []const u8, origin_host: []const u8) bool { @@ -867,7 +865,7 @@ test "Jar: forRequest" { { // test with no cookies - try expectCookies("", &jar, test_url, .{ .is_http = true }); + try expectCookies("", &jar, test_url, .{ .origin_url = .{ .url = test_url }, .is_http = true }); } try jar.add(try Cookie.parse(testing.allocator, test_url, "global1=1"), now, true); @@ -881,7 +879,7 @@ test "Jar: forRequest" { try jar.add(try Cookie.parse(testing.allocator, url2, "domain1=9;domain=test.lightpanda.io"), now, true); // nothing fancy here - try expectCookies("global1=1; global2=2", &jar, test_url, .{ .is_http = true }); + try expectCookies("global1=1; global2=2", &jar, test_url, .{ .origin_url = .{ .url = test_url }, .is_http = true }); try expectCookies("global1=1; global2=2", &jar, test_url, .{ .origin_url = .{ .url = test_url }, .is_navigation = false, .is_http = true }); // We have a cookie where Domain=lightpanda.io @@ -1024,13 +1022,14 @@ test "Jar: forRequest SameSite=Strict on cross-site navigation" { .is_http = true, }); - // Browser-initiated navigation (origin_url=null) is treated as same-site. + // Browser-initiated navigation: the initiator is the destination itself. try expectCookies("sid=STRICT_COOKIE", &jar, "http://victim.example/transfer", .{ + .origin_url = .{ .url = "http://victim.example/transfer" }, .is_http = true, }); } -test "Jar: forRequest with a null site-for-cookies" { +test "Jar: forRequest with .none site-for-cookies" { const expectCookies = struct { fn expect(expected: []const u8, jar: *Jar, target_url: [:0]const u8, opts: Jar.LookupOpts) !void { var aw: std.Io.Writer.Allocating = .init(testing.allocator); diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index 71cd2fd84..de2a20005 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -613,15 +613,15 @@ pub fn newRequest(self: *Client, req: Request, owner: ?*Owner) anyerror!*Transfe if (owned.loader_id == 0) owned.loader_id = o.loader_id; if (owned.document_frame_id == null) owned.document_frame_id = o.document_frame_id; if (owned.notification == null) owned.notification = o.notification; - if (owned.cookie_origin == null) owned.cookie_origin = o.siteForCookies(); if (req.cookies) cookie_jar = o.cookie_jar; } - if (owned.cookie_origin) |cookie_origin| { - owned.cookie_origin = switch (cookie_origin) { - .none => .none, - .url => |url| .{ .url = try arena.dupeZ(u8, url) }, - }; - } + // Resolved onto the transfer; the request's copy is left null so + // nothing reads the caller's (possibly short-lived) url through it. + const cookie_origin: Cookie.SiteForCookies = switch (req.cookie_origin orelse if (owner) |o| o.siteForCookies() else .none) { + .none => .none, + .url => |url| .{ .url = try arena.dupeZ(u8, url) }, + }; + owned.cookie_origin = null; if (req.credentials) |c| { owned.credentials = try arena.dupeZ(u8, c); } @@ -639,6 +639,7 @@ pub fn newRequest(self: *Client, req: Request, owner: ?*Owner) anyerror!*Transfe t.* = .{ .req = owned, .cookie_jar = cookie_jar, + .cookie_origin = cookie_origin, .client = self, .arena = arena, .id = self.incrReqId(), @@ -2096,6 +2097,8 @@ pub const Transfer = struct { // The owner's jar, unless the request opted out of cookies. cookie_jar: ?*CookieJar = null, + // The site for SameSite checks: Request.cookie_origin, else the owner's. + cookie_origin: Cookie.SiteForCookies = .none, req_headers: std.ArrayList(RequestHeader) = .empty, @@ -2490,7 +2493,7 @@ pub const Transfer = struct { var aw: std.Io.Writer.Allocating = .init(arena); try jar.forRequest(req.url, &aw.writer, .{ .is_http = true, - .origin_url = req.cookie_origin, + .origin_url = self.cookie_origin, .is_navigation = req.resource_type == .document, }); if (aw.written().len == 0) { diff --git a/src/server/cdp/domains/network.zig b/src/server/cdp/domains/network.zig index 80e2a22ad..e257e84cd 100644 --- a/src/server/cdp/domains/network.zig +++ b/src/server/cdp/domains/network.zig @@ -1296,8 +1296,6 @@ const EchoDriver = struct { .method = .POST, .url = "http://127.0.0.1:9582/echo_body", .body = body, - .cookie_jar = null, - .cookie_origin = .{ .url = "http://127.0.0.1:9582/" }, .resource_type = .fetch, .notification = bc.session.notification, .ctx = &driver,