SQL persister uses | to store scopes and audiences without any escaping

[mitar]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• This issue affects my Ory Cloud project.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Describe the bug

See here:

		Scopes:            strings.Join(r.GetRequestedScopes(), "|"),
		GrantedScope:      strings.Join(r.GetGrantedScopes(), "|"),
		GrantedAudience:   strings.Join(r.GetGrantedAudience(), "|"),
		RequestedAudience: strings.Join(r.GetRequestedAudience(), "|"),

| is not a reserved character in any way and could happen to be included inside scopes and audiences. This means that if any legitimate scope contains | character (e.g., user|mails|read), this will break. I have not seen this character be used like that in scopes (generally people use : or / or .) but still, I think it is unnecessary potential issue. If you want to use this custom encoding of a list (and not use JSON or something), then you have to escape | inside scopes and audiences.

On the other hand, (space) cannot happen to be in scopes or audiences, because it is a reserved character by the spec itself, so I would suggest that this is simple replaced with as delimiter.

Reproducing the bug

This was found during code review.

Relevant log output

No response

Relevant configuration

No response

Version

observed on current master branch

On which operating system are you observing this issue?

No response

In which environment are you deploying?

No response

Additional Context

No response

[aeneasr]

Good point and thank you for the thorough review! I'd like to add though that spaces are reserved (iirc) for scope but are not reserved in other parts. aud (a JWT claim) is StringOrURI which does not have a distinguished character set - it's a plain string:

StringOrURI
A JSON string value, with the additional requirement that while
arbitrary string values MAY be used, any value containing a ":"
character MUST be a URI [RFC3986]. StringOrURI values are
compared as case-sensitive strings with no transformations or
canonicalizations applied. (Source)

Thus, | should indeed be escaped. Changing delimiter is, IMO, not possible without tons and tons of work. As this has never been reported as a bug since the project's inception, I'd prefer to escape vs refactor as it doesn't seem that anyone is affected by this behaviour at the moment!

Contribs welcomed :)

[aeneasr]

@grantzvolsky we have changed this to a different method which now uses JSON for these types of arrays. The relevant type is this one: https://github.com/ory/x/blob/7112e2632340e269159c73b5ebde8149374aecea/sqlxx/types.go#L267

Maybe it makes sense as part of Hydra 2.0 to address this since we're changing the schema anyways?

[aeneasr]

I have code that helps you convert the piped version to a JSON array literal. I can show you when you take a look at this.

[grantzvolsky]

@aeneasr I'll keep this in mind and let you know when I need it. Thanks!