From 6d5d77950406a82929bfa62b2dff0afcc6c33016 Mon Sep 17 00:00:00 2001 From: Robin Windey Date: Wed, 19 Aug 2026 20:38:57 +0000 Subject: [PATCH 1/3] 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/3] 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/3] 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}'")