search

istio-ecosystem / authservice

Move OIDC token acquisition out of your app code and into the Istio mesh
Apache License 2.0
217 stars 63 forks source link

Fix url encoding handling for code parameter. #206

Closed incfly closed 2 years ago

incfly commented 2 years ago

Signed-off-by: Jianfei Hu hujianfei258@gmail.com

Fix https://github.com/istio-ecosystem/authservice/issues/205

incfly commented 2 years ago

/cc @vikaschoudhary16

istio-testing commented 2 years ago

@incfly: GitHub didn't allow me to request PR reviews from the following users: vikaschoudhary16.

Note that only istio-ecosystem members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/istio-ecosystem/authservice/pull/206#issuecomment-1019820894): >/cc @vikaschoudhary16 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vikaschoudhary16 commented 2 years ago

Accommodating just one character, /, will not help for very long I think. As we discussed offline, in my case it was the client(browser) which is passing code in the decoded form. my authz code was having %2f in encoded form which in decoded form is /. Is not it possible that there is some other combination of letters in encoded form which when decode is some other key than the /. Instead of handling / individually, can we determine if the code is encoded or decode by checking presence of reserved keys and if already decoded, skip decoding.

incfly commented 2 years ago

Accommodating just one character, /, will not help for very long I think.

There's actually only a few char to be considred. /, : = etc. The = would not be considered valid. Also in other identity provider, the issue does not occur, say Auth0. I think it's fine to be conservative and keep track of what characters we allow explicitly. i.e. only add chars when we encounter one.

https://stackoverflow.com/questions/14856241/url-encoding-of-the-state-parameter-for-google-oauth2-gets-decoded-during-redire

vikaschoudhary16 commented 2 years ago

Accommodating just one character, /, will not help for very long I think.

There's actually only a few char to be considred. /, : = etc. The = would not be considered valid. Also in other identity provider, the issue does not occur, say Auth0. I think it's fine to be conservative and keep track of what characters we allow explicitly. i.e. only add chars when we encounter one.

https://stackoverflow.com/questions/14856241/url-encoding-of-the-state-parameter-for-google-oauth2-gets-decoded-during-redire

sounds good!

istio-testing commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incfly, Shikugawa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/istio-ecosystem/authservice/blob/master/OWNERS)~~ [Shikugawa,incfly] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment