🐛 bug: Remove Flash Cookie from Response headers after parsing by gaby · Pull Request #3840 · gofiber/fiber

gaby · 2025-11-04T14:24:07Z

Summary

avoid scanning raw headers in App.requestHandler by checking the flash cookie once
refactor redirect flash parsing to accept a provided cookie payload and reuse in tests/benchmarks

coderabbitai · 2025-11-04T14:24:16Z

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

The changes add cookie cleanup logic to the flash message parsing function, ensuring the Flash cookie is deleted after successful parsing. Corresponding test assertions were added to verify the cookie expiration in response headers across redirect operations.

Changes

Cohort / File(s)	Summary
Flash Cookie Cleanup `redirect.go`	Added `r.c.Response().Header.DelClientCookie(FlashCookieName)` call in `parseAndClearFlashMessages` to delete the Flash cookie after successful parsing, executing only when no decoding error occurs.
Verification Tests `redirect_test.go`	Added `strings` import and new runtime assertions across redirect tests to validate Flash cookie eviction by inspecting Set-Cookie header for the pattern `FlashCookieName=; expires=`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Single, straightforward cleanup addition to existing parsing logic
Homogeneous test additions with consistent validation pattern
Minimal scope expansion with low logic density

Possibly related PRs

🐛 bug: fix redirection flash messages violate cookie structure #3457: Modifies flash-cookie handling in the same parseAndClearFlashMessages function, changing encoding/decoding from the current approach to hex format, creating potential interaction points with this cleanup addition.

Suggested reviewers

sixcolors
ReneWerner87
efectn

Poem

🐰 A cookie so fleeting, now cleared with care,
No crumbs left behind in the digital air!
Flash messages parsed, then swept from the scene,
The neatest response cache you've ever seen! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is minimal and lacks required sections from the template such as issue reference, detailed changes, type of change, and comprehensive checklist items.	Add the missing required template sections: issue reference (Fixes #), detailed changes list with checkboxes, type of change selection, and completion of the required checklist items.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: removing the Flash cookie from response headers after parsing, which aligns with the code modifications that delete the FlashCookieName cookie.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch update-flash-cookie-handling-in-redirect-and-router

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc74824 and 93b4bbe.

📒 Files selected for processing (2)

