firebase / php-jwt

PHP package for JWT
BSD 3-Clause "New" or "Revised" License
9.3k stars 1.26k forks source link

Update Return type of JWT::urlsafeB64Decode #567

Closed smyhill closed 3 weeks ago

smyhill commented 1 month ago

After debugging for a while in my personal implementation of this library I released it is possible for the following method to return false on failure as well as the decoded string. This can happen when the core PHP base64_decode method returns false on failure because this method is returned in the parent method. To fix possible confusion my suggestion would be to update the method to use a union type to avoid confusion in the future. I imagine this can occur because the class is not strictly typed. Old:

/**
     * Decode a string with URL-safe Base64.
     *
     * @param string $input A Base64 encoded string
     *
     * @return string A decoded string
     *
     * @throws InvalidArgumentException invalid base64 characters
     */
    public static function urlsafeB64Decode(string $input): string
    {
        return \base64_decode(self::convertBase64UrlToBase64($input));
    }

New:

/**
     * Decode a string with URL-safe Base64.
     *
     * @param string $input A Base64 encoded string
     *
     * @return string|false A decoded string or false on failure
     *
     * @throws InvalidArgumentException invalid base64 characters
     */
    public static function urlsafeB64Decode(string $input): string|false
    {
        return \base64_decode(self::convertBase64UrlToBase64($input));
    }
bshaffer commented 3 weeks ago

Hello! thanks for submitting this issue.

Rather than making false a valid return value from this method, I'd rather have it throw an exception (as indicated in the @throws PHPdoc):

    /**
     * Decode a string with URL-safe Base64.
     *
     * @param string $input A Base64 encoded string
     *
     * @return string A decoded string
     *
     * @throws InvalidArgumentException invalid base64 characters
     */
    public static function urlsafeB64Decode(string $input): string
    {
        if (false === $decoded = \base64_decode(self::convertBase64UrlToBase64($input))) {
            throw new InvalidArgumentException('the input contains character from outside the base64 alphabet.');
        }
        return $decoded;
    }
smyhill commented 3 weeks ago

Ah thank you. I think I need to update my library to the latest version because in the current version false is not checked for. Appreciate you getting back to me. This current version handles things well.