Repository: ory/fosite (go)

Issue:
audience parameter splitting by space
**Is your feature request related to a problem? Please describe.**

Currently multiple audience values are obtained by:

```
audience := stringsx.Splitx(request.Form.Get("audience"), " ")
```

I think audiences are generally URIs, so I find it surprising that they are simply passed as a space-delimited string. I can understand that spec is like that for `scope` and `grant_type`, but I think there is no spec for `audience` and this is a custom parameter?

The issue here is that what does one do if URI contains a query parameter with a space? I know this might not be realistic, but just makes me think. So I do not have here a real problem I encountered, except that I simply assumed I have to list multiple audiences multiple times, and not with a space in between.

**Describe the solution you'd like**

I think fosite should support also passing audience as multiple parameters. golang already parses this correctly and you can obtain an array of them through `request.Form["audience"]`. Because I think space is rare in the URI we could keep that for backwards compatibility and simply process all values for the `audience` query parameter and split them by space as well.

I also think it should be documented somewhere how you pass this parameter. I have not found it documented anywhere. And the fact that it should be space separated (and if this issue is resolved, also that you can pass it by repeating the parameter).

**Describe alternatives you've considered**

We can leave it as is, but then we should at least document it better.

Pull request:
feat: Support passing repeated audience parameter
## Related issue

Fixes #504.

## Proposed changes

Added `GetAudiences` helper function which tries to have current behavior and also support multiple/repeated audience parameters. If there are parameter is repeated, then it is not split by space. If there is only one then it is split by space. I think this is the best balance between standard/backwards behavior and allowing repeated parameter and allowing also URIs/audiences with spaces in them (which we probably all agree is probably not something anyone should be doing).

Also added `ExactAudienceMatchingStrategy` which is slightly more suitable to use for audiences which are not URIs. In [OIDC spec](https://openid.net/specs/openid-connect-core-1_0.html) audience is described as:

> Audience(s) that this ID Token is intended for. It MUST contain the OAuth 2.0 client_id of the Relying Party as an audience value. It MAY also contain identifiers for other audiences. In the general case, the aud value is an array of case sensitive strings. In the common special case when there is one audience, the aud value MAY be a single case sensitive string. 

`client_id` is generally not an URI, but some UUID or some other random string.

## Checklist

- [x] I have read the [contributing guidelines](../blob/master/CONTRIBUTING.md) and signed the CLA.
- [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).


Patch:
diff --git a/access_request_handler.go b/access_request_handler.go
index a843c22..6e6ceec 100644
--- a/access_request_handler.go
+++ b/access_request_handler.go
@@ -72,7 +72,7 @@ func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session
 	}
 
 	accessRequest.SetRequestedScopes(RemoveEmpty(strings.Split(r.PostForm.Get("scope"), " ")))
