fix: redirct_url with query escape character outside of query is failing #480

ajanthan · 2020-09-19T00:43:16Z

Related issue

ory/hydra#2055

Proposed changes

Updated the GetRedirectURIFromRequestValues() method to unescape the query parameters only.

Checklist

I have read the contributing guidelines
I have read the security policy
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation within the code base (if appropriate)

Further comments

…utside of query parameters

mitar · 2020-09-20T05:37:53Z

Thank you for working on this. I have reported this issue in the past in #464. So this is not the only place where this code logic exist, so it would be great if you could also go around the full codebase (just search for QueryUnescape) and fix it everywhere? And add also related tests.

mitar · 2020-09-20T05:38:13Z

authorize_helper.go

@@ -41,11 +41,17 @@ import (
 func GetRedirectURIFromRequestValues(values url.Values) (string, error) {
 	// rfc6749 3.1.   Authorization Endpoint
 	// The endpoint URI MAY include an "application/x-www-form-urlencoded" formatted (per Appendix B) query component
-	redirectURI, err := url.QueryUnescape(values.Get("redirect_uri"))
+	redirectURI, err := url.Parse(values.Get("redirect_uri"))


I think this is correct. We just want to validate the redirect_uri value.

I'm also wondering if values.Get isn't already unescaped?

Yes. That is the root of the issue.

mitar · 2020-09-20T05:39:37Z

authorize_helper.go

 	if err != nil {
 		return "", errors.WithStack(ErrInvalidRequest.WithHint(`The "redirect_uri" parameter is malformed or missing.`).WithCause(err).WithDebug(err.Error()))
 	}
-	return redirectURI, nil
+	rawQuery, err := url.QueryUnescape(redirectURI.RawQuery)


But I do not think this is necessary. values should already include correctly parsed parameter from the form payload, so no further unescaping of the redirect_uri should be necessary. We should just pass it on as-is. We should just validate that the redirect_uri is valid URL.

Do you have a test case where QueryUnescape is necessary?

Without escaping the query part, one existing test case fails.
According to this test case, During the client registration, if redirct_uri has an url-encoded query parameter, it is expected to be escaped. I am not sure if it is expected according to the spec.

But that unescaping should be done by accessing the query string using Query(), no? So form payload unparses it once as a whole redirect_uri parameter. And then URL as result of url.Parse has Query() method which unescapes when needed. There is no point in doing redirectURI.RawQuery = url.QueryUnescape(redirectURI.RawQuery).

I think the confusion here is from this sentence in the spec:

The endpoint URI MAY include an "application/x-www-form-urlencoded" formatted (per Appendix B) query component ([RFC3986] Section 3.4), which MUST be retained when adding additional query parameters.

I think confusion here is that this does not mean that there is extra escaping going on, but that standard query string escaping is what should be done, or is expected that it has been done in the provided redirect URI. That standard escaping is called application/x-www-form-urlencoded escaping. Also, both urlescape("param1=value1&param2=value2") and "param1=" + urlescape(value1) + "&param2=" + urlescape(value2) form are both exactly equivalent and should be processed the same by url.Query() call. See this seciton (referenced from RFC section referenced from the test you linked above) which says:

However, as query components are often used to carry identifying information in the form of "key=value" pairs and one frequently used value is a reference to another URI, it is sometimes better for usability to avoid percent-encoding those characters.

So urlescaping also + and = characters in the query string is fine. But it is not necessary for usability. But both of those are demeed to be "application/x-www-form-urlencoded" formatted. In any case, we do not have to double unescape things. We just have to make sure that we append additional query values to existing. Which we should be doing by calling url.Query() (which would unescape things), add additional ones, and then encode them back. Which I think we are doing? See here. So there we unescape at that point, add more query values, and escape back.

I think the point the spec is trying to make is that you should expect that proper escaping of the query string has been done and use that knowledge to parse the query string to be able to append new things.

So, no need for double decoding here.

Yes. I completely agree with you. So, we can get rid of GetRedirectURIFromRequestValues method and the associated test case.

No, we can keep it, just to verify the redirect URI.

You should probably update the test case accordingly.

ajanthan · 2020-09-23T21:29:38Z

Thank you for working on this. I have reported this issue in the past in #464. So this is not the only place where this code logic exist, so it would be great if you could also go around the full codebase (just search for QueryUnescape) and fix it everywhere? And add also related tests.

In other places[1][2][3][4], QueryUnescape is used to unescape the special URL characters from the Basic authentication username and password. These special characters won't be escaped during the golang request parsing. It makes sense to keep the logic there.

WDYT?

[1]

fosite/introspection_request_handler.go

Line 140 in b53f8f5

clientID, err := url.QueryUnescape(id)

[2]

fosite/introspection_request_handler.go

Line 145 in b53f8f5

clientSecret, err := url.QueryUnescape(secret)

[3]

fosite/client_authentication.go

Line 277 in b53f8f5

} else if clientID, err = url.QueryUnescape(id); err != nil {

[4]

fosite/client_authentication.go

Line 279 in b53f8f5

} else if clientSecret, err = url.QueryUnescape(secret); err != nil {

mitar · 2020-09-24T04:38:26Z

These special characters won't be escaped during the golang request parsing. It makes sense to keep the logic there.

You are right. Those are the only ones left. And yes, those should be kept in as per RFC itself. So cool, this is then the only place left to fix this.

mitar · 2020-09-25T17:22:26Z

authorize_helper.go

-//   "application/x-www-form-urlencoded" formatted (per Appendix B) query
-//   component ([RFC3986] Section 3.4), which MUST be retained when adding
-//   additional query parameters.
-func GetRedirectURIFromRequestValues(values url.Values) (string, error) {


No, as I mentioned. You should still validate that the url is correct. So just parse the URL, check error return value, and this is it.

Sorry, I realized that this function does not have to validate anything, it is written in its description and redirect URI validation is done later in MatchRedirectURIWithClientRedirectURIs.

I do agree that it could probably just be removed. But let's wait for @aeneasr to confirm.

mitar · 2020-09-25T19:34:11Z

authorize_helper.go

 	if err != nil {
 		return "", errors.WithStack(ErrInvalidRequest.WithHint(`The "redirect_uri" parameter is malformed or missing.`).WithCause(err).WithDebug(err.Error()))
+	} else if rawRedirectURI != "" && (redirectURI.Scheme == "" || redirectURI.Host == "") {


Any particular reason to add this additional check at this point?

According to the test case empty redirect_uri is allowed

Golang url.parse doesn't catch bad URL scheme or host thus the method fails to catch urls like https//google.com/foo=bar

That's why we have url.ParseRequestURI("https//google.com/foo=bar") :)

https://play.golang.org/p/eblL4ikZTSi

Please keep in mind that this errors if the string is empty, so you need to handle that as a special case!

But this function should not be doing any validation based on its docstring!

So see other comments. Both @ajanthan and me think now that this function should just be removed and replaced with simply calling values.Get("redirect_uri"). The validation happens anyway in MatchRedirectURIWithClientRedirectURIs.

Right, overlooked that - makes sense!

aeneasr · 2020-10-02T08:48:04Z

@ajanthan are you still up for the changes? :) If you need any help, let us know!

…edir � Conflicts: � authorize_helper_test.go

…ecial characters again in the query parameters

ajanthan · 2020-10-02T21:28:05Z

@aeneasr updated the pr

mitar

Looks good to me.

aeneasr

Awesome 🎉

Thank you for your contribution!

ajanthan added 2 commits September 18, 2020 17:35

Fixing redirect_uri validation for url with query escape characters o…

d43bad2

…utside of query parameters

Merge branch 'specCharRedir' of https://github.com/ajanthan/fosite

4ef10b4

mitar suggested changes Sep 20, 2020

View reviewed changes

mitar reviewed Sep 25, 2020

View reviewed changes

Fixing redirect_uri parsing and validation logic for double escaping

94ab1a2

ajanthan force-pushed the specCharRedir branch from 72dcfe8 to 94ab1a2 Compare September 25, 2020 19:30

mitar reviewed Sep 25, 2020

View reviewed changes

ajanthan changed the title ~~Bug: redirct_url with query escape character outside of query is failing~~ fix: redirct_url with query escape character outside of query is failing Sep 26, 2020

Merge branch 'master' into specCharRedir

8dff2de

ajanthan added 2 commits October 2, 2020 13:55

Merge branch 'master' of https://github.com/ory/fosite into specCharR…

b4ae42e

…edir � Conflicts: � authorize_helper_test.go

Remving GetRedirectURIFromRequestValues as it not needed to escape sp…

e639c17

…ecial characters again in the query parameters

ajanthan requested a review from aeneasr October 2, 2020 21:28

mitar approved these changes Oct 2, 2020

View reviewed changes

aeneasr approved these changes Oct 6, 2020

View reviewed changes

aeneasr merged commit 6e49c57 into ory:master Oct 6, 2020

mitar mentioned this pull request Oct 7, 2020

Double query parameter escaping around code #464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: redirct_url with query escape character outside of query is failing #480

fix: redirct_url with query escape character outside of query is failing #480

ajanthan commented Sep 19, 2020

mitar commented Sep 20, 2020

mitar Sep 20, 2020

aeneasr Sep 20, 2020

ajanthan Sep 22, 2020

mitar Sep 20, 2020

ajanthan Sep 22, 2020

mitar Sep 24, 2020 •

edited

Loading

ajanthan Sep 25, 2020

mitar Sep 25, 2020

ajanthan commented Sep 23, 2020

mitar commented Sep 24, 2020

mitar Sep 25, 2020

mitar Sep 25, 2020

mitar Sep 25, 2020

ajanthan Sep 25, 2020

aeneasr Sep 30, 2020

aeneasr Sep 30, 2020

mitar Sep 30, 2020

aeneasr Sep 30, 2020

aeneasr commented Oct 2, 2020

ajanthan commented Oct 2, 2020

mitar left a comment

aeneasr left a comment

fix: redirct_url with query escape character outside of query is failing #480

fix: redirct_url with query escape character outside of query is failing #480

Conversation

ajanthan commented Sep 19, 2020

Related issue

Proposed changes

Checklist

Further comments

mitar commented Sep 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitar Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajanthan commented Sep 23, 2020

mitar commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Oct 2, 2020

ajanthan commented Oct 2, 2020

mitar left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

mitar Sep 24, 2020 •

edited

Loading