Skip to content

🐛 bug: fix redirection flash messages violate cookie structure#3457

Merged
ReneWerner87 merged 5 commits intomainfrom
fix-redirect
May 18, 2025
Merged

🐛 bug: fix redirection flash messages violate cookie structure#3457
ReneWerner87 merged 5 commits intomainfrom
fix-redirect

Conversation

@efectn
Copy link
Member

@efectn efectn commented May 15, 2025

Description

This issue fixes redirection with flash messages breaks HTTP header format by encoding msgpack serialized binary data with hex. It has little overhead, but still we get benefit of using Messagepack. Here are the benchmark results:

New:
Benchmark_Redirect_Route-16                         	 6792206	       173.7 ns/op	      16 B/op	       1 allocs/op
Benchmark_Redirect_Route_WithQueries-16             	 3043641	       391.3 ns/op	      40 B/op	       2 allocs/op
Benchmark_Redirect_Route_WithFlashMessages-16       	 2439327	       481.1 ns/op	     293 B/op	       3 allocs/op
Benchmark_Redirect_parseAndClearFlashMessages-16    	 2350215	       510.5 ns/op	     192 B/op	       7 allocs/op
Benchmark_Redirect_processFlashMessages-16          	 3483320	       344.6 ns/op	     288 B/op	       2 allocs/op
Benchmark_Redirect_Messages-16                      	13537572	        87.54 ns/op	     128 B/op	       2 allocs/op
Benchmark_Redirect_OldInputs-16                     	14712171	        77.44 ns/op	      96 B/op	       2 allocs/op
Benchmark_Redirect_Message-16                       	83187558	        14.43 ns/op	       0 B/op	       0 allocs/op
Benchmark_Redirect_OldInput-16                      	153820603	         7.814 ns/op	       0 B/op	       0 allocs/op

Old:
Benchmark_Redirect_Route-16                         	 6861298	       175.9 ns/op	      16 B/op	       1 allocs/op
Benchmark_Redirect_Route_WithQueries-16             	 2901906	       408.6 ns/op	      40 B/op	       2 allocs/op
Benchmark_Redirect_Route_WithFlashMessages-16       	 3456422	       328.7 ns/op	     117 B/op	       2 allocs/op
Benchmark_Redirect_parseAndClearFlashMessages-16    	 4298755	       278.1 ns/op	      32 B/op	       6 allocs/op
Benchmark_Redirect_processFlashMessages-16          	 6022623	       198.2 ns/op	     112 B/op	       1 allocs/op
Benchmark_Redirect_Messages-16                      	13548512	        87.84 ns/op	     128 B/op	       2 allocs/op
Benchmark_Redirect_OldInputs-16                     	14798696	        78.96 ns/op	      96 B/op	       2 allocs/op
Benchmark_Redirect_Message-16                       	82884747	        14.44 ns/op	       0 B/op	       0 allocs/op
Benchmark_Redirect_OldInput-16                      	148364563	         8.104 ns/op	       0 B/op	       0 allocs/op

Fixes #3437

Type of change

  • Enhancement (improvement to existing features and functionality)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • 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.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

Commit formatting

Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

@efectn efectn requested a review from a team as a code owner May 15, 2025 15:35
@efectn efectn requested review from ReneWerner87, gaby and sixcolors May 15, 2025 15:35
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 15, 2025

"""

Walkthrough

The changes implement hex encoding and decoding for flash message data stored in cookies. In both the main logic and corresponding tests, flash messages are now hex-encoded before being set in cookies and hex-decoded when read, replacing previous direct usage of raw byte slices. Error handling for decoding is also introduced in tests.

Changes

Files/Paths Change Summary
redirect.go Added hex encoding when setting flash message cookies and hex decoding when reading them.
redirect_test.go Updated all flash message cookie handling in tests to use hex encoding/decoding, with added decoding error checks; added new tests for invalid hex, full flow, and special characters.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Server

    Client->>Server: Sends request triggering redirect with flash message
    Server->>Server: Marshal flash messages
    Server->>Server: Hex-encode marshaled data
    Server->>Client: Set cookie with hex-encoded flash message
    Client->>Server: Sends subsequent request with cookie
    Server->>Server: Read cookie value
    Server->>Server: Hex-decode cookie value
    Server->>Server: Unmarshal flash messages
    Server->>Client: Respond with flash message
Loading

Assessment against linked issues

Objective Addressed Explanation
Prevent null bytes in HTTP headers by encoding flash messages in cookies (#3437)
Ensure flash message serialization/deserialization is HTTP-compliant (#3437)
Update tests to reflect new encoding and add error handling (#3437)

Possibly related PRs

Suggested reviewers

  • ReneWerner87
  • sixcolors

Poem

A hop and a skip, a bug’s on the run—
Flash messages in cookies, now hexed for fun!
No more null bytes to ruin the day,
With each redirect, the rabbits say “Yay!”
🍪➡️🐇✨
Safe and sweet, the Fiber way!
"""

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.


Note

⚡️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.


📜 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 fcb5ba5 and 3a35702.