-	accessRequest.SetRequestedAudience(RemoveEmpty(strings.Split(r.PostForm.Get("audience"), " ")))
+	accessRequest.SetRequestedAudience(GetAudiences(r.PostForm))
 	accessRequest.GrantTypes = RemoveEmpty(strings.Split(r.PostForm.Get("grant_type"), " "))
 	if len(accessRequest.GrantTypes) < 1 {
 		return accessRequest, errors.WithStack(ErrInvalidRequest.WithHint(`Request parameter "grant_type"" is missing`))
diff --git a/audience_strategy.go b/audience_strategy.go
index 0386970..ada242d 100644
--- a/audience_strategy.go
+++ b/audience_strategy.go
@@ -46,8 +46,50 @@ func DefaultAudienceMatchingStrategy(haystack []string, needle []string) error {
 	return nil
 }
 
+// ExactAudienceMatchingStrategy does not assume that audiences are URIs, but compares strings as-is and
+// does matching with exact string comparison. It requires that all strings in "needle" are present in
+// "haystack". Use this strategy when your audience values are not URIs (e.g., you use client IDs for
+// audience and they are UUIDs or random strings).
+func ExactAudienceMatchingStrategy(haystack []string, needle []string) error {
+	if len(needle) == 0 {
+		return nil
+	}
+
+	for _, n := range needle {
+		var found bool
+		for _, h := range haystack {
+			if n == h {
+				found = true
+			}
+		}
+
+		if !found {
+			return errors.WithStack(ErrInvalidRequest.WithHintf(`Requested audience "%s" has not been whitelisted by the OAuth 2.0 Client.`, n))
+		}
+	}
+
+	return nil
+}
+
+// GetAudiences allows audiences to be provided as repeated "audience" form parameter,
+// or as a space-delimited "audience" form parameter if it is not repeated.
+// RFC 8693 in section 2.1 specifies that multiple audience values should be multiple
+// query parameters, while RFC 6749 says that that request parameter must not be included
+// more than once (and thus why we use space-delimited value). This function tries to satisfy both.
+// If "audience" form parameter is repeated, we do not split the value by space.
+func GetAudiences(form url.Values) []string {
+	audiences := form["audience"]
+	if len(audiences) > 1 {
+		return RemoveEmpty(audiences)
+	} else if len(audiences) == 1 {
+		return RemoveEmpty(strings.Split(audiences[0], " "))
+	} else {
+		return []string{}
+	}
+}
+
 func (f *Fosite) validateAuthorizeAudience(r *http.Request, request *AuthorizeRequest) error {
-	audience := RemoveEmpty(strings.Split(request.Form.Get("audience"), " "))
+	audience := GetAudiences(request.Form)
 
 	if err := f.AudienceMatchingStrategy(request.Client.GetAudience(), audience); err != nil {
 		return err
diff --git a/handler/openid/flow_refresh_token.go b/handler/openid/flow_refresh_token.go
index 1f9ef0e..5ea20d1 100644
--- a/handler/openid/flow_refresh_token.go
+++ b/handler/openid/flow_refresh_token.go
@@ -62,6 +62,9 @@ func (c *OpenIDConnectRefreshHandler) HandleTokenEndpointRequest(ctx context.Con
 	// We need to reset the expires at value
 	sess.IDTokenClaims().ExpiresAt = time.Time{}
 	sess.IDTokenClaims().Nonce = ""
+	sess.IDTokenClaims().JTI = ""
+	sess.IDTokenClaims().AccessTokenHash = ""
+	sess.IDTokenClaims().CodeHash = ""
 	return nil
 }
 


Test patch:
diff --git a/audience_strategy_test.go b/audience_strategy_test.go
index 87d7d0a..8f84128 100644
--- a/audience_strategy_test.go
+++ b/audience_strategy_test.go
@@ -83,6 +83,41 @@ func TestDefaultAudienceMatchingStrategy(t *testing.T) {
 			n:   []string{"https://cloud.ory.xyz/api/users"},
 			err: true,
 		},
+		{
+			h:   []string{"foobar"},
+			n:   []string{"foobar"},
+			err: false,
+		},
+		{
+			h:   []string{"foo bar"},
+			n:   []string{"foo bar"},
+			err: false,
+		},
+		{
+			h:   []string{"foobar"},
+			n:   []string{"foobar"},
+			err: false,
+		},
+		{
+			h:   []string{"zoo", "bar"},
+			n:   []string{"zoo"},
+			err: false,
+		},
+		{
+			h:   []string{"zoo"},
+			n:   []string{"zoo", "bar"},
+			err: true,
+		},
+		{
+			h:   []string{"foobar"},
+			n:   []string{"foobar/"},
+			err: false,
+		},
+		{
+			h:   []string{"foobar/"},
+			n:   []string{"foobar"},
+			err: false,
+		},
 	} {
 		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
 			err := DefaultAudienceMatchingStrategy(tc.h, tc.n)
@@ -94,3 +129,126 @@ func TestDefaultAudienceMatchingStrategy(t *testing.T) {
 		})
 	}
 }
