Add more cookie tests

Fix trimming and incorrect early loop termination when expiring cookies.
2026-03-22 04:34:44 +00:00 · 2026-02-20 20:03:18 +08:00
parent 279f2dd633
commit c823b8d7ae
3 changed files with 114 additions and 14 deletions
--- a/src/browser/tests/document/document.html
+++ b/src/browser/tests/document/document.html
@@ -1,5 +1,6 @@
 <!DOCTYPE html>
 <head id="the_head">
+  <meta charset="UTF-8">
  <title>Test Document Title</title>
  <script src="../testing.js"></script>
 </head>
@@ -176,15 +177,111 @@
  testing.expectEqual(initialLength, anchors.length);
 </script>

-<script id=cookie>
-  testing.expectEqual('', document.cookie);
-  document.cookie = 'name=Oeschger;';
-  document.cookie = 'favorite_food=tripe;';
+<script id=cookie_basic>
+  // Basic cookie operations
+  document.cookie = 'testbasic1=Oeschger';
+  testing.expectEqual(true, document.cookie.includes('testbasic1=Oeschger'));

-  testing.expectEqual('name=Oeschger; favorite_food=tripe', document.cookie);
-  // "" should be returned, but the framework overrules it atm
+  document.cookie = 'testbasic2=tripe';
+  testing.expectEqual(true, document.cookie.includes('testbasic1=Oeschger'));
+  testing.expectEqual(true, document.cookie.includes('testbasic2=tripe'));
+
+  // HttpOnly should be ignored from JavaScript
+  const beforeHttp = document.cookie;
  document.cookie = 'IgnoreMy=Ghost; HttpOnly';
-  testing.expectEqual('name=Oeschger; favorite_food=tripe', document.cookie);
+  testing.expectEqual(false, document.cookie.includes('IgnoreMy=Ghost'));
+
+  // Clean up
+  document.cookie = 'testbasic1=; Max-Age=0';
+  document.cookie = 'testbasic2=; Max-Age=0';
+</script>
+
+<script id=cookie_special_chars>
+  // Test special characters in cookie values
+  document.cookie = 'testspaces=hello world';
+  testing.expectEqual(true, document.cookie.includes('testspaces=hello world'));
+  document.cookie = 'testspaces=; Max-Age=0';
+
+  // Test various allowed special characters
+  document.cookie = 'testspecial=!#$%&\'()*+-./';
+  testing.expectEqual(true, document.cookie.includes('testspecial='));
+  document.cookie = 'testspecial=; Max-Age=0';
+
+  // Semicolon terminates the cookie value
+  document.cookie = 'testsemi=before;after';
+  testing.expectEqual(true, document.cookie.includes('testsemi=before'));
+  testing.expectEqual(false, document.cookie.includes('after'));
+  document.cookie = 'testsemi=; Max-Age=0';
+</script>
+
+<script id=cookie_empty_name>
+  // Cookie with empty name (just a value)
+  document.cookie = 'teststandalone';
+  testing.expectEqual(true, document.cookie.includes('teststandalone'));
+  document.cookie = 'teststandalone; Max-Age=0';
+</script>
+
+<script id=cookie_whitespace>
+  // Names and values should be trimmed
+  document.cookie = '  testtrim  =  trimmed_value  ';
+  testing.expectEqual(true, document.cookie.includes('testtrim=trimmed_value'));
+  document.cookie = 'testtrim=; Max-Age=0';
+</script>
+
+<script id=cookie_max_age>
+  // Max-Age=0 should immediately delete
+  document.cookie = 'testtemp0=value; Max-Age=0';
+  testing.expectEqual(false, document.cookie.includes('testtemp0=value'));
+
+  // Negative Max-Age should also delete
+  document.cookie = 'testinstant=value';
+  testing.expectEqual(true, document.cookie.includes('testinstant=value'));
+  document.cookie = 'testinstant=value; Max-Age=-1';
+  testing.expectEqual(false, document.cookie.includes('testinstant=value'));
+
+  // Positive Max-Age should keep cookie
+  document.cookie = 'testkept=value; Max-Age=3600';
+  testing.expectEqual(true, document.cookie.includes('testkept=value'));
+  document.cookie = 'testkept=; Max-Age=0';
+</script>
+
+<script id=cookie_overwrite>
+  // Setting a cookie with the same name should overwrite
+  document.cookie = 'testoverwrite=first';
+  testing.expectEqual(true, document.cookie.includes('testoverwrite=first'));
+
+  document.cookie = 'testoverwrite=second';
+  testing.expectEqual(true, document.cookie.includes('testoverwrite=second'));
+  testing.expectEqual(false, document.cookie.includes('testoverwrite=first'));
+
+  document.cookie = 'testoverwrite=; Max-Age=0';
+</script>
+
+<script id=cookie_path>
+  // Path attribute
+  document.cookie = 'testpath1=value; Path=/';
+  testing.expectEqual(true, document.cookie.includes('testpath1=value'));
+
+  // Different path cookie should coexist
+  document.cookie = 'testpath2=value2; Path=/src';
+  testing.expectEqual(true, document.cookie.includes('testpath1=value'));
+
+  document.cookie = 'testpath1=; Max-Age=0; Path=/';
+  document.cookie = 'testpath2=; Max-Age=0; Path=/src';
+</script>
+
+<script id=cookie_invalid_chars>
+  // Control characters (< 32 or > 126) should be rejected
+  const beforeBad = document.cookie;
+
+  document.cookie = 'testbad1\x00=value';
+  testing.expectEqual(false, document.cookie.includes('testbad1'));
+
+  document.cookie = 'testbad2\x1F=value';
+  testing.expectEqual(false, document.cookie.includes('testbad2'));
+
+  document.cookie = 'testbad3=val\x7F';
+  testing.expectEqual(false, document.cookie.includes('testbad3'));
 </script>

 <script id=createAttribute>
