diff --git a/README.md b/README.md index 600b0f2..d591d37 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,12 @@ 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 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 new file mode 100644 index 0000000..3242d7a --- /dev/null +++ b/doc/CODE_REVIEW.md @@ -0,0 +1,228 @@ +# 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/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..85fd394 --- /dev/null +++ b/test/test_ocrservice.py @@ -0,0 +1,117 @@ +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 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..41fc08b 100644 --- a/workflow_ocr_backend/ocrservice.py +++ b/workflow_ocr_backend/ocrservice.py @@ -3,13 +3,86 @@ from datetime import datetime, timezone 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 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 @@ -77,5 +150,37 @@ def _split_parameters(self, ocrmypdf_parameters: str) -> dict[str, str | bool | # Flag value = True - params[key] = value + 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}'")