fix: Throw exception on invalid base64#576
Conversation
Fixed JWT::urlsafeB64Decode not actually throwing an InvalidArgumentException on invalid base64 characters, despite doc comment saying that it does.
|
It looks like it currently throws an As this would be a breaking change, wouldn't it be better to just reflect the doc comment to match what's happening, rather than alter the existing behavior? |
|
The current behavior of In regards to the changing I do in I would had had |
Last change still allowed input that was invalid base64URL but valid base64
|
I have added tests to demonstrate the error I described However, while working on that I realized that my solution would still allow invalid base64URL through, as long as it is valid base64. I have also fixed that now. |
bshaffer
left a comment
bshaffer
left a comment
There was a problem hiding this comment.
I don't think this should be considered a breaking change
Unfortunately, I think this is still a breaking change. I wish we had merged this for the 7.0 release, because it's a good change. But I don't feel comfortable validating these errors differently without protecting them behind a new major version.
| try { | ||
| $headerRaw = static::urlsafeB64Decode($headb64); | ||
| } catch (InvalidArgumentException $e) { | ||
| throw new UnexpectedValueException('Unable to decode header'); |
There was a problem hiding this comment.
every other validation error in decode uses that exception
This method also can throw InvalidArgumentException, although you're right the majority of validation in this method throws UnexpectedValueException
|
|
||
| public function testMalformedBase64Url() | ||
| { | ||
| $this->expectException(InvalidArgumentException::class); |
There was a problem hiding this comment.
Can we add checking the exception message as well using expectExceptionMessage? Same with the test below
2 participants
Fixed JWT::urlsafeB64Decode not actually throwing an InvalidArgumentException on invalid base64 characters, despite doc comment saying that it does.