From e4a51e460e0e7020fed0eeeb8b6cce7148e0bb58 Mon Sep 17 00:00:00 2001 From: marc tobias Date: Sun, 16 Sep 2018 17:16:42 +0200 Subject: [PATCH] set exception handler by request format, not always HTML --- lib/DatabaseError.php | 32 ++++++++ lib/init-website.php | 98 +++++++++-------------- lib/lib.php | 19 +++-- lib/template/error-html.php | 61 ++++++++++++++ lib/template/error-json.php | 11 +++ lib/template/error-xml.php | 7 ++ test/bdd/api/errors/formats.feature | 13 +++ test/bdd/api/lookup/simple.feature | 2 +- test/bdd/api/search/simple.feature | 2 +- test/bdd/steps/queries.py | 12 +++ test/php/Nominatim/AddressDetailsTest.php | 6 +- test/php/Nominatim/DatabaseErrorTest.php | 44 ++++++++++ test/php/Nominatim/LibTest.php | 3 + test/php/Nominatim/StatusTest.php | 1 + test/php/Nominatim/TokenListTest.php | 4 +- test/php/bootstrap.php | 2 + website/details.php | 1 + website/lookup.php | 1 + website/reverse.php | 1 + website/search.php | 1 + 20 files changed, 244 insertions(+), 77 deletions(-) create mode 100644 lib/DatabaseError.php create mode 100644 lib/template/error-html.php create mode 100644 lib/template/error-json.php create mode 100644 lib/template/error-xml.php create mode 100644 test/bdd/api/errors/formats.feature create mode 100644 test/php/Nominatim/DatabaseErrorTest.php diff --git a/lib/DatabaseError.php b/lib/DatabaseError.php new file mode 100644 index 00000000..2df331bf --- /dev/null +++ b/lib/DatabaseError.php @@ -0,0 +1,32 @@ +oSql = $oSql; + } + + public function __toString() + { + return __CLASS__ . ": [{$this->code}]: {$this->message}\n"; + } + + public function getSqlError() + { + return $this->oSql->getMessage(); + } + + public function getSqlDebugDump() + { + if (CONST_Debug) { + return var_export($this->oSql, true); + } else { + return $this->oSql->getUserInfo(); + } + } +} diff --git a/lib/init-website.php b/lib/init-website.php index ca5214da..ae2a5d36 100644 --- a/lib/init-website.php +++ b/lib/init-website.php @@ -2,6 +2,7 @@ require_once('init.php'); require_once('ParameterParser.php'); +require_once('DatabaseError.php'); require_once(CONST_Debug ? 'DebugHtml.php' : 'DebugNone.php'); /*************************************************************************** @@ -15,74 +16,51 @@ function chksql($oSql, $sMsg = 'Database request failed') { if (!PEAR::isError($oSql)) return $oSql; - header('HTTP/1.0 500 Internal Server Error'); - header('Content-type: text/html; charset=utf-8'); - - $sSqlError = $oSql->getMessage(); - - echo << - Internal Server Error - -

Internal Server Error

Nominatim has encountered an internal error while accessing the database. - This may happen because the database is broken or because of a bug in - the software. If you think it is a bug, feel free to report - it over on - Github. Please include the URL that caused the problem and the - complete error details below.

Message: $sMsg

SQL Error: $sSqlError

Details:

-INTERNALFAIL;
-
-    if (CONST_Debug) {
-        var_dump($oSql);
-    } else {
-        echo "\n".$oSql->getUserInfo().'';
-    }
+    throw new Nominatim\DatabaseError($sMsg, 500, null, $oSql);
+}
+
 
-    echo '

'; - exit; +function userError($sMsg) +{ + throw new Exception($sMsg, 400); } -function failInternalError($sError, $sSQL = false, $vDumpVar = false) + +function exception_handler_html($exception) { - header('HTTP/1.0 500 Internal Server Error'); - header('Content-type: text/html; charset=utf-8'); - echo '

Internal Server Error

'; - echo '

Nominatim has encountered an internal error while processing your request. This is most likely because of a bug in the software.

'; - echo '

Details: '.$sError,'

'; - echo '

Feel free to file an issue on Github. '; - echo 'Please include the error message above and the URL you used.

'; - if (CONST_Debug) { - echo '

Debugging Information

'; - if ($sSQL) { - echo '

SQL query

'.$sSQL.''; - } - if ($vDumpVar) { - echo '

Result

';
-            var_dump($vDumpVar);
-            echo '

'; - } - } - echo "\n\n"; - exit; + http_response_code($exception->getCode()); + header('Content-type: text/html; charset=UTF-8'); + include(CONST_BasePath.'/lib/template/error-html.php'); } +function exception_handler_json($exception) +{ + http_response_code($exception->getCode()); + header('Content-type: application/json; charset=utf-8'); + include(CONST_BasePath.'/lib/template/error-json.php'); +} -function userError($sError) +function exception_handler_xml($exception) { - header('HTTP/1.0 400 Bad Request'); - header('Content-type: text/html; charset=utf-8'); - echo '

Bad Request

'; - echo '

Nominatim has encountered an error with your request.

'; - echo '

Details: '.$sError.'

'; - echo '

If you feel this error is incorrect feel file an issue on Github. '; - echo 'Please include the error message above and the URL you used.

'; - echo "\n\n"; - exit; + http_response_code($exception->getCode()); + header('Content-type: text/xml; charset=utf-8'); + echo ''."\n"; + include(CONST_BasePath.'/lib/template/error-xml.php'); +} + + +function set_exception_handler_by_format($sFormat = 'html') +{ + if ($sFormat == 'html') { + set_exception_handler('exception_handler_html'); + } elseif ($sFormat == 'xml') { + set_exception_handler('exception_handler_xml'); + } else { + set_exception_handler('exception_handler_json'); + } } +// set a default +set_exception_handler_by_format(); /*************************************************************************** @@ -96,6 +74,6 @@ if (CONST_NoAccessControl) { header('Access-Control-Allow-Headers: '.$_SERVER['HTTP_ACCESS_CONTROL_REQUEST_HEADERS']); } } -if ($_SERVER['REQUEST_METHOD'] == 'OPTIONS') exit; +if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] == 'OPTIONS') exit; if (CONST_Debug) header('Content-type: text/html; charset=utf-8'); diff --git a/lib/lib.php b/lib/lib.php index a749cb6b..cd23b068 100644 --- a/lib/lib.php +++ b/lib/lib.php @@ -61,6 +61,13 @@ function byImportance($a, $b) function javascript_renderData($xVal, $iOptions = 0) { + $sCallback = isset($_GET['json_callback']) ? $_GET['json_callback'] : ''; + if ($sCallback && !preg_match('/^[$_\p{L}][$_\p{L}\p{Nd}.[\]]*$/u', $sCallback)) { + // Unset, we call javascript_renderData again during exception handling + unset($_GET['json_callback']); + throw new Exception('Invalid json_callback value', 400); + } + $iOptions |= JSON_UNESCAPED_UNICODE; if (isset($_GET['pretty']) && in_array(strtolower($_GET['pretty']), array('1', 'true'))) { $iOptions |= JSON_PRETTY_PRINT; @@ -68,16 +75,12 @@ function javascript_renderData($xVal, $iOptions = 0) $jsonout = json_encode($xVal, $iOptions); - if (!isset($_GET['json_callback'])) { + if ($sCallback) { + header('Content-Type: application/javascript; charset=UTF-8'); + echo $_GET['json_callback'].'('.$jsonout.')'; + } else { header('Content-Type: application/json; charset=UTF-8'); echo $jsonout; - } else { - if (preg_match('/^[$_\p{L}][$_\p{L}\p{Nd}.[\]]*$/u', $_GET['json_callback'])) { - header('Content-Type: application/javascript; charset=UTF-8'); - echo $_GET['json_callback'].'('.$jsonout.')'; - } else { - header('HTTP/1.0 400 Bad Request'); - } } } diff --git a/lib/template/error-html.php b/lib/template/error-html.php new file mode 100644 index 00000000..208237eb --- /dev/null +++ b/lib/template/error-html.php @@ -0,0 +1,61 @@ +getCode() == 400 ) { + $title = 'Bad Request'; + } +?> + + + + + + +

+ + + +

Nominatim has encountered an internal error while accessing the database. + This may happen because the database is broken or because of a bug in + the software.

+ + + +

Nominatim has encountered an error with your request.

+ + + + +

Details

+ + Uncaught exception + with message getMessage() ?> + + +
+ thrown in getFile() . '('. $exception->getLine() . ')' ?>. + + + +

SQL Error

+ getSqlError() ?> + +

getSqlDebugDump() ?>

+ + + +

Stack trace

getTraceAsString() ?>

+ + + +

+ If you feel this error is incorrect feel file an issue on + Github. + + Please include the error message above and the URL you used. +

+ + diff --git a/lib/template/error-json.php b/lib/template/error-json.php new file mode 100644 index 00000000..81caa71d --- /dev/null +++ b/lib/template/error-json.php @@ -0,0 +1,11 @@ + $exception->getCode(), + 'message' => $exception->getMessage() + ); + + if (CONST_Debug) { + $error['details'] = $exception->getFile() . '('. $exception->getLine() . ')'; + } + + echo javascript_renderData(array('error' => $error)); diff --git a/lib/template/error-xml.php b/lib/template/error-xml.php new file mode 100644 index 00000000..a21ac198 --- /dev/null +++ b/lib/template/error-xml.php @@ -0,0 +1,7 @@ + + getCode() ?> + getMessage() ?> + +

getFile() . '('. $exception->getLine() . ')' ?>

+ + \ No newline at end of file diff --git a/test/bdd/api/errors/formats.feature b/test/bdd/api/errors/formats.feature new file mode 100644 index 00000000..8a8e6561 --- /dev/null +++ b/test/bdd/api/errors/formats.feature @@ -0,0 +1,13 @@ +@APIDB +Feature: Places by osm_type and osm_id Tests + Simple tests for errors in various response formats. + + Scenario Outline: Force error by providing too many ids + When sending lookup query for N1,N2,N3,N4,N5,N6,N7,N8,N9,N10,N11,N12,N13,N14,N15,N16,N17,N18,N19,N20,N21,N22,N23,N24,N25,N26,N27,N28,N29,N30,N31,N32,N33,N34,N35,N36,N37,N38,N39,N40,N41,N42,N43,N44,N45,N46,N47,N48,N49,N50,N51 + Then a user error is returned + + Examples: + | format | + | xml | + | json | + | geojson | diff --git a/test/bdd/api/lookup/simple.feature b/test/bdd/api/lookup/simple.feature index e29812e0..660598db 100644 --- a/test/bdd/api/lookup/simple.feature +++ b/test/bdd/api/lookup/simple.feature @@ -1,6 +1,6 @@ @APIDB Feature: Places by osm_type and osm_id Tests - Simple tests for internal server errors and response format. + Simple tests for response format. Scenario Outline: address lookup for existing node, way, relation When sending lookup query for N3284625766,W6065798,,R123924,X99,N0 diff --git a/test/bdd/api/search/simple.feature b/test/bdd/api/search/simple.feature index ca441258..c1263597 100644 --- a/test/bdd/api/search/simple.feature +++ b/test/bdd/api/search/simple.feature @@ -194,7 +194,7 @@ Feature: Simple Tests When sending json search query "Tokyo" | param | value | |json_callback | | - Then a HTTP 400 is returned + Then a json user error is returned Examples: | data | diff --git a/test/bdd/steps/queries.py b/test/bdd/steps/queries.py index df34b5cc..4d59b923 100644 --- a/test/bdd/steps/queries.py +++ b/test/bdd/steps/queries.py @@ -494,6 +494,18 @@ def step_impl(context, fmt): context.execute_steps("Then a HTTP 200 is returned") eq_(context.response.format, fmt) +@then(u'a (?P\w+) user error is returned') +def check_page_error(context, fmt): + context.execute_steps("Then a HTTP 400 is returned") + eq_(context.response.format, fmt) + + if fmt == 'html': + assert_is_not_none(re.search(r').+', context.response.page, re.DOTALL)) + elif fmt == 'xml': + assert_is_not_none(re.search(r'.+', context.response.page, re.DOTALL)) + else: + assert_is_not_none(re.search(r'({"error":)', context.response.page, re.DOTALL)) + @then(u'result header contains') def check_header_attr(context): for line in context.table: diff --git a/test/php/Nominatim/AddressDetailsTest.php b/test/php/Nominatim/AddressDetailsTest.php index 62faf1a4..b29d9088 100644 --- a/test/php/Nominatim/AddressDetailsTest.php +++ b/test/php/Nominatim/AddressDetailsTest.php @@ -2,14 +2,10 @@ namespace Nominatim; +require_once(CONST_BasePath.'/lib/init-website.php'); require_once(CONST_BasePath.'/lib/AddressDetails.php'); -function chksql($oSql, $sMsg = 'Database request failed') -{ - return $oSql; -} - class AddressDetailsTest extends \PHPUnit\Framework\TestCase { diff --git a/test/php/Nominatim/DatabaseErrorTest.php b/test/php/Nominatim/DatabaseErrorTest.php new file mode 100644 index 00000000..25b4aa0b --- /dev/null +++ b/test/php/Nominatim/DatabaseErrorTest.php @@ -0,0 +1,44 @@ +getMockBuilder(\DB_Error::class) + ->setMethods(array('getMessage')) + ->getMock(); + + $oSqlStub->method('getMessage') + ->willReturn('Unknown table.'); + + $oErr = new DatabaseError('Sql error', 123, null, $oSqlStub); + $this->assertEquals('Sql error', $oErr->getMessage()); + $this->assertEquals(123, $oErr->getCode()); + $this->assertEquals('Unknown table.', $oErr->getSqlError()); + + // causes a circular reference warning during dump + // $this->assertRegExp('/Mock_DB_Error/', $oErr->getSqlDebugDump()); + } + + public function testSqlObjectDump() + { + $oErr = new DatabaseError('Sql error', 123, null, array('one' => 'two')); + $this->assertRegExp('/two/', $oErr->getSqlDebugDump()); + } + + public function testChksqlThrows() + { + $this->expectException(DatabaseError::class); + $this->expectExceptionMessage('My custom error message'); + $this->expectExceptionCode(500); + + $oDB = new \DB_Error; + $this->assertEquals(false, chksql($oDB, 'My custom error message')); + } +} diff --git a/test/php/Nominatim/LibTest.php b/test/php/Nominatim/LibTest.php index 2891388d..dbf8feca 100644 --- a/test/php/Nominatim/LibTest.php +++ b/test/php/Nominatim/LibTest.php @@ -2,6 +2,9 @@ namespace Nominatim; +require_once(CONST_BasePath.'/lib/lib.php'); +require_once(CONST_BasePath.'/lib/ClassTypes.php'); + class LibTest extends \PHPUnit\Framework\TestCase { diff --git a/test/php/Nominatim/StatusTest.php b/test/php/Nominatim/StatusTest.php index 4f21706e..eb4ad68a 100644 --- a/test/php/Nominatim/StatusTest.php +++ b/test/php/Nominatim/StatusTest.php @@ -2,6 +2,7 @@ namespace Nominatim; +require_once(CONST_BasePath.'/lib/db.php'); require_once(CONST_BasePath.'/lib/Status.php'); diff --git a/test/php/Nominatim/TokenListTest.php b/test/php/Nominatim/TokenListTest.php index fa1331e8..4016a839 100644 --- a/test/php/Nominatim/TokenListTest.php +++ b/test/php/Nominatim/TokenListTest.php @@ -2,8 +2,8 @@ namespace Nominatim; -require_once(CONST_BasePath.'/lib/db.php'); -require_once(CONST_BasePath.'/lib/cmd.php'); +// require_once(CONST_BasePath.'/lib/db.php'); +// require_once(CONST_BasePath.'/lib/cmd.php'); require_once(CONST_BasePath.'/lib/TokenList.php'); diff --git a/test/php/bootstrap.php b/test/php/bootstrap.php index 0d475962..d6968717 100644 --- a/test/php/bootstrap.php +++ b/test/php/bootstrap.php @@ -1,2 +1,4 @@ getSet('format', array('html', 'json'), 'html'); +set_exception_handler_by_format($sOutputFormat); $aLangPrefOrder = $oParams->getPreferredLanguages(); $sLanguagePrefArraySQL = 'ARRAY['.join(',', array_map('getDBQuoted', $aLangPrefOrder)).']'; diff --git a/website/lookup.php b/website/lookup.php index f09506a4..ec2a3a8a 100755 --- a/website/lookup.php +++ b/website/lookup.php @@ -12,6 +12,7 @@ $oParams = new Nominatim\ParameterParser(); // Format for output $sOutputFormat = $oParams->getSet('format', array('xml', 'json', 'geojson'), 'xml'); +set_exception_handler_by_format($sOutputFormat); // Preferred language $aLangPrefOrder = $oParams->getPreferredLanguages(); diff --git a/website/reverse.php b/website/reverse.php index a7261802..963f7500 100755 --- a/website/reverse.php +++ b/website/reverse.php @@ -13,6 +13,7 @@ $oParams = new Nominatim\ParameterParser(); // Format for output $sOutputFormat = $oParams->getSet('format', array('html', 'xml', 'json', 'jsonv2', 'geojson', 'geocodejson'), 'xml'); +set_exception_handler_by_format($sOutputFormat); // Preferred language $aLangPrefOrder = $oParams->getPreferredLanguages(); diff --git a/website/search.php b/website/search.php index 0ebf1814..0b678caa 100755 --- a/website/search.php +++ b/website/search.php @@ -27,6 +27,7 @@ if (CONST_Search_ReversePlanForAll // Format for output $sOutputFormat = $oParams->getSet('format', array('html', 'xml', 'json', 'jsonv2', 'geojson', 'geocodejson'), 'html'); +set_exception_handler_by_format($sOutputFormat); $sForcedGeometry = ($sOutputFormat == 'html') ? 'geojson' : null; $oGeocode->loadParamArray($oParams, $sForcedGeometry); -- 2.43.2