From 1d79bd6036e3e004669c625b91023d755fc4f38b Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 6 Mar 2014 09:37:26 -0500 Subject: [PATCH] API: Handle exceptions from ApiBeforeMain hook in a user-friendly manner The immediate impetus behind this change is this series of events: 1. CirrusSearch hooks ApiBeforeMain to handle some setup that requires the User object. 2. So User is loaded from the session. 3. OAuth checks the headers as part of loading User. 4. OAuth sees that the headers are invalid, and since it was called from the API it throws a UsageException, expecting the API to catch it and return an appropriate response to the client. 5. But nothing does so, leading to an unhelpful "Internal Error" page being returned to the client. We can do better than that. Bug: 62312 Change-Id: Ib5735661eec6ebe57eaa69c67b399e703cc90fc4 --- api.php | 27 ++++++++--- includes/api/ApiMain.php | 99 +++++++++++++++++++++++++++------------- 2 files changed, 89 insertions(+), 37 deletions(-) diff --git a/api.php b/api.php index 0d2312a7ea..554c2726bc 100644 --- a/api.php +++ b/api.php @@ -70,10 +70,21 @@ $wgTitle = Title::makeTitle( NS_MAIN, 'API' ); $processor = new ApiMain( RequestContext::getMain(), $wgEnableWriteAPI ); // Last chance hook before executing the API -wfRunHooks( 'ApiBeforeMain', array( &$processor ) ); +try { + wfRunHooks( 'ApiBeforeMain', array( &$processor ) ); + if ( !$processor instanceof ApiMain ) { + throw new MWException( 'ApiBeforMain hook set $processor to a non-ApiMain class' ); + } +} catch ( Exception $e ) { + // Crap. Try to report the exception in API format to be friendly to clients. + ApiMain::handleApiBeforeMainException( $e ); + $processor = false; +} // Process data & print results -$processor->execute(); +if ( $processor ) { + $processor->execute(); +} if ( function_exists( 'fastcgi_finish_request' ) ) { fastcgi_finish_request(); @@ -97,11 +108,15 @@ if ( $wgAPIRequestLog ) { $_SERVER['HTTP_USER_AGENT'] ); $items[] = $wgRequest->wasPosted() ? 'POST' : 'GET'; - $module = $processor->getModule(); - if ( $module->mustBePosted() ) { - $items[] = "action=" . $wgRequest->getVal( 'action' ); + if ( $processor ) { + $module = $processor->getModule(); + if ( $module->mustBePosted() ) { + $items[] = "action=" . $wgRequest->getVal( 'action' ); + } else { + $items[] = wfArrayToCgi( $wgRequest->getValues() ); + } } else { - $items[] = wfArrayToCgi( $wgRequest->getValues() ); + $items[] = "failed in ApiBeforeMain"; } wfErrorLog( implode( ',', $items ) . "\n", $wgAPIRequestLog ); wfDebug( "Logged API request to $wgAPIRequestLog\n" ); diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index eb24a35b86..8f270dcb18 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -363,37 +363,7 @@ class ApiMain extends ApiBase { try { $this->executeAction(); } catch ( Exception $e ) { - // Allow extra cleanup and logging - wfRunHooks( 'ApiMain::onException', array( $this, $e ) ); - - // Log it - if ( !( $e instanceof UsageException ) ) { - MWExceptionHandler::logException( $e ); - } - - // Handle any kind of exception by outputting properly formatted error message. - // If this fails, an unhandled exception should be thrown so that global error - // handler will process and log it. - - $errCode = $this->substituteResultWithError( $e ); - - // Error results should not be cached - $this->setCacheMode( 'private' ); - - $response = $this->getRequest()->response(); - $headerStr = 'MediaWiki-API-Error: ' . $errCode; - if ( $e->getCode() === 0 ) { - $response->header( $headerStr ); - } else { - $response->header( $headerStr, true, $e->getCode() ); - } - - // Reset and print just the error message - ob_clean(); - - // If the error occurred during printing, do a printer->profileOut() - $this->mPrinter->safeProfileOut(); - $this->printResult( true ); + $this->handleException( $e ); } // Log the request whether or not there was an error @@ -410,6 +380,73 @@ class ApiMain extends ApiBase { ob_end_flush(); } + /** + * Handle an exception as an API response + * + * @since 1.23 + * @param Exception $e + */ + protected function handleException( Exception $e ) { + // Allow extra cleanup and logging + wfRunHooks( 'ApiMain::onException', array( $this, $e ) ); + + // Log it + if ( !( $e instanceof UsageException ) ) { + MWExceptionHandler::logException( $e ); + } + + // Handle any kind of exception by outputting properly formatted error message. + // If this fails, an unhandled exception should be thrown so that global error + // handler will process and log it. + + $errCode = $this->substituteResultWithError( $e ); + + // Error results should not be cached + $this->setCacheMode( 'private' ); + + $response = $this->getRequest()->response(); + $headerStr = 'MediaWiki-API-Error: ' . $errCode; + if ( $e->getCode() === 0 ) { + $response->header( $headerStr ); + } else { + $response->header( $headerStr, true, $e->getCode() ); + } + + // Reset and print just the error message + ob_clean(); + + // If the error occurred during printing, do a printer->profileOut() + $this->mPrinter->safeProfileOut(); + $this->printResult( true ); + } + + /** + * Handle an exception from the ApiBeforeMain hook. + * + * This tries to print the exception as an API response, to be more + * friendly to clients. If it fails, it will rethrow the exception. + * + * @since 1.23 + * @param Exception $e + */ + public static function handleApiBeforeMainException( Exception $e ) { + ob_start(); + + try { + $main = new self( RequestContext::getMain(), false ); + $main->handleException( $e ); + } catch ( Exception $e2 ) { + // Nope, even that didn't work. Punt. + throw $e; + } + + // Log the request and reset cache headers + $main->logRequest( 0 ); + $main->sendCacheHeaders(); + + ob_end_flush(); + } + /** * Check the &origin= query parameter against the Origin: HTTP header and respond appropriately. * -- 2.20.1