fix: redirct_url with query escape character outside of query is fail…

…ing (#480) See ory/hydra#2055 Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com> Co-authored-by: ajanthan <ca52ca6fe18c44787827017e14ca2d0c3c5bdb58>
ory · Oct 6, 2020 · 6e49c57 · 6e49c57
1 parent f163102
commit 6e49c57
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 49 deletions.
diff --git a/authorize_helper.go b/authorize_helper.go
@@ -30,24 +30,6 @@ import (
 	"github.com/pkg/errors"
 )
 
-// GetRedirectURIFromRequestValues extracts the redirect_uri from values but does not do any sort of validation.
-//
-// Considered specifications
-// * https://tools.ietf.org/html/rfc6749#section-3.1.2
-//   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.
-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"))
-	if err != nil {
-		return "", errors.WithStack(ErrInvalidRequest.WithHint(`The "redirect_uri" parameter is malformed or missing.`).WithCause(err).WithDebug(err.Error()))
-	}
-	return redirectURI, nil
-}
-
 // MatchRedirectURIWithClientRedirectURIs if the given uri is a registered redirect uri. Does not perform
 // uri validation.
 //

diff --git a/authorize_helper_test.go b/authorize_helper_test.go
@@ -47,33 +47,6 @@ func TestIsLocalhost(t *testing.T) {
 	}
 }
 
-// Test for
-// * https://tools.ietf.org/html/rfc6749#section-3.1.2
-//   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.
-func TestGetRedirectURI(t *testing.T) {
-	for k, c := range []struct {
-		in       string
-		isError  bool
-		expected string
-	}{
-		{in: "", isError: false, expected: ""},
-		{in: "https://google.com/", isError: false, expected: "https://google.com/"},
-		{in: "https://google.com/?foo=bar%20foo+baz", isError: false, expected: "https://google.com/?foo=bar foo baz"},
-	} {
-		values := url.Values{}
-		values.Set("redirect_uri", c.in)
-		res, err := GetRedirectURIFromRequestValues(values)
-		assert.Equal(t, c.isError, err != nil, "%s", err)
-		if err == nil {
-			assert.Equal(t, c.expected, res)
-		}
-		t.Logf("Passed test case %d", k)
-	}
-}
-
 // rfc6749 10.6.
 // Authorization Code Redirection URI Manipulation
 // The authorization server	MUST require public clients and SHOULD require confidential clients
@@ -239,6 +212,18 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
 			url:     "https://www.ory.sh/cb",
 			isError: true,
 		},
+		{
+			client:   &DefaultClient{RedirectURIs: []string{"web+application://callback"}},
+			url:      "web+application://callback",
+			isError:  false,
+			expected: "web+application://callback",
+		},
+		{
+			client:   &DefaultClient{RedirectURIs: []string{"https://google.com/?foo=bar%20foo+baz"}},
+			url:      "https://google.com/?foo=bar%20foo+baz",
+			isError:  false,
+			expected: "https://google.com/?foo=bar%20foo+baz",
+		},
 	} {
 		redir, err := MatchRedirectURIWithClientRedirectURIs(c.url, c.client)
 		assert.Equal(t, c.isError, err != nil, "%d: %+v", k, c)

diff --git a/authorize_request_handler.go b/authorize_request_handler.go
@@ -160,10 +160,7 @@ func (f *Fosite) authorizeRequestParametersFromOpenIDConnectRequest(request *Aut
 
 func (f *Fosite) validateAuthorizeRedirectURI(r *http.Request, request *AuthorizeRequest) error {
 	// Fetch redirect URI from request
-	rawRedirURI, err := GetRedirectURIFromRequestValues(request.Form)
-	if err != nil {
-		return err
-	}
+	rawRedirURI := request.Form.Get("redirect_uri")
 
 	// Validate redirect uri
 	redirectURI, err := MatchRedirectURIWithClientRedirectURIs(rawRedirURI, request.Client)

diff --git a/authorize_request_handler_test.go b/authorize_request_handler_test.go
@@ -49,6 +49,7 @@ func TestNewAuthorizeRequest(t *testing.T) {
 	defer ctrl.Finish()
 
 	redir, _ := url.Parse("https://foo.bar/cb")
+	specialCharRedir, _ := url.Parse("web+application://callback")
 	for k, c := range []struct {
 		desc          string
 		conf          *Fosite
@@ -226,6 +227,41 @@ func TestNewAuthorizeRequest(t *testing.T) {
 				},
 			},
 		},
+		/* redirect_uri with special character in protocol*/
+		{
+			desc: "redirect_uri with special character",
+			conf: &Fosite{Store: store, ScopeStrategy: ExactScopeStrategy, AudienceMatchingStrategy: DefaultAudienceMatchingStrategy},
+			query: url.Values{
+				"redirect_uri":  {"web+application://callback"},
+				"client_id":     {"1234"},
+				"response_type": {"code token"},
+				"state":         {"strong-state"},
+				"scope":         {"foo bar"},
+				"audience":      {"https://cloud.ory.sh/api https://www.ory.sh/api"},
+			},
+			mock: func() {
+				store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{
+					ResponseTypes: []string{"code token"},
+					RedirectURIs:  []string{"web+application://callback"},
+					Scopes:        []string{"foo", "bar"},
+					Audience:      []string{"https://cloud.ory.sh/api", "https://www.ory.sh/api"},
+				}, nil)
+			},
+			expect: &AuthorizeRequest{
+				RedirectURI:   specialCharRedir,
+				ResponseTypes: []string{"code", "token"},
+				State:         "strong-state",
+				Request: Request{
+					Client: &DefaultClient{
+						ResponseTypes: []string{"code token"}, RedirectURIs: []string{"web+application://callback"},
+						Scopes:   []string{"foo", "bar"},
+						Audience: []string{"https://cloud.ory.sh/api", "https://www.ory.sh/api"},
+					},
+					RequestedScope:    []string{"foo", "bar"},
+					RequestedAudience: []string{"https://cloud.ory.sh/api", "https://www.ory.sh/api"},
+				},
+			},
+		},
 	} {
 		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
 			c.mock()