Should getLastAddressFromXFF() handle comma-separated lists without spaces too? #3607
Labels
Comments
|
Looks like both cases (space and no space) are valid in HTTP lists: |
|
Ugh. Yes, we definitely need to handle both cases. cc @danielhochman this might require another rollback. We might want to just revert 035a816 while we sort this out. |
rgs1
pushed a commit
to rgs1/envoy
that referenced
this issue
Jun 12, 2018
Currently, `Utility::getLastAddressFromXFF()` only handles lists that use a comma plus a space as the separator. Per https://tools.ietf.org/html/rfc7239#section-7.1, spaces are allowed around commas — so we should handle that too. This fixes envoyproxy#3607. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
|
@mattklein123 something like #3610 perhaps? |
rgs1
pushed a commit
to rgs1/envoy
that referenced
this issue
Jun 13, 2018
This should catch some regressions related to XFF parsing, like what's described in envoyproxy#3607. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
alyssawilk
pushed a commit
that referenced
this issue
Jun 13, 2018
This should catch some regressions related to XFF parsing, like what's described in #3607. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I just bumped into 035a816 where
Utility::appendXff()went from generatingx-forwarded-forwith a comma plus a space as the separator, to just a comma (no spaces).Does that mean that
Utility::getLastAddressFromXFF()needs to handle both types of separators (with and without spaces)? The IP tagging module certainly breaks when there's no space after the comma...Happy to send a PR if the desired behavior is to handle both cases (I am looking up what's actually valid for XFF values...).
@mattklein123, @alyssawilk thoughts?
The text was updated successfully, but these errors were encountered: