Closed CheckerService catch variable $e leaks into the outer scope on a non-JSON response

[Eris Greyrat]

Affected version

2.3.9

File: src/XF/Service/Upgrade/CheckerService.php
FIle: src/XF/Service/Upgrade/Checker.php

In CheckerService::check() and Checker::check(), the inner try/catch block catches \InvalidArgumentException into a variable named $e. In PHP, catch block variables leak into the outer scope. When the response is non-JSON (e.g. a 502 Bad Gateway returning error code: 502), $responseJson is set to null and $e holds the InvalidArgumentException from Utils::jsonDecode.

The else branch then checks if (!isset($e)), but $e is already set from the inner catch, so logCheckFailure logs the InvalidArgumentException (with message json_decode error: Syntax error) as a server error. A transient upstream 502 ends up appearing as a site exception in the admin error log.

Steps to reproduce:​

1. Wait for XF\Job\UpgradeCheck to run while xenforo.com returns a non-JSON response (e.g. a CDN 502)
2. Check Admin CP -> Logs -> Server Error Log

Expected: A transient external 502 is not logged as a site server error.

Actual: GuzzleHttp\Exception\InvalidArgumentException: json_decode error: Syntax error appears in the server error log attributed to the upgrade check.


CheckerService.php and Checker.php Before:

try
{
    $responseJson = Utils::jsonDecode($contents, true);
}
catch (\InvalidArgumentException $e)  // $e leaks into outer scope
{
    $responseJson = null;
}

CheckerService.php and Checker.php After:

try
{
    $responseJson = Utils::jsonDecode($contents, true);
}
catch (\InvalidArgumentException $jsonException)  // isolated, avoid leak
{
    $responseJson = null;
}

Fix:​

Rename the catch variable from $e to $jsonException in both CheckerService.php and Checker.php. This ensures the if (!isset($e)) check in the else branch creates a clean new \Exception('') Instead of reusing the json_decode exception, giving a more meaningful log entry and avoid the misleading stack trace.

 

[XF Bug Bot]

Automated triage: As designed

Logging a failed upgrade check, including one caused by a temporary error page from our servers, is intended so administrators can see why the check failed. The exception is reused on purpose to explain the failure. Renaming the variable would still log an entry, just with less detail.

This is an automated assessment. A member of staff may review and update it.