📒 Files selected for processing (1)
  • redirect_test.go (16 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Analyse
  • GitHub Check: Compare
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: repeated
🔇 Additional comments (15)
redirect_test.go (15)

9-14: New imports support the hex encoding implementation for flash messages

The addition of encoding/hex enables the hex encoding/decoding of flash message data as described in the PR objectives. Additional imports (encoding/json, io, net/http, net/http/httptest) support the new integration tests that verify the complete flash message workflow.


53-57: Proper hex decoding with immediate error checking

The code now correctly hex-decodes the flash cookie data before unmarshaling it, with immediate error verification using require.NoError(). This implementation properly handles the change from using raw bytes to hex-encoded strings in cookies.


199-202: Consistent implementation of hex decoding

The implementation of hex decoding is consistent with the previous example and properly checks for errors immediately after decoding, which addresses the ineffectual error assignment issue mentioned in the previous review.


423-423: Setting correctly encoded flash cookie for test

The flash cookie is now properly set using hex encoding of the marshaled message data, consistent with the implementation changes in the main code.


477-482: Testing with invalid flash cookie value

This test section verifies that when a non-hex-encoded flash cookie is provided, the messages remain empty after parsing. This ensures graceful handling of malformed cookies.


484-509: Comprehensive test for invalid hex data

Excellent addition of a dedicated test for the invalid hex data case. This test ensures that the system gracefully handles malformed hex data in the flash cookie without crashing, which is a critical edge case for robust error handling.


511-572: End-to-end test for flash message redirection

This comprehensive test validates the complete redirect flow with flash messages, which directly addresses the request in the PR comments for a test that verifies the entire process. The test effectively:

  1. Sets multiple flash messages and redirects
  2. Verifies the redirect status and headers
  3. Extracts the flash cookie
  4. Makes a second request with the cookie
  5. Confirms the flash messages are correctly received

This test is especially valuable as it uses the actual HTTP handlers rather than just unit testing the internal components.


574-636: Robust testing of special characters in flash messages

This test excellently covers edge cases with flash messages containing potentially problematic characters such as null bytes, control characters, Unicode, and emojis. This is critical for ensuring the hex encoding solution properly handles all types of content without corruption, addressing the core issue fixed by this PR.


722-725: Consistent hex decoding in benchmarks

The benchmark tests have been properly updated to use hex decoding, maintaining consistency with the implementation changes in the main code. Error handling is also included, which is good practice even in benchmarks.


764-764: Hex-encoded flash cookie in benchmark setup

The benchmark setup correctly uses hex encoding for the flash cookie, ensuring that the benchmarks test the actual implemented behavior.


815-818: Consistent error handling in benchmarks

The benchmark correctly includes error handling for the hex decoding operation, ensuring that any issues are caught during the tests.


837-838: Properly hex-encoded cookie in benchmark

The benchmark setup correctly uses hex encoding for the flash cookie values, maintaining consistency with the implementation changes.


874-875: Consistent use of hex encoding in benchmarks

Again, this benchmark properly uses hex encoding for the flash cookie, ensuring consistent testing of the implemented behavior.


909-910: Hex-encoded flash cookie in message benchmark

The benchmark consistently uses hex encoding for the flash cookie, properly testing the implementation.


940-941: Hex-encoded flash cookie in old input benchmark

The final benchmark also correctly uses hex encoding for the flash cookie, maintaining consistency throughout the test suite.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov
Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.20%. Comparing base (fa1f8c9) to head (3a35702).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3457      +/-   ##
==========================================
+ Coverage   84.10%   84.20%   +0.10%     
==========================================
  Files         119      119              
  Lines       11939    11947       +8     
==========================================
+ Hits        10041    10060      +19     
+ Misses       1468     1460       -8     
+ Partials      430      427       -3     
Flag Coverage Δ
unittests 84.20% <100.00%> (+0.10%) ⬆️

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa1f8c9 and d159f1e.

📒 Files selected for processing (2)
  • redirect.go (3 hunks)
  • redirect_test.go (15 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
redirect.go

[failure] 300-300:
Error return value of hex.DecodeString is not checked (errcheck)

redirect_test.go

[failure] 325-325:
ineffectual assignment to err (ineffassign)

🪛 GitHub Actions: golangci-lint
redirect.go

[error] 300-300: golangci-lint: Error return value of hex.DecodeString is not checked (errcheck)

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.23.x, macos-latest)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: Analyse
  • GitHub Check: Compare
🔇 Additional comments (3)
redirect.go (1)

320-326: LGTM! Hex encoding implementation

Properly implementing hex encoding for the cookie value resolves the issue with flash messages violating cookie structure.

redirect_test.go (2)

417-417: LGTM! Correctly updated test cases

All test cases have been correctly updated to use hex encoding/decoding, maintaining consistency with the implementation changes.

Also applies to: 598-598, 671-671, 708-708, 743-743, 774-774


49-52: LGTM! Updated cookie decoding with proper error handling

The test decoding logic has been updated to handle hex-encoded cookie values correctly, with proper error checks in place.

Also applies to: 194-197, 247-250, 286-289, 369-372, 556-559, 649-652

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d159f1e and 1a744ec.

📒 Files selected for processing (2)
  • redirect.go (3 hunks)
  • redirect_test.go (15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • redirect_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
redirect.go

[warning] 302-303: redirect.go#L302-L303
Added lines #L302 - L303 were not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Analyse
  • GitHub Check: Compare
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: unit (1.23.x, macos-latest)
  • GitHub Check: unit (1.23.x, macos-13)
🔇 Additional comments (3)
redirect.go (3)

8-8: Appropriate import added for hex encoding

The addition of the hex package import is necessary for the new encoding approach and aligns with the PR objective to fix cookie structure violations.


323-325: Hex encoding implementation is correct and efficient

The implementation correctly allocates a buffer of the appropriate size and performs hex encoding of the MessagePack serialized data. This approach fixes the cookie structure violation issue while maintaining the benefits of MessagePack serialization.


328-328: Properly using hex-encoded value for cookie

The cookie now uses the hex-encoded value instead of the binary data, which completes the fix for the cookie structure violation.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7cd33f and 4ca3cae.

📒 Files selected for processing (2)
  • redirect.go (3 hunks)
  • redirect_test.go (16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • redirect_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
redirect.go (1)
ctx.go (1)
  • Cookie (180-192)
🪛 GitHub Check: codecov/patch
redirect.go

[warning] 302-303: redirect.go#L302-L303
Added lines #L302 - L303 were not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: unit (1.24.x, macos-13)
  • GitHub Check: Compare
  • GitHub Check: repeated
🔇 Additional comments (3)
redirect.go (3)

8-8: Appropriate import addition for hex encoding/decoding

The addition of the hex package is necessary for the implementation of hex encoding and decoding for flash messages in cookies.


323-325: Hex encoding correctly implemented

The code properly allocates a buffer of the right size using hex.EncodedLen() and encodes the binary data using hex.Encode(). This implementation avoids cookie structure violations by ensuring the binary data is represented in a cookie-safe format.


328-328: Cookie value properly set with hex-encoded data

The cookie value is now correctly set using the hex-encoded flash message data, which resolves the issue of cookie structure violations.

@ReneWerner87
Copy link
Member

@coderabbitai can you please generate more test cases
which test the complete process coherently

i.e.
in the code a redirect with a flash message is set, then we expect as response a status code with redirect and a url and a cookie
we now send this as well as the client or browser in a 2nd request to the server with the redirect url and the cookie header from the previous response and expect that in the response by the handler our flash message from is output (this message initially came from the first handler)

@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 16, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@ReneWerner87 Based on my analysis of the codebase, I've created a comprehensive test case that covers the complete flow of the redirect process with flash messages. This test simulates a realistic scenario with a first handler setting flash messages and redirecting, followed by a second request that should receive those messages.

func Test_Redirect_CompleteFlowWithFlashMessages(t *testing.T) {
	t.Parallel()

	app := New()
	
	// Create a handler that sets flash messages and redirects
	app.Get("/source", func(c *Ctx) error {
		// Set multiple types of flash messages to test encoding/decoding
		c.Flash("string_message", "Hello, World!")
		c.Flash("number_message", 12345)
		c.Flash("bool_message", true)
		c.Flash("map_message", map[string]string{"key": "value"})
		c.Flash("slice_message", []string{"one", "two", "three"})
		
		// Redirect to the target handler
		return c.Redirect("/target", StatusFound)
	})
	
	// Create a handler that receives and processes flash messages
	app.Get("/target", func(c *Ctx) error {
		// Get all flash messages and return them as a JSON response
		flashMessages := make(map[string]interface{})
		flashMessages["string_message"] = c.Flash("string_message")
		flashMessages["number_message"] = c.Flash("number_message")
		flashMessages["bool_message"] = c.Flash("bool_message")
		flashMessages["map_message"] = c.Flash("map_message")
		flashMessages["slice_message"] = c.Flash("slice_message")
		
		return c.JSON(flashMessages)
	})
	
	// Step 1: Make the initial request to the source route
	req := httptest.NewRequest(MethodGet, "/source", nil)
	resp, err := app.Test(req)
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, StatusFound, resp.StatusCode)
	utils.AssertEqual(t, "/target", resp.Header.Get(HeaderLocation))
	
	// Get the cookie from the response
	cookies := resp.Cookies()
	var flashCookie *http.Cookie
	for _, cookie := range cookies {
		if cookie.Name == "fiber_flash" {
			flashCookie = cookie
			break
		}
	}
	utils.AssertEqual(t, true, flashCookie != nil, "Expected flash cookie to be set")
	
	// Step 2: Make the second request to the target route with the cookie
	req = httptest.NewRequest(MethodGet, "/target", nil)
	req.Header.Set("Cookie", flashCookie.Name+"="+flashCookie.Value)
	resp, err = app.Test(req)
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, StatusOK, resp.StatusCode)
	
	// Step 3: Parse the JSON response and verify flash messages
	body, err := io.ReadAll(resp.Body)
	utils.AssertEqual(t, nil, err)
	
	var result map[string]interface{}
	err = json.Unmarshal(body, &result)
	utils.AssertEqual(t, nil, err)
	
	// Verify all flash messages were received correctly
	utils.AssertEqual(t, "Hello, World!", result["string_message"])
	utils.AssertEqual(t, float64(12345), result["number_message"]) // JSON numbers are float64
	utils.AssertEqual(t, true, result["bool_message"])
	
	// Check map_message
	mapMsg, ok := result["map_message"].(map[string]interface{})
	utils.AssertEqual(t, true, ok)
	utils.AssertEqual(t, "value", mapMsg["key"])
	
	// Check slice_message
	sliceMsg, ok := result["slice_message"].([]interface{})
	utils.AssertEqual(t, true, ok)
	utils.AssertEqual(t, 3, len(sliceMsg))
	utils.AssertEqual(t, "one", sliceMsg[0])
	utils.AssertEqual(t, "two", sliceMsg[1])
	utils.AssertEqual(t, "three", sliceMsg[2])
}

Here's a test that focuses on edge cases with large data and special characters:

func Test_Redirect_FlashMessagesWithSpecialChars(t *testing.T) {
	t.Parallel()

	app := New()
	
	// Create a handler that sets flash messages with special characters and redirects
	app.Get("/special-source", func(c *Ctx) error {
		// Test with messages containing special characters that could break headers
		c.Flash("null_bytes", "Contains\x00null\x00bytes")
		c.Flash("control_chars", "Contains\r\ncontrol\tcharacters")
		c.Flash("unicode", "Unicode: 你好世界")
		c.Flash("emoji", "Emoji: 🔥🚀😊")
		
		// Create a large message to test encoding of larger data
		largeMessage := strings.Repeat("abcdefghijklmnopqrstuvwxyz", 100)
		c.Flash("large_message", largeMessage)
		
		return c.Redirect("/special-target", StatusFound)
	})
	
	// Create a target handler
	app.Get("/special-target", func(c *Ctx) error {
		return c.JSON(fiber.Map{
			"null_bytes": c.Flash("null_bytes"),
			"control_chars": c.Flash("control_chars"),
			"unicode": c.Flash("unicode"),
			"emoji": c.Flash("emoji"),
			"large_message_correct": c.Flash("large_message") == strings.Repeat("abcdefghijklmnopqrstuvwxyz", 100),
			"large_message_length": len(c.Flash("large_message").(string)),
		})
	})
	
	// Step 1: Make the initial request
	req := httptest.NewRequest(MethodGet, "/special-source", nil)
	resp, err := app.Test(req)
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, StatusFound, resp.StatusCode)
	
	// Get the flash cookie
	var flashCookie *http.Cookie
	for _, cookie := range resp.Cookies() {
		if cookie.Name == "fiber_flash" {
			flashCookie = cookie
			break
		}
	}
	utils.AssertEqual(t, true, flashCookie != nil)
	
	// Step 2: Make the second request with the cookie
	req = httptest.NewRequest(MethodGet, "/special-target", nil)
	req.Header.Set("Cookie", flashCookie.Name+"="+flashCookie.Value)
	resp, err = app.Test(req)
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, StatusOK, resp.StatusCode)
	
	// Step 3: Parse and verify the response
	body, err := io.ReadAll(resp.Body)
	utils.AssertEqual(t, nil, err)
	
	var result map[string]interface{}
	err = json.Unmarshal(body, &result)
	utils.AssertEqual(t, nil, err)
	
	// Verify special character handling
	utils.AssertEqual(t, "Contains\x00null\x00bytes", result["null_bytes"])
	utils.AssertEqual(t, "Contains\r\ncontrol\tcharacters", result["control_chars"])
	utils.AssertEqual(t, "Unicode: 你好世界", result["unicode"])
	utils.AssertEqual(t, "Emoji: 🔥🚀😊", result["emoji"])
	utils.AssertEqual(t, true, result["large_message_correct"])
	utils.AssertEqual(t, 26*100, int(result["large_message_length"].(float64)))
}

Would you like me to generate any additional test cases that focus on specific aspects of the flow?


🏁 Script executed:

#!/bin/bash
# Get the content of redirect.go to understand the implementation
cat redirect.go

Length of output: 7854


🏁 Script executed:

#!/bin/bash
# Get the content of redirect_test.go to understand existing tests
cat redirect_test.go

Length of output: 24641


🏁 Script executed:

#!/bin/bash
# Check if there's an app.go file and look for examples of app setup in tests
ls app.go || echo "No app.go file found"

# Look for examples of app setup in existing tests
grep -A 10 "New(" redirect_test.go | head -n 20

# Look for examples of App handler definitions in tests
grep -A 10 "app\.Get" redirect_test.go | head -n 20

Length of output: 1358


@ReneWerner87 I've created two comprehensive test cases that test the complete flow of redirect with flash messages, simulating the full client/server experience:

func Test_Redirect_CompleteFlowWithFlashMessages(t *testing.T) {
	t.Parallel()

	app := New()
	
	// First handler that sets flash messages and redirects
	app.Get("/source", func(c Ctx) error {
		// Set multiple types of flash messages to test encoding/decoding
		c.Flash("string_message", "Hello, World!")
		c.Flash("number_message", 12345)
		c.Flash("bool_message", true)
		c.Flash("map_message", map[string]string{"key": "value"})
		c.Flash("slice_message", []string{"one", "two", "three"})
		
		// Redirect to the target handler
		return c.Redirect().To("/target")
	})
	
	// Second handler that receives and processes flash messages
	app.Get("/target", func(c Ctx) error {
		// Get all flash messages and return them as a JSON response
		return c.JSON(fiber.Map{
			"string_message": c.Flash("string_message"),
			"number_message": c.Flash("number_message"),
			"bool_message":   c.Flash("bool_message"),
			"map_message":    c.Flash("map_message"),
			"slice_message":  c.Flash("slice_message"),
		})
	})
	
	// Step 1: Make the initial request to the source route
	req := httptest.NewRequest(MethodGet, "/source", nil)
	resp, err := app.Test(req)
	require.NoError(t, err)
	require.Equal(t, StatusSeeOther, resp.StatusCode)
	require.Equal(t, "/target", resp.Header.Get(HeaderLocation))
	
	// Verify and get the cookie from the response
	cookies := resp.Cookies()
	var flashCookie *http.Cookie
	for _, cookie := range cookies {
		if cookie.Name == "fiber_flash" {
			flashCookie = cookie
			break
		}
	}
	require.NotNil(t, flashCookie, "Flash cookie should be set")
	
	// Step 2: Make the second request to the target route with the cookie
	req = httptest.NewRequest(MethodGet, "/target", nil)
	req.Header.Set("Cookie", flashCookie.Name+"="+flashCookie.Value)
	resp, err = app.Test(req)
	require.NoError(t, err)
	require.Equal(t, StatusOK, resp.StatusCode)
	
	// Parse the JSON response and verify flash messages
	body, err := io.ReadAll(resp.Body)
	require.NoError(t, err)
	
	var result map[string]interface{}
	err = json.Unmarshal(body, &result)
	require.NoError(t, err)
	
	// Verify all flash messages were received correctly
	require.Equal(t, "Hello, World!", result["string_message"])
	require.Equal(t, float64(12345), result["number_message"]) // JSON numbers are float64
	require.Equal(t, true, result["bool_message"])
	
	// Check map_message
	mapMsg, ok := result["map_message"].(map[string]interface{})
	require.True(t, ok)
	require.Equal(t, "value", mapMsg["key"])
	
	// Check slice_message
	sliceMsg, ok := result["slice_message"].([]interface{})
	require.True(t, ok)
	require.Equal(t, 3, len(sliceMsg))
	require.Equal(t, "one", sliceMsg[0])
	require.Equal(t, "two", sliceMsg[1])
	require.Equal(t, "three", sliceMsg[2])
}

