oauth2: fix passthrough logic to avoid modifying request when skipping filter#40718
Conversation
|
@zhaohuabing could you take a look? |
|
@ggmoy you need to write a unit test for this to prevent this from being resurfaced again. |
|
@mathetake, the unit test has just been added. |
|
@mathetake , I'm just looking into the |
|
@mathetake, envoyproxy/examples PR #814 has been created to fix the issue with the |
|
you can ignore the verify_examples CI for now -- it's not mandatory for PRs as long as they don't touch them |
|
would be good to get signoff from @zhaohuabing here and in examples repo preferably dont land with examples broken as it will break examples for all ci |
zhaohuabing
left a comment
There was a problem hiding this comment.
This PR looks good to me. Thanks for fixing this!
5fc7245 to
337e4cf
Compare
|
@phlax / @zhaohuabing / @wbpcode , all tests ran successfully now that envoyproxy/examples PR #819 is merged. |
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Thought it looks goold to me. ping @zhaohuabing for a second check to this for the logic change.
|
/wait-any |
337e4cf to
7823b58
Compare
The PR makes sense as multiple OAuth2 filters may exist in the same HCM filter chain, and each has its own PassThroughMatcher. |
|
@ggmoy Can you rebase this PR based on the lastest main branch? |
7823b58 to
7b2726d
Compare
Signed-off-by: Gustavo Moyano <gustavo.g.moyano@gmail.com>
Signed-off-by: Gustavo Moyano <gustavo.g.moyano@gmail.com>
Signed-off-by: Gustavo Moyano <gustavo.g.moyano@gmail.com>
7b2726d to
5d05e37
Compare
@zhaohuabing, done! |
…g filter (envoyproxy#40718) ### Description This PR fixes a bug introduced in PR envoyproxy#40228, where OAuth2 cookies were removed for requests matching the `pass_through_matcher` configuration. This broke setups with multiple OAuth2 filter instances using different `pass_through_matcher` configurations, because the first matching instance removed the OAuth2 cookies--even when a passthrough was intended--impacting subsequent filters that still needed those cookies. The changes in this PR realign the filter behavior with the documentation, which states that `pass_through_matcher` provides an interface for users to specify header matching criteria such that, when applicable, the OAuth flow is entirely skipped. When this occurs, only the `oauth_passthrough` metric is incremented; no cookies or request headers are modified, and no OAuth success is recorded. In summary, when the `pass_through_matcher` configuration matches, the filter should simply skip processing and leave the request untouched. --- **Commit Message:** oauth2: fix passthrough logic to avoid modifying request when skipping filter **Risk Level:** Low **Testing:** Unit test added and single page app test fixed by [envoyproxy/examples PR envoyproxy#819](envoyproxy/examples#819) **Docs Changes:** N/A **Release Notes:** Added **Platform Specific Features:** N/A --------- Signed-off-by: Gustavo Moyano <gustavo.g.moyano@gmail.com> Signed-off-by: Melissa Ginaldi <mginaldi@google.com>
…g filter (envoyproxy#40718) ### Description This PR fixes a bug introduced in PR envoyproxy#40228, where OAuth2 cookies were removed for requests matching the `pass_through_matcher` configuration. This broke setups with multiple OAuth2 filter instances using different `pass_through_matcher` configurations, because the first matching instance removed the OAuth2 cookies--even when a passthrough was intended--impacting subsequent filters that still needed those cookies. The changes in this PR realign the filter behavior with the documentation, which states that `pass_through_matcher` provides an interface for users to specify header matching criteria such that, when applicable, the OAuth flow is entirely skipped. When this occurs, only the `oauth_passthrough` metric is incremented; no cookies or request headers are modified, and no OAuth success is recorded. In summary, when the `pass_through_matcher` configuration matches, the filter should simply skip processing and leave the request untouched. --- **Commit Message:** oauth2: fix passthrough logic to avoid modifying request when skipping filter **Risk Level:** Low **Testing:** Unit test added and single page app test fixed by [envoyproxy/examples PR envoyproxy#819](envoyproxy/examples#819) **Docs Changes:** N/A **Release Notes:** Added **Platform Specific Features:** N/A --------- Signed-off-by: Gustavo Moyano <gustavo.g.moyano@gmail.com>
Description
This PR fixes a bug introduced in PR #40228, where OAuth2 cookies were removed for requests matching the
pass_through_matcherconfiguration. This broke setups with multiple OAuth2 filter instances using differentpass_through_matcherconfigurations, because the first matching instance removed the OAuth2 cookies--even when a passthrough was intended--impacting subsequent filters that still needed those cookies.The changes in this PR realign the filter behavior with the documentation, which states that
pass_through_matcherprovides an interface for users to specify header matching criteria such that, when applicable, the OAuth flow is entirely skipped. When this occurs, only theoauth_passthroughmetric is incremented; no cookies or request headers are modified, and no OAuth success is recorded.In summary, when the
pass_through_matcherconfiguration matches, the filter should simply skip processing and leave the request untouched.Commit Message: oauth2: fix passthrough logic to avoid modifying request when skipping filter
Risk Level: Low
Testing: Unit test added and single page app test fixed by envoyproxy/examples PR #819
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A