From ad3dea6a15f0766367428f1fdb3498bda1b2d948 Mon Sep 17 00:00:00 2001 From: GGThed Date: Tue, 11 Aug 2026 19:36:25 -0400 Subject: [PATCH] fix(web): une erreur sur un point JSON ne renvoie plus une page HTML STD-09, trouve en recroisant l'audit anterieur -- celui mene sur le miroir GitHub, jamais repasse depuis qu'on a decouvert que ce n'etait pas la bonne source. Sept gestionnaires d'erreur portaient chacun leur copie d'une liste de prefixes d'URL decidant "JSON ou page HTML". Les copies avaient derive -- trois testaient /users/coach-availability, quatre non -- et toutes manquaient les memes points. Un fetch() qui recoit une page d'erreur HTML leve en la parsant : sur le calendrier, les listes de selections et d'equipes restaient vides, sans message dans la page et sans rien dans le journal. Deux choses apprises en ecrivant le test, aucune n'etait dans le constat. L'approche par prefixe ne pouvait pas etre reparee. Trois des seize vues JSON sont a des chemins qu'aucun prefixe ne distingue des pages HTML voisines -- /matches//toggle-presence/ et ses deux cousins, que les gabarits appellent justement en fetch(). Les vues se declarent donc elles-memes (@json_endpoint, app/api.py), et un test parcourt la carte des URL pour verifier qu'aucune vue appelant jsonify n'a ete oubliee. Et surtout : @login_required n'atteint jamais le gestionnaire 401. Flask-Login intercepte avant et redirige. Les seize points JSON repondaient donc a une session expiree par une 302 vers un formulaire HTML, quoi que dise la liste de prefixes. Reecrire la liste seule aurait eu l'air d'un correctif sans rien changer. Au passage, le message flash de ce gestionnaire etait la seule chaine de l'application qui n'avait jamais ete traduite. Co-Authored-By: Claude Opus 5 --- app/api.py | 47 ++++++ app/app.py | 122 ++++++++++----- app/routes/matches.py | 6 + app/routes/team_matches.py | 3 + app/routes/teams.py | 2 + app/routes/users/availability.py | 9 ++ app/translations/en/LC_MESSAGES/messages.mo | Bin 44630 -> 44716 bytes app/translations/en/LC_MESSAGES/messages.po | 122 +++++++-------- app/translations/fr/LC_MESSAGES/messages.mo | Bin 48881 -> 48985 bytes app/translations/fr/LC_MESSAGES/messages.po | 122 +++++++-------- tests/test_api_error_format.py | 156 ++++++++++++++++++++ 11 files changed, 436 insertions(+), 153 deletions(-) create mode 100644 app/api.py create mode 100644 tests/test_api_error_format.py diff --git a/app/api.py b/app/api.py new file mode 100644 index 0000000..9b1d6b2 --- /dev/null +++ b/app/api.py @@ -0,0 +1,47 @@ +"""Marking the views whose answers — including their failures — are JSON. + +STD-09. Deciding "JSON or HTML page" from the URL path could not work here, +and the audit's own recommendation ("gestion d'erreurs API par préfixe d'URL +codé en dur", fix the prefixes) would not have fixed it either. Three of the +sixteen JSON views sit at paths no prefix can single out: + + /matches//toggle-presence/ + /team-matches//toggle-presence/ + /teams//toggle_status/ + +They are interleaved with the HTML routes of the same blueprints, and the +templates fetch them. Any prefix wide enough to catch them catches every +page of the section with them. + +So the view says so itself. `wants_json_response()` in app.py reads the mark +off the registered view function, and `tests/test_api_error_format.py` walks +the URL map to prove that every view calling `jsonify` carries it — the +mechanism that was missing before was not a better list, it was anything at +all that checked the list. + +Usage — directly under the route decorator, above `login_required`, so the +mark lands on the object the route registers:: + + @matches_bp.route('/api/events') + @json_endpoint + @login_required + def api_events(): + ... +""" + + +def json_endpoint(view): + """Mark a view as answering in JSON, errors included. + + Args: + view: The view function, already wrapped by any decorator below this + one (`login_required` in every current case). + + Returns: + The same object, with the mark set. Nothing is wrapped: an extra + wrapper here would be one more thing between Flask and the view for + no gain, and `functools.wraps` copying `__dict__` is exactly the + detail that would make this fragile. + """ + view.returns_json = True + return view diff --git a/app/app.py b/app/app.py index a3fb45c..8d0a5c3 100644 --- a/app/app.py +++ b/app/app.py @@ -9,7 +9,18 @@ import secrets import markupsafe from dotenv import load_dotenv -from flask import Flask, g, jsonify, redirect, render_template, request, url_for +from flask import ( + Flask, + current_app, + flash, + g, + jsonify, + redirect, + render_template, + request, + url_for, +) +from flask_babel import gettext as _ from flask_cors import CORS from sqlalchemy import text from werkzeug.exceptions import HTTPException @@ -19,6 +30,38 @@ from app.extensions import babel, csrf, db, limiter, login_manager load_dotenv() +#: Fallback for requests that never reached a view: a 404 on an unrouted +#: path has no endpoint to read a mark off, and `/users/api/typo` should +#: still answer a fetch() in JSON. +#: +#: Not the primary mechanism. Seven error handlers each carried their own +#: copy of a list like this (STD-09); the copies had drifted, and all seven +#: were missing the same endpoints. Views now mark themselves — see +#: `app/api.py` for why a prefix list could not have been made correct. +JSON_URL_PREFIXES = ( + '/users/disponibilities', + '/users/coach-availability', + '/users/api/', + '/matches/api/', + '/team-matches/api/', +) + + +def wants_json_response(): + """Whether this request must be answered with JSON rather than an HTML page. + + Three signals, in order of authority: the view said so (`@json_endpoint`), + the path is under a JSON prefix (for requests that matched no view at + all), or the caller asked for JSON and nothing else. + """ + view = current_app.view_functions.get(request.endpoint) if request.endpoint else None + if getattr(view, 'returns_json', False): + return True + if request.path.startswith(JSON_URL_PREFIXES): + return True + accept = request.accept_mimetypes + return accept.best == 'application/json' and not accept.accept_html + def nl2br(value): """Convert newlines to HTML line breaks. @@ -482,6 +525,36 @@ def create_app(config=None): # ========================================================================= # Custom Error Handlers # ========================================================================= + # + # STD-09. The seven handlers below each carried their own copy of a list + # of URL prefixes, and the copies had drifted: three of them checked + # `/users/coach-availability`, four did not. Worse, every copy was + # missing the same five endpoints — `/matches/api/…` and + # `/team-matches/api/…` — so an error on any of those answered a + # `fetch()` with an HTML error page. The browser then failed to parse it + # as JSON and the page simply did nothing: on the calendar, the tryout + # and team selects stayed empty with no message anywhere. A session that + # expired mid-page produced exactly that, because the 401 handler + # redirects to an HTML login form. + # + # The mechanism now lives in wants_json_response() / app/api.py, and a + # test walks the URL map to prove no jsonify-returning view is missed. + # + # The handler below is the one that actually mattered. `@login_required` + # never reaches the 401 handler: Flask-Login intercepts first and calls + # its own unauthorized callback, which redirects. So every one of the + # sixteen JSON endpoints answered an expired session with a 302 to an + # HTML login form, whatever the prefix list said — and the page's + # `fetch()` threw parsing it. Rewriting the prefix list alone would have + # left this untouched and looked like a fix. + @login_manager.unauthorized_handler + def handle_unauthorized(): + """What an unauthenticated request gets: a redirect, or a 401 in JSON.""" + if wants_json_response(): + return jsonify({'error': 'Unauthorized', 'message': 'Your session has expired.'}), 401 + flash(_('Please log in to access this page.'), 'warning') + return redirect(url_for('auth.login')) + @app.errorhandler(400) def bad_request(error): """Handle 400 Bad Request errors. @@ -492,32 +565,21 @@ def create_app(config=None): Returns: Response: Rendered error page or JSON for API requests. """ - if ( - request.path.startswith('/users/disponibilities') - or request.path.startswith('/users/coach-availability') - or request.path.startswith('/users/api/') - ): + if wants_json_response(): return jsonify({'error': 'Bad request', 'message': str(error)}), 400 return render_template('errors/400.html', error=error), 400 @app.errorhandler(401) def unauthorized(error): - """Handle 401 Unauthorized errors. + """Handle an explicit abort(401). - Args: - error: The error object. - - Returns: - Response: Redirect to login for pages, JSON for API. + Rarely reached: `@login_required` is intercepted by Flask-Login + before Flask's error handling, and answered by handle_unauthorized + above. This covers code that aborts with 401 itself, and gives the + same answer — the two used to differ, and the flash message here was + the one string in the application that had never been translated. """ - if request.path.startswith('/users/disponibilities') or request.path.startswith( - '/users/api/' - ): - return jsonify({'error': 'Unauthorized'}), 401 - from flask import flash as _flash - - _flash('Please log in to access this page.', 'warning') - return redirect(url_for('auth.login')) + return handle_unauthorized() @app.errorhandler(403) def forbidden(error): @@ -529,9 +591,7 @@ def create_app(config=None): Returns: Response: Rendered error page or JSON for API requests. """ - if request.path.startswith('/users/disponibilities') or request.path.startswith( - '/users/api/' - ): + if wants_json_response(): return jsonify({'error': 'Forbidden', 'message': str(error)}), 403 return render_template('errors/403.html', error=error), 403 @@ -545,9 +605,7 @@ def create_app(config=None): Returns: Response: Rendered error page or JSON for API requests. """ - if request.path.startswith('/users/disponibilities') or request.path.startswith( - '/users/api/' - ): + if wants_json_response(): return jsonify({'error': 'Not found'}), 404 return render_template('errors/404.html', error=error), 404 @@ -561,9 +619,7 @@ def create_app(config=None): Returns: Response: JSON error for API or rendered page. """ - if request.path.startswith('/users/disponibilities') or request.path.startswith( - '/users/api/' - ): + if wants_json_response(): return jsonify( {'error': 'Too many requests', 'message': 'Please try again later.'} ), 429 @@ -593,9 +649,7 @@ def create_app(config=None): # attacker can use — so showing it costs nothing (OBS-005). request_id = g.get('request_id', '-') - if request.path.startswith('/users/disponibilities') or request.path.startswith( - '/users/api/' - ): + if wants_json_response(): return jsonify( { 'error': 'Internal server error', @@ -615,9 +669,7 @@ def create_app(config=None): Returns: Response: JSON error for API, re-raises for others. """ - if request.path.startswith('/users/disponibilities') or request.path.startswith( - '/users/api/' - ): + if wants_json_response(): return jsonify( {'error': error.name, 'message': error.description, 'code': error.code} ), error.code diff --git a/app/routes/matches.py b/app/routes/matches.py index 9580dec..0e9998d 100644 --- a/app/routes/matches.py +++ b/app/routes/matches.py @@ -11,6 +11,7 @@ from flask_login import current_user, login_required from marshmallow import ValidationError from sqlalchemy.orm import joinedload +from app.api import json_endpoint from app.extensions import db from app.forms import flash_validation_errors, form_payload from app.models import ( @@ -135,6 +136,7 @@ def calendar_window(args): @matches_bp.route('/api/events') +@json_endpoint @login_required def api_events(): """Calendar events for FullCalendar. @@ -264,6 +266,7 @@ def api_events(): @matches_bp.route('/api/events/') +@json_endpoint @login_required def api_events_for_tryout(tryout_id): """API endpoint returning calendar events for a specific tryout.""" @@ -540,6 +543,7 @@ def edit_match(match_id): @matches_bp.route('/api/manageable-tryouts') +@json_endpoint @login_required def api_manageable_tryouts(): """API endpoint returning tryouts the current user can manage.""" @@ -637,6 +641,7 @@ def get_players_available_at_time(date_str, time_str): @matches_bp.route('/api/available_players//