Conversation
9c46a58 to
6d676e6
Compare
There was a problem hiding this comment.
Pull request overview
This PR overhauls Scramjet’s cookie handling by vendoring set-cookie-parser, tightening SameSite/domain/path semantics (including cross-site redirect poisoning), and improving how cookies are synced/broadcast/persisted between fetch/client/controller/service worker. It also expands Runway coverage with cookie-focused WPT fixtures and harness transport changes to support “cleartext HTTPS” test origins.
Changes:
- Vendor a
set-cookie-parserimplementation and updateCookieJarparsing/matching/SameSite behavior. - Add cookie sync batching/options through fetch → client/controller/SW, plus controller-side persistence + broadcast updates.
- Add cookie WPT vendoring/selection + new Runway cookie tests and harness cleartext-HTTPS transport support.
Reviewed changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Removes set-cookie-parser lock entry after vendoring parser. |
| packages/scramjet/packages/runway/src/tests/wpt/selection.ts | Adds explicit cookie WPT file list and helpers to include/filter them. |
| packages/scramjet/packages/runway/src/tests/wpt/cookies.ts | Adds a dedicated WPT cookie test server + reporting bridge for Runway. |
| packages/scramjet/packages/runway/src/tests/cookies.ts | Adds targeted cookie regression tests (fetch/img race; x.com/api.x.com domain cases). |
| packages/scramjet/packages/runway/src/testcommon.ts | Extends Test with hostname/cleartext host mapping and URL helpers. |
| packages/scramjet/packages/runway/src/inspect.ts | Uses new target URL helpers + syncs cleartext host/site config into harness page. |
| packages/scramjet/packages/runway/src/index.ts | Syncs cleartext host/site config before each test; uses computed target URLs. |
| packages/scramjet/packages/runway/src/harness/scramjet/public/runway-cleartext-https-transport.js | Adds transport wrapper to rewrite logical https/wss to cleartext http/ws for tests. |
| packages/scramjet/packages/runway/src/harness/scramjet/public/index.html | Installs the cleartext transport wrapper around LibcurlClient and exposes harness handles. |
| packages/scramjet/packages/runway/src/cdp-page.ts | Exposes __runwayControl binding and implements cookie clearing for WPT driver calls. |
| packages/scramjet/packages/runway/scripts/generate-wpt.ts | Vendors cookie WPT subtree via sparse checkout + explicit file copy. |
| packages/scramjet/packages/core/src/shared/snapshot.ts | Binds Promise.* methods to avoid unbound builtin pitfalls. |
| packages/scramjet/packages/core/src/shared/set-cookie-parser.ts | Adds vendored Set-Cookie parsing utilities. |
| packages/scramjet/packages/core/src/shared/cookie.ts | Updates CookieJar parsing, host-only behavior, path matching, SameSite context filtering, and sync API shape. |
| packages/scramjet/packages/core/src/fetch/util.ts | Moves/refactors createReferrerString into fetch util. |
| packages/scramjet/packages/core/src/fetch/index.ts | Expands cookie sync API types and adds crossSiteRedirect tracking field. |
| packages/scramjet/packages/core/src/fetch/headers.ts | Deletes set-cookie from rewritten responses and enforces SameSite context on outgoing Cookie header. |
| packages/scramjet/packages/core/src/fetch/fetch.ts | Propagates cross-site redirect flag; batches Set-Cookie sync calls; adds registrable-domain helper for redirect tracking. |
| packages/scramjet/packages/core/src/client/dom/document.ts | Updates createReferrerString import to new location. |
| packages/scramjet/packages/core/src/client/dom/cookie.ts | Updates cookie setter to use new cookie sync API shape. |
| packages/scramjet/packages/core/src/client/client.ts | Updates ScramjetClientInit.sendSetCookie typing to cookie batch/options API. |
| packages/scramjet/packages/core/package.json | Removes set-cookie-parser dependency after vendoring. |
| packages/scramjet/packages/controller/src/types.d.ts | Updates controller/SW RPC types for cookie batch sync + options. |
| packages/scramjet/packages/controller/src/sw.ts | Implements batched cookie broadcast and avoids deadlock on navigation cookie sync. |
| packages/scramjet/packages/controller/src/inject.ts | Adds SW → page cookie sync handler; updates transport cookie RPC calls. |
| packages/scramjet/packages/controller/src/index.ts | Adds cookie persistence (IndexedDB), broadcast invalidation (BroadcastChannel), and cookie sync plumbing. |
| .gitignore | Ignores vendored cookie WPT directory. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (typeof cookie.maxAge === "number") { | ||
| if (!Number.isFinite(cookie.maxAge)) { | ||
| delete cookie.maxAge; | ||
| } else if (cookie.maxAge <= 0) { | ||
| delete this.cookies[id]; | ||
| continue; | ||
| } else { | ||
| cookie.expires = new Date( | ||
| Date.now() + cookie.maxAge * 1000 | ||
| ).toString(); | ||
| } |
There was a problem hiding this comment.
CookieJar.setCookies uses runtime globals (Number.isFinite, Date.now, new Date(...)) even though core enforces scramjet-core/no-globals for src/**/*.ts (see packages/scramjet/packages/core/eslint.config.mjs). Please switch these to the snapshot wrappers (e.g. import Number and use Number.isFinite, and use _Date.now() / new _Date(...)) to avoid lint/runtime-global issues.
| if (typeof cookies === "object") return cookies; | ||
| load(cookies: string | Record<string, Cookie>) { | ||
| if (typeof cookies === "object") { | ||
| console.error("??"); |
There was a problem hiding this comment.
CookieJar.load currently logs console.error("??") and returns early when passed an object, leaving the jar unchanged. This looks like a debug placeholder and will cause silent failures if load is ever called with an already-parsed cookie map (the method signature explicitly allows it). Either support object input by assigning it to this.cookies, or remove the object overload and throw a clear error.
| console.error("??"); | |
| this.cookies = cookies; |
| const MAX_COOKIE_PAIR_BYTES = 4096; | ||
| const textEncoder = new TextEncoder(); | ||
|
|
There was a problem hiding this comment.
This new shared module instantiates new TextEncoder() directly. In scramjet/core, src/**/*.ts is under scramjet-core/no-globals, so direct runtime globals like TextEncoder will fail linting. Use the snapshot exports (e.g. TextEncoder_encode or _TextEncoder) from src/shared/snapshot.ts instead.
| if (key === "expires") { | ||
| cookie.expires = new Date(sideValue); | ||
| } else if (key === "max-age") { | ||
| cookie.maxAge = parseInt(sideValue, 10); | ||
| } else if (key === "secure") { |
There was a problem hiding this comment.
parseString uses runtime globals (new Date(...), parseInt(...)) which will violate scramjet-core/no-globals in core’s src/**/*.ts. Please use snapshot wrappers (e.g. _Date and Number_parseInt) so this file passes the same lint/runtime-global constraints as the rest of src/shared/*.
vendors set-cookie-parser, fixes cross site cookie issues, overhauls the controller's cookie broadcasting and persistence
brings in cookie related wpt tests