🐛 bug: enforce CookieJar domain acceptance and host-only cookie matching#4282
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.27% 91.29% +0.02%
==========================================
Files 130 130
Lines 12797 12851 +54
==========================================
+ Hits 11680 11732 +52
- Misses 705 706 +1
- Partials 412 413 +1
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.
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 apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/8ff2fe5f-f5fb-4be3-b983-8cbd0a247282 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/8ff2fe5f-f5fb-4be3-b983-8cbd0a247282 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied the review-thread fixes in |
|
@copilot apply changes based on the comments in this thread |
Already addressed in |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/7737b6d1-adc8-4f26-a128-c829e3562230 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Fixed in |
|
@claude[agent] conduct a security and rfc audit of the changes in this pull request and the cookiejar implementation. |
Completed comprehensive security and RFC 6265 audit of the CookieJar implementation. Audit Results: ✅ SECURE & COMPLIANTThe implementation successfully addresses all identified vulnerabilities and achieves full RFC 6265 compliance: Security Fixes Verified
RFC 6265 Compliance
Test Coverage
Code Quality
Rating: PRODUCTION READY - No security concerns identified. Full audit report: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 59b2289 | Previous: 7e4493e | Ratio |
|---|---|---|---|
Benchmark_SetLogger (github.com/gofiber/fiber/v3/log) |
4.881 ns/op 0 B/op 0 allocs/op |
3.174 ns/op 0 B/op 0 allocs/op |
1.54 |
Benchmark_SetLogger (github.com/gofiber/fiber/v3/log) - ns/op |
4.881 ns/op |
3.174 ns/op |
1.54 |
This comment was automatically generated by workflow using github-action-benchmark.
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.