+
+func TestExactAudienceMatchingStrategy(t *testing.T) {
+	for k, tc := range []struct {
+		h   []string
+		n   []string
+		err bool
+	}{
+		{
+			h:   []string{},
+			n:   []string{},
+			err: false,
+		},
+		{
+			h:   []string{"http://foo/bar"},
+			n:   []string{},
+			err: false,
+		},
+		{
+			h:   []string{},
+			n:   []string{"http://foo/bar"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.sh/api/users"},
+			err: false,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.sh/api/users/"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users/"},
+			n:   []string{"https://cloud.ory.sh/api/users/"},
+			err: false,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users/"},
+			n:   []string{"https://cloud.ory.sh/api/users"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.sh/api/users/1234"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.sh/api/users", "https://cloud.ory.sh/api/users/", "https://cloud.ory.sh/api/users/1234"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users", "https://cloud.ory.sh/api/tenants"},
+			n:   []string{"https://cloud.ory.sh/api/users", "https://cloud.ory.sh/api/users/", "https://cloud.ory.sh/api/users/1234", "https://cloud.ory.sh/api/tenants"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.sh/api/users1234"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"http://cloud.ory.sh/api/users"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.sh:8000/api/users"},
+			err: true,
+		},
+		{
+			h:   []string{"https://cloud.ory.sh/api/users"},
+			n:   []string{"https://cloud.ory.xyz/api/users"},
+			err: true,
+		},
+		{
+			h:   []string{"foobar"},
+			n:   []string{"foobar"},
+			err: false,
+		},
+		{
+			h:   []string{"foo bar"},
+			n:   []string{"foo bar"},
+			err: false,
+		},
+		{
+			h:   []string{"foobar"},
+			n:   []string{"foobar"},
+			err: false,
+		},
+		{
+			h:   []string{"zoo", "bar"},
+			n:   []string{"zoo"},
+			err: false,
+		},
+		{
+			h:   []string{"zoo"},
+			n:   []string{"zoo", "bar"},
+			err: true,
+		},
+		{
+			h:   []string{"foobar"},
+			n:   []string{"foobar/"},
+			err: true,
+		},
+		{
+			h:   []string{"foobar/"},
+			n:   []string{"foobar"},
+			err: true,
+		},
+	} {
+		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
+			err := ExactAudienceMatchingStrategy(tc.h, tc.n)
+			if tc.err {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
diff --git a/authorize_request_handler_test.go b/authorize_request_handler_test.go
index 86afaa6..12e0abe 100644
--- a/authorize_request_handler_test.go
+++ b/authorize_request_handler_test.go
@@ -227,6 +227,76 @@ func TestNewAuthorizeRequest(t *testing.T) {
 				},
 			},
 		},
+		/* repeated audience parameter */
+		{
+			desc: "repeated audience parameter",
+			conf: &Fosite{Store: store, ScopeStrategy: ExactScopeStrategy, AudienceMatchingStrategy: DefaultAudienceMatchingStrategy},
+			query: url.Values{
+				"redirect_uri":  {"https://foo.bar/cb"},
+				"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{"https://foo.bar/cb"},
+					Scopes:        []string{"foo", "bar"},
+					Audience:      []string{"https://cloud.ory.sh/api", "https://www.ory.sh/api"},
+				}, nil)
+			},
+			expect: &AuthorizeRequest{
+				RedirectURI:   redir,
+				ResponseTypes: []string{"code", "token"},
+				State:         "strong-state",
+				Request: Request{
+					Client: &DefaultClient{
+						ResponseTypes: []string{"code token"}, RedirectURIs: []string{"https://foo.bar/cb"},
+						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"},
+				},
+			},
+		},
+		/* repeated audience parameter with tricky values */
+		{
+			desc: "repeated audience parameter with tricky values",
+			conf: &Fosite{Store: store, ScopeStrategy: ExactScopeStrategy, AudienceMatchingStrategy: ExactAudienceMatchingStrategy},
+			query: url.Values{
+				"redirect_uri":  {"https://foo.bar/cb"},
+				"client_id":     {"1234"},
+				"response_type": {"code token"},
+				"state":         {"strong-state"},
+				"scope":         {"foo bar"},
+				"audience":      {"test value", ""},
+			},
+			mock: func() {
+				store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{
+					ResponseTypes: []string{"code token"},
+					RedirectURIs:  []string{"https://foo.bar/cb"},
+					Scopes:        []string{"foo", "bar"},
+					Audience:      []string{"test value"},
+				}, nil)
+			},
+			expect: &AuthorizeRequest{
+				RedirectURI:   redir,
+				ResponseTypes: []string{"code", "token"},
+				State:         "strong-state",
+				Request: Request{
+					Client: &DefaultClient{
+						ResponseTypes: []string{"code token"}, RedirectURIs: []string{"https://foo.bar/cb"},
+						Scopes:   []string{"foo", "bar"},
+						Audience: []string{"test value"},
+					},
+					RequestedScope:    []string{"foo", "bar"},
+					RequestedAudience: []string{"test value"},
+				},
+			},
+		},
 		/* redirect_uri with special character in protocol*/
 		{
 			desc: "redirect_uri with special character",