🔥Feature: Add support for TrustProxy #3170
Conversation
xEricL
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes involve a comprehensive update to the Fiber web framework's configuration settings related to trusted proxies. Key modifications include the renaming of configuration fields and the introduction of a new Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (26)
docs/middleware/earlydata.md (1)
10-10: Update configuration instruction for clarity and conciseness.The updated configuration instruction accurately reflects the change from
EnableTrustedProxyChecktoTrustProxy, aligning with the PR objectives. However, we can improve its clarity and conciseness.Consider revising the line as follows:
-Make sure to enable fiber's `TrustProxy` config option before using this middleware in order to not trust bogus HTTP request headers of the client. +Enable Fiber's `TrustProxy` config option before using this middleware to prevent trusting potentially misleading HTTP request headers from clients.This revision:
- Removes the wordiness flagged by the static analysis tool.
- Maintains the essential information about the configuration change.
- Clarifies the purpose of the configuration option.
🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Consider a shorter alternative to avoid wordiness.
Context: ...fig option before using this middleware in order to not trust bogus HTTP request headers of...(IN_ORDER_TO_PREMIUM)
middleware/earlydata/earlydata_test.go (1)
Line range hint
1-192: Overall, the changes look good and align with the PR objectives.The test cases have been successfully updated to reflect the new
TrustProxyandTrustProxyConfigstructure. The changes are consistent with the PR's goal of renamingEnableTrustedProxyChecktoTrustProxyand introducing the newTrustProxyConfigstruct.To ensure the robustness of these changes:
- Verify that similar updates have been made in all relevant test files across the codebase.
- Consider adding test cases that specifically validate the behavior of the new
TrustProxyConfigstructure, especially if there are any default behaviors or special cases to consider.- Ensure that the documentation (both in-code and external) has been updated to reflect these changes, particularly focusing on migration guidelines for users updating from the previous version.
ctx_interface_gen.go (4)
130-130: LGTM! Consider adding more context to the comment.The update from
Config.EnableTrustedProxyChecktoConfig.TrustProxyaligns with the PR objectives. This change ensures consistency with the new configuration option.Consider expanding the comment to provide more context about the
TrustProxyconfiguration and its implications. For example:-// Please use Config.TrustProxy to prevent header spoofing, in case when your app is behind the proxy. +// Please use Config.TrustProxy to prevent header spoofing when your app is behind a proxy. +// When enabled, it allows the app to trust the X-Forwarded-Host header.
136-136: LGTM! Consider adding more context to the comment.The update from
Config.EnableTrustedProxyChecktoConfig.TrustProxyaligns with the PR objectives and ensures consistency with the new configuration option.Consider expanding the comment to provide more context about the
TrustProxyconfiguration and its implications, similar to the suggestion for theHost()method. For example:-// Please use Config.TrustProxy to prevent header spoofing, in case when your app is behind the proxy. +// Please use Config.TrustProxy to prevent header spoofing when your app is behind a proxy. +// When enabled, it allows the app to trust the X-Forwarded-Host header for hostname extraction.
142-142: LGTM! Consider adding more context to the comment.The update from
Config.EnableTrustedProxyChecktoConfig.TrustProxyaligns with the PR objectives and ensures consistency with the new configuration option.Consider expanding the comment to provide more context about the
TrustProxyconfiguration and its implications for IP address handling. For example:-// Please use Config.TrustProxy to prevent header spoofing, in case when your app is behind the proxy. +// Please use Config.TrustProxy to prevent IP spoofing when your app is behind a proxy. +// When enabled, it allows the app to trust headers like X-Forwarded-For for determining the client's IP address.
318-318: LGTM! Consider clarifying the comment.The update from
Config.EnableTrustedProxyChecktoConfig.TrustProxyaligns with the PR objectives and ensures consistency with the new configuration option.Consider clarifying the comment to better explain the behavior of the
IsProxyTrusted()method. For example:-// If Config.TrustProxy false, it returns true +// Returns true if Config.TrustProxy is true and the remote IP is trusted, or if Config.TrustProxy is false (trusting all IPs).This clarification helps developers understand that when
TrustProxyis false, all IPs are considered trusted, which is why the method returns true in that case.docs/whats_new.md (5)
58-59: LGTM! Consider adding more context to the documentation.The changes to
TrustProxyandTrustProxyConfigaccurately reflect the PR objectives. This improvement simplifies the configuration process for trusted proxies.Consider adding a brief explanation of why these changes were made and how they benefit users, especially in terms of performance and usability when configuring trusted proxies.
391-419: LGTM! Consider adding a note about performance benefits.The migration guide clearly demonstrates how to update the TrustProxy configuration from v2 to v3. It effectively showcases the new structure and options available in
TrustProxyConfig.Consider adding a brief note about the performance benefits of the new configuration, especially regarding the ability to trust loopback, private, or link-local IP addresses without manually adding these ranges to the whitelist. This would help users understand the advantages of migrating to the new system.
Line range hint
1-1: LGTM! Consider adding examples for the new CORS configuration.The changes to the CORS middleware, including the new
AllowPrivateNetworkfield and the updated fields that now accept slices, significantly improve flexibility and usability. This aligns well with the PR objectives.Consider adding a brief code example demonstrating how to use the new slice-based configuration for
AllowOrigins,AllowMethods,AllowHeaders, andExposeHeaders. This would help users quickly understand and adopt the new configuration style.🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
Line range hint
1-1: LGTM! Consider adding a note about theAllowPrivateNetworkoption.The CORS middleware migration guide clearly demonstrates how to update the configuration from v2 to v3, effectively showcasing the transition from comma-separated strings to slices for various options.
Consider adding a brief example or note about how to use the new
AllowPrivateNetworkoption in the migration guide. This would provide a complete picture of all the new features available in the updated CORS middleware.🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
Line range hint
1-1: LGTM! Consider adding a summary section and version compatibility note.The "What's New in v3" document provides a comprehensive overview of the changes in Fiber v3 and offers clear migration guides for various components. It effectively communicates the enhancements in usability, performance improvements, and provides clear migration paths.
Consider adding:
- A brief summary section at the beginning or end of the document highlighting the most significant changes and benefits of upgrading to v3.
- A note about version compatibility, specifying which versions of Go are supported by Fiber v3 and any potential breaking changes that users should be aware of when upgrading.
🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
.github/README.md (1)
564-567: Update to Trusted Proxy configuration looks good, but consider adding more context.The changes to the Trusted Proxy configuration align well with the PR objectives. The new
TrustProxyConfigstruct with theProxiesfield allows for more granular control over trusted IP addresses and ranges. This is a positive change that enhances security and flexibility.However, to improve clarity for users, consider adding a brief comment explaining the purpose of the
TrustProxyConfigandProxiesfields. Additionally, it might be helpful to mention that theProxyHeadersetting is used to specify which header to trust for the client's IP address.Here's a suggested modification to provide more context:
func main() { app := fiber.New(fiber.Config{ TrustProxy: true, TrustProxyConfig: fiber.TrustProxyConfig{ - Proxies: []string{"0.0.0.0", "1.1.1.1/30"}, // IP address or IP address range + // Specify trusted proxy IP addresses or IP ranges + Proxies: []string{"0.0.0.0", "1.1.1.1/30"}, }, + // Specify which header to trust for the client's IP address ProxyHeader: fiber.HeaderXForwardedFor, }) log.Fatal(app.Listen(":3000")) }docs/api/fiber.md (4)
45-83: LGTM! Config fields table updated correctly.The new entries for
TrustProxyandTrustProxyConfigare well-documented and align with the PR objectives. The descriptions provide clear information about their purpose and usage.Minor grammatical suggestion for the AppName field:
-| <Reference id="appname">AppName</Reference> | `string` | This allows to setup app name for the app | `""` | +| <Reference id="appname">AppName</Reference> | `string` | This allows setting up the app name for the app | `""` |🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position.
Context: ...tained by the top-level app and applied on prefix associated requests. ...(AI_HYDRA_LEO_REPLACE_ON_TO)
[uncategorized] ~68-~68: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...(BY_DEFAULT_COMMA)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~78-~78: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...(SO_THAT_UNNECESSARY_COMMA)
60-60: LGTM! TrustProxy and TrustProxyConfig entries are well-documented.The new entries for
TrustProxyandTrustProxyConfigprovide comprehensive explanations of their functionality and impact on Fiber context methods. This aligns well with the PR objectives.For improved readability, consider breaking the long line in the TrustProxy description:
-| <Reference id="trustproxy">TrustProxy</Reference> | `bool` | When set to true, fiber will check whether proxy is trusted, using TrustProxyConfig.Proxies list. <br /><br />By default `c.Protocol()` will get value from X-Forwarded-Proto, X-Forwarded-Protocol, X-Forwarded-Ssl or X-Url-Scheme header, `c.IP()` will get value from `ProxyHeader` header, `c.Hostname()` will get value from X-Forwarded-Host header. <br /> If `TrustProxy` is true, and `RemoteIP` is in the list of `TrustProxyConfig.Proxies` `c.Protocol()`, `c.IP()`, and `c.Hostname()` will have the same behaviour when `TrustProxy` disabled, if `RemoteIP` isn't in the list, `c.Protocol()` will return https in case when tls connection is handled by the app, or http otherwise, `c.IP()` will return RemoteIP() from fasthttp context, `c.Hostname()` will return `fasthttp.Request.URI().Host()` | `false` | +| <Reference id="trustproxy">TrustProxy</Reference> | `bool` | When set to true, fiber will check whether proxy is trusted, using TrustProxyConfig.Proxies list. <br /><br />By default `c.Protocol()` will get value from X-Forwarded-Proto, X-Forwarded-Protocol, X-Forwarded-Ssl or X-Url-Scheme header, `c.IP()` will get value from `ProxyHeader` header, `c.Hostname()` will get value from X-Forwarded-Host header. <br /> If `TrustProxy` is true, and `RemoteIP` is in the list of `TrustProxyConfig.Proxies` `c.Protocol()`, `c.IP()`, and `c.Hostname()` will have the same behaviour when `TrustProxy` disabled, if `RemoteIP` isn't in the list, `c.Protocol()` will return https in case when tls connection is handled by the app, or http otherwise, `c.IP()` will return RemoteIP() from fasthttp context, `c.Hostname()` will return `fasthttp.Request.URI().Host()` <br /><br />This setting is crucial for applications deployed behind reverse proxies to ensure correct handling of client information. | `false` |Also applies to: 77-77
Line range hint
85-170: LGTM! Server listening section is well-documented.The new Server listening section provides clear documentation for the updated listening configurations, including the new ListenConfig struct and updated Listen and Listener methods. This aligns well with the PR objectives by offering more flexibility in server setup.
Consider adding a brief explanation of the ListenConfig struct's purpose at the beginning of the section:
### Config +ListenConfig provides a way to customize the behavior of the server when it starts listening. It allows you to configure various aspects such as TLS, prefork mode, and startup messages. + You can pass an optional ListenConfig when calling the [`Listen`](#listen) or [`Listener`](#listener) method. ```go title="Example" // Custom config app.Listen(":8080", fiber.ListenConfig{ EnablePrefork: true, DisableStartupMessage: true, })<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space. Context: ... | This allows to setup app name for the app ... (NOUN_VERB_CONFUSION) --- [uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position. Context: ...tained by the top-level app and applied on prefix associated requests. ... (AI_HYDRA_LEO_REPLACE_ON_TO) --- [uncategorized] ~68-~68: Did you mean: “By default,”? Context: ...turn the value of the given header key. By default `c.IP()`will return the Remote IP from ... (BY_DEFAULT_COMMA) --- [uncategorized] ~76-~76: Possible missing article found. Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step... (AI_HYDRA_LEO_MISSING_A) --- [uncategorized] ~76-~76: Possible missing article found. Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ... (AI_HYDRA_LEO_MISSING_THE) --- [typographical] ~78-~78: The conjunction “so that” does not require a comma. Context: ... before setting the path for the context, so that the routing can also work with URL enco... (SO_THAT_UNNECESSARY_COMMA) </blockquote></details> </details> --- Line range hint `172-217`: **LGTM! Helper functions are well-documented.** The new Helper functions section accurately documents the NewError and IsChild functions, providing clear examples of their usage. This aligns well with the overall Fiber functionality and improves the documentation's completeness. Consider improving the IsChild function example for clarity: ```diff // Config app app := fiber.New() app.Get("/", func(c fiber.Ctx) error { if !fiber.IsChild() { - fmt.Println("I'm the parent process") + fmt.Println("I'm the parent process (main process)") } else { - fmt.Println("I'm a child process") + fmt.Println("I'm a child process (forked by prefork)") } return c.SendString("Hello, World!") }) // ... // With prefork enabled, the parent process will spawn child processes app.Listen(":8080", fiber.ListenConfig{EnablePrefork: true})This change provides more context about the parent and child processes in relation to the prefork feature.
🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position.
Context: ...tained by the top-level app and applied on prefix associated requests. ...(AI_HYDRA_LEO_REPLACE_ON_TO)
[uncategorized] ~68-~68: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...(BY_DEFAULT_COMMA)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~78-~78: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...(SO_THAT_UNNECESSARY_COMMA)
docs/api/ctx.md (4)
Line range hint
834-850: LGTM! Consider enhancing the documentation.The changes to
IsProxyTrustedalign well with the PR objectives. The new implementation usingTrustProxyandTrustProxyConfigprovides more flexibility in configuring trusted proxies.Consider adding a brief explanation of how
TrustProxyConfigworks in the method documentation, particularly mentioning the ability to trust loopback, private, or link-local IP addresses without manual configuration.
1646-1646: LGTM! Good security practice addition.The added note about using
Config.TrustProxyto prevent header spoofing is a valuable security recommendation, especially for applications behind a proxy.Consider adding a brief example of how to set
Config.TrustProxyin the application configuration to make it easier for developers to implement this security measure.
1646-1646: LGTM! Helpful tip for handling special characters in file paths.The added note about escaping URL-specific characters in file paths is a valuable addition that can help prevent errors when serving files with special characters in their names.
Consider adding a brief code example demonstrating how to use
url.PathEscape()withSendFile()to make the implementation clearer for developers.
1646-1646: LGTM! Useful addition for controlling file downloads.The new
Downloadoption in theSendFileconfiguration is a valuable addition that gives developers more control over how files are served to clients.Consider adding a brief example in the documentation showing how to use the
Downloadoption when callingSendFile(). This would help developers quickly understand how to implement this feature in their applications.ctx.go (1)
Line range hint
1822-1852: LGTM: Implemented TrustProxyConfig functionalityThe
IsProxyTrustedmethod has been successfully updated to use the newTrustProxyConfigstruct, allowing for more granular control over trusted IP addresses. This implementation aligns well with the PR objectives and improves the flexibility of the proxy trust system.Consider adding error handling for the case where
c.app.config.TrustProxyConfigis nil. This could prevent potential nil pointer dereferences:func (c *DefaultCtx) IsProxyTrusted() bool { if !c.app.config.TrustProxy { return true } ip := c.fasthttp.RemoteIP() + if c.app.config.TrustProxyConfig == nil { + return false + } + if (c.app.config.TrustProxyConfig.Private && ip.IsPrivate()) || (c.app.config.TrustProxyConfig.Loopback && ip.IsLoopback()) || (c.app.config.TrustProxyConfig.LinkLocal && ip.IsLinkLocalUnicast()) { return true } // ... rest of the function }This addition would ensure that the function handles cases where
TrustProxyConfigmight not be properly initialized.app.go (2)
395-396: Unexported fields within exported structIn the
TrustProxyConfigstruct, the fieldsipsandrangesare unexported, but the struct is exported. This could cause issues when trying to useTrustProxyConfigoutside the package. If these fields are intended for internal use only, consider making the struct unexported or adding comments to clarify.If the fields are meant to be unexported, no action is needed. Otherwise, consider exporting them:
type TrustProxyConfig struct { - ips map[string]struct{} + Ips map[string]struct{} ... - ranges []*net.IPNet + Ranges []*net.IPNetAlso applies to: 402-403
395-418: Initialize maps and slices in struct initializationIn the
TrustProxyConfigstruct, the unexported fieldsipsandrangesare initialized during proxy handling, but it might be clearer and safer to initialize them when the struct is created.Consider initializing the maps and slices when creating a new
TrustProxyConfiginstance:type TrustProxyConfig struct { - ips map[string]struct{} ... - ranges []*net.IPNet + ips map[string]struct{} // Initialized in NewTrustProxyConfig() + ranges []*net.IPNet // Initialized in NewTrustProxyConfig() ... } +// NewTrustProxyConfig creates a new instance of TrustProxyConfig with initialized fields. +func NewTrustProxyConfig() *TrustProxyConfig { + return &TrustProxyConfig{ + ips: make(map[string]struct{}), + ranges: make([]*net.IPNet, 0), + } +}ctx_test.go (3)
Line range hint
1575-1597: Consider refactoring repeated test configurations to use table-driven testsThe code in this segment sets up multiple test cases with different proxy configurations. There is duplication in the way
NewandConfigare initialized with differentTrustProxyConfigsettings. To improve maintainability and readability, consider refactoring these tests into a table-driven format. This will reduce code duplication and make it easier to add or modify test cases in the future.
Line range hint
2668-2752: Consolidate trusted proxy tests to improve maintainabilityThe test functions
Test_Ctx_Scheme_TrustedProxy,Test_Ctx_Scheme_TrustedProxyRange, andTest_Ctx_Scheme_UntrustedProxyRangecontain similar logic with slight variations in proxy configurations. Refactoring these tests into a single table-driven test function can reduce redundancy and make it easier to see the differences between each test case.
Line range hint
6237-6485: Improve benchmarks forIsProxyTrustedwith varying configurationsThe benchmark tests for
IsProxyTrustedcover multiple scenarios with differentTrustProxyConfigsettings. To enhance clarity and maintainability, consider organizing these benchmarks using sub-benchmarks with descriptive names that indicate the configuration being tested. This will make it easier to identify performance implications of different configurations and ensure that all relevant cases are adequately benchmarked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- .github/README.md (1 hunks)
- app.go (5 hunks)
- ctx.go (5 hunks)
- ctx_interface_gen.go (3 hunks)
- ctx_test.go (27 hunks)
- docs/api/ctx.md (3 hunks)
- docs/api/fiber.md (1 hunks)
- docs/middleware/earlydata.md (1 hunks)
- docs/whats_new.md (2 hunks)
- middleware/earlydata/earlydata_test.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/api/fiber.md
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position.
Context: ...tained by the top-level app and applied on prefix associated requests. ...(AI_HYDRA_LEO_REPLACE_ON_TO)
[uncategorized] ~68-~68: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...(BY_DEFAULT_COMMA)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~78-~78: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...(SO_THAT_UNNECESSARY_COMMA)
docs/middleware/earlydata.md
[style] ~10-~10: Consider a shorter alternative to avoid wordiness.
Context: ...fig option before using this middleware in order to not trust bogus HTTP request headers of...(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (13)
docs/middleware/earlydata.md (1)
Line range hint
1-93: Documentation update looks good and remains comprehensive.The update to the EarlyData middleware documentation accurately reflects the configuration change from
EnableTrustedProxyChecktoTrustProxy. The rest of the documentation remains comprehensive and well-structured, providing:
- Clear explanations of the EarlyData middleware's purpose and potential security implications.
- Proper examples of how to use the middleware.
- Detailed configuration options and their descriptions.
- Default configuration and relevant constants.
This documentation update aligns well with the PR objectives of improving clarity and usability for developers working with trusted proxies in the Fiber framework.
🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Consider a shorter alternative to avoid wordiness.
Context: ...fig option before using this middleware in order to not trust bogus HTTP request headers of...(IN_ORDER_TO_PREMIUM)
middleware/earlydata/earlydata_test.go (2)
Line range hint
182-192: LGTM! Consider using a more specific IP for testing.The update to use
TrustProxyandTrustProxyConfig.Proxiesis correctly implemented. The test case name has been appropriately updated to reflect the new configuration structure.However, consider using a more specific IP address for testing instead of "0.0.0.0", which represents all IPv4 addresses. A loopback address like "127.0.0.1" or a private network address might be more appropriate for this test case.
Here's a suggested change:
TrustProxyConfig: fiber.TrustProxyConfig{ Proxies: []string{ - "0.0.0.0", + "127.0.0.1", }, },Let's verify the consistency of this change across the codebase:
#!/bin/bash # Description: Check for the new TrustProxyConfig structure usage # Test: Search for TrustProxyConfig. Expect: Multiple results including this file. rg --type go 'TrustProxyConfig' # Test: Search for TrustProxyConfig.Proxies. Expect: Results including this file. rg --type go 'TrustProxyConfig\.Proxies'
176-180: LGTM! Verify consistency across the codebase.The renaming of
EnableTrustedProxyChecktoTrustProxyis correctly implemented in this test case. The test case name has been appropriately updated to reflect this change.To ensure consistency, let's verify that this change has been applied throughout the codebase:
✅ Verification successful
✅ Consistent Renaming Verified Across Codebase
The renaming of
EnableTrustedProxyChecktoTrustProxyhas been successfully applied throughout the codebase with no remaining instances of the old identifier found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of EnableTrustedProxyCheck # Test: Search for EnableTrustedProxyCheck. Expect: No results. rg --type go 'EnableTrustedProxyCheck' # Test: Search for TrustProxy. Expect: Multiple results including this file. rg --type go 'TrustProxy'Length of output: 5297
ctx_interface_gen.go (1)
Line range hint
1-332: Overall, the changes in this file are consistent and align with the PR objectives.The updates to the comments in the
Ctxinterface correctly reflect the renaming ofEnableTrustedProxyChecktoTrustProxy. This change enhances consistency across the framework and improves clarity for developers using these methods.A few minor suggestions have been made to further improve the clarity and context of the comments. These suggestions aim to provide more detailed information about the
TrustProxyconfiguration and its implications for each method.No major issues or inconsistencies were found in the changes. The updates successfully maintain the integrity of the interface documentation while reflecting the new configuration option.
docs/api/fiber.md (1)
Line range hint
1-217: Overall, excellent documentation updates!The changes to the
docs/api/fiber.mdfile effectively communicate the new features and modifications introduced in this PR, particularly the TrustProxy and TrustProxyConfig additions. The documentation is clear, well-structured, and provides helpful examples for users.A few minor suggestions have been made to further improve readability and clarity, but these are not critical issues. Great job on maintaining high-quality documentation alongside the code changes!
🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position.
Context: ...tained by the top-level app and applied on prefix associated requests. ...(AI_HYDRA_LEO_REPLACE_ON_TO)
[uncategorized] ~68-~68: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...(BY_DEFAULT_COMMA)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~78-~78: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...(SO_THAT_UNNECESSARY_COMMA)
ctx.go (4)
687-687: LGTM: Updated comment for clarityThe comment update for the
Hostmethod correctly reflects the change fromEnableTrustedProxyChecktoTrustProxy. This change improves clarity and aligns with the new configuration option.
705-705: LGTM: Updated comment for consistencyThe comment update for the
Hostnamemethod is consistent with the change made to theHostmethod, reflecting the use ofConfig.TrustProxy. This maintains consistency across the codebase.
723-723: LGTM: Updated comment for consistencyThe comment update for the
IPmethod maintains consistency with the changes made to theHostandHostnamemethods, correctly referencingConfig.TrustProxy.
Line range hint
1-1952: Overall assessment: Changes align with PR objectivesThe modifications to
ctx.gosuccessfully implement the newTrustProxyConfigfunctionality and update relevant comments for consistency. These changes align well with the PR objectives of renamingEnableTrustedProxyChecktoTrustProxyand introducing more granular control over trusted proxy configurations.Key improvements:
- Updated comments for
Host,Hostname, andIPmethods to referenceConfig.TrustProxy.- Implemented new
IsProxyTrustedmethod usingTrustProxyConfigfor more flexible proxy trust settings.The changes enhance the framework's ability to handle proxy configurations, particularly in environments with reverse proxies. The added flexibility in defining trusted IP ranges and types (private, loopback, link-local) should improve usability for developers working with various network setups.
app.go (2)
404-418: Provide default values in documentation commentsFor the fields
LinkLocal,Loopback, andPrivate, it's good that you included the default value in the comments. Ensure that this practice is consistent throughout the codebase.
Line range hint
312-314: Verify the version number before releaseThe
Versionconstant is set to"3.0.0-beta.3". Ensure that this version number is updated to the correct release version before finalizing the release.Please confirm the version number is accurate for the release.
ctx_test.go (2)
1790-1793: Approve changes inBenchmark_Ctx_HostnameThe modifications in this benchmark function correctly reflect the changes in the proxy configuration settings. The
Newfunction is updated with the appropriateTrustProxyandTrustProxyConfigfields.
Line range hint
1974-1989: Ensure consistency in proxy header usageIn
Test_Ctx_IP_UntrustedProxyandTest_Ctx_IP_TrustedProxy, theProxyHeaderis set toHeaderXForwardedFor. Ensure that the proxy headers used in your tests are consistent with those expected by your application and that all test cases cover the necessary scenarios for proxy header handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3170 +/- ##
==========================================
+ Coverage 82.38% 82.44% +0.05%
==========================================
Files 113 113
Lines 8466 8474 +8
==========================================
+ Hits 6975 6986 +11
+ Misses 1088 1085 -3
Partials 403 403
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ctx.go (1)
Line range hint
1841-1848: LGTM! Enhanced flexibility in proxy trust configuration.The additional checks for specific trusted IPs and IP ranges provide great flexibility in configuring trusted proxies. This aligns well with the PR objectives and offers users fine-grained control over proxy trust.
Consider the performance implications of iterating through IP ranges for every request. If the list of ranges is expected to be large, you might want to consider using a more efficient data structure or algorithm for IP range checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app.go (5 hunks)
- ctx.go (5 hunks)
- ctx_test.go (27 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_test.go
🧰 Additional context used
🔇 Additional comments (7)
ctx.go (3)
705-705: LGTM! Consistent documentation update.The comment for the
Hostnamemethod has been updated to useConfig.TrustProxy, maintaining consistency with the previous change in theHostmethod.
723-723: LGTM! Documentation consistency maintained.The comment for the
IPmethod has been updated to useConfig.TrustProxy, consistent with the previous changes in theHostandHostnamemethods.
1822-1840: LGTM! Significant improvement to proxy trust checking.The
IsProxyTrustedmethod has been enhanced with more granular checks for different types of IP addresses (loopback, private, link-local). This aligns well with the PR objectives of improving trusted proxy configuration.The use of
TrustProxyConfigstruct provides a more structured and flexible approach to configuration.To ensure the changes are properly covered by tests:
#!/bin/bash # Check for test coverage of the IsProxyTrusted method rg --type go -C 5 'func TestIsProxyTrusted'app.go (4)
336-336: Improve clarity in documentation commentConsider rephrasing the comment on line 336 for better readability:
- Current: "If request ip in TrustProxyConfig.Proxies whitelist, then:"
- Suggestion: "If the request IP is in the
TrustProxyConfig.Proxieswhitelist, then:"This enhances clarity and aligns with standard grammatical conventions.
340-340: Improve clarity in documentation commentSimilarly, on line 340, you might want to rephrase:
- Current: "But if request ip NOT in TrustProxyConfig.Proxies whitelist, then:"
- Suggestion: "But if the request IP is not in the
TrustProxyConfig.Proxieswhitelist, then:"This improves the documentation's readability.
545-546: Potential data race in concurrent environmentsAccessing
TrustProxyConfig.ipsandTrustProxyConfig.rangeswithout synchronization could lead to data races ifhandleTrustedProxyis called concurrently.Ensure that access to
TrustProxyConfig.ipsandTrustProxyConfig.rangesis thread-safe. Consider using mutexes or other synchronization mechanisms to protect these shared resources.
564-574: Handle potential errors when adding IP addressesIn the
handleTrustedProxyfunction, when an IP address fails to parse in theelseblock, there's no error handling. Adding error handling for invalid IP addresses enhances robustness.Here's a suggested modification:
func (app *App) handleTrustedProxy(ipAddress string) { if strings.Contains(ipAddress, "/") { _, ipNet, err := net.ParseCIDR(ipAddress) if err != nil { log.Warnf("IP range %q could not be parsed: %v", ipAddress, err) } else { app.config.TrustProxyConfig.ranges = append(app.config.TrustProxyConfig.ranges, ipNet) } } else { + ip := net.ParseIP(ipAddress) + if ip == nil { + log.Warnf("IP address %q could not be parsed", ipAddress) + } else { app.config.TrustProxyConfig.ips[ipAddress] = struct{}{} + } } }
gaby
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
docs/middleware/earlydata.md (1)
10-10: Consider rewording for conciseness.While the current wording is clear, it could be slightly more concise. Consider the following alternative:
-Make sure to enable fiber's `TrustProxy` config option before using this middleware in order to not trust bogus HTTP request headers of the client. +Enable fiber's `TrustProxy` config option before using this middleware to prevent trusting bogus HTTP request headers from clients.This revision maintains the essential information while addressing the wordiness concern raised by the static analysis tool.
🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Consider a shorter alternative to avoid wordiness.
Context: ...fig option before using this middleware in order to not trust bogus HTTP request headers of...(IN_ORDER_TO_PREMIUM)
ctx_interface_gen.go (1)
318-318: LGTM with a minor suggestion: Updated comment forIsProxyTrusted()methodThe comment has been correctly updated to use the new
Config.TrustProxyoption, which aligns with the PR objectives. The change preserves the original meaning of the comment.For consistency with the other updated comments, consider adding "Please use" at the beginning of the comment:
-// If Config.TrustProxy false, it returns true +// Please use Config.TrustProxy to prevent header spoofing. If Config.TrustProxy is false, it returns true.This suggestion maintains consistency across all updated comments and provides a clearer instruction to users.
.github/README.md (1)
Line range hint
1-568: Consider updating related sections for consistency.While the changes to the "Using Trusted Proxy" section are good, consider reviewing and updating other relevant sections of the README for consistency. This might include:
- The "Features" list, if it mentions proxy handling.
- Any examples or quickstart guides that involve proxy configuration.
- The changelog section, if present, to highlight this new feature.
Ensuring consistency throughout the documentation will help users understand and adopt the new proxy configuration options more easily.
docs/api/fiber.md (5)
60-60: Excellent addition of TrustProxy and TrustProxyConfig optionsThe new TrustProxy and TrustProxyConfig options are valuable additions that align well with the PR objectives. They provide enhanced control over proxy trust settings, improving security and flexibility.
However, I have a few suggestions to improve clarity:
- For TrustProxy, consider breaking down the long description into bullet points for better readability.
- For TrustProxyConfig, it might be helpful to provide a brief example of how to use it in conjunction with TrustProxy.
Consider updating the descriptions as follows:
- | <Reference id="trustproxy">TrustProxy</Reference> | `bool` | When set to true, fiber will check whether proxy is trusted, using TrustProxyConfig.Proxies list. <br /><br />By default `c.Protocol()` will get value from X-Forwarded-Proto, X-Forwarded-Protocol, X-Forwarded-Ssl or X-Url-Scheme header, `c.IP()` will get value from `ProxyHeader` header, `c.Hostname()` will get value from X-Forwarded-Host header. <br /> If `TrustProxy` is true, and `RemoteIP` is in the list of `TrustProxyConfig.Proxies` `c.Protocol()`, `c.IP()`, and `c.Hostname()` will have the same behaviour when `TrustProxy` disabled, if `RemoteIP` isn't in the list, `c.Protocol()` will return https in case when tls connection is handled by the app, or http otherwise, `c.IP()` will return RemoteIP() from fasthttp context, `c.Hostname()` will return `fasthttp.Request.URI().Host()` | `false` | + | <Reference id="trustproxy">TrustProxy</Reference> | `bool` | When set to true, Fiber will check whether a proxy is trusted, using the TrustProxyConfig.Proxies list. <br /><br />Effects when TrustProxy is true:<br />• `c.Protocol()` will get value from X-Forwarded-Proto, X-Forwarded-Protocol, X-Forwarded-Ssl, or X-Url-Scheme header<br />• `c.IP()` will get value from `ProxyHeader` header<br />• `c.Hostname()` will get value from X-Forwarded-Host header<br /><br />If RemoteIP is in TrustProxyConfig.Proxies:<br />• Behavior remains the same as when TrustProxy is disabled<br /><br />If RemoteIP is not in the list:<br />• `c.Protocol()` returns 'https' for TLS connections, 'http' otherwise<br />• `c.IP()` returns RemoteIP() from fasthttp context<br />• `c.Hostname()` returns `fasthttp.Request.URI().Host()` | `false` | - | <Reference id="trustproxyconfig">TrustProxyConfig</Reference> | `TrustProxyConfig` | Configure trusted proxy IP's. Look at `TrustProxy` doc. <br /> <br /> `TrustProxyConfig.Proxies` can take IP or IP range addresses. | `nil` | + | <Reference id="trustproxyconfig">TrustProxyConfig</Reference> | `TrustProxyConfig` | Configure trusted proxy IPs. See `TrustProxy` doc for more details. <br /><br />`TrustProxyConfig.Proxies` can take IP or IP range addresses.<br /><br />Example:<br />`app := fiber.New(fiber.Config{`<br />` TrustProxy: true,`<br />` TrustProxyConfig: fiber.TrustProxyConfig{`<br />` Proxies: []string{"192.168.0.0/24", "10.0.0.0/8"},`<br />` },`<br />`})` | `nil` |Also applies to: 77-77
Line range hint
84-149: Excellent additions to the server listening documentationThe new ListenConfig struct and updated Listen method signature provide great flexibility for server configuration. The examples are clear and cover important use cases like TLS and prefork.
Consider adding a brief comment to each TLS example to explain the specific use case. For instance:
#### TLS TLS serves HTTPs requests from the given address using certFile and keyFile paths to as TLS certificate and key file. ```go title="Examples" +// Basic TLS configuration app.Listen(":443", fiber.ListenConfig{CertFile: "./cert.pem", CertKeyFile: "./cert.key"})TLS with certificate
+// TLS configuration with client certificate for mTLS app.Listen(":443", fiber.ListenConfig{CertClientFile: "./ca-chain-cert.pem"})TLS with certFile, keyFile and clientCertFile
+// Complete TLS configuration with server and client certificates app.Listen(":443", fiber.ListenConfig{CertFile: "./cert.pem", CertKeyFile: "./cert.key", CertClientFile: "./ca-chain-cert.pem"})This will help users quickly understand the purpose of each example. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space. Context: ... | This allows to setup app name for the app ... (NOUN_VERB_CONFUSION) --- [uncategorized] ~58-~58: Possible missing comma found. Context: ....IP()` will return only the first valid IP rather than just the raw header value t... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position. Context: ...tained by the top-level app and applied on prefix associated requests. ... (AI_HYDRA_LEO_REPLACE_ON_TO) --- [uncategorized] ~68-~68: Did you mean: “By default,”? Context: ...turn the value of the given header key. By default `c.IP()`will return the Remote IP from ... (BY_DEFAULT_COMMA) --- [uncategorized] ~76-~76: Possible missing article found. Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step... (AI_HYDRA_LEO_MISSING_A) --- [uncategorized] ~76-~76: Possible missing article found. Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ... (AI_HYDRA_LEO_MISSING_THE) --- [typographical] ~78-~78: The conjunction “so that” does not require a comma. Context: ... before setting the path for the context, so that the routing can also work with URL enco... (SO_THAT_UNNECESSARY_COMMA) </blockquote></details> </details> --- Line range hint `188-200`: **Clear documentation of server shutdown methods** The documentation for the shutdown methods (Shutdown, ShutdownWithTimeout, and ShutdownWithContext) is concise and clear. It provides users with options for graceful shutdown in different scenarios. To further improve the documentation, consider adding a small example for each method. This will help users understand how to implement these methods in their applications. For instance: ```diff func (app *App) Shutdown() error func (app *App) ShutdownWithTimeout(timeout time.Duration) error func (app *App) ShutdownWithContext(ctx context.Context) error + +```go title="Examples" +// Basic shutdown +if err := app.Shutdown(); err != nil { + log.Fatal(err) +} + +// Shutdown with timeout +if err := app.ShutdownWithTimeout(30 * time.Second); err != nil { + log.Fatal(err) +} + +// Shutdown with context +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel() +if err := app.ShutdownWithContext(ctx); err != nil { + log.Fatal(err) +} +```These examples will provide a quick reference for users implementing graceful shutdown in their Fiber applications.
🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ....IP()` will return only the first valid IP rather than just the raw header value t...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position.
Context: ...tained by the top-level app and applied on prefix associated requests. ...(AI_HYDRA_LEO_REPLACE_ON_TO)
[uncategorized] ~68-~68: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...(BY_DEFAULT_COMMA)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~78-~78: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...(SO_THAT_UNNECESSARY_COMMA)
Line range hint
202-241: Well-documented helper functionsThe documentation for the new helper functions (NewError and IsChild) is clear and includes useful examples. The IsChild function is particularly relevant for the prefork feature mentioned earlier in the document.
To improve the IsChild example, consider adding a comment explaining the significance of the prefork feature and how it relates to the IsChild function. For instance:
```go title="Example" // Config app app := fiber.New() app.Get("/", func(c fiber.Ctx) error { if !fiber.IsChild() { fmt.Println("I'm the parent process") } else { fmt.Println("I'm a child process") } return c.SendString("Hello, World!") }) // ... +// EnablePrefork allows the server to utilize all CPU cores efficiently +// by spawning multiple child processes. IsChild() helps distinguish +// between the parent and child processes. // With prefork enabled, the parent process will spawn child processes app.Listen(":8080", fiber.ListenConfig{EnablePrefork: true})This addition will help users understand the relationship between the prefork feature and the IsChild function, providing context for when and why they might use it. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space. Context: ... | This allows to setup app name for the app ... (NOUN_VERB_CONFUSION) --- [uncategorized] ~58-~58: Possible missing comma found. Context: ....IP()` will return only the first valid IP rather than just the raw header value t... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position. Context: ...tained by the top-level app and applied on prefix associated requests. ... (AI_HYDRA_LEO_REPLACE_ON_TO) --- [uncategorized] ~68-~68: Did you mean: “By default,”? Context: ...turn the value of the given header key. By default `c.IP()`will return the Remote IP from ... (BY_DEFAULT_COMMA) --- [uncategorized] ~76-~76: Possible missing article found. Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step... (AI_HYDRA_LEO_MISSING_A) --- [uncategorized] ~76-~76: Possible missing article found. Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ... (AI_HYDRA_LEO_MISSING_THE) --- [typographical] ~78-~78: The conjunction “so that” does not require a comma. Context: ... before setting the path for the context, so that the routing can also work with URL enco... (SO_THAT_UNNECESSARY_COMMA) </blockquote></details> </details> --- `47-47`: **Minor grammatical and typographical improvements** There are a few minor issues in the documentation that could be addressed to improve clarity and correctness: 1. Line 47: "This allows to setup app name for the app" should be "This allows setting up the app name for the app". 2. Line 58: Consider adding a comma after "rather than" for better readability. 3. Line 61: "applied on prefix associated requests" could be changed to "applied to prefix-associated requests" for clarity. 4. Line 68: Add a comma after "By default" for consistency. 5. Line 76: Consider adding articles: "Fiber doesn't have a default validator, so it'll skip the validator step". 6. Line 78: Remove the comma before "so that" as it's unnecessary in this context. Here's a diff with the suggested changes: ```diff -| <Reference id="appname">AppName</Reference> | `string` | This allows to setup app name for the app | `""` | +| <Reference id="appname">AppName</Reference> | `string` | This allows setting up the app name for the app | `""` | -| <Reference id="enableipvalidation">EnableIPValidation</Reference> | `bool` | If set to true, `c.IP()` and `c.IPs()` will validate IP addresses before returning them. Also, `c.IP()` will return only the first valid IP rather than just the raw header value that may be a comma separated string.<br /><br />**WARNING:** There is a small performance cost to doing this validation. Keep disabled if speed is your only concern and your application is behind a trusted proxy that already validates this header. | `false` | +| <Reference id="enableipvalidation">EnableIPValidation</Reference> | `bool` | If set to true, `c.IP()` and `c.IPs()` will validate IP addresses before returning them. Also, `c.IP()` will return only the first valid IP rather than, just the raw header value that may be a comma separated string.<br /><br />**WARNING:** There is a small performance cost to doing this validation. Keep disabled if speed is your only concern and your application is behind a trusted proxy that already validates this header. | `false` | -| <Reference id="errorhandler">ErrorHandler</Reference> | `ErrorHandler` | ErrorHandler is executed when an error is returned from fiber.Handler. Mounted fiber error handlers are retained by the top-level app and applied on prefix associated requests. | `DefaultErrorHandler` | +| <Reference id="errorhandler">ErrorHandler</Reference> | `ErrorHandler` | ErrorHandler is executed when an error is returned from fiber.Handler. Mounted fiber error handlers are retained by the top-level app and applied to prefix-associated requests. | `DefaultErrorHandler` | -| <Reference id="proxyheader">ProxyHeader</Reference> | `string` | This will enable `c.IP()` to return the value of the given header key. By default `c.IP()`will return the Remote IP from the TCP connection, this property can be useful if you are behind a load balancer e.g. _X-Forwarded-\*_. | `""` | +| <Reference id="proxyheader">ProxyHeader</Reference> | `string` | This will enable `c.IP()` to return the value of the given header key. By default, `c.IP()`will return the Remote IP from the TCP connection, this property can be useful if you are behind a load balancer e.g. _X-Forwarded-\*_. | `""` | -| <Reference id="structvalidator">StructValidator</Reference> | `StructValidator` | If you want to validate header/form/query... automatically when to bind, you can define struct validator. Fiber doesn't have default validator, so it'll skip validator step if you don't use any validator. | `nil` | +| <Reference id="structvalidator">StructValidator</Reference> | `StructValidator` | If you want to validate header/form/query... automatically when to bind, you can define struct validator. Fiber doesn't have a default validator, so it'll skip the validator step if you don't use any validator. | `nil` | -| <Reference id="unescapepath">UnescapePath</Reference> | `bool` | Converts all encoded characters in the route back before setting the path for the context, so that the routing can also work with URL encoded special characters | `false` | +| <Reference id="unescapepath">UnescapePath</Reference> | `bool` | Converts all encoded characters in the route back before setting the path for the context so that the routing can also work with URL encoded special characters | `false` |These changes will improve the overall quality and consistency of the documentation.
Also applies to: 58-58, 61-61, 68-68, 76-76, 78-78
🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
docs/api/ctx.md (2)
Line range hint
834-850: LGTM! Documentation accurately reflects the updated proxy trust configuration.The changes to the
IsProxyTrustedmethod documentation clearly explain the newTrustProxyandTrustProxyConfigoptions. This update provides developers with a better understanding of how proxy trust is determined in Fiber.Consider adding a brief explanation of what "proxy ranges" and "ip map" mean in the context of proxy trust configuration. This would further clarify the concept for developers who might be new to working with proxies.
1646-1648: Excellent addition to prevent security vulnerabilities!The new note about using
Config.TrustProxyto prevent header spoofing is a crucial security recommendation, especially for applications running behind a proxy. This addition helps developers understand the importance of proper configuration to maintain security.Consider adding a link to the
Config.TrustProxydocumentation or a brief example of how to set this configuration. This would make it easier for developers to implement this security measure correctly.app.go (1)
354-356: Enhance documentation forTrustProxyConfigThe comment "// Read TrustProxy doc." may not provide sufficient guidance. Consider adding more detailed documentation to help users understand how to configure
TrustProxyConfig.For example:
-// Read TrustProxy doc. +// TrustProxyConfig provides configuration options for trusted proxies. // Refer to the documentation for detailed usage instructions.ctx_test.go (1)
Line range hint
2665-2675: Trusted proxy headers should be considered when 'TrustProxy' is enabled.In
Test_Ctx_Scheme_TrustedProxy, even thoughTrustProxyis enabled, the scheme defaults tohttpdespite proxy headers being set:c.Request().Header.Set(HeaderXForwardedProto, schemeHTTPS) require.Equal(t, schemeHTTPS, c.Scheme()) c.Request().Header.Reset() // Repeats for other headers... require.Equal(t, schemeHTTP, c.Scheme())Ensure that when
TrustProxyis enabled and the request comes from a trusted proxy, the scheme reflects the proxy headers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- .github/README.md (1 hunks)
- app.go (5 hunks)
- ctx.go (5 hunks)
- ctx_interface_gen.go (3 hunks)
- ctx_test.go (27 hunks)
- docs/api/ctx.md (3 hunks)
- docs/api/fiber.md (1 hunks)
- docs/middleware/earlydata.md (1 hunks)
- docs/whats_new.md (2 hunks)
- middleware/earlydata/earlydata_test.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/api/fiber.md
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...(NOUN_VERB_CONFUSION)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ....IP()` will return only the first valid IP rather than just the raw header value t...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position.
Context: ...tained by the top-level app and applied on prefix associated requests. ...(AI_HYDRA_LEO_REPLACE_ON_TO)
[uncategorized] ~68-~68: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...(BY_DEFAULT_COMMA)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~76-~76: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...(AI_HYDRA_LEO_MISSING_THE)
[typographical] ~78-~78: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...(SO_THAT_UNNECESSARY_COMMA)
docs/middleware/earlydata.md
[style] ~10-~10: Consider a shorter alternative to avoid wordiness.
Context: ...fig option before using this middleware in order to not trust bogus HTTP request headers of...(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (23)
docs/middleware/earlydata.md (1)
10-10: LGTM: Configuration option name updated correctly.The change from
EnableTrustedProxyChecktoTrustProxyaligns with the PR objectives and improves consistency with the new naming convention. This update is crucial for users to understand the new configuration.🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Consider a shorter alternative to avoid wordiness.
Context: ...fig option before using this middleware in order to not trust bogus HTTP request headers of...(IN_ORDER_TO_PREMIUM)
middleware/earlydata/earlydata_test.go (3)
Line range hint
1-192: Overall, the changes effectively implement the new trusted proxy configuration.The updates to this test file successfully reflect the changes in the trusted proxy configuration structure. The renaming of
EnableTrustedProxyChecktoTrustProxyand the introduction of theTrustProxyConfigstruct align well with the PR objectives of simplifying and enhancing the trusted proxy configuration process.These changes contribute to:
- Improved clarity in configuration naming.
- More granular control over proxy settings.
- Better alignment with other frameworks like Express and Echo.
The test cases have been appropriately updated, ensuring that the new configuration structure is properly validated. This will help maintain the reliability and correctness of the
earlydatamiddleware as the Fiber framework evolves.
Line range hint
182-192: LGTM! Consider a minor improvement and verify consistency.The restructuring of the configuration for trusted proxies is well implemented. The introduction of
TrustProxyConfigprovides more granular control over proxy settings, which is a good improvement.Consider using a more descriptive IP address in the test case:
TrustProxyConfig: fiber.TrustProxyConfig{ Proxies: []string{ - "0.0.0.0", + "127.0.0.1", // or "192.168.1.1" for a typical private IP }, },This change would make the test case more realistic and easier to understand at a glance.
To ensure consistency across the codebase, please run the following script:
176-181: LGTM! Verify consistency across the codebase.The renaming of the configuration field from
EnableTrustedProxyChecktoTrustProxyis correctly reflected in this test case. The change is consistent with the summary provided and improves the clarity of the configuration.To ensure consistency across the codebase, please run the following script:
✅ Verification successful
Verification Successful: All instances of
EnableTrustedProxyCheckhave been successfully renamed toTrustProxy. No remaining references found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all occurrences of EnableTrustedProxyCheck have been updated to TrustProxy # Test: Search for any remaining instances of EnableTrustedProxyCheck # Expect: No results, indicating all instances have been updated rg --type go 'EnableTrustedProxyCheck' # Test: Confirm the presence of the new TrustProxy field # Expect: Results showing the usage of TrustProxy in configuration rg --type go 'TrustProxy.*bool'Length of output: 4834
Script:
#!/bin/bash # Description: Verify that all occurrences of EnableTrustedProxyCheck have been updated to TrustProxy # Test: Search for any remaining instances of EnableTrustedProxyCheck # Expect: No results, indicating all instances have been updated rg --type go 'EnableTrustedProxyCheck' # Test: Confirm the presence of the new TrustProxy field # Expect: Results showing the usage of TrustProxy in configuration rg --type go 'TrustProxy\s*bool'Length of output: 118
ctx_interface_gen.go (4)
130-130: LGTM: Updated comment forHost()methodThe comment has been correctly updated to reflect the new configuration option name
Config.TrustProxy. This change is consistent with the PR objectives and maintains the original meaning of the comment.
136-136: LGTM: Updated comment forHostname()methodThe comment has been appropriately updated to use the new configuration option name
Config.TrustProxy. This change aligns with the PR objectives and preserves the original intent of the comment.
142-142: LGTM: Updated comment forIP()methodThe comment for the
IP()method has been correctly updated to reference the newConfig.TrustProxyoption. This change is in line with the PR objectives and maintains the original purpose of the comment.
Line range hint
1-353: Summary: Successfully updated comments for proxy-related methodsThe changes in this file consistently update the comments for the
Host(),Hostname(),IP(), andIsProxyTrusted()methods to reflect the renaming of the configuration option fromEnableTrustedProxyChecktoTrustProxy. These updates align with the PR objectives and improve the documentation without altering the functionality of theCtxinterface.The changes are approved, with a minor suggestion for the
IsProxyTrusted()method comment to maintain consistency with the other updated comments.docs/whats_new.md (6)
Line range hint
1-57: LGTM: Clear announcement about Go version supportThe documentation clearly states that Fiber v3 drops support for Go versions below 1.22 and recommends upgrading to Go 1.22 or higher. This is important information for users planning to upgrade to Fiber v3.
🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
Line range hint
60-190: Significant updates to Router interface and functionalityThe Router section outlines several important changes:
- HTTP method registration signatures now require a mandatory handler parameter, with middleware specified separately.
- The
Routemethod has been updated to support chaining, similar to Express.js.- Middleware registration now aligns with Express.js conventions, supporting multiple prefixes and subapp registration through the
Usemethod.These changes enhance the routing capabilities and bring Fiber's API closer to Express.js, which may ease the transition for developers familiar with Express.js. However, it's important to note that these changes may require significant updates to existing Fiber applications.
To verify these changes, we can check the router implementation:
#!/bin/bash # Search for route method implementations rg -n -A 10 'func \(app \*App\) (Get|Post|Put|Delete|Patch|Head|Options|Trace|All)' --type go # Search for the Route method implementation rg -n -A 10 'func \(app \*App\) Route' --type go # Search for the Use method implementation rg -n -A 10 'func \(app \*App\) Use' --type go🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
Line range hint
241-244: Major overhaul of the Client packageThe Client package has undergone a complete rebuild, introducing several new features:
- Cookiejar support
- Request/response hooks
- Other unspecified improvements
This represents a significant enhancement to Fiber's HTTP client capabilities. Users of the client package should review the new documentation carefully to understand and leverage these new features.
To verify these changes, we can check the client package implementation:
#!/bin/bash # Search for Cookiejar implementation rg -n -A 10 'type Cookiejar' --type go # Search for hook implementations rg -n -A 10 'type (Request|Response)Hook' --type go🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
Line range hint
245-390: Significant updates to middleware implementationsThe Middlewares section outlines several important changes:
- Adaptor: Significantly optimized for performance and efficiency.
- Cache: New Cache Invalidator feature added.
- CORS: Updated to use slices instead of strings for configuration, and added support for Private Network Access.
- Compression: Added support for zstd compression.
- EncryptCookie: Added support for specifying key length.
- Healthcheck: Enhanced to support more than two routes with default endpoints.
- Filesystem: Removed and functionality merged into static middleware.
- Monitor: Migrated to the Contrib package.
These changes represent significant updates to how middlewares are configured and used in Fiber v3. Developers will need to update their middleware configurations, especially for CORS and Healthcheck. The removal of the Filesystem middleware and the migration of the Monitor middleware to the Contrib package are breaking changes that require attention during migration.
To verify these changes, we can check the implementations of the updated middlewares:
#!/bin/bash # Search for CORS configuration rg -n -A 10 'type Config struct' --type go -- '**/cors' # Search for Healthcheck configuration rg -n -A 10 'type Config struct' --type go -- '**/healthcheck' # Verify removal of Filesystem middleware rg -n 'package filesystem' --type go # Verify Monitor middleware in Contrib package gh repo view gofiber/contrib
58-59: Important change in TrustProxy configurationThe documentation indicates a significant change in how TrustProxy is configured:
EnabledTrustedProxyCheckhas been moved toapp.Config.TrustProxyTrustedProxieshas been moved toTrustProxyConfig.ProxiesThis change affects how users configure trusted proxies in their Fiber applications. It's crucial to ensure that the migration guide provides clear instructions on how to update existing configurations to match this new structure.
To verify the implementation of these changes, we can check the relevant configuration files:
✅ Verification successful
TrustProxy configuration changes verified successfully
The changes mentioned in the review comment are implemented correctly:
EnabledTrustedProxyCheckhas been moved toapp.Config.TrustProxy.TrustedProxieshas been moved toTrustProxyConfig.Proxies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TrustProxy and TrustProxyConfig in the codebase rg -n -A 5 'TrustProxy|TrustProxyConfig' --type goLength of output: 7435
Script:
#!/bin/bash # Search for 'app.Config.TrustProxy' and 'TrustProxyConfig.Proxies' in Go files rg -n -A 5 'app\.Config\.TrustProxy|TrustProxyConfig\.Proxies' --type goLength of output: 2215
Line range hint
191-240: Significant changes to Context methods and featuresThe Context section outlines several important updates:
New features:
- Support for Partitioned cookies (CHIPS)
- New methods aligned with Express.js (e.g., AutoFormat, Host, Port)
Removed methods:
- Several parsing methods (e.g., BodyParser, CookieParser) have been removed in favor of the new Bind method.
Changed methods:
- Bind is now used for binding instead of View (use ViewBind for views)
- Redirect now uses a chaining API (e.g., c.Redirect().To())
These changes aim to align Fiber more closely with Express.js and introduce more modern features like CHIPS support. However, they may require significant updates to existing Fiber applications, especially those heavily relying on the removed or changed methods.
To verify these changes, we can check the context implementation:
🧰 Tools
🪛 LanguageTool
[style] ~57-~57: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...nablePrintRoutes - ListenerNetwork -> previously Network - app.Config.EnabledTrustedProx...(ADVERB_REPETITION_PREMIUM)
.github/README.md (1)
564-567: New configuration options for trusted proxies look good.The changes introduce a more flexible and secure way to configure trusted proxies:
TrustProxy: trueenables the trusted proxy feature.TrustProxyConfigallows for fine-grained control over which proxies to trust.- The
Proxiesfield accepts both IP addresses and IP ranges, enhancing flexibility.These changes align well with the PR objectives to improve proxy handling and provide a more Express-like API. The new configuration options offer better security by allowing developers to explicitly specify trusted proxies.
To ensure these changes are properly documented and consistent throughout the codebase, let's run a quick check:
✅ Verification successful
Documentation Consistency Verified
The documentation consistently references
TrustProxyConfig, reflecting the recent changes:
docs/whats_new.mddocs/api/ctx.mddocs/api/fiber.mdAll relevant documentation appears up-to-date and accurate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent documentation of the new TrustProxyConfig rg -i "trustproxyconfig" --type mdLength of output: 2613
docs/api/ctx.md (1)
Line range hint
1-1648: Overall, excellent updates to the Ctx documentation!The changes to the
IsProxyTrustedandSchemamethod documentation significantly improve the clarity and security awareness of the Fiber framework's proxy handling capabilities. These updates will help developers better understand and implement secure configurations, especially when working with applications behind proxies.app.go (2)
390-392: InitializeDefaultTrustProxyConfigwith meaningful defaultsThe
DefaultTrustProxyConfigis currently initialized with zero values, which means all boolean fields are set tofalse. To align with the PR objectives of simplifying the configuration process and trusting common internal IP ranges by default, consider initializingDefaultTrustProxyConfigwith these fields set totrue.Apply this diff to set default values:
var DefaultTrustProxyConfig = TrustProxyConfig{ + LinkLocal: true, + Loopback: true, + Private: true, }Please verify that changing these default values aligns with the desired security posture and does not introduce unintended trust of unverified proxies.
395-418: Export unexported fields if serialization is requiredThe fields
ipsandrangesinTrustProxyConfigare unexported, which means they won't be serialized when usingjson.Marshal. If serialization of the entire configuration is intended, consider exporting these fields.Please verify if these fields need to be exported. If they are meant to be internal and serialization is not required, then no action is needed. Otherwise, consider the following change:
-type TrustProxyConfig struct { - ips map[string]struct{} - ranges []*net.IPNet{} +type TrustProxyConfig struct { + Ips map[string]struct{} + Ranges []*net.IPNet{} // ... existing fields ... }Be cautious that exporting these fields may expose internal implementation details.
ctx_test.go (5)
Line range hint
1575-1582: Clarify the behavior when 'Proxies' slice is empty in 'TrustProxyConfig'.In the test
Test_Ctx_Host_UntrustedProxy, theProxiesslice is initialized as an empty slice:app := New(Config{ TrustProxy: true, TrustProxyConfig: TrustProxyConfig{ Proxies: []string{}, }, })Please verify that an empty
Proxiesslice correctly means no proxies are trusted. If the intention is to trust no proxies, this setup is correct. Otherwise, consider settingProxiestonilor specifying the proxies to trust.
1664-1666: Unexpected behavior when 'TrustProxy' is 'true' without specified proxies.Here,
TrustProxyis set totrue, but no proxies are specified:app := New(Config{ TrustProxy: true, }) c := app.AcquireCtx(&fasthttp.RequestCtx{}) require.False(t, c.IsProxyTrusted())Ensure that this configuration is intended to result in
c.IsProxyTrusted()returningfalse. If all proxies should be trusted when none are specified, this may need adjustment.
1671-1674: Verify the effect of empty 'Proxies' slice with 'TrustProxy' enabled.In this test,
TrustProxyistrue, butProxiesis an empty slice:app := New(Config{ TrustProxy: true, TrustProxyConfig: TrustProxyConfig{ Proxies: []string{}, }, })Please confirm that an empty
Proxiesslice correctly implies that no proxies are trusted, causingc.IsProxyTrusted()to returnfalse.
1691-1694: Validate CIDR notation handling in 'Proxies'.In this configuration, the
Proxiesslice includes127.0.0.1/8:app := New(Config{ TrustProxy: true, TrustProxyConfig: TrustProxyConfig{ Proxies: []string{"127.0.0.1/8"}, }, })Ensure that the CIDR notation is correctly parsed and that the proxy trust logic handles it as expected.
1787-1790: Possible typo in benchmark assertion.In the benchmark
Benchmark_Ctx_Hostname, the following assertion is made:require.Equal(b, "google.com", hostname)However, within the same code block,
c.Request().Header.Set(HeaderXForwardedHost, "google1.com")is set. Verify if the expected hostname should be"google.com"or"google1.com".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app.go (1)
354-354: Improve documentation comment for clarityConsider rephrasing the comment for better readability.
Apply this diff:
-// Read TrustProxy doc. +// Refer to the TrustProxy documentation for more details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app.go (5 hunks)
- docs/middleware/cors.md (3 hunks)
- middleware/cors/config.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- middleware/cors/config.go
🧰 Additional context used
🔇 Additional comments (5)
docs/middleware/cors.md (5)
121-121: Approved: Improved terminology in ExposeHeaders descriptionThe change from "whitelist" to "allowlist" is a positive update. It aligns with modern, more inclusive terminology while maintaining the same meaning. This modification enhances the clarity and precision of the documentation.
194-195: Approved: Consistent terminology and improved example in ExposeHeaders explanationThe changes in this section maintain consistency with the earlier terminology update, replacing "whitelist" with "allowlist". Additionally, the example has been refined to use "X-Custom-Header" instead of just "X-Custom-Header", which provides a more specific and clear illustration of how the ExposeHeaders option works.
210-211: Approved: Valuable addition to security considerationsThe new bullet point about limiting exposed headers is a excellent addition to the security considerations section. It provides specific, actionable advice on using the ExposeHeaders option to enhance security. This guidance aligns with the principle of least privilege and helps developers understand how to minimize the risk of exposing sensitive information through CORS headers.
210-211: Approved: Critical security advice for AllowOriginsFunc usageThe new content in the "Inadequate
AllowOriginsFuncValidation" section is a crucial addition to the documentation. It provides essential guidance on the secure use of AllowOriginsFunc, particularly highlighting the potential security risks when used in conjunction with AllowCredentials. The suggestion to use AllowOrigins with a wildcard "*" instead of AllowOriginsFunc for allowing all origins offers a safer alternative for developers. This addition significantly enhances the security-related guidance in the documentation.
Line range hint
1-224: Overall documentation improvements are commendableThe changes made to the CORS middleware documentation in this file are consistently positive and align well with the PR objectives. Key improvements include:
- Updated terminology for better inclusivity and clarity.
- Enhanced examples for better understanding of configuration options.
- Added crucial security considerations, particularly regarding the use of ExposeHeaders and AllowOriginsFunc.
These updates significantly enhance the quality and usefulness of the documentation, providing developers with clearer guidance on configuring and securing the CORS middleware in the Fiber framework.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~122-~122: You might be missing the article “the” here.
Context: ...lt. To disable caching completely, pass MaxAge value negative. It will set the Access-...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app.go (3)
333-357: LGTM! Comprehensive documentation for new proxy trust configuration.The addition of
TrustProxyandTrustProxyConfigfields to theConfigstruct, along with their detailed documentation, is well-implemented. The explanation of different scenarios based on the request IP's presence in the allowlist is thorough and helpful.Consider adding a brief example of how to set up
TrustProxyConfigin the documentation to make it even more user-friendly.
393-418: LGTM! Well-structured TrustProxyConfig with clear configuration options.The
TrustProxyConfigstruct provides a comprehensive set of options for configuring trusted proxies. The use of unexported fields for internal processing and exported fields for user configuration is a good design choice.Consider adding godoc comments for the exported fields (
Proxies,LinkLocal,Loopback,Private) to provide more context on their usage directly in the code.
545-546: LGTM! Proper initialization of TrustProxyConfig.ips map.The initialization of the
TrustProxyConfig.ipsmap and the loop to process each proxy are good practices to ensure the configuration is properly set up.Consider providing an initial capacity hint for the map to potentially improve performance:
-app.config.TrustProxyConfig.ips = make(map[string]struct{}, len(app.config.TrustProxyConfig.Proxies)) +app.config.TrustProxyConfig.ips = make(map[string]struct{}, len(app.config.TrustProxyConfig.Proxies))This can help reduce map resizing operations if the number of proxies is known in advance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app.go (5 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app.go (2)
390-391: LGTM! Default configuration for TrustProxyConfig.The addition of
DefaultTrustProxyConfigprovides a sensible default for when no specific configuration is provided. This is a good practice for configuration management.
Line range hint
333-580: Overall LGTM with suggestions for improvement.The introduction of
TrustProxyandTrustProxyConfigprovides a comprehensive and flexible way to configure trusted proxies in the Fiber framework. The implementation is generally solid, with good error handling and clear documentation.Main points for improvement:
- Enhance thread safety in the
handleTrustedProxymethod and related operations.- Add more detailed documentation for the exported fields in the
TrustProxyConfigstruct.- Consider providing usage examples in the documentation for
TrustProxyConfig.These changes significantly enhance the proxy handling capabilities of the Fiber framework while maintaining good code quality and user-friendliness.
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
@xEricL Thank you! 💪 |
Description
This renames
EnableTrustedProxyChecktoTrustProxy. It also adds a new config option calledTrustProxyConfig, allowing developers to trust loopback, private, or link-local IP addresses without adding the ranges to the whitelist.If these ranges are added manually, it can negatively impact performance (see #2933 benchmarks).
I chose these three ranges because these are the same ranges enabled by default when configuring an IP extractor in Echo.
Changes introduced
Type of change
Checklist
/docs/directory for Fiber's documentation.