paragonie / paseto

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

Remove misleading implicit param from v1 and v2 #144

Closed aidantwoods closed 2 years ago

aidantwoods commented 2 years ago

This seems to have been added to aid unit testing though a generic protocol.

Given the known expected behaviour for "implicit", IMO allowing this param to be specified (but ignored) seems like a fairly dangerous footgun. For example, a user who has recently learnt about PASETO in terms of v3 and v4 may specify this parameter during the verification stage, expecting that it will be validated against the string they specified during encryption/signing. Possibly unbeknownst to them, this parameter will be ignored during both production and verification of a PASETO in v1 and v2 modes.

An alternative to these changes is to throw an exception if implicit is ever specified for v1 or v2. I have chosen to gate this param though an interface extension because it seems clearer to catch this through static analysis than at runtime.

I've also added a check to throw if the user attempts to set or read an implicit assertion with the Builder (since the exact protocol is specified upon Builder construction, so we know if that is a sane thing to be doing or not).

aidantwoods commented 2 years ago

I've also removed the method that allows checking if implicit is supported by the current version (since this can be accomplished through an interface check). This is a BC break, so possibly this should be added back so as to preserve that.

I've reverted this change so as not to break BC

aidantwoods commented 2 years ago

I think GitHub actions are only running on my branch because the trigger is just push here:

https://github.com/paragonie/paseto/blob/6e3ce7fb5dfb2da512f38877e0b6e987ab519d8b/.github/workflows/ci.yml#L1-L5

If you like I can update this to also run on pull-request?

paragonie-security commented 2 years ago

In a future version of the library, we are simply going to remove the Version1 / Version2 support.

aidantwoods commented 2 years ago

we are simply going to remove the Version1 / Version2 support.

That also works! 😄