🐛 fix: correctness fixes for log injection, cookie jar, routing, and Content-Type handling

[gaby]

Description

This branch collects a set of correctness fixes found while auditing the framework. They are independent of one another and each ships with a regression test that was checked to fail when its fix is reverted.

The headline issue is log injection in the logger middleware. Access-log tags that render request-controlled data wrote their values verbatim, so any value that reaches the handler percent-decoded — a query parameter, a form field, or c.Path() under UnescapePath — could carry CR/LF and forge additional log lines, corrupting audit trails and hiding requests from SIEM tooling.

Important

One change alters observable behavior for existing callers: CookieJar.Set(uri, ...) now scopes a cookie with no Path attribute to the URI's RFC 6265 §5.1.4 default-path instead of /. See the cookie-jar section below.

Fixes #4341

Changes introduced

Logger — log injection (#4341)

• Scrub C0 control bytes and DEL (preserving HTAB) from the tags that render request-controlled data: ${path}, ${url}, ${ua}, ${referer}, ${ip}, ${ips}, ${host}, ${body}, ${resBody}, ${reqHeaders}, ${queryParams}, ${error}, ${scheme}, ${route}, and the parametric ${reqHeader:}, ${respHeader:}, ${query:}, ${form:}, ${cookie:}, ${locals:}.
• Sanitize the default format as well. defaultLoggerInstance short-circuits when cfg.Format == DefaultFormat and never consults the tag map, so the initial fix left the out-of-the-box configuration — the one most deployments use — unprotected.
• ${method} is deliberately not scrubbed: fasthttp's isValidMethod already rejects control bytes, and Config.MethodOverride only accepts registered methods.
• The scrubbing helpers moved to internal/logtemplate so the logger middleware and log/context.go share one implementation instead of two.

Client cookie jar — RFC 6265 conformance

• Key replacement on an exact path match. Previously a cookie whose path was a prefix of an existing one replaced it, silently dropping a distinct cookie.
• Scope path-less cookies from a Set-Cookie to the RFC 6265 §5.1.4 default-path rather than the request path, so Set-Cookie: a=1 from /foo/bar is no longer sent to /foo/barbaz.
• Behavior change: Set(uri, ...) applies that same default-path. It previously dropped uri.Path() and delegated to SetByHost, so a cookie with no usable Path was scoped to / — the identical cookie arriving as a Set-Cookie header and set programmatically ended up with different scopes. A cookie set against /a/b with no Path is now scoped to /a rather than host-wide; set Path explicitly to keep the old scope. SetByHost, SetKeyValue, and SetKeyValueBytes have no request path to derive a scope from and are unchanged.
• Order cookies by descending path length per §5.4, with a creation-order tiebreak, so the most specific cookie wins deterministically.
• Bound per-host storage (§5.3) with recency-based eviction, and bound the number of cookies a single request can carry.
• Stop case-folding the caller's cookie Domain in place — the jar was mutating a struct the caller still owned.
• SetByHost searches on the path the entry will actually carry. SetPathBytes runs the value through normalizePath, so for a cookie built by ParseBytes the stored path can differ from the one passed in; looking up the raw form missed the entry an earlier identical call created and appended a duplicate instead of replacing it, consuming per-host slots until eviction dropped a legitimate cookie.
• maxCookiesPerRequest applies in dumpCookiesToReq rather than cookiesForRequest. The cap is a wire concern, but cookiesForRequest also backs the exported Get, which silently returned at most 64 cookies while documenting that it returns those stored for the URI.

Content-Type handling

• Stop case-folding request headers in place. Bind().Body() lowercased the Content-Type header in the request's own storage, so the mutation was visible to every later reader of that request.
• Normalize only the parts that are case-insensitive per RFC 9110: the media type (§8.3.1) and parameter names (§5.6.6). Parameter values are left untouched, which matters because a multipart/form-data boundary is case-sensitive and folding it broke multipart parsing.
• Apply the same normalization in Bind().Form() and Redirect().WithInput(), which reached the media type through separate paths.

Routing

• Keep optional-slash routes reachable from the tree bucket. A leading constant of exactly maxDetectionPaths bytes (/a/) can match a detection path one byte shorter (/a), and that shorter path always hashes to bucket 0 — so /a/:id? and /a/* were unreachable for /a, while the otherwise identical /ab/... routes matched.
• Make RoutePatternMatch a faithful mirror of the router, as its own documentation claims. It trimmed trailing slashes from the pattern but not from the path, lowercased the path it also sliced parameter values from, skipped UnescapePath decoding, and compared the un-escape-stripped pattern in its root/star shortcut.

Other fixes

• Res.Cookie treats its argument as read-only. It normalized the caller's Cookie struct in place, so a caller reusing one struct across responses saw its values rewritten.
• basicauth rejects {SHA256}/{SHA512} hashes whose decoded length is not the digest size, instead of accepting a truncated hash.
• expvar and pprof only claim paths at a segment boundary, so /debug/varsdump and /debug/pprofiler fall through to the application instead of being swallowed.

Test and tooling

• Fix a data race in Test_Redirect_Back. require.ErrorAs(t, err, &ErrRedirectBackNoFallback) passes the address of a package-level sentinel, so errors.As writes into that global while a parallel test reads it. The assertion was also vacuous: the target is a **Error, so it matched any fiber *Error — pointing it at ErrBadRequest passed just as well. ErrorIs against the sentinel is what the test means.
• Every function this branch introduces or changes now reports full statement coverage; Codecov reports 100% patch coverage. Newly covered: WriteSanitized 75→100, ScrubControls 87.5→100, writeSanitizedColored 77.8→100, samePath 60→100, pathMatch 81.8→100, CookieJar.Get 75→100, CookieJar.Set 66.7→100, normalizeContentTypeMediaType 92.9→100, RoutePatternMatch 94.3→100, plus both halves of enforceHostCookieLimitLocked and every arm of ${locals:}'s type switch.

Note

An earlier commit here relaxed app.Test's 1s default timeout and the listener-readiness budgets to stop three tests flaking under -race. That has been reverted — the timeout flakes are being handled separately. The measurements are preserved in the revert commit message for whoever picks them up: a DefaultBodyLimit-sized round trip measured 198ms–1.29s under -race against the 1s default, and the listener waits are require.Eventually around a blocking InmemoryListener.Dial, so their duration only matters when the server never starts.

• Benchmarks: no benchmarks added. The fixes preserve the existing allocation-free fast paths — control-byte scanning uses SWAR and only copies when a control byte is present, utilsstrings.ToLower returns its input unchanged when there is no uppercase byte, and the cookie-jar dedupe scan avoids allocating.
• Documentation Update: docs/middleware/logger.md, docs/middleware/basicauth.md, docs/guide/utils.md, docs/client/examples.md.
• Changelog/What's New: summarized above, grouped by area. The CookieJar.Set scoping change belongs in the release notes.
• Migration Guide: only CookieJar.Set needs one, and it is a one-liner — pass an explicit Path on the cookie to keep the previous host-wide scope. No signature changes, so nothing else has to move.
• API Alignment with Express: not applicable — no new API surface.
• API Longevity: no existing exported signature changes. basicauth.ErrInvalidSHA512PasswordLength is the only addition to the public surface; the new internal/logtemplate helpers are not importable outside the module.
• Examples: docs/client/examples.md covers the cookie-jar behavior.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Code consistency (non-breaking change which improves code reliability and robustness)
• Documentation update (changes to documentation)

Checklist

• Conducted a self-review of the code and provided comments for complex or critical parts.
• Updated the documentation in the /docs/ directory for Fiber's documentation.
• Added or updated unit tests to validate the effectiveness of the changes or new features. Each regression test was checked to fail with its fix reverted.
• Ensured that new and existing unit tests pass locally with the changes, including under -race.
• Verified that any new dependencies are essential and have been agreed upon by the maintainers/community. No new dependencies are introduced.
• Aimed for optimal performance with minimal allocations in the new code.
• Provided benchmarks for the new code to analyze and improve upon.

🤖 Generated with Claude Code

https://claude.ai/code/session_011DMnryEc8UAkz73vAeMBf6

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR centralizes log sanitization, updates cookie-jar semantics, normalizes request headers, aligns route matching with router configuration, tightens middleware prefixes, validates BasicAuth digest sizes, and expands tests and documentation.

Changes

Logging sanitization

Layer / File(s)	Summary
Shared sanitization and logger integration internal/logtemplate/*, log/context.go, middleware/logger/*, docs/middleware/logger.md	Adds control-byte scrubbing and applies it to contextual and middleware logger output.

Cookie behavior

Layer / File(s)	Summary
Cookie storage, matching, and eviction client/cookiejar.go, client/cookiejar_test.go, docs/client/examples.md	Normalizes cookie domains and paths, orders matching cookies, deduplicates output, and enforces eviction limits.
Response cookie contract res.go, ctx_test.go, *_interface_gen.go	Normalizes cookies on local copies and documents and tests non-mutation behavior.

Request normalization

Layer / File(s)	Summary
Content-Type normalization and binding helpers.go, bind.go, binder/form.go, redirect.go, bind_test.go, helpers_test.go	Normalizes media types and parameter names while preserving values such as multipart boundaries.
Encoding and test timing req.go, app_test.go, app_integration_test.go, listen_test.go, redirect_test.go	Updates Content-Encoding handling and extends selected test wait bounds.

Routing and middleware matching

Layer / File(s)	Summary
Normalized route matching path.go, path_test.go, docs/guide/utils.md	Matches paths using configured unescaping, case sensitivity, and strict-routing behavior.
Optional-slash route bucketing router.go, router_test.go	Prevents optional-slash routes from being hidden by tree hashing.
Diagnostic middleware prefixes middleware/expvar/*, middleware/pprof/*	Allows sibling prefix routes to pass through normally.

BasicAuth validation

Layer / File(s)	Summary
Digest-length validation middleware/basicauth/*, docs/middleware/basicauth.md	Rejects SHA-256 and SHA-512 hashes with incorrect decoded digest lengths.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• gofiber/fiber#4552 — Related logger tag sanitization changes.
• gofiber/fiber#3564 — Related cookie-jar path and selection logic.
• gofiber/fiber#4571 — Related logger sanitization plumbing and tag rendering.

Suggested labels: 📜 RFC Compliance

Suggested reviewers: efectn

Poem

I’m a rabbit with logs scrubbed clean,
Cookies hop paths in order serene.
Routes find their proper way,
Bad hashes stop at startup’s gate,
And tests wait safely through the day.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR also bundles unrelated cookie, routing, Content-Type, Basic Auth, and middleware fixes beyond linked issue #4341.	Split the non-logger fixes into separate PRs or remove them if this PR should only address #4341.
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.32% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The logger sanitization changes address #4341 by scrubbing control bytes in affected tags and default output, with regression tests.
Title check	✅ Passed	The title is concise and accurately summarizes the main areas of the changeset.
Description check	✅ Passed	The description is detailed, covers the required sections, links the issue, and includes change categories, tests, docs, migration, and examples.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/bug-fixes-g8ywbs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.47%. Comparing base (d8ae9aa) to head (ede8793).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4570      +/-   ##
==========================================
+ Coverage   93.31%   93.47%   +0.16%     
==========================================
  Files         140      140              
  Lines       14858    14981     +123     
==========================================
+ Hits        13864    14004     +140     
+ Misses        619      608      -11     
+ Partials      375      369       -6     

Flag	Coverage Δ
unittests	93.47% <100.00%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

docs/client/examples.md (1)

231-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Docs match the implemented limits — run the Markdown lint before merge.

The 1024/64/64 figures line up with maxCookieJarHosts, maxCookiesPerHost, and maxCookiesPerRequest in client/cookiejar.go. Please run make markdown to lint the modified Markdown.

As per coding guidelines: "Run make markdown to lint all Markdown files when modifying code".

🤖 Prompt for 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.

In `@docs/client/examples.md` around lines 231 - 250, Run the repository’s
Markdown lint target, make markdown, after updating the cookie jar
documentation, and resolve any reported issues before completing the change.

Source: Coding guidelines

client/cookiejar.go (1)

576-586: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider deleting before releasing the evicted cookies.

releaseStoredCookies(evicted) hands the cookies back to the global pool while evicted pointers are still used as identity keys in the following DeleteFunc. It is correct today (the jar is locked and pointer comparison remains valid), but it relies on nothing else being able to observe those pointers. Swapping the order removes the subtlety at zero cost.

♻️ Proposed reordering

 		evicted := byRecency[:overflow]
-		releaseStoredCookies(evicted)
 		kept = slices.DeleteFunc(kept, func(sc storedCookie) bool {
 			return slices.ContainsFunc(evicted, func(e storedCookie) bool { return e.cookie == sc.cookie })
 		})
+		releaseStoredCookies(evicted)

🤖 Prompt for 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.

In `@client/cookiejar.go` around lines 576 - 586, In the overflow handling block,
update the eviction sequence so `kept = slices.DeleteFunc(...)` removes cookies
identified by `evicted` before calling `releaseStoredCookies(evicted)`. Preserve
the existing victim selection and insertion-order behavior, changing only the
order of deletion and pool release.

🤖 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 324-352: Update SetByHost’s lookup and storage flow to account for
SetPathBytes transforming percent-encoded characters and semicolons. Apply the
same round-trip normalization guard used by setDefaultCookiePath before
searchCookieByKeyAndPath, or escape lookupPath before storing, so the lookup
path exactly matches the persisted cookie path.

In `@middleware/logger/utils.go`:
- Around line 143-157: Update writeSanitizedValue to explicitly justify ignoring
the fmt.Fprintf error, using the repository’s established //nolint:errcheck
convention and noting that the pooled byte buffer write cannot fail; preserve
the existing formatting and writeSanitized flow.

In `@path.go`:
- Line 236: Update the routeParser retrieval in the surrounding parser setup to
check the type assertion result instead of discarding it. Handle an unexpected
pool value safely before dereferencing or using parser, preserving the existing
routeParser pooling behavior for valid values.

---

Nitpick comments:
In `@client/cookiejar.go`:
- Around line 576-586: In the overflow handling block, update the eviction
sequence so `kept = slices.DeleteFunc(...)` removes cookies identified by
`evicted` before calling `releaseStoredCookies(evicted)`. Preserve the existing
victim selection and insertion-order behavior, changing only the order of
deletion and pool release.

In `@docs/client/examples.md`:
- Around line 231-250: Run the repository’s Markdown lint target, make markdown,
after updating the cookie jar documentation, and resolve any reported issues
before completing the change.

🪄 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 Plus

Run ID: f26e9ffb-ac71-457a-b22f-f96a4ef94f4e

📥 Commits

Reviewing files that changed from the base of the PR and between 31d7210 and eeadd36.

📒 Files selected for processing (38)

• app_integration_test.go
• app_test.go
• bind.go
• bind_test.go
• binder/form.go
• client/cookiejar.go
• client/cookiejar_test.go
• ctx_interface_gen.go
• ctx_test.go
• docs/client/examples.md
• docs/guide/utils.md
• docs/middleware/basicauth.md
• docs/middleware/logger.md
• extractors/extractors.go
• helpers.go
• internal/logtemplate/sanitize.go
• internal/logtemplate/sanitize_test.go
• listen_test.go
• log/context.go
• middleware/basicauth/basicauth_test.go
• middleware/basicauth/config.go
• middleware/expvar/expvar.go
• middleware/expvar/expvar_test.go
• middleware/logger/default_logger.go
• middleware/logger/logger_test.go
• middleware/logger/tags.go
• middleware/logger/utils.go
• middleware/pprof/pprof.go
• middleware/pprof/pprof_test.go
• path.go
• path_test.go
• redirect.go
• redirect_test.go
• req.go
• res.go
• res_interface_gen.go
• router.go
• router_test.go

[ObeeJ]

Thanks for the comprehensive fix in #4570 — the centralisation into internal/logtemplate and the coverage of the default logger path look solid.

A couple of observations after reviewing the logger-related changes:

1. The SWAR-based scrubber is a nice performance-oriented approach. Have you measured the impact on the hot path for clean inputs (the common case)? It would be useful to see a quick before/after on Benchmark_Logger_* if one is available.

2. The documentation update in docs/middleware/logger.md listing which tags are scrubbed is helpful. One small clarification that might prevent future confusion: it may be worth explicitly noting that c.Path() / query values remain percent-encoded in the common case, so the scrubber primarily protects the tags that can receive already-decoded content (form values, body, error messages, certain headers, etc.).

Happy to help with any additional regression tests if useful.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

client/cookiejar.go (1)

269-274: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Avoid routing Set(uri, ...) through SetByHost when cookies can omit Path.

Set(uri, cookies...) drops uri.Path() and calls SetByHost(uri.Host(), cookies...), so cookies without a valid Path get defaulted to /. Response parsing correctly uses defaultCookiePathFor(path) from the request path per RFC 6265, but programmatic setters broaden those cookies host-wide. Thread the URI’s path into an internal helper while keeping SetByHost’s existing signature and behavior.

🤖 Prompt for 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.

In `@client/cookiejar.go` around lines 269 - 274, The CookieJar.Set method
currently loses the request path by delegating directly to SetByHost, causing
cookies without Path to default incorrectly. Add or reuse an internal helper
that accepts both host and URI path, update Set to pass uri.Host() and
uri.Path() through it, and keep SetByHost’s existing signature and behavior
unchanged.

🧹 Nitpick comments (1)

middleware/logger/logger_test.go (1)

1815-1826: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover reset-suffix write failure.

No case sets failAfter: 2, so the final WriteString(reset) error path and its returned byte count are unpinned.

Suggested test

+	t.Run("reset write fails", func(t *testing.T) {
+		t.Parallel()
+
+		buf := &failingBuffer{ByteBuffer: bytebufferpool.Get(), failAfter: 2}
+		defer bytebufferpool.Put(buf.ByteBuffer)
+
+		n, err := writeSanitizedColored(buf, color, value, reset)
+		require.ErrorIs(t, err, errWriteFailed)
+		require.Equal(t, len(color)+len(value), n)
+		require.Equal(t, "<c>va lue", buf.String())
+	})

🤖 Prompt for 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.

In `@middleware/logger/logger_test.go` around lines 1815 - 1826, Add a test case
alongside “all writes succeed” that configures failingBuffer with failAfter: 2,
invokes writeSanitizedColored, and asserts the reset-suffix write error plus the
returned byte count from the successful preceding writes. Verify the buffer
content reflects only the color and sanitized value writes before the reset
failure.

🤖 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_test.go`:
- Around line 1017-1046: Update the cookie construction in
Test_CookieJar_SetByHost_ReplacesOnNormalizedPath to use fmt.Appendf for
formatting the “a=v%d; path=%s” bytes instead of fmt.Sprintf wrapped in []byte,
matching the existing modernized formatting style.

---

Outside diff comments:
In `@client/cookiejar.go`:
- Around line 269-274: The CookieJar.Set method currently loses the request path
by delegating directly to SetByHost, causing cookies without Path to default
incorrectly. Add or reuse an internal helper that accepts both host and URI
path, update Set to pass uri.Host() and uri.Path() through it, and keep
SetByHost’s existing signature and behavior unchanged.

---

Nitpick comments:
In `@middleware/logger/logger_test.go`:
- Around line 1815-1826: Add a test case alongside “all writes succeed” that
configures failingBuffer with failAfter: 2, invokes writeSanitizedColored, and
asserts the reset-suffix write error plus the returned byte count from the
successful preceding writes. Verify the buffer content reflects only the color
and sanitized value writes before the reset failure.

🪄 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 Plus

Run ID: 675a6691-bbdd-4eb8-90e6-5f27cd1a066a

📥 Commits

Reviewing files that changed from the base of the PR and between eeadd36 and 1c94e90.

📒 Files selected for processing (7)

• client/cookiejar.go
• client/cookiejar_test.go
• helpers_test.go
• internal/logtemplate/sanitize_test.go
• middleware/logger/logger_test.go
• middleware/logger/utils.go
• path_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• middleware/logger/utils.go

[gaby]

Addressed the outside-diff finding on CookieJar.Set in 711feb3.

Set(uri, ...) dropped uri.Path(), so a cookie with no usable Path was scoped to / while the identical cookie arriving as a Set-Cookie header got the RFC 6265 §5.1.4 default-path. Both now go through an internal setByHostAndPath: Set passes uri.Path(), SetByHost passes nil (yielding /), so SetByHost, SetKeyValue, and SetKeyValueBytes are unchanged — they have no request path to derive a scope from.

This is a behavior change worth a maintainer's eye. A cookie set via Set against /a/b without an explicit Path is now scoped to /a instead of the whole host. That is the RFC-correct scope and it matches response parsing, but it narrows what gets sent for existing callers. Setting Path explicitly restores the old scope; both doc comments now say so.

Test_CookieJar_SetUsesURIDefaultPath covers the new scope, the sibling-directory match, the host-root non-match, the explicit-Path override, and that SetByHost still scopes to /. It fails on the first assertion with the change reverted.

Happy to drop this commit if you would rather keep Set as it was — it is self-contained.

---

Generated by Claude Code

[coderabbitai]

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 281-282: Update the SetByHost documentation to state that
replacement occurs only when the cookie name and normalized path match within
the same storage bucket; same-name cookies with different paths coexist.

🪄 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 Plus

Run ID: b0af47e6-5a71-4390-b6d3-49be166f4dd1

📥 Commits

Reviewing files that changed from the base of the PR and between 60d2ec9 and 711feb3.

📒 Files selected for processing (2)

• client/cookiejar.go
• client/cookiejar_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• client/cookiejar_test.go

[gaby]

benchmark / report — the flagged regression is measurement noise

Benchmark_Ctx_IsProxyTrusted/WithProxyCheckParallelMultipleSubnet was reported 1.97x worse (16.46 → 32.43 ns/op). It is noise, not a regression.

This branch does not touch IsProxyTrusted or anything on its call path. The only change to req.go is inside Body()'s Content-Encoding handling.

Measured locally, 6 runs each at -benchtime=2s:

	ns/op across 6 runs	spread
this branch	15.42, 31.93, 32.53, 17.30, 16.55, 16.99	2.1x
origin/main	43.71, 15.49, 16.27, 45.62, 15.85, 15.87	2.9x

origin/main, with none of these changes, swings wider than the 1.97x the alert fired on — its own run-to-run spread exceeds the reported regression. Both sides are 0 B/op and 0 allocs/op. A parallel benchmark measuring ~16 ns of work is dominated by scheduler noise on a shared runner.

No action taken. Happy to be corrected if the harness has a stabler methodology than a raw repeat count.

Also pushed in 3694fe9

• ${locals:} []byte rendering and enforceHostCookieLimitLocked's empty-key deletion — the four lines Codecov reported uncovered. Both were reachable, so they are covered now rather than accepted.
• Set/SetByHost doc comments corrected per the review comment: replacement matches on key and normalized path within one storage scope, so same-key cookies at different paths coexist.

---

Generated by Claude Code

[ReneWerner87]

ok thx for the hint -> i will improve the benchmark flow

[github-actions]

Outdated benchmark report, superseded by a newer run.

⚠️ Performance Alert

2 result(s) got at least 1.50x worse than the base branch.

Regressions

Benchmark	Package	Base	Current	Worse by
BenchmarkMarshalMsgresponse	github.com/gofiber/fiber/v3/middleware/idempotency	73.81 ns/op	125.3 ns/op	1.70x
BenchmarkResolvePolicy_Override	github.com/gofiber/fiber/v3/middleware/proxy	68.94 ns/op	110.8 ns/op	1.61x

Improvements (at least 1.50x)

Benchmark	Package	Base	Current	Better by
Benchmark_Ctx_SendFile	github.com/gofiber/fiber/v3	12748 ns/op	5976 ns/op	2.13x

Measured on Ampere-1a (GOMAXPROCS=4).

Compared 1584 of 1584 results against the base branch.

[gaby]

The _Parallel naming fix in 29ef0c0 worked — the ten IsProxyTrusted sub-benchmarks are gone from the comparison (1614 → 1584 results) after tripping five reports in a row. Thanks for tracking that down; the naming mismatch was a better root cause than the "just noise" I'd landed on.

The latest run flags two different ones, though, and both are in packages this PR does not touch:

Benchmark	Package	Base → Current
BenchmarkMarshalMsgresponse	middleware/idempotency	73.81 → 125.3 ns/op (1.70x)
BenchmarkResolvePolicy_Override	middleware/proxy	68.94 → 110.8 ns/op (1.61x)

Neither middleware/idempotency nor middleware/proxy appears in this branch's 36-file diff, so the changes here cannot be the cause.

Benchmark_Ctx_SendFile is the clearest illustration that sequential benchmarks at this scale still misfire — it has now returned three different verdicts on the same branch:

• 5301 → 9799 ns/op — 1.85x worse
• 11708 → 5432 ns/op — 2.16x better
• 12748 → 5976 ns/op — 2.13x better

Its base value alone moved 5301 → 12748 across runs, so the baseline is drifting as much as the measurement. Flagging in case it is useful input for the harness work — no action needed on this PR's side.

---

Generated by Claude Code

[ReneWerner87]

Docs check out on the facts. The tag list matches tags.go, and the ${method}/${protocol} reasoning holds against fasthttp's request-line parsing. A few points on completeness and placement:

The caution has no supported API behind it. It asks readers to sanitize the values they write from a custom tag, but nothing is exported for that: internal/logtemplate is not importable and utils/v2 has no equivalent. Exporting a helper, or inlining a short strings.Map snippet, would make it actionable.

The warning is far from the code it applies to. The custom tag guide at logger.md:148 and the CustomTags example at :192 both show plain output.WriteString(...); the caution appears at :300. A pointer at both spots would connect them.

RegisterContextTag and LoggerFunc are not covered. context_tag.go:57/:65 writes unscrubbed, and the built-in tags on that path are safe only because each middleware sanitizes at the source. LoggerFunc replaces the pipeline entirely. Separately, the comment at logger.go:124 refers custom implementations to logtemplate.ExecuteChains, which is internal and not importable.

The CookieJar.Set change has no migration note in the docs. It is flagged IMPORTANT in the description, but whats_new.md is untouched and examples.md covers only the Set-Cookie side, so the note currently lives only in the PR body.

Minor: TagScheme is absent from the constants block at :327, leaving ${scheme} introduced by the new section and defined nowhere else. C1 controls are not scrubbed, while basicauth does strip them. The new section is also the only hard-wrapped paragraph in the file.

[gaby]

Thanks — that's a thorough read, and every finding held up when I checked it against the code. All addressed in ede8793.

1. Caution asked for something unavailable. Correct, and it was the worst of the five: the section told people to sanitize with no supported way to do it. I went with exporting rather than an inline snippet — logger.SanitizeValue(s string) string. A hand-rolled strings.Map invites getting the details wrong (tab is preserved, DEL is not), and exporting keeps the guidance and the implementation from drifting. It delegates to the same sanitizeLogValue the built-in tags use, and a test asserts the two cannot diverge.

2. Placement. Both sites now point at the section: a line after the RegisterTag paragraph at :150, and a clause in the CustomTags lead-in at :194.

3. RegisterContextTag. Confirmed — context_tag.go:57 and :65 both write with a bare output.WriteString(v). It is in the caution now, and I added your point that ${username}, ${api-key}, ${csrf-token}, ${requestid} and ${session-id} are safe only because each middleware defends at the source. That is genuinely not inferable from the docs.

4. Config.LoggerFunc. Confirmed at logger.go:153, and you are right that it is the wider hole — it bypasses the pipeline entirely rather than just one tag. Added. I also rewrote the logtemplate.ExecuteChains comment; it now says the helper lives in internal/ and is unreachable, and points at walking Data.TemplateChain / Data.LogFuncChain instead.

5. CookieJar.Set. Fair — it existed only in the PR description and a godoc. Now a :::caution in docs/whats_new.md beside the Cookie Jar migration row, with the SetPath("/") escape hatch and a note that SetByHost / SetKeyValue / SetKeyValueBytes are unaffected.

Smaller: TagScheme added to ## Constants in code order after TagProtocol; heading is now ## Control-Character Sanitization; the hard-wrapped paragraph is unwrapped to one line per paragraph; and the ASCII limit is stated explicitly, naming NEL U+0085 and contrasting with basicauth's containsCTL, which reaches C1 through unicode.IsControl.

One thing I did not do, since it is a security-behavior call rather than a docs one: extend the scrub set to C1. NEL is a real forging vector against pipelines that treat it as a line break, so it is arguably in scope for #4341 — but it would change the SWAR fast path and wants its own benchmark. Happy to do it here or as a follow-up, whichever you prefer.

Verification: markdownlint clean on both files, build/vet/gofmt clean, full suite green, golangci-lint 0 issues.

---

Generated by Claude Code