#46 Add cookie management commands#47
Conversation
| DebugURL string `json:"debug_url"` | ||
| ChromePID int `json:"chrome_pid"` | ||
| ActivePage int `json:"active_page"` // index into pages list | ||
| DataDir string `json:"data_dir"` | ||
| ProxyPID int `json:"proxy_pid,omitempty"` // PID of auth proxy helper | ||
| ProxyPort int `json:"proxy_port,omitempty"` // local port of auth proxy |
There was a problem hiding this comment.
Nit: These whitespace-only alignment changes (here, the .Leakless comment at line 387, the queryAXNodes field at line 1929, and the TestAssert_ValueFormatting table at line 1180 in the test file) are unrelated to the cookie feature and make the diff harder to review. They can also cause unnecessary merge conflicts if other PRs touch these lines. Consider dropping these or splitting them into a separate formatting PR.
| cookie, err := getCookie(page, opts.Name) | ||
| if err != nil { | ||
| fatal("failed to check cookie: %v", err) | ||
| } | ||
| if cookie == nil || cookie.Domain != opts.Domain || cookie.Path != opts.Path { | ||
| fmt.Fprintf(os.Stderr, "cookie not found: %s\n", opts.Name) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
This pre-check may give false "not found" results. getCookie calls listCookies, which filters cookies by the current page URL. If the cookie's domain doesn't match the page the user currently has open, listCookies won't return it — even though NetworkDeleteCookies could still delete it by name+domain+path.
Consider either:
- Removing the pre-check and just calling
deleteCookiedirectly (the CDP call is idempotent — deleting a non-existent cookie is a no-op), or - Documenting that the current page must match the cookie's domain for delete to work.
| type cookieSetOptions struct { | ||
| Name string | ||
| Value string | ||
| Domain string | ||
| Path string | ||
| Secure bool | ||
| HTTPOnly bool | ||
| SameSite proto.NetworkCookieSameSite |
There was a problem hiding this comment.
Not a blocker, but worth noting: there's no Expires or MaxAge field here, so cookie set can only create session cookies. Expiry is a very common need (e.g., setting a persistent auth cookie). Consider tracking --expires / --max-age support as a follow-up issue.
| return sb.String() | ||
| } | ||
|
|
||
| func findCookie(cookies []*proto.NetworkCookie, name string) *proto.NetworkCookie { |
There was a problem hiding this comment.
findCookie returns the first match by name. If two cookies share the same name but differ by domain (e.g., session on example.com and session on sub.example.com), cookie get will return whichever appears first — which may not be what the user expects.
Consider accepting an optional --domain filter on cookie get, or at minimum documenting this limitation.
| } | ||
|
|
||
| func TestParseCookieSetArgs_WithOptionalFlags(t *testing.T) { | ||
| opts, err := parseCookieSetArgs([]string{"session", "abc123", "--domain", "example.com", "--path", "/app", "--secure", "--http-only", "--same-site", "strict"}) |
There was a problem hiding this comment.
This test only covers the space-separated form --same-site strict. The parsing code also supports --same-site=strict (equals form). Consider adding a test case for the = variant to prevent regressions in that code path.
- Revert whitespace-only alignment changes in State struct, Leakless comment, AccessibilityQueryAXTree field, and TestAssert_ValueFormatting table to keep the diff focused on cookies. - Drop the cookie delete pre-check. NetworkDeleteCookies is idempotent, and the old pre-check via getCookie/listCookies filtered by the current page URL, so it gave false 'not found' results when the cookie's domain didn't match the current page. - Add optional --domain filter to 'cookie get' so duplicate cookie names on different domains can be disambiguated; add parser tests and a findCookie filter test.
Closes #46
This adds first-class cookie management commands to Rodney. See the issue for the full command and behavior spec.
Help output
Tests
TestParseCookieSetArgs_Minimalcookie setTestParseCookieSetArgs_WithOptionalFlagsTestParseCookieSetArgs_RequiresDomain--domainis required for setTestSetCookie_SetsCookieForDomainTestSetCookie_AppliesOptionalAttributesTestParseCookieDeleteArgs_Minimalcookie deleteTestParseCookieDeleteArgs_RequiresDomain--domainis required for deleteTestListCookies_ReturnsCurrentPageCookiescookie listreturns cookies for the current pageTestGetCookie_ReturnsMatchingCookiecookie getreturns the expected valueTestGetCookie_ReturnsNilWhenMissingTestDeleteCookie_RemovesCookieTestFormatCookieList_SortsByNameNotes
This feature was built using a red/green TDD workflow.
It has also been exercised in a real project against a major SaaS platform, without relying on any platform-specific code or tests in this change.