fix(security): backport DoS caps + log hygiene lost in the refactor - #591
Merged
Conversation
Backport of 8ca8561 (v1.1.x hardening, M8 family) lost in the hexagonal refactor. save_file_to_disk streamed uploads with no byte cap; the per-user quota limits file count, not bytes, so one request could write an arbitrarily large file and exhaust disk/RAM. Enforce a configurable max (MAX_UPLOAD_SIZE_MB, default 1024) during streaming, returning 413 and removing the partial file when exceeded.
Backport of 221f8ed (M8). The hexagonal refactor dropped the parser-bomb caps and reintroduced the attacker-controlled allocation the original fix removed: DocxParser._extract_embedded_images built [None]*max_order where max_order is parsed from the untrusted word/media/imageN filename -> a small crafted DOCX could OOM the indexer. - DOCX: cap embedded media entries iterated and per-entry decompressed size; skip non-positive indices; only materialise the positional array when the max index is within the cap, else fall back to a compact ordered list. - PPTX: cap slides walked and pictures decoded into memory. - PDF (marker): cap pages processed per file (_MAX_PDF_PAGES). EML fan-out/recursion caps were already carried over (_MAX_EML_ATTACHMENTS + _build_eml depth bound), so are unchanged. Caps follow the existing module-level constant pattern used by the EML parser.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds upload-size enforcement, parser safety caps for PDF/PPTX/DOCX ingestion, and logging default changes that lower verbosity and omit query text from a web search warning. ChangesParser-bomb, upload, and logging caps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce5842c57a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ch log Backport of 1bfca31 (M10). DEBUG was the default level and the web-search zero-results warning logged the raw query text (survives an INFO default and persists to the long-lived JSON sink), leaking potentially sensitive request content. - conf/config.yaml + infra/compose/.env.example: default log level INFO. - websearch/service.py: drop the query string from the zero-results warning. The pre-refactor pipeline.py temporal-filter warning that also logged the query has no hexagonal equivalent (that retry path no longer logs the query), so needs no change. Search endpoints already log query_len, not the text.
andyne13
changed the title
MAX_UPLOAD_SIZE_BYTES was evaluated at import, but api.main imports the admin routers (and thus this module) before it calls load_config()/ load_dotenv(). A MAX_UPLOAD_SIZE_MB set in .env was therefore ignored on local starts and the process silently used the 1024 MB default. Read the value lazily inside save_file_to_disk via _max_upload_size_bytes().
save_file_to_disk raises ValidationError (413 for oversize uploads, 400 for bad filenames), but add_file and execute_tool wrapped it in a broad 'except Exception' that re-raised HTTP 500 — so the new payload-too-large contract looked like a server failure. Let OpenRAGError propagate to the registered handler, which maps it to its declared status. Adds a route-level regression test asserting 413 (not 500) for an oversize upload.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/api/dependencies/files.py`:
- Around line 16-28: `_max_upload_size_bytes` currently treats
`MAX_UPLOAD_SIZE_MB=0` as unlimited, but zero should be an enforced zero-byte
cap and only negative values should disable the limit. Update the docstring in
`_max_upload_size_bytes` to reflect that negative values disable enforcement,
then change the upload-size check in the file upload path (the `max_bytes` / `>
0` guard) so enforcement runs for `0` as well. Finally, adjust the env-based
test to expect non-empty uploads to be rejected when `MAX_UPLOAD_SIZE_MB` is
`0`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b20bd25c-789e-4a95-9200-719ea34b6f43
📒 Files selected for processing (5)
openrag/api/dependencies/files.pyopenrag/api/routers/admin/indexing.pyopenrag/api/routers/admin/tools.pytests/unit/api/dependencies/test_files.pytests/unit/api/routers/admin/test_indexing_upload_errors.py
Ahmath-Gadji
force-pushed
the
fix/v2-security-backport
branch
from
June 30, 2026 08:19
48be90e to
697aabe
Compare
This was referenced Jul 15, 2026
Closed
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.
What
Backports the v1.1.x security-hardening fixes that the hexagonal refactor dropped (relocated code reimplemented without the protections). All are confirmed reachable in hexagonal.
1. Bound upload size (
8ca8561e)save_file_to_diskstreamed uploads with no byte cap — the per-user quota limits file count, not bytes, so one request could write an arbitrarily large file and exhaust disk/RAM. Now enforcesMAX_UPLOAD_SIZE_MB(default 1024) during streaming, returns 413, and removes the partial file on overflow.2. Parser fan-out/page caps + DOCX memory-bomb fix (
221f8ed8)The refactor reintroduced an attacker-controlled allocation:
DocxParser._extract_embedded_imagesbuilt[None] * max_order, wheremax_orderis parsed from the untrustedword/media/imageNfilename — a small crafted DOCX (e.g.image999999999.png) could OOM the indexer.EML fan-out/recursion caps were already carried over (
_MAX_EML_ATTACHMENTS+_build_emldepth bound) and are unchanged. New caps follow the EML parser's existing module-constant pattern.3. Log hygiene (
1bfca310)DEBUG was the default level and the web-search zero-results warning logged raw query text (survives an INFO default, persists to the long-lived JSON sink).
conf/config.yaml+infra/compose/.env.example: default log level INFO.websearch/service.py: drop the query string from the zero-results warning.Investigated and intentionally NOT included
63a857af) — the pre-refactor sink wasloaders/base.py:get_image_description, which forwarded a raw document image URL to the VLM. Hexagonal removed that sink: the markdown parser storessource_urlbut never fetches it, and the caption stage passesimage_bytes(not the URL) to the VLM. No reachable SSRF exists today. Tracked as defense-in-depth for if a remote-image fetch stage is ever wired (restoreimage_captioning_url=false+ SSRF guard at that point).Tests
test_save_file_to_disk_rejects_oversize_upload(413 + partial cleanup)test_huge_positional_index_does_not_allocate(memory-bomb regression),test_non_positive_index_skippedContext
Pre-v2.0 backport audit of v1.1.12–1.1.13 fixes vs
refactor/hexagonal. The remaining audit items were all confirmed already present; this PR closes the real gaps.Summary by CodeRabbit