feat!: align claim validation to specification

[agubler]

BREAKING CHANGE
Enhances the claim validation to match the JWT specification by ensuring that claim values are strings and not arrays

Resolves #552

Pull Request Overview

This PR enhances claim validation in the JWT verifier to align with the specification by ensuring claim values are strings rather than arrays. The changes update the signature and behavior of the claim validation functions in src/verifier.js and adjust error message expectations in the tests.

• Updated validateClaimType to accept an additional parameter for array handling.
• Removed the separate validateClaimArrayValues function and streamlined claim type and value validation.
• Modified test cases to expect error messages indicating that the claim must be a string.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/verifier.js	Refactored claim type validation to better align with JWT specs.
test/verifier.spec.js	Updated test assertions to check for error messages matching string-only claim validation.

Comments suppressed due to low confidence (1)

src/verifier.js:144

• [nitpick] The parameter name 'arrayValue' may be ambiguous regarding its purpose. Consider renaming it to a more descriptive name such as 'isArrayClaim' to clarify that it represents a boolean flag.

function validateClaimType(values, claim, allowArray, arrayValue, type) {

[simoneb]

Even thought I didn't review the tests, I can't easily follow the logic change in the production code.

First of all, I can see that the validators in the existing code were already specifying whether array values were supported, and apparently we simply didn't use it for any purpose apart from crafting the error message. Would you agree with this?

Secondly, there is only one validator that does support an array: aud. I wonder if for that validator we should preserve the previous logic that used .some instead of .every

[agubler]

Thanks @simoneb

First of all, I can see that the validators in the existing code were already specifying whether array values were supported, and apparently we simply didn't use it for any purpose apart from crafting the error message. Would you agree with this?

Correct, the validator already accepted an array option which like you say was not used for anything other than crafting the error message. So the intent looks to have always been to actually validate the other claims to be only a string value but was not implemented.

Passing whether the original value is an array or not and using this combined with whether arrays are allowed (as specified in the validator) enables us to correctly validate the claim attributes.

Secondly, there is only one validator that does support an array: aud. I wonder if for that validator we should preserve the previous logic that used .some instead of .every

I did change the logic back to use .some as it was previously now that we are correctly validating the claim "type" before the value validation occurs in validateClaimValues

The diff isn't obvious but you can see that the logic is now back to use .some instead of .every here

[simoneb]

I'm taking this claim as an example of an opportunity I see to improve the tests, but these considerations apply to other claims as well, with the exception of aud, which supports arrays.

1. in this test it's in array format, with a single item. the test below does the same thing by providing an array with 2 items. do we really need that? I don't think so
2. do we have a test that checks that an error is thrown if the string value provided does not match either the expected string or one of the items of the expected array?
3. do we have a test that checks the complementary case of the above? meaning, the test succeeds if the value of the claim either matches the expected string or one of the items of the expected array?

Do we have thorough coverage for the aud claim?

[agubler]

@simoneb

in this test it's in array format, with a single item. the test below does the same thing by providing an array with 2 items. do we really need that? I don't think so

I have removed the duplicate test cases

do we have a test that checks that an error is thrown if the string value provided does not match either the expected string or one of the items of the expected array?

I believe we have test cases that cover both single and an array of allowed values, I have refactored the tests to generate the token that is tested for the verifier, this hopefully improves the clarity about what is being tested and also helps future us when making additional changes

do we have a test that checks the complementary case of the above? meaning, the test succeeds if the value of the claim either matches the expected string or one of the items of the expected array?

There should be a test for each claim to that checks the value against a single allowed value, checks against a claim value against an array of allowed values and also tests the regex support for allowed values

[simoneb]

there a bit more refactoring I would do here, because the array detection/conversion is a bit redundant and confusing. I appreciate that this logic is used in both the type validation and the value validation below, but let's at least give this arrayValue a better name, e.g. isArray, both here and in the argument names of the functions it's passed to.

[simoneb]

Apart from the comment about the minor refactoring, this LGTM

[agubler]

Apart from the comment about the minor refactoring, this LGTM

This should be updated now