Remove rack_strip_client_ip middleware #2629
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes middleware that strips the Client-IP header from requests to prevent Rails from raising a "IpSpoofAttackError" exception. Removing this because it isn't needed, we are stripping the Client-IP header at the CDN level which should prevent this expection being raised anyway. If this header is being set else were in the request path, we should fix that instead of blindly stripping the header altogether. There is also a bug in the middleware, where it prevent previous middleware from receiving information from following middleware in the 'env' variable. This is because it creates a copy of env, which isn't returned to the calling middleware. This prevents middleware such as the prometheus exporter from functioning properly.
sengi
approved these changes
Dec 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes middleware that strips the Client-IP header from requests to prevent Rails from raising a "IpSpoofAttackError" exception. Removing this because it isn't needed, we are stripping the Client-IP header at the CDN level which should prevent this expection being raised anyway. If this header is being set else were in the request path, we should fix that instead of blindly stripping the header altogether.
There is also a bug in the middleware, where it prevents previous middleware from receiving information from following middleware in the 'env' variable. This is because it creates a copy of env, which isn't returned to the calling middleware. This prevents middleware such as the prometheus exporter from generating correct metrics stats.
https://github.com/alext/rack_strip_client_ip
Details of the bug:
Instead of editing the same env object it creates a copy using the "reject" method. This new copy is passed on to subsequent middleware, which means any changes to 'env' isn't available to previous middleware.
https://github.com/alext/rack_strip_client_ip/blob/98029a661616b5a1091e6b822aed5b498a4e5d42/lib/rack_strip_client_ip/middleware.rb#LL14
Follow these steps if you are doing a Rails upgrade.