_verify_claims: Refactor iss, sub, aud, and jti checks

[clayne]

• In the latest version of Crypt::JWT, I had a dev get caught out on setting verify_iss => undef and verify_aud => undef in a call to decode_jwt. This is a problem because the documentation specifically says the default value of these args is undef, e.g.:

  verify_aud
      INCOMPATIBLE CHANGE in 0.024: If "verify_aud" is
      specified and claim "aud" (Audience) is completely
      missing it is a failure since 0.024
  
      "CODE ref" - subroutine (with 'aud' claim value
      passed as argument) should return "true" otherwise
      verification fails
  
      "Regexp ref" - 'aud' claim value has to match given
      regexp otherwise verification fails
  
      "undef" (default) - do not verify 'aud' claim

  In reality the code was checking for the existence of the argument rather than if it were defined or not and hence failed on the last conditional (JWT: <x> must be Regexp or CODE).

  Rather than target that specifically, I've taken the liberty of compressing all of these similar checks into a single loop which does the same thing being done before, but without the repetition (DRY!), and proper handling of undef args.

[clayne]

clayne@i860:~/git/perl-Crypt-JWT (verify-undef-value-fix *) $ prove -rblj8 t
t/compile.t ............ ok                  
t/jws_no_key.t ......... ok        
t/flattened.t .......... ok
t/kw_ecdh.t ............ ok
t/kw_gcm.t ............. ok
t/kw_pbes2.t ........... ok                               
t/kw_ecdhaes.t ......... ok                     
t/kw_aes.t ............. ok           
t/kw_rsa.t ............. ok 
t/jwt_decode_tv.t ...... ok
t/jwt_params.t ......... ok
t/jwt_encode_decode.t .. ok     
All tests successful.
Files=12, Tests=572,  1 wallclock secs ( 0.11 usr  0.03 sys +  2.97 cusr  0.26 csys =  3.37 CPU)
Result: PASS

[karel-m]

I agree that the original if (exists $args{verify_iss}) ... should use defined instead of exists. However, the if (exists $payload->{iss}) ... should stay with exists.

I'd prefer something like this:

  foreach my $claim (qw(iss sub aud jti)) {
    my $check = $args{"verify_$claim"};
    next unless (defined $check);

    if (exists $payload->{$claim}) {
      if (ref $check eq 'Regexp') {
        croak "JWT: $claim claim re check failed" unless $payload->{$claim} =~ $check;
      }
      elsif (ref $check eq 'CODE') {
        croak "JWT: $claim claim check failed" unless $check->($payload->{$claim});
      }
      else {
        croak "JWT: verify_$claim must be Regexp or CODE";
      }
    }
    else {
      croak "JWT: $claim claim required but missing"
    }
  }

[clayne]

I have no problem changing it to that style but there still is a problem with not checking the definedness of $payload->{$claim} and that's that if it's undef the regex comparison is going to bomb out with a perl runtime warning (which is always bad):

$ perl -we 'undef =~ /foo/'
Use of uninitialized value in pattern match (m//) at -e line 1.

In what cases would $payload->{$claim} = undef be considered a valid claim?

[karel-m]

What about:

  foreach my $claim (qw(iss sub aud jti)) {
    my $check = $args{"verify_$claim"};
    next unless (defined $check);

    if (exists $payload->{$claim}) {
      if (ref $check eq 'Regexp') {
        my $value = $payload->{$claim};
        $value = "" if !defined $value;
        croak "JWT: $claim claim re check failed" unless $value =~ $check;
      }
      elsif (ref $check eq 'CODE') {
        croak "JWT: $claim claim check failed" unless $check->($payload->{$claim});
      }
      else {
        croak "JWT: verify_$claim must be Regexp or CODE";
      }
    }
    else {
      croak "JWT: $claim claim required but missing"
    }
  }

[clayne]

Well, could just as simply do that with my $value = $payload->{$claim} || '' or even ($payload->{$claim} || '') =~ $check but I think we should consider the fundamental question of what we're trying to avoid by allowing undef values to still be considered "present claims." Obviously in perl exists != defined, but conceptually in use I struggle to see why that should be preserved for the actual claim. What is actually wrong with ignoring it altogether if it's undef?

[karel-m]

my $value = $payload->{$claim} || '' is wrong as it turns '0' into ''

my $value = $payload->{$claim} // '' would do the job but requires perl 5.10+

Of course using the claim value undef does not make much sense, but I can imagine that we somehow got e.g. "iss": null (it might be a malicious attempt, an error, an intent or whatever).

If there is the claim (therefore exists check) we have to pass it for verification (either to callback or regex match).

[clayne]

Yep, you're completely right about the 0 vs '' case, had a brain slip here.

I've incorporated your version of the inner loop, added 2 unit tests for both undef values or undef args, and rebased.

[karel-m]

Looks good. Thank you for your time.