Anomie has uploaded a new change for review. https://gerrit.wikimedia.org/r/117189
Change subject: API: Handle exceptions from ApiBeforeMain hook in a user-friendly manner ...................................................................... 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 --- M api.php M includes/api/ApiMain.php 2 files changed, 89 insertions(+), 37 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/89/117189/1 diff --git a/api.php b/api.php index 0d2312a..554c272 100644 --- a/api.php +++ b/api.php @@ -70,10 +70,21 @@ $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 @@ $_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 0939dea..fea002e 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -362,37 +362,7 @@ 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 @@ } /** + * 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. * * If no origin parameter is present, nothing happens. -- To view, visit https://gerrit.wikimedia.org/r/117189 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib5735661eec6ebe57eaa69c67b399e703cc90fc4 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Anomie <bjor...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits