🐛 bug: enforce CookieJar domain acceptance and host-only cookie matching#4282
🐛 bug: enforce CookieJar domain acceptance and host-only cookie matching#4282gaby wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCookieJar now wraps stored cookies with a hostOnly flag and uses map[string][]storedCookie. Request matching rejects host-only cookies for non-matching hosts. SetByHost/parseCookiesFromResp compute/update hostOnly, adjust domain bytes for host-only cookies, and prune expired entries. New tests cover host-only, domain, and case-matching behaviors. ChangesHost-only Cookie Security
Sequence Diagram(s)sequenceDiagram
participant Client
participant CookieJar
participant hostCookiesStorage
participant NetworkResp
Client->>CookieJar: cookiesForRequest(host, url)
CookieJar->>hostCookiesStorage: iterate storedCookie entries
hostCookiesStorage-->>CookieJar: return underlying *fasthttp.Cookie (skip/release expired)
CookieJar->>CookieJar: filter by domain, hostOnly, path, Secure
CookieJar-->>Client: matching cookies
NetworkResp->>CookieJar: parseCookiesFromResp(resp, host)
CookieJar->>CookieJar: compute hostOnly, set cookie.Domain bytes for host-only
CookieJar->>hostCookiesStorage: store/replace storedCookie entries (update hostOnly)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4282 +/- ##
==========================================
- Coverage 91.21% 91.14% -0.08%
==========================================
Files 130 130
Lines 12763 12817 +54
==========================================
+ Hits 11642 11682 +40
- Misses 709 719 +10
- Partials 412 416 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/cookiejar.go`:
- Around line 121-126: The host-only filter is applied once per bucket which
allows mixed host-only/domain cookies to either be skipped incorrectly or leak;
inside the code that iterates a bucket (the cookies slice returned by
SetByHost/parseCookiesFromResp) move the host-only check into the inner
per-cookie loop (after the expiry prune that updates kept) so each cookie is
individually skipped when cookie.hostOnly && host != domain; update the logic
that appends to the output to only append cookies that pass this per-cookie
host-only test, and add a regression test (e.g., extend
Test_CookieJar_HostOnlyCookieNotSentToSubdomain) that stores both a host-only
and a domain cookie in the same bucket and asserts only the domain cookie is
sent to a subdomain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c10d7ed5-7f6f-48ce-97f7-b45be3374772
📒 Files selected for processing (3)
client/client_test.goclient/cookiejar.goclient/cookiejar_test.go
There was a problem hiding this comment.
Pull request overview
This PR hardens the client CookieJar by tracking host-only cookies separately from domain cookies and rejecting response cookies with unrelated Domain attributes.
Changes:
- Adds
storedCookiemetadata to preserve host-only cookie semantics. - Rejects response cookies whose
Domaindoes not domain-match the response host. - Adds regression tests for host-only subdomain leakage and unrelated response domains.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
client/cookiejar.go |
Updates cookie storage and matching logic to track host-only cookies and validate response domains. |
client/cookiejar_test.go |
Adds regression coverage for host-only and unrelated-domain cookie behavior. |
client/client_test.go |
Updates tests for the new internal storage type and applies formatting changes. |
|
@copilot apply changes based on the comments in this thread |
![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 enhances the CookieJar implementation by introducing a storedCookie struct to track the hostOnly attribute, ensuring more accurate cookie scoping. It also adds a security check to reject cookies from unrelated domains and includes new test cases for these scenarios. The review feedback correctly identifies a high-severity logic error where the hostOnly check was being applied to an entire bucket based on the first cookie's state rather than per-cookie, which could lead to incorrect cookie transmission to subdomains. Additionally, the reviewer suggested minor performance optimizations to break loops early once a matching cookie is found during updates.
| for i := range hostCookies { | ||
| if hostCookies[i].cookie == existing { | ||
| hostCookies[i].hostOnly = hostOnly | ||
| } | ||
| } |
There was a problem hiding this comment.
Since each cookie (identified by key and path) is unique within the host's cookie slice, you should break the loop once the matching cookie is found and updated for better efficiency.
for i := range hostCookies {
if hostCookies[i].cookie == existing {
hostCookies[i].hostOnly = hostOnly
break
}
}Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/a4820f32-106c-4491-8997-895c6a12b34d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/a4820f32-106c-4491-8997-895c6a12b34d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/cookiejar.go`:
- Around line 137-139: cookiesForRequest is doing a case-sensitive host
comparison because the incoming request host (variable host in getByHostAndPath
/ cookiesForRequest) is not normalized to lowercase while stored domain keys
(set via SetByHost and parseCookiesFromResp) are lowercased; fix by lowercasing
the request host once on entry (e.g., at the start of cookiesForRequest or
getByHostAndPath) so subsequent checks like "if sc.hostOnly && host != domain"
and any domainMatch usage compare lowercased values consistently; add/adjust
tests to assert that a host-only cookie set for "example.com" is returned for a
request to "Example.com".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31f06cdd-147a-483d-8099-606306065e75
📒 Files selected for processing (2)
client/cookiejar.goclient/cookiejar_test.go
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/a4820f32-106c-4491-8997-895c6a12b34d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied the review-thread fixes in |
|
@copilot Address all the pull request review comments. |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/ed8365ba-efa2-405e-8765-85e10f842e93 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Addressed the remaining review comments in |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/da46c08d-50c8-4a08-9e15-e541af0e8f57 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/da46c08d-50c8-4a08-9e15-e541af0e8f57 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/da46c08d-50c8-4a08-9e15-e541af0e8f57 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied the review-thread fixes in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
client/cookiejar.go:288
- The new response-domain acceptance path only has rejection coverage; there is no parseCookiesFromResp regression test showing that a valid parent Domain (for example, sub.example.com setting Domain=example.com) is still accepted and sent to sibling subdomains. Adding that positive case would guard the stated requirement to preserve valid domain-cookie behavior.
acceptance := acceptCookieDomain(hostStr, domain)
if !acceptance.ok {
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/f1c2008f-e4ec-4ca3-aa43-37a50b70bf2f Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied the review-thread fixes in |
| @@ -38,10 +39,21 @@ func ReleaseCookieJar(c *CookieJar) { | |||
|
|
|||
| // CookieJar manages cookie storage for the client. It stores cookies keyed by host. | |||
| type CookieJar struct { | |||
Motivation
Domainattributes and later matching them by suffix, enabling cross-domain cookie injection and host-only leakage when a singleCookieJaris reused across untrusted hosts.Description
hostCookies map[string][]*fasthttp.CookiewithhostCookies map[string][]storedCookieand add astoredCookie{cookie *fasthttp.Cookie, hostOnly bool}to retain host-only semantics.Set-Cookiefrom a response, reject cookies that carry aDomainattribute unless the response host domain-matches thatDomain, and mark cookies without aDomainashostOnly.Test_CookieJar_HostOnlyCookieNotSentToSubdomainandTest_CookieJar_RejectUnrelatedResponseDomain, and updateTestClientResetClearsStateto use the new internal storage type.