🐛 bug: Remove Flash Cookie from Response headers after parsing

[gaby]

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]

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (bc74824) to head (93b4bbe).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3840      +/-   ##
==========================================
+ Coverage   92.16%   92.20%   +0.03%     
==========================================
  Files         115      115              
  Lines        9744     9745       +1     
==========================================
+ Hits         8981     8985       +4     
+ Misses        488      485       -3     
  Partials      275      275              

Flag	Coverage Δ
unittests	92.20% <100.00%> (+0.03%)	⬆️

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]

/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.

[gemini-code-assist]

The function parseAndClearFlashMessages does not clear the flash cookie from the client's browser after parsing it. This contradicts its name and the expected "flash" behavior, where messages are displayed only once. Without clearing the cookie, the same flash message will be sent by the browser and processed on subsequent requests.

To fix this, the fiber_flash cookie should be deleted from the response headers after it has been successfully parsed.

    r.c.Response().Header.DelClientCookie(FlashCookieName)

[gaby]

@efectn Seems gemini found a bug here? 😂

[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]

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