And a second test that focuses on the specific edge cases the PR is fixing (null bytes, special characters, etc.):

func Test_Redirect_FlashMessagesWithSpecialChars(t *testing.T) {
	t.Parallel()

	app := New()
	
	// Handler that sets flash messages with special characters and redirects
	app.Get("/special-source", func(c Ctx) error {
		// Set flash messages with content that would previously break headers
		c.Flash("null_bytes", "Contains\x00null\x00bytes")
		c.Flash("control_chars", "Contains\r\ncontrol\tcharacters")
		c.Flash("unicode", "Unicode: 你好世界")
		c.Flash("emoji", "Emoji: 🔥🚀😊")
		
		// Create a large message to test encoding of larger data
		largeMessage := strings.Repeat("abcdefghijklmnopqrstuvwxyz", 100)
		c.Flash("large_message", largeMessage)
		
		return c.Redirect().To("/special-target")
	})
	
	// Target handler that receives the flash messages
	app.Get("/special-target", func(c Ctx) error {
		return c.JSON(fiber.Map{
			"null_bytes": c.Flash("null_bytes"),
			"control_chars": c.Flash("control_chars"),
			"unicode": c.Flash("unicode"),
			"emoji": c.Flash("emoji"),
			"large_message_correct": c.Flash("large_message") == strings.Repeat("abcdefghijklmnopqrstuvwxyz", 100),
			"large_message_length": len(c.Flash("large_message").(string)),
		})
	})
	
	// Step 1: Make the initial request
	req := httptest.NewRequest(MethodGet, "/special-source", nil)
	resp, err := app.Test(req)
	require.NoError(t, err)
	require.Equal(t, StatusSeeOther, resp.StatusCode)
	require.Equal(t, "/special-target", resp.Header.Get(HeaderLocation))
	
	// Get the flash cookie
	var flashCookie *http.Cookie
	for _, cookie := range resp.Cookies() {
		if cookie.Name == "fiber_flash" {
			flashCookie = cookie
			break
		}
	}
	require.NotNil(t, flashCookie, "Flash cookie should be set")
	
	// Step 2: Make the second request with the cookie
	req = httptest.NewRequest(MethodGet, "/special-target", nil)
	req.Header.Set("Cookie", flashCookie.Name+"="+flashCookie.Value)
	resp, err = app.Test(req)
	require.NoError(t, err)
	require.Equal(t, StatusOK, resp.StatusCode)
	
	// Parse and verify the response
	body, err := io.ReadAll(resp.Body)
	require.NoError(t, err)
	
	var result map[string]interface{}
	err = json.Unmarshal(body, &result)
	require.NoError(t, err)
	
	// Verify special character handling
	require.Equal(t, "Contains\x00null\x00bytes", result["null_bytes"])
	require.Equal(t, "Contains\r\ncontrol\tcharacters", result["control_chars"])
	require.Equal(t, "Unicode: 你好世界", result["unicode"])
	require.Equal(t, "Emoji: 🔥🚀😊", result["emoji"])
	require.Equal(t, true, result["large_message_correct"])
	require.Equal(t, 26*100, int(result["large_message_length"].(float64)))
}

