WS-ART-001-03B3B3A: bounded OOXML container security - #233
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds bounded, validation-only OOXML/OPC security for DOCX, PPTX, and XLSX packages. It validates archive metadata, XML, relationships, content, and resource limits, then maps failures in the isolated extraction worker. ChangesOOXML container security
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExtractionWorker
participant OOXMLValidator
participant DefusedXML
ExtractionWorker->>OOXMLValidator: Validate package payload
OOXMLValidator->>DefusedXML: Parse XML safely
DefusedXML-->>OOXMLValidator: Parsed XML or parser failure
OOXMLValidator-->>ExtractionWorker: Package facts or security failure
ExtractionWorker-->>ExtractionWorker: Map failure to ExtractionFailure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 `@backend/app/modules/artifacts/guide_ooxml.py`:
- Around line 140-149: Restrict _part_is_allowed so root-level relationships are
accepted only when the name is exactly "_rels/.rels", while preserving valid
directory-level relationship parts. Update _relationship_target_is_external to
fail closed or raise for any .rels name that matches neither "_rels/.rels" nor
the "<dir>/_rels/<partname>.rels" pattern, preventing incorrect source-directory
derivation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b27fe8ee-b1fd-4e43-a257-f80ea36f70c8
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3A-ooxml-container-security.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.mdbackend/app/modules/artifacts/guide_extraction_worker.pybackend/app/modules/artifacts/guide_formats.pybackend/app/modules/artifacts/guide_ooxml.pybackend/pyproject.tomlbackend/scripts/run_test_lanes.pybackend/tests/test_artifact_architecture.pybackend/tests/test_guide_extraction.pybackend/tests/test_guide_ooxml.pydocs/spec_artifact_storage_service.md
2 participants
WS-ART-001-03B3B3A PR Trust Bundle
Chunk
WS-ART-001-03B3B3A— OOXML Container Security (L1).Goal and human-approved intent
Install only the previously approved
defusedxmldependency and add a shared,bounded OPC/OOXML security boundary for verified guide-source bytes. Project
Manager guide uploads remain distinct from contributor submission ZIPs. This
chunk must not extract DOCX/PPTX/XLSX content or activate AUTH or sufficiency.
What changed and why
defusedxml==0.7.1wheel and lock evidence.passive package-part/content-type/relationship policies, and bounded XML.
classification and child validation cannot drift.
existing bounded extraction protocol without registering an adapter.
shared_foundationsand documented the boundary.Design chosen and alternatives rejected
The existing format classifier owns server classification and shared ZIP facts;
the isolated child owns
defusedxmlparsing and strict OOXML validation. FutureDOCX, PPTX, and XLSX adapters must pass this boundary before semantic parsing.
Rejected alternatives were upload-time parsing, root-only allowlists, filename-
only active-content checks, direct provider access, generic ZIP support, and
activating incomplete document adapters.
Scope control and product behavior
The corrected contract lists every changed implementation, test, lane, and
documentation file. There is no route, provider, binding, AUTH, Celery,
submission, review, payment, reputation, or agent-input behavior. Malformed,
ambiguous, active, external, or over-limit packages become bounded internal
extraction outcomes, never guide-insufficiency decisions.
Acceptance criteria proof
encrypted, duplicate, unsafe-path, unknown/cross-format, marker-conflict,
special-entry, and executable metadata failures.
and relationship-size boundaries are covered.
macros, embeddings, executables, DTD/entities including UTF-16, active MIME
or relationship metadata, and external/escaping targets reject fail closed.
directory entries cannot satisfy required file markers.
Tests and checks
git diff --check— pass.proof; no local full-suite run was used.
Test delta and CI integrity
No test, assertion, lane, workflow, or coverage threshold was removed, skipped,
or weakened. The new focused module is in the existing
shared_foundationslane. The dependency is an exact approved PyPI wheel URL/hash and the lockfile
resolves the same bytes.
Reviewer results
Architecture, security, QA, senior engineering, CI integrity, docs, and test
delta pass. Product/ops and reuse/dedup pass with non-blocking future advice.
Initial strictness, metadata, DTD, nested-archive, reuse, and assertion findings
were repaired and re-reviewed.
External review
Pending CodeRabbit and hosted GitHub Backend/Agent Gates on the published PR
head.
Remaining risks and follow-up
OOXML is a complex untrusted container, so isolated execution and the strict
positive policy remain essential. The later 03B3B3B/C/D chunks add document-
specific extraction separately; 03B3B4 handles image metadata. AUTH remains
planned/unavailable until the complete hidden 03B series merges.
Human review focus and merge ownership
Review the exact dependency hash, central-directory-first ordering, passive
part/content-type/relationship policies, shared classifier facts, stable
failure mapping, and absence of adapter activation. A human owns merge approval;
the agent will not merge this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation