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//