cdp: setCookie sameSite parity with Chrome

src/browser/webapi/storage/Cookie.zig

@@ -43,9 +43,10 @@ expires: ?f64,
 secure: bool = false,
 http_only: bool = false,
 same_site: SameSite = .none,
-// True when Set-Cookie carried no SameSite attribute: the cookie is Lax
-// through the "Default" enforcement mode (RFC 6265bis 5.6.7.1), which
-// makes it eligible for "Lax-allowing-unsafe", see appliesTo.
+// True when no SameSite was given, by the Set-Cookie header, the CDP
+// driver or the cookie file: the cookie is Lax through the "Default"
+// enforcement mode (RFC 6265bis 5.6.7.1), which makes it eligible for
+// "Lax-allowing-unsafe", see appliesTo.
 same_site_default: bool = false,
 // Seconds since the epoch. Set by Jar.add, and inherited from the cookie it
 // replaces, so a site re-setting a cookie on every response can't keep it
@@ -57,13 +58,20 @@ pub const SameSite = enum {
     lax,
     none,
 
-    pub fn parse(value: []const u8) error{InvalidSameSite}!?SameSite {
-        if (std.ascii.eqlIgnoreCase(value, "strict")) return .strict;
-        if (std.ascii.eqlIgnoreCase(value, "lax")) return .lax;
-        if (std.ascii.eqlIgnoreCase(value, "none")) return .none;
-        if (std.ascii.eqlIgnoreCase(value, "no_restriction")) return .none;
-        if (std.ascii.eqlIgnoreCase(value, "unspecified")) return null;
-        return error.InvalidSameSite;
+    // The SameSite a Set-Cookie attribute or a cookie file spells out:
+    // Strict, Lax or None, case-insensitive (RFC 6265bis 5.6.7; the file
+    // has both saveToFile's lowercase tags and CDP's casing). Anything
+    // else, including no value at all, "unspecified" and the
+    // chrome.cookies "no_restriction", is not a value: the cookie is
+    // unspecified and Lax by default, see `same_site_default`. CDP takes
+    // Chrome's exact spelling only, see `parseSameSite` in
+    // server/cdp/domains/storage.zig.
+    pub fn parse(value: ?[]const u8) ?SameSite {
+        const raw = value orelse return null;
+        if (std.ascii.eqlIgnoreCase(raw, "strict")) return .strict;
+        if (std.ascii.eqlIgnoreCase(raw, "lax")) return .lax;
+        if (std.ascii.eqlIgnoreCase(raw, "none")) return .none;
+        return null;
     }
 };
 
@@ -155,12 +163,7 @@ pub fn parse(allocator: Allocator, url: [:0]const u8, str: []const u8) !Cookie {
             .@"max-age" => max_age = std.fmt.parseInt(i64, value, 10) catch continue,
             .expires => expires = value,
             .httponly => http_only = true,
-            .samesite => {
-                if (value.len > scrap.len) {
-                    continue;
-                }
-                same_site = std.meta.stringToEnum(Cookie.SameSite, std.ascii.lowerString(&scrap, value)) orelse continue;
-            },
+            .samesite => same_site = SameSite.parse(value) orelse continue,
         }
     }
 

src/cookies.zig

@@ -20,10 +20,12 @@ const Session = @import("browser/Session.zig");
 const Cookie = @import("browser/webapi/storage/Cookie.zig");
 
 const log = lp.log;
+const Allocator = std.mem.Allocator;
 
 /// Load cookies from a JSON file into the cookie jar.
 /// The file format is an array of objects with: name, value, domain, path,
