Secure field lost in cookie round-trip and setCookies fails on invalid batch #75

New Issue

Claude · 2026-02-24T02:02:44Z

Claude commented

2026-02-24 02:02:44 +00:00

Problem

Two related bugs in cookie handling that cause Storage.setCookies failures when using Chromium:

1. `Secure` field dropped in both conversion directions

playwrightCookieToCookie (playwright.go ~line 139) does NOT read cookie.Secure:

func playwrightCookieToCookie(cookie playwright.Cookie) Cookie {
    return Cookie{
        Name:     cookie.Name,
        Value:    cookie.Value,
        Host:     cookie.Domain,
        Path:     cookie.Path,
        Expires:  time.Unix(int64(cookie.Expires), 0),
        HttpOnly: cookie.HttpOnly,
        SameSite: playwrightSameSiteToSameSite(cookie.SameSite),
        // Secure: cookie.Secure  <-- MISSING
    }
}

cookieToPlaywrightOptionalCookie (playwright.go ~line 124) does NOT write cookie.Secure:

func cookieToPlaywrightOptionalCookie(cookie Cookie) playwright.OptionalCookie {
    oc := playwright.OptionalCookie{
        Name:     cookie.Name,
        Value:    cookie.Value,
        Domain:   playwright.String(cookie.Host),
        Path:     playwright.String(cookie.Path),
        Expires:  playwright.Float(float64(cookie.Expires.Unix())),
        HttpOnly: playwright.Bool(cookie.HttpOnly),
        // Secure: playwright.Bool(cookie.Secure)  <-- MISSING
    }

This means Secure is always false after a round-trip, causing Chromium to reject cookies with __Secure- or __Host- prefixes (which require Secure=true).

2. `AddCookies` is all-or-nothing with no error recovery

In browser_init.go (~line 159), all cookies are added in a single batch:

if err := bctx.AddCookies(pwCookies); err != nil {
    return nil, fmt.Errorf("error adding cookies to browser: %w", err)
}

If any cookie in the batch is invalid, Chromium rejects the entire call and browser creation fails. There's no way to identify which cookie caused the failure, and all valid cookies are lost too.

Proposed Fix

Fix 1: Map `Secure` field in both directions

// playwrightCookieToCookie
Secure: cookie.Secure,

// cookieToPlaywrightOptionalCookie  
oc.Secure = playwright.Bool(cookie.Secure)

Fix 2: Add cookies one-by-one with error logging

for _, c := range pwCookies {
    if err := bctx.AddCookies([]playwright.OptionalCookie{c}); err != nil {
        slog.Warn("skipping invalid cookie", "name", c.Name, "domain", *c.Domain, "error", err)
        continue
    }
}

This way one bad cookie doesn't prevent all cookies from loading, and the problematic cookies are logged for debugging.

Impact

Downstream consumer steve/mort currently works around this with validation in its cookie jar (filtering __Secure-/__Host- cookies), but the root cause is here in go-extractor.

## Problem Two related bugs in cookie handling that cause `Storage.setCookies` failures when using Chromium: ### 1. `Secure` field dropped in both conversion directions **`playwrightCookieToCookie`** (playwright.go ~line 139) does NOT read `cookie.Secure`: ```go func playwrightCookieToCookie(cookie playwright.Cookie) Cookie { return Cookie{ Name: cookie.Name, Value: cookie.Value, Host: cookie.Domain, Path: cookie.Path, Expires: time.Unix(int64(cookie.Expires), 0), HttpOnly: cookie.HttpOnly, SameSite: playwrightSameSiteToSameSite(cookie.SameSite), // Secure: cookie.Secure <-- MISSING } } ``` **`cookieToPlaywrightOptionalCookie`** (playwright.go ~line 124) does NOT write `cookie.Secure`: ```go func cookieToPlaywrightOptionalCookie(cookie Cookie) playwright.OptionalCookie { oc := playwright.OptionalCookie{ Name: cookie.Name, Value: cookie.Value, Domain: playwright.String(cookie.Host), Path: playwright.String(cookie.Path), Expires: playwright.Float(float64(cookie.Expires.Unix())), HttpOnly: playwright.Bool(cookie.HttpOnly), // Secure: playwright.Bool(cookie.Secure) <-- MISSING } ``` This means `Secure` is always `false` after a round-trip, causing Chromium to reject cookies with `__Secure-` or `__Host-` prefixes (which require `Secure=true`). ### 2. `AddCookies` is all-or-nothing with no error recovery In `browser_init.go` (~line 159), all cookies are added in a single batch: ```go if err := bctx.AddCookies(pwCookies); err != nil { return nil, fmt.Errorf("error adding cookies to browser: %w", err) } ``` If **any** cookie in the batch is invalid, Chromium rejects the **entire** call and browser creation fails. There's no way to identify which cookie caused the failure, and all valid cookies are lost too. ## Proposed Fix ### Fix 1: Map `Secure` field in both directions ```go // playwrightCookieToCookie Secure: cookie.Secure, // cookieToPlaywrightOptionalCookie oc.Secure = playwright.Bool(cookie.Secure) ``` ### Fix 2: Add cookies one-by-one with error logging ```go for _, c := range pwCookies { if err := bctx.AddCookies([]playwright.OptionalCookie{c}); err != nil { slog.Warn("skipping invalid cookie", "name", c.Name, "domain", *c.Domain, "error", err) continue } } ``` This way one bad cookie doesn't prevent all cookies from loading, and the problematic cookies are logged for debugging. ## Impact Downstream consumer `steve/mort` currently works around this with validation in its cookie jar (filtering `__Secure-`/`__Host-` cookies), but the root cause is here in go-extractor.

Claude added the bug priority/high labels 2026-02-24 02:02:47 +00:00

Claude commented

2026-02-24 02:13:04 +00:00

Starting work on this. Plan of approach:

Add Secure field mapping in both cookieToPlaywrightOptionalCookie and playwrightCookieToCookie in playwright.go
Replace the batch AddCookies call in browser_init.go with one-by-one cookie adding, logging a warning on failure instead of failing browser creation
Add unit tests for both conversion functions in a new playwright_test.go

Starting work on this. Plan of approach: 1. Add `Secure` field mapping in both `cookieToPlaywrightOptionalCookie` and `playwrightCookieToCookie` in `playwright.go` 2. Replace the batch `AddCookies` call in `browser_init.go` with one-by-one cookie adding, logging a warning on failure instead of failing browser creation 3. Add unit tests for both conversion functions in a new `playwright_test.go`

Claude referenced this issue from a commit

2026-02-24 02:13:24 +00:00

fix: map Secure field in cookie conversions and add per-cookie error handling

Claude referenced a pull request that will close this issue

2026-02-24 02:13:28 +00:00

fix: secure field lost in cookie round-trip & batch AddCookies failure #77

Claude commented

2026-02-24 02:13:34 +00:00

Work finished. PR: #77

Changes:

playwright.go: Added Secure field mapping in both cookieToPlaywrightOptionalCookie and playwrightCookieToCookie
browser_init.go: Replaced batch AddCookies with per-cookie adding + slog.Warn on failure
playwright_test.go: 5 new unit tests for the conversion functions

All tests pass, clean build.

Work finished. PR: #77 Changes: - `playwright.go`: Added `Secure` field mapping in both `cookieToPlaywrightOptionalCookie` and `playwrightCookieToCookie` - `browser_init.go`: Replaced batch `AddCookies` with per-cookie adding + `slog.Warn` on failure - `playwright_test.go`: 5 new unit tests for the conversion functions All tests pass, clean build.

steve closed this issue

2026-02-24 02:14:43 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: steve/go-extractor#75

Secure field lost in cookie round-trip and setCookies fails on invalid batch #75

Problem

1. Secure field dropped in both conversion directions

2. AddCookies is all-or-nothing with no error recovery

Proposed Fix

Fix 1: Map Secure field in both directions

Fix 2: Add cookies one-by-one with error logging

Impact

1. `Secure` field dropped in both conversion directions

2. `AddCookies` is all-or-nothing with no error recovery

Fix 1: Map `Secure` field in both directions