paragonie / paseto

Platform-Agnostic Security Tokens
https://paseto.io
Other
3.23k stars 108 forks source link

Non-string claim values? #150

Closed spantaleev closed 2 years ago

spantaleev commented 2 years ago

Looks like the following methods expect that claim values would be string, at least according to the docblocks:

Looks like these claims are ultimately serialized using json_encode here: https://github.com/paragonie/paseto/blob/a0134f9501a132192136886bf85f8ce3368e1787/src/Builder.php#L635

.. and deserialized here: https://github.com/paragonie/paseto/blob/a0134f9501a132192136886bf85f8ce3368e1787/src/Parser.php#L455

I've managed to store an array value in a claim just fine, but according to these docblocks, it's somewhat of a coincidence that it works. Looking at how JsonToken::set() and JsonToken::with() avoid typehinting $value and how deserializing expects arbitrary depths, however, I suppose that non-string claims are indeed supported.

It'd be nice if this confusion can be clarified and static analyzers (like phpstan) can be appeased.

aidantwoods commented 2 years ago

Related conversation. AFAIK there is an intent that this is supported, there's even an example of a nested structure in the spec.

spantaleev commented 2 years ago

Great! If the spec mentions it, then that's encouraging.

Fixing it on the PHP side shouldn't be too difficult. We can just replace string with mixed and even typehint the non-typehinted $value arguments with mixed. I believe that typehinting with mixed is PHP 8.0 only, so if PHP 7.1 compatibility is important (I guess it is), then this part can be skipped.

aidantwoods commented 2 years ago

I think this probably isn't a good enough reason on its own to drop PHP 7 support, so switching to mixed in the docblock sounds most sensible 😀

Do you want to open a PR to add this? I'd imagine it should be quite a straightforward one to get merged (especially since the typehints already allow this, would just be a docblock update I think).