Fixed \XF\Service\Oembed::fetchJsonDataFromUrl may falsely report success

K

Kirby

Well-known member
Affected version
2.2.13
PHP: 
if ($response->getStatusCode() == 200)
{
    try
    {
        $json = json_decode($jsonText, true);

        if (!empty($json['title']))
        {
            $title = $json['title'];
        }
        else if (!empty($json['author_name']))
        {
            $title = $json['author_name'];
        }

        $validOembed = true;
    }
    catch (\Exception $e)
    {
        $error = \XF::phraseDeferred('returned_data_is_not_json');
    }

json_decode doesn't throw an exception if it can't decode, it will just return null in which case this code will just happily continue without setting any title but treating the data as valid (which later on saves nonsense like HTML in internal_data/oembed-cache)

Suggested Fix
Use GuzzleHttp\json_decode which does throw an exception on decode error so the code works as expected.
 
Probably easier to just add an is_array($json) check in given the versions of php XF supports. I'm not sure the $validOembed being unconditionally set to true makes sense if there is no title.
 
It doesn't make sense to set $validOembed if there is no title.

The current code as outlined above doesn't make much sense to me at all as there never will be an exception that could be catched.

By using GuzzleHttp\json_decode instead of just json_decode an exception would be thrown if there is a decode error so the existing exception handling code can kick in (in which case $validOembed would stay false).

Using is_array($json) would work but still have the unused try-catch block .
 
Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.14).

Change log:
Properly throw an exception when failing to decode JSON for oEmbed
Click to expand...
There may be a delay before changes are rolled out to the XenForo Community.
 
Back
Top Bottom