fix: url-encode the fragment in the redirect URL of the authorize res…

…ponse (#649)

This patch reverts the encoding logic for the fragment of the redirect URL returned as part of the authorize response to what was the one before version `0.36.0`. In that version, the code was refactored and the keys and values of the fragment ceased to be url-encoded. This in turn reflected on all Ory Hydra versions starting from `1.9.0` and provoked a breaking change that made the parsing of the fragment impossible if any of the params contain a character like `&` or `=` because they get treated as separators instead of as text

Fixes #648

authorize_helper.go

@@ -212,6 +212,7 @@ func WriteAuthorizeFormPostResponse(redirectURL string, parameters url.Values, t
 	})
 }
 
+// Deprecated: Do not use.
 func URLSetFragment(source *url.URL, fragment url.Values) {
 	var f string
 	for k, v := range fragment {

authorize_write.go

@@ -57,8 +57,13 @@ func (f *Fosite) WriteAuthorizeResponse(rw http.ResponseWriter, ar AuthorizeRequ
 		// Implicit grants
 		// The endpoint URI MUST NOT include a fragment component.
 		redir.Fragment = ""
-		URLSetFragment(redir, resp.GetParameters())
-		sendRedirect(redir.String(), rw)
+
+		u := redir.String()
+		fr := resp.GetParameters()
+		if len(fr) > 0 {
+			u = u + "#" + fr.Encode()
+		}
+		sendRedirect(u, rw)
 		return
 	default:
 		if f.ResponseModeHandler().ResponseModes().Has(rm) {

authorize_write_test.go

@@ -118,7 +118,7 @@ func TestWriteAuthorizeResponse(t *testing.T) {
 			expect: func() {
 				assert.Equal(t, http.Header{
 					"X-Bar":         {"baz"},
-					"Location":      {"https://foobar.com/?foo=bar#bar=b+az%20ab"},
+					"Location":      {"https://foobar.com/?foo=bar#bar=b%2Baz+ab"},
 					"Cache-Control": []string{"no-store"},
 					"Pragma":        []string{"no-cache"},
 				}, header)
@@ -160,7 +160,45 @@ func TestWriteAuthorizeResponse(t *testing.T) {
 			expect: func() {
 				assert.Equal(t, http.Header{
 					"X-Bar":         {"baz"},
-					"Location":      {"https://foobar.com/?foo=bar#scope=api:*"},
+					"Location":      {"https://foobar.com/?foo=bar#scope=api%3A%2A"},
+					"Cache-Control": []string{"no-store"},
+					"Pragma":        []string{"no-cache"},
+				}, header)
+			},
+		},
+		{
+			setup: func() {
+				redir, _ := url.Parse("https://foobar.com/?foo=bar#bar=baz")
+				ar.EXPECT().GetRedirectURI().Return(redir)
+				ar.EXPECT().GetResponseMode().Return(ResponseModeFragment)
+				resp.EXPECT().GetParameters().Return(url.Values{"qux": {"quux"}})
+				resp.EXPECT().GetHeader().Return(http.Header{})
+
+				rw.EXPECT().Header().Return(header).Times(2)
+				rw.EXPECT().WriteHeader(http.StatusSeeOther)
+			},
+			expect: func() {
+				assert.Equal(t, http.Header{
+					"Location":      {"https://foobar.com/?foo=bar#qux=quux"},
+					"Cache-Control": []string{"no-store"},
+					"Pragma":        []string{"no-cache"},
+				}, header)
+			},
+		},
+		{
+			setup: func() {
+				redir, _ := url.Parse("https://foobar.com/?foo=bar")
+				ar.EXPECT().GetRedirectURI().Return(redir)
+				ar.EXPECT().GetResponseMode().Return(ResponseModeFragment)
+				resp.EXPECT().GetParameters().Return(url.Values{"state": {"{\"a\":\"b=c&d=e\"}"}})
+				resp.EXPECT().GetHeader().Return(http.Header{})
+
+				rw.EXPECT().Header().Return(header).Times(2)
+				rw.EXPECT().WriteHeader(http.StatusSeeOther)
+			},
+			expect: func() {
+				assert.Equal(t, http.Header{
+					"Location":      {"https://foobar.com/?foo=bar#state=%7B%22a%22%3A%22b%3Dc%26d%3De%22%7D"},
 					"Cache-Control": []string{"no-store"},
 					"Pragma":        []string{"no-cache"},
 				}, header)