Would you like me to create any additional test scenarios that focus on other aspects of the redirect flow with flash messages?

@gaby gaby requested a review from Copilot May 16, 2025 12:40
@gaby gaby added the v3 label May 16, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

The PR fixes a bug in flash message redirection where the HTTP header was being broken by using raw msgpack binary data, by switching to hex encoding/decoding of the flash cookie values. Key changes include:

  • Updating tests to decode the flash cookie value using hex.DecodeString before unmarshalling.
  • Modifying processFlashMessages and parseAndClearFlashMessages to use hex encoding instead of plain string conversion.
  • Adjusting benchmarks to reflect the new hex encoding in flash message processing.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
redirect_test.go Updated tests and benchmarks to decode/encode flash cookies in hex
redirect.go Modified flash message parsing and processing to use hex encoding
Comments suppressed due to low confidence (1)

redirect.go:323

  • [nitpick] Consider renaming the variable 'dst' to something like 'encodedVal' to enhance code clarity.
dst := make([]byte, hex.EncodedLen(len(val)))

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
redirect_test.go (2)

564-564: Consider using 'any' instead of 'interface{}'.

Since Go 1.18, you can use 'any' as a more concise alias for 'interface{}'.

-var result map[string]interface{}
+var result map[string]any
🧰 Tools
🪛 GitHub Check: lint

