Repository: ory/fosite (go)

Issue:
Fragment is not escaped properly when computing the authorize response redirect URL
### Preflight checklist

- [X] I could not find a solution in the existing issues, docs, nor discussions.
- [X] I agree to follow this project's [Code of Conduct](https://github.com/ory/fosite/blob/master/CODE_OF_CONDUCT.md).
- [X] I have read and am following this repository's [Contribution Guidelines](https://github.com/ory/fosite/blob/master/CONTRIBUTING.md).
- [ ] This issue affects my [Ory Cloud](https://www.ory.sh/) project.
- [ ] I have joined the [Ory Community Slack](https://slack.ory.sh).
- [ ] I am signed up to the [Ory Security Patch Newsletter](https://ory.us10.list-manage.com/subscribe?u=ffb1a878e4ec6c0ed312a3480&id=f605a41b53).

### Describe the bug

When writing the authorize response, different response modes are supported: form post, query and fragment. For the fragment mode, the URL values are joined together using `&` as separator between each pair of key/value and `=` for separator between the key and value. Although the [RFC3896](https://datatracker.ietf.org/doc/html/rfc3986) does not mandate to escape those characters in the fragment, it makes sense to do it because otherwise every value containing them will break the whole fragment. Before #509, the fragment was encoded and then appended plain to the final URL to avoid double-escaping issues. However, in that PR, a regression was introduced in `URLSetFragment` that makes the values of the fragment not be escaped. When `String()` is called subsequently on the `net.URL` object, Go strictly follows the aforementioned RFC and does not escape the fragment, leading to the issue.

### Reproducing the bug

```sh
$ git clone https://github.com/ory/fosite-example
$ go run main.go
$ curl -X POST --data-raw 'scopes=openid&scopes=offline&username=peter' -i 'http://localhost:3846/oauth2/auth?client_id=my-client&redirect_uri=http%3A%2F%2Flocalhost%3A3846%2Fcallback&response_type=id_token%20code&scope=photos+openid+offline&state=%7B%22foo%22%3A%22bar%3Dbaz%26baz%3Dfoo%22%7D&nonce=some-random-nonce&code_challenge=KcstGto0NlamBHCw_hEABNx5hiFaUX5qDhEYos8Mk0M&code_challenge_method=S256'
```

If you take a look at the `Location` header returned in the response, you will see that it contains a fragment with the `state` parameter which is not properly escaped

### Relevant log output

_No response_

### Relevant configuration

_No response_

### Version

0.42.0

### On which operating system are you observing this issue?

_No response_

### In which environment are you deploying?

_No response_

### Additional Context

This bug is actually affecting Hydra starting from version `1.9` and changes how the fragment was escaped up to version `1.8.5`

Pull request description:
fix: url-encode the fragment of the redirect URL of the authorize response
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

## Related Issue or Design Document

Fixes #648

## Checklist

- [x] I have read the [contributing guidelines](../blob/master/CONTRIBUTING.md)
      and signed the CLA.
- [x] I have referenced an issue containing the design document if my change
      introduces a new feature.
- [x] I have read the [security policy](../security/policy).
- [x] 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
      [security@ory.sh](mailto:security@ory.sh)) from the maintainers to push
      the changes.
- [x] I have added tests that prove my fix is effective or that my feature
      works.
- [x] I have added necessary documentation within the code base (if
      appropriate).

## Further comments

These changes reflect on Ory Hydra, partially restoring the behaviour related to the fragment encoding to the one that was before version `1.9`. The OAuth specs mandate the fragment to be encoded using the `application/x-www-form-urlencoded` format, which is slightly different from the usual query string format: in fact, the spaces are not encoded as `%20` but rather as a `+`. For this reason, decoding the value in Javascript must first replace all `+` signs with spaces and only then call `decodeURIComponent()`. Since before the spaces were encoded wrongly, this path strictly speaking is still breaking the frontends. Let me know if we want to completely restore the logic or if the current implementation is fine as-is: in that case, a note should probably be added to the CHANGELOG

Patch:
diff --git a/authorize_helper.go b/authorize_helper.go
index 9eadbb6..13383e8 100644
--- a/authorize_helper.go
+++ b/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 {
diff --git a/authorize_write.go b/authorize_write.go
index d12a714..6ff75dc 100644
--- a/authorize_write.go
+++ b/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) {


Test patch:
diff --git a/authorize_write_test.go b/authorize_write_test.go
index 7500cbd..09212fa 100644
--- a/authorize_write_test.go
+++ b/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)