From fd571c479e96371dd5774a8e9b98ce6f04446647 Mon Sep 17 00:00:00 2001 From: Adriano Dal Pastro Date: Thu, 11 Jun 2026 12:16:29 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20code=20review=20=E2=80=94=20security=20f?= =?UTF-8?q?ixes,=20dedup,=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - setup: validate roles against users.VALID_ROLES (Supervisor was missing) - files: fix path traversal prefix-match edge case (is_relative_to), dedupe path validation into resolve_upload_path(), use logging not print - measurements: extract shared _build_measurement_filters() helper - client app: prevent open redirect via Referer on /set-language - maker: guard resp.json() in parse-technical-sheet proxy - measure/maker: extract shared file proxy into services/file_proxy.py - measure: localize supervisor validation error messages - annotation-editor: remove global keydown listener in destroy() Co-Authored-By: Claude Fable 5 --- src/backend/api/routers/files.py | 86 ++++++++--------- src/backend/api/routers/measurements.py | 93 +++++++++---------- src/backend/api/routers/setup.py | 2 +- src/frontend/flask_app/app.py | 10 +- src/frontend/flask_app/blueprints/maker.py | 26 +++--- src/frontend/flask_app/blueprints/measure.py | 25 ++--- src/frontend/flask_app/services/file_proxy.py | 25 +++++ .../flask_app/static/js/annotation-editor.js | 8 +- 8 files changed, 141 insertions(+), 134 deletions(-) create mode 100644 src/frontend/flask_app/services/file_proxy.py diff --git a/src/backend/api/routers/files.py b/src/backend/api/routers/files.py index 06e4fc2..28cbfa0 100644 --- a/src/backend/api/routers/files.py +++ b/src/backend/api/routers/files.py @@ -1,5 +1,5 @@ """File upload/download/delete router for images and PDFs.""" -import os +import logging import re from pathlib import Path @@ -11,6 +11,8 @@ from src.backend.config import settings from src.backend.api.middleware.api_key import get_current_user, require_maker from src.backend.models.orm.user import User +logger = logging.getLogger(__name__) + router = APIRouter(prefix="/api/files", tags=["files"]) # Allowed MIME types @@ -65,6 +67,37 @@ def sanitize_filename(filename: str) -> str: return filename +def resolve_upload_path(file_path: str) -> Path: + """Resolve a user-supplied relative path inside the uploads directory. + + Raises: + HTTPException: 404 if the path escapes the uploads directory, + cannot be resolved, or does not point to an existing file. + """ + try: + full_path = (settings.upload_path / file_path).resolve() + if not full_path.is_relative_to(settings.upload_path.resolve()): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied", + ) + except HTTPException: + raise + except (OSError, RuntimeError, ValueError): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="File not found", + ) + + if not full_path.exists() or not full_path.is_file(): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="File not found", + ) + + return full_path + + def generate_thumbnail(image_path: Path, thumbnail_path: Path, max_size: tuple[int, int] = (200, 200)): """Generate a thumbnail for an image.""" try: @@ -73,7 +106,7 @@ def generate_thumbnail(image_path: Path, thumbnail_path: Path, max_size: tuple[i img.save(thumbnail_path, quality=85) except Exception as e: # If thumbnail generation fails, we continue without it - print(f"Thumbnail generation failed: {e}") + logger.warning("Thumbnail generation failed for %s: %s", image_path, e) @router.post("/upload") @@ -164,30 +197,7 @@ async def get_file( Requires authentication but no specific role. """ - # Construct full path - full_path = settings.upload_path / file_path - - # Security: ensure path is within uploads directory - try: - full_path = full_path.resolve() - if not str(full_path).startswith(str(settings.upload_path.resolve())): - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Access denied", - ) - except Exception: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="File not found", - ) - - # Check if file exists - if not full_path.exists() or not full_path.is_file(): - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="File not found", - ) - + full_path = resolve_upload_path(file_path) return FileResponse(full_path) @@ -200,29 +210,7 @@ async def delete_file( Also deletes associated thumbnail if it exists. """ - # Construct full path - full_path = settings.upload_path / file_path - - # Security: ensure path is within uploads directory - try: - full_path = full_path.resolve() - if not str(full_path).startswith(str(settings.upload_path.resolve())): - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Access denied", - ) - except Exception: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="File not found", - ) - - # Check if file exists - if not full_path.exists() or not full_path.is_file(): - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="File not found", - ) + full_path = resolve_upload_path(file_path) # Delete thumbnail if it exists if full_path.parent.name != "thumbnails": diff --git a/src/backend/api/routers/measurements.py b/src/backend/api/routers/measurements.py index a3057e9..13ceab0 100644 --- a/src/backend/api/routers/measurements.py +++ b/src/backend/api/routers/measurements.py @@ -84,6 +84,44 @@ async def create_measurement_batch( ) +def _build_measurement_filters( + recipe_id: int | None, + version_id: int | None, + subtask_id: int | None, + measured_by: int | None, + lot_number: str | None, + serial_number: str | None, + date_from: datetime | None, + date_to: datetime | None, + pass_fail: str | None, +) -> list: + """Build SQLAlchemy filter conditions shared by list and CSV export.""" + filters = [] + if recipe_id is not None: + # Filter by recipe via subquery on RecipeVersion + version_ids = select(RecipeVersion.id).where( + RecipeVersion.recipe_id == recipe_id + ) + filters.append(Measurement.version_id.in_(version_ids)) + if version_id is not None: + filters.append(Measurement.version_id == version_id) + if subtask_id is not None: + filters.append(Measurement.subtask_id == subtask_id) + if measured_by is not None: + filters.append(Measurement.measured_by == measured_by) + if lot_number is not None: + filters.append(Measurement.lot_number == lot_number) + if serial_number is not None: + filters.append(Measurement.serial_number == serial_number) + if date_from is not None: + filters.append(Measurement.measured_at >= date_from) + if date_to is not None: + filters.append(Measurement.measured_at <= date_to) + if pass_fail is not None: + filters.append(Measurement.pass_fail == pass_fail) + return filters + + @router.get("/", response_model=MeasurementListResponse) async def get_measurements( recipe_id: int | None = Query(None), @@ -108,30 +146,10 @@ async def get_measurements( filters. Measurements carry no PII beyond numeric values + a recipe reference, so role-gating beyond authentication isn't justified. """ - # Build filter conditions - filters = [] - if recipe_id is not None: - # Filter by recipe via subquery on RecipeVersion - version_ids = select(RecipeVersion.id).where( - RecipeVersion.recipe_id == recipe_id - ) - filters.append(Measurement.version_id.in_(version_ids)) - if version_id is not None: - filters.append(Measurement.version_id == version_id) - if subtask_id is not None: - filters.append(Measurement.subtask_id == subtask_id) - if measured_by is not None: - filters.append(Measurement.measured_by == measured_by) - if lot_number is not None: - filters.append(Measurement.lot_number == lot_number) - if serial_number is not None: - filters.append(Measurement.serial_number == serial_number) - if date_from is not None: - filters.append(Measurement.measured_at >= date_from) - if date_to is not None: - filters.append(Measurement.measured_at <= date_to) - if pass_fail is not None: - filters.append(Measurement.pass_fail == pass_fail) + filters = _build_measurement_filters( + recipe_id, version_id, subtask_id, measured_by, lot_number, + serial_number, date_from, date_to, pass_fail, + ) # Build query query = select(Measurement).where(and_(*filters) if filters else True) @@ -196,29 +214,10 @@ async def export_measurements_csv( decimal_setting = decimal_result.scalar_one_or_none() decimal_separator = decimal_setting.setting_value if decimal_setting else "." - # Build filter conditions (same as get_measurements) - filters = [] - if recipe_id is not None: - version_ids = select(RecipeVersion.id).where( - RecipeVersion.recipe_id == recipe_id - ) - filters.append(Measurement.version_id.in_(version_ids)) - if version_id is not None: - filters.append(Measurement.version_id == version_id) - if subtask_id is not None: - filters.append(Measurement.subtask_id == subtask_id) - if measured_by is not None: - filters.append(Measurement.measured_by == measured_by) - if lot_number is not None: - filters.append(Measurement.lot_number == lot_number) - if serial_number is not None: - filters.append(Measurement.serial_number == serial_number) - if date_from is not None: - filters.append(Measurement.measured_at >= date_from) - if date_to is not None: - filters.append(Measurement.measured_at <= date_to) - if pass_fail is not None: - filters.append(Measurement.pass_fail == pass_fail) + filters = _build_measurement_filters( + recipe_id, version_id, subtask_id, measured_by, lot_number, + serial_number, date_from, date_to, pass_fail, + ) # Query all matching measurements query = select(Measurement).where(and_(*filters) if filters else True) diff --git a/src/backend/api/routers/setup.py b/src/backend/api/routers/setup.py index c0f1246..b272a38 100644 --- a/src/backend/api/routers/setup.py +++ b/src/backend/api/routers/setup.py @@ -15,6 +15,7 @@ from pydantic import BaseModel from sqlalchemy import inspect as sa_inspect, select from sqlalchemy.ext.asyncio import AsyncSession +from src.backend.api.routers.users import VALID_ROLES from src.backend.config import settings from src.backend.database import Base, engine, async_session_factory from src.backend.models.orm import ( @@ -544,7 +545,6 @@ async def manage_user(body: ManageUserBody): """Create or update a user. user_id=null creates new, user_id=int updates.""" _check_password(body.password) - VALID_ROLES = {"Maker", "MeasurementTec", "Metrologist"} invalid = set(body.roles) - VALID_ROLES if invalid: raise HTTPException( diff --git a/src/frontend/flask_app/app.py b/src/frontend/flask_app/app.py index 20cc278..1b1ddd9 100644 --- a/src/frontend/flask_app/app.py +++ b/src/frontend/flask_app/app.py @@ -1,6 +1,7 @@ """TieMeasureFlow Client - Flask Entry Point.""" import json import os +from urllib.parse import urlparse from flask import Flask, redirect, url_for, session, request from flask_babel import Babel @@ -63,7 +64,14 @@ def create_app() -> Flask: """Set user's preferred language and store in session.""" if lang in Config.LANGUAGES: session["language"] = lang - return redirect(request.referrer or url_for("auth.login")) + # Only follow the referrer if it points back to this host + # (prevents open redirect via a forged Referer header). + referrer = request.referrer + if referrer: + parsed = urlparse(referrer) + if parsed.netloc and parsed.netloc != request.host: + referrer = None + return redirect(referrer or url_for("auth.login")) @app.template_filter("tojson_attr") def tojson_attr_filter(value): diff --git a/src/frontend/flask_app/blueprints/maker.py b/src/frontend/flask_app/blueprints/maker.py index 769d54f..9932978 100644 --- a/src/frontend/flask_app/blueprints/maker.py +++ b/src/frontend/flask_app/blueprints/maker.py @@ -1,12 +1,13 @@ """Maker blueprint - recipe creation and editing.""" import requests as http_requests -from flask import Blueprint, Response, flash, jsonify, redirect, render_template, request, session, url_for +from flask import Blueprint, flash, jsonify, redirect, render_template, request, session, url_for from flask_babel import gettext as _ from blueprints.auth import login_required, role_required from config import Config from services.api_client import api_client +from services.file_proxy import proxy_file maker_bp = Blueprint("maker", __name__, url_prefix="/maker") @@ -380,19 +381,7 @@ def api_upload_file(): @login_required def api_get_file(file_path: str): """Proxy: Serve file from API server (browser can't send X-API-Key).""" - api_key = session.get("api_key", "") - base_url = Config.API_SERVER_URL.rstrip("/") - resp = http_requests.get( - f"{base_url}/api/files/{file_path}", - headers={"X-API-Key": api_key}, - timeout=30, - ) - if resp.status_code != 200: - return Response(resp.text, status=resp.status_code) - return Response( - resp.content, - content_type=resp.headers.get("content-type", "application/octet-stream"), - ) + return proxy_file(file_path) @maker_bp.route("/api/files/", methods=["DELETE"]) @@ -444,4 +433,11 @@ def api_parse_technical_sheet(recipe_id: int): timeout=90, ) - return jsonify(resp.json()), resp.status_code + try: + payload = resp.json() + except ValueError: + payload = { + "error": True, + "detail": resp.text or f"HTTP {resp.status_code}", + } + return jsonify(payload), resp.status_code diff --git a/src/frontend/flask_app/blueprints/measure.py b/src/frontend/flask_app/blueprints/measure.py index 8874c03..29d5ae4 100644 --- a/src/frontend/flask_app/blueprints/measure.py +++ b/src/frontend/flask_app/blueprints/measure.py @@ -1,8 +1,6 @@ """MeasurementTec blueprint - recipe selection and measurement execution.""" -import requests as http_requests - from flask import ( - Blueprint, Response, flash, jsonify, redirect, render_template, + Blueprint, flash, jsonify, redirect, render_template, request, session, url_for, ) from flask_babel import gettext as _ @@ -10,6 +8,7 @@ from flask_babel import gettext as _ from blueprints.auth import login_required, role_required from config import Config from services.api_client import api_client +from services.file_proxy import proxy_file measure_bp = Blueprint("measure", __name__) @@ -339,17 +338,17 @@ def validate_supervisor(): password = data.get("password", "") if not username or not password: - return jsonify({"error": True, "detail": "Username e password richiesti"}), 400 + return jsonify({"error": True, "detail": _("Username e password richiesti")}), 400 resp = api_client.post("/api/auth/login", data={"username": username, "password": password}) if resp.get("error"): - return jsonify({"error": True, "detail": "Credenziali non valide"}), 401 + return jsonify({"error": True, "detail": _("Credenziali non valide")}), 401 user = resp.get("user", {}) is_supervisor = "Supervisor" in (user.get("roles") or []) if not (is_supervisor or user.get("is_admin")): - return jsonify({"error": True, "detail": "Utente non autorizzato (richiesto capoturno)"}), 403 + return jsonify({"error": True, "detail": _("Utente non autorizzato (richiesto capoturno)")}), 403 return jsonify({"authorized": True, "supervisor": user.get("display_name", username)}), 200 @@ -361,16 +360,4 @@ def validate_supervisor(): @login_required def api_get_file(file_path: str): """Proxy: Serve file from API server (browser can't send X-API-Key).""" - api_key = session.get("api_key", "") - base_url = Config.API_SERVER_URL.rstrip("/") - resp = http_requests.get( - f"{base_url}/api/files/{file_path}", - headers={"X-API-Key": api_key}, - timeout=30, - ) - if resp.status_code != 200: - return Response(resp.text, status=resp.status_code) - return Response( - resp.content, - content_type=resp.headers.get("content-type", "application/octet-stream"), - ) + return proxy_file(file_path) diff --git a/src/frontend/flask_app/services/file_proxy.py b/src/frontend/flask_app/services/file_proxy.py new file mode 100644 index 0000000..8c31936 --- /dev/null +++ b/src/frontend/flask_app/services/file_proxy.py @@ -0,0 +1,25 @@ +"""Shared file proxy: relay uploads from the FastAPI server to the browser. + +The browser can't send the X-API-Key header directly, so blueprints expose +a proxy route and delegate here. +""" +import requests as http_requests +from flask import Response, session + +from config import Config + + +def proxy_file(file_path: str) -> Response: + """Fetch a file from the API server and relay it with its content type.""" + base_url = Config.API_SERVER_URL.rstrip("/") + resp = http_requests.get( + f"{base_url}/api/files/{file_path}", + headers={"X-API-Key": session.get("api_key", "")}, + timeout=30, + ) + if resp.status_code != 200: + return Response(resp.text, status=resp.status_code) + return Response( + resp.content, + content_type=resp.headers.get("content-type", "application/octet-stream"), + ) diff --git a/src/frontend/flask_app/static/js/annotation-editor.js b/src/frontend/flask_app/static/js/annotation-editor.js index 27cfd5d..7bddbe5 100644 --- a/src/frontend/flask_app/static/js/annotation-editor.js +++ b/src/frontend/flask_app/static/js/annotation-editor.js @@ -956,7 +956,7 @@ function annotationEditor() { setupKeyboard() { var self = this; - document.addEventListener('keydown', function (e) { + this._onKeyDown = function (e) { // Skip when typing in form fields var tag = e.target.tagName; if (tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT') return; @@ -970,7 +970,8 @@ function annotationEditor() { self.canvas.discardActiveObject(); self.canvas.renderAll(); } - }); + }; + document.addEventListener('keydown', this._onKeyDown); }, // ---------------------------------------------------------------- @@ -1289,6 +1290,9 @@ function annotationEditor() { if (this._onImageLoaded) { window.removeEventListener('image-loaded', this._onImageLoaded); } + if (this._onKeyDown) { + document.removeEventListener('keydown', this._onKeyDown); + } if (this.canvas) { this.canvas.dispose(); this.canvas = null;