-/// expires (optional, float), secure (optional, bool), httpOnly (optional, bool).
+/// expires (optional, float), secure (optional, bool), httpOnly (optional, bool),
+/// sameSite (optional, Strict/Lax/None).
 /// This matches the CDP Network.Cookie format used by Puppeteer and Playwright.
 pub fn loadFromFile(session: *Session, path: []const u8) void {
     _loadFromFile(session, path) catch |err| {
@@ -68,27 +70,7 @@ fn _loadFromFile(session: *Session, path: []const u8) !void {
 
     var loaded: usize = 0;
     for (json_cookies) |jc| {
-        var cookie_arena = std.heap.ArenaAllocator.init(jar.allocator);
-        errdefer cookie_arena.deinit();
-
-        const a = cookie_arena.allocator();
-        const name = try a.dupe(u8, jc.name);
-        const value = try a.dupe(u8, jc.value);
-        const domain = try a.dupe(u8, jc.domain);
-        const cookie_path = if (jc.path) |p| try a.dupe(u8, p) else "/";
-
-        const cookie = Cookie{
-            .arena = cookie_arena,
-            .name = name,
-            .value = value,
-            .domain = domain,
-            .path = cookie_path,
-            .expires = jc.expires,
-            .secure = jc.secure orelse false,
-            .http_only = jc.httpOnly orelse false,
-            .same_site = parseJsonSameSite(jc.sameSite),
-        };
-
+        const cookie = try jc.toCookie(jar.allocator);
         jar.add(cookie, now, true) catch |err| {
             log.warn(.app, "invalid cookie", .{ .name = jc.name, .err = err });
             continue;
@@ -114,8 +96,13 @@ fn _saveToFile(jar: *Cookie.Jar, path: []const u8) !void {
 
     var buf: [8192]u8 = undefined;
     var writer = file.writer(lp.io, &buf);
-    const w = &writer.interface;
+    try writeJson(jar, &writer.interface);
+    try writer.end();
 
+    log.info(.app, "Cookie.saveToFile", .{ .path = path, .count = jar.cookies.items.len });
+}
+
+fn writeJson(jar: *const Cookie.Jar, w: *std.Io.Writer) !void {
     try w.writeByte('[');
     for (jar.cookies.items, 0..) |c, i| {
         if (i > 0) {
@@ -131,17 +118,16 @@ fn _saveToFile(jar: *Cookie.Jar, path: []const u8) !void {
             .expires = c.expires,
             .secure = c.secure,
             .httpOnly = c.http_only,
-            .sameSite = @tagName(c.same_site),
-        }, .{}, w);
+            // Left out for an unspecified cookie, as Network.getCookies
+            // does, so that it's still Lax by default once loaded back.
+            .sameSite = if (c.same_site_default) null else @tagName(c.same_site),
+        }, .{ .emit_null_optional_fields = false }, w);
     }
 
     if (jar.cookies.items.len > 0) {
         try w.writeByte('\n');
     }
     try w.writeAll("]\n");
-    try writer.end();
-
-    log.info(.app, "Cookie.saveToFile", .{ .path = path, .count = jar.cookies.items.len });
 }
 
 const JsonCookie = struct {
@@ -153,12 +139,35 @@ const JsonCookie = struct {
     secure: ?bool = null,
     httpOnly: ?bool = null,
     sameSite: ?[]const u8 = null,
-};
 
-fn parseJsonSameSite(value: ?[]const u8) Cookie.SameSite {
-    const raw = value orelse return .lax;
-    return (Cookie.SameSite.parse(raw) catch .lax) orelse .lax;
-}
+    fn toCookie(self: JsonCookie, allocator: Allocator) !Cookie {
+        var arena = std.heap.ArenaAllocator.init(allocator);
+        errdefer arena.deinit();
+        const a = arena.allocator();
+
+        // Allocate before the struct literal copies `arena` into the result.
+        const name = try a.dupe(u8, self.name);
+        const value = try a.dupe(u8, self.value);
+        const domain = try a.dupe(u8, self.domain);
+        const path = if (self.path) |p| try a.dupe(u8, p) else "/";
+
+        // A missing or unrecognised sameSite leaves the cookie unspecified.
+        const same_site = Cookie.SameSite.parse(self.sameSite);
+
+        return .{
+            .arena = arena,
+            .name = name,
+            .value = value,
+            .domain = domain,
+            .path = path,
+            .expires = self.expires,
+            .secure = self.secure orelse false,
+            .http_only = self.httpOnly orelse false,
+            .same_site = same_site orelse .lax,
+            .same_site_default = same_site == null,
+        };
+    }
+};
 
 /// Netscape cookie file format parser with `#HttpOnly_` addition from curl.
 /// https://docs.cyotek.com/cyowcopy/1.10/netscapecookieformat.html
@@ -418,5 +427,7 @@ test "cookies: load JSON accepts CDP SameSite casing" {
         .{ .ignore_unknown_fields = true },
     );
 
-    try std.testing.expectEqual(Cookie.SameSite.lax, parseJsonSameSite(parsed[0].sameSite));
+    const cookie = try parsed[0].toCookie(std.testing.allocator);
+    defer cookie.deinit();
+    try std.testing.expectEqual(Cookie.SameSite.lax, cookie.same_site);
 }

src/server/cdp/domains/network.zig

@@ -291,12 +291,9 @@ fn setCookie(cmd: *CDP.Command) !void {
     )) orelse return error.InvalidParams;
 
     const bc = cmd.browser_context orelse return error.BrowserContextNotLoaded;
-    CdpStorage.setCdpCookie(&bc.session.cookie_jar, params) catch |err| switch (err) {
-        error.InvalidSameSite => return CdpStorage.invalidSameSiteError(cmd, params.sameSite),
-        else => return err,
-    };
+    const stored = try CdpStorage.setCdpCookies(&bc.session.cookie_jar, &.{params});
 
-    try cmd.sendResult(.{ .success = true }, .{});
+    try cmd.sendResult(.{ .success = stored == 1 }, .{});
 }
 
 fn setCookies(cmd: *CDP.Command) !void {
@@ -305,12 +302,7 @@ fn setCookies(cmd: *CDP.Command) !void {
     })) orelse return error.InvalidParams;
 
     const bc = cmd.browser_context orelse return error.BrowserContextNotLoaded;