[failure] 564-564:
use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)


627-627: Consider using 'any' instead of 'interface{}'.

Since Go 1.18, you can use 'any' as a more concise alias for 'interface{}'.

-var result map[string]interface{}
+var result map[string]any
🧰 Tools
🪛 GitHub Check: lint

[failure] 627-627:
use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4ca3cae and fcb5ba5.

📒 Files selected for processing (1)
  • redirect_test.go (16 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
redirect_test.go

[failure] 627-627:
use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)


[failure] 564-564:
use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: unit (1.24.x, macos-13)
  • GitHub Check: unit (1.24.x, macos-latest)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: Analyse
  • GitHub Check: Compare
🔇 Additional comments (7)
redirect_test.go (7)

9-14: Appropriate imports for the newly added test functionality.

The added imports accurately support the new functionality, particularly hex encoding for flash messages and additional HTTP testing capabilities.


54-57: Correctly implemented hex decoding with proper error handling.

The code now properly hex-decodes the flash cookie data before unmarshaling, with appropriate error handling. This aligns with the fix in the main implementation to prevent HTTP header structure violations.


423-423: Properly encoding flash message data with hex.

All instances of setting the flash cookie now use hex encoding, which fixes the issue where binary data in cookies violated HTTP header structure requirements.


477-482: Good addition of error case handling.

