Pre-deploy security/correctness fixes (port log E17)

- 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 <noreply@anthropic.com>
This commit is contained in:
jtricerolph 2026-08-06 14:44:58 +00:00
parent 78744278f8
commit bcc94024e3
15 changed files with 124 additions and 109 deletions

View file

@ -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 \

View file

@ -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)

View file

@ -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
)

View file

@ -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"

View file

@ -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
]

View file

@ -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(

View file

@ -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)

View file

@ -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
}

View file

@ -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]:

View file

@ -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")

View file

@ -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")

View file

@ -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"

View file

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

View file

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

View file

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