fix: harden deserialization, path traversal, log redaction, and size limits #57
Conversation
…limits Address all 5 security findings from issue #55: 1. [CRITICAL] Restricted pickle deserialization via _RestrictedUnpickler (blocks arbitrary code execution from crafted index files) 2. [HIGH] Path traversal validation on index_name at all entry points 3. [MEDIUM] Staging boundary check before file deletion in crash handler 4. [MEDIUM] API key redaction in LEANN debug/error log output 5. [LOW] Bounded zlib decompression (100MB), download size (500MB), and tarball extraction limits (100MB/file, 500MB total) Includes 25 new tests covering all fixes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: fe427a03c3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
paperpipe/paperqa.py
Outdated
| for i in range(0, len(data), block_size): | ||
| chunk = dobj.decompress(data[i : i + block_size]) | ||
| total += len(chunk) | ||
| if total > max_size: |
There was a problem hiding this comment.
The new _safe_zlib_decompress still calls dobj.decompress(...) without a max_length, so a crafted zlib stream that expands massively in a single call will allocate the full output before the total > max_size check runs. That means a decompression bomb can still exhaust memory even with the limit in place when reading untrusted files.zip contents. To actually bound memory, pass a remaining-length cap (e.g., dobj.decompress(chunk, max_size - total)) and handle unconsumed_tail/unused_data accordingly. The same issue appears in paperpipe/paperqa_mcp_server.py’s _safe_zlib_decompress.
Useful? React with 👍 / 👎.
- Validate index_name at entry of get_pqa_index_status before path construction - Use decompressobj max_length to truly bound memory during decompression - Guard int(Content-Length) against malformed headers (ValueError) - Use validated index_name in _write_index_metadata path construction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 participant
Summary
Fix all 5 security vulnerabilities reported in #55 (code audit findings), ordered by severity.
Closes: #55
Changes
_RestrictedUnpickler+_safe_unpicklereplace rawpickle.loads()inpaperqa.pyandpaperqa_mcp_server.py. Only allows basic Python builtins (dict,str,int, etc.), blocking arbitrary code execution from crafted index files._validate_index_name()rejects..,/,\, null bytes. Applied at 7 path-construction entry points acrosspaperqa.py,paperqa_mcp_server.py, andleann.py._paperqa_find_crashing_file()now verifies all returned paths are withinpaper_directoryviais_relative_to(). Additional guard incli/ask.pybeforef.unlink()._redact_cmd()inleann.pymasks--api-key/--embedding-api-keyvalues with***in 3 debug/error log locations._safe_zlib_decompress()(100MB limit),_MAX_DOWNLOAD_SIZE(500MB) check on Content-Length + body,_MAX_TAR_MEMBER_SIZE/_MAX_TAR_TOTAL_SIZEfor tarball extraction.test_paperqa.py,test_leann.py,test_paper.py.headersattribute toMockResponseintest_figure_extraction.py(required by new download size check).Type of Change
Testing
make check(required) — 470 passed, 0 failed, lint clean, 75.80% coverageChecklist