-    for (params.cookies) |param| {
-        CdpStorage.setCdpCookie(&bc.session.cookie_jar, param) catch |err| switch (err) {
-            error.InvalidSameSite => return CdpStorage.invalidSameSiteError(cmd, param.sameSite),
-            else => return err,
-        };
-    }
+    _ = try CdpStorage.setCdpCookies(&bc.session.cookie_jar, params.cookies);
 
     try cmd.sendResult(null, .{});
 }
@@ -980,57 +972,6 @@ test "cdp.Network: cookies" {
     try ctx.expectSentResult(.{ .cookies = &[_]ResCookie{} }, .{ .id = 10 });
 }
 
-test "cdp.Network: setCookie accepts the sameSite spellings drivers send" {
-    // Issue #3453: cookies bridged from chrome.cookies / other tooling come
-    // with lowercase or `no_restriction` sameSite values. Accept them on
-    // input; getCookies keeps reporting the canonical CDP spelling.
-    const ResCookie = CdpStorage.ResCookie;
-
-    var ctx = try testing.context();
-    defer ctx.deinit();
-    _ = try ctx.loadBrowserContext(.{ .id = "BID-SS" });
-
-    try ctx.processMessage(
-        \\{"id":1,"method":"Network.setCookie","params":{"name":"a","value":"1","url":"https://example.com/","sameSite":"lax"}}
-    );
-    try ctx.expectSentResult(.{ .success = true }, .{ .id = 1 });
-
-    try ctx.processMessage(
-        \\{"id":2,"method":"Network.setCookies","params":{"cookies":[
-        \\  {"name":"b","value":"2","url":"https://example.com/","sameSite":"no_restriction"},
-        \\  {"name":"c","value":"3","url":"https://example.com/","sameSite":"STRICT"},
-        \\  {"name":"d","value":"4","url":"https://example.com/","sameSite":"unspecified"}
-        \\]}}
-    );
-    try ctx.expectSentResult(null, .{ .id = 2 });
-
-    try ctx.processMessage(.{
-        .id = 3,
-        .method = "Network.getAllCookies",
-    });
-    try ctx.expectSentResult(.{
-        .cookies = &[_]ResCookie{
-            .{ .name = "a", .value = "1", .domain = "example.com", .size = 2, .secure = true, .sameSite = "Lax" },
-            // `no_restriction` is the chrome.cookies spelling of None.
-            .{ .name = "b", .value = "2", .domain = "example.com", .size = 2, .secure = true, .sameSite = "None" },
-            .{ .name = "c", .value = "3", .domain = "example.com", .size = 2, .secure = true, .sameSite = "Strict" },
-            // `unspecified` means no SameSite was set, so it takes the default.
-            .{ .name = "d", .value = "4", .domain = "example.com", .size = 2, .secure = true, .sameSite = "Lax" },
-        },
-    }, .{ .id = 3 });
-}
-
-test "cdp.Network: setCookie rejects an unknown sameSite by name" {
-    var ctx = try testing.context();
-    defer ctx.deinit();
-    _ = try ctx.loadBrowserContext(.{ .id = "BID-SE" });
-
-    try ctx.processMessage(
-        \\{"id":1,"method":"Network.setCookie","params":{"name":"a","value":"1","url":"https://example.com/","sameSite":"sometimes"}}
-    );
-    try ctx.expectSentError(-31998, "Invalid value 'sometimes' for 'sameSite'. Accepted (case-insensitive): Strict, Lax, None, no_restriction, unspecified", .{ .id = 1 });
-}
-
 test "cdp.Network: clearBrowserCookies accepts empty params object" {
     const CdpCookie = CdpStorage.CdpCookie;
     const ResCookie = CdpStorage.ResCookie;

src/server/cdp/domains/storage.zig

@@ -24,6 +24,7 @@ const URL = @import("../../../browser/URL.zig");
 const Cookie = @import("../../../browser/webapi/storage/storage.zig").Cookie;
 
 const log = lp.log;
+const Allocator = std.mem.Allocator;
 const CookieJar = Cookie.Jar;
 pub const PreparedUri = Cookie.PreparedUri;
 
@@ -85,24 +86,11 @@ fn setCookies(cmd: *CDP.Command) !void {
         }
     }
 
