Repository navigation
feat: Netscape HTTP Cookie File support via --cookie-file - #6
Conversation
Add cookie file parsing and URL-based filtering so users can pass browser-exported cookie files (the standard format used by curl and browser extensions). Cookie file cookies merge with --cookie flags, with explicit --cookie values taking precedence on conflicts. - New module: src/fetch/cookie-file.ts (parser, URL filter, resolver) - CLI: --cookie-file flag + AGENT_FETCH_COOKIE_FILE env var - API: cookieFile option on HttpFetchOptions for programmatic use - 31 new tests covering parser, URL filtering, and resolver
- Extract mergeCookies() helper to eliminate triplicated merge pattern - Fix path matching to enforce segment boundaries per RFC 6265 §5.1.4 (/api no longer matches /apiv2, only /api and /api/*) - Add error handling in loadCookiesFromFile with descriptive message - Rename domainWithoutDot → cookieDomain for conciseness - Add tests: path boundary attack, file-not-found error, mergeCookies
Summary of ChangesHello @teng-lin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's ability to manage and utilize HTTP cookies by introducing support for the widely adopted Netscape HTTP Cookie File format. This allows users to seamlessly integrate existing cookie stores from other tools, improving authentication workflows and overall flexibility when interacting with web resources. The implementation includes careful consideration for security and correct cookie behavior, ensuring that cookies are applied only when appropriate for the target URL. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for loading cookies from a Netscape-format cookie file, enhancing authenticated session handling. However, critical security vulnerabilities exist, including Path Traversal and Information Exposure, due to a lack of validation on file paths and sensitive error messages. Additionally, a high-severity code issue was found where an explicit empty cookie file path fails to correctly override environment variables, leading to unintended behavior.
|
|
||
| let content: string; | ||
| try { | ||
| content = readFileSync(cookieFilePath, 'utf-8'); |
There was a problem hiding this comment.
The cookieFilePath is used directly in readFileSync without validation or size limits. This allows for arbitrary file read (Path Traversal) and potential Denial of Service (DoS) if a very large file is provided. An attacker could exfiltrate sensitive data by sending file contents as cookies to a malicious URL. Consider validating that the path is within an allowed directory and checking the file size before reading.
There was a problem hiding this comment.
This is a CLI tool — the user provides their own file path on their own machine via --cookie-file or an env var they set themselves. There is no server, no untrusted input, and the user already has full filesystem access. This is the same trust model as curl --cookie / curl --cookie-jar. Path traversal and arbitrary file read are not meaningful threats in this context.
A file size limit would prevent legitimate use cases (large cookie jars) without mitigating any real attack vector, since the "attacker" here would be the user themselves.
| * Priority: explicit > AGENT_FETCH_COOKIE_FILE | ||
| */ | ||
| export function resolveCookieFile(explicit?: string): string | undefined { | ||
| return explicit || process.env.AGENT_FETCH_COOKIE_FILE || undefined; |
There was a problem hiding this comment.
There's a subtle issue here. If a user provides an empty string for the explicit path (e.g., via --cookie-file "" on the command line), it will be treated as a falsy value, causing the function to fall back to the AGENT_FETCH_COOKIE_FILE environment variable. This is likely not the user's intent; an explicit empty path should probably disable the cookie file feature by returning undefined, overriding any environment variable.
if (explicit !== undefined) {
return explicit || undefined; // An explicit empty string means no file
}
return process.env.AGENT_FETCH_COOKIE_FILE || undefined;There was a problem hiding this comment.
Good catch. Fixed in 68c1a22 — resolveCookieFile now checks explicit !== undefined first, so an explicit empty string returns undefined instead of falling through to the env var. Added a test for this case as well.
| try { | ||
| content = readFileSync(cookieFilePath, 'utf-8'); | ||
| } catch (err) { | ||
| throw new Error(`Failed to read cookie file: ${cookieFilePath}: ${(err as Error).message}`); |
There was a problem hiding this comment.
The current error message exposes sensitive information like the full file path and system error details (e.g., ENOENT), which could reveal server file system structure to an attacker. Additionally, the message can be redundant by repeating the file path. It's recommended to use a more generic error message for the user and log detailed information internally to prevent information exposure and improve clarity.
There was a problem hiding this comment.
This is a CLI tool — the user typed the file path themselves (or set the env var). Showing the path and the OS error (ENOENT, EACCES) in the message is the expected UX and helps the user debug typos or permission issues. Hiding this information would make the error less actionable. Same approach as curl, wget, etc.
An explicit empty string (--cookie-file "") now correctly returns undefined instead of falling through to AGENT_FETCH_COOKIE_FILE.
1 participant
Summary
--cookie-fileflag andAGENT_FETCH_COOKIE_FILEenv var to load cookies from Netscape HTTP Cookie File format (standard format used by curl and browser extensions)--cookieflags take precedence over cookie file entries on name conflictscookieFileoption onHttpFetchOptions)Changes
src/fetch/cookie-file.ts— parser, URL-based filter, resolver, merge helpersrc/__tests__/cookie-file.test.ts— 37 tests covering parser, URL filtering, resolver, merge, and error pathssrc/cli.ts—--cookie-fileshared flag, cookie merge in fetch and crawl commandssrc/fetch/http-fetch.ts—cookieFileoption onHttpFetchOptionsSecurity
.example.comwon't matchbadexample.com)/apiwon't match/apiv2)Test plan