feat: Implement OCR parameter validation and error handling - #14
Open
R0Wi wants to merge 6 commits into
Open
Conversation
…ion 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwE3BeYazHp7QpGXzSNsHL
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwE3BeYazHp7QpGXzSNsHL
PR #12/#13 closed the reachable --plugins RCE with a hand-maintained allow-list, but the endpoint's contract was still "send me a string, I will parse it into kwargs and splat them into ocrmypdf.ocr(**kwargs)". The allow-list validated parameter *names*; values (--max-image-mpixels, --jobs, --tesseract-timeout) were still fully caller-controlled. This introduces POST /v1/ocr: a hand-written Pydantic OcrOptions model (extra="forbid", every scalar bounded/enumerated/regex-constrained, no path- or argv-typed field) that is the API's own contract rather than something derived from ocrmypdf's signature. Resource limits (jobs, max_image_mpixels, the tesseract_timeout ceiling) move into an OcrPolicy built from environment variables - operator policy, never a request field; a caller-supplied timeout is clamped, never honoured upward. Mapping to ocrmypdf kwargs is written out field by field, with no **caller_data splat anywhere. Two structural invariants are enforced in CI: no field may carry a Path/PathLike type (test_no_path_typed_fields), and test/ocrmypdf_signature.json snapshots ocrmypdf's keyword-only parameters so an upstream release that adds one fails the build instead of silently widening what's reachable. /process_ocr stays as a deprecated shim (Deprecation/Sunset/Link headers): the legacy --flag string is tokenised with shlex and translated field-by-field onto OcrOptions through an explicit table, so anything not in that table (--plugins, --tesseract-config, --unpaper-args, any operator-owned knob) is a 400 by construction. This also fixes the old tokenizer's silent multi-word truncation and duplicate-key overwrite bugs. Also: both endpoints are now plain `def` so FastAPI runs the blocking ocrmypdf.ocr() call in the threadpool instead of stalling the event loop (and AppAPI's /heartbeat poll) for the run's duration; the generic exception handler no longer echoes str(exc) (internal paths) at 500, returning a correlation id instead and logging detail server-side. See doc/DESIGN.md for the full rationale.
The Dockerfile installs every tesseract-ocr-data language package (see doc/CODE_REVIEW.md BP-7), so "jpn" is actually installed in the CI/prod image and test_ocr_v1_rejects_uninstalled_language got a 200 instead of the expected 400. Use a syntactically valid but non-existent language code instead of relying on a specific real code being absent.
There was a problem hiding this comment.
Pull request overview
This PR hardens the OCR backend’s public surface by replacing the legacy free-form ocrmypdf_parameters string → **kwargs passthrough with a typed, closed OcrOptions schema plus operator-owned OcrPolicy, adding runtime validation and safer error handling. It introduces a new /v1/ocr endpoint while keeping /process_ocr as a deprecated translating shim.
Changes:
- Add
OcrOptions/OcrPolicyPydantic models with explicit mapping to OCRmyPDF kwargs, plus invariants enforced by tests. - Add a legacy parameter shim (
legacy.py) that tokenizes withshlex.splitand maps an allow-listed set of flags ontoOcrOptions. - Update FastAPI app wiring: new
/v1/ocrendpoint, policy caching at startup, and revised exception handling (400/422 vs 500).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow_ocr_backend/ocrservice.py | Service now takes typed options + policy, adds legacy delegator method. |
| workflow_ocr_backend/ocroptions.py | New typed OCR request schema + explicit kwargs mapping + policy model/constants. |
| workflow_ocr_backend/legacy.py | New legacy --flag value parser/translator into OcrOptions. |
| workflow_ocr_backend/app.py | Adds /v1/ocr, deprecates /process_ocr, caches policy, adds exception handlers. |
| test/test_ocroptions.py | New invariant tests for trust boundary + upstream signature drift checks. |
| test/test_legacy.py | Tests for legacy parsing/translation and rejection of dangerous/unknown flags. |
| test/test_app.py | Endpoint-level tests for new API and legacy shim safety/deprecation headers. |
| test/ocrmypdf_signature.json | Snapshot of OCRmyPDF keyword-only parameters for drift detection. |
| requirements.txt | Adds Pydantic v2 pin. |
| README.md | Documents new OCR API and legacy deprecation details (TOC needs alignment). |
| doc/DESIGN.md | Design rationale for schema/policy split and invariant testing strategy. |
| doc/CODE_REVIEW.md | Adds structured security/correctness review notes and roadmap. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @APP.post( | ||
| "/v1/ocr", | ||
| response_model=OcrResult, | ||
| responses={400: {"model": ErrorResult}, 422: {"model": ErrorResult}, 500: {"model": ErrorResult}}, |
Comment on lines
+143
to
+145
| response.headers["Deprecation"] = "true" | ||
| response.headers["Sunset"] = "Wed, 01 Jul 2026 00:00:00 GMT" | ||
| response.headers["Link"] = '</v1/ocr>; rel="successor-version"' |
Comment on lines
+30
to
+37
| kwargs = {} | ||
| if (v := os.getenv("OCR_JOBS")) is not None: | ||
| kwargs["jobs"] = int(v) | ||
| if (v := os.getenv("OCR_MAX_IMAGE_MPIXELS")) is not None: | ||
| kwargs["max_image_mpixels"] = float(v) | ||
| if (v := os.getenv("OCR_MAX_TESSERACT_TIMEOUT_S")) is not None: | ||
| kwargs["max_tesseract_timeout_s"] = float(v) | ||
| return OcrPolicy(installed_languages=installed_languages, **kwargs) |
Comment on lines
69
to
72
| def installed_languages(self) -> Iterable[str]: | ||
| result = subprocess.run(["tesseract", "--list-langs"], capture_output=True, text=True) | ||
| languages = result.stdout.splitlines()[1:] # Skip the first line | ||
| return [lang for lang in languages if lang != "osd"] |
Comment on lines
+29
to
+31
| # Boolean "presence" flags that select OcrOptions.mode instead of a same-named | ||
| # field. Mutually exclusive by construction - the last one seen wins, matching | ||
| # the OcrOptions default of TextMode.SKIP when none are present. |
| - [Installation](#installation) | ||
| - [`docker-compose` Example](#docker-compose-example) | ||
| - [HaRP Support (Nextcloud 32+)](#harp-support-nextcloud-32) | ||
| - [OCR Parameter Validation](#ocr-parameter-validation) |
… logging
Copilot review (pull#14 review 4987030551):
- /v1/ocr's 422 response advertised ErrorResult but actually returned
{message, errors}; added a ValidationErrorResult model and used it
for both the response docs and the handler's actual payload.
- The /process_ocr Sunset header was already in the past. Moved it out
to a real future date.
- _policy_from_env let a bad OCR_* env var raise a bare ValueError (or
an opaque pydantic error for an in-range-type-but-out-of-bounds value)
at import time. Both now raise a ConfigurationError with the offending
variable name and value.
- installed_languages() ignored subprocess failures with no check= and
no timeout, which could silently cache an empty language set at
startup and 400 every subsequent OCR request. Added check=True, a
timeout, and a logged error before re-raising.
- Fixed a stale comment in legacy.py claiming the shim defaults to
TextMode.SKIP; it actually leaves mode unset, matching OcrOptions'
own None default.
- Fixed the README TOC entry (wrong indentation/anchor) for the OCR API
section.
Also, per request:
- Removed doc/DESIGN.md and doc/CODE_REVIEW.md; the README is now the
only doc.
- The generic exception handler now passes exc_info=exc explicitly to
logger.error() instead of relying on logger.exception()'s ambient
sys.exc_info(), so the full traceback is guaranteed to be logged
alongside the correlation id regardless of how the ASGI framework
dispatches to the handler.
Minimum allowed line rate is |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.