Skip to content

fix: Throw exception on invalid base64 #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Oinkling
Copy link

@Oinkling Oinkling commented Aug 8, 2024

Fixed JWT::urlsafeB64Decode not actually throwing an InvalidArgumentException on invalid base64 characters, despite doc comment saying that it does.

Fixed JWT::urlsafeB64Decode not actually throwing an
InvalidArgumentException on invalid base64 characters, despite doc
comment saying that it does.
@bshaffer
Copy link
Collaborator

bshaffer commented Apr 9, 2025

It looks like it currently throws an InvalidArgumentException but you want it to throw a UnexpectedValueException?

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?

@Oinkling
Copy link
Author

The current behavior of urlsafeB64Decode is to call base64_decode using the default value (false) for the $strict parameter.
This will result in PHP just ignoring any characters that are not a part of base64 and as such never actually fail, despite the documentation saying that it will.
Since a valid JWT will never contain any characters that are outside of base64url I don't think this should be considered a breaking change as those tokens would most likely have triggered one of the other exceptions that validates the token anyway.

In regards to the changing I do in decode with InvalidArgumentException to UnexpectedValueException
As explained above the current behavior is to ignore invalid characters, the fact that urlsafeB64Decode even throws an InvalidArgumentException now is because of my change, so that change in itself doesn't break much.
The reason I change them to UnexpectedValueException is because every other validation error in decode uses that exception, so any try/catch around it will still work as normal, introducing no breaking changes, only adding more explanations as to what went wrong.

I would had had urlsafeB64Decode simply throw an UnexpectedValueException and then not having to do all the converting up in decode if it weren't for the documentation stating that it should throw an InvalidArgumentException

Last change still allowed input that was invalid base64URL but valid
base64
@Oinkling
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants