feat: Implement OCR parameter validation and error handling - #13
Open
R0Wi wants to merge 3 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
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
2 participants
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.