fix: return all errors as JSON instead of HTML pages

README.md

@@ -223,7 +223,7 @@ To store data, send a POST request to `/measure` with the values in one of the f
   curl -X POST http://localhost:8080/measure -H "Content-Type: application/xml" -d '<data><temperature>21.5</temperature><humidity>40</humidity></data>'
   ```
 
-A malformed XML body or a JSON body that is not an object is rejected with a `400` response. With `format='xml'`, column names must be valid XML element names: letters, digits, `_`, `-` or `.`, starting with a letter or `_`, without spaces, and not starting with `xml`.
+A malformed XML body or a JSON body that is not an object (or not valid JSON) is rejected with a `400` response. All errors are returned as JSON with a `message` field, e.g. `{"message": "Invalid JSON: the request body could not be parsed."}`. With `format='xml'`, column names must be valid XML element names: letters, digits, `_`, `-` or `.`, starting with a letter or `_`, without spaces, and not starting with `xml`.
 
 To restrict access to the collected data, define the following setting in the `configuration.yml` file in the root directory:
 ```yml

docs/getting_started.rst

@@ -96,7 +96,7 @@ To store data, send a POST request to ``/measure`` with the values in one of the
 
     curl -X POST http://localhost:8080/measure -H "Content-Type: application/xml" -d '<data><temperature>21.5</temperature><humidity>40</humidity></data>'
 
-A malformed XML body or a JSON body that is not an object is rejected with a ``400`` response. With ``format='xml'``, column names must be valid XML element names: letters, digits, ``_``, ``-`` or ``.``, starting with a letter or ``_``, without spaces, and not starting with ``xml``.
+A malformed XML body or a JSON body that is not an object (or not valid JSON) is rejected with a ``400`` response. All errors are returned as JSON with a ``message`` field, e.g. ``{"message": "Invalid JSON: the request body could not be parsed."}``. With ``format='xml'``, column names must be valid XML element names: letters, digits, ``_``, ``-`` or ``.``, starting with a letter or ``_``, without spaces, and not starting with ``xml``.
 
 To restrict access to the collected data, define the following setting in the ``configuration.yml`` file in the root directory:
 

succulent/api.py

@@ -1,10 +1,27 @@
+import json
 import numpy as np
 import pandas as pd
-from flask import Flask, jsonify, request, url_for, make_response
+from flask import Flask, Request, jsonify, request, url_for, make_response
+from werkzeug.exceptions import BadRequest, HTTPException
 from succulent.configuration import Configuration
 from succulent.processing import Processing
 
 
+class SucculentRequest(Request):
+    """Request class with a clear error message for invalid JSON bodies."""
+
+    def on_json_loading_failed(self, e):
+        """Raise a 400 error that says the JSON body could not be parsed.
+
+        Args:
+            e (ValueError or None): The error raised while parsing.
+
+        Raises:
+            BadRequest: Always.
+        """
+        raise BadRequest('Invalid JSON: the request body could not be parsed.')
+
+
 class SucculentAPI:
     """Succulent API server.
 
@@ -48,6 +65,7 @@ def __init__(self, config, host='0.0.0.0', port=8080, format='csv'):
 
         # Initialise Flask
         self.app = Flask(__name__, static_url_path='/succulent/static')
+        self.app.request_class = SucculentRequest
 
         # Limit the maximum accepted size of incoming request bodies to
         # protect the server against oversized or flooding POST requests.
@@ -66,6 +84,11 @@ def __init__(self, config, host='0.0.0.0', port=8080, format='csv'):
         self.app.register_error_handler(
             413, self.payload_too_large)
 
+        # Return every other error as JSON ({"message": ...}) instead of
+        # Flask's default HTML error page, like the API's own errors
+        self.app.register_error_handler(HTTPException, self.http_error)
+        self.app.register_error_handler(Exception, self.internal_error)
+
     def index(self):
         """Generate index HTML page with information about the API.
 