Testing the case where the flash cookie contains an invalid value ("test") ensures the system handles malformed inputs gracefully.


484-509: Comprehensive testing for invalid hex data.

This test case specifically validates the system's behavior when encountering improperly formatted hex data in the flash cookie, ensuring no messages are processed rather than causing errors or undefined behavior.


511-572: Excellent end-to-end test coverage.

This comprehensive test verifies the complete redirect flow with flash messages using standard Go HTTP testing utilities, validating both the redirect mechanism and the proper transmission of various data types through flash messages.

🧰 Tools
🪛 GitHub Check: lint

[failure] 564-564:
use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)


574-636: Thorough edge case testing with special characters.

Testing with null bytes, control characters, Unicode, and emoji ensures the hex encoding solution properly handles all types of data that might otherwise break HTTP headers.

🧰 Tools
🪛 GitHub Check: lint

[failure] 627-627:
use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)

@ReneWerner87 ReneWerner87 added this to v3 May 18, 2025
@ReneWerner87 ReneWerner87 added this to the v3 milestone May 18, 2025
@ReneWerner87 ReneWerner87 merged commit cf4074f into main May 18, 2025
18 checks passed
@github-project-automation github-project-automation bot moved this to Done in v3 May 18, 2025
mdelapenya added a commit to mdelapenya/fiber that referenced this pull request May 19, 2025
* main:
  Update AGENTS.md
  Create AGENTS.md
  🐛 bug: fix redirection flash messages violate cookie structure (gofiber#3457)
  build(deps): bump codecov/codecov-action from 5.4.2 to 5.4.3
  🚀 Improve routing treeBuild flow (gofiber#3456)
  build(deps): bump github.com/tinylib/msgp from 1.2.5 to 1.3.0 (gofiber#3447)
  build(deps): bump DavidAnson/markdownlint-cli2-action from 19 to 20
  🔥 feat: Add All method to Bind (gofiber#3373)
  📝 docs: Document usage of Custom Tags in Logger middleware (gofiber#3446)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

🐛 [Bug]: MessagePack serialization broke flash messages

4 participants