redirect.go (1 hunks)
redirect_test.go (4 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

redirect_test.go
redirect.go

🧠 Learnings (7)

📓 Common learnings

Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

📚 Learning: 2024-07-02T13:29:56.992Z

Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.

Applied to files:

redirect_test.go

📚 Learning: 2024-10-08T19:06:06.583Z

Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.

Applied to files:

redirect_test.go

📚 Learning: 2025-10-16T07:19:52.418Z

Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

redirect_test.go

📚 Learning: 2024-12-13T08:14:22.851Z

Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.

Applied to files:

redirect_test.go

📚 Learning: 2024-07-26T21:00:12.902Z

Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

redirect_test.go

📚 Learning: 2024-09-25T15:57:10.221Z

Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.

Applied to files:

redirect_test.go
redirect.go

🧬 Code graph analysis (2)

redirect_test.go (2)

constants.go (1)

HeaderSetCookie (197-197)

redirect.go (1)

FlashCookieName (29-29)

redirect.go (1)

client/response.go (1)

Response (19-25)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: repeated
GitHub Check: Compare

🔇 Additional comments (4)

redirect.go (2)

309-310: Cookie cleanup implemented correctly.

The addition of DelClientCookie(FlashCookieName) after successful parsing addresses the bug identified in the previous review. The flash cookie is now properly cleared from the client after messages are parsed, preventing the same flash messages from being processed on subsequent requests.

297-311: Consider clearing invalid flash cookies as well.

Currently, when hex decoding fails (line 300-303) or unmarshalling fails (line 305-308), the function returns early without deleting the flash cookie. This means malformed or invalid cookies will persist and be re-parsed on every subsequent request.

Should invalid flash cookies also be cleared to prevent repeated parsing attempts?

redirect_test.go (2)

16-16: LGTM: Import added for cookie verification.

The strings import is correctly added to support the HasPrefix checks for verifying flash cookie expiration in the tests.

478-479: Test coverage for cookie cleanup looks good.

The assertions correctly verify that the flash cookie is expired after parsing by checking for the FlashCookieName+"=; expires=" prefix in the Set-Cookie header. This confirms that DelClientCookie is working as expected to clear the flash cookie from the client.

The coverage spans unit tests (line 478-479) and integration tests (lines 564, 629), ensuring the fix works across different scenarios including special characters.

Also applies to: 564-564, 629-629

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

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

codecov · 2025-11-04T14:25:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.18%. Comparing base (42b159b) to head (5be5257).
⚠️ Report is 86 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3840   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files         115      115           
  Lines        9753     9754    +1     
=======================================
+ Hits         8991     8992    +1     
  Misses        484      484           
  Partials      278      278

Flag	Coverage Δ
unittests	`92.18% <100.00%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

gaby · 2025-11-04T14:26:20Z

/gemini review

Copilot

Pull Request Overview

This PR refactors the flash message cookie parsing logic to improve performance by avoiding unnecessary byte operations. The key change is passing the cookie value directly as a parameter to parseAndClearFlashMessages() instead of retrieving it within the function.

Key changes:

Modified parseAndClearFlashMessages() to accept cookie value as a parameter
Removed the bytes package import from router.go as it's no longer needed
Updated flash message detection to use Cookies() method instead of checking raw headers with bytes.Contains()

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
router.go	Removed `bytes` import; refactored flash cookie detection to retrieve cookie value before calling `parseAndClearFlashMessages()`
redirect.go	Modified `parseAndClearFlashMessages()` to accept `cookieValue` parameter and added early return for empty values
redirect_test.go	Updated all test and benchmark calls to `parseAndClearFlashMessages()` to pass the cookie value as a parameter

gemini-code-assist

Code Review

This pull request introduces a significant performance improvement for flash cookie parsing by replacing an inefficient raw header scan with a direct cookie lookup. The refactoring of parseAndClearFlashMessages is well-executed, making the code cleaner and more testable. The accompanying test updates are appropriate. I have one suggestion to address a potential bug in the flash message handling logic to ensure they are cleared after being read.

redirect.go

github-actions

⚠️ 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: `e9872ac`	Previous: `bc74824`	Ratio
`Benchmark_Redirect_parseAndClearFlashMessages`	`868.7` ns/op 192 B/op 7 allocs/op	`543.2` ns/op 192 B/op 7 allocs/op	`1.60`
`Benchmark_Redirect_parseAndClearFlashMessages - ns/op`	`868.7` ns/op	`543.2` ns/op	`1.60`
`Benchmark_Compress/Zstd - B/op`	`1` B/op	`0` B/op	`+∞`

This comment was automatically generated by workflow using github-action-benchmark.

gaby · 2025-11-05T03:44:02Z

These changes make the router slower, so i'm only going to include the fix for deleting the cookie.

…-router

Optimize flash cookie parsing

95687ad

gaby added the codex label Nov 4, 2025 — with ChatGPT Codex Connector

gaby changed the title ~~Optimize flash cookie parsing~~ ⚡ perf: Optimize router parsing of flash cookie Nov 4, 2025

gaby added v3 ⚡️ Performance labels Nov 4, 2025

gaby requested a review from Copilot November 4, 2025 14:26

Copilot AI reviewed Nov 4, 2025

View reviewed changes

gemini-code-assist bot reviewed Nov 4, 2025

View reviewed changes

redirect.go Show resolved Hide resolved

Clear flash cookie after parsing

be8b5ea

github-actions bot reviewed Nov 4, 2025

View reviewed changes

gaby added 2 commits November 4, 2025 23:50

Restore flash parser signature while clearing cookie

e9872ac

Revert flash cookie guard in router

93b4bbe

gaby added ☢️ Bug and removed ⚡️ Performance labels Nov 5, 2025

gaby changed the title ~~⚡ perf: Optimize router parsing of flash cookie~~ 🐛 bug: Remove Flash Cookie from Response headers after parsing Nov 5, 2025

gaby marked this pull request as ready for review November 5, 2025 04:53

gaby requested a review from a team as a code owner November 5, 2025 04:53

gaby requested review from ReneWerner87, efectn and sixcolors November 5, 2025 04:53

Merge branch 'main' into update-flash-cookie-handling-in-redirect-and…

5be5257

…-router

ReneWerner87 approved these changes Nov 5, 2025

View reviewed changes

ReneWerner87 added this to v3 Nov 5, 2025

ReneWerner87 added this to the v3 milestone Nov 5, 2025

efectn approved these changes Nov 5, 2025

View reviewed changes

gaby merged commit ff04aa0 into main Nov 5, 2025
16 checks passed

gaby deleted the update-flash-cookie-handling-in-redirect-and-router branch November 5, 2025 13:36

github-project-automation bot moved this to Done in v3 Nov 5, 2025

gaby added the 📜 RFC Compliance Feature, implementation, or contribution adheres to relevant RFC standards. label Nov 27, 2025

coderabbitai bot mentioned this pull request Dec 20, 2025

🐛 [Bug]: Malformed Cookie Header Causes Panic in fasthttp collectCookies via Fiber Logger #3957

Closed

3 tasks

coderabbitai bot mentioned this pull request Feb 11, 2026

🐛 bug: harden flash cookie detection #4078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🐛 bug: Remove Flash Cookie from Response headers after parsing#3840

🐛 bug: Remove Flash Cookie from Response headers after parsing#3840
gaby merged 5 commits intomainfrom
update-flash-cookie-handling-in-redirect-and-router

gaby commented Nov 4, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 4, 2025 •

edited

Loading

Other AI code review bot(s) detected

Uh oh!

codecov bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

gaby commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

github-actions bot left a comment •

edited

Loading

Uh oh!

gaby commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

gaby commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

coderabbitai bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gaby commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

gaby commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaby commented Nov 4, 2025 •

edited

Loading

coderabbitai bot commented Nov 4, 2025 •

edited

Loading

codecov bot commented Nov 4, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading