csrf: fix issues with host/origin header parsing #12133
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
After envoyproxy#11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]>
Signed-off-by: Raul Gutierrez Segales <[email protected]>
Two things were missing: * the origin/referer headers in the integration test needed their respective schemes. * for some reason, headers.Scheme() is nullptr during integration tests... Hmm, not sure why. Signed-off-by: Raul Gutierrez Segales <[email protected]>
dschaller
reviewed
Jul 16, 2020
mattklein123
approved these changes
Jul 17, 2020
KBaichoo
pushed a commit
to KBaichoo/envoy
that referenced
this pull request
Jul 30, 2020
After envoyproxy#11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>
scheler
pushed a commit
to scheler/envoy
that referenced
this pull request
Aug 4, 2020
After envoyproxy#11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]> Signed-off-by: scheler <[email protected]>
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.
Commit Message: csrf: fix issues with host/origin header parsing
Additional Description:
After #11670, the CSRF filter started failing for us.
This change fixes 3 issues that were uncovered after moving
to gURL for parsing URLs:
the hostAndPort() utility method, in the CSRF filter, was
returning a string view of a stack variable.
the Origin header always includes the scheme, so let's ensure
this is exercised in tests (which were missing this and passing
due to relaxed checks).
the Url::initialize method expects an absolute URL, something that
the CSRF filter wasn't complying with when parsing the Host header.
Risk Level: low
Testing: changed/added
Docs Changes: none
Release Notes: added
Signed-off-by: Raul Gutierrez Segales [email protected]