Fix/sl-verification

[cre8]

Fixes #331

• throws an error when status list validation fails. If a custom error message should be thrown, the verify funcation has to be overriden
  Signed-off-by: Mirko Mollik [email protected]

[cre8]

@charsleysa please have a look if this approach is fine for you

[charsleysa]

This code is redundant currently because of the slJwt.verify call
https://github.com/openwallet-foundation/sd-jwt-js/pull/332/files#diff-4ee231c9aa73a11f113ae88bb99461dabec3cb2507f6451cd3a127486c83769cR309-R314

That calls into the JWT validation code which already has all those checks
https://github.com/cre8/sd-jwt-js/blob/4e9ca991e82eff38414f63341e6201293738f116/packages/core/src/jwt.ts#L149-L182

[cre8]

Oh you are right (no work without coffee in the morning), I removed also the redundant exp check.

Instead of the first fix, I appended all error messages during the verify function so it is clear when the sl jwt fails, it is clear from the error message.

Thank you for reviewing!!!

[charsleysa]

That would work. Though one downside is that it would suppress custom errors (you wouldn't be able to use instanceof checks).

Maybe it's better for the status list not to use the jwt verify at all and instead have it's own independent implementation.

[cre8]

That would work. Though one downside is that it would suppress custom errors (you wouldn't be able to use instanceof checks).

Maybe it's better for the status list not to use the jwt verify at all and instead have it's own independent implementation.

this would end up in code duplication that I want to avoid. We may define a new Exception type like SLException?

[charsleysa]

this would end up in code duplication that I want to avoid. We may define a new Exception type like SLException?

That would still result in the suppression of custom errors from the user supplied verifier function. Though it does help in identifying exactly status list specific errors.

My thinking is the best way to avoid duplication would be to have check helper functions which return boolean results which can then be used from the status list verify function and it is the responsibility of the status list verify function to throw the errors.

[cre8]

this would end up in code duplication that I want to avoid. We may define a new Exception type like SLException?

That would still result in the suppression of custom errors from the user supplied verifier function. Though it does help in identifying exactly status list specific errors.

My thinking is the best way to avoid duplication would be to have check helper functions which return boolean results which can then be used from the status list verify function and it is the responsibility of the status list verify function to throw the errors.

With:

        await slJWT
          .verify(
            this.userConfig.statusVerifier ??
              (this.userConfig.verifier as Verifier),
            options,
          )
          .catch((err) => {
            throw new SLException(
              `Status List JWT verification failed: ${err.message}`, err.details
            );
          });
          ```
I would not say it is supressed, but appended. I agree that the type of the error gets overwritten, but the error message gets passed as defined in your custom statusVerifier or verifier

Eg. with your verifier:

```ts
verifier(data: string, sig: string) {
  throw Error("it's always false");
}

Will result in an error message like Status List JWT verification failed: it's always false

[charsleysa]

That's correct, though if you throw a custom error class any other custom features would be lost too.

For example, if you had an ErrorWithKeyDetails class where the keyDetails are not included in the message, those details would be lost.

I'm not entirely opposed to the proposed solution, however I think it's important to document that only the message will ever be propagated and instance checks won't work.

[lukasjhan]

Looks good to me, Thanks.
Could you fix the lint error plz?

[cre8]

That's correct, though if you throw a custom error class any other custom features would be lost too.

For example, if you had an ErrorWithKeyDetails class where the keyDetails are not included in the message, those details would be lost.

I'm not entirely opposed to the proposed solution, however I think it's important to document that only the message will ever be propagated and instance checks won't work.

the sdjwtexception and slexception allow to pass a message and also an object. This object is passed so no information except the type of exception is lost

[lukasjhan]

Thank you

[charsleysa]

the sdjwtexception and slexception allow to pass a message and also an object. This object is passed so no information except the type of exception is lost

Ah, so the expectation is for users to throw SDJWTException or SLException instead of their own error class so that information can be correctly propagated?

[cre8]

the sdjwtexception and slexception allow to pass a message and also an object. This object is passed so no information except the type of exception is lost

Ah, so the expectation is for users to throw SDJWTException or SLException instead of their own error class so that information can be correctly propagated?

yes, we should add this to the documentation. Since the own function that can be inserted are quite small, it should not be a problem to have the tradeoff to not be able to "throw up" your own exception class, as long as all relevant informations are passed. But I am open for an update once someone struggles there

[charsleysa]

Looks good!

[TimoGlastra]

I think casting this as an SLException is unsafe, since the method could throw other errors as well (since a custom statusVerifier can be passed).

[TimoGlastra]

I think it should rather be implemented as if (err instance SLException) {} else {}

[cre8]

I made a suggestion with a fix here

@TimoGlastra can you please verify it if this was your intention? I agree that we should give the flexibility of throwing own exceptions.