From bcc94024e3a8e3257aca7291d2943125d20dfb9e Mon Sep 17 00:00:00 2001 From: jtricerolph Date: Thu, 6 Aug 2026 14:44:58 +0000 Subject: [PATCH] Pre-deploy security/correctness fixes (port log E17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove dead kitchen->KDS internal API (api/internal.py, verify_internal_secret) — KDS reads kitchen_db directly (E16), nothing ever called this endpoint - Add expires_at to dispute_attachments; public attachment links now expire after 30 days instead of staying valid forever (A4) - Add services/upload_validation.py: sniff real file content via python-magic instead of trusting the client-supplied Content-Type header, plus a 20MB cap. Applied across invoices/logbook/food_flags/credit_notes/disputes upload endpoints (A5) — disputes previously had no file-type check at all - Fix nginx client_max_body_size drift (800m -> the plan's intended 20m) Co-Authored-By: Claude Sonnet 5 --- backend/Dockerfile | 1 + backend/api/credit_notes.py | 9 +-- backend/api/disputes.py | 17 ++++- backend/api/food_flags.py | 17 ++--- backend/api/internal.py | 64 ------------------- backend/api/invoices.py | 10 +-- backend/api/logbook.py | 9 ++- backend/api/public.py | 11 +++- backend/auth.py | 8 --- backend/main.py | 4 +- .../add_dispute_attachment_expiry.py | 35 ++++++++++ backend/models/dispute.py | 2 + backend/requirements.txt | 4 ++ backend/services/upload_validation.py | 40 ++++++++++++ frontend/nginx.conf | 2 +- 15 files changed, 124 insertions(+), 109 deletions(-) delete mode 100644 backend/api/internal.py create mode 100644 backend/migrations/add_dispute_attachment_expiry.py create mode 100644 backend/services/upload_validation.py diff --git a/backend/Dockerfile b/backend/Dockerfile index 3f602f4..a931306 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -10,6 +10,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ unixodbc \ unixodbc-dev \ postgresql-client \ + libmagic1 \ && curl -fsSL https://packages.microsoft.com/keys/microsoft.asc \ | gpg --dearmor -o /usr/share/keyrings/microsoft-prod.gpg \ && curl -fsSL https://packages.microsoft.com/config/debian/12/prod.list \ diff --git a/backend/api/credit_notes.py b/backend/api/credit_notes.py index 0cadc2a..9fe738c 100644 --- a/backend/api/credit_notes.py +++ b/backend/api/credit_notes.py @@ -23,6 +23,7 @@ from models.dispute import CreditNote, InvoiceDispute, DisputeStatus, DisputeAct from models.invoice import Invoice from models.supplier import Supplier from services.dispute_archival_service import DisputeArchivalService +from services.upload_validation import read_and_validate_upload router = APIRouter() @@ -83,12 +84,8 @@ async def upload_credit_note( except ValueError: raise HTTPException(status_code=400, detail="Invalid date format. Use YYYY-MM-DD") - # Read file content - file_content = await file.read() - - # Validate file type (should be PDF) - if file.content_type and "pdf" not in file.content_type.lower(): - raise HTTPException(status_code=400, detail="Only PDF files are supported for credit notes") + # Read + validate file content (sniffed, not the client header — A5) + file_content = await read_and_validate_upload(file, {"application/pdf"}) # Save file archival_service = DisputeArchivalService(db, current_user.kitchen_id) diff --git a/backend/api/disputes.py b/backend/api/disputes.py index 012f0a1..62a6f05 100644 --- a/backend/api/disputes.py +++ b/backend/api/disputes.py @@ -28,12 +28,16 @@ from models.dispute import ( from models.invoice import Invoice from models.supplier import Supplier from services.dispute_archival_service import DisputeArchivalService +from services.upload_validation import read_and_validate_upload def generate_public_hash() -> str: """Generate a secure random hash for public attachment links""" return secrets.token_urlsafe(32) # 43 character URL-safe string + +PUBLIC_LINK_EXPIRY_DAYS = 30 # A4 — public attachment links must not live forever + router = APIRouter() @@ -563,8 +567,13 @@ async def upload_dispute_attachment( if not dispute: raise HTTPException(status_code=404, detail="Dispute not found") - # Read file content - file_content = await file.read() + # Read + validate file content (sniffed, not the client header — A5). + # Broad allowlist: photos, PDFs, delivery-note scans and emailed evidence. + allowed_types = { + "image/jpeg", "image/png", "image/webp", "image/heic", + "application/pdf", "message/rfc822", + } + file_content = await read_and_validate_upload(file, allowed_types) file_size = len(file_content) # Save file @@ -579,8 +588,9 @@ async def upload_dispute_attachment( if not success: raise HTTPException(status_code=500, detail=f"Failed to save file: {file_path}") - # Generate public hash for shareable link + # Generate public hash for shareable link (expires — A4) public_hash = generate_public_hash() + expires_at = datetime.utcnow() + timedelta(days=PUBLIC_LINK_EXPIRY_DAYS) # Create attachment record attachment = DisputeAttachment( @@ -592,6 +602,7 @@ async def upload_dispute_attachment( file_size_bytes=file_size, attachment_type=attachment_type, description=description, + expires_at=expires_at, uploaded_by=current_user.id, public_hash=public_hash ) diff --git a/backend/api/food_flags.py b/backend/api/food_flags.py index 3a9c594..0d3185d 100644 --- a/backend/api/food_flags.py +++ b/backend/api/food_flags.py @@ -17,6 +17,7 @@ from sqlalchemy.orm import selectinload from pydantic import BaseModel from database import get_db +from services.upload_validation import read_and_validate_upload from models.user import User from models.food_flag import FoodFlagCategory, FoodFlag, LineItemFlag, RecipeFlag, RecipeFlagOverride, AllergenKeyword, BrakesProductCache from models.ingredient import Ingredient, IngredientFlag, IngredientFlagNone, IngredientFlagDismissal @@ -1983,10 +1984,9 @@ async def scan_label( """OCR a product ingredient label image and suggest allergen flags. For create mode (ingredient doesn't exist yet) — returns raw text + suggestions. """ - # Validate file type + # Validate file type (sniffed from content, not the client header — A5) allowed = {"image/jpeg", "image/png", "image/webp", "image/heic"} - if file.content_type not in allowed: - raise HTTPException(400, f"Unsupported file type: {file.content_type}") + image_bytes = await read_and_validate_upload(file, allowed) # Get Azure credentials settings_result = await db.execute( @@ -1996,9 +1996,6 @@ async def scan_label( if not settings or not settings.azure_endpoint or not settings.azure_key: raise HTTPException(400, "Azure Document Intelligence not configured. Set it up in Settings.") - # Read file content - image_bytes = await file.read() - # OCR with Azure prebuilt-read try: from azure.ai.formrecognizer import DocumentAnalysisClient @@ -2042,10 +2039,9 @@ async def scan_label_for_ingredient( if not ing or ing.kitchen_id != user.kitchen_id: raise HTTPException(404, "Ingredient not found") - # Validate file type + # Validate file type (sniffed from content, not the client header — A5) allowed = {"image/jpeg", "image/png", "image/webp", "image/heic"} - if file.content_type not in allowed: - raise HTTPException(400, f"Unsupported file type: {file.content_type}") + image_bytes = await read_and_validate_upload(file, allowed) # Get Azure credentials settings_result = await db.execute( @@ -2055,9 +2051,6 @@ async def scan_label_for_ingredient( if not settings or not settings.azure_endpoint or not settings.azure_key: raise HTTPException(400, "Azure Document Intelligence not configured. Set it up in Settings.") - # Read file content - image_bytes = await file.read() - # Save label image ext = file.filename.rsplit(".", 1)[-1] if file.filename and "." in file.filename else "jpg" label_dir = f"/app/data/{user.kitchen_id}/labels" diff --git a/backend/api/internal.py b/backend/api/internal.py deleted file mode 100644 index 0d9ba2e..0000000 --- a/backend/api/internal.py +++ /dev/null @@ -1,64 +0,0 @@ -""" -Internal API — endpoints consumed by other stack apps (not public). -nginx denies /kitchen/api/internal/ from the public side; these endpoints -are called directly on the backend port (8000) from within the docker bridge. -""" -from datetime import date -from typing import Optional - -from fastapi import APIRouter, Depends, HTTPException, Query -from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy import select - -from database import get_db -from auth import verify_internal_secret -from models.resos import ResosBooking - -router = APIRouter() - - -@router.get("/api/internal/resos/bookings") -async def internal_resos_bookings( - booking_date: Optional[str] = Query(None, alias="date"), - _: None = Depends(verify_internal_secret), - db: AsyncSession = Depends(get_db), -): - """ - Cached ResOS bookings for a given date — consumed by the KDS app bookings screen. - KDS calls: GET http://10.10.10.110:8000/api/internal/resos/bookings?date=YYYY-MM-DD - Authorization: Bearer {STACK_INTERNAL_SECRET} - """ - try: - target_date = date.fromisoformat(booking_date) if booking_date else date.today() - except (ValueError, TypeError): - raise HTTPException(status_code=400, detail="Invalid date format — use YYYY-MM-DD") - - result = await db.execute( - select(ResosBooking) - .where( - ResosBooking.kitchen_id == 1, - ResosBooking.booking_date == target_date, - ) - .order_by(ResosBooking.booking_time) - ) - bookings = result.scalars().all() - - return [ - { - "id": b.id, - "resos_booking_id": b.resos_booking_id, - "booking_date": b.booking_date.isoformat(), - "booking_time": b.booking_time.strftime("%H:%M"), - "people": b.people, - "status": b.status, - "seating_area": b.seating_area, - "table_name": b.table_name, - "hotel_booking_number": b.hotel_booking_number, - "is_hotel_guest": b.is_hotel_guest, - "is_dbb": b.is_dbb, - "allergies": b.allergies, - "notes": b.notes, - "opening_hour_name": b.opening_hour_name, - } - for b in bookings - ] diff --git a/backend/api/invoices.py b/backend/api/invoices.py index 2f6e16b..fcb8b5a 100644 --- a/backend/api/invoices.py +++ b/backend/api/invoices.py @@ -23,6 +23,7 @@ from auth import get_current_user, require_cap, get_current_user_from_token from ocr.extractor import process_invoice_image from ocr.azure_extractor import parse_pack_size from services.duplicate_detector import DuplicateDetector +from services.upload_validation import read_and_validate_upload router = APIRouter() logger = logging.getLogger(__name__) @@ -526,12 +527,8 @@ async def upload_invoice( db: AsyncSession = Depends(get_db) ): """Upload an invoice image or PDF for OCR processing""" - allowed_types = ["image/jpeg", "image/png", "image/webp", "image/heic", "application/pdf"] - if file.content_type not in allowed_types: - raise HTTPException( - status_code=400, - detail=f"File type not allowed. Allowed: {allowed_types}" - ) + allowed_types = {"image/jpeg", "image/png", "image/webp", "image/heic", "application/pdf"} + content = await read_and_validate_upload(file, allowed_types) ext = file.filename.split(".")[-1] if file.filename else "jpg" filename = f"{uuid.uuid4()}.{ext}" @@ -540,7 +537,6 @@ async def upload_invoice( os.makedirs(os.path.dirname(filepath), exist_ok=True) async with aiofiles.open(filepath, "wb") as f: - content = await file.read() await f.write(content) invoice = Invoice( diff --git a/backend/api/logbook.py b/backend/api/logbook.py index 5a8f114..43e559d 100644 --- a/backend/api/logbook.py +++ b/backend/api/logbook.py @@ -14,6 +14,7 @@ import logging from auth import get_current_user, require_cap from database import get_db +from services.upload_validation import read_and_validate_upload from models.user import User from models.logbook import ( LogbookEntry, LogbookLineItem, LogbookAttachment, @@ -670,10 +671,9 @@ async def upload_attachment( if not entry: raise HTTPException(status_code=404, detail="Entry not found") - # Validate file type - allowed_types = ["image/jpeg", "image/png", "image/heic", "image/webp", "application/pdf"] - if file.content_type not in allowed_types: - raise HTTPException(status_code=400, detail=f"File type {file.content_type} not allowed. Allowed: {allowed_types}") + # Validate file type (sniffed from content, not the client header — A5) + allowed_types = {"image/jpeg", "image/png", "image/heic", "image/webp", "application/pdf"} + content = await read_and_validate_upload(file, allowed_types) # Save file upload_dir = f"/app/attachments/logbook/kitchen_{current_user.kitchen_id}" @@ -684,7 +684,6 @@ async def upload_attachment( file_name = f"entry_{entry_id}_{timestamp}{file_extension}" file_path = f"{upload_dir}/{file_name}" - content = await file.read() with open(file_path, "wb") as f: f.write(content) diff --git a/backend/api/public.py b/backend/api/public.py index 7f92f96..6cc64a2 100644 --- a/backend/api/public.py +++ b/backend/api/public.py @@ -5,6 +5,7 @@ These endpoints are designed for sharing with external parties (e.g., suppliers) via hash-based URLs that don't require login. """ import os +from datetime import datetime from fastapi import APIRouter, HTTPException from fastapi.responses import Response from sqlalchemy.ext.asyncio import AsyncSession @@ -39,6 +40,10 @@ async def get_public_attachment( if not attachment: raise HTTPException(status_code=404, detail="Attachment not found") + # A4 — public links expire; don't serve a stale/leaked link forever + if attachment.expires_at and attachment.expires_at < datetime.utcnow(): + raise HTTPException(status_code=410, detail="This link has expired") + # Get file content content = None @@ -108,11 +113,15 @@ async def get_public_attachment_info( if not attachment: raise HTTPException(status_code=404, detail="Attachment not found") + if attachment.expires_at and attachment.expires_at < datetime.utcnow(): + raise HTTPException(status_code=410, detail="This link has expired") + return { "file_name": attachment.file_name, "file_type": attachment.file_type, "file_size_bytes": attachment.file_size_bytes, "attachment_type": attachment.attachment_type, "description": attachment.description, - "uploaded_at": attachment.uploaded_at.isoformat() if attachment.uploaded_at else None + "uploaded_at": attachment.uploaded_at.isoformat() if attachment.uploaded_at else None, + "expires_at": attachment.expires_at.isoformat() if attachment.expires_at else None } diff --git a/backend/auth.py b/backend/auth.py index 55b9cbd..ca758aa 100644 --- a/backend/auth.py +++ b/backend/auth.py @@ -108,14 +108,6 @@ async def get_admin_user(user=Depends(get_current_user)): return user -def verify_internal_secret(authorization: Optional[str] = Header(None)) -> None: - """Verify STACK_INTERNAL_SECRET for inter-app calls (e.g. KDS bookings feed).""" - if not STACK_INTERNAL_SECRET: - raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Internal secret not configured") - if not authorization or authorization != f"Bearer {STACK_INTERNAL_SECRET}": - raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Invalid internal secret") - - # ─── API KEY AUTH (public/external API) ────────────────────────────────────── def generate_api_key() -> tuple[str, str, str]: diff --git a/backend/main.py b/backend/main.py index 1784b96..c25a359 100644 --- a/backend/main.py +++ b/backend/main.py @@ -20,7 +20,6 @@ from api import ( purchase_orders, cost_distributions, ingredients, recipes, food_flags, event_orders, external, menus, reconciliation, kds_settings, ) -from api.internal import router as internal_router from migrations.add_invoice_features import run_migration from migrations.add_newbook_tables import run_migration as run_newbook_migration @@ -74,6 +73,7 @@ from migrations.add_llm_infrastructure import migrate as run_llm_infrastructure_ from migrations.add_changelog_invoice_link import migrate as run_changelog_invoice_link_migration from migrations.add_sambapos_portion_name import migrate as run_sambapos_portion_name_migration from migrations.add_global_settings_flags import migrate as run_global_settings_flags_migration +from migrations.add_dispute_attachment_expiry import run_migration as run_dispute_attachment_expiry_migration from scheduler import start_scheduler, stop_scheduler @@ -148,6 +148,7 @@ async def lifespan(app: FastAPI): await _run("Changelog invoice link", run_changelog_invoice_link_migration) await _run("SambaPOS portion name", run_sambapos_portion_name_migration) await _run("Global settings flags", run_global_settings_flags_migration) + await _run("Dispute attachment expiry", run_dispute_attachment_expiry_migration) start_scheduler() @@ -197,7 +198,6 @@ app.include_router(event_orders.router, prefix="/api/event-orders", tags=["Event app.include_router(menus.router, prefix="/api/menus", tags=["Menus"]) app.include_router(external.router, prefix="/api/external", tags=["External API"]) app.include_router(kds_settings.router, prefix="/api/kds", tags=["KDS Settings"]) -app.include_router(internal_router) @app.get("/health") diff --git a/backend/migrations/add_dispute_attachment_expiry.py b/backend/migrations/add_dispute_attachment_expiry.py new file mode 100644 index 0000000..a9d504b --- /dev/null +++ b/backend/migrations/add_dispute_attachment_expiry.py @@ -0,0 +1,35 @@ +""" +Migration to add expires_at to dispute_attachments. + +Public attachment links (public_hash — see add_dispute_attachment_public_hash) +were valid forever with no expiry, so a leaked/forwarded supplier link stayed +live indefinitely. See port log A4. +""" +import logging +from sqlalchemy import text +from database import engine + +logger = logging.getLogger(__name__) + + +async def run_migration(): + """Add expires_at column to dispute_attachments""" + async with engine.begin() as conn: + result = await conn.execute(text(""" + SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'dispute_attachments' + AND column_name = 'expires_at' + ); + """)) + exists = result.scalar() + + if not exists: + logger.info("Adding 'expires_at' column to dispute_attachments table") + await conn.execute(text(""" + ALTER TABLE dispute_attachments + ADD COLUMN IF NOT EXISTS expires_at TIMESTAMP; + """)) + logger.info("Successfully added 'expires_at' column") + else: + logger.info("'expires_at' column already exists in dispute_attachments") diff --git a/backend/models/dispute.py b/backend/models/dispute.py index c780cb0..6466e55 100644 --- a/backend/models/dispute.py +++ b/backend/models/dispute.py @@ -178,6 +178,8 @@ class DisputeAttachment(Base): # Public sharing - hash for unauthenticated access (e.g., email to suppliers) public_hash: Mapped[str | None] = mapped_column(String(64), nullable=True, unique=True, index=True) + # Expiry for the public_hash link — A4: links must not stay valid forever + expires_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True) # File storage (following Invoice model pattern) file_storage_location: Mapped[str] = mapped_column(String(20), default="local") # "local" or "nextcloud" diff --git a/backend/requirements.txt b/backend/requirements.txt index 22e8b70..e431248 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -17,6 +17,10 @@ azure-ai-formrecognizer==3.3.0 # Image processing Pillow==10.2.0 +# Real content-type sniffing for uploads (A5) — trusts file bytes, not the +# client-supplied Content-Type header. Needs libmagic1 (see Dockerfile). +python-magic==0.4.27 + # PDF processing (highlighting non-stock items) PyMuPDF==1.23.8 diff --git a/backend/services/upload_validation.py b/backend/services/upload_validation.py new file mode 100644 index 0000000..a577fcd --- /dev/null +++ b/backend/services/upload_validation.py @@ -0,0 +1,40 @@ +""" +Shared upload validation — sniffs the real file content (libmagic) rather +than trusting the client-supplied Content-Type header, and enforces a size +cap. See port log A5: the archive only checked `file.content_type`, which is +attacker-controlled and proves nothing about what's actually in the body. +""" +import magic +from fastapi import HTTPException, UploadFile + +DEFAULT_MAX_BYTES = 20 * 1024 * 1024 # 20 MB — matches nginx client_max_body_size + + +async def read_and_validate_upload( + file: UploadFile, + allowed_mimes: set[str], + max_bytes: int = DEFAULT_MAX_BYTES, +) -> bytes: + """ + Read an UploadFile fully, verify its sniffed MIME type is in + `allowed_mimes`, and enforce `max_bytes`. Returns the file bytes for the + caller to save/process. Raises HTTPException(400) on any failure. + """ + content = await file.read() + + if not content: + raise HTTPException(status_code=400, detail="Empty file") + if len(content) > max_bytes: + raise HTTPException( + status_code=400, + detail=f"File too large — max {max_bytes // (1024 * 1024)}MB", + ) + + sniffed = magic.from_buffer(content, mime=True) + if sniffed not in allowed_mimes: + raise HTTPException( + status_code=400, + detail=f"File content doesn't match an allowed type (detected: {sniffed})", + ) + + return content diff --git a/frontend/nginx.conf b/frontend/nginx.conf index d1baa23..22b5d7f 100644 --- a/frontend/nginx.conf +++ b/frontend/nginx.conf @@ -30,7 +30,7 @@ server { proxy_set_header Cookie $http_cookie; proxy_read_timeout 600s; proxy_connect_timeout 30s; - client_max_body_size 800m; + client_max_body_size 20m; } # Health