From 001cf6807cb9f580392febc2ead8ddd4ebb7a69e Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 31 Mar 2026 11:28:42 +0200 Subject: [PATCH] Fix audit findings: security, robustness, and dead code - Remove dead get_analysis_service() function in main.py - Scope file deletion to UPLOAD_DIR to prevent path traversal - Parse datetime strings back to datetime objects in repos - Add 10-minute polling timeout in frontend analysis store - Accept .pdf extension (not just MIME type) on drag-and-drop - Guard localStorage access for private browsing compatibility Co-Authored-By: Claude Opus 4.6 --- document-parser/main.py | 7 +----- document-parser/persistence/analysis_repo.py | 15 +++++++++--- document-parser/persistence/document_repo.py | 7 +++++- document-parser/services/document_service.py | 10 +++++--- frontend/src/features/analysis/store.ts | 13 ++++++++++ .../features/document/ui/DocumentUpload.vue | 2 +- frontend/src/features/settings/store.ts | 24 +++++++++++++++---- 7 files changed, 60 insertions(+), 18 deletions(-) diff --git a/document-parser/main.py b/document-parser/main.py index 2a8e305..20a5478 100644 --- a/document-parser/main.py +++ b/document-parser/main.py @@ -14,7 +14,7 @@ from __future__ import annotations import logging from contextlib import asynccontextmanager -from fastapi import FastAPI, Request +from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware from api.analyses import router as analyses_router @@ -89,11 +89,6 @@ app.include_router(documents_router) app.include_router(analyses_router) -def get_analysis_service(request: Request) -> AnalysisService: - """FastAPI dependency — retrieve the AnalysisService from app.state.""" - return request.app.state.analysis_service - - @app.get("/health") def health(): """Health check endpoint.""" diff --git a/document-parser/persistence/analysis_repo.py b/document-parser/persistence/analysis_repo.py index a16367d..f8b3df7 100644 --- a/document-parser/persistence/analysis_repo.py +++ b/document-parser/persistence/analysis_repo.py @@ -2,10 +2,19 @@ from __future__ import annotations +from datetime import datetime + from domain.models import AnalysisJob, AnalysisStatus from persistence.database import get_connection +def _parse_dt(value: str | None) -> datetime | None: + """Parse an ISO-format datetime string back into a datetime object.""" + if not value: + return None + return datetime.fromisoformat(value) + + def _row_to_job(row) -> AnalysisJob: return AnalysisJob( id=row["id"], @@ -15,9 +24,9 @@ def _row_to_job(row) -> AnalysisJob: content_html=row["content_html"], pages_json=row["pages_json"], error_message=row["error_message"], - started_at=row["started_at"], - completed_at=row["completed_at"], - created_at=row["created_at"], + started_at=_parse_dt(row["started_at"]), + completed_at=_parse_dt(row["completed_at"]), + created_at=_parse_dt(row["created_at"]) or datetime.now(), document_filename=row["filename"] if "filename" in row.keys() else None, # noqa: SIM118 ) diff --git a/document-parser/persistence/document_repo.py b/document-parser/persistence/document_repo.py index 3649dc1..868b24d 100644 --- a/document-parser/persistence/document_repo.py +++ b/document-parser/persistence/document_repo.py @@ -2,11 +2,16 @@ from __future__ import annotations +from datetime import datetime + from domain.models import Document from persistence.database import get_connection def _row_to_document(row) -> Document: + created = row["created_at"] + if isinstance(created, str): + created = datetime.fromisoformat(created) return Document( id=row["id"], filename=row["filename"], @@ -14,7 +19,7 @@ def _row_to_document(row) -> Document: file_size=row["file_size"], page_count=row["page_count"], storage_path=row["storage_path"], - created_at=row["created_at"], + created_at=created, ) diff --git a/document-parser/services/document_service.py b/document-parser/services/document_service.py index f36825d..cba3d5c 100644 --- a/document-parser/services/document_service.py +++ b/document-parser/services/document_service.py @@ -70,10 +70,14 @@ async def delete(doc_id: str) -> bool: # Delete associated analyses first (cascade) await analysis_repo.delete_by_document(doc_id) - # Delete file from disk + # Delete file from disk (only if inside UPLOAD_DIR) try: - if os.path.exists(doc.storage_path): - os.unlink(doc.storage_path) + real_path = os.path.realpath(doc.storage_path) + real_upload_dir = os.path.realpath(UPLOAD_DIR) + if real_path.startswith(real_upload_dir + os.sep) and os.path.exists(real_path): + os.unlink(real_path) + elif os.path.exists(doc.storage_path): + logger.warning("Refused to delete file outside upload dir: %s", doc.storage_path) except OSError: logger.warning("Could not delete file: %s", doc.storage_path) diff --git a/frontend/src/features/analysis/store.ts b/frontend/src/features/analysis/store.ts index ee3c548..d66dac5 100644 --- a/frontend/src/features/analysis/store.ts +++ b/frontend/src/features/analysis/store.ts @@ -9,6 +9,8 @@ export const useAnalysisStore = defineStore('analysis', () => { const running = ref(false) const error = ref(null) const pollingInterval = ref | null>(null) + const pollingTimeout = ref | null>(null) + const MAX_POLLING_DURATION = 10 * 60 * 1000 // 10 minutes const currentPages = computed(() => { if (!currentAnalysis.value?.pagesJson) return [] @@ -72,6 +74,13 @@ export const useAnalysisStore = defineStore('analysis', () => { running.value = false } }, 2000) + pollingTimeout.value = setTimeout(() => { + if (pollingInterval.value) { + error.value = 'Analysis timed out' + stopPolling() + running.value = false + } + }, MAX_POLLING_DURATION) } function stopPolling(): void { @@ -79,6 +88,10 @@ export const useAnalysisStore = defineStore('analysis', () => { clearInterval(pollingInterval.value) pollingInterval.value = null } + if (pollingTimeout.value) { + clearTimeout(pollingTimeout.value) + pollingTimeout.value = null + } } async function select(id: string): Promise { diff --git a/frontend/src/features/document/ui/DocumentUpload.vue b/frontend/src/features/document/ui/DocumentUpload.vue index f5e3c0b..26b664c 100644 --- a/frontend/src/features/document/ui/DocumentUpload.vue +++ b/frontend/src/features/document/ui/DocumentUpload.vue @@ -57,7 +57,7 @@ async function onFileSelect(e: Event) { async function onDrop(e: DragEvent) { dragging.value = false const file = e.dataTransfer?.files?.[0] - if (file && file.type === 'application/pdf') { + if (file && (file.type === 'application/pdf' || file.name.toLowerCase().endsWith('.pdf'))) { try { store.clearError() const doc = await store.upload(file) diff --git a/frontend/src/features/settings/store.ts b/frontend/src/features/settings/store.ts index 6cc7193..6123e12 100644 --- a/frontend/src/features/settings/store.ts +++ b/frontend/src/features/settings/store.ts @@ -2,13 +2,29 @@ import { defineStore } from 'pinia' import { ref, watch, watchEffect } from 'vue' import type { Locale, Theme } from '../../shared/types' +function safeGetItem(key: string): string | null { + try { + return localStorage.getItem(key) + } catch { + return null + } +} + +function safeSetItem(key: string, value: string): void { + try { + localStorage.setItem(key, value) + } catch { + // localStorage unavailable (private browsing, quota exceeded) + } +} + export const useSettingsStore = defineStore('settings', () => { const apiUrl = ref('http://localhost:8000') - const theme = ref((localStorage.getItem('docling-theme') as Theme) || 'dark') - const locale = ref((localStorage.getItem('docling-locale') as Locale) || 'fr') + const theme = ref((safeGetItem('docling-theme') as Theme) || 'dark') + const locale = ref((safeGetItem('docling-locale') as Locale) || 'fr') - watch(theme, (v) => localStorage.setItem('docling-theme', v)) - watch(locale, (v) => localStorage.setItem('docling-locale', v)) + watch(theme, (v) => safeSetItem('docling-theme', v)) + watch(locale, (v) => safeSetItem('docling-locale', v)) watchEffect(() => { document.documentElement.classList.toggle('light', theme.value === 'light')