Tighten string to non-empty-string where possible
#936
Conversation
| } catch (JsonException $exception) { | ||
| throw CannotEncodeContent::jsonIssues($exception); | ||
| } | ||
|
|
||
| assert($jsonEncoded !== ''); |
There was a problem hiding this comment.
Hmm, I would expect psalm and phpstan to have better type inference here 🤔
| @@ -12,6 +12,8 @@ interface Parser | |||
| /** | |||
| * Parses the JWT and returns a token | |||
| * | |||
| * @param non-empty-string $jwt | |||
There was a problem hiding this comment.
I think a parser should accept an empty string here, with CannotDecodeContent as an outcome 🤔
There was a problem hiding this comment.
An empty string JWT is always an error, just like a boolean would be, that's why there's a string native type ATM.
Why not preventing also empty string at all?
There was a problem hiding this comment.
Need to think about it, but I see a CannotDecodeContent exception as quite valuable on an empty string scenario here: forcing further verification on the client side seems wasteful, at this stage.
I agree that a string&empty will always result in a crash here: just wondering whether forcing an additional upfront assertion is added value.
In the following (valid) code:
function foo(string $token) {
Assert::notEmpty($token);
$parsed = $this->parser->parse($token);
// ...If a CannotDecodeContent is thrown (explaining that the content was empty) is the same value of AssertionFailed, except with an additional piece of code for it.
I'm just trying to figure out the ergonomics of the library here:
- correctness leads to
string&empty -> never - usability leads to
non-empty-string|(string&empty) -> Token|Exception
There was a problem hiding this comment.
Now I understand better your doubts.
I can only say the sooner the type is safe, the better:
- if the library allows an empty string, my whole app may allow it
- if the library denies an empty string, I have to design the app upfront to get rid of it. This basically means input validation before consumption
There was a problem hiding this comment.
There is one good thing coming from this new type restriction: the existence of the token must be verified by the consumer.
Anyway, I'm just presenting the options: I think that the parser (being a parser) is responsible for upcasting what comes in (or throwing), so it's our decision to decide how wide the API is supposed to be.
The parser makes the type safe anyway, so I have doubts on the value of an early assertion, in this specific case.
There was a problem hiding this comment.
That said, we can experiment with this, and then later-on relax the type constraint, if we consider it excessive.
| @@ -29,6 +32,8 @@ final public function sign(string $payload, Key $key): string | |||
|
|
|||
| final public function verify(string $expected, string $payload, Key $key): bool | |||
| { | |||
| assert($expected !== ''); | |||
There was a problem hiding this comment.
Wait, is the signature not supposed to be non-empty upfront?
There was a problem hiding this comment.
Not until we allow \Lcobucci\JWT\Signer\None to exist.
\Lcobucci\JWT\Signer::verify API must take into account all available signers.
| @@ -41,7 +41,7 @@ public function parseMustRaiseExceptionWhenTokenDoesNotHaveThreeParts(): void | |||
| $this->expectException(InvalidTokenStructure::class); | |||
| $this->expectExceptionMessage('The JWT string must have two dots'); | |||
|
|
|||
| $parser->parse(''); | |||
| $parser->parse('.'); | |||
There was a problem hiding this comment.
IMO OK to parse an empty string and get a crash here.
Slamdunk
force-pushed
the
non_empty_string
branch
from
938ed73 to
d6e5967
Compare
|
Closed in favor of #939 |
No description provided.