From 71c1afda47af976202ba21ec35b5d66457105d1c Mon Sep 17 00:00:00 2001 From: scott2b Date: Mon, 11 May 2026 16:41:29 -0500 Subject: [PATCH] Fix AJAX endpoints returning HTML instead of JSON on transient errors Users intermittently see "Unexpected token '<', --- deploy/templates/storymap.conf | 1 + deploy/templates/storymapjs.service.j2 | 3 ++ docker-compose.yml | 2 +- storymap/api.py | 60 +++++++++++++++++++-- storymap/static/js/editor.js | 39 +++++++++++++- tests/unit_tests.py | 72 ++++++++++++++++++++++++++ 6 files changed, 170 insertions(+), 7 deletions(-) diff --git a/deploy/templates/storymap.conf b/deploy/templates/storymap.conf index 1fb34a33..6c740cac 100644 --- a/deploy/templates/storymap.conf +++ b/deploy/templates/storymap.conf @@ -56,6 +56,7 @@ server { proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_read_timeout 95s; } {% endif %} } diff --git a/deploy/templates/storymapjs.service.j2 b/deploy/templates/storymapjs.service.j2 index 7551c8ad..ebb236d2 100644 --- a/deploy/templates/storymapjs.service.j2 +++ b/deploy/templates/storymapjs.service.j2 @@ -14,6 +14,7 @@ EnvironmentFile={{ env_file }} # Never reaches the OOM threshold. Jitter prevents all 3 workers from recycling at the same instant. ExecStart={{ virtualenv }}/bin/gunicorn -b :{{ service_port }} \ --workers 3 \ + --timeout 90 \ --max-requests 200 \ --max-requests-jitter 50 {{ wsgi_application }} @@ -32,6 +33,8 @@ MemoryMax=2000M # Why these numbers +# - --timeout 90 — S3 read_timeout is 60s with up to 5 retry attempts. 90s gives retries room to complete +# without gunicorn killing the worker. nginx proxy_read_timeout is 95s to match. # - --max-requests 200 — at ~5–10 MB worker drift per heavy save, 200 requests stays well under the 2 GB ceiling # even in the worst case. Tune down to 100 if drift turns out worse. # - MemoryMax=2000M — 3 workers × 2 GB = 6 GB cap, which exceeds the 3.7 GB box, so this never bites under normal diff --git a/docker-compose.yml b/docker-compose.yml index 06682123..f2bafda7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -22,7 +22,7 @@ services: REDIS_HOST: redis RABBITMQ_HOST: rabbitmq env_file: .env - command: gunicorn -b :5000 --workers 1 --reload --log-level debug storymap.core.wsgi:application --keyfile KnightLabRootCA.key --certfile KnightLabRootCA.crt + command: gunicorn -b :5000 --workers 1 --timeout 90 --reload --log-level debug storymap.core.wsgi:application --keyfile KnightLabRootCA.key --certfile KnightLabRootCA.crt depends_on: - pg #- localstack diff --git a/storymap/api.py b/storymap/api.py index 7867c298..bed17279 100644 --- a/storymap/api.py +++ b/storymap/api.py @@ -95,6 +95,34 @@ def https_redirect(): ''' +@app.errorhandler(401) +def handle_401(e): + if _is_ajax_request(): + return jsonify({ + 'error': 'Your session has expired. Please log in again.|' + 'Reload this page to sign back in.' + }), 401 + return redirect(url_for('select')) + +@app.errorhandler(403) +def handle_403(e): + if _is_ajax_request(): + return jsonify({ + 'error': 'You do not have permission to perform this action.' + }), 403 + return redirect(url_for('select')) + +@app.errorhandler(500) +def handle_500(e): + app.logger.error("Unhandled 500 error: %s", e) + if _is_ajax_request(): + return jsonify({ + 'error': 'A temporary server error occurred. Your changes were not saved.|' + 'Please wait a moment and try again.' + }), 500 + return redirect(url_for('select')) + + @app.context_processor def inject_urls(): """ @@ -154,6 +182,11 @@ def _request_wants_json(): request.accept_mimetypes[best] > \ request.accept_mimetypes['text/html'] +def _is_ajax_request(): + if request.headers.get('X-Requested-With') == 'XMLHttpRequest': + return True + return _request_wants_json() + def _jsonify(*args, **kwargs): """Convert to JSON""" return app.response_class(json.dumps(dict(*args, **kwargs), cls=APIEncoder), @@ -319,7 +352,13 @@ def google_auth_verify(): def get_session_user(): """Enforce authenticated user""" uid = session.get('uid') - user = get_user(uid, db=db()) + if not uid: + return None + try: + user = get_user(uid, db=db()) + except Exception as e: + app.logger.error("get_session_user: DB error for uid=%s: %s", uid, e) + raise if not user: try: session.pop('uid') @@ -341,8 +380,21 @@ def require_user(f): """ @wraps(f) def decorated_function(*args, **kwargs): - user = get_session_user() + try: + user = get_session_user() + except Exception as e: + traceback.print_exc() + if _is_ajax_request(): + return jsonify({ + 'error': 'A temporary server error occurred. Please try again.' + }), 500 + return redirect(url_for('select')) if user is None: + if _is_ajax_request(): + return jsonify({ + 'error': 'Your session has expired. Please log in again.|' + 'Reload this page to sign back in.' + }), 401 return redirect(url_for('select')) request.user = user kwargs['user'] = user @@ -358,7 +410,7 @@ def require_user_id(template=None): def decorator(f): @wraps(f) def decorated_function(*args, **kwargs): - user = get_session_user() + user = request.user id = _request_get_required('id') if id not in user['storymaps']: error = 'You do not have permission to access to this StoryMap' @@ -370,7 +422,6 @@ def decorated_function(*args, **kwargs): return render_template('select.html', user=user, error=error, selector_message=message) else: return jsonify({'error': error}) - request.user = user kwargs['user'] = user kwargs['id'] = id return f(*args, **kwargs) @@ -489,6 +540,7 @@ def storymap_update_meta(user, id): @app.route('/storymap/export/') +@require_user @require_user_id() def storymap_export(user, id): """ diff --git a/storymap/static/js/editor.js b/storymap/static/js/editor.js index ab9ab2d1..b83ae6af 100644 --- a/storymap/static/js/editor.js +++ b/storymap/static/js/editor.js @@ -78,9 +78,44 @@ function _ajax(options, on_error, on_success, on_complete) { cache: false, dataType: 'json', timeout: 45000, // ms - error: function(xhr, status, err) { + error: function(xhr, status, err) { + debug('ajax error', status, xhr.status, err); + + if (status === 'parsererror') { + var responseText = xhr.responseText || ''; + if (responseText.indexOf('entry_login') > -1) { + _error = 'Your session has expired.|Please reload this page to sign back in.'; + } else { + _error = 'A temporary server error occurred. Your changes were not saved.|Please wait a moment and try again.'; + } + on_error(_error); + return; + } + + if (xhr.status === 401) { + try { + var data = JSON.parse(xhr.responseText); + _error = data.error || 'Your session has expired.|Please reload this page to sign back in.'; + } catch(e) { + _error = 'Your session has expired.|Please reload this page to sign back in.'; + } + on_error(_error); + return; + } + + if (xhr.status === 502 || xhr.status === 503 || xhr.status === 504) { + _error = 'The server is temporarily unavailable.|Please wait a moment and try again.'; + on_error(_error); + return; + } + + if (status === 'timeout') { + _error = 'The request timed out.|Please wait a moment and try again.'; + on_error(_error); + return; + } + _error = err || status; - debug('ajax error', _error) on_error(_error); }, success: function(data) { diff --git a/tests/unit_tests.py b/tests/unit_tests.py index 569cfa23..4773421d 100644 --- a/tests/unit_tests.py +++ b/tests/unit_tests.py @@ -113,3 +113,75 @@ def always_fail_save_json(key_name, content): assert body['error_type'] == 'transient failure' assert body['error_attempts'] == 2 assert attempts['count'] == 2 + + +@pytest.mark.unit +def test_require_user_returns_json_401_for_ajax_when_no_session(api_client): + """When session has no valid user and request is AJAX, return JSON 401.""" + client, _ = api_client + with client.session_transaction() as sess: + sess.pop('uid', None) + + response = client.get( + '/storymap/?id=map-123', + headers={'X-Requested-With': 'XMLHttpRequest'}, + ) + assert response.status_code == 401 + body = response.get_json() + assert 'error' in body + assert 'session' in body['error'].lower() + + +@pytest.mark.unit +def test_require_user_redirects_for_non_ajax_when_no_session(api_client): + """When session has no valid user and request is NOT AJAX, redirect to select.""" + client, _ = api_client + with client.session_transaction() as sess: + sess.pop('uid', None) + + response = client.get('/storymap/?id=map-123') + assert response.status_code == 302 + assert 'select' in response.headers['Location'] + + +@pytest.mark.unit +def test_require_user_returns_json_500_for_ajax_on_db_error(api_client, monkeypatch): + """When DB throws during auth check and request is AJAX, return JSON 500.""" + client, _ = api_client + + def failing_get_user(uid, db=None): + raise Exception('connection refused') + + monkeypatch.setattr(api, 'get_user', failing_get_user) + + response = client.get( + '/storymap/?id=map-123', + headers={'X-Requested-With': 'XMLHttpRequest'}, + ) + assert response.status_code == 500 + body = response.get_json() + assert 'error' in body + assert 'temporary' in body['error'].lower() + + +@pytest.mark.unit +def test_is_ajax_request_with_xhr_header(): + """Detect AJAX via X-Requested-With header.""" + with api.app.test_request_context( + '/storymap/', + headers={'X-Requested-With': 'XMLHttpRequest'}, + ): + assert api._is_ajax_request() is True + + +@pytest.mark.unit +def test_is_ajax_request_without_xhr_header(): + """Non-AJAX request should not be detected as AJAX.""" + with api.app.test_request_context('/storymap/'): + assert api._is_ajax_request() is False + + +@pytest.mark.unit +def test_flask_500_handler_returns_json_for_ajax(): + """Flask 500 error handler should be registered and return JSON for AJAX.""" + assert 500 in api.app.error_handler_spec[None]