Fix url encoding handling for code parameter.

[incfly]

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

Fix #205

[incfly]

/cc @vikaschoudhary16

[istio-testing]

@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:

/cc @vikaschoudhary16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Shikugawa]

I think that http.cc should be independent from filter implementation. It seems this rule is OIDC specific. Thus we should move this to oidc_filter.cc or new file such as oidc_query_decoder.cc and pass it to Http::DecodeQueryData. For example, I prefer to implement that as follows:

class UrlQueryDecoder {
  virtual absl::string_view decode(absl::string_view) = 0;
};

class OidcUrlQueryDecoder : public UrlQueryDecoder {
    // implement...
};

But, we should prioritize to fix this problem, Therefore we should postpone this refactor as TODO.

[incfly]

Add a TODO.

I think putting out of http lib is fine (this is not filter lib, but http, btw). but I'm not sure if we have to setup class/inheritance, especially if there's only one method, without actual state to maintain.
Pure util function seems work well and clear.

[incfly]

ping, is that sufficient for now? @Shikugawa

[Shikugawa]

Yes. SGTM.

[vikaschoudhary16]

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]

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]

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]

[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 [Shikugawa,incfly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment