firebase / php-jwt

PHP package for JWT
BSD 3-Clause "New" or "Revised" License
9.3k stars 1.26k forks source link

Overriding typ header #530

Closed Tilogorn closed 7 months ago

Tilogorn commented 10 months ago

I know it has already been issued/discussed in https://github.com/firebase/php-jwt/issues/472 and https://github.com/firebase/php-jwt/issues/418 but got closed without result.

I need to change the typ header to JOSE, which is sort of a pre-defined value for it, see last paragraph of this RFC 7515 chapter. The "need" is just the result of a requirement by the interface I want to talk to, which verifies that the typ header of my JWT equals JOSE. I am not in a position to argue for or against the point of it.

I already use php-jwt on other parts of my application and love the interface and simplicity of it and I am now forced to integrate another JWT library just because you insist on JWT hardcoded (source).

Any chance you'll reconsider that decision?

bshaffer commented 7 months ago

Hello @Tilogorn ! Thanks for filing this feature request. Your issue is the first that actually details a valid use-case.

I would love to help you support this feature, now that I know it's preventing you from using this library. The problem is, I am not sure how we can support it at this point without breaking backwards compatibility.

Some potential ways to support it (just brainstorming)

  1. Set it as a static property
    Firebase\JWT\JWT::$typ = 'JOSE';
  2. Set it as another parameter to JWT::encode
    public static function encode(
        array $payload,
        $key,
        string $alg,
        string $keyId = null,
        array $head = null,
        string $typ = 'JWT',
    ): string {

    and this could be called by:

    $jwt = Firebase\JWT\JWT::encode(payload: $payload, key: $key, typ: 'JOSE');

    This works great since we already have alg, which is analogous to typ. It's not ideal that the $alg and $typ parameters are not in any sensical order.

  3. Reverse the order of $head and $header in the call to array_merge
    $header = \array_merge($header, $head);

    This would break BC in a case where $head contained a typ parameter (and it also it makes the $alg parameter silly), but that risk is small enough that it might be worth it.

I am personally leaning towards the third option. Any thoughts?

Tilogorn commented 7 months ago

Hi @bshaffer, I am pleased that you are aware of my concern.

I would also vote for the third option. The amount of devs who needed to change the typ header and were not able to do so should not face any issues. They had either been changing their client library as I did or changed the JWT validation on the server side to meet the current implementation requirements of Firebase\JWT – if there were able to modify that part (which I am not). The last one would only be a problem if they forgot to unset the $head['typ'] in their current call to JWT::encode and I think that case is so edgy that you can take the risk.

bshaffer commented 7 months ago

THis is released in v6.10.0. Please confirm that this resolves your issue!

Tilogorn commented 7 months ago

@bshaffer did not forget you and will def try it out and report back. I'll have to implement new features in Q1/24 for the same project, that's a good opportunity.