-    for (params.cookies) |param| {
-        setCdpCookie(&bc.session.cookie_jar, param) catch |err| switch (err) {
-            error.InvalidSameSite => return invalidSameSiteError(cmd, param.sameSite),
-            else => return err,
-        };
-    }
+    _ = try setCdpCookies(&bc.session.cookie_jar, params.cookies);
 
     try cmd.sendResult(null, .{});
 }
 
-pub fn invalidSameSiteError(cmd: *CDP.Command, value: []const u8) !void {
-    const message = try std.fmt.allocPrint(
-        cmd.arena,
-        "Invalid value '{s}' for 'sameSite'. Accepted (case-insensitive): Strict, Lax, None, no_restriction, unspecified",
-        .{value},
-    );
-    return cmd.sendError(-31998, message, .{});
-}
 const CookiePriority = enum {
     Low,
     Medium,
@@ -127,7 +115,7 @@ pub const CdpCookie = struct {
     path: ?[:0]const u8 = null,
     secure: ?bool = null, // default: https://www.rfc-editor.org/rfc/rfc6265#section-5.3
     httpOnly: bool = false, // default: https://www.rfc-editor.org/rfc/rfc6265#section-5.3
-    sameSite: []const u8 = "None", // default: https://datatracker.ietf.org/doc/html/draft-west-first-party-cookies
+    sameSite: ?[]const u8 = null, // Strict, Lax or None; anything else is unspecified, see parseSameSite
     expires: ?f64 = null, // -1? says google
     priority: CookiePriority = .Medium, // default: https://datatracker.ietf.org/doc/html/draft-west-cookie-priority-00
     sameParty: ?bool = null,
@@ -136,46 +124,93 @@ pub const CdpCookie = struct {
     partitionKey: ?CookiePartitionKey = null,
 };
 
-pub fn setCdpCookie(cookie_jar: *CookieJar, param: CdpCookie) !void {
+/// Network.setCookie, Network.setCookies and Storage.setCookies. Every
+/// cookie is built before any is added: an entry `buildCdpCookie` refuses
+/// in the middle of a batch leaves the jar untouched, as Chrome's
+/// SetCookies does. `Jar.add`'s own checks can still stop a batch part-way.
+/// Returns how many cookies were stored.
+pub fn setCdpCookies(cookie_jar: *CookieJar, params: []const CdpCookie) !usize {
+    var cookies = try std.ArrayList(Cookie).initCapacity(cookie_jar.allocator, params.len);
+    defer cookies.deinit(cookie_jar.allocator);
+
+    // A cookie handed to `Jar.add` is its to free, stored or not; the ones
+    // we still hold when something fails are ours.
+    var added: usize = 0;
+    errdefer for (cookies.items[added..]) |*cookie| cookie.deinit();
+
+    for (params) |param| {
+        cookies.appendAssumeCapacity(try buildCdpCookie(cookie_jar.allocator, param));
+    }
+
+    const now = lp.datetime.timestamp(.real);
+    var stored: usize = 0;
+    for (cookies.items) |cookie| {
+        added += 1;
+        if (cookie.same_site == .none and !cookie.secure) {
+            // Chrome's store refuses SameSite=None without Secure, as
+            // `Cookie.parse` does for a Set-Cookie.
+            cookie.deinit();
+            continue;
+        }
+        try cookie_jar.add(cookie, now, true);
+        stored += 1;
+    }
+    return stored;
+}
+
+fn buildCdpCookie(allocator: Allocator, param: CdpCookie) !Cookie {
     // Silently ignore partitionKey since we don't support partitioned cookies (CHIPS).
     // This allows Puppeteer's frame.setCookie() to work, which may send cookies with
     // partitionKey as part of its cookie-setting workflow.
     if (param.partitionKey != null) {
-        log.warn(.not_implemented, "partition key", .{ .src = "setCdpCookie" });
+        log.warn(.not_implemented, "partition key", .{ .src = "buildCdpCookie" });
     }
     // Still reject unsupported features
     if (param.priority != .Medium or param.sameParty != null or param.sourceScheme != null) {
         return error.NotImplemented;
     }
 
-    const same_site = (try Cookie.SameSite.parse(param.sameSite)) orelse .lax;
+    // NOTE: The param.url can affect the default domain, (NOT path), secure, source port, and source scheme.
+    const secure = if (param.secure) |s| s else if (param.url) |url| URL.isSecure(url) else false;
 
-    // The errdefer only protects construction failures. Once we `break :blk`
-    // with the Cookie value, `Jar.add` owns its lifetime.
-    const cookie = blk: {
-        var arena = std.heap.ArenaAllocator.init(cookie_jar.allocator);
-        errdefer arena.deinit();
-        const a = arena.allocator();
+    const same_site = parseSameSite(param.sameSite);
 
-        // NOTE: The param.url can affect the default domain, (NOT path), secure, source port, and source scheme.
-        const domain = try Cookie.parseDomain(a, param.url, param.domain);
-        const path = if (param.path == null) "/" else try Cookie.parsePath(a, null, param.path);
+    var arena = std.heap.ArenaAllocator.init(allocator);
+    errdefer arena.deinit();
+    const a = arena.allocator();
 
-        const secure = if (param.secure) |s| s else if (param.url) |url| URL.isSecure(url) else false;
+    // Allocate before the struct literal copies `arena` into the result.
+    const name = try a.dupe(u8, param.name);
+    const value = try a.dupe(u8, param.value);
+    const domain = try Cookie.parseDomain(a, param.url, param.domain);
+    const path = if (param.path == null) "/" else try Cookie.parsePath(a, null, param.path);
 
-        break :blk Cookie{
-            .arena = arena,
-            .name = try a.dupe(u8, param.name),
-            .value = try a.dupe(u8, param.value),
-            .path = path,
-            .domain = domain,
-            .expires = param.expires,
-            .secure = secure,
-            .http_only = param.httpOnly,
-            .same_site = same_site,
-        };
+    return .{
+        .arena = arena,
+        .name = name,
+        .value = value,
+        .path = path,
+        .domain = domain,
+        .expires = param.expires,
+        .secure = secure,
+        .http_only = param.httpOnly,
+        .same_site = same_site orelse .lax,
+        .same_site_default = same_site == null,
+    };
+}
+
+// Chrome's MakeCookieFromProtocolValues takes CDP's exact Strict, Lax or
+// None. Anything else, "lax" included, or no value leaves the cookie
+// unspecified: Lax by default with the Lax-allowing-unsafe window, like a
+// Set-Cookie without the attribute (`Cookie.parse`, which, unlike CDP, is
+// case-insensitive).
+fn parseSameSite(value: ?[]const u8) ?Cookie.SameSite {
+    const same_site = std.meta.stringToEnum(enum { Strict, Lax, None }, value orelse return null) orelse return null;
+    return switch (same_site) {
+        .Strict => .strict,
+        .Lax => .lax,
+        .None => .none,
     };
-    try cookie_jar.add(cookie, lp.datetime.timestamp(.real), true);
 }
 
 pub const CookieWriter = struct {
@@ -239,11 +274,15 @@ fn writeCookie(cookie: *const Cookie, w: anytype) !void {
         try w.objectField("session");
         try w.write(cookie.expires == null);
 
-        try w.objectField("sameSite");
-        switch (cookie.same_site) {
-            .none => try w.write("None"),
-            .lax => try w.write("Lax"),
-            .strict => try w.write("Strict"),
+        // Chrome's BuildCookie reports an explicit Strict/Lax/None only; a
+        // cookie that is Lax by default has no sameSite.
+        if (!cookie.same_site_default) {
+            try w.objectField("sameSite");
+            switch (cookie.same_site) {
+                .none => try w.write("None"),
+                .lax => try w.write("Lax"),
+                .strict => try w.write("Strict"),
+            }
         }
 
         // TODO experimentals
@@ -317,5 +356,5 @@ pub const ResCookie = struct {
     size: usize = 0,
     httpOnly: bool = false,
     secure: bool = false,
-    sameSite: []const u8 = "None",
+    sameSite: ?[]const u8 = null,
 };