Claims validation should not mutate incoming requiredClaims array

[nazrhyn]

What happened?

jose/src/lib/jwt_claims_set.ts

Lines 56 to 61 in b7b1e3a

	if (maxTokenAge !== undefined) requiredClaims.push('iat')
	if (audience !== undefined) requiredClaims.push('aud')
	if (subject !== undefined) requiredClaims.push('sub')
	if (issuer !== undefined) requiredClaims.push('iss')
	for (const claim of new Set(requiredClaims.reverse())) {

requiredClaims is mutated via the push calls and by reverse, which happens in place. If calling code maintains a reference to this array and passes it in to multiple calls, it could grow over time.

A shallow copy would probably be sufficient to avoid this, or the code could just construct the set earlier.

const { requiredClaims = [], issuer, subject, audience, maxTokenAge } = options
const uniqueRequiredClaims = new Set(requiredClaims)

if (maxTokenAge !== undefined) uniqueRequiredClaims.add('iat')
if (audience !== undefined) uniqueRequiredClaims.add('aud')
if (subject !== undefined) uniqueRequiredClaims.add('sub')
if (issuer !== undefined) uniqueRequiredClaims.add('iss')

To be honest, I'm not 100% sure of the purpose of the reverse call.

Version

v5.1.1

Runtime

Node.js

Runtime Details

v18.18.0

Code to reproduce

/*
I wanted to write a runnable script here, but there's a lot of stuff I'd need to include
before I could call `jwtVerify`, so this is just a simple example.
*/

const REQUIRED_CLAIMS = ['foo'];

// ... later

// The size of the REQUIRED_CLAIMS array will grow with every call to `jwtVerify`,
//  as it would continue to push the value 'aud'.
await jwtVerify(token, publicKey, {
  audience: 'urn:something',
  requiredClaims: REQUIRED_CLAIMS,
});

Required

• I have searched the issues tracker and discussions for similar topics and couldn't find anything related.
• I agree to follow this project's Code of Conduct

[panva]

good catch!

[nazrhyn]

I could make a PR, though I'd have to figure out how your tests are organized so I could add one that verifies the change.

[nazrhyn]

LOL, yeah. That's why I didn't do that from the start. I figured you'd be able to do it in 1/10 of the time 🤓.

[nazrhyn]

That said, I'm curious about the reverse. Do you remember why?

[panva]

So that the claims that are implied required from the other options come first. It could've been a side effect of the bug that I didn't spot and my test suite made me believe it's all good then. I guess I could also just unshift the array now. If you want to make a refactor PR that's okay

[nazrhyn]

Ahhh, that makes sense. Yeah I'm writing some unit tests around this and I had to do something like this because it didn't seem like a good idea to rely on order, even though this is less precise.

    test.each([
      { jwt: {}, options: { aud: null, iat: null, exp: null }, missing: ['aud', 'iat', 'exp', 'deployment', 'deployment_hostname' ] },
      { jwt: {}, options: { iat: null, exp: null }, missing: [ 'iat', 'exp', 'deployment', 'deployment_hostname' ] },
      { jwt: {}, options: { exp: null }, missing: [ 'exp', 'deployment', 'deployment_hostname' ] },
      { jwt: {}, options: {}, missing: [ 'deployment', 'deployment_hostname' ] },
      { jwt: { deployment: 'foo' }, options: {}, missing: [ 'deployment_hostname' ] },
    ])('fails when the required claims are not present ($missing)', async ({ jwt, options, missing }) => {
      await request(wrapper)
        .post('/')
        .set('Authorization', `Bearer ${await signJwt(jwt, options)}`);

      const error = expectUnauthorizedError(errors, UnauthorizedErrorReason.JWT_VERIFY_FAILED);
      const joseError = expectJoseError(error.originalError, JWTClaimValidationFailed);
      expect(missing).toContain(joseError.claim);
    });

I guess a refactor could make it more deterministic, always validating implied first then incoming or whatever. That said, it's probably fine.