@@ -168,6 +191,44 @@ def payload_too_large(self, error):
         """
         return jsonify({'message': 'Payload too large.'}), 413
 
+    def http_error(self, error):
+        """Return an HTTP error (e.g. 400, 404, 405) as JSON.
+
+        Headers of the error are kept, e.g. ``Allow`` for 405 responses.
+
+        Args:
+            error (HTTPException): The raised HTTP error.
+
+        Returns:
+            Response: JSON response with an error message.
+        """
+        # Short message (e.g. "Not Found.") instead of Werkzeug's generic
+        # description; a specific description (e.g. invalid JSON) is kept
+        if error.description and error.description != type(error).description:
+            message = error.description
+        else:
+            message = f'{error.name}.'
+
+        response = error.get_response()
+        response.data = json.dumps({'message': message})
+        response.content_type = 'application/json'
+        return response
+
+    def internal_error(self, error):
+        """Return an unexpected error as a JSON 500 response.
+
+        The error is logged with its traceback. Its details are not sent to
+        the client.
+
+        Args:
+            error (Exception): The unexpected error.
+
+        Returns:
+            Response: JSON response with an error message.
+        """
+        self.app.logger.exception('Unexpected error on %s %s', request.method, request.path)
+        return jsonify({'message': 'Internal server error.'}), 500
+
     def start(self):
         """Start the Flask application on the specified host and port.
 

tests/test_succulent.py

@@ -671,6 +671,63 @@ def test_invalid_allowed_extensions(self):
                                format='image', unittest=True)
 
 
+class TestJsonErrors(unittest.TestCase):
+    """
+    Test that errors are returned as JSON instead of HTML pages.
+    """
+
+    def setUp(self):
+        config_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'configuration.yml')
+        self.api = SucculentAPI(host='0.0.0.0', port=8080, config=config_path, format='csv')
+        self.client = self.api.app.test_client()
+
+    def tearDown(self):
+        shutil.rmtree(self.api.processing.directory, ignore_errors=True)
+
+    def assertJsonError(self, response, status, message):
+        self.assertEqual(response.status_code, status)
+        self.assertEqual(response.content_type, 'application/json')
+        self.assertEqual(response.get_json(), {'message': message})
+
+    def test_invalid_json_body(self):
+        for body in ['{bad', '']:
+            with self.subTest(body=body):
+                response = self.client.post('/measure', data=body,
+                                            content_type='application/json')
+                self.assertJsonError(response, 400,
+                                     'Invalid JSON: the request body could not be parsed.')
+
+    def test_unknown_path(self):
+        self.assertJsonError(self.client.get('/nope'), 404, 'Not Found.')
+
+    def test_method_not_allowed_keeps_allow_header(self):
+        response = self.client.delete('/measure')
+        self.assertJsonError(response, 405, 'Method Not Allowed.')
+        self.assertIn('POST', response.headers['Allow'])
+
+    def test_unexpected_error_is_logged_and_hidden(self):
+        def crash(req):
+            raise RuntimeError('secret internal detail')
+        self.api.processing.process = crash
+
+        with self.assertLogs(self.api.app.logger, level='ERROR') as logs:
+            response = self.client.post('/measure', json={'temperature': 20})
+
+        self.assertJsonError(response, 500, 'Internal server error.')
+        self.assertNotIn('secret', response.get_data(as_text=True))
+        self.assertIn('secret internal detail', '\n'.join(logs.output))
+
+    def test_existing_json_errors_unchanged(self):
+        self.api.app.config['MAX_CONTENT_LENGTH'] = 10
+        response = self.client.post('/measure', data='x' * 100, content_type='application/json')
+        self.assertJsonError(response, 413, 'Payload too large.')
+
+    def test_index_is_still_html(self):
+        response = self.client.get('/')
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(response.content_type.startswith('text/html'))
+
+
 class TestStaleData(unittest.TestCase):
     """
     Test that /data and /export do not return stale in-memory data.