diff --git a/.jules/sentinel.md b/.jules/sentinel.md index dd9abd56..2b5d819c 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -42,6 +42,11 @@ **Learning:** For large file uploads, loading the entire payload into a single Python object (even just to process or save it) creates a bottleneck where large chunks of contiguous memory are required simultaneously. The Strix security scanner will flag this as a Resource Exhaustion Vulnerability ("security theater") if you attempt to just bound a single `file.read()`. **Prevention:** Stream the chunks (e.g. 8192 bytes) directly to a `NamedTemporaryFile` on disk while verifying the accumulation does not exceed the maximum allowed payload size. Ensure the temporary file is securely unlinked in a `finally` block or when an upload limit exception is raised. +## 2026-07-09 - Keep upload cleanup non-fatal and observable +**Vulnerability:** Temporary-file cleanup can fail after a successful parse because of filesystem races, antivirus locks, or platform-specific deletion semantics. If cleanup exceptions are allowed to propagate, a successful parse can become a 500 while still leaving unclear forensic evidence. +**Learning:** Cleanup must be guaranteed on all upload paths, but cleanup failure handling should be isolated from the user-facing parse result and logged with enough context for operators to see why disk hygiene failed. +**Prevention:** Run upload temporary-file unlinking in the endpoint `finally` block, catch `OSError`, and log the temporary path at exception level without exposing it in public API responses. + ## 2025-02-28 - [Subprocess argument injection via newlines] **Vulnerability:** Unsanitized user inputs containing newline (`\n`) and carriage return (`\r`) characters passed as arguments to subprocesses can lead to command and log injection vulnerabilities, even when `shell=False` is used, depending on how downstream CLI tools process the inputs. **Learning:** Standard shell metacharacter filters (like `[\0&;|`$<>]`) are insufficient to prevent injection if they omit whitespace control characters. Attackers can inject newlines to manipulate tool behavior or spoof log entries if the downstream executable processes inputs line-by-line or uses them in script evaluation. diff --git a/AGENTS.md b/AGENTS.md index 39b36e2d..1f232d36 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -184,4 +184,3 @@ follow it. (e.g. layout detection, reading-order recovery, table structure recognition). - diff --git a/src/newsdom_api/main.py b/src/newsdom_api/main.py index cd1a315d..3e412793 100644 --- a/src/newsdom_api/main.py +++ b/src/newsdom_api/main.py @@ -156,7 +156,7 @@ async def parse( if file_size is not None and file_size > MAX_PARSE_UPLOAD_BYTES: raise HTTPException(status_code=413, detail=PAYLOAD_TOO_LARGE_DETAIL) - tmp_path = None + tmp_path: Path | None = None try: header = await file.read(5) if header != b"%PDF-": @@ -167,17 +167,22 @@ async def parse( with tempfile.NamedTemporaryFile(delete=False) as tmp: tmp_path = Path(tmp.name) + LOGGER.debug("Created temporary upload file %s", tmp_path) tmp.write(header) bytes_read = len(header) while chunk := await file.read(8192): bytes_read += len(chunk) if bytes_read > MAX_PARSE_UPLOAD_BYTES: + LOGGER.warning( + "Rejecting upload over limit: %s bytes read", bytes_read + ) raise HTTPException( status_code=413, detail=PAYLOAD_TOO_LARGE_DETAIL ) tmp.write(chunk) + LOGGER.debug("Wrote %s upload bytes to %s", bytes_read, tmp_path) _validate_pdf_structure(tmp_path) return await asyncio.to_thread( parse_pdf, tmp_path, filename=file.filename or "upload.pdf" @@ -188,5 +193,8 @@ async def parse( except MineruIncompleteOutputError: raise HTTPException(status_code=502, detail="Bad Gateway") from None finally: - if tmp_path and tmp_path.exists(): - tmp_path.unlink(missing_ok=True) + if tmp_path is not None: + try: + tmp_path.unlink(missing_ok=True) + except OSError: + LOGGER.exception("Failed to remove temporary upload file %s", tmp_path) diff --git a/src/newsdom_api/mineru_runner.py b/src/newsdom_api/mineru_runner.py index 9846cc13..a2a91392 100644 --- a/src/newsdom_api/mineru_runner.py +++ b/src/newsdom_api/mineru_runner.py @@ -68,9 +68,11 @@ def _resolve_mineru_bin() -> str: return configured found = _cached_which("mineru") if not found: - raise FileNotFoundError( - "Could not find 'mineru' executable. " - "Ensure it is installed and on the PATH, or set NEWSDOM_MINERU_BIN." + raise MineruRuntimeUnavailableError( + stderr=( + "Could not find 'mineru' executable. " + "Ensure it is installed and on the PATH, or set NEWSDOM_MINERU_BIN." + ) ) return found diff --git a/tests/test_mineru_runner_paths.py b/tests/test_mineru_runner_paths.py index bddde1d9..427681b4 100644 --- a/tests/test_mineru_runner_paths.py +++ b/tests/test_mineru_runner_paths.py @@ -61,9 +61,13 @@ def test_resolve_mineru_bin_rechecks_env_after_cached_lookup(monkeypatch): def test_resolve_mineru_bin_raises_when_not_found(monkeypatch): monkeypatch.delenv("NEWSDOM_MINERU_BIN", raising=False) monkeypatch.setattr(mineru_runner.shutil, "which", lambda name: None) - with pytest.raises(FileNotFoundError): + with pytest.raises(MineruRuntimeUnavailableError) as exc_info: mineru_runner._resolve_mineru_bin() + assert exc_info.value.returncode is None + assert "Could not find 'mineru' executable" in (exc_info.value.stderr or "") + _assert_no_private_path_material(str(exc_info.value)) + def test_find_output_dir_raises_when_missing(tmp_path: Path): with pytest.raises(FileNotFoundError): diff --git a/tests/test_parse_endpoint.py b/tests/test_parse_endpoint.py index be6177b0..3bd5a40c 100644 --- a/tests/test_parse_endpoint.py +++ b/tests/test_parse_endpoint.py @@ -190,6 +190,30 @@ def fake_parse_pdf_bytes(file_path, filename): assert response.status_code == 200 +def test_parse_endpoint_logs_tempfile_cleanup_failure(monkeypatch, caplog): + def fake_parse_pdf_bytes(file_path, filename): + return {"document_id": "fixture", "pages": []} + + def failing_unlink(self, missing_ok=False): + raise OSError("locked temp file") + + caplog.set_level("ERROR", logger="newsdom_api") + monkeypatch.setattr("newsdom_api.main._validate_pdf_structure", lambda _: None) + monkeypatch.setattr("newsdom_api.main.parse_pdf", fake_parse_pdf_bytes) + monkeypatch.setattr(Path, "unlink", failing_unlink) + + client = TestClient(app) + response = client.post( + "/parse", + files={"file": ("fixture.pdf", b"%PDF-1.4\n%synthetic\n", "application/pdf")}, + ) + + assert response.status_code == 200 + assert response.json()["document_id"] == "fixture" + assert response.json()["pages"] == [] + assert "Failed to remove temporary upload file" in caplog.text + + def test_parse_endpoint_returns_503_for_mineru_runtime_failure(monkeypatch): def fake_run(cmd, check, capture_output, text, timeout=None, shell=False): assert check is True