Handle when aud OIDC claim is an Array

[liamdiprose]

The aud claim of OIDC id_tokens can be an array but the existing logic incorrectly assumes aud is always a string.

This PR adds the necessary check.

Signed-off-by: Liam Diprose [email protected]

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[liamdiprose]

I have a use case where the aud contains all the applications the token is authenticated with. I trust all of them but a unit test seems to be checking that clientId is alone:

matrix-js-sdk/spec/unit/oidc/validate.spec.ts

Lines 164 to 171 in 8155b0a

	it("should throw when audience includes clientId and other audiences", () => {
	mocked(jwtDecode).mockReturnValue({
	...validDecodedIdToken,
	aud: `${clientId},uiop,asdf`,
	});
	expect(() => validateIdToken(idToken, issuer, clientId, nonce)).toThrow(new Error(OidcError.InvalidIdToken));
	expect(logger.error).toHaveBeenCalledWith("Invalid ID token", new Error("Invalid audience"));
	});

Can we simply trust a token if the client is in the aud?

[dbkr]

Yeah, I noticed the unit test above which looks wrong as far as I can see. Well... actually strictly speaking I think it's correct because I don't think a comma-separated string means multiple auds, as far as I can see... it would just be a different string, but that's not what the name of the test implies its trying to test.

So I'd be open to fixing that. If so, I'd probably suggest sanitising into an array with something like const sanitisedAuds = typeof claims.aud === 'string' : [claims.aud] : claims.aud and then doing the includes, because I find the logic there quite hard to reason about. I'd probably also prefer tests asserting it actually does the right thing with an array, rather than just not throwing. Wdyt?

[liamdiprose]

Well... actually strictly speaking I think it's correct because I don't think a comma-separated string means multiple auds, as far as I can see... it would just be a different string, but that's not what the name of the test implies its trying to test.

This is my understanding as well.

I've added the check for an array that doesn't contain clientId. I think that covers all the added code.