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:
iferr:=bctx.AddCookies(pwCookies);err!=nil{returnnil,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.
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.
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`
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Problem
Two related bugs in cookie handling that cause
Storage.setCookiesfailures when using Chromium:1.
Securefield dropped in both conversion directionsplaywrightCookieToCookie(playwright.go ~line 139) does NOT readcookie.Secure:cookieToPlaywrightOptionalCookie(playwright.go ~line 124) does NOT writecookie.Secure:This means
Secureis alwaysfalseafter a round-trip, causing Chromium to reject cookies with__Secure-or__Host-prefixes (which requireSecure=true).2.
AddCookiesis all-or-nothing with no error recoveryIn
browser_init.go(~line 159), all cookies are added in a single batch: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
Securefield in both directionsFix 2: Add cookies one-by-one with error logging
This way one bad cookie doesn't prevent all cookies from loading, and the problematic cookies are logged for debugging.
Impact
Downstream consumer
steve/mortcurrently works around this with validation in its cookie jar (filtering__Secure-/__Host-cookies), but the root cause is here in go-extractor.Starting work on this. Plan of approach:
Securefield mapping in bothcookieToPlaywrightOptionalCookieandplaywrightCookieToCookieinplaywright.goAddCookiescall inbrowser_init.gowith one-by-one cookie adding, logging a warning on failure instead of failing browser creationplaywright_test.goWork finished. PR: #77
Changes:
playwright.go: AddedSecurefield mapping in bothcookieToPlaywrightOptionalCookieandplaywrightCookieToCookiebrowser_init.go: Replaced batchAddCookieswith per-cookie adding +slog.Warnon failureplaywright_test.go: 5 new unit tests for the conversion functionsAll tests pass, clean build.