--- a/src/browser/webapi/HTMLDocument.zig
+++ b/src/browser/webapi/HTMLDocument.zig
@@ -201,7 +201,10 @@ pub fn setCookie(_: *HTMLDocument, cookie_str: []const u8, page: *Page) ![]const
    // we use the cookie jar's allocator to parse the cookie because it
    // outlives the page's arena.
    const Cookie = @import("storage/Cookie.zig");
-    const c = try Cookie.parse(page._session.cookie_jar.allocator, page.url, cookie_str);
+    const c = Cookie.parse(page._session.cookie_jar.allocator, page.url, cookie_str) catch {
+        // Invalid cookies should be silently ignored, not throw errors
+        return "";
+    };
    errdefer c.deinit();
    if (c.http_only) {
        c.deinit();
--- a/src/browser/webapi/storage/Cookie.zig
+++ b/src/browser/webapi/storage/Cookie.zig
@@ -49,10 +49,10 @@ pub fn deinit(self: *const Cookie) void {

 // There's https://datatracker.ietf.org/doc/html/rfc6265 but browsers are
 // far less strict. I only found 2 cases where browsers will reject a cookie:
-//   - a byte 0...32 and 127..255 anywhere in the cookie (the HTTP header
+//   - a byte 0...31 and 127...255 anywhere in the cookie (the HTTP header
 //     parser might take care of this already)
 //   - any shenanigans with the domain attribute - it has to be the current
-//     domain or one of higher order, exluding TLD.
+//     domain or one of higher order, excluding TLD.
 // Anything else, will turn into a cookie.
 // Single value? That's a cookie with an emtpy name and a value
 // Key or Values with characters the RFC says aren't allowed? Allowed! (
@@ -318,7 +318,7 @@ fn parseNameValue(str: []const u8) !struct { []const u8, []const u8, []const u8

 pub fn appliesTo(self: *const Cookie, url: *const PreparedUri, same_site: bool, is_navigation: bool, is_http: bool) bool {
    if (self.http_only and is_http == false) {
-        // http only cookies can be accessed from Javascript
+        // http only cookies cannot be accessed from Javascript
        return false;
    }

@@ -435,9 +435,9 @@ pub const Jar = struct {
    pub fn removeExpired(self: *Jar, request_time: ?i64) void {
        if (self.cookies.items.len == 0) return;
        const time = request_time orelse std.time.timestamp();
-        var i: usize = self.cookies.items.len - 1;
+        var i: usize = self.cookies.items.len ;
        while (i > 0) {
-            defer i -= 1;
+            i -= 1;
            const cookie = &self.cookies.items[i];
            if (isCookieExpired(cookie, time)) {
                self.cookies.swapRemove(i).deinit();
@@ -558,7 +558,7 @@ fn trimLeft(str: []const u8) []const u8 {
 }

 fn trimRight(str: []const u8) []const u8 {
-    return std.mem.trimLeft(u8, str, &std.ascii.whitespace);
+    return std.mem.trimRight(u8, str, &std.ascii.whitespace);
 }

 fn toLower(str: []u8) []u8 {