From 6d5d77950406a82929bfa62b2dff0afcc6c33016 Mon Sep 17 00:00:00 2001 From: Robin Windey Date: Wed, 19 Aug 2026 20:38:57 +0000 Subject: [PATCH 1/6] feat: Implement OCR parameter validation and error handling --- README.md | 9 +++++ test/test_app.py | 30 +++++++++++++++ test/test_ocrservice.py | 62 ++++++++++++++++++++++++++++++ workflow_ocr_backend/app.py | 9 ++++- workflow_ocr_backend/ocrservice.py | 62 ++++++++++++++++++++++++++++++ 5 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 test/test_ocrservice.py diff --git a/README.md b/README.md index 600b0f2..bc4d396 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,7 @@ It's written in Python and provides a simple REST API for [ocrmypdf](https://ocr - [Installation](#installation) - [`docker-compose` Example](#docker-compose-example) - [HaRP Support (Nextcloud 32+)](#harp-support-nextcloud-32) +- [OCR Parameter Validation](#ocr-parameter-validation) ## Prerequisites @@ -175,3 +176,11 @@ Since Nextcloud 32, [HaRP (AppAPI HaProxy Reversed Proxy)](https://github.com/ne HaRP simplifies deployment and improves performance by enabling direct communication between clients and ExApps. The implementation is fully backward compatible with Docker Socket Proxy deployments. For installation and migration instructions, see the [HaRP documentation](https://github.com/nextcloud/HaRP#readme). + +## OCR Parameter Validation + +The `ocrmypdf_parameters` sent to `/process_ocr` are validated before they are handed over to OCRmyPDF: + +- Only documented keyword arguments of [`ocrmypdf.ocr()`](https://ocrmypdf.readthedocs.io/en/latest/api.html) are accepted. Unknown parameters are rejected with HTTP `400` instead of being silently ignored. +- The parameters `plugins`, `plugin_manager`, `user_words`, `user_patterns`, `keep_temporary_files` as well as the input/output/sidecar parameters (which are controlled by this app) are never accepted from a request. `--plugins` in particular would make OCRmyPDF load and execute arbitrary Python code. +- Language codes must match `^[A-Za-z][A-Za-z0-9_/]{0,31}$` (e.g. `eng`, `chi_sim`, `script/Latin`), which is the same allow-list the [workflow_ocr](https://github.com/R0Wi-DEV/workflow_ocr) Nextcloud App uses. diff --git a/test/test_app.py b/test/test_app.py index 63f92fb..e9350ea 100644 --- a/test/test_app.py +++ b/test/test_app.py @@ -77,6 +77,36 @@ def test_process_ocr_error_invalid_file(): assert "ocrMyPdfExitCode" in response_json assert response_json["ocrMyPdfExitCode"] == 2 +def test_process_ocr_rejects_plugin_parameter(tmp_path): + # The "plugins" parameter would make OCRmyPDF load and execute an arbitrary + # Python file => must be rejected before ocrmypdf.ocr() is called. + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + marker = tmp_path / "pwned" + plugin = tmp_path / "evil_plugin.py" + plugin.write_text(f"open({str(marker)!r}, 'w').write('pwned')\n") + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers, raise_server_exceptions=False) as client: + response = client.post( + "/process_ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"ocrmypdf_parameters": f"--skip-text --plugins {plugin}"} + ) + assert response.status_code == 400 + assert response.json()["message"].startswith("Parameter 'plugins' is not allowed") + assert not marker.exists() + +def test_process_ocr_rejects_injected_language(): + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers, raise_server_exceptions=False) as client: + response = client.post( + "/process_ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"ocrmypdf_parameters": "--skip-text --language eng+$(id)"} + ) + assert response.status_code == 400 + assert response.json()["message"].startswith("Invalid language value '$(id)'") + def test_installed_languages(): with TestClient(APP, headers=headers) as client: response = client.get("/installed_languages") diff --git a/test/test_ocrservice.py b/test/test_ocrservice.py new file mode 100644 index 0000000..37ed692 --- /dev/null +++ b/test/test_ocrservice.py @@ -0,0 +1,62 @@ +import logging +import pytest + +from workflow_ocr_backend.ocrservice import InvalidOcrParameterError, OcrService + +service = OcrService(logging.getLogger(__name__)) + +def test_split_parameters_valid(): + params = service._split_parameters("--skip-text --tesseract-pagesegmode 7 --language eng+chi_sim") + assert params == {"skip_text": True, "tesseract_pagesegmode": 7, "language": ["eng", "chi_sim"]} + +def test_split_parameters_none(): + assert service._split_parameters(None) == {} + +@pytest.mark.parametrize("parameters", [ + "--plugins /tmp/evil.py", + "--plugin-manager foo", + "--user-words /etc/passwd", + "--user-patterns /etc/passwd", + "--keep-temporary-files", + "--sidecar /tmp/out.txt", + "--output-file /tmp/out.pdf", + "--progress-bar", +]) +def test_split_parameters_rejects_blocked_parameters(parameters): + # These parameters would allow the caller to execute arbitrary code (plugins), + # access the backend's filesystem or overwrite values controlled by this service. + with pytest.raises(InvalidOcrParameterError): + service._split_parameters(parameters) + +@pytest.mark.parametrize("parameters", [ + "--not-an-ocrmypdf-parameter", + "--some-unknown-option value", +]) +def test_split_parameters_rejects_unknown_parameters(parameters): + with pytest.raises(InvalidOcrParameterError): + service._split_parameters(parameters) + +@pytest.mark.parametrize("parameters", [ + "--language eng;id", + "--language $(id)", + "--language `id`", + "--language |id", + "--language eng+;id", + "--language ../../etc/passwd", + "--language -eng", + "--language 123", +]) +def test_split_parameters_rejects_invalid_languages(parameters): + # Language values must match the allow-list pattern used by the Nextcloud app, + # so nothing which could be (ab)used as a shell metacharacter is passed on. + with pytest.raises(InvalidOcrParameterError): + service._split_parameters(parameters) + +@pytest.mark.parametrize("parameters,expected", [ + ("--language eng", "eng"), + ("--language chi_sim", "chi_sim"), + ("--language script/Latin", "script/Latin"), + ("--language eng+deu+script/Latin", ["eng", "deu", "script/Latin"]), +]) +def test_split_parameters_accepts_valid_languages(parameters, expected): + assert service._split_parameters(parameters) == {"language": expected} diff --git a/workflow_ocr_backend/app.py b/workflow_ocr_backend/app.py index 1e30bbc..c410a3f 100644 --- a/workflow_ocr_backend/app.py +++ b/workflow_ocr_backend/app.py @@ -11,7 +11,7 @@ from ocrmypdf import ExitCodeException from .model.ocrresult import ErrorResult, OcrResult -from .ocrservice import OcrService +from .ocrservice import InvalidOcrParameterError, OcrService @asynccontextmanager async def lifespan(app: FastAPI): @@ -33,6 +33,11 @@ async def enabled_handler(enabled: bool, _: AsyncNextcloudApp) -> str: async def exit_code_exception_handler(_: Request, exc: ExitCodeException): return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})", "ocrMyPdfExitCode": exc.exit_code}, status_code=500) +@APP.exception_handler(InvalidOcrParameterError) +async def invalid_ocr_parameter_exception_handler(_: Request, exc: InvalidOcrParameterError): + # The caller sent an OCR parameter which is not allowed -> client error. + return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})"}, status_code=400) + @APP.exception_handler(Exception) async def exception_handler(_: Request, exc: Exception): # Exception will be logged by uvicorn automatically. @@ -40,7 +45,7 @@ async def exception_handler(_: Request, exc: Exception): return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})"}, status_code=500) -@APP.post("/process_ocr", response_model=OcrResult, responses={500: {"model": ErrorResult}}) +@APP.post("/process_ocr", response_model=OcrResult, responses={400: {"model": ErrorResult}, 500: {"model": ErrorResult}}) async def process_ocr( file: UploadFile = File(..., description="The file to be processed using OCR."), ocrmypdf_parameters: str = Form(None, description="Additional parameters for the OCRmyPdf process (see https://ocrmypdf.readthedocs.io/en/latest/cookbook.html#basic-examples).") diff --git a/workflow_ocr_backend/ocrservice.py b/workflow_ocr_backend/ocrservice.py index dfe855f..fd66c4b 100644 --- a/workflow_ocr_backend/ocrservice.py +++ b/workflow_ocr_backend/ocrservice.py @@ -1,15 +1,53 @@ import base64 from datetime import datetime, timezone +import inspect import io from logging import Logger +import re from typing import BinaryIO, Iterable import ocrmypdf from .model.ocrresult import OcrResult import subprocess +class InvalidOcrParameterError(ValueError): + """Raised when the caller sent an OCRmyPDF parameter which is not allowed.""" + class OcrService: + # Allow-list for tesseract/OCRmyPDF language codes (e.g. 'eng', 'chi_sim', 'script/Latin'). + # Same pattern as the one used by the Nextcloud app (workflow_ocr) so that language values + # which could be (ab)used as shell metacharacters never reach the OCR engine. + LANGUAGE_CODE_REGEX = re.compile(r"^[A-Za-z][A-Za-z0-9_/]{0,31}$") + + # Parameters which must never be taken from a request, even though ocrmypdf.ocr() accepts them: + # * plugins/plugin_manager load arbitrary Python code => remote code execution + # * input/output/sidecar/progress_bar are controlled by this service + # * user_words/user_patterns/keep_temporary_files give access to the backend's filesystem + BLOCKED_PARAMETERS = frozenset({ + "plugins", + "plugin_manager", + "input_file", + "input_file_or_options", + "output_file", + "output_folder", + "sidecar", + "progress_bar", + "user_words", + "user_patterns", + "keep_temporary_files", + }) + + # Everything OCRmyPDF documents as a keyword argument of ocrmypdf.ocr(), minus the blocked ones. + # Unknown parameters are rejected instead of being silently forwarded, so that neither typos nor + # future (potentially dangerous) OCRmyPDF options can be smuggled in via the request. + ALLOWED_PARAMETERS = frozenset( + name for name, param in inspect.signature(ocrmypdf.ocr).parameters.items() + if param.kind is inspect.Parameter.KEYWORD_ONLY + ) - BLOCKED_PARAMETERS + + LANGUAGE_PARAMETERS = frozenset({"language"}) + def __init__(self, logger: Logger): self.logger = logger @@ -77,5 +115,29 @@ def _split_parameters(self, ocrmypdf_parameters: str) -> dict[str, str | bool | # Flag value = True + self._check_parameter(key, value) + params[key] = value return params + + def _check_parameter(self, key: str, value: str | bool | Iterable[str] | int | float) -> None: + """ + Validates a single OCRmyPDF parameter before it's handed over to ocrmypdf.ocr(). + This is a security relevant check: the parameters are fully controlled by the caller + and are used to invoke the OCR engine (which in turn spawns subprocesses), so only + known-good parameters and language codes may pass. + """ + if key in self.BLOCKED_PARAMETERS: + self.logger.warning(f"Rejected blocked OCR parameter '{key}'") + raise InvalidOcrParameterError(f"Parameter '{key}' is not allowed") + + if key not in self.ALLOWED_PARAMETERS: + self.logger.warning(f"Rejected unknown OCR parameter '{key}'") + raise InvalidOcrParameterError(f"Unknown parameter '{key}'") + + if key in self.LANGUAGE_PARAMETERS: + languages = value if isinstance(value, list) else [value] + for language in languages: + if not isinstance(language, str) or not self.LANGUAGE_CODE_REGEX.match(language): + self.logger.warning(f"Rejected invalid OCR language value: {language!r}") + raise InvalidOcrParameterError(f"Invalid language value '{language}'") From 617af7f48bb52c5c4a710c7c3bbcbdb1892addce Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 20:57:46 +0000 Subject: [PATCH 2/6] docs: add in-depth security and code review with prioritized remediation plan Reviews the whole app (FastAPI layer, OcrService, Dockerfile, start.sh, CI workflows, packaging) with a focus on security and coding practices. Key finding: ocrmypdf_parameters is parsed into a dict and splatted into ocrmypdf.ocr(**kwargs) with no allowlist. That reaches ocrmypdf's plugins parameter, which resolves via importlib.import_module() and spec.loader.exec_module(), and also lets callers disable ocrmypdf's own decompression-bomb and worker-count guards. Also documents unbounded request memory, blocking CPU work on the asyncio event loop (which stalls /heartbeat), exception detail leaking to clients, unsanitised filename handling, twelve reproduced parser bugs, and supply chain gaps. Findings marked 'verified' were reproduced against the pinned dependency versions rather than inferred. Closes with a five-phase plan ordered by risk reduced per unit of work. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VwE3BeYazHp7QpGXzSNsHL --- doc/CODE_REVIEW.md | 324 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 324 insertions(+) create mode 100644 doc/CODE_REVIEW.md diff --git a/doc/CODE_REVIEW.md b/doc/CODE_REVIEW.md new file mode 100644 index 0000000..3ae5b94 --- /dev/null +++ b/doc/CODE_REVIEW.md @@ -0,0 +1,324 @@ +# Code Review — Workflow OCR Backend + +**Scope:** the whole application at commit `7579129` — `main.py`, `workflow_ocr_backend/`, `test/`, `Dockerfile`, `start.sh`, `.github/`, packaging and configuration. +**Focus:** security and coding best practices. +**Method:** source reading, plus behavioural verification against the pinned dependency versions (`ocrmypdf==17.4.2`, `nc-py-api==0.30.1`, uvicorn). Every claim marked *verified* below was reproduced, not inferred. + +--- + +## Summary + +The app is small, readable and does one thing. The structure (thin FastAPI layer → `OcrService` → `ocrmypdf`) is the right shape, the HaRP/FRP integration is carefully done, and the Docker build gets some things right that most projects get wrong (gosu pinned *and* GPG-verified, the sudo-enabled `devcontainer`/`test` stages deliberately excluded from the published `app` target). + +The dominant problem is a single design decision: **the `ocrmypdf_parameters` form field is parsed into a `dict` and splatted into `ocrmypdf.ocr(**kwargs)` with no allowlist.** That one line is the root of the critical finding and of eight of the twelve correctness bugs. Fixing it properly fixes most of this report. + +The second theme is that the service has **no resource ceiling of any kind** — no upload size limit, no OCR timeout, no concurrency bound — and it does its CPU-bound work on the asyncio event loop, so a single large document makes the whole process, including `/heartbeat`, unresponsive. + +| Severity | Count | +|---|---| +| Critical | 1 | +| High | 3 | +| Medium | 5 | +| Low / correctness | 12 | +| Best practice | 12 | + +--- + +## Critical + +### SEC-1 — Caller-controlled `ocrmypdf` kwargs allow arbitrary Python import and code execution + +`workflow_ocr_backend/ocrservice.py:24-25` + +```python +kwargs = self._split_parameters(ocrmypdf_parameters) +exit_code = ocrmypdf.ocr(file, output_buffer, sidecar=sidecar_buffer, progress_bar=False, **kwargs) +``` + +`_split_parameters` accepts *any* key. `ocrmypdf.ocr()` accepts a `plugins` parameter, and `OcrmypdfPluginManager._setup_plugins` resolves it like this (`ocrmypdf/_plugin_manager.py:96-106`): + +```python +for name in self._plugins: + if isinstance(name, Path) or name.endswith('.py'): + spec = importlib.util.spec_from_file_location(module_name, name) + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + spec.loader.exec_module(module) # <- executes the file + else: + module = importlib.import_module(name) # <- imports any installed module +``` + +`ocrmypdf.api.ocr` normalises a bare string to a one-element list (`if isinstance(plugins, str | Path): plugins = [plugins]`), so a scalar works. + +**Verified:** + +``` +_split_parameters("--plugins /tmp/evil.py") -> {'plugins': '/tmp/evil.py'} +``` + +Which reaches `exec_module()` on that path. + +**Impact.** Any caller who can reach `/process_ocr` gets: + +1. **Arbitrary Python module import** by dotted name — unconditional, requiring nothing but the request. Import side effects run in the ExApp process. +2. **Arbitrary code execution** as `serviceuser` in the container, as soon as any `.py` file exists at a path the attacker can name — a mounted volume, a shared data directory, a file planted through any other route. + +**Caveat, stated honestly:** the endpoint sits behind `AppAPIAuthMiddleware`, so the caller must already be authenticated as Nextcloud. This is not a pre-auth internet-facing RCE. It is a privilege-boundary failure: the OCR backend is supposed to be a sandboxed document processor, and instead any component that can submit a document can execute code inside it. In the intended `workflow_ocr` deployment, the parameter string originates from a *per-workflow admin setting*, which makes this at minimum an admin → container-RCE escalation, and a full RCE for any path where those parameters become user-influenced. + +Related dangerous keys reachable the same way: `user_words` / `user_patterns` (arbitrary local file paths handed to tesseract), `plugin_manager`, `keep_temporary_files`, `output_file`. + +**Fix:** a strict allowlist — see the plan, item P0. + +--- + +## High + +### SEC-2 — The same pass-through disables ocrmypdf's own DoS guards + +`ocrmypdf` ships defensive defaults. All of them are caller-overridable here. **Verified:** + +``` +_split_parameters("--max-image-mpixels 0") -> {'max_image_mpixels': 0} # decompression-bomb guard OFF +_split_parameters("--jobs 9999") -> {'jobs': 9999} # unbounded worker fan-out +_split_parameters("--keep-temporary-files") -> {'keep_temporary_files': True} # fills the container disk +``` + +A small crafted PDF plus `--max-image-mpixels 0` is enough to exhaust container memory. `--jobs` at a large value fans out subprocesses against a container that has no cgroup limits declared. `--keep-temporary-files` leaves every intermediate raster on disk, permanently, across requests. + +Even absent SEC-1, the parameter surface must be an allowlist with *bounds*, not just names. + +### SEC-3 — No size limits anywhere; peak memory is a multiple of the document + +`workflow_ocr_backend/ocrservice.py:17-39`, `workflow_ocr_backend/app.py:43-53` + +The whole pipeline is in-memory and copies repeatedly: + +1. Starlette spools the upload (memory, then a temp file past its threshold). +2. `ocrmypdf` writes the output PDF into an in-memory `BytesIO`. +3. `base64.b64encode(output_buffer.getvalue())` — a full copy, +33%. +4. `.decode("utf-8")` — another full copy. +5. FastAPI/pydantic serialises it into a JSON response — another copy. + +Peak resident memory is roughly 4–5× the output document, and there is **no maximum upload size** at the app, at uvicorn, or in the ExApp deployment. Nothing rejects a 500 MB PDF. + +Compounding it: `ocrmypdf.api` holds a process-global `threading.Lock` (`_api_lock`, `api.py:69`) around the entire pipeline run, so requests are already serialised to one at a time — but nothing *rejects* the queued ones, they simply accumulate, each holding its uploaded bytes. + +### SEC-4 — Blocking CPU work on the asyncio event loop stalls the whole process, including `/heartbeat` + +`workflow_ocr_backend/app.py:44-53` + +```python +async def process_ocr(...): + service = OcrService(logger) + return service.ocr(file.file, file.filename, ocrmypdf_parameters) # fully synchronous +``` + +The handler is `async def` but its body is entirely blocking — `ocrmypdf.ocr()` is synchronous, CPU-bound, and can run for minutes. Declaring it `async` means it runs *on the event loop* rather than in the threadpool, so for the duration of an OCR run the process serves nothing else. + +`nc_py_api`'s `set_handlers` registers `/heartbeat` (`integration_fastapi.py:144-147`). AppAPI polls it. While a large document is processing, that poll gets no response, and AppAPI concludes the ExApp is dead. + +Note the inversion: `installed_languages` is declared `def` (sync), so FastAPI *does* run it in the threadpool. The cheap endpoint is offloaded and the expensive one is not — this looks like an oversight rather than a decision. + +**Fix:** `def process_ocr(...)` (FastAPI offloads it automatically), or `await run_in_threadpool(...)`, combined with an explicit `asyncio.Semaphore`, a request timeout, and a `tesseract_timeout` floor. + +--- + +## Medium + +### SEC-5 — Arbitrary local file paths via `user_words` / `user_patterns` + +`ocrmypdf.ocr()` takes `user_words: os.PathLike` and `user_patterns: os.PathLike` and hands them to tesseract. **Verified:** `_split_parameters("--user-words /etc/passwd") -> {'user_words': '/etc/passwd'}`. This yields file-existence probing inside the container and, depending on tesseract's parsing, limited content influence on the returned `recognizedText`. Same root cause as SEC-1; listed separately because it survives any fix that only blocks `plugins`. + +### SEC-6 — Internal exception detail returned to the caller + +`workflow_ocr_backend/app.py:32-40` + +```python +@APP.exception_handler(Exception) +async def exception_handler(_: Request, exc: Exception): + return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})"}, status_code=500) +``` + +*Every* unhandled exception — including ones that have nothing to do with OCR — has its message and class name returned over HTTP. Exception strings routinely carry absolute temp paths, library internals, and partial input. The existing tests show it working as designed for `ocrmypdf` errors, but the catch-all `Exception` handler applies the same treatment to `TypeError`, `OSError`, `UnicodeDecodeError` and anything else. + +The `ExitCodeException` handler is a different case and should be kept — the PHP `workflow_ocr` client depends on `message` + `ocrMyPdfExitCode`. The fix is to keep that contract and make the generic handler return a fixed string plus a correlation id, with the full detail logged server-side. + +### SEC-7 — Unsanitised filename in logs and in the response + +`workflow_ocr_backend/ocrservice.py:22,37,39` + +`file.filename` is fully attacker-controlled and is (a) interpolated into log lines and (b) echoed back verbatim as `OcrResult.filename`. + +- **Log injection:** a filename containing `\r\n` forges log entries. With `log_level="trace"` (see BP-1) these lines are always emitted. +- **Downstream path handling:** the consuming Nextcloud app receives whatever was sent. A filename of `../../foo.pdf` is echoed unchanged; whether that matters depends on the client, which is exactly why the boundary should sanitise rather than assume. + +The same applies to `ocrmypdf_parameters`, which is logged raw at line 22. + +**Fix:** `os.path.basename()`, strip control characters, cap length, and use structured logging (`logger.debug("Processing %s", name)`) rather than f-strings. + +### SEC-8 — `/docs` and `/openapi.json` are unauthenticated + +`workflow_ocr_backend/app.py:23` + +```python +APP.add_middleware(AppAPIAuthMiddleware, disable_for=["docs", "openapi.json"]) +``` + +`AppAPIAuthMiddleware` matches with `fnmatch` on the stripped path (`integration_fastapi.py:365-366`), so the exemption is exactly those two paths — no wildcard hazard. But both are served without authentication on the ExApp port, exposing the full API schema and an interactive request builder to anyone who can reach it. In HaRP deployments that's whoever reaches HaRP; in docker-socket-proxy deployments it's the Docker network. + +The schema is not secret, but it is free reconnaissance for SEC-1. **Fix:** gate `docs_url`/`openapi_url` behind an env flag, default off in production. + +### SEC-9 — Supply chain and release integrity + +Several independent gaps, grouped because they share a fix strategy: + +- **`appinfo/info.xml:34` — `master`.** Every Nextcloud installation pulls a *mutable* tag. There is no way to pin, audit, or roll back a deployed version, and a compromised or simply broken `master` build propagates to all installs on next pull. The release workflow even extracts this literal string as its "version" (`appstore-build-publish.yml:47`). +- **No transitive dependency pinning.** `requirements.txt` pins three direct deps exactly; everything underneath floats. Builds are not reproducible and a compromised transitive release lands silently. Use a compiled lock file with `--require-hashes`. +- **Actions pinned by tag, not SHA.** `actions/checkout@v4`, `docker/build-push-action@v6`, `svenstaro/upload-release-action@v2`, `irongut/CodeCoverageSummary@v1.3.0`, `R0Wi/nextcloud-appstore-push-action@v1`. Mutable refs in a workflow that holds `APPSTORE_TOKEN` and `APP_PRIVATE_KEY`. +- **No `permissions:` block** in any workflow — `GITHUB_TOKEN` runs at the repository default rather than least privilege, in jobs that push to GHCR and publish releases. +- **Base image not digest-pinned** (`python:3.12-alpine`). +- **No automated scanning:** no Dependabot config, no CodeQL, no container image scan. + +--- + +## Low / correctness + +All of the following were reproduced against the current `_split_parameters`. + +| ID | Issue | Evidence | +|---|---|---| +| BUG-1 | Multi-token values are silently truncated to the first token | `--title Hello World` → `{'title': 'Hello'}`; `--tesseract-config a b c` → `{'tesseract_config': 'a'}` | +| BUG-2 | `str.isnumeric()` is true for Unicode numerics, then `int()` raises → unhandled 500 | `--oversample ²` → `ValueError: invalid literal for int()` | +| BUG-3 | Negative numbers are never coerced; `--` inside a value corrupts the parse | `--skip-big -1` → `{'skip_big': '-1'}` (string); `--pages 1--2` → `{'pages': 1, '2': True}` | +| BUG-4 | Any value containing `+` becomes a list, even where a scalar is expected | `--title a+b` → `{'title': ['a', 'b']}` | +| BUG-5 | Duplicate keys silently overwrite instead of erroring | `--language eng --language deu` → `{'language': 'deu'}` | +| BUG-6 | Misspelled parameters are silently discarded by `ocrmypdf` into `extra_attrs` — no error, no effect | `--languge eng` is a no-op with zero feedback | +| BUG-7 | `--sidecar …` collides with the hardcoded `sidecar=` kwarg → `TypeError: got multiple values for keyword argument` → 500 | `_split_parameters("--sidecar /tmp/x.txt")` → `{'sidecar': ...}` | +| BUG-8 | `installed_languages` runs `subprocess.run` with no `check=` and no `timeout=`; a tesseract failure returns `[]`, indistinguishable from "no languages installed"; the unconditional `[1:]` header-skip is brittle | `ocrservice.py:46-48` | +| BUG-9 | `UploadFile.filename` is `str \| None`; a multipart part without a filename → pydantic `ValidationError` → 500 | `OcrResult.filename: str` | +| BUG-10 | `sidecar_buffer.getvalue().decode("utf-8")` can raise `UnicodeDecodeError` on unusual tesseract output → 500 | `ocrservice.py:33` | +| BUG-11 | Type annotations claim `str` where `None` is the documented default | `ocrmypdf_parameters: str = Form(None)` in `app.py:46`, and `ocrservice.py:16,50` | +| BUG-12 | `output_buffer.close()` is called twice (line 31 and again in `finally`) | harmless for `BytesIO`, but the cleanup path is untidy | + +--- + +## Best practices + +| ID | Observation | +|---|---| +| BP-1 | `main.py:6` hardcodes `log_level="trace"`. This activates uvicorn's `MessageLoggerMiddleware`, which logs an entry per ASGI message per request. *Checked:* it replaces headers and bodies with placeholders, so this is **not** a credential leak — it is log volume and disk pressure in production, plus it guarantees the unsanitised `logger.debug` lines from SEC-7 are always emitted. Make it env-driven, default `info`. | +| BP-2 | `app.py:24` — `logging.getLogger('uvicorn.error')` couples application code to the server implementation; logs vanish silently under any other runner. Use `getLogger(__name__)` and configure handlers at the edge. | +| BP-3 | No `__init__.py` in `workflow_ocr_backend/` or `workflow_ocr_backend/model/` — implicit namespace packages. Works at runtime; fragile for coverage attribution and packaging. | +| BP-4 | No linter, formatter or type checker anywhere (`ruff`, `mypy`). Several findings here (BUG-9, BUG-11) are exactly what a type checker reports for free. | +| BP-5 | **`_split_parameters` has no unit tests at all.** The single highest-risk function in the codebase is covered only incidentally through slow end-to-end OCR runs. There is also no test asserting that an unauthenticated request is rejected. | +| BP-6 | `.env` is committed with `APP_SECRET=secret` and `APP_HOST=0.0.0.0`, loaded with `override=True` at test-module import time, and `COPY`'d into the test image. The values are dummies, but the pattern trains everyone to keep real secrets there. Ship `.env.example`, gitignore `.env`. | +| BP-7 | Dockerfile: `apk update` is redundant alongside `--no-cache`; `apk search tesseract-ocr-data-` installs *every* tesseract language pack, making the image very large, build-time network-dependent and non-reproducible; `pip install` has no `--no-cache-dir`; no `HEALTHCHECK`; base image not digest-pinned. **Credit where due:** gosu is version-pinned and GPG-verified, and the published `app` target correctly excludes the passwordless-sudo `devcontainer` and `test` stages. | +| BP-8 | `start.sh`: `set -e` without `-u`/`pipefail`; env vars are interpolated into TOML unquoted and unvalidated (an unset `HP_FRP_PORT` emits `serverPort = `, invalid TOML); `frpc` is backgrounded with no supervision, so if the tunnel dies the app keeps serving into nothing; `echo "Starting application: $@"` should be `$*`. | +| BP-9 | `ErrorResult` is declared and referenced in `responses={500: ...}` but never used to *build* a response — both handlers hand-roll dicts. The model and the wire format can drift apart with nothing to catch it. | +| BP-10 | `test.yml` builds and runs repository code in a job where the HaRP container receives `/var/run/docker.sock`. On ephemeral GitHub-hosted runners with `pull_request` (no secrets, read-only token) this is contained. It becomes a critical runner escape the day this moves to a self-hosted runner — worth documenting as a hard constraint on the workflow. | +| BP-11 | `info.xml` carries `1.35.0-dev` on `master`, and the release workflow publishes straight from it. | +| BP-12 | No `SECURITY.md` / disclosure policy for an app distributed through the Nextcloud appstore. | + +--- + +## Prioritized plan + +Ordered by risk reduced per unit of work. P0 is the one that matters most: it is a single self-contained change that closes the critical finding, both resource-guard bypasses, and seven of the twelve correctness bugs. + +### P0 — Replace `_split_parameters` with a validating allowlist parser + +**Closes:** SEC-1 (critical), SEC-2, SEC-5, BUG-1 … BUG-7. + +**Why:** the vulnerability is not "`plugins` is dangerous" — it is that the function is a *denylist of nothing*. Blocking `plugins` by name leaves `user_words`, `plugin_manager`, `keep_temporary_files`, and whatever the next `ocrmypdf` release adds. The only durable fix is to enumerate what is permitted, with types and bounds, and reject everything else. + +**Shape of the change**, in `ocrservice.py`: + +```python +# Exhaustive allowlist. Anything absent is rejected with 400 — notably +# plugins, plugin_manager, user_words, user_patterns, sidecar, output_file, +# input_file and keep_temporary_files. +_ALLOWED: dict[str, _Spec] = { + "language": _Spec(list_of=str, pattern=r"\A[a-z]{3}(_[a-z]+)?\Z", max_items=8), + "image_dpi": _Spec(int, lo=50, hi=1200), + "oversample": _Spec(int, lo=0, hi=1200), + "jobs": _Spec(int, lo=1, hi=os.cpu_count() or 4), + "max_image_mpixels": _Spec(float, lo=1, hi=500), # lower bound: never 0 + "skip_big": _Spec(float, lo=0, hi=10_000), + "optimize": _Spec(int, lo=0, hi=3), + "tesseract_pagesegmode": _Spec(int, lo=0, hi=13), + "tesseract_oem": _Spec(int, lo=0, hi=3), + "tesseract_timeout": _Spec(float, lo=0, hi=MAX_TESSERACT_TIMEOUT), + "mode": _Spec(str, choices={"force", "skip", "redo"}), + "output_type": _Spec(str, choices={"pdf", "pdfa", "pdfa-1", "pdfa-2", "pdfa-3"}), + "rotate_pages": _Spec(bool), + "deskew": _Spec(bool), + "clean": _Spec(bool), + "remove_background": _Spec(bool), + "force_ocr": _Spec(bool), + "skip_text": _Spec(bool), + "redo_ocr": _Spec(bool), + # ... extend deliberately, one reviewed entry at a time +} +``` + +Three rules alongside it: + +1. **Tokenise with `shlex.split()`**, not `split("--")`. That alone fixes BUG-1 (truncation), BUG-3 (`1--2`) and quoting generally. +2. **Reject, don't ignore.** An unknown or out-of-range parameter returns `400` with a message naming the offender. Silent no-ops (BUG-6) are worse than errors: an admin sets `--languge deu`, sees no error, and ships broken OCR. +3. **Reject duplicates** (BUG-5) and any key that collides with a kwarg the service sets itself (BUG-7). + +Cover it with a real unit test table — this is where BP-5 gets paid off, and these tests run in milliseconds, unlike the current end-to-end suite. + +### P1 — Put a ceiling on every resource + +**Closes:** SEC-3, SEC-4, BUG-8. + +1. Change `async def process_ocr` to `def process_ocr` so FastAPI runs it in the threadpool. One keyword; it stops OCR from blocking `/heartbeat`, which is the difference between "slow" and "AppAPI restarts the container". +2. Enforce a **maximum upload size** — read `Content-Length`, reject over the limit before touching the body, and make the limit an env var with a sane default. Also stream the upload to a `NamedTemporaryFile` and hand `ocrmypdf` a path rather than holding it in memory. +3. Bound **concurrency** with an `asyncio.Semaphore` sized to the container's CPU budget, returning `503` when saturated rather than queueing unboundedly. `ocrmypdf`'s global `_api_lock` already serialises the work; this makes the backpressure explicit instead of accidental. +4. Apply a **wall-clock timeout** to the OCR run and a default `tesseract_timeout`. +5. Give `installed_languages`' `subprocess.run` a `timeout=` and `check=True`, and surface a failure as an error rather than an empty list. Cache the result — the language set cannot change while the container runs. + +### P2 — Tighten the response and logging boundary + +**Closes:** SEC-6, SEC-7, BUG-9 … BUG-12, BP-2, BP-9. + +- Generic exception handler returns a fixed message plus a correlation id; the detail goes to the log. Keep the `ExitCodeException` handler's `message` + `ocrMyPdfExitCode` contract intact — the PHP client depends on it — and build both responses *through* `ErrorResult` so the model can't drift from the wire format. +- Sanitise `file.filename`: `os.path.basename`, strip control characters, cap the length, fall back to a generated name when it's `None`. +- Switch to structured logging (`logger.debug("...", name)`), which also removes the log-injection vector. +- `getLogger(__name__)`; fix the `str | None` annotations; `decode("utf-8", errors="replace")`; drop the duplicate `close()`. + +### P3 — Supply chain and release integrity + +**Closes:** SEC-9, BP-7, BP-10. + +- **Publish immutable image tags.** Change `` to a real version and cut a new image per release. This is the single highest-value item in this phase: today there is no such thing as "the version I have installed". +- Compile a hash-pinned lock file; install with `--require-hashes`. +- Pin every GitHub Action to a full commit SHA. +- Add an explicit least-privilege `permissions:` block to each workflow. +- Digest-pin the base image; add `--no-cache-dir`, `PIP_NO_CACHE_DIR`, and a `HEALTHCHECK`. +- Narrow the tesseract language-pack install to a declared set, or accept the image size as a documented trade-off — but stop deriving it from a live `apk search` at build time. +- Add Dependabot, CodeQL, and a container scan; document the self-hosted-runner constraint on the HaRP job. + +### P4 — Tooling and hygiene + +**Closes:** BP-1, BP-3, BP-4, BP-6, BP-8, BP-11, BP-12. + +- `log_level` from the environment, default `info`. +- Add `ruff` + `mypy` with a CI gate. Add `__init__.py` files. +- Replace the committed `.env` with `.env.example`; gitignore `.env`. +- `start.sh`: `set -euo pipefail`, validate required env vars before writing TOML, supervise or `exec` the frpc process, `"$*"` in the echo. +- `SECURITY.md` with a disclosure address. + +--- + +## What's already good + +Worth stating, because a review that only lists problems misrepresents the codebase: + +- The layering is clean — the FastAPI module has no OCR logic and `OcrService` has no HTTP concerns. +- gosu is version-pinned *and* GPG signature-verified, which is rarer than it should be. +- The multi-stage Dockerfile deliberately keeps the passwordless-sudo `devcontainer`/`test` stages out of the published `app` image. +- The HaRP integration test is genuinely thorough: it stands up a real HaRP container, drives the real ExApp lifecycle, asserts on the generated `frpc.toml`, and cleans up in a `finally`. +- Direct dependencies are pinned to exact versions. +- CI runs tests in the same container image that ships, which eliminates a whole class of "works on my machine". From e546c8df65c6422e0798b7e77e6e81dc7fb31da2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 04:45:51 +0000 Subject: [PATCH 3/6] fix: key OCR parameter allow-list on CLI names, not Python kwargs PR #12 derived the allow-list from inspect.signature(ocrmypdf.ocr) keyword-only parameters, but callers send OCRmyPDF CLI option names. Those two sets differ, which both rejected valid input and would widen the accepted set on upgrade. * --ocr-engine none returned 400. ocr_engine is a real OcrOptions field, so it previously reached create_options and worked; this was a functional regression. * --jpeg-quality 80 returned 400. It is the primary documented CLI flag, while the signature only exposes the argparse.SUPPRESS alias jpg_quality. Replace the introspected set with an explicit literal allow-list of 49 CLI option names plus an alias map, so the accepted surface is reviewed rather than inherited from whatever OCRmyPDF happens to expose. Introspection is retained as a test-time drift guard that fails if an upgrade renames or removes an option. Also: * Block tesseract_config. It is appended verbatim to the tesseract argv (_exec/tesseract.py), making it an arbitrary config-file path in the same way the already-blocked user_words is. * Accept and drop CLI-only flags (--quiet, --verbose, --no-progress-bar) instead of rejecting them, so existing workflow configurations keep working. * Use re.fullmatch for language codes. '$' also matches before a trailing newline, so re.match accepted 'eng\n' via '--language eng\n+deu'. * Log rejected keys with %r rather than f-strings, so control characters in caller-supplied input cannot forge log lines. Verified against ocrmypdf 17.4.2: every allow-listed parameter lands on a real OcrOptions field rather than extra_attrs, and --ocr-engine none returns 200 with an empty text layer end-to-end. Full suite: 43 passed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VwE3BeYazHp7QpGXzSNsHL --- README.md | 5 +- doc/CODE_REVIEW.md | 340 +++++++++++------------------ test/test_ocrservice.py | 55 +++++ workflow_ocr_backend/ocrservice.py | 71 ++++-- 4 files changed, 237 insertions(+), 234 deletions(-) diff --git a/README.md b/README.md index bc4d396..d591d37 100644 --- a/README.md +++ b/README.md @@ -181,6 +181,7 @@ For installation and migration instructions, see the [HaRP documentation](https: The `ocrmypdf_parameters` sent to `/process_ocr` are validated before they are handed over to OCRmyPDF: -- Only documented keyword arguments of [`ocrmypdf.ocr()`](https://ocrmypdf.readthedocs.io/en/latest/api.html) are accepted. Unknown parameters are rejected with HTTP `400` instead of being silently ignored. -- The parameters `plugins`, `plugin_manager`, `user_words`, `user_patterns`, `keep_temporary_files` as well as the input/output/sidecar parameters (which are controlled by this app) are never accepted from a request. `--plugins` in particular would make OCRmyPDF load and execute arbitrary Python code. +- Only parameters on an explicit allow-list of documented [OCRmyPDF CLI options](https://ocrmypdf.readthedocs.io/en/latest/cookbook.html) are accepted. Unknown parameters are rejected with HTTP `400` instead of being silently ignored. The allow-list is a literal set rather than something derived from OCRmyPDF at runtime, so an OCRmyPDF upgrade can never widen it without review. +- The parameters `plugins`, `plugin_manager`, `user_words`, `user_patterns`, `keep_temporary_files`, `tesseract_config` as well as the input/output/sidecar parameters (which are controlled by this app) are never accepted from a request. `--plugins` in particular would make OCRmyPDF load and execute arbitrary Python code; `--tesseract-config` and `--user-words` would expose the backend's filesystem to the caller. +- CLI-only flags with no API equivalent (`--quiet`, `--verbose`, `--no-progress-bar`) are accepted and ignored rather than rejected. - Language codes must match `^[A-Za-z][A-Za-z0-9_/]{0,31}$` (e.g. `eng`, `chi_sim`, `script/Latin`), which is the same allow-list the [workflow_ocr](https://github.com/R0Wi-DEV/workflow_ocr) Nextcloud App uses. diff --git a/doc/CODE_REVIEW.md b/doc/CODE_REVIEW.md index 3ae5b94..3242d7a 100644 --- a/doc/CODE_REVIEW.md +++ b/doc/CODE_REVIEW.md @@ -1,324 +1,228 @@ # Code Review — Workflow OCR Backend -**Scope:** the whole application at commit `7579129` — `main.py`, `workflow_ocr_backend/`, `test/`, `Dockerfile`, `start.sh`, `.github/`, packaging and configuration. +**Scope:** the whole application — `main.py`, `workflow_ocr_backend/`, `test/`, `Dockerfile`, `start.sh`, `.github/`, packaging and configuration. +**Baseline:** PR #12 (`bugfix/security-enhancements`), plus the follow-up commit on this branch. **Focus:** security and coding best practices. -**Method:** source reading, plus behavioural verification against the pinned dependency versions (`ocrmypdf==17.4.2`, `nc-py-api==0.30.1`, uvicorn). Every claim marked *verified* below was reproduced, not inferred. +**Method:** source reading, plus behavioural verification against the pinned dependency versions (`ocrmypdf==17.4.2`, `nc-py-api==0.30.1`, uvicorn). Every claim marked *verified* was reproduced by execution, not inferred. --- ## Summary -The app is small, readable and does one thing. The structure (thin FastAPI layer → `OcrService` → `ocrmypdf`) is the right shape, the HaRP/FRP integration is carefully done, and the Docker build gets some things right that most projects get wrong (gosu pinned *and* GPG-verified, the sudo-enabled `devcontainer`/`test` stages deliberately excluded from the published `app` target). +The first pass of this review found a critical RCE: `ocrmypdf_parameters` was parsed into a dict and splatted into `ocrmypdf.ocr(**kwargs)` with no allow-list, reaching ocrmypdf's `plugins` parameter and from there `spec.loader.exec_module()`. -The dominant problem is a single design decision: **the `ocrmypdf_parameters` form field is parsed into a `dict` and splatted into `ocrmypdf.ocr(**kwargs)` with no allowlist.** That one line is the root of the critical finding and of eight of the twelve correctness bugs. Fixing it properly fixes most of this report. +**PR #12 closes it.** `plugins` and `plugin_manager` are blocked, and the accompanying test asserts the exploit's marker file is never written rather than merely checking for a 400 — the right kind of test for a code-execution fix. -The second theme is that the service has **no resource ceiling of any kind** — no upload size limit, no OCR timeout, no concurrency bound — and it does its CPU-bound work on the asyncio event loop, so a single large document makes the whole process, including `/heartbeat`, unresponsive. +PR #12 also introduced four defects of its own, because its allow-list was derived at import time from `inspect.signature(ocrmypdf.ocr)` — that is, from **Python keyword names** — while callers send **CLI option names**. Those two sets are not the same. The follow-up commit on this branch fixes all four. They are recorded in full below, because the reasoning matters more than the patch. -| Severity | Count | +What remains is what the first pass called P1 onward: the service still has **no resource ceiling of any kind** — no upload size limit, no OCR timeout, no concurrency bound — and it still does CPU-bound work on the asyncio event loop, so a single large document makes the whole process, including `/heartbeat`, unresponsive. + +| | Count | |---|---| -| Critical | 1 | -| High | 3 | -| Medium | 5 | -| Low / correctness | 12 | -| Best practice | 12 | +| Closed by PR #12 | 4 (incl. the critical) | +| Introduced by PR #12, fixed on this branch | 5 | +| Security findings still open | 7 | +| Correctness bugs still open | 10 | +| Best-practice items still open | 12 | --- -## Critical - -### SEC-1 — Caller-controlled `ocrmypdf` kwargs allow arbitrary Python import and code execution - -`workflow_ocr_backend/ocrservice.py:24-25` - -```python -kwargs = self._split_parameters(ocrmypdf_parameters) -exit_code = ocrmypdf.ocr(file, output_buffer, sidecar=sidecar_buffer, progress_bar=False, **kwargs) -``` - -`_split_parameters` accepts *any* key. `ocrmypdf.ocr()` accepts a `plugins` parameter, and `OcrmypdfPluginManager._setup_plugins` resolves it like this (`ocrmypdf/_plugin_manager.py:96-106`): - -```python -for name in self._plugins: - if isinstance(name, Path) or name.endswith('.py'): - spec = importlib.util.spec_from_file_location(module_name, name) - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - spec.loader.exec_module(module) # <- executes the file - else: - module = importlib.import_module(name) # <- imports any installed module -``` - -`ocrmypdf.api.ocr` normalises a bare string to a one-element list (`if isinstance(plugins, str | Path): plugins = [plugins]`), so a scalar works. - -**Verified:** - -``` -_split_parameters("--plugins /tmp/evil.py") -> {'plugins': '/tmp/evil.py'} -``` - -Which reaches `exec_module()` on that path. - -**Impact.** Any caller who can reach `/process_ocr` gets: - -1. **Arbitrary Python module import** by dotted name — unconditional, requiring nothing but the request. Import side effects run in the ExApp process. -2. **Arbitrary code execution** as `serviceuser` in the container, as soon as any `.py` file exists at a path the attacker can name — a mounted volume, a shared data directory, a file planted through any other route. +## Closed by PR #12 -**Caveat, stated honestly:** the endpoint sits behind `AppAPIAuthMiddleware`, so the caller must already be authenticated as Nextcloud. This is not a pre-auth internet-facing RCE. It is a privilege-boundary failure: the OCR backend is supposed to be a sandboxed document processor, and instead any component that can submit a document can execute code inside it. In the intended `workflow_ocr` deployment, the parameter string originates from a *per-workflow admin setting*, which makes this at minimum an admin → container-RCE escalation, and a full RCE for any path where those parameters become user-influenced. - -Related dangerous keys reachable the same way: `user_words` / `user_patterns` (arbitrary local file paths handed to tesseract), `plugin_manager`, `keep_temporary_files`, `output_file`. +| ID | Finding | How | +|---|---|---| +| SEC-1 | **Critical** — arbitrary Python import and code execution via `--plugins` | `plugins` / `plugin_manager` blocked; test asserts the marker file is never created | +| SEC-5 | Arbitrary local file paths via `--user-words` / `--user-patterns` | both blocked | +| BUG-6 | Misspelled parameters silently discarded into `extra_attrs` — no error, no effect | unknown parameters now return HTTP 400 | +| BUG-7 | `--sidecar` collided with the hardcoded `sidecar=` kwarg → `TypeError` → 500 | `sidecar` blocked | -**Fix:** a strict allowlist — see the plan, item P0. +Also verified correct in #12, for the record: `InvalidOcrParameterError` resolves ahead of the generic `Exception` handler via Starlette's MRO lookup, so it genuinely returns 400 rather than 500; and the language regex rejects every injection form in its test matrix. --- -## High +## Introduced by PR #12 — fixed on this branch -### SEC-2 — The same pass-through disables ocrmypdf's own DoS guards +### PR-1 — Allow-list keyed on Python names, not CLI names (HIGH, a real regression) -`ocrmypdf` ships defensive defaults. All of them are caller-overridable here. **Verified:** +The allow-list was `inspect.signature(ocrmypdf.ocr)` keyword-only parameters. Callers send CLI option names. Verified by execution against the real ocrmypdf 17.4.2: -``` -_split_parameters("--max-image-mpixels 0") -> {'max_image_mpixels': 0} # decompression-bomb guard OFF -_split_parameters("--jobs 9999") -> {'jobs': 9999} # unbounded worker fan-out -_split_parameters("--keep-temporary-files") -> {'keep_temporary_files': True} # fills the container disk -``` +| Sent by caller | On PR #12 | Before PR #12 | +|---|---|---| +| `--ocr-engine none` | **400 Unknown parameter** | **worked** — `ocr_engine` is a real `OcrOptions` model field (`_options.py:197`), so `**kwargs` → `create_options` set it | +| `--jpeg-quality 80` | **400 Unknown parameter** | silently ignored (routed to `extra_attrs`) | +| `--jpg-quality 80` | passed | passed | -A small crafted PDF plus `--max-image-mpixels 0` is enough to exhaust container memory. `--jobs` at a large value fans out subprocesses against a container that has no cgroup limits declared. `--keep-temporary-files` leaves every intermediate raster on disk, permanently, across requests. +`--jpeg-quality` is the *primary documented* CLI flag; `--jpg-quality` is its `argparse.SUPPRESS`ed alias (`builtin_plugins/optimize.py:74,86`). The signature exposes only `jpg_quality`, so the allow-list accepted the hidden alias and rejected the documented spelling. -Even absent SEC-1, the parameter surface must be an allowlist with *bounds*, not just names. +`--ocr-engine none` is the sharper case: a documented flag (`cli.py:413`) that **worked before and failed every job after**. -### SEC-3 — No size limits anywhere; peak memory is a multiple of the document +**Fix:** an explicit literal allow-list of 49 CLI option names, plus an alias map (`jpeg_quality → jpg_quality`) applied after validation, plus an `IGNORED_PARAMETERS` set for CLI-only flags (`--quiet`, `--verbose`, `--no-progress-bar`) that are accepted and dropped rather than rejected, so existing configurations carrying them keep working. -`workflow_ocr_backend/ocrservice.py:17-39`, `workflow_ocr_backend/app.py:43-53` +### PR-2 — Allow-list auto-widened on every dependency bump (MEDIUM) -The whole pipeline is in-memory and copies repeatedly: +The comment claimed future dangerous options could not be smuggled in. The code did the opposite: because the set was introspected from the *installed* ocrmypdf, any keyword-only parameter a future release adds would be accepted automatically, unreviewed. -1. Starlette spools the upload (memory, then a temp file past its threshold). -2. `ocrmypdf` writes the output PDF into an in-memory `BytesIO`. -3. `base64.b64encode(output_buffer.getvalue())` — a full copy, +33%. -4. `.decode("utf-8")` — another full copy. -5. FastAPI/pydantic serialises it into a JSON response — another copy. +**Fix:** the explicit literal set above. Introspection is retained as a *test-time drift guard* (`test_allowed_parameters_still_resolve_against_installed_ocrmypdf`) asserting every allow-listed name still resolves against the installed library — so the list stays reviewed, but a rename or removal upstream fails loudly instead of silently 400ing at runtime. -Peak resident memory is roughly 4–5× the output document, and there is **no maximum upload size** at the app, at uvicorn, or in the ExApp deployment. Nothing rejects a 500 MB PDF. +### PR-3 — `tesseract_config` left allowed (MEDIUM) -Compounding it: `ocrmypdf.api` holds a process-global `threading.Lock` (`_api_lock`, `api.py:69`) around the entire pipeline run, so requests are already serialised to one at a time — but nothing *rejects* the queued ones, they simply accumulate, each holding its uploaded bytes. +Same class as the blocked `user_words`/`user_patterns`. Traced `options.tesseract.config` → `_exec/tesseract.py:366,447` → `args_tesseract.extend(tessconfig)`: appended verbatim to the tesseract argv. Not shell injection — no shell is involved — but arbitrary argv injection, and `+` yields multiple tokens: `--tesseract-config /tmp/a+/tmp/b` → `['/tmp/a', '/tmp/b']`. Verified. -### SEC-4 — Blocking CPU work on the asyncio event loop stalls the whole process, including `/heartbeat` +**Fix:** moved into `BLOCKED_PARAMETERS`. -`workflow_ocr_backend/app.py:44-53` +### PR-4 — Language regex used `re.match` with `$` (LOW, but reachable) -```python -async def process_ocr(...): - service = OcrService(logger) - return service.ocr(file.file, file.filename, ocrmypdf_parameters) # fully synchronous -``` +`$` also matches before a trailing newline. Verified reachable: `--language eng\n+deu` → `{'language': ['eng\n', 'deu']}` **passed validation**. Low impact (argv, not shell), but it defeated the regex's stated purpose. -The handler is `async def` but its body is entirely blocking — `ocrmypdf.ocr()` is synchronous, CPU-bound, and can run for minutes. Declaring it `async` means it runs *on the event loop* rather than in the threadpool, so for the duration of an OCR run the process serves nothing else. +**Fix:** `re.fullmatch`. -`nc_py_api`'s `set_handlers` registers `/heartbeat` (`integration_fastapi.py:144-147`). AppAPI polls it. While a large document is processing, that poll gets no response, and AppAPI concludes the ExApp is dead. +### PR-5 — New log-injection sites (LOW) -Note the inversion: `installed_languages` is declared `def` (sync), so FastAPI *does* run it in the threadpool. The cheap endpoint is offloaded and the expensive one is not — this looks like an oversight rather than a decision. +The new validation path logged the caller-controlled key with f-strings — `logger.warning(f"Rejected unknown OCR parameter '{key}'")` — a fresh instance of SEC-7. A key containing CR/LF forges log entries. -**Fix:** `def process_ocr(...)` (FastAPI offloads it automatically), or `await run_in_threadpool(...)`, combined with an explicit `asyncio.Semaphore`, a request timeout, and a `tesseract_timeout` floor. +**Fix:** `%r` lazy formatting, which escapes control characters. --- -## Medium - -### SEC-5 — Arbitrary local file paths via `user_words` / `user_patterns` - -`ocrmypdf.ocr()` takes `user_words: os.PathLike` and `user_patterns: os.PathLike` and hands them to tesseract. **Verified:** `_split_parameters("--user-words /etc/passwd") -> {'user_words': '/etc/passwd'}`. This yields file-existence probing inside the container and, depending on tesseract's parsing, limited content influence on the returned `recognizedText`. Same root cause as SEC-1; listed separately because it survives any fix that only blocks `plugins`. +## Still open — security -### SEC-6 — Internal exception detail returned to the caller +### SEC-2 — Resource guards remain caller-overridable (HIGH, partially closed) -`workflow_ocr_backend/app.py:32-40` +`keep_temporary_files` is now blocked. The rest are not. Verified against the current branch: -```python -@APP.exception_handler(Exception) -async def exception_handler(_: Request, exc: Exception): - return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})"}, status_code=500) +``` +--max-image-mpixels 100000 -> accepted # decompression-bomb guard effectively disabled +--jobs 10000 -> accepted # unbounded worker fan-out ``` -*Every* unhandled exception — including ones that have nothing to do with OCR — has its message and class name returned over HTTP. Exception strings routinely carry absolute temp paths, library internals, and partial input. The existing tests show it working as designed for `ocrmypdf` errors, but the catch-all `Exception` handler applies the same treatment to `TypeError`, `OSError`, `UnicodeDecodeError` and anything else. +The allow-list validates *names*. It does not validate *values*. A small crafted PDF plus a large `--max-image-mpixels` still exhausts container memory. This is the top remaining item. -The `ExitCodeException` handler is a different case and should be kept — the PHP `workflow_ocr` client depends on `message` + `ocrMyPdfExitCode`. The fix is to keep that contract and make the generic handler return a fixed string plus a correlation id, with the full detail logged server-side. +### SEC-3 — No size limits anywhere; peak memory is a multiple of the document (HIGH) -### SEC-7 — Unsanitised filename in logs and in the response +`ocrservice.py`, `app.py`. The pipeline is in-memory and copies repeatedly: Starlette spools the upload, ocrmypdf writes the output into a `BytesIO`, `b64encode` copies at +33%, `.decode()` copies again, pydantic serialises a third time into the JSON response. Peak resident memory is roughly 4–5× the output document, and there is no maximum upload size at the app, at uvicorn, or in the ExApp deployment. -`workflow_ocr_backend/ocrservice.py:22,37,39` +Compounding it: `ocrmypdf.api` holds a process-global `threading.Lock` around the whole pipeline run, so requests already serialise to one at a time — but nothing *rejects* the queued ones. They accumulate, each holding its uploaded bytes. -`file.filename` is fully attacker-controlled and is (a) interpolated into log lines and (b) echoed back verbatim as `OcrResult.filename`. +### SEC-4 — Blocking CPU work on the event loop stalls the process, including `/heartbeat` (HIGH) -- **Log injection:** a filename containing `\r\n` forges log entries. With `log_level="trace"` (see BP-1) these lines are always emitted. -- **Downstream path handling:** the consuming Nextcloud app receives whatever was sent. A filename of `../../foo.pdf` is echoed unchanged; whether that matters depends on the client, which is exactly why the boundary should sanitise rather than assume. +`app.py` — `process_ocr` is `async def` but its body is entirely blocking. ocrmypdf is synchronous, CPU-bound, and can run for minutes. Declaring it `async` runs it *on the event loop*, so for the duration of an OCR run the process serves nothing else. `nc_py_api` registers `/heartbeat`, AppAPI polls it, and a stalled poll makes AppAPI conclude the ExApp is dead. -The same applies to `ocrmypdf_parameters`, which is logged raw at line 22. +Note the inversion that suggests oversight rather than intent: `installed_languages` *is* declared `def`, so FastAPI offloads it to the threadpool. The cheap endpoint is offloaded; the expensive one is not. -**Fix:** `os.path.basename()`, strip control characters, cap length, and use structured logging (`logger.debug("Processing %s", name)`) rather than f-strings. +### SEC-6 — Internal exception detail returned to the caller (MEDIUM) -### SEC-8 — `/docs` and `/openapi.json` are unauthenticated +`app.py` — the catch-all handler returns `f"{str(exc)} ({exc.__class__.__name__})"` for *every* unhandled exception. Exception strings routinely carry absolute temp paths, library internals, and fragments of input. -`workflow_ocr_backend/app.py:23` +The `ExitCodeException` and `InvalidOcrParameterError` handlers are different cases and should stay as they are — the first is a contract the PHP client depends on (`message` + `ocrMyPdfExitCode`), and the second returns an app-authored message. Only the generic handler needs to become a fixed string plus a correlation id. -```python -APP.add_middleware(AppAPIAuthMiddleware, disable_for=["docs", "openapi.json"]) -``` +### SEC-7 — Unsanitised filename in logs and in the response (MEDIUM, partially closed) -`AppAPIAuthMiddleware` matches with `fnmatch` on the stripped path (`integration_fastapi.py:365-366`), so the exemption is exactly those two paths — no wildcard hazard. But both are served without authentication on the ExApp port, exposing the full API schema and an interactive request builder to anyone who can reach it. In HaRP deployments that's whoever reaches HaRP; in docker-socket-proxy deployments it's the Docker network. +The validation-path log injection introduced by #12 is fixed (PR-5). The original instance is not: `file.filename` is fully attacker-controlled and is still interpolated into a `logger.debug` f-string and echoed back verbatim as `OcrResult.filename`. Needs `os.path.basename`, control-character stripping, a length cap, and structured logging. -The schema is not secret, but it is free reconnaissance for SEC-1. **Fix:** gate `docs_url`/`openapi_url` behind an env flag, default off in production. +### SEC-8 — `/docs` and `/openapi.json` are unauthenticated (MEDIUM) -### SEC-9 — Supply chain and release integrity +`AppAPIAuthMiddleware(disable_for=["docs", "openapi.json"])`. The middleware matches with `fnmatch` on the stripped path, so the exemption is exactly those two — no wildcard hazard — but both serve without authentication on the ExApp port. Gate them behind an env flag, default off in production. -Several independent gaps, grouped because they share a fix strategy: +### SEC-9 — Supply chain and release integrity (MEDIUM) -- **`appinfo/info.xml:34` — `master`.** Every Nextcloud installation pulls a *mutable* tag. There is no way to pin, audit, or roll back a deployed version, and a compromised or simply broken `master` build propagates to all installs on next pull. The release workflow even extracts this literal string as its "version" (`appstore-build-publish.yml:47`). -- **No transitive dependency pinning.** `requirements.txt` pins three direct deps exactly; everything underneath floats. Builds are not reproducible and a compromised transitive release lands silently. Use a compiled lock file with `--require-hashes`. -- **Actions pinned by tag, not SHA.** `actions/checkout@v4`, `docker/build-push-action@v6`, `svenstaro/upload-release-action@v2`, `irongut/CodeCoverageSummary@v1.3.0`, `R0Wi/nextcloud-appstore-push-action@v1`. Mutable refs in a workflow that holds `APPSTORE_TOKEN` and `APP_PRIVATE_KEY`. -- **No `permissions:` block** in any workflow — `GITHUB_TOKEN` runs at the repository default rather than least privilege, in jobs that push to GHCR and publish releases. -- **Base image not digest-pinned** (`python:3.12-alpine`). -- **No automated scanning:** no Dependabot config, no CodeQL, no container image scan. +- **`master`** — every installation pulls a *mutable* tag. There is no way to pin, audit, or roll back a deployed version. +- **No transitive pinning** — direct deps are pinned exactly; everything underneath floats. +- **Actions pinned by tag, not SHA** — in workflows holding `APPSTORE_TOKEN` and `APP_PRIVATE_KEY`. +- **No `permissions:` block** in any workflow. +- Base image not digest-pinned; no Dependabot, CodeQL, or container scanning. --- -## Low / correctness +## Still open — correctness -All of the following were reproduced against the current `_split_parameters`. +All reproduced against the current branch. | ID | Issue | Evidence | |---|---|---| -| BUG-1 | Multi-token values are silently truncated to the first token | `--title Hello World` → `{'title': 'Hello'}`; `--tesseract-config a b c` → `{'tesseract_config': 'a'}` | +| BUG-1 | Multi-token values silently truncated to the first token. **#12 made this worse**: it used to mangle silently, now it hard-fails | `--title Hello World` → `{'title': 'Hello'}`; `--clean --unpaper-args --layout single` → `400 Unknown parameter 'layout'` | | BUG-2 | `str.isnumeric()` is true for Unicode numerics, then `int()` raises → unhandled 500 | `--oversample ²` → `ValueError: invalid literal for int()` | -| BUG-3 | Negative numbers are never coerced; `--` inside a value corrupts the parse | `--skip-big -1` → `{'skip_big': '-1'}` (string); `--pages 1--2` → `{'pages': 1, '2': True}` | -| BUG-4 | Any value containing `+` becomes a list, even where a scalar is expected | `--title a+b` → `{'title': ['a', 'b']}` | -| BUG-5 | Duplicate keys silently overwrite instead of erroring | `--language eng --language deu` → `{'language': 'deu'}` | -| BUG-6 | Misspelled parameters are silently discarded by `ocrmypdf` into `extra_attrs` — no error, no effect | `--languge eng` is a no-op with zero feedback | -| BUG-7 | `--sidecar …` collides with the hardcoded `sidecar=` kwarg → `TypeError: got multiple values for keyword argument` → 500 | `_split_parameters("--sidecar /tmp/x.txt")` → `{'sidecar': ...}` | -| BUG-8 | `installed_languages` runs `subprocess.run` with no `check=` and no `timeout=`; a tesseract failure returns `[]`, indistinguishable from "no languages installed"; the unconditional `[1:]` header-skip is brittle | `ocrservice.py:46-48` | -| BUG-9 | `UploadFile.filename` is `str \| None`; a multipart part without a filename → pydantic `ValidationError` → 500 | `OcrResult.filename: str` | -| BUG-10 | `sidecar_buffer.getvalue().decode("utf-8")` can raise `UnicodeDecodeError` on unusual tesseract output → 500 | `ocrservice.py:33` | -| BUG-11 | Type annotations claim `str` where `None` is the documented default | `ocrmypdf_parameters: str = Form(None)` in `app.py:46`, and `ocrservice.py:16,50` | -| BUG-12 | `output_buffer.close()` is called twice (line 31 and again in `finally`) | harmless for `BytesIO`, but the cleanup path is untidy | +| BUG-3 | Negative numbers never coerced; `--` inside a value corrupts the parse | `--skip-big -1` → `'-1'` (string); `--pages 1--2` → `{'pages': 1, '2': True}` | +| BUG-4 | Any value containing `+` becomes a list, even where a scalar is expected | `--title a+b` → `['a', 'b']` | +| BUG-5 | Duplicate keys silently overwrite instead of erroring | `--language eng --language deu` → `'deu'` | +| BUG-8 | `installed_languages` has no `check=` and no `timeout=`; a tesseract failure returns `[]`, indistinguishable from "no languages installed"; the `[1:]` header-skip is brittle | `ocrservice.py` | +| BUG-9 | `UploadFile.filename` is `str \| None`; a part without a filename → pydantic ValidationError → 500 | `OcrResult.filename: str` | +| BUG-10 | `sidecar_buffer.getvalue().decode("utf-8")` can raise `UnicodeDecodeError` → 500 | `ocrservice.py` | +| BUG-11 | Annotations claim `str` where `None` is the documented default | `app.py`, `ocrservice.py` | +| BUG-12 | `output_buffer.close()` called twice | harmless for `BytesIO`, but untidy | + +Every one of BUG-1 through BUG-5 has the same root cause: `_split_parameters` still tokenises with `str.split("--")` and `str.split(" ")`. `shlex.split` fixes the class. --- -## Best practices +## Still open — best practices | ID | Observation | |---|---| -| BP-1 | `main.py:6` hardcodes `log_level="trace"`. This activates uvicorn's `MessageLoggerMiddleware`, which logs an entry per ASGI message per request. *Checked:* it replaces headers and bodies with placeholders, so this is **not** a credential leak — it is log volume and disk pressure in production, plus it guarantees the unsanitised `logger.debug` lines from SEC-7 are always emitted. Make it env-driven, default `info`. | -| BP-2 | `app.py:24` — `logging.getLogger('uvicorn.error')` couples application code to the server implementation; logs vanish silently under any other runner. Use `getLogger(__name__)` and configure handlers at the edge. | -| BP-3 | No `__init__.py` in `workflow_ocr_backend/` or `workflow_ocr_backend/model/` — implicit namespace packages. Works at runtime; fragile for coverage attribution and packaging. | -| BP-4 | No linter, formatter or type checker anywhere (`ruff`, `mypy`). Several findings here (BUG-9, BUG-11) are exactly what a type checker reports for free. | -| BP-5 | **`_split_parameters` has no unit tests at all.** The single highest-risk function in the codebase is covered only incidentally through slow end-to-end OCR runs. There is also no test asserting that an unauthenticated request is rejected. | -| BP-6 | `.env` is committed with `APP_SECRET=secret` and `APP_HOST=0.0.0.0`, loaded with `override=True` at test-module import time, and `COPY`'d into the test image. The values are dummies, but the pattern trains everyone to keep real secrets there. Ship `.env.example`, gitignore `.env`. | -| BP-7 | Dockerfile: `apk update` is redundant alongside `--no-cache`; `apk search tesseract-ocr-data-` installs *every* tesseract language pack, making the image very large, build-time network-dependent and non-reproducible; `pip install` has no `--no-cache-dir`; no `HEALTHCHECK`; base image not digest-pinned. **Credit where due:** gosu is version-pinned and GPG-verified, and the published `app` target correctly excludes the passwordless-sudo `devcontainer` and `test` stages. | -| BP-8 | `start.sh`: `set -e` without `-u`/`pipefail`; env vars are interpolated into TOML unquoted and unvalidated (an unset `HP_FRP_PORT` emits `serverPort = `, invalid TOML); `frpc` is backgrounded with no supervision, so if the tunnel dies the app keeps serving into nothing; `echo "Starting application: $@"` should be `$*`. | -| BP-9 | `ErrorResult` is declared and referenced in `responses={500: ...}` but never used to *build* a response — both handlers hand-roll dicts. The model and the wire format can drift apart with nothing to catch it. | -| BP-10 | `test.yml` builds and runs repository code in a job where the HaRP container receives `/var/run/docker.sock`. On ephemeral GitHub-hosted runners with `pull_request` (no secrets, read-only token) this is contained. It becomes a critical runner escape the day this moves to a self-hosted runner — worth documenting as a hard constraint on the workflow. | -| BP-11 | `info.xml` carries `1.35.0-dev` on `master`, and the release workflow publishes straight from it. | -| BP-12 | No `SECURITY.md` / disclosure policy for an app distributed through the Nextcloud appstore. | +| BP-1 | `main.py` hardcodes `log_level="trace"`, activating uvicorn's `MessageLoggerMiddleware` — one log entry per ASGI message per request. *Checked:* it replaces headers and bodies with placeholders, so this is **not** a credential leak; it is log volume and disk pressure. Make it env-driven, default `info`. | +| BP-2 | `logging.getLogger('uvicorn.error')` couples application code to the server; logs vanish silently under any other runner. | +| BP-3 | No `__init__.py` in either package directory — implicit namespace packages. | +| BP-4 | No linter, formatter or type checker. BUG-9 and BUG-11 are exactly what `mypy` reports for free. | +| BP-5 | Largely addressed by #12, which added `test_ocrservice.py`. Still missing: a test asserting unauthenticated requests are rejected. | +| BP-6 | `.env` committed with `APP_SECRET=secret` and `APP_HOST=0.0.0.0`, loaded with `override=True` at test-import time and copied into the test image. Ship `.env.example`. | +| BP-7 | Dockerfile: `apk update` redundant alongside `--no-cache`; `apk search tesseract-ocr-data-` installs *every* language pack, making the image large and non-reproducible; no `--no-cache-dir`; no `HEALTHCHECK`; base image not digest-pinned. | +| BP-8 | `start.sh`: `set -e` without `-u`/`pipefail`; env vars interpolated into TOML unquoted and unvalidated; `frpc` backgrounded with no supervision; `echo "... $@"` should be `$*`. | +| BP-9 | `ErrorResult` is declared and referenced in `responses={...}` but never used to *build* a response — all three handlers hand-roll dicts, so the model and the wire format can drift. | +| BP-10 | `test.yml` builds and runs repository code in a job where HaRP receives `/var/run/docker.sock`. Contained on ephemeral GitHub-hosted runners; a critical escape the day it moves to a self-hosted runner. | +| BP-11 | `info.xml` carries `1.35.0-dev` on `master`. | +| BP-12 | No `SECURITY.md` or disclosure policy. | --- -## Prioritized plan - -Ordered by risk reduced per unit of work. P0 is the one that matters most: it is a single self-contained change that closes the critical finding, both resource-guard bypasses, and seven of the twelve correctness bugs. - -### P0 — Replace `_split_parameters` with a validating allowlist parser - -**Closes:** SEC-1 (critical), SEC-2, SEC-5, BUG-1 … BUG-7. - -**Why:** the vulnerability is not "`plugins` is dangerous" — it is that the function is a *denylist of nothing*. Blocking `plugins` by name leaves `user_words`, `plugin_manager`, `keep_temporary_files`, and whatever the next `ocrmypdf` release adds. The only durable fix is to enumerate what is permitted, with types and bounds, and reject everything else. - -**Shape of the change**, in `ocrservice.py`: - -```python -# Exhaustive allowlist. Anything absent is rejected with 400 — notably -# plugins, plugin_manager, user_words, user_patterns, sidecar, output_file, -# input_file and keep_temporary_files. -_ALLOWED: dict[str, _Spec] = { - "language": _Spec(list_of=str, pattern=r"\A[a-z]{3}(_[a-z]+)?\Z", max_items=8), - "image_dpi": _Spec(int, lo=50, hi=1200), - "oversample": _Spec(int, lo=0, hi=1200), - "jobs": _Spec(int, lo=1, hi=os.cpu_count() or 4), - "max_image_mpixels": _Spec(float, lo=1, hi=500), # lower bound: never 0 - "skip_big": _Spec(float, lo=0, hi=10_000), - "optimize": _Spec(int, lo=0, hi=3), - "tesseract_pagesegmode": _Spec(int, lo=0, hi=13), - "tesseract_oem": _Spec(int, lo=0, hi=3), - "tesseract_timeout": _Spec(float, lo=0, hi=MAX_TESSERACT_TIMEOUT), - "mode": _Spec(str, choices={"force", "skip", "redo"}), - "output_type": _Spec(str, choices={"pdf", "pdfa", "pdfa-1", "pdfa-2", "pdfa-3"}), - "rotate_pages": _Spec(bool), - "deskew": _Spec(bool), - "clean": _Spec(bool), - "remove_background": _Spec(bool), - "force_ocr": _Spec(bool), - "skip_text": _Spec(bool), - "redo_ocr": _Spec(bool), - # ... extend deliberately, one reviewed entry at a time -} -``` +## Revised plan -Three rules alongside it: +The original P0 was "replace `_split_parameters` with a validating allow-list parser". PR #12 plus this branch have done the **allow-list** half. The **validating** half is not done: names are checked, values are not. -1. **Tokenise with `shlex.split()`**, not `split("--")`. That alone fixes BUG-1 (truncation), BUG-3 (`1--2`) and quoting generally. -2. **Reject, don't ignore.** An unknown or out-of-range parameter returns `400` with a message naming the offender. Silent no-ops (BUG-6) are worse than errors: an admin sets `--languge deu`, sees no error, and ships broken OCR. -3. **Reject duplicates** (BUG-5) and any key that collides with a kwarg the service sets itself (BUG-7). +### P0 — Validate values, not just names -Cover it with a real unit test table — this is where BP-5 gets paid off, and these tests run in milliseconds, unlike the current end-to-end suite. +**Closes:** SEC-2, BUG-1 … BUG-5. + +The allow-list stops `--plugins`. It does nothing about `--max-image-mpixels 100000`, `--jobs 10000`, or `--optimize high` (which still reaches ocrmypdf and surfaces as a 500 from pydantic rather than a 400). + +1. Give each allow-listed parameter a type and, where it governs resource use, a **bound**: + `jobs` ≤ CPU budget, `max_image_mpixels` in `[1, 500]` — never 0 — `optimize` in `{0,1,2,3}`, `tesseract_timeout` ≤ a ceiling, enumerations checked against their choices. +2. **Tokenise with `shlex.split()`** instead of `split("--")` / `split(" ")`. One change closes BUG-1 through BUG-5 and makes quoting work. +3. Reject duplicates rather than silently overwriting. ### P1 — Put a ceiling on every resource **Closes:** SEC-3, SEC-4, BUG-8. -1. Change `async def process_ocr` to `def process_ocr` so FastAPI runs it in the threadpool. One keyword; it stops OCR from blocking `/heartbeat`, which is the difference between "slow" and "AppAPI restarts the container". -2. Enforce a **maximum upload size** — read `Content-Length`, reject over the limit before touching the body, and make the limit an env var with a sane default. Also stream the upload to a `NamedTemporaryFile` and hand `ocrmypdf` a path rather than holding it in memory. -3. Bound **concurrency** with an `asyncio.Semaphore` sized to the container's CPU budget, returning `503` when saturated rather than queueing unboundedly. `ocrmypdf`'s global `_api_lock` already serialises the work; this makes the backpressure explicit instead of accidental. -4. Apply a **wall-clock timeout** to the OCR run and a default `tesseract_timeout`. -5. Give `installed_languages`' `subprocess.run` a `timeout=` and `check=True`, and surface a failure as an error rather than an empty list. Cache the result — the language set cannot change while the container runs. +1. `def process_ocr` instead of `async def`, so FastAPI runs it in the threadpool. One keyword; it is the difference between "slow" and "AppAPI restarts the container". +2. Enforce a maximum upload size from `Content-Length` before touching the body; stream to a `NamedTemporaryFile` and give ocrmypdf a path. +3. Bound concurrency with an `asyncio.Semaphore`, returning `503` when saturated. +4. Wall-clock timeout on the OCR run, and a default `tesseract_timeout`. +5. `timeout=` and `check=True` on the `installed_languages` subprocess; cache the result. ### P2 — Tighten the response and logging boundary **Closes:** SEC-6, SEC-7, BUG-9 … BUG-12, BP-2, BP-9. -- Generic exception handler returns a fixed message plus a correlation id; the detail goes to the log. Keep the `ExitCodeException` handler's `message` + `ocrMyPdfExitCode` contract intact — the PHP client depends on it — and build both responses *through* `ErrorResult` so the model can't drift from the wire format. -- Sanitise `file.filename`: `os.path.basename`, strip control characters, cap the length, fall back to a generated name when it's `None`. -- Switch to structured logging (`logger.debug("...", name)`), which also removes the log-injection vector. -- `getLogger(__name__)`; fix the `str | None` annotations; `decode("utf-8", errors="replace")`; drop the duplicate `close()`. +Generic handler returns a fixed message plus a correlation id; keep the `ExitCodeException` and `InvalidOcrParameterError` contracts and build all three through `ErrorResult`. Sanitise `file.filename`. Structured logging throughout. Fix the `str | None` annotations. ### P3 — Supply chain and release integrity **Closes:** SEC-9, BP-7, BP-10. -- **Publish immutable image tags.** Change `` to a real version and cut a new image per release. This is the single highest-value item in this phase: today there is no such thing as "the version I have installed". -- Compile a hash-pinned lock file; install with `--require-hashes`. -- Pin every GitHub Action to a full commit SHA. -- Add an explicit least-privilege `permissions:` block to each workflow. -- Digest-pin the base image; add `--no-cache-dir`, `PIP_NO_CACHE_DIR`, and a `HEALTHCHECK`. -- Narrow the tesseract language-pack install to a declared set, or accept the image size as a documented trade-off — but stop deriving it from a live `apk search` at build time. -- Add Dependabot, CodeQL, and a container scan; document the self-hosted-runner constraint on the HaRP job. +Publish immutable image tags — the highest-value item here, since today there is no such thing as "the version I have installed". Hash-pinned lock file. SHA-pinned actions. Least-privilege `permissions:`. Digest-pinned base image. Dependabot, CodeQL, container scanning. ### P4 — Tooling and hygiene -**Closes:** BP-1, BP-3, BP-4, BP-6, BP-8, BP-11, BP-12. +**Closes:** BP-1, BP-3, BP-4, BP-5, BP-6, BP-8, BP-11, BP-12. -- `log_level` from the environment, default `info`. -- Add `ruff` + `mypy` with a CI gate. Add `__init__.py` files. -- Replace the committed `.env` with `.env.example`; gitignore `.env`. -- `start.sh`: `set -euo pipefail`, validate required env vars before writing TOML, supervise or `exec` the frpc process, `"$*"` in the echo. -- `SECURITY.md` with a disclosure address. +Env-driven `log_level`. `ruff` + `mypy` in CI. `.env.example`. `start.sh` hardening. `SECURITY.md`. --- ## What's already good -Worth stating, because a review that only lists problems misrepresents the codebase: - +- PR #12's plugin test asserts the exploit *marker file* is never created, not merely that a 400 came back. That is how a code-execution fix should be tested. - The layering is clean — the FastAPI module has no OCR logic and `OcrService` has no HTTP concerns. -- gosu is version-pinned *and* GPG signature-verified, which is rarer than it should be. -- The multi-stage Dockerfile deliberately keeps the passwordless-sudo `devcontainer`/`test` stages out of the published `app` image. -- The HaRP integration test is genuinely thorough: it stands up a real HaRP container, drives the real ExApp lifecycle, asserts on the generated `frpc.toml`, and cleans up in a `finally`. -- Direct dependencies are pinned to exact versions. -- CI runs tests in the same container image that ships, which eliminates a whole class of "works on my machine". +- gosu is version-pinned *and* GPG signature-verified. +- The multi-stage Dockerfile keeps the passwordless-sudo `devcontainer` and `test` stages out of the published `app` image. +- The HaRP integration test stands up a real HaRP container, drives the real ExApp lifecycle, asserts on the generated `frpc.toml`, and cleans up in a `finally`. +- Direct dependencies are pinned exactly, and CI runs the tests inside the image that ships. diff --git a/test/test_ocrservice.py b/test/test_ocrservice.py index 37ed692..85fd394 100644 --- a/test/test_ocrservice.py +++ b/test/test_ocrservice.py @@ -1,6 +1,10 @@ +import inspect import logging import pytest +import ocrmypdf +from ocrmypdf._options import OcrOptions + from workflow_ocr_backend.ocrservice import InvalidOcrParameterError, OcrService service = OcrService(logging.getLogger(__name__)) @@ -21,6 +25,10 @@ def test_split_parameters_none(): "--sidecar /tmp/out.txt", "--output-file /tmp/out.pdf", "--progress-bar", + # tesseract_config is appended verbatim to the tesseract argv, so a caller-supplied + # value is an arbitrary config-file path in the same way user_words is. + "--tesseract-config /tmp/evil.conf", + "--tesseract-config /tmp/a+/tmp/b", ]) def test_split_parameters_rejects_blocked_parameters(parameters): # These parameters would allow the caller to execute arbitrary code (plugins), @@ -45,6 +53,9 @@ def test_split_parameters_rejects_unknown_parameters(parameters): "--language ../../etc/passwd", "--language -eng", "--language 123", + # '$' in a regex also matches before a trailing newline, so this passed while the + # check used re.match instead of re.fullmatch. + "--language eng\n+deu", ]) def test_split_parameters_rejects_invalid_languages(parameters): # Language values must match the allow-list pattern used by the Nextcloud app, @@ -60,3 +71,47 @@ def test_split_parameters_rejects_invalid_languages(parameters): ]) def test_split_parameters_accepts_valid_languages(parameters, expected): assert service._split_parameters(parameters) == {"language": expected} + +@pytest.mark.parametrize("parameters,expected", [ + # --ocr-engine is a documented CLI flag and a real OcrOptions field, but it is not a + # keyword argument of ocrmypdf.ocr(), so a signature-derived allow-list rejects it. + ("--ocr-engine none", {"ocr_engine": "none"}), + # --jpeg-quality is the documented spelling; --jpg-quality is the hidden alias. + # Both must be accepted, and both must arrive as the keyword ocrmypdf.ocr() takes. + ("--jpeg-quality 80", {"jpg_quality": 80}), + ("--jpg-quality 80", {"jpg_quality": 80}), +]) +def test_split_parameters_accepts_documented_cli_names(parameters, expected): + assert service._split_parameters(parameters) == expected + +@pytest.mark.parametrize("parameters,expected", [ + ("--quiet", {}), + ("--verbose", {}), + ("--quiet --language eng", {"language": "eng"}), +]) +def test_split_parameters_drops_cli_only_flags(parameters, expected): + # CLI-only logging flags have no OCRmyPDF API equivalent. They are dropped rather than + # rejected so that existing workflow configurations carrying them keep working. + assert service._split_parameters(parameters) == expected + +def test_allowed_parameters_still_resolve_against_installed_ocrmypdf(): + # The allow-list is an explicit literal, so an OCRmyPDF upgrade cannot silently widen + # it. This guard catches the opposite risk: an upgrade renaming or removing an option + # would otherwise leave a dead entry that 400s at runtime with no test failure. + ocr_keywords = { + name for name, param in inspect.signature(ocrmypdf.ocr).parameters.items() + if param.kind is inspect.Parameter.KEYWORD_ONLY + } + option_fields = set(OcrOptions.model_fields.keys()) + unresolved = sorted( + name for name in OcrService.ALLOWED_PARAMETERS + if OcrService.PARAMETER_ALIASES.get(name, name) not in ocr_keywords | option_fields + ) + assert not unresolved, ( + f"Allow-listed parameters no longer accepted by ocrmypdf {ocrmypdf.__version__}: " + f"{unresolved}. Check whether they were renamed or removed." + ) + +def test_blocked_and_allowed_parameters_are_disjoint(): + assert not (OcrService.ALLOWED_PARAMETERS & OcrService.BLOCKED_PARAMETERS) + assert not (OcrService.ALLOWED_PARAMETERS & OcrService.IGNORED_PARAMETERS) diff --git a/workflow_ocr_backend/ocrservice.py b/workflow_ocr_backend/ocrservice.py index fd66c4b..41fc08b 100644 --- a/workflow_ocr_backend/ocrservice.py +++ b/workflow_ocr_backend/ocrservice.py @@ -1,7 +1,6 @@ import base64 from datetime import datetime, timezone -import inspect import io from logging import Logger import re @@ -20,10 +19,12 @@ class OcrService: # which could be (ab)used as shell metacharacters never reach the OCR engine. LANGUAGE_CODE_REGEX = re.compile(r"^[A-Za-z][A-Za-z0-9_/]{0,31}$") - # Parameters which must never be taken from a request, even though ocrmypdf.ocr() accepts them: + # Parameters which must never be taken from a request, even though OCRmyPDF accepts them: # * plugins/plugin_manager load arbitrary Python code => remote code execution # * input/output/sidecar/progress_bar are controlled by this service # * user_words/user_patterns/keep_temporary_files give access to the backend's filesystem + # * tesseract_config is appended verbatim to the tesseract argv (see _exec/tesseract.py), + # so a caller-supplied value is an arbitrary config-file path just like user_words BLOCKED_PARAMETERS = frozenset({ "plugins", "plugin_manager", @@ -36,15 +37,49 @@ class OcrService: "user_words", "user_patterns", "keep_temporary_files", + "tesseract_config", }) - # Everything OCRmyPDF documents as a keyword argument of ocrmypdf.ocr(), minus the blocked ones. - # Unknown parameters are rejected instead of being silently forwarded, so that neither typos nor - # future (potentially dangerous) OCRmyPDF options can be smuggled in via the request. - ALLOWED_PARAMETERS = frozenset( - name for name, param in inspect.signature(ocrmypdf.ocr).parameters.items() - if param.kind is inspect.Parameter.KEYWORD_ONLY - ) - BLOCKED_PARAMETERS + # Allow-list of OCRmyPDF *CLI option* names (normalised: '-' replaced by '_'), because that + # is what callers send. Deliberately an explicit literal instead of introspecting + # ocrmypdf.ocr(): its Python keyword names differ from the documented CLI spellings + # (e.g. --jpeg-quality vs jpg_quality, --ocr-engine is not a keyword argument at all), + # and introspection would silently widen this set on every OCRmyPDF upgrade. + # test_ocrservice.py asserts every entry still resolves against the installed OCRmyPDF. + ALLOWED_PARAMETERS = frozenset({ + # Language and OCR engine selection + "language", "ocr_engine", "mode", "force_ocr", "skip_text", "redo_ocr", + "pages", "skip_big", + # Image preprocessing + "image_dpi", "oversample", "deskew", "clean", "clean_final", "unpaper_args", + "remove_background", "remove_vectors", "rotate_pages", "rotate_pages_threshold", + # Tesseract tuning + "tesseract_oem", "tesseract_pagesegmode", "tesseract_thresholding", + "tesseract_timeout", "tesseract_non_ocr_timeout", + "tesseract_downsample_above", "tesseract_downsample_large_images", + # Output and PDF generation + "output_type", "pdf_renderer", "rasterizer", "pdfa_image_compression", + "color_conversion_strategy", "tagged_pdf_mode", "fast_web_view", "no_overwrite", + "invalidate_digital_signatures", "continue_on_soft_render_error", + # Optimisation + "optimize", "jpeg_quality", "jpg_quality", "png_quality", + "jbig2_lossy", "jbig2_page_group_size", "jbig2_threshold", + # Document metadata + "title", "author", "subject", "keywords", + # Resource usage + "jobs", "use_threads", "max_image_mpixels", + }) + + # Documented CLI option name -> ocrmypdf.ocr() keyword argument, where the two differ. + # --jpeg-quality is the documented flag; --jpg-quality is its hidden (argparse.SUPPRESS) + # alias and the only spelling the Python signature exposes. + PARAMETER_ALIASES = { + "jpeg_quality": "jpg_quality", + } + + # CLI-only flags with no OCRmyPDF API equivalent. Accepted and dropped rather than + # rejected, so existing workflow configurations carrying them keep working. + IGNORED_PARAMETERS = frozenset({"quiet", "verbose", "no_progress_bar"}) LANGUAGE_PARAMETERS = frozenset({"language"}) @@ -115,9 +150,13 @@ def _split_parameters(self, ocrmypdf_parameters: str) -> dict[str, str | bool | # Flag value = True + if key in self.IGNORED_PARAMETERS: + self.logger.debug("Ignoring CLI-only OCR parameter %r", key) + continue + self._check_parameter(key, value) - params[key] = value + params[self.PARAMETER_ALIASES.get(key, key)] = value return params def _check_parameter(self, key: str, value: str | bool | Iterable[str] | int | float) -> None: @@ -127,17 +166,21 @@ def _check_parameter(self, key: str, value: str | bool | Iterable[str] | int | f and are used to invoke the OCR engine (which in turn spawns subprocesses), so only known-good parameters and language codes may pass. """ + # Note: %r rather than an f-string, so control characters in the caller-supplied + # key are escaped instead of forging additional log lines. if key in self.BLOCKED_PARAMETERS: - self.logger.warning(f"Rejected blocked OCR parameter '{key}'") + self.logger.warning("Rejected blocked OCR parameter %r", key) raise InvalidOcrParameterError(f"Parameter '{key}' is not allowed") if key not in self.ALLOWED_PARAMETERS: - self.logger.warning(f"Rejected unknown OCR parameter '{key}'") + self.logger.warning("Rejected unknown OCR parameter %r", key) raise InvalidOcrParameterError(f"Unknown parameter '{key}'") if key in self.LANGUAGE_PARAMETERS: languages = value if isinstance(value, list) else [value] for language in languages: - if not isinstance(language, str) or not self.LANGUAGE_CODE_REGEX.match(language): - self.logger.warning(f"Rejected invalid OCR language value: {language!r}") + # fullmatch, not match: '$' would also match before a trailing newline, + # so re.match would accept 'eng\n' (reachable via '--language eng\n+deu'). + if not isinstance(language, str) or not self.LANGUAGE_CODE_REGEX.fullmatch(language): + self.logger.warning("Rejected invalid OCR language value: %r", language) raise InvalidOcrParameterError(f"Invalid language value '{language}'") From c05d607058625aa5e52943825bb83e4763805e9c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 19:11:00 +0000 Subject: [PATCH 4/6] feat: redesign OCR API around a typed, closed OcrOptions schema PR #12/#13 closed the reachable --plugins RCE with a hand-maintained allow-list, but the endpoint's contract was still "send me a string, I will parse it into kwargs and splat them into ocrmypdf.ocr(**kwargs)". The allow-list validated parameter *names*; values (--max-image-mpixels, --jobs, --tesseract-timeout) were still fully caller-controlled. This introduces POST /v1/ocr: a hand-written Pydantic OcrOptions model (extra="forbid", every scalar bounded/enumerated/regex-constrained, no path- or argv-typed field) that is the API's own contract rather than something derived from ocrmypdf's signature. Resource limits (jobs, max_image_mpixels, the tesseract_timeout ceiling) move into an OcrPolicy built from environment variables - operator policy, never a request field; a caller-supplied timeout is clamped, never honoured upward. Mapping to ocrmypdf kwargs is written out field by field, with no **caller_data splat anywhere. Two structural invariants are enforced in CI: no field may carry a Path/PathLike type (test_no_path_typed_fields), and test/ocrmypdf_signature.json snapshots ocrmypdf's keyword-only parameters so an upstream release that adds one fails the build instead of silently widening what's reachable. /process_ocr stays as a deprecated shim (Deprecation/Sunset/Link headers): the legacy --flag string is tokenised with shlex and translated field-by-field onto OcrOptions through an explicit table, so anything not in that table (--plugins, --tesseract-config, --unpaper-args, any operator-owned knob) is a 400 by construction. This also fixes the old tokenizer's silent multi-word truncation and duplicate-key overwrite bugs. Also: both endpoints are now plain `def` so FastAPI runs the blocking ocrmypdf.ocr() call in the threadpool instead of stalling the event loop (and AppAPI's /heartbeat poll) for the run's duration; the generic exception handler no longer echoes str(exc) (internal paths) at 500, returning a correlation id instead and logging detail server-side. See doc/DESIGN.md for the full rationale. --- README.md | 41 +++- doc/DESIGN.md | 111 ++++++++++ requirements.txt | 3 +- test/ocrmypdf_signature.json | 21 ++ test/test_app.py | 67 ++++++ test/test_legacy.py | 115 ++++++++++ test/test_ocroptions.py | 187 +++++++++++++++++ test/test_ocrservice.py | 117 ----------- workflow_ocr_backend/app.py | 122 +++++++++-- workflow_ocr_backend/legacy.py | 159 ++++++++++++++ workflow_ocr_backend/ocroptions.py | 325 +++++++++++++++++++++++++++++ workflow_ocr_backend/ocrservice.py | 178 +++------------- 12 files changed, 1157 insertions(+), 289 deletions(-) create mode 100644 doc/DESIGN.md create mode 100644 test/ocrmypdf_signature.json create mode 100644 test/test_legacy.py create mode 100644 test/test_ocroptions.py delete mode 100644 test/test_ocrservice.py create mode 100644 workflow_ocr_backend/legacy.py create mode 100644 workflow_ocr_backend/ocroptions.py diff --git a/README.md b/README.md index d591d37..41474a9 100644 --- a/README.md +++ b/README.md @@ -177,11 +177,36 @@ HaRP simplifies deployment and improves performance by enabling direct communica For installation and migration instructions, see the [HaRP documentation](https://github.com/nextcloud/HaRP#readme). -## OCR Parameter Validation - -The `ocrmypdf_parameters` sent to `/process_ocr` are validated before they are handed over to OCRmyPDF: - -- Only parameters on an explicit allow-list of documented [OCRmyPDF CLI options](https://ocrmypdf.readthedocs.io/en/latest/cookbook.html) are accepted. Unknown parameters are rejected with HTTP `400` instead of being silently ignored. The allow-list is a literal set rather than something derived from OCRmyPDF at runtime, so an OCRmyPDF upgrade can never widen it without review. -- The parameters `plugins`, `plugin_manager`, `user_words`, `user_patterns`, `keep_temporary_files`, `tesseract_config` as well as the input/output/sidecar parameters (which are controlled by this app) are never accepted from a request. `--plugins` in particular would make OCRmyPDF load and execute arbitrary Python code; `--tesseract-config` and `--user-words` would expose the backend's filesystem to the caller. -- CLI-only flags with no API equivalent (`--quiet`, `--verbose`, `--no-progress-bar`) are accepted and ignored rather than rejected. -- Language codes must match `^[A-Za-z][A-Za-z0-9_/]{0,31}$` (e.g. `eng`, `chi_sim`, `script/Latin`), which is the same allow-list the [workflow_ocr](https://github.com/R0Wi-DEV/workflow_ocr) Nextcloud App uses. +## OCR API + +`POST /v1/ocr` is the current API: a multipart `file` plus an `options` part holding a JSON object +validated against a typed, closed schema (`OcrOptions`, see +[`workflow_ocr_backend/ocroptions.py`](workflow_ocr_backend/ocroptions.py) and +[`doc/DESIGN.md`](doc/DESIGN.md) for the full rationale). Unknown fields are rejected with `422` +rather than forwarded, every scalar is bounded or enumerated, and resource limits (`jobs`, +`max_image_mpixels`, the `tesseract_timeout` ceiling) are operator policy set via environment +variables (`OCR_JOBS`, `OCR_MAX_IMAGE_MPIXELS`, `OCR_MAX_TESSERACT_TIMEOUT_S`), never part of the +request body. See `/docs` on a running instance for the generated OpenAPI schema. + +### Legacy `/process_ocr` (deprecated) + +`POST /process_ocr` accepts the older `ocrmypdf_parameters` flag string (e.g. +`--skip-text --tesseract-pagesegmode 7 --language eng`) for backward compatibility. It is a thin +shim: the string is parsed and translated onto the same `OcrOptions` schema `/v1/ocr` uses, so it +inherits every validation rule from that schema rather than maintaining a separate allow/deny list. +Responses carry `Deprecation`/`Sunset`/`Link` headers pointing at `/v1/ocr`. Concretely: + +- Only an explicit, hand-maintained table of legacy flag names is translated into schema fields. + A flag that isn't in that table - `plugins`, `plugin_manager`, `user_words`, `user_patterns`, + `keep_temporary_files`, `tesseract_config`, `unpaper_args`, the input/output/sidecar parameters, + and any operator-owned resource knob (`jobs`, `max_image_mpixels`) - can never reach OCRmyPDF, + because there is no path in the shim that puts it on the schema. `--plugins` in particular would + make OCRmyPDF load and execute arbitrary Python code; `--tesseract-config` and `--user-words` + would expose the backend's filesystem to the caller. +- CLI-only flags with no API equivalent (`--quiet`, `--verbose`, `--no-progress-bar`) are accepted + and ignored rather than rejected. +- Language codes must match `^[A-Za-z][A-Za-z0-9_]{0,31}$` (or `script/`, e.g. `chi_sim`, + `script/Latin`), the same allow-list the [workflow_ocr](https://github.com/R0Wi-DEV/workflow_ocr) + Nextcloud App uses. +- Values are parsed with `shlex.split`, so a quoted value survives intact; duplicate flags and + unquoted multi-word values are now a `400` instead of being silently truncated or overwritten. diff --git a/doc/DESIGN.md b/doc/DESIGN.md new file mode 100644 index 0000000..1f8af9b --- /dev/null +++ b/doc/DESIGN.md @@ -0,0 +1,111 @@ +# Redesigning the `workflow_ocr_backend` OCR API + +## The bug class, not the bug + +[`doc/CODE_REVIEW.md`](CODE_REVIEW.md) closes the reachable RCE (`--plugins /tmp/evil.py`) and +tightens the parameter allow-list to CLI names. But the shape that produced the bug survived that +fix. The endpoint's contract was, effectively: + +> Send me a string. I will parse it into keyword arguments and splat them into a third-party +> function. + +```python +kwargs = self._split_parameters(ocrmypdf_parameters) +exit_code = ocrmypdf.ocr(file, output_buffer, sidecar=..., **kwargs) +``` + +Three compounding properties made this a recurring RCE generator rather than a one-off mistake: + +1. **The sink is unbounded.** `ocrmypdf.ocr()` ends in `**kwargs`, and unknown keys are forwarded + to `create_options`. It loads plugins and shells out to tesseract, ghostscript, unpaper, + pngquant and jbig2enc. Its parameter list was never designed to be a trust boundary. +2. **The transport was untyped.** A CLI-ish string parsed by `split("--")` → `split(" ")` → + shape-guessed types. Multi-token values were silently truncated, `+` turned any value into a + list, and a value containing `--` corrupted the whole parse. +3. **An allow-list only checks names, never values.** `--max-image-mpixels 100000` or + `--jobs 10000` still reached OCRmyPDF: the decompression-bomb guard and the worker-count knob + were request fields, not operator policy. + +## The redesign + +**The API exposes a small closed vocabulary of OCR intents. It does not expose the dependency's +function signature, and resource limits are not caller options.** + +### 1. Typed options, not a flag string + +`POST /v1/ocr`, multipart: `file` plus an `options` part of `application/json` validated by a +hand-written Pydantic model with `extra="forbid"` +([`workflow_ocr_backend/ocroptions.py`](../workflow_ocr_backend/ocroptions.py)). Unknown key → 422 +with a field path, never a silent forward. FastAPI generates the OpenAPI schema from the model, so +callers get a real contract instead of a doc link to the OCRmyPDF cookbook. + +### 2. Constraints live in the types + +Every scalar is bounded (`optimize: 0–3`, `tesseract_pagesegmode: 0–13`), every enumerated value is +a real enum, every string is a regex-constrained alias (`LanguageCode`, `PageRange`). Invalid states +are unrepresentable rather than rejected at runtime: `skip_text`/`force_ocr`/`redo_ocr` - three +booleans the legacy API let you set simultaneously, producing a 500 from OCRmyPDF's own validation +- collapse into one `TextMode` enum (`None` is a deliberate fourth state: "no override", matching +OCRmyPDF's own conservative default of refusing to touch a document that already has text). + +### 3. Caller intent vs. operator policy + +The split the legacy design lacked entirely. `jobs`, `max_image_mpixels` and the +`tesseract_timeout` ceiling live in `OcrPolicy`, built from environment variables +(`OCR_JOBS`, `OCR_MAX_IMAGE_MPIXELS`, `OCR_MAX_TESSERACT_TIMEOUT_S`) once at startup. A +caller-supplied timeout is *clamped*, never honoured upward +(`test_caller_cannot_raise_the_timeout_ceiling`). A test asserts that no request field can +influence any operator-owned kwarg (`test_operator_owned_kwargs_come_only_from_policy`). + +### 4. Explicit mapping, no reflection + +`OcrOptions.to_ocrmypdf_kwargs()` is written out field by field. No `**caller_data` anywhere. +Adding an option is a deliberate edit in three places (the field, the mapping, the frozen +`EMITTABLE_OCRMYPDF_KWARGS` set). This also decouples the public vocabulary from upstream's +representation: the API says `"sauvola"`, the mapping converts it to the `IntEnum` value `2` +OCRmyPDF actually wants; the public field is named `jpeg_quality` after the documented CLI flag, +and mapped to the `jpg_quality` keyword OCRmyPDF's Python signature actually exposes. + +### 5. Two structural invariants, enforced in CI + +- **No path-typed field.** `test_no_path_typed_fields` walks `OcrOptions.model_fields` and fails + on any `Path`/`PathLike`/`FilePath` annotation. `--plugins` and `--user-words` were both "a path + in the request body"; ban the shape, not the instances. +- **Signature drift breaks the build.** [`test/ocrmypdf_signature.json`](../test/ocrmypdf_signature.json) + snapshots upstream's keyword-only parameters; `test_ocrmypdf_signature_has_not_drifted` diffs + against it. This is the exact inverse of a derived allow-list: an OCRmyPDF upgrade that adds a + parameter *fails CI* and someone has to review it and update the snapshot deliberately, instead + of the boundary silently widening on `pip upgrade`. + +### 6. Language validation against reality + +`OcrOptions.validate_against_policy()` intersects requested languages with +`OcrPolicy.installed_languages`, cached from `tesseract --list-langs` at startup. Turns "language +not installed" from an OCRmyPDF-side 500 into an application-level 400. + +## Migration: the legacy shim + +Since the old contract can break callers, `/process_ocr` stays as a thin deprecated shim +([`workflow_ocr_backend/legacy.py`](../workflow_ocr_backend/legacy.py)) rather than being removed: +the flag string is tokenised (`shlex.split`, fixing the silent-truncation and corrupted-parse bugs +in the old tokenizer) and translated field-by-field onto `OcrOptions` through an explicit table. +Anything not in that table - `--plugins`, `--tesseract-config`, `--unpaper-args`, any +operator-owned knob - becomes a 400 by construction, because no code path ever puts it on the +model. Responses carry `Deprecation`/`Sunset`/`Link` headers pointing at `/v1/ocr`. + +## Beyond the API surface + +Two items from the schema's own blast radius were fixed alongside it: + +- **`process_ocr` was `async def` but called blocking `ocrmypdf.ocr()`**, stalling the event loop - + and AppAPI's `/heartbeat` poll - for the duration of every OCR run. Both `/v1/ocr` and + `/process_ocr` are now plain `def`, so FastAPI runs them in the threadpool. +- **The generic exception handler echoed `str(exc)` at 500**, which routinely carries absolute + temp paths and library internals. It now logs the detail server-side against a correlation id and + returns only `"Internal server error []"`. The `ExitCodeException` handler is unchanged - its + message is a deliberate part of the API contract, not a leak. + +Everything else flagged in `doc/CODE_REVIEW.md` under P1–P4 (upload size limits, concurrency +bounds, a sandboxed worker process for the OCRmyPDF subprocess fan-out, filename sanitisation, +supply-chain pinning) is still open and tracked there; this redesign is scoped to the request +schema and the two items above. diff --git a/requirements.txt b/requirements.txt index 5693f7d..cdc5af6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ nc-py-api[app]==0.30.1 ocrmypdf==17.4.2 -python-multipart==0.0.20 \ No newline at end of file +python-multipart==0.0.20 +pydantic==2.13.4 \ No newline at end of file diff --git a/test/ocrmypdf_signature.json b/test/ocrmypdf_signature.json new file mode 100644 index 0000000..f57e210 --- /dev/null +++ b/test/ocrmypdf_signature.json @@ -0,0 +1,21 @@ +{ + "_comment": "Snapshot of ocrmypdf.ocr() keyword-only parameters. Regenerate DELIBERATELY after reviewing each added parameter for path/plugin/argv semantics.", + "ocrmypdf_version": "17.4.2", + "keyword_only_parameters": [ + "author", "clean", "clean_final", "color_conversion_strategy", + "continue_on_soft_render_error", "deskew", "fast_web_view", "force_ocr", + "image_dpi", "invalidate_digital_signatures", "jbig2_lossy", + "jbig2_page_group_size", "jbig2_threshold", "jobs", "jpg_quality", + "keep_temporary_files", "keywords", "language", + "max_image_mpixels", "mode", "no_overwrite", "optimize", "output_type", + "oversample", "pages", "pdf_renderer", "pdfa_image_compression", + "plugin_manager", "plugins", "png_quality", "progress_bar", "rasterizer", + "redo_ocr", "remove_background", "remove_vectors", "rotate_pages", + "rotate_pages_threshold", "sidecar", "skip_big", "skip_text", "subject", + "tagged_pdf_mode", "tesseract_config", "tesseract_downsample_above", + "tesseract_downsample_large_images", "tesseract_non_ocr_timeout", + "tesseract_oem", "tesseract_pagesegmode", "tesseract_thresholding", + "tesseract_timeout", "title", "unpaper_args", "use_threads", "user_patterns", + "user_words" + ] +} diff --git a/test/test_app.py b/test/test_app.py index e9350ea..a24bcd6 100644 --- a/test/test_app.py +++ b/test/test_app.py @@ -107,6 +107,73 @@ def test_process_ocr_rejects_injected_language(): assert response.status_code == 400 assert response.json()["message"].startswith("Invalid language value '$(id)'") +def test_process_ocr_deprecation_headers(): + # The legacy endpoint is kept as a shim, but must advertise that it is one. + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers) as client: + response = client.post( + "/process_ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"ocrmypdf_parameters": "--skip-text --language eng"} + ) + assert response.status_code == 200 + assert response.headers["Deprecation"] == "true" + assert "successor-version" in response.headers["Link"] + +def test_ocr_v1(): + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + ocr_content = "This document is ready for OCR\n" + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers) as client: + response = client.post( + "/v1/ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"options": '{"mode": "skip-text", "languages": ["eng"], "tesseract_pagesegmode": 7}'} + ) + assert response.status_code == 200 + response_json = response.json() + assert "recognizedText" in response_json + assert response_json["recognizedText"] == ocr_content + +def test_ocr_v1_defaults_to_english_when_no_options_given(): + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + ocr_content = "This document is ready for OCR\n" + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers) as client: + response = client.post( + "/v1/ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"options": '{"mode": "skip-text"}'} + ) + assert response.status_code == 200 + assert response.json()["recognizedText"] == ocr_content + +def test_ocr_v1_rejects_unknown_option_field(): + # extra="forbid" - an unknown key is a 422, never silently forwarded. + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers, raise_server_exceptions=False) as client: + response = client.post( + "/v1/ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"options": '{"plugins": "/tmp/evil.py"}'} + ) + assert response.status_code == 422 + assert response.json()["errors"][0]["loc"] == ["plugins"] + +def test_ocr_v1_rejects_uninstalled_language(): + current_dir = os.path.dirname(__file__) + file_name = "document-ready-for-ocr.pdf" + with open(f"{current_dir}/testdata/{file_name}", "rb") as file, TestClient(APP, headers=headers, raise_server_exceptions=False) as client: + response = client.post( + "/v1/ocr", + files={"file": (file_name, file, "application/pdf")}, + data={"options": '{"languages": ["jpn"]}'} + ) + assert response.status_code == 400 + assert "not installed" in response.json()["message"] + def test_installed_languages(): with TestClient(APP, headers=headers) as client: response = client.get("/installed_languages") diff --git a/test/test_legacy.py b/test/test_legacy.py new file mode 100644 index 0000000..ee95744 --- /dev/null +++ b/test/test_legacy.py @@ -0,0 +1,115 @@ +import pytest + +from workflow_ocr_backend.legacy import InvalidOcrParameterError, options_from_legacy_parameters +from workflow_ocr_backend.ocroptions import TextMode + + +def test_options_from_legacy_parameters_none(): + options = options_from_legacy_parameters(None) + assert options.languages == ["eng"] + # No mode flag given -> no override, matching ocrmypdf's own conservative + # default of erroring on an already-processed document. + assert options.mode is None + + +def test_options_from_legacy_parameters_valid(): + options = options_from_legacy_parameters("--skip-text --tesseract-pagesegmode 7 --language eng+chi_sim") + assert options.mode == TextMode.SKIP + assert options.tesseract_pagesegmode == 7 + assert options.languages == ["eng", "chi_sim"] + + +def test_options_from_legacy_parameters_force_and_redo_ocr(): + assert options_from_legacy_parameters("--force-ocr").mode == TextMode.FORCE + assert options_from_legacy_parameters("--redo-ocr").mode == TextMode.REDO + + +@pytest.mark.parametrize("parameters", [ + "--plugins /tmp/evil.py", + "--plugin-manager foo", + "--user-words /etc/passwd", + "--user-patterns /etc/passwd", + "--keep-temporary-files", + "--sidecar /tmp/out.txt", + "--output-file /tmp/out.pdf", + # tesseract_config is appended verbatim to the tesseract argv, so a caller-supplied + # value is an arbitrary config-file path in the same way user_words is. + "--tesseract-config /tmp/evil.conf", + "--unpaper-args --layout single", +]) +def test_options_from_legacy_parameters_rejects_dangerous_parameters(parameters): + # These parameters aren't in the translation table at all, so they can never + # reach OcrOptions - the shim's allow-list is what it can express, not a + # denylist of what it blocks. + with pytest.raises(InvalidOcrParameterError): + options_from_legacy_parameters(parameters) + + +@pytest.mark.parametrize("parameters", [ + "--not-an-ocrmypdf-parameter", + "--some-unknown-option value", + # Operator-owned resource knobs are not caller options at all anymore. + "--jobs 10000", + "--max-image-mpixels 100000", +]) +def test_options_from_legacy_parameters_rejects_unknown_parameters(parameters): + with pytest.raises(InvalidOcrParameterError): + options_from_legacy_parameters(parameters) + + +@pytest.mark.parametrize("parameters", [ + "--language eng;id", + "--language $(id)", + "--language `id`", + "--language |id", + "--language eng+;id", + "--language ../../etc/passwd", + "--language -eng", +]) +def test_options_from_legacy_parameters_rejects_invalid_languages(parameters): + with pytest.raises(InvalidOcrParameterError, match="Invalid language value"): + options_from_legacy_parameters(parameters) + + +@pytest.mark.parametrize("parameters,expected", [ + ("--language eng", ["eng"]), + ("--language chi_sim", ["chi_sim"]), + ("--language eng+deu", ["eng", "deu"]), +]) +def test_options_from_legacy_parameters_accepts_valid_languages(parameters, expected): + assert options_from_legacy_parameters(parameters).languages == expected + + +@pytest.mark.parametrize("parameters,expected", [ + ("--jpeg-quality 80", 80), + ("--jpg-quality 80", 80), +]) +def test_options_from_legacy_parameters_accepts_both_jpeg_quality_spellings(parameters, expected): + # --jpeg-quality is the documented CLI flag; --jpg-quality is its hidden + # argparse.SUPPRESS-ed alias. Both must land on the same field. + assert options_from_legacy_parameters(parameters).jpeg_quality == expected + + +@pytest.mark.parametrize("parameters", ["--quiet", "--verbose", "--no-progress-bar"]) +def test_options_from_legacy_parameters_drops_cli_only_flags(parameters): + # CLI-only logging flags have no OCRmyPDF API equivalent. They are dropped + # rather than rejected so existing workflow configurations keep working. + options = options_from_legacy_parameters(f"{parameters} --language eng") + assert options.languages == ["eng"] + + +def test_options_from_legacy_parameters_rejects_duplicate_flags(): + with pytest.raises(InvalidOcrParameterError, match="Duplicate"): + options_from_legacy_parameters("--language eng --language deu") + + +def test_options_from_legacy_parameters_rejects_unquoted_multi_word_value(): + # Unquoted multi-word values used to be silently truncated to their first + # token. They are now a 400 - the caller must quote the value. + with pytest.raises(InvalidOcrParameterError): + options_from_legacy_parameters("--title Hello World") + + +def test_options_from_legacy_parameters_accepts_quoted_multi_word_value(): + options = options_from_legacy_parameters('--title "Hello World"') + assert options.title == "Hello World" diff --git a/test/test_ocroptions.py b/test/test_ocroptions.py new file mode 100644 index 0000000..e6e4694 --- /dev/null +++ b/test/test_ocroptions.py @@ -0,0 +1,187 @@ +""" +These tests are the load-bearing part of the design. + +The schema alone stops today's RCE. These tests stop tomorrow's, by making the +trust boundary something that *fails CI when it moves* rather than something +that silently follows a dependency. +""" + +import json +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from workflow_ocr_backend.ocroptions import ( + EMITTABLE_OCRMYPDF_KWARGS, + NEVER_EMITTED, + OPERATOR_OWNED, + OcrOptions, + OcrPolicy, + TextMode, +) + +POLICY = OcrPolicy(installed_languages=frozenset({"eng", "deu", "chi_sim", "script/Latin"})) + + +# --------------------------------------------------------------------------- +# Invariant 1: the mapping can only ever produce keys from the frozen literal. +# --------------------------------------------------------------------------- + + +def test_mapping_never_emits_outside_the_frozen_set(): + """Exhaustive over the schema: set every field, check nothing new appears.""" + maximal = OcrOptions( + languages=["eng", "deu"], + mode=TextMode.FORCE, + pages="1-4,7", + rotate_pages=True, + rotate_pages_threshold=12.0, + deskew=True, + clean=True, + clean_final=True, + remove_background=True, + remove_vectors=True, + oversample_dpi=400, + image_dpi=300, + optimize=3, + jpeg_quality=80, + png_quality=80, + tesseract_pagesegmode=7, + tesseract_oem=1, + tesseract_thresholding="sauvola", + tesseract_timeout_s=60.0, + title="t", + author="a", + subject="s", + keywords="k", + ) + emitted = set(maximal.to_ocrmypdf_kwargs(POLICY)) + assert emitted <= EMITTABLE_OCRMYPDF_KWARGS, emitted - EMITTABLE_OCRMYPDF_KWARGS + + +@pytest.mark.parametrize("forbidden", sorted(NEVER_EMITTED)) +def test_forbidden_kwargs_are_not_emittable(forbidden): + assert forbidden not in EMITTABLE_OCRMYPDF_KWARGS, NEVER_EMITTED[forbidden] + + +def test_operator_owned_kwargs_come_only_from_policy(): + """Two very different requests must produce identical operator-owned values.""" + a = OcrOptions().to_ocrmypdf_kwargs(POLICY) + b = OcrOptions( + languages=["deu"], mode=TextMode.REDO, optimize=3, tesseract_timeout_s=3600.0 + ).to_ocrmypdf_kwargs(POLICY) + for key in OPERATOR_OWNED: + assert a[key] == b[key], f"caller influenced operator-owned kwarg {key!r}" + + +def test_caller_cannot_raise_the_timeout_ceiling(): + policy = OcrPolicy(max_tesseract_timeout_s=30.0, installed_languages=frozenset({"eng"})) + kwargs = OcrOptions(tesseract_timeout_s=3600.0).to_ocrmypdf_kwargs(policy) + assert kwargs["tesseract_timeout"] == 30.0 + + +def test_jpeg_quality_field_maps_to_the_kwarg_ocrmypdf_actually_takes(): + """Public field is named after the documented CLI flag; the Python keyword differs.""" + kwargs = OcrOptions(jpeg_quality=80).to_ocrmypdf_kwargs(POLICY) + assert kwargs["jpg_quality"] == 80 + assert "jpeg_quality" not in kwargs + + +# --------------------------------------------------------------------------- +# Invariant 2: no field in the schema can carry a path or free-form argv. +# --------------------------------------------------------------------------- + + +def test_no_path_typed_fields(): + """A path-typed field is how --plugins and --user-words got in. Ban the shape.""" + banned = {"Path", "PurePath", "PathLike", "FilePath", "DirectoryPath", "AnyUrl"} + for name, field in OcrOptions.model_fields.items(): + rendered = str(field.annotation) + assert not (banned & set(rendered.replace("'", " ").split())), ( + f"field {name!r} is path-typed: {rendered}" + ) + + +def test_unknown_fields_are_rejected_not_forwarded(): + with pytest.raises(ValidationError): + OcrOptions(plugins="/tmp/evil.py") + with pytest.raises(ValidationError): + OcrOptions(tesseract_config=["/etc/passwd"]) + with pytest.raises(ValidationError): + OcrOptions(unpaper_args="--layout single") + + +@pytest.mark.parametrize( + "language", + ["eng;id", "$(id)", "`id`", "|id", "../../etc/passwd", "-eng", "123", "eng+deu", "e" * 64], +) +def test_language_allowlist(language): + with pytest.raises(ValidationError): + OcrOptions(languages=[language]) + + +def test_unavailable_language_is_a_client_error(): + opts = OcrOptions(languages=["jpn"]) # syntactically fine + with pytest.raises(ValueError, match="not installed"): + opts.validate_against_policy(POLICY) + + +@pytest.mark.parametrize("pages", ["1;id", "$(id)", "1-4 7", "../1", "a-b"]) +def test_page_range_allowlist(pages): + with pytest.raises(ValidationError): + OcrOptions(pages=pages) + + +def test_mutually_exclusive_modes_are_unrepresentable(): + """The legacy API accepted skip_text and force_ocr together. This one can't.""" + assert set(TextMode) == {TextMode.SKIP, TextMode.FORCE, TextMode.REDO} + kwargs = OcrOptions(mode=TextMode.FORCE).to_ocrmypdf_kwargs(POLICY) + assert kwargs["force_ocr"] is True + assert "skip_text" not in kwargs and "redo_ocr" not in kwargs + + +# --------------------------------------------------------------------------- +# Invariant 3: upstream drift is a human decision, not a silent widening. +# --------------------------------------------------------------------------- + +SNAPSHOT = Path(__file__).parent / "ocrmypdf_signature.json" + + +def _current_signature() -> list[str]: + import inspect + + import ocrmypdf + + return sorted( + name + for name, p in inspect.signature(ocrmypdf.ocr).parameters.items() + if p.kind is inspect.Parameter.KEYWORD_ONLY + ) + + +def test_ocrmypdf_signature_has_not_drifted(): + """ + Fails when an ocrmypdf upgrade adds or removes a keyword parameter. + + This is the inverse of deriving an allowlist from inspect.signature(). A + derived allowlist grows automatically on `pip upgrade` - a new upstream + option becomes reachable from the internet with no code change and no + review. Here the upgrade breaks the build instead, and someone has to look + at the new parameter and decide whether it belongs in the schema. + """ + expected = json.loads(SNAPSHOT.read_text())["keyword_only_parameters"] + actual = _current_signature() + added, removed = sorted(set(actual) - set(expected)), sorted(set(expected) - set(actual)) + assert not (added or removed), ( + f"ocrmypdf.ocr() signature changed. Added: {added}. Removed: {removed}. " + "Review each new parameter for path/plugin/argv semantics, then update " + "the snapshot deliberately." + ) + + +def test_every_emittable_kwarg_still_exists_upstream(): + """Catches the opposite failure: we emit a kwarg upstream has dropped.""" + upstream = set(_current_signature()) + stale = EMITTABLE_OCRMYPDF_KWARGS - upstream + assert not stale, f"emitting kwargs ocrmypdf no longer accepts: {sorted(stale)}" diff --git a/test/test_ocrservice.py b/test/test_ocrservice.py deleted file mode 100644 index 85fd394..0000000 --- a/test/test_ocrservice.py +++ /dev/null @@ -1,117 +0,0 @@ -import inspect -import logging -import pytest - -import ocrmypdf -from ocrmypdf._options import OcrOptions - -from workflow_ocr_backend.ocrservice import InvalidOcrParameterError, OcrService - -service = OcrService(logging.getLogger(__name__)) - -def test_split_parameters_valid(): - params = service._split_parameters("--skip-text --tesseract-pagesegmode 7 --language eng+chi_sim") - assert params == {"skip_text": True, "tesseract_pagesegmode": 7, "language": ["eng", "chi_sim"]} - -def test_split_parameters_none(): - assert service._split_parameters(None) == {} - -@pytest.mark.parametrize("parameters", [ - "--plugins /tmp/evil.py", - "--plugin-manager foo", - "--user-words /etc/passwd", - "--user-patterns /etc/passwd", - "--keep-temporary-files", - "--sidecar /tmp/out.txt", - "--output-file /tmp/out.pdf", - "--progress-bar", - # tesseract_config is appended verbatim to the tesseract argv, so a caller-supplied - # value is an arbitrary config-file path in the same way user_words is. - "--tesseract-config /tmp/evil.conf", - "--tesseract-config /tmp/a+/tmp/b", -]) -def test_split_parameters_rejects_blocked_parameters(parameters): - # These parameters would allow the caller to execute arbitrary code (plugins), - # access the backend's filesystem or overwrite values controlled by this service. - with pytest.raises(InvalidOcrParameterError): - service._split_parameters(parameters) - -@pytest.mark.parametrize("parameters", [ - "--not-an-ocrmypdf-parameter", - "--some-unknown-option value", -]) -def test_split_parameters_rejects_unknown_parameters(parameters): - with pytest.raises(InvalidOcrParameterError): - service._split_parameters(parameters) - -@pytest.mark.parametrize("parameters", [ - "--language eng;id", - "--language $(id)", - "--language `id`", - "--language |id", - "--language eng+;id", - "--language ../../etc/passwd", - "--language -eng", - "--language 123", - # '$' in a regex also matches before a trailing newline, so this passed while the - # check used re.match instead of re.fullmatch. - "--language eng\n+deu", -]) -def test_split_parameters_rejects_invalid_languages(parameters): - # Language values must match the allow-list pattern used by the Nextcloud app, - # so nothing which could be (ab)used as a shell metacharacter is passed on. - with pytest.raises(InvalidOcrParameterError): - service._split_parameters(parameters) - -@pytest.mark.parametrize("parameters,expected", [ - ("--language eng", "eng"), - ("--language chi_sim", "chi_sim"), - ("--language script/Latin", "script/Latin"), - ("--language eng+deu+script/Latin", ["eng", "deu", "script/Latin"]), -]) -def test_split_parameters_accepts_valid_languages(parameters, expected): - assert service._split_parameters(parameters) == {"language": expected} - -@pytest.mark.parametrize("parameters,expected", [ - # --ocr-engine is a documented CLI flag and a real OcrOptions field, but it is not a - # keyword argument of ocrmypdf.ocr(), so a signature-derived allow-list rejects it. - ("--ocr-engine none", {"ocr_engine": "none"}), - # --jpeg-quality is the documented spelling; --jpg-quality is the hidden alias. - # Both must be accepted, and both must arrive as the keyword ocrmypdf.ocr() takes. - ("--jpeg-quality 80", {"jpg_quality": 80}), - ("--jpg-quality 80", {"jpg_quality": 80}), -]) -def test_split_parameters_accepts_documented_cli_names(parameters, expected): - assert service._split_parameters(parameters) == expected - -@pytest.mark.parametrize("parameters,expected", [ - ("--quiet", {}), - ("--verbose", {}), - ("--quiet --language eng", {"language": "eng"}), -]) -def test_split_parameters_drops_cli_only_flags(parameters, expected): - # CLI-only logging flags have no OCRmyPDF API equivalent. They are dropped rather than - # rejected so that existing workflow configurations carrying them keep working. - assert service._split_parameters(parameters) == expected - -def test_allowed_parameters_still_resolve_against_installed_ocrmypdf(): - # The allow-list is an explicit literal, so an OCRmyPDF upgrade cannot silently widen - # it. This guard catches the opposite risk: an upgrade renaming or removing an option - # would otherwise leave a dead entry that 400s at runtime with no test failure. - ocr_keywords = { - name for name, param in inspect.signature(ocrmypdf.ocr).parameters.items() - if param.kind is inspect.Parameter.KEYWORD_ONLY - } - option_fields = set(OcrOptions.model_fields.keys()) - unresolved = sorted( - name for name in OcrService.ALLOWED_PARAMETERS - if OcrService.PARAMETER_ALIASES.get(name, name) not in ocr_keywords | option_fields - ) - assert not unresolved, ( - f"Allow-listed parameters no longer accepted by ocrmypdf {ocrmypdf.__version__}: " - f"{unresolved}. Check whether they were renamed or removed." - ) - -def test_blocked_and_allowed_parameters_are_disjoint(): - assert not (OcrService.ALLOWED_PARAMETERS & OcrService.BLOCKED_PARAMETERS) - assert not (OcrService.ALLOWED_PARAMETERS & OcrService.IGNORED_PARAMETERS) diff --git a/workflow_ocr_backend/app.py b/workflow_ocr_backend/app.py index c410a3f..ebc47df 100644 --- a/workflow_ocr_backend/app.py +++ b/workflow_ocr_backend/app.py @@ -1,7 +1,9 @@ from contextlib import asynccontextmanager +import os +import uuid from typing import Iterable -from fastapi import FastAPI, File, Form, UploadFile, Request +from fastapi import FastAPI, File, Form, UploadFile, Request, Response from fastapi.responses import JSONResponse from nc_py_api import AsyncNextcloudApp, NextcloudApp @@ -9,19 +11,49 @@ import logging from ocrmypdf import ExitCodeException +from pydantic import ValidationError from .model.ocrresult import ErrorResult, OcrResult -from .ocrservice import InvalidOcrParameterError, OcrService +from .ocroptions import InvalidOcrOptionsError, OcrOptions, OcrPolicy +from .ocrservice import OcrService + +logger = logging.getLogger('uvicorn.error') # Use same logging as uvicorn + + +def _policy_from_env(installed_languages: frozenset[str]) -> OcrPolicy: + """Builds the operator-owned resource policy from the process environment. + + These are deliberately not request fields (see ocroptions.py): raising them + re-enables the decompression-bomb guard being disabled, or lets one request + pin the whole backend, which only takes one OCRmyPDF task at a time. + """ + kwargs = {} + if (v := os.getenv("OCR_JOBS")) is not None: + kwargs["jobs"] = int(v) + if (v := os.getenv("OCR_MAX_IMAGE_MPIXELS")) is not None: + kwargs["max_image_mpixels"] = float(v) + if (v := os.getenv("OCR_MAX_TESSERACT_TIMEOUT_S")) is not None: + kwargs["max_tesseract_timeout_s"] = float(v) + return OcrPolicy(installed_languages=installed_languages, **kwargs) + @asynccontextmanager async def lifespan(app: FastAPI): set_handlers(app, enabled_handler) + # Installed languages only change with the container image, so this is + # read once at startup rather than shelling out to tesseract per request. + app.state.ocr_policy = _policy_from_env( + frozenset(OcrService(logger).installed_languages()) + ) yield APP = FastAPI(lifespan=lifespan) APP.add_middleware(AppAPIAuthMiddleware, disable_for=["docs", "openapi.json"]) -logger = logging.getLogger('uvicorn.error') # Use same logging as uvicorn + + +def get_policy() -> OcrPolicy: + return APP.state.ocr_policy async def enabled_handler(enabled: bool, _: AsyncNextcloudApp) -> str: @@ -33,29 +65,85 @@ async def enabled_handler(enabled: bool, _: AsyncNextcloudApp) -> str: async def exit_code_exception_handler(_: Request, exc: ExitCodeException): return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})", "ocrMyPdfExitCode": exc.exit_code}, status_code=500) -@APP.exception_handler(InvalidOcrParameterError) -async def invalid_ocr_parameter_exception_handler(_: Request, exc: InvalidOcrParameterError): - # The caller sent an OCR parameter which is not allowed -> client error. +@APP.exception_handler(InvalidOcrOptionsError) +async def invalid_ocr_options_exception_handler(_: Request, exc: InvalidOcrOptionsError): + # The caller sent an OCR option which is not allowed or not valid right now + # (e.g. an uninstalled language) -> client error, not a server error. return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})"}, status_code=400) +@APP.exception_handler(ValidationError) +async def validation_error_exception_handler(_: Request, exc: ValidationError): + # Schema validation of the "options" body -> 422 with field-level detail, + # matching FastAPI's own convention for an invalid request body. + return JSONResponse({"message": "Invalid OCR options", "errors": exc.errors(include_url=False, include_context=False)}, status_code=422) + @APP.exception_handler(Exception) async def exception_handler(_: Request, exc: Exception): - # Exception will be logged by uvicorn automatically. - # It will also be turned into an ErrorResult response. - return JSONResponse({"message": f"{str(exc)} ({exc.__class__.__name__})"}, status_code=500) + # Never echo str(exc) here: exception text routinely carries absolute temp + # paths and library internals. Log the detail server-side against a + # correlation id and return only that id to the caller. + correlation_id = str(uuid.uuid4()) + logger.exception(f"Unhandled error [{correlation_id}]") + return JSONResponse({"message": f"Internal server error [{correlation_id}]"}, status_code=500) -@APP.post("/process_ocr", response_model=OcrResult, responses={400: {"model": ErrorResult}, 500: {"model": ErrorResult}}) -async def process_ocr( - file: UploadFile = File(..., description="The file to be processed using OCR."), - ocrmypdf_parameters: str = Form(None, description="Additional parameters for the OCRmyPdf process (see https://ocrmypdf.readthedocs.io/en/latest/cookbook.html#basic-examples).") +@APP.post( + "/v1/ocr", + response_model=OcrResult, + responses={400: {"model": ErrorResult}, 422: {"model": ErrorResult}, 500: {"model": ErrorResult}}, +) +def ocr_v1( + response: Response, + file: UploadFile = File(..., description="The file to be processed using OCR."), + options: str = Form( + "{}", + description="OCR options as a JSON object validated against the OcrOptions schema " + "(see /docs). Unknown fields are rejected, not forwarded.", + ), ): """ - Processes an OCR request. - This endpoint accepts a file upload and optional OCR parameters to process the file using OCR (Optical Character Recognition). + Processes an OCR request using the typed ``OcrOptions`` schema. + + This endpoint's contract is the schema, not OCRmyPDF's own parameter set: + every option is bounded, enumerated, or pattern-constrained, and resource + limits (jobs, image size, timeout ceiling) are operator policy, never part + of the request body. + """ + parsed_options = OcrOptions.model_validate_json(options) + # Declared synchronous on purpose: ocrmypdf.ocr() is a long-running, CPU-bound + # blocking call. FastAPI runs a sync endpoint in the threadpool instead of on + # the event loop, so one OCR job no longer stalls every other request + # (including AppAPI's own heartbeat poll) for its whole duration. + service = OcrService(logger) + response.headers["Cache-Control"] = "no-store" + return service.ocr(file.file, file.filename, parsed_options, get_policy()) + + +@APP.post( + "/process_ocr", + response_model=OcrResult, + responses={400: {"model": ErrorResult}, 500: {"model": ErrorResult}}, + deprecated=True, +) +def process_ocr( + response: Response, + file: UploadFile = File(..., description="The file to be processed using OCR."), + ocrmypdf_parameters: str = Form(None, description="Additional parameters for the OCRmyPdf process (see https://ocrmypdf.readthedocs.io/en/latest/cookbook.html#basic-examples)."), + ): + """ + Deprecated. Processes an OCR request using the legacy ``--flag value`` string. + + Kept as a thin, translating shim: the string is parsed and mapped onto the + same ``OcrOptions`` schema ``/v1/ocr`` uses, so it inherits every validation + rule from that schema. Anything the shim cannot express (``--plugins``, + ``--tesseract-config``, ``--unpaper-args``, ...) is a 400, not a silent + passthrough. Use ``/v1/ocr`` for new integrations. """ service = OcrService(logger) - return service.ocr(file.file, file.filename, ocrmypdf_parameters) + response.headers["Deprecation"] = "true" + response.headers["Sunset"] = "Wed, 01 Jul 2026 00:00:00 GMT" + response.headers["Link"] = '; rel="successor-version"' + return service.ocr_legacy(file.file, file.filename, ocrmypdf_parameters, get_policy()) @APP.get("/installed_languages", response_model=Iterable[str]) def installed_languages(): @@ -63,4 +151,4 @@ def installed_languages(): Retrieves the list of installed Tesseract languages - relevant for OCRmyPDF. """ service = OcrService(logger) - return service.installed_languages() \ No newline at end of file + return service.installed_languages() diff --git a/workflow_ocr_backend/legacy.py b/workflow_ocr_backend/legacy.py new file mode 100644 index 0000000..d4822af --- /dev/null +++ b/workflow_ocr_backend/legacy.py @@ -0,0 +1,159 @@ +""" +Deprecated ``--flag value`` request format, translated into :class:`OcrOptions`. + +This is the shim promised by the redesign: ``/process_ocr`` keeps accepting the +old ``ocrmypdf_parameters`` string, but it no longer builds a kwargs dict that +gets splatted into ``ocrmypdf.ocr()``. It tokenises the string, translates each +recognised flag into a field on :class:`workflow_ocr_backend.ocroptions.OcrOptions` +through an explicit table, and lets that schema's own validation - not a +denylist - decide what is accepted. Anything the table does not recognise +(``--plugins``, ``--tesseract-config``, ``--unpaper-args``, ...) is a 400 by +construction, because there is no line in the table that would ever place it on +the model. +""" + +from __future__ import annotations + +import shlex +from typing import Any, Final + +from pydantic import ValidationError + +from .ocroptions import NEVER_EMITTED, InvalidOcrOptionsError, OcrOptions, TextMode + + +class InvalidOcrParameterError(InvalidOcrOptionsError): + """Raised when the caller sent a legacy OCR parameter which is not allowed.""" + + +# Boolean "presence" flags that select OcrOptions.mode instead of a same-named +# field. Mutually exclusive by construction - the last one seen wins, matching +# the OcrOptions default of TextMode.SKIP when none are present. +_MODE_FLAGS: Final[dict[str, TextMode]] = { + "skip_text": TextMode.SKIP, + "force_ocr": TextMode.FORCE, + "redo_ocr": TextMode.REDO, +} + +# Legacy CLI option name (normalised: '-' -> '_') -> OcrOptions field name. +# Deliberately a literal, hand-maintained table: the set of names this shim can +# translate IS the allow-list. A name that isn't here can never reach OcrOptions, +# so it can never reach ocrmypdf, regardless of what future versions accept. +_FIELD_MAP: Final[dict[str, str]] = { + "pages": "pages", + "rotate_pages": "rotate_pages", + "rotate_pages_threshold": "rotate_pages_threshold", + "deskew": "deskew", + "clean": "clean", + "clean_final": "clean_final", + "remove_background": "remove_background", + "remove_vectors": "remove_vectors", + "oversample": "oversample_dpi", + "image_dpi": "image_dpi", + "output_type": "output_type", + "optimize": "optimize", + "jpeg_quality": "jpeg_quality", + "jpg_quality": "jpeg_quality", # hidden ocrmypdf CLI alias, same target field + "png_quality": "png_quality", + "pdf_renderer": "pdf_renderer", + "tesseract_pagesegmode": "tesseract_pagesegmode", + "tesseract_oem": "tesseract_oem", + "tesseract_thresholding": "tesseract_thresholding", + "tesseract_timeout": "tesseract_timeout_s", + "title": "title", + "author": "author", + "subject": "subject", + "keywords": "keywords", +} + +# CLI-only flags with no OCRmyPDF API equivalent. Accepted and dropped rather +# than rejected, so existing workflow configurations carrying them keep working. +_IGNORED_PARAMETERS: Final[frozenset[str]] = frozenset({"quiet", "verbose", "no_progress_bar"}) + + +def _tokenize(ocrmypdf_parameters: str) -> dict[str, str | bool]: + """ + Splits a ``--flag value --other-flag`` string into a raw {name: value} dict. + + Uses ``shlex.split`` rather than ``str.split("--")`` / ``str.split(" ")``, so + a quoted value ("--title 'Hello World'") survives intact instead of being + silently truncated to its first token, and a value containing "--" is not + misread as a new flag. + """ + try: + tokens = shlex.split(ocrmypdf_parameters) + except ValueError as exc: # e.g. unbalanced quotes + raise InvalidOcrParameterError(f"Could not parse OCR parameters: {exc}") from exc + + raw: dict[str, str | bool] = {} + i = 0 + while i < len(tokens): + token = tokens[i] + if not token.startswith("--"): + raise InvalidOcrParameterError(f"Expected a '--flag', got {token!r}") + key = token[2:].strip().replace("-", "_") + if not key: + raise InvalidOcrParameterError("Empty parameter name") + if key in raw: + raise InvalidOcrParameterError(f"Duplicate parameter {key!r}") + + if i + 1 < len(tokens) and not tokens[i + 1].startswith("--"): + raw[key] = tokens[i + 1] + i += 2 + else: + raw[key] = True + i += 1 + return raw + + +def options_from_legacy_parameters(ocrmypdf_parameters: str | None) -> OcrOptions: + """Translates the deprecated flag-string format into a validated OcrOptions.""" + if not ocrmypdf_parameters: + return OcrOptions() + + raw = _tokenize(ocrmypdf_parameters) + fields: dict[str, Any] = {} + mode: TextMode | None = None + + for key, value in raw.items(): + if key in _IGNORED_PARAMETERS: + continue + + if key == "language": + languages = value.split("+") if isinstance(value, str) else [value] + fields["languages"] = languages + continue + + if key in _MODE_FLAGS: + if value is True: + mode = _MODE_FLAGS[key] + continue + + if key not in _FIELD_MAP: + if key in NEVER_EMITTED: + # Known-dangerous parameter: name it explicitly, matching the + # legacy API's own wording for this case. + raise InvalidOcrParameterError(f"Parameter '{key}' is not allowed") + raise InvalidOcrParameterError(f"Unknown parameter '{key}'") + + fields[_FIELD_MAP[key]] = value + + if mode is not None: + fields["mode"] = mode + + try: + return OcrOptions(**fields) + except ValidationError as exc: + raise InvalidOcrParameterError(_first_validation_message(exc)) from exc + + +def _first_validation_message(exc: ValidationError) -> str: + """Renders a pydantic ValidationError as the single-line message the legacy + API contract expects, e.g. "Invalid language value '$(id)'" for a bad + language rather than pydantic's generic pattern-mismatch wording.""" + error = exc.errors()[0] + field = ".".join(str(loc) for loc in error["loc"]) + if field.startswith("languages"): + bad = error.get("input") + return f"Invalid language value {bad!r}" + return f"Invalid value for '{field}': {error['msg']}" diff --git a/workflow_ocr_backend/ocroptions.py b/workflow_ocr_backend/ocroptions.py new file mode 100644 index 0000000..44fc0ae --- /dev/null +++ b/workflow_ocr_backend/ocroptions.py @@ -0,0 +1,325 @@ +""" +Typed, closed vocabulary of OCR options exposed over the REST API. + +Design rules enforced here (see also test/test_ocroptions.py, which enforces them +mechanically so they cannot rot): + +1. The API contract is THIS schema, not ``ocrmypdf.ocr()``'s signature. + Nothing is derived by reflection from the dependency. +2. ``extra="forbid"`` - unknown fields are a 422, never a silent passthrough. +3. No field is path-typed, plugin-typed, or free-form argv. Structurally + impossible to express "load this file" or "append this to a subprocess + command line" in this schema. +4. Every scalar is bounded. Caller intent and operator policy are separate: + resource limits (jobs, timeouts, image size caps) are NOT caller options. +5. Mapping to ocrmypdf kwargs is written out by hand, field by field. There is + no ``**caller_data`` splat anywhere in this module. +""" + +from __future__ import annotations + +from enum import Enum +from typing import Annotated, Any, Final + +from pydantic import BaseModel, ConfigDict, Field, StringConstraints, model_validator + + +class InvalidOcrOptionsError(ValueError): + """Raised for a caller-supplied option that is well-typed but invalid at + runtime (e.g. a language not installed on this backend) - a 400, not a 422.""" + + +# -------------------------------------------------------------------------- +# Server-side policy. Operator-controlled, never caller-controlled. +# -------------------------------------------------------------------------- + + +class OcrPolicy(BaseModel): + """Limits owned by whoever runs the backend, injected from config/env.""" + + model_config = ConfigDict(frozen=True) + + jobs: int = Field(default=1, ge=1, le=16) + max_image_mpixels: float = Field(default=250.0, gt=0, le=1000.0) + # Hard ceiling. A caller-supplied tesseract_timeout is clamped to this. + max_tesseract_timeout_s: float = Field(default=180.0, gt=0, le=3600.0) + installed_languages: frozenset[str] = frozenset({"eng"}) + + +# -------------------------------------------------------------------------- +# Enumerations. Replace stringly-typed ocrmypdf options with closed sets. +# -------------------------------------------------------------------------- + + +class TextMode(str, Enum): + """Replaces the mutually exclusive skip_text/force_ocr/redo_ocr booleans. + + The legacy API let a caller set two of them at once and get a 500 out of + ocrmypdf's own validation. As an enum the invalid state is unrepresentable. + """ + + SKIP = "skip-text" + FORCE = "force-ocr" + REDO = "redo-ocr" + + +class OutputType(str, Enum): + PDFA = "pdfa" + PDF = "pdf" + PDFA_1 = "pdfa-1" + PDFA_2 = "pdfa-2" + PDFA_3 = "pdfa-3" + + +class PdfRenderer(str, Enum): + AUTO = "auto" + HOCR = "hocr" + SANDWICH = "sandwich" + + +class Thresholding(str, Enum): + OTSU = "otsu" + ADAPTIVE_OTSU = "adaptive-otsu" + SAUVOLA = "sauvola" + + +# -------------------------------------------------------------------------- +# Constrained scalars. The constraint travels with the type. +# -------------------------------------------------------------------------- + +# Tesseract language / script code, e.g. "eng", "chi_sim", "script/Latin". +LanguageCode = Annotated[ + str, + StringConstraints(pattern=r"^[A-Za-z][A-Za-z0-9_]{0,31}$|^script/[A-Za-z_]{1,31}$"), +] + +# Page selection, e.g. "1-4,7,9-". Deliberately narrow: digits, comma, hyphen. +PageRange = Annotated[ + str, + StringConstraints(pattern=r"^[0-9]+(-[0-9]*)?(,[0-9]+(-[0-9]*)?)*$", max_length=128), +] + +MetadataText = Annotated[str, StringConstraints(max_length=512)] + + +# -------------------------------------------------------------------------- +# The request schema. +# -------------------------------------------------------------------------- + + +class OcrOptions(BaseModel): + """Everything a caller is allowed to say about how to OCR a document.""" + + model_config = ConfigDict( + extra="forbid", # unknown key -> 422, never forwarded + frozen=True, + use_enum_values=False, + str_strip_whitespace=True, + ) + + # --- recognition ------------------------------------------------------- + languages: list[LanguageCode] = Field(default_factory=lambda: ["eng"], min_length=1, max_length=8) + # None is a fourth, deliberate state distinct from the three enum values: it + # means "don't override OCRmyPDF's own disposition", which is to raise if the + # document already has text. Defaulting to None rather than TextMode.SKIP + # keeps that conservative default - a caller has to opt into skipping, + # forcing, or redoing OCR on already-processed pages. + mode: TextMode | None = None + pages: PageRange | None = None + + # --- preprocessing ----------------------------------------------------- + rotate_pages: bool = False + rotate_pages_threshold: float | None = Field(default=None, ge=0.0, le=30.0) + deskew: bool = False + clean: bool = False + clean_final: bool = False + remove_background: bool = False + remove_vectors: bool = False + oversample_dpi: int | None = Field(default=None, ge=0, le=1200) + image_dpi: int | None = Field(default=None, ge=1, le=5000) + + # --- output ------------------------------------------------------------ + output_type: OutputType = OutputType.PDFA + optimize: int = Field(default=1, ge=0, le=3) + jpeg_quality: int | None = Field(default=None, ge=0, le=100) + png_quality: int | None = Field(default=None, ge=0, le=100) + pdf_renderer: PdfRenderer = PdfRenderer.AUTO + + # --- tesseract tuning -------------------------------------------------- + tesseract_pagesegmode: int | None = Field(default=None, ge=0, le=13) + tesseract_oem: int | None = Field(default=None, ge=0, le=3) + tesseract_thresholding: Thresholding | None = None + # Requested, not guaranteed: clamped to policy ceiling at map time. + tesseract_timeout_s: float | None = Field(default=None, gt=0, le=3600.0) + + # --- document metadata ------------------------------------------------- + title: MetadataText | None = None + author: MetadataText | None = None + subject: MetadataText | None = None + keywords: MetadataText | None = None + + @model_validator(mode="after") + def _check_coherent(self) -> OcrOptions: + if self.clean_final and not self.clean: + raise ValueError("clean_final requires clean") + if self.rotate_pages_threshold is not None and not self.rotate_pages: + raise ValueError("rotate_pages_threshold requires rotate_pages") + if len(set(self.languages)) != len(self.languages): + raise ValueError("languages must not contain duplicates") + return self + + def validate_against_policy(self, policy: OcrPolicy) -> None: + """Checks that depend on runtime state, so they can return 400 not 500.""" + unknown = [lang for lang in self.languages if lang not in policy.installed_languages] + if unknown: + raise InvalidOcrOptionsError( + f"Language(s) not installed on this backend: {', '.join(sorted(unknown))}" + ) + + # ---------------------------------------------------------------------- + # Explicit mapping. Hand-written on purpose: adding an option to the API + # is a deliberate edit here, not a side effect of upgrading a dependency. + # ---------------------------------------------------------------------- + + def to_ocrmypdf_kwargs(self, policy: OcrPolicy) -> dict[str, Any]: + kwargs: dict[str, Any] = { + # Operator policy - never caller-controlled. + "jobs": policy.jobs, + "max_image_mpixels": policy.max_image_mpixels, + "progress_bar": False, + # Caller intent. + "language": list(self.languages), + "output_type": self.output_type.value, + "optimize": self.optimize, + "pdf_renderer": self.pdf_renderer.value, + "rotate_pages": self.rotate_pages, + "deskew": self.deskew, + "clean": self.clean, + "clean_final": self.clean_final, + "remove_background": self.remove_background, + "remove_vectors": self.remove_vectors, + } + + # Mode: at most one enum in, at most one legacy boolean out. None means + # "no override" - all three stay unset, and ocrmypdf applies its own + # (conservative) default disposition. + if self.mode is not None: + kwargs[ + { + TextMode.SKIP: "skip_text", + TextMode.FORCE: "force_ocr", + TextMode.REDO: "redo_ocr", + }[self.mode] + ] = True + + optional: list[tuple[str, Any]] = [ + ("pages", self.pages), + ("rotate_pages_threshold", self.rotate_pages_threshold), + ("oversample", self.oversample_dpi), + ("image_dpi", self.image_dpi), + # ocrmypdf.ocr()'s keyword is "jpg_quality" - "--jpeg-quality" is only the + # documented *CLI* spelling; the Python signature never exposed it. Keep the + # public field named after the CLI flag callers actually know, and translate + # here so the public contract does not depend on which alias ocrmypdf's + # argument parser happens to forward to the API. + ("jpg_quality", self.jpeg_quality), + ("png_quality", self.png_quality), + ("tesseract_pagesegmode", self.tesseract_pagesegmode), + ("tesseract_oem", self.tesseract_oem), + ("title", self.title), + ("author", self.author), + ("subject", self.subject), + ("keywords", self.keywords), + ] + for key, value in optional: + if value is not None: + kwargs[key] = value + + if self.tesseract_thresholding is not None: + # ocrmypdf takes an IntEnum here. The API keeps a readable string so + # the public contract survives upstream changing its encoding - which + # is the whole point of not exposing the library's own types. + kwargs["tesseract_thresholding"] = { + Thresholding.OTSU: 0, + Thresholding.ADAPTIVE_OTSU: 1, + Thresholding.SAUVOLA: 2, + }[self.tesseract_thresholding] + + # Clamp rather than reject: the caller asked for a timeout, the operator + # decides the maximum. A caller can never extend it. + kwargs["tesseract_timeout"] = min( + self.tesseract_timeout_s or policy.max_tesseract_timeout_s, + policy.max_tesseract_timeout_s, + ) + + return kwargs + + +# -------------------------------------------------------------------------- +# The literal, frozen boundary. Asserted by tests. +# -------------------------------------------------------------------------- + +#: Every ocrmypdf kwarg this service is capable of producing. Written out as a +#: literal so it appears in code review whenever it changes. +EMITTABLE_OCRMYPDF_KWARGS: Final[frozenset[str]] = frozenset( + { + "author", + "clean", + "clean_final", + "deskew", + "force_ocr", + "image_dpi", + "jobs", + "jpg_quality", + "keywords", + "language", + "max_image_mpixels", + "optimize", + "output_type", + "oversample", + "pages", + "pdf_renderer", + "png_quality", + "progress_bar", + "redo_ocr", + "remove_background", + "remove_vectors", + "rotate_pages", + "rotate_pages_threshold", + "skip_text", + "subject", + "tesseract_oem", + "tesseract_pagesegmode", + "tesseract_thresholding", + "tesseract_timeout", + "title", + } +) + +#: Parameters this service must never emit at all, with the reason. This is a +#: tripwire for review, not the security mechanism - the mechanism is that the +#: schema has no field capable of expressing any of them. +NEVER_EMITTED: Final[dict[str, str]] = { + "plugins": "loads and executes arbitrary Python -> RCE", + "plugin_manager": "same as plugins", + "user_words": "caller-supplied path appended to the tesseract argv", + "user_patterns": "caller-supplied path appended to the tesseract argv", + "tesseract_config": "caller-supplied list extended directly into the tesseract argv", + "unpaper_args": "caller-supplied argv for the unpaper subprocess", + "keep_temporary_files": "leaves caller data on the backend filesystem", + "invalidate_digital_signatures": "changes document trust semantics", + "input_file": "owned by this service", + "input_file_or_options": "owned by this service", + "output_file": "owned by this service", + "output_folder": "owned by this service", + "sidecar": "owned by this service", + "no_overwrite": "owned by this service", +} + +#: Emitted, but sourced from OcrPolicy only. No request field may influence them. +#: Raising max_image_mpixels re-enables decompression bombs; raising jobs and +#: tesseract_timeout are the cheapest DoS vectors against a backend that can only +#: run one OCRmyPDF task per process. +OPERATOR_OWNED: Final[frozenset[str]] = frozenset( + {"jobs", "max_image_mpixels", "progress_bar", "tesseract_timeout"} +) diff --git a/workflow_ocr_backend/ocrservice.py b/workflow_ocr_backend/ocrservice.py index 41fc08b..2ec432d 100644 --- a/workflow_ocr_backend/ocrservice.py +++ b/workflow_ocr_backend/ocrservice.py @@ -1,105 +1,47 @@ - import base64 from datetime import datetime, timezone import io from logging import Logger -import re from typing import BinaryIO, Iterable import ocrmypdf +from .legacy import options_from_legacy_parameters from .model.ocrresult import OcrResult +from .ocroptions import OcrOptions, OcrPolicy import subprocess -class InvalidOcrParameterError(ValueError): - """Raised when the caller sent an OCRmyPDF parameter which is not allowed.""" class OcrService: - # Allow-list for tesseract/OCRmyPDF language codes (e.g. 'eng', 'chi_sim', 'script/Latin'). - # Same pattern as the one used by the Nextcloud app (workflow_ocr) so that language values - # which could be (ab)used as shell metacharacters never reach the OCR engine. - LANGUAGE_CODE_REGEX = re.compile(r"^[A-Za-z][A-Za-z0-9_/]{0,31}$") - - # Parameters which must never be taken from a request, even though OCRmyPDF accepts them: - # * plugins/plugin_manager load arbitrary Python code => remote code execution - # * input/output/sidecar/progress_bar are controlled by this service - # * user_words/user_patterns/keep_temporary_files give access to the backend's filesystem - # * tesseract_config is appended verbatim to the tesseract argv (see _exec/tesseract.py), - # so a caller-supplied value is an arbitrary config-file path just like user_words - BLOCKED_PARAMETERS = frozenset({ - "plugins", - "plugin_manager", - "input_file", - "input_file_or_options", - "output_file", - "output_folder", - "sidecar", - "progress_bar", - "user_words", - "user_patterns", - "keep_temporary_files", - "tesseract_config", - }) - - # Allow-list of OCRmyPDF *CLI option* names (normalised: '-' replaced by '_'), because that - # is what callers send. Deliberately an explicit literal instead of introspecting - # ocrmypdf.ocr(): its Python keyword names differ from the documented CLI spellings - # (e.g. --jpeg-quality vs jpg_quality, --ocr-engine is not a keyword argument at all), - # and introspection would silently widen this set on every OCRmyPDF upgrade. - # test_ocrservice.py asserts every entry still resolves against the installed OCRmyPDF. - ALLOWED_PARAMETERS = frozenset({ - # Language and OCR engine selection - "language", "ocr_engine", "mode", "force_ocr", "skip_text", "redo_ocr", - "pages", "skip_big", - # Image preprocessing - "image_dpi", "oversample", "deskew", "clean", "clean_final", "unpaper_args", - "remove_background", "remove_vectors", "rotate_pages", "rotate_pages_threshold", - # Tesseract tuning - "tesseract_oem", "tesseract_pagesegmode", "tesseract_thresholding", - "tesseract_timeout", "tesseract_non_ocr_timeout", - "tesseract_downsample_above", "tesseract_downsample_large_images", - # Output and PDF generation - "output_type", "pdf_renderer", "rasterizer", "pdfa_image_compression", - "color_conversion_strategy", "tagged_pdf_mode", "fast_web_view", "no_overwrite", - "invalidate_digital_signatures", "continue_on_soft_render_error", - # Optimisation - "optimize", "jpeg_quality", "jpg_quality", "png_quality", - "jbig2_lossy", "jbig2_page_group_size", "jbig2_threshold", - # Document metadata - "title", "author", "subject", "keywords", - # Resource usage - "jobs", "use_threads", "max_image_mpixels", - }) - - # Documented CLI option name -> ocrmypdf.ocr() keyword argument, where the two differ. - # --jpeg-quality is the documented flag; --jpg-quality is its hidden (argparse.SUPPRESS) - # alias and the only spelling the Python signature exposes. - PARAMETER_ALIASES = { - "jpeg_quality": "jpg_quality", - } - - # CLI-only flags with no OCRmyPDF API equivalent. Accepted and dropped rather than - # rejected, so existing workflow configurations carrying them keep working. - IGNORED_PARAMETERS = frozenset({"quiet", "verbose", "no_progress_bar"}) - - LANGUAGE_PARAMETERS = frozenset({"language"}) - def __init__(self, logger: Logger): self.logger = logger - def ocr(self, file: BinaryIO, file_name: str, ocrmypdf_parameters: str) -> OcrResult: - output_buffer = io.BytesIO() + def ocr(self, file: BinaryIO, file_name: str, options: OcrOptions, policy: OcrPolicy) -> OcrResult: + """ + Runs OCRmyPDF for a validated, typed set of options. + + ``options`` is the only caller-controlled input handed to ocrmypdf. It was + produced either by validating a JSON request body against ``OcrOptions`` + (the ``/v1/ocr`` endpoint) or by translating the deprecated flag-string + format through ``legacy.options_from_legacy_parameters`` (the + ``/process_ocr`` shim) - either way it already passed the schema's own + (stateless) validation. The one check that depends on runtime state - + whether the requested languages are installed - happens here, against + ``policy``, so it can return 400 rather than surface as an OCRmyPDF 500. + """ + output_buffer = io.BytesIO() sidecar_buffer = io.BytesIO() - + try: current_time = datetime.now(timezone.utc).isoformat() - self.logger.debug(f"{current_time} - Start processing file {file_name} (OCR parameters: {ocrmypdf_parameters})") + self.logger.debug(f"{current_time} - Start processing file {file_name} (OCR options: {options!r})") - kwargs = self._split_parameters(ocrmypdf_parameters) - exit_code = ocrmypdf.ocr(file, output_buffer, sidecar=sidecar_buffer, progress_bar=False, **kwargs) + options.validate_against_policy(policy) + kwargs = options.to_ocrmypdf_kwargs(policy) + exit_code = ocrmypdf.ocr(file, output_buffer, sidecar=sidecar_buffer, **kwargs) if exit_code != 0: raise Exception(f"ocr failed ({exit_code})") - + file_base64 = base64.b64encode(output_buffer.getvalue()).decode("utf-8") output_buffer.close() @@ -110,77 +52,21 @@ def ocr(self, file: BinaryIO, file_name: str, ocrmypdf_parameters: str) -> OcrRe self.logger.debug(f"{current_time} - Finished processing file {file_name}") return OcrResult(filename=file_name, content_type="application/pdf", recognized_text=sidecar_text, file_content=file_base64) - + finally: output_buffer.close() sidecar_buffer.close() + def ocr_legacy(self, file: BinaryIO, file_name: str, ocrmypdf_parameters: str | None, policy: OcrPolicy) -> OcrResult: + """ + Deprecated entry point for ``/process_ocr``. Translates the legacy + ``--flag value`` string into ``OcrOptions`` (raising ``InvalidOcrParameterError`` + - a 400 - for anything the shim can't express) and delegates to ``ocr()``. + """ + options = options_from_legacy_parameters(ocrmypdf_parameters) + return self.ocr(file, file_name, options, policy) + def installed_languages(self) -> Iterable[str]: result = subprocess.run(["tesseract", "--list-langs"], capture_output=True, text=True) languages = result.stdout.splitlines()[1:] # Skip the first line return [lang for lang in languages if lang != "osd"] - - def _split_parameters(self, ocrmypdf_parameters: str) -> dict[str, str | bool | Iterable[str] | int | float]: - if ocrmypdf_parameters is None: - return {} - - params = {} - - for param in [p.strip() for p in ocrmypdf_parameters.split("--")]: - if not param: - continue - splitted_param = [p.strip() for p in param.split(" ")] - key = splitted_param[0].replace("-", "_") - length = len(splitted_param) - if length >= 2: - value = splitted_param[1] - # Multiple values - if "+" in value: - value = value.split("+") - # Single value (might be of type str, bool, int or float) - elif value.isnumeric(): - value = int(value) - elif value.replace(".", "", 1).isnumeric(): - value = float(value) - elif value.lower() == "true": - value = True - elif value.lower() == "false": - value = False - else: - # Flag - value = True - - if key in self.IGNORED_PARAMETERS: - self.logger.debug("Ignoring CLI-only OCR parameter %r", key) - continue - - self._check_parameter(key, value) - - params[self.PARAMETER_ALIASES.get(key, key)] = value - return params - - def _check_parameter(self, key: str, value: str | bool | Iterable[str] | int | float) -> None: - """ - Validates a single OCRmyPDF parameter before it's handed over to ocrmypdf.ocr(). - This is a security relevant check: the parameters are fully controlled by the caller - and are used to invoke the OCR engine (which in turn spawns subprocesses), so only - known-good parameters and language codes may pass. - """ - # Note: %r rather than an f-string, so control characters in the caller-supplied - # key are escaped instead of forging additional log lines. - if key in self.BLOCKED_PARAMETERS: - self.logger.warning("Rejected blocked OCR parameter %r", key) - raise InvalidOcrParameterError(f"Parameter '{key}' is not allowed") - - if key not in self.ALLOWED_PARAMETERS: - self.logger.warning("Rejected unknown OCR parameter %r", key) - raise InvalidOcrParameterError(f"Unknown parameter '{key}'") - - if key in self.LANGUAGE_PARAMETERS: - languages = value if isinstance(value, list) else [value] - for language in languages: - # fullmatch, not match: '$' would also match before a trailing newline, - # so re.match would accept 'eng\n' (reachable via '--language eng\n+deu'). - if not isinstance(language, str) or not self.LANGUAGE_CODE_REGEX.fullmatch(language): - self.logger.warning("Rejected invalid OCR language value: %r", language) - raise InvalidOcrParameterError(f"Invalid language value '{language}'") From d534f0c4076123f990e5d2b5d1a939e47e84dd21 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:07:08 +0000 Subject: [PATCH 5/6] fix: test fixture for uninstalled language used a real tesseract pack The Dockerfile installs every tesseract-ocr-data language package (see doc/CODE_REVIEW.md BP-7), so "jpn" is actually installed in the CI/prod image and test_ocr_v1_rejects_uninstalled_language got a 200 instead of the expected 400. Use a syntactically valid but non-existent language code instead of relying on a specific real code being absent. --- test/test_app.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/test_app.py b/test/test_app.py index a24bcd6..5631fac 100644 --- a/test/test_app.py +++ b/test/test_app.py @@ -169,7 +169,10 @@ def test_ocr_v1_rejects_uninstalled_language(): response = client.post( "/v1/ocr", files={"file": (file_name, file, "application/pdf")}, - data={"options": '{"languages": ["jpn"]}'} + # Not a real tesseract language pack, and the CI/production image installs + # every language ocrmypdf-data ships (see doc/CODE_REVIEW.md BP-7), so a + # real-but-uninstalled code like "jpn" isn't a safe fixture here. + data={"options": '{"languages": ["zzzzzz"]}'} ) assert response.status_code == 400 assert "not installed" in response.json()["message"] From 6de5b315c735d15408bbc80c699a7c8f71c0a3b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:36:01 +0000 Subject: [PATCH 6/6] address PR #14 review feedback; drop design/review docs; harden error logging Copilot review (pull#14 review 4987030551): - /v1/ocr's 422 response advertised ErrorResult but actually returned {message, errors}; added a ValidationErrorResult model and used it for both the response docs and the handler's actual payload. - The /process_ocr Sunset header was already in the past. Moved it out to a real future date. - _policy_from_env let a bad OCR_* env var raise a bare ValueError (or an opaque pydantic error for an in-range-type-but-out-of-bounds value) at import time. Both now raise a ConfigurationError with the offending variable name and value. - installed_languages() ignored subprocess failures with no check= and no timeout, which could silently cache an empty language set at startup and 400 every subsequent OCR request. Added check=True, a timeout, and a logged error before re-raising. - Fixed a stale comment in legacy.py claiming the shim defaults to TextMode.SKIP; it actually leaves mode unset, matching OcrOptions' own None default. - Fixed the README TOC entry (wrong indentation/anchor) for the OCR API section. Also, per request: - Removed doc/DESIGN.md and doc/CODE_REVIEW.md; the README is now the only doc. - The generic exception handler now passes exc_info=exc explicitly to logger.error() instead of relying on logger.exception()'s ambient sys.exc_info(), so the full traceback is guaranteed to be logged alongside the correlation id regardless of how the ASGI framework dispatches to the handler. --- README.md | 9 +- doc/CODE_REVIEW.md | 228 ------------------------ doc/DESIGN.md | 111 ------------ test/test_app.py | 4 +- workflow_ocr_backend/app.py | 57 ++++-- workflow_ocr_backend/legacy.py | 6 +- workflow_ocr_backend/model/ocrresult.py | 6 + workflow_ocr_backend/ocrservice.py | 11 +- 8 files changed, 70 insertions(+), 362 deletions(-) delete mode 100644 doc/CODE_REVIEW.md delete mode 100644 doc/DESIGN.md diff --git a/README.md b/README.md index 41474a9..1936723 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,8 @@ It's written in Python and provides a simple REST API for [ocrmypdf](https://ocr - [Installation](#installation) - [`docker-compose` Example](#docker-compose-example) - [HaRP Support (Nextcloud 32+)](#harp-support-nextcloud-32) -- [OCR Parameter Validation](#ocr-parameter-validation) + - [OCR API](#ocr-api) + - [Legacy `/process_ocr` (deprecated)](#legacy-process_ocr-deprecated) ## Prerequisites @@ -181,9 +182,9 @@ For installation and migration instructions, see the [HaRP documentation](https: `POST /v1/ocr` is the current API: a multipart `file` plus an `options` part holding a JSON object validated against a typed, closed schema (`OcrOptions`, see -[`workflow_ocr_backend/ocroptions.py`](workflow_ocr_backend/ocroptions.py) and -[`doc/DESIGN.md`](doc/DESIGN.md) for the full rationale). Unknown fields are rejected with `422` -rather than forwarded, every scalar is bounded or enumerated, and resource limits (`jobs`, +[`workflow_ocr_backend/ocroptions.py`](workflow_ocr_backend/ocroptions.py)). Unknown fields are +rejected with `422` rather than forwarded, every scalar is bounded or enumerated, and resource +limits (`jobs`, `max_image_mpixels`, the `tesseract_timeout` ceiling) are operator policy set via environment variables (`OCR_JOBS`, `OCR_MAX_IMAGE_MPIXELS`, `OCR_MAX_TESSERACT_TIMEOUT_S`), never part of the request body. See `/docs` on a running instance for the generated OpenAPI schema. diff --git a/doc/CODE_REVIEW.md b/doc/CODE_REVIEW.md deleted file mode 100644 index 3242d7a..0000000 --- a/doc/CODE_REVIEW.md +++ /dev/null @@ -1,228 +0,0 @@ -# Code Review — Workflow OCR Backend - -**Scope:** the whole application — `main.py`, `workflow_ocr_backend/`, `test/`, `Dockerfile`, `start.sh`, `.github/`, packaging and configuration. -**Baseline:** PR #12 (`bugfix/security-enhancements`), plus the follow-up commit on this branch. -**Focus:** security and coding best practices. -**Method:** source reading, plus behavioural verification against the pinned dependency versions (`ocrmypdf==17.4.2`, `nc-py-api==0.30.1`, uvicorn). Every claim marked *verified* was reproduced by execution, not inferred. - ---- - -## Summary - -The first pass of this review found a critical RCE: `ocrmypdf_parameters` was parsed into a dict and splatted into `ocrmypdf.ocr(**kwargs)` with no allow-list, reaching ocrmypdf's `plugins` parameter and from there `spec.loader.exec_module()`. - -**PR #12 closes it.** `plugins` and `plugin_manager` are blocked, and the accompanying test asserts the exploit's marker file is never written rather than merely checking for a 400 — the right kind of test for a code-execution fix. - -PR #12 also introduced four defects of its own, because its allow-list was derived at import time from `inspect.signature(ocrmypdf.ocr)` — that is, from **Python keyword names** — while callers send **CLI option names**. Those two sets are not the same. The follow-up commit on this branch fixes all four. They are recorded in full below, because the reasoning matters more than the patch. - -What remains is what the first pass called P1 onward: the service still has **no resource ceiling of any kind** — no upload size limit, no OCR timeout, no concurrency bound — and it still does CPU-bound work on the asyncio event loop, so a single large document makes the whole process, including `/heartbeat`, unresponsive. - -| | Count | -|---|---| -| Closed by PR #12 | 4 (incl. the critical) | -| Introduced by PR #12, fixed on this branch | 5 | -| Security findings still open | 7 | -| Correctness bugs still open | 10 | -| Best-practice items still open | 12 | - ---- - -## Closed by PR #12 - -| ID | Finding | How | -|---|---|---| -| SEC-1 | **Critical** — arbitrary Python import and code execution via `--plugins` | `plugins` / `plugin_manager` blocked; test asserts the marker file is never created | -| SEC-5 | Arbitrary local file paths via `--user-words` / `--user-patterns` | both blocked | -| BUG-6 | Misspelled parameters silently discarded into `extra_attrs` — no error, no effect | unknown parameters now return HTTP 400 | -| BUG-7 | `--sidecar` collided with the hardcoded `sidecar=` kwarg → `TypeError` → 500 | `sidecar` blocked | - -Also verified correct in #12, for the record: `InvalidOcrParameterError` resolves ahead of the generic `Exception` handler via Starlette's MRO lookup, so it genuinely returns 400 rather than 500; and the language regex rejects every injection form in its test matrix. - ---- - -## Introduced by PR #12 — fixed on this branch - -### PR-1 — Allow-list keyed on Python names, not CLI names (HIGH, a real regression) - -The allow-list was `inspect.signature(ocrmypdf.ocr)` keyword-only parameters. Callers send CLI option names. Verified by execution against the real ocrmypdf 17.4.2: - -| Sent by caller | On PR #12 | Before PR #12 | -|---|---|---| -| `--ocr-engine none` | **400 Unknown parameter** | **worked** — `ocr_engine` is a real `OcrOptions` model field (`_options.py:197`), so `**kwargs` → `create_options` set it | -| `--jpeg-quality 80` | **400 Unknown parameter** | silently ignored (routed to `extra_attrs`) | -| `--jpg-quality 80` | passed | passed | - -`--jpeg-quality` is the *primary documented* CLI flag; `--jpg-quality` is its `argparse.SUPPRESS`ed alias (`builtin_plugins/optimize.py:74,86`). The signature exposes only `jpg_quality`, so the allow-list accepted the hidden alias and rejected the documented spelling. - -`--ocr-engine none` is the sharper case: a documented flag (`cli.py:413`) that **worked before and failed every job after**. - -**Fix:** an explicit literal allow-list of 49 CLI option names, plus an alias map (`jpeg_quality → jpg_quality`) applied after validation, plus an `IGNORED_PARAMETERS` set for CLI-only flags (`--quiet`, `--verbose`, `--no-progress-bar`) that are accepted and dropped rather than rejected, so existing configurations carrying them keep working. - -### PR-2 — Allow-list auto-widened on every dependency bump (MEDIUM) - -The comment claimed future dangerous options could not be smuggled in. The code did the opposite: because the set was introspected from the *installed* ocrmypdf, any keyword-only parameter a future release adds would be accepted automatically, unreviewed. - -**Fix:** the explicit literal set above. Introspection is retained as a *test-time drift guard* (`test_allowed_parameters_still_resolve_against_installed_ocrmypdf`) asserting every allow-listed name still resolves against the installed library — so the list stays reviewed, but a rename or removal upstream fails loudly instead of silently 400ing at runtime. - -### PR-3 — `tesseract_config` left allowed (MEDIUM) - -Same class as the blocked `user_words`/`user_patterns`. Traced `options.tesseract.config` → `_exec/tesseract.py:366,447` → `args_tesseract.extend(tessconfig)`: appended verbatim to the tesseract argv. Not shell injection — no shell is involved — but arbitrary argv injection, and `+` yields multiple tokens: `--tesseract-config /tmp/a+/tmp/b` → `['/tmp/a', '/tmp/b']`. Verified. - -**Fix:** moved into `BLOCKED_PARAMETERS`. - -### PR-4 — Language regex used `re.match` with `$` (LOW, but reachable) - -`$` also matches before a trailing newline. Verified reachable: `--language eng\n+deu` → `{'language': ['eng\n', 'deu']}` **passed validation**. Low impact (argv, not shell), but it defeated the regex's stated purpose. - -**Fix:** `re.fullmatch`. - -### PR-5 — New log-injection sites (LOW) - -The new validation path logged the caller-controlled key with f-strings — `logger.warning(f"Rejected unknown OCR parameter '{key}'")` — a fresh instance of SEC-7. A key containing CR/LF forges log entries. - -**Fix:** `%r` lazy formatting, which escapes control characters. - ---- - -## Still open — security - -### SEC-2 — Resource guards remain caller-overridable (HIGH, partially closed) - -`keep_temporary_files` is now blocked. The rest are not. Verified against the current branch: - -``` ---max-image-mpixels 100000 -> accepted # decompression-bomb guard effectively disabled ---jobs 10000 -> accepted # unbounded worker fan-out -``` - -The allow-list validates *names*. It does not validate *values*. A small crafted PDF plus a large `--max-image-mpixels` still exhausts container memory. This is the top remaining item. - -### SEC-3 — No size limits anywhere; peak memory is a multiple of the document (HIGH) - -`ocrservice.py`, `app.py`. The pipeline is in-memory and copies repeatedly: Starlette spools the upload, ocrmypdf writes the output into a `BytesIO`, `b64encode` copies at +33%, `.decode()` copies again, pydantic serialises a third time into the JSON response. Peak resident memory is roughly 4–5× the output document, and there is no maximum upload size at the app, at uvicorn, or in the ExApp deployment. - -Compounding it: `ocrmypdf.api` holds a process-global `threading.Lock` around the whole pipeline run, so requests already serialise to one at a time — but nothing *rejects* the queued ones. They accumulate, each holding its uploaded bytes. - -### SEC-4 — Blocking CPU work on the event loop stalls the process, including `/heartbeat` (HIGH) - -`app.py` — `process_ocr` is `async def` but its body is entirely blocking. ocrmypdf is synchronous, CPU-bound, and can run for minutes. Declaring it `async` runs it *on the event loop*, so for the duration of an OCR run the process serves nothing else. `nc_py_api` registers `/heartbeat`, AppAPI polls it, and a stalled poll makes AppAPI conclude the ExApp is dead. - -Note the inversion that suggests oversight rather than intent: `installed_languages` *is* declared `def`, so FastAPI offloads it to the threadpool. The cheap endpoint is offloaded; the expensive one is not. - -### SEC-6 — Internal exception detail returned to the caller (MEDIUM) - -`app.py` — the catch-all handler returns `f"{str(exc)} ({exc.__class__.__name__})"` for *every* unhandled exception. Exception strings routinely carry absolute temp paths, library internals, and fragments of input. - -The `ExitCodeException` and `InvalidOcrParameterError` handlers are different cases and should stay as they are — the first is a contract the PHP client depends on (`message` + `ocrMyPdfExitCode`), and the second returns an app-authored message. Only the generic handler needs to become a fixed string plus a correlation id. - -### SEC-7 — Unsanitised filename in logs and in the response (MEDIUM, partially closed) - -The validation-path log injection introduced by #12 is fixed (PR-5). The original instance is not: `file.filename` is fully attacker-controlled and is still interpolated into a `logger.debug` f-string and echoed back verbatim as `OcrResult.filename`. Needs `os.path.basename`, control-character stripping, a length cap, and structured logging. - -### SEC-8 — `/docs` and `/openapi.json` are unauthenticated (MEDIUM) - -`AppAPIAuthMiddleware(disable_for=["docs", "openapi.json"])`. The middleware matches with `fnmatch` on the stripped path, so the exemption is exactly those two — no wildcard hazard — but both serve without authentication on the ExApp port. Gate them behind an env flag, default off in production. - -### SEC-9 — Supply chain and release integrity (MEDIUM) - -- **`master`** — every installation pulls a *mutable* tag. There is no way to pin, audit, or roll back a deployed version. -- **No transitive pinning** — direct deps are pinned exactly; everything underneath floats. -- **Actions pinned by tag, not SHA** — in workflows holding `APPSTORE_TOKEN` and `APP_PRIVATE_KEY`. -- **No `permissions:` block** in any workflow. -- Base image not digest-pinned; no Dependabot, CodeQL, or container scanning. - ---- - -## Still open — correctness - -All reproduced against the current branch. - -| ID | Issue | Evidence | -|---|---|---| -| BUG-1 | Multi-token values silently truncated to the first token. **#12 made this worse**: it used to mangle silently, now it hard-fails | `--title Hello World` → `{'title': 'Hello'}`; `--clean --unpaper-args --layout single` → `400 Unknown parameter 'layout'` | -| BUG-2 | `str.isnumeric()` is true for Unicode numerics, then `int()` raises → unhandled 500 | `--oversample ²` → `ValueError: invalid literal for int()` | -| BUG-3 | Negative numbers never coerced; `--` inside a value corrupts the parse | `--skip-big -1` → `'-1'` (string); `--pages 1--2` → `{'pages': 1, '2': True}` | -| BUG-4 | Any value containing `+` becomes a list, even where a scalar is expected | `--title a+b` → `['a', 'b']` | -| BUG-5 | Duplicate keys silently overwrite instead of erroring | `--language eng --language deu` → `'deu'` | -| BUG-8 | `installed_languages` has no `check=` and no `timeout=`; a tesseract failure returns `[]`, indistinguishable from "no languages installed"; the `[1:]` header-skip is brittle | `ocrservice.py` | -| BUG-9 | `UploadFile.filename` is `str \| None`; a part without a filename → pydantic ValidationError → 500 | `OcrResult.filename: str` | -| BUG-10 | `sidecar_buffer.getvalue().decode("utf-8")` can raise `UnicodeDecodeError` → 500 | `ocrservice.py` | -| BUG-11 | Annotations claim `str` where `None` is the documented default | `app.py`, `ocrservice.py` | -| BUG-12 | `output_buffer.close()` called twice | harmless for `BytesIO`, but untidy | - -Every one of BUG-1 through BUG-5 has the same root cause: `_split_parameters` still tokenises with `str.split("--")` and `str.split(" ")`. `shlex.split` fixes the class. - ---- - -## Still open — best practices - -| ID | Observation | -|---|---| -| BP-1 | `main.py` hardcodes `log_level="trace"`, activating uvicorn's `MessageLoggerMiddleware` — one log entry per ASGI message per request. *Checked:* it replaces headers and bodies with placeholders, so this is **not** a credential leak; it is log volume and disk pressure. Make it env-driven, default `info`. | -| BP-2 | `logging.getLogger('uvicorn.error')` couples application code to the server; logs vanish silently under any other runner. | -| BP-3 | No `__init__.py` in either package directory — implicit namespace packages. | -| BP-4 | No linter, formatter or type checker. BUG-9 and BUG-11 are exactly what `mypy` reports for free. | -| BP-5 | Largely addressed by #12, which added `test_ocrservice.py`. Still missing: a test asserting unauthenticated requests are rejected. | -| BP-6 | `.env` committed with `APP_SECRET=secret` and `APP_HOST=0.0.0.0`, loaded with `override=True` at test-import time and copied into the test image. Ship `.env.example`. | -| BP-7 | Dockerfile: `apk update` redundant alongside `--no-cache`; `apk search tesseract-ocr-data-` installs *every* language pack, making the image large and non-reproducible; no `--no-cache-dir`; no `HEALTHCHECK`; base image not digest-pinned. | -| BP-8 | `start.sh`: `set -e` without `-u`/`pipefail`; env vars interpolated into TOML unquoted and unvalidated; `frpc` backgrounded with no supervision; `echo "... $@"` should be `$*`. | -| BP-9 | `ErrorResult` is declared and referenced in `responses={...}` but never used to *build* a response — all three handlers hand-roll dicts, so the model and the wire format can drift. | -| BP-10 | `test.yml` builds and runs repository code in a job where HaRP receives `/var/run/docker.sock`. Contained on ephemeral GitHub-hosted runners; a critical escape the day it moves to a self-hosted runner. | -| BP-11 | `info.xml` carries `1.35.0-dev` on `master`. | -| BP-12 | No `SECURITY.md` or disclosure policy. | - ---- - -## Revised plan - -The original P0 was "replace `_split_parameters` with a validating allow-list parser". PR #12 plus this branch have done the **allow-list** half. The **validating** half is not done: names are checked, values are not. - -### P0 — Validate values, not just names - -**Closes:** SEC-2, BUG-1 … BUG-5. - -The allow-list stops `--plugins`. It does nothing about `--max-image-mpixels 100000`, `--jobs 10000`, or `--optimize high` (which still reaches ocrmypdf and surfaces as a 500 from pydantic rather than a 400). - -1. Give each allow-listed parameter a type and, where it governs resource use, a **bound**: - `jobs` ≤ CPU budget, `max_image_mpixels` in `[1, 500]` — never 0 — `optimize` in `{0,1,2,3}`, `tesseract_timeout` ≤ a ceiling, enumerations checked against their choices. -2. **Tokenise with `shlex.split()`** instead of `split("--")` / `split(" ")`. One change closes BUG-1 through BUG-5 and makes quoting work. -3. Reject duplicates rather than silently overwriting. - -### P1 — Put a ceiling on every resource - -**Closes:** SEC-3, SEC-4, BUG-8. - -1. `def process_ocr` instead of `async def`, so FastAPI runs it in the threadpool. One keyword; it is the difference between "slow" and "AppAPI restarts the container". -2. Enforce a maximum upload size from `Content-Length` before touching the body; stream to a `NamedTemporaryFile` and give ocrmypdf a path. -3. Bound concurrency with an `asyncio.Semaphore`, returning `503` when saturated. -4. Wall-clock timeout on the OCR run, and a default `tesseract_timeout`. -5. `timeout=` and `check=True` on the `installed_languages` subprocess; cache the result. - -### P2 — Tighten the response and logging boundary - -**Closes:** SEC-6, SEC-7, BUG-9 … BUG-12, BP-2, BP-9. - -Generic handler returns a fixed message plus a correlation id; keep the `ExitCodeException` and `InvalidOcrParameterError` contracts and build all three through `ErrorResult`. Sanitise `file.filename`. Structured logging throughout. Fix the `str | None` annotations. - -### P3 — Supply chain and release integrity - -**Closes:** SEC-9, BP-7, BP-10. - -Publish immutable image tags — the highest-value item here, since today there is no such thing as "the version I have installed". Hash-pinned lock file. SHA-pinned actions. Least-privilege `permissions:`. Digest-pinned base image. Dependabot, CodeQL, container scanning. - -### P4 — Tooling and hygiene - -**Closes:** BP-1, BP-3, BP-4, BP-5, BP-6, BP-8, BP-11, BP-12. - -Env-driven `log_level`. `ruff` + `mypy` in CI. `.env.example`. `start.sh` hardening. `SECURITY.md`. - ---- - -## What's already good - -- PR #12's plugin test asserts the exploit *marker file* is never created, not merely that a 400 came back. That is how a code-execution fix should be tested. -- The layering is clean — the FastAPI module has no OCR logic and `OcrService` has no HTTP concerns. -- gosu is version-pinned *and* GPG signature-verified. -- The multi-stage Dockerfile keeps the passwordless-sudo `devcontainer` and `test` stages out of the published `app` image. -- The HaRP integration test stands up a real HaRP container, drives the real ExApp lifecycle, asserts on the generated `frpc.toml`, and cleans up in a `finally`. -- Direct dependencies are pinned exactly, and CI runs the tests inside the image that ships. diff --git a/doc/DESIGN.md b/doc/DESIGN.md deleted file mode 100644 index 1f8af9b..0000000 --- a/doc/DESIGN.md +++ /dev/null @@ -1,111 +0,0 @@ -# Redesigning the `workflow_ocr_backend` OCR API - -## The bug class, not the bug - -[`doc/CODE_REVIEW.md`](CODE_REVIEW.md) closes the reachable RCE (`--plugins /tmp/evil.py`) and -tightens the parameter allow-list to CLI names. But the shape that produced the bug survived that -fix. The endpoint's contract was, effectively: - -> Send me a string. I will parse it into keyword arguments and splat them into a third-party -> function. - -```python -kwargs = self._split_parameters(ocrmypdf_parameters) -exit_code = ocrmypdf.ocr(file, output_buffer, sidecar=..., **kwargs) -``` - -Three compounding properties made this a recurring RCE generator rather than a one-off mistake: - -1. **The sink is unbounded.** `ocrmypdf.ocr()` ends in `**kwargs`, and unknown keys are forwarded - to `create_options`. It loads plugins and shells out to tesseract, ghostscript, unpaper, - pngquant and jbig2enc. Its parameter list was never designed to be a trust boundary. -2. **The transport was untyped.** A CLI-ish string parsed by `split("--")` → `split(" ")` → - shape-guessed types. Multi-token values were silently truncated, `+` turned any value into a - list, and a value containing `--` corrupted the whole parse. -3. **An allow-list only checks names, never values.** `--max-image-mpixels 100000` or - `--jobs 10000` still reached OCRmyPDF: the decompression-bomb guard and the worker-count knob - were request fields, not operator policy. - -## The redesign - -**The API exposes a small closed vocabulary of OCR intents. It does not expose the dependency's -function signature, and resource limits are not caller options.** - -### 1. Typed options, not a flag string - -`POST /v1/ocr`, multipart: `file` plus an `options` part of `application/json` validated by a -hand-written Pydantic model with `extra="forbid"` -([`workflow_ocr_backend/ocroptions.py`](../workflow_ocr_backend/ocroptions.py)). Unknown key → 422 -with a field path, never a silent forward. FastAPI generates the OpenAPI schema from the model, so -callers get a real contract instead of a doc link to the OCRmyPDF cookbook. - -### 2. Constraints live in the types - -Every scalar is bounded (`optimize: 0–3`, `tesseract_pagesegmode: 0–13`), every enumerated value is -a real enum, every string is a regex-constrained alias (`LanguageCode`, `PageRange`). Invalid states -are unrepresentable rather than rejected at runtime: `skip_text`/`force_ocr`/`redo_ocr` - three -booleans the legacy API let you set simultaneously, producing a 500 from OCRmyPDF's own validation -- collapse into one `TextMode` enum (`None` is a deliberate fourth state: "no override", matching -OCRmyPDF's own conservative default of refusing to touch a document that already has text). - -### 3. Caller intent vs. operator policy - -The split the legacy design lacked entirely. `jobs`, `max_image_mpixels` and the -`tesseract_timeout` ceiling live in `OcrPolicy`, built from environment variables -(`OCR_JOBS`, `OCR_MAX_IMAGE_MPIXELS`, `OCR_MAX_TESSERACT_TIMEOUT_S`) once at startup. A -caller-supplied timeout is *clamped*, never honoured upward -(`test_caller_cannot_raise_the_timeout_ceiling`). A test asserts that no request field can -influence any operator-owned kwarg (`test_operator_owned_kwargs_come_only_from_policy`). - -### 4. Explicit mapping, no reflection - -`OcrOptions.to_ocrmypdf_kwargs()` is written out field by field. No `**caller_data` anywhere. -Adding an option is a deliberate edit in three places (the field, the mapping, the frozen -`EMITTABLE_OCRMYPDF_KWARGS` set). This also decouples the public vocabulary from upstream's -representation: the API says `"sauvola"`, the mapping converts it to the `IntEnum` value `2` -OCRmyPDF actually wants; the public field is named `jpeg_quality` after the documented CLI flag, -and mapped to the `jpg_quality` keyword OCRmyPDF's Python signature actually exposes. - -### 5. Two structural invariants, enforced in CI - -- **No path-typed field.** `test_no_path_typed_fields` walks `OcrOptions.model_fields` and fails - on any `Path`/`PathLike`/`FilePath` annotation. `--plugins` and `--user-words` were both "a path - in the request body"; ban the shape, not the instances. -- **Signature drift breaks the build.** [`test/ocrmypdf_signature.json`](../test/ocrmypdf_signature.json) - snapshots upstream's keyword-only parameters; `test_ocrmypdf_signature_has_not_drifted` diffs - against it. This is the exact inverse of a derived allow-list: an OCRmyPDF upgrade that adds a - parameter *fails CI* and someone has to review it and update the snapshot deliberately, instead - of the boundary silently widening on `pip upgrade`. - -### 6. Language validation against reality - -`OcrOptions.validate_against_policy()` intersects requested languages with -`OcrPolicy.installed_languages`, cached from `tesseract --list-langs` at startup. Turns "language -not installed" from an OCRmyPDF-side 500 into an application-level 400. - -## Migration: the legacy shim - -Since the old contract can break callers, `/process_ocr` stays as a thin deprecated shim -([`workflow_ocr_backend/legacy.py`](../workflow_ocr_backend/legacy.py)) rather than being removed: -the flag string is tokenised (`shlex.split`, fixing the silent-truncation and corrupted-parse bugs -in the old tokenizer) and translated field-by-field onto `OcrOptions` through an explicit table. -Anything not in that table - `--plugins`, `--tesseract-config`, `--unpaper-args`, any -operator-owned knob - becomes a 400 by construction, because no code path ever puts it on the -model. Responses carry `Deprecation`/`Sunset`/`Link` headers pointing at `/v1/ocr`. - -## Beyond the API surface - -Two items from the schema's own blast radius were fixed alongside it: - -- **`process_ocr` was `async def` but called blocking `ocrmypdf.ocr()`**, stalling the event loop - - and AppAPI's `/heartbeat` poll - for the duration of every OCR run. Both `/v1/ocr` and - `/process_ocr` are now plain `def`, so FastAPI runs them in the threadpool. -- **The generic exception handler echoed `str(exc)` at 500**, which routinely carries absolute - temp paths and library internals. It now logs the detail server-side against a correlation id and - returns only `"Internal server error []"`. The `ExitCodeException` handler is unchanged - its - message is a deliberate part of the API contract, not a leak. - -Everything else flagged in `doc/CODE_REVIEW.md` under P1–P4 (upload size limits, concurrency -bounds, a sandboxed worker process for the OCRmyPDF subprocess fan-out, filename sanitisation, -supply-chain pinning) is still open and tracked there; this redesign is scoped to the request -schema and the two items above. diff --git a/test/test_app.py b/test/test_app.py index 5631fac..0ef21e4 100644 --- a/test/test_app.py +++ b/test/test_app.py @@ -170,8 +170,8 @@ def test_ocr_v1_rejects_uninstalled_language(): "/v1/ocr", files={"file": (file_name, file, "application/pdf")}, # Not a real tesseract language pack, and the CI/production image installs - # every language ocrmypdf-data ships (see doc/CODE_REVIEW.md BP-7), so a - # real-but-uninstalled code like "jpn" isn't a safe fixture here. + # every language ocrmypdf-data ships, so a real-but-uninstalled code like + # "jpn" isn't a safe fixture here. data={"options": '{"languages": ["zzzzzz"]}'} ) assert response.status_code == 400 diff --git a/workflow_ocr_backend/app.py b/workflow_ocr_backend/app.py index ebc47df..a11ccef 100644 --- a/workflow_ocr_backend/app.py +++ b/workflow_ocr_backend/app.py @@ -13,13 +13,27 @@ from ocrmypdf import ExitCodeException from pydantic import ValidationError -from .model.ocrresult import ErrorResult, OcrResult +from .model.ocrresult import ErrorResult, OcrResult, ValidationErrorResult from .ocroptions import InvalidOcrOptionsError, OcrOptions, OcrPolicy from .ocrservice import OcrService logger = logging.getLogger('uvicorn.error') # Use same logging as uvicorn +class ConfigurationError(RuntimeError): + """Raised when an OCR_* environment variable can't be parsed at startup.""" + + +def _env_number(name: str, cast: type) -> int | float | None: + value = os.getenv(name) + if value is None: + return None + try: + return cast(value) + except ValueError as exc: + raise ConfigurationError(f"Environment variable {name}={value!r} is not a valid {cast.__name__}") from exc + + def _policy_from_env(installed_languages: frozenset[str]) -> OcrPolicy: """Builds the operator-owned resource policy from the process environment. @@ -28,13 +42,19 @@ def _policy_from_env(installed_languages: frozenset[str]) -> OcrPolicy: pin the whole backend, which only takes one OCRmyPDF task at a time. """ kwargs = {} - if (v := os.getenv("OCR_JOBS")) is not None: - kwargs["jobs"] = int(v) - if (v := os.getenv("OCR_MAX_IMAGE_MPIXELS")) is not None: - kwargs["max_image_mpixels"] = float(v) - if (v := os.getenv("OCR_MAX_TESSERACT_TIMEOUT_S")) is not None: - kwargs["max_tesseract_timeout_s"] = float(v) - return OcrPolicy(installed_languages=installed_languages, **kwargs) + if (v := _env_number("OCR_JOBS", int)) is not None: + kwargs["jobs"] = v + if (v := _env_number("OCR_MAX_IMAGE_MPIXELS", float)) is not None: + kwargs["max_image_mpixels"] = v + if (v := _env_number("OCR_MAX_TESSERACT_TIMEOUT_S", float)) is not None: + kwargs["max_tesseract_timeout_s"] = v + try: + return OcrPolicy(installed_languages=installed_languages, **kwargs) + except ValidationError as exc: + # e.g. OCR_JOBS=0 or OCR_MAX_IMAGE_MPIXELS=-1 - well-formed numbers, but + # outside OcrPolicy's own bounds. Fail fast with a clear message rather + # than an opaque pydantic stacktrace at import time. + raise ConfigurationError(f"Invalid OCR policy configuration: {exc}") from exc @asynccontextmanager @@ -75,22 +95,30 @@ async def invalid_ocr_options_exception_handler(_: Request, exc: InvalidOcrOptio async def validation_error_exception_handler(_: Request, exc: ValidationError): # Schema validation of the "options" body -> 422 with field-level detail, # matching FastAPI's own convention for an invalid request body. - return JSONResponse({"message": "Invalid OCR options", "errors": exc.errors(include_url=False, include_context=False)}, status_code=422) + body = ValidationErrorResult( + message="Invalid OCR options", + errors=exc.errors(include_url=False, include_context=False), + ) + return JSONResponse(body.model_dump(by_alias=True), status_code=422) @APP.exception_handler(Exception) async def exception_handler(_: Request, exc: Exception): # Never echo str(exc) here: exception text routinely carries absolute temp - # paths and library internals. Log the detail server-side against a - # correlation id and return only that id to the caller. + # paths and library internals. Log the full exception (message + traceback) + # server-side against a correlation id, and return only that id to the + # caller. exc_info is passed explicitly rather than relying on the ambient + # sys.exc_info() - this handler runs as an awaited coroutine, and passing + # the exception directly guarantees the right traceback is logged + # regardless of how the framework dispatches to it. correlation_id = str(uuid.uuid4()) - logger.exception(f"Unhandled error [{correlation_id}]") + logger.error(f"Unhandled error [{correlation_id}]", exc_info=exc) return JSONResponse({"message": f"Internal server error [{correlation_id}]"}, status_code=500) @APP.post( "/v1/ocr", response_model=OcrResult, - responses={400: {"model": ErrorResult}, 422: {"model": ErrorResult}, 500: {"model": ErrorResult}}, + responses={400: {"model": ErrorResult}, 422: {"model": ValidationErrorResult}, 500: {"model": ErrorResult}}, ) def ocr_v1( response: Response, @@ -141,7 +169,8 @@ def process_ocr( """ service = OcrService(logger) response.headers["Deprecation"] = "true" - response.headers["Sunset"] = "Wed, 01 Jul 2026 00:00:00 GMT" + # Revisit alongside the workflow_ocr app release that switches to /v1/ocr. + response.headers["Sunset"] = "Mon, 01 Feb 2027 00:00:00 GMT" response.headers["Link"] = '; rel="successor-version"' return service.ocr_legacy(file.file, file.filename, ocrmypdf_parameters, get_policy()) diff --git a/workflow_ocr_backend/legacy.py b/workflow_ocr_backend/legacy.py index d4822af..683066e 100644 --- a/workflow_ocr_backend/legacy.py +++ b/workflow_ocr_backend/legacy.py @@ -27,8 +27,10 @@ class InvalidOcrParameterError(InvalidOcrOptionsError): # Boolean "presence" flags that select OcrOptions.mode instead of a same-named -# field. Mutually exclusive by construction - the last one seen wins, matching -# the OcrOptions default of TextMode.SKIP when none are present. +# field. Mutually exclusive by construction - the last one seen wins. When none +# of these are present, options_from_legacy_parameters leaves "mode" unset, so +# OcrOptions falls back to its own default (None: no override, matching +# OCRmyPDF's conservative default of erroring on an already-OCR'd document). _MODE_FLAGS: Final[dict[str, TextMode]] = { "skip_text": TextMode.SKIP, "force_ocr": TextMode.FORCE, diff --git a/workflow_ocr_backend/model/ocrresult.py b/workflow_ocr_backend/model/ocrresult.py index 5c523a4..0a140a1 100644 --- a/workflow_ocr_backend/model/ocrresult.py +++ b/workflow_ocr_backend/model/ocrresult.py @@ -1,9 +1,15 @@ +from typing import Any + from pydantic import BaseModel, Field class ErrorResult(BaseModel): message: str = Field(description='Error message') ocr_my_pdf_exit_code: int | None = Field(default=None, serialization_alias='ocrMyPdfExitCode', description='Exit code of the OCRmyPDF process (if applicable)') +class ValidationErrorResult(BaseModel): + message: str = Field(description='Error message') + errors: list[dict[str, Any]] = Field(description='Field-level validation errors, as returned by pydantic') + class OcrResult(BaseModel): filename: str = Field(description='Name of the file') content_type: str = Field(serialization_alias='contentType', description='Content type of the file. For example: application/pdf') diff --git a/workflow_ocr_backend/ocrservice.py b/workflow_ocr_backend/ocrservice.py index 2ec432d..42941b6 100644 --- a/workflow_ocr_backend/ocrservice.py +++ b/workflow_ocr_backend/ocrservice.py @@ -67,6 +67,15 @@ def ocr_legacy(self, file: BinaryIO, file_name: str, ocrmypdf_parameters: str | return self.ocr(file, file_name, options, policy) def installed_languages(self) -> Iterable[str]: - result = subprocess.run(["tesseract", "--list-langs"], capture_output=True, text=True) + # check=True + a timeout: this result seeds OcrPolicy at startup, so a + # tesseract failure here must fail loudly rather than silently produce + # an empty language set that then rejects every OCR request as 400. + try: + result = subprocess.run( + ["tesseract", "--list-langs"], capture_output=True, text=True, check=True, timeout=30 + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as exc: + self.logger.error(f"Could not determine installed tesseract languages: {exc}") + raise languages = result.stdout.splitlines()[1:] # Skip the first line return [lang for lang in languages if lang != "osd"]