From 0c3d5573f084bfbf9b818467418e9092caf580d9 Mon Sep 17 00:00:00 2001 From: Halil Durak Date: Wed, 29 Apr 2026 17:40:48 +0300 Subject: [PATCH] `Cookie`: don't allow JS context to mutate HttpOnly cookies Changes function signature for `Jar.add` in order to do this, not sure if we should have separate functions for that or comptime-if sufficient. --- src/browser/webapi/HTMLDocument.zig | 2 +- src/browser/webapi/storage/Cookie.zig | 87 ++++++++++++++++++--------- src/cdp/domains/storage.zig | 2 +- src/cookies.zig | 2 +- 4 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/browser/webapi/HTMLDocument.zig b/src/browser/webapi/HTMLDocument.zig index c4ed0c92..41782cc8 100644 --- a/src/browser/webapi/HTMLDocument.zig +++ b/src/browser/webapi/HTMLDocument.zig @@ -255,7 +255,7 @@ pub fn setCookie(_: *HTMLDocument, cookie_str: []const u8, frame: *Frame) ![]con c.deinit(); return ""; // HttpOnly cookies cannot be set from JS } - try frame._session.cookie_jar.add(c, std.time.timestamp()); + try frame._session.cookie_jar.add(c, std.time.timestamp(), false); return cookie_str; } diff --git a/src/browser/webapi/storage/Cookie.zig b/src/browser/webapi/storage/Cookie.zig index 9e3d3bc5..bed59d01 100644 --- a/src/browser/webapi/storage/Cookie.zig +++ b/src/browser/webapi/storage/Cookie.zig @@ -477,6 +477,8 @@ pub const Jar = struct { self: *Jar, cookie: Cookie, request_time: i64, + /// Checks if addition comes from HTTP request or JS context. + comptime is_http: bool, ) !void { const is_expired = isCookieExpired(&cookie, request_time); defer if (is_expired) { @@ -491,15 +493,25 @@ pub const Jar = struct { } for (self.cookies.items, 0..) |*c, i| { - if (areCookiesEqual(&cookie, c)) { - c.deinit(); - if (is_expired) { - _ = self.cookies.swapRemove(i); - } else { - self.cookies.items[i] = cookie; - } + // We're only looking for the equal one. + if (areCookiesEqual(&cookie, c) == false) { + continue; + } + + // RFC 6265bis 5.7.2: a non-HTTP API (e.g. document.cookie) must + // not replace an HttpOnly cookie. + if (c.http_only and is_http == false) { + if (is_expired == false) cookie.deinit(); return; } + + c.deinit(); + if (is_expired) { + _ = self.cookies.swapRemove(i); + } else { + self.cookies.items[i] = cookie; + } + return; } if (!is_expired) { @@ -563,7 +575,7 @@ pub const Jar = struct { }; const now = std.time.timestamp(); - try self.add(c, now); + try self.add(c, now, true); } fn writeCookie(cookie: *const Cookie, writer: anytype) !void { @@ -685,31 +697,50 @@ test "Jar: add" { defer jar.deinit(); try expectCookies(&.{}, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9000;Max-Age=0"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9000;Max-Age=0"), now, true); try expectCookies(&.{}, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9000"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9000"), now, true); try expectCookies(&.{.{ "over", "9000" }}, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9000!!"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9000!!"), now, true); try expectCookies(&.{.{ "over", "9000!!" }}, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "spice=flow"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "spice=flow"), now, true); try expectCookies(&.{ .{ "over", "9000!!" }, .{ "spice", "flow" } }, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "spice=flows;Path=/"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "spice=flows;Path=/"), now, true); try expectCookies(&.{ .{ "over", "9000!!" }, .{ "spice", "flows" } }, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9001;Path=/other"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9001;Path=/other"), now, true); try expectCookies(&.{ .{ "over", "9000!!" }, .{ "spice", "flows" }, .{ "over", "9001" } }, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9002;Path=/;Domain=lightpanda.io"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "over=9002;Path=/;Domain=lightpanda.io"), now, true); try expectCookies(&.{ .{ "over", "9000!!" }, .{ "spice", "flows" }, .{ "over", "9001" }, .{ "over", "9002" } }, jar); - try jar.add(try Cookie.parse(testing.allocator, test_url, "over=x;Path=/other;Max-Age=-200"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "over=x;Path=/other;Max-Age=-200"), now, true); try expectCookies(&.{ .{ "over", "9000!!" }, .{ "spice", "flows" }, .{ "over", "9002" } }, jar); } +test "Jar: non-HTTP add must not replace or duplicate an HttpOnly cookie" { + const now = std.time.timestamp(); + + var jar = Jar.init(testing.allocator); + defer jar.deinit(); + + try jar.add(try Cookie.parse(testing.allocator, test_url, "session=REAL;Path=/;HttpOnly"), now, true); + try testing.expectEqual(@as(usize, 1), jar.cookies.items.len); + + try jar.add(try Cookie.parse(testing.allocator, test_url, "session=ATTACKER;Path=/"), now, false); + try testing.expectEqual(@as(usize, 1), jar.cookies.items.len); + try testing.expectEqual("REAL", jar.cookies.items[0].value); + try testing.expectEqual(true, jar.cookies.items[0].http_only); + + try jar.add(try Cookie.parse(testing.allocator, test_url, "session=REFRESHED;Path=/;HttpOnly"), now, true); + try testing.expectEqual(@as(usize, 1), jar.cookies.items.len); + try testing.expectEqual("REFRESHED", jar.cookies.items[0].value); +} + test "Jar: add limit" { var jar = Jar.init(testing.allocator); defer jar.deinit(); @@ -724,7 +755,7 @@ test "Jar: add limit" { .path = "/", .expires = null, .value = "v" ** 4096 ++ "v", - }, now)); + }, now, true)); // generate unique names. const names = comptime blk: { @@ -748,7 +779,7 @@ test "Jar: add limit" { .value = "v", }; - try jar.add(c, now); + try jar.add(c, now, true); } try testing.expectError(error.CookieJarQuotaExceeded, jar.add(.{ @@ -758,7 +789,7 @@ test "Jar: add limit" { .path = "/", .expires = null, .value = "v", - }, now)); + }, now, true)); } test "Jar: forRequest" { @@ -783,15 +814,15 @@ test "Jar: forRequest" { try expectCookies("", &jar, test_url, .{ .is_http = true }); } - try jar.add(try Cookie.parse(testing.allocator, test_url, "global1=1"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "global2=2;Max-Age=30;domain=lightpanda.io"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "path1=3;Path=/about"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "path2=4;Path=/docs/"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "secure=5;Secure"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "sitenone=6;SameSite=None;Path=/x/;Secure"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "sitelax=7;SameSite=Lax;Path=/x/"), now); - try jar.add(try Cookie.parse(testing.allocator, test_url, "sitestrict=8;SameSite=Strict;Path=/x/"), now); - try jar.add(try Cookie.parse(testing.allocator, url2, "domain1=9;domain=test.lightpanda.io"), now); + try jar.add(try Cookie.parse(testing.allocator, test_url, "global1=1"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "global2=2;Max-Age=30;domain=lightpanda.io"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "path1=3;Path=/about"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "path2=4;Path=/docs/"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "secure=5;Secure"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "sitenone=6;SameSite=None;Path=/x/;Secure"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "sitelax=7;SameSite=Lax;Path=/x/"), now, true); + try jar.add(try Cookie.parse(testing.allocator, test_url, "sitestrict=8;SameSite=Strict;Path=/x/"), now, true); + 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 }); diff --git a/src/cdp/domains/storage.zig b/src/cdp/domains/storage.zig index 4d185f6c..65a7509e 100644 --- a/src/cdp/domains/storage.zig +++ b/src/cdp/domains/storage.zig @@ -167,7 +167,7 @@ pub fn setCdpCookie(cookie_jar: *CookieJar, param: CdpCookie) !void { .None => .none, }, }; - try cookie_jar.add(cookie, std.time.timestamp()); + try cookie_jar.add(cookie, std.time.timestamp(), true); } pub const CookieWriter = struct { diff --git a/src/cookies.zig b/src/cookies.zig index 4805c4f5..4340a6ef 100644 --- a/src/cookies.zig +++ b/src/cookies.zig @@ -76,7 +76,7 @@ fn _loadFromFile(session: *Session, path: []const u8) !void { .same_site = jc.sameSite, }; - jar.add(cookie, now) catch |err| { + jar.add(cookie, now, true) catch |err| { cookie.deinit(); log.warn(.app, "invalid cookie", .{ .name = jc.name, .err = err }); continue;