Skip to content
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

Decode method does not enforce object return type #370

Closed
markrandall opened this issue Nov 6, 2021 · 3 comments
Closed

Decode method does not enforce object return type #370

markrandall opened this issue Nov 6, 2021 · 3 comments

Comments

@markrandall
Copy link

markrandall commented Nov 6, 2021

The decode method declares an object return type using docblock, however this is not enforced by either userland runtime checks or a PHP return type, and the function will quite happily return a non-object result if the original JWT was created in such a way:

public function testDecodeNonObject(): void {
	$key = 'example';
	$token = JWT::encode(123, $key, 'HS256');
	$decoded = JWT::decode($token, $key, ['HS256']);
	self::assertIsObject($decoded);
}

This causes higher levels of static analysis to either fail to detect a possible error condition when using the return value, or alternatively create a false positive for a redundant check when verifying the return type is an object.

@bshaffer
Copy link
Collaborator

bshaffer commented Nov 8, 2021

Thank you for pointing this out!

You're right, the library assumes the $payload parameter to JWT::encode (in this example, 123) is going to be object|array, but does not enforce it.

As the types are correct if used as documented, can this really cause a static analysis error? Also, without breaking backwards compatibility, how would you recommend fixing this?

@markrandall
Copy link
Author

markrandall commented Nov 9, 2021

As the types are correct if used as documented, can this really cause a static analysis error?

As there is no requirement that the JWT originates from this library, or even the same application, the documented type of encode (which itself is not enforced, as can be seen in the example) is not super relevant.

Using decode (as documented) expects to return an object, but this does not have to be the case, and may cause errors in the code consuming the return value of decode (such as by accessing a property) if some other type is returned.

My recommendation would be throwing an UnexpectedValueException if the decoded object was not an object. This would then match the behaviour specified in the docblock.

An alternative would be to remove the @return object in the docblock.

If this was typed as object using native PHP, it would throw a TypeError, allowing a malformed JWT to trigger an \Error branch rather than \Exception branch throwable.

@bshaffer
Copy link
Collaborator

This is now enforced: https://github.com/firebase/php-jwt/blob/main/src/JWT.php#L162

It will be released in v6.1

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

No branches or pull requests

2 participants