From b31e5653e05631608726769d54b07f2a21d779c3 Mon Sep 17 00:00:00 2001 From: Abiorh001 Date: Fri, 31 Jul 2026 12:44:32 +0100 Subject: [PATCH 1/2] feat(artifacts): add bounded OOXML container security --- .../REVIEW_LOG.md | 16 + .../STATUS.md | 10 +- ...RT-001-03B3B3A-ooxml-container-security.md | 12 +- ...RT-001-03B3B3A-internal-review-evidence.md | 60 +++ .../WS-ART-001-03B3B3A-pr-trust-bundle.md | 97 +++++ .../artifacts/guide_extraction_worker.py | 13 + .../app/modules/artifacts/guide_formats.py | 52 +-- backend/app/modules/artifacts/guide_ooxml.py | 319 +++++++++++++++ backend/pyproject.toml | 1 + backend/scripts/run_test_lanes.py | 1 + backend/tests/test_artifact_architecture.py | 23 ++ backend/tests/test_guide_extraction.py | 10 + backend/tests/test_guide_ooxml.py | 369 ++++++++++++++++++ backend/uv.lock | 10 + docs/spec_artifact_storage_service.md | 15 + 15 files changed, 978 insertions(+), 30 deletions(-) create mode 100644 .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-internal-review-evidence.md create mode 100644 .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md create mode 100644 backend/app/modules/artifacts/guide_ooxml.py create mode 100644 backend/tests/test_guide_ooxml.py diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md index e5ce52ff..26804e34 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md @@ -264,3 +264,19 @@ advice is recorded but is not a blocker for this bounded format adapter. - CodeRabbit's valid external findings add bounded `KeyError`/`IndexError` malformed-PDF mapping and document the 100,000-object inspection limit. + +## WS-ART-001-03B3B3A + +- Installed only the approved hash-bound `defusedxml` wheel and added hidden, + validation-only OOXML container security without activating a document + adapter, AUTH action, or sufficiency flow. +- Initial internal review found broad package-part acceptance, active metadata, + DTD, nested-archive, root-relationship, shared-policy, and weak assertion + gaps. Repairs use exact-format positive policies, strict passive metadata, + parser-enforced XML denial, bounded read failures, canonical classifier facts, + and zero-body-read ordering proof. +- Focused validation passes 136 tests with 93.92 percent OOXML coverage; lane, + dependency, lock, stale-contract, link, and diff gates pass. +- Architecture, security, QA, senior engineering, product/ops, reuse, CI, + test-delta, and docs review tracks pass after repair. Hosted GitHub and + CodeRabbit evidence remain the publication gate. diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md index 38848e0c..1b7e452a 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md @@ -73,9 +73,13 @@ package, lock, runtime import, or parser behavior change. Its approval gate requires independent protected GitHub review of the exact final PR head before merge; repository-authored evidence alone is not authority. -03B3B2 is the active successor. It installs only the approved `pypdf` wheel and -adds bounded passive-PDF text extraction inside the existing isolated child. -It does not activate AUTH, invoke sufficiency, or begin OOXML/image work. +03B3B2 is merged through PR #231. It installs only the approved `pypdf` wheel +and adds bounded passive-PDF text extraction inside the existing isolated +child. + +03B3B3A is the active successor. It installs only the approved `defusedxml` +wheel and adds the shared bounded OPC/OOXML container security capability. It +does not activate a DOCX, PPTX, or XLSX extractor, AUTH, or sufficiency. AUTH `WS-XINT-002-04B` follows the complete hidden split-03B series and activates only fixed-service binding and guide read. ART-03C then removes the diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3A-ooxml-container-security.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3A-ooxml-container-security.md index bcc92819..4269cdc9 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3A-ooxml-container-security.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3A-ooxml-container-security.md @@ -29,8 +29,13 @@ P2 ```text backend/app/modules/artifacts/guide_ooxml.py +backend/app/modules/artifacts/guide_formats.py backend/app/modules/artifacts/guide_extraction_worker.py +backend/scripts/run_test_lanes.py backend/tests/test_guide_ooxml.py +backend/tests/test_guide_extraction.py +backend/tests/test_artifact_architecture.py +backend/tests/test_guide_extractor_dependencies.py backend/tests/fixtures/guide_ooxml/** backend/pyproject.toml backend/uv.lock @@ -59,13 +64,16 @@ No document adapter/registry activation, generic ZIP semantics, AUTH/Celery/subm - Require exact classification and reject ambiguity. Parser imports execute only in the isolated child. Prove every rejection class and boundary plus crash, timeout, cancellation, cleanup, and the dependency gate. +- Assign the new focused test module to an existing canonical hosted semantic + lane without changing lane or coverage policy. ## Verification commands ```bash (cd backend && uv run ruff check app tests) -(cd backend && python scripts/check_guide_extractor_dependencies.py) -(cd backend && uv run pytest -q tests/test_guide_ooxml.py --cov=app.modules.artifacts --cov-report=term-missing --cov-fail-under=90) +(cd backend && uv run python scripts/check_guide_extractor_dependencies.py) +(cd backend && uv run pytest -q tests/test_guide_ooxml.py tests/test_guide_extraction.py tests/test_artifact_architecture.py) +(cd backend && uv run pytest -q tests/test_guide_ooxml.py --cov=app.modules.artifacts.guide_ooxml --cov-report=term-missing --cov-fail-under=90) (metadata_dir="$(mktemp -d)" && trap 'rm -rf "$metadata_dir"' EXIT && (cd backend && WORKSTREAM_TEST_ADMIN_DATABASE_URL=postgresql+asyncpg://workstream:workstream@localhost:5433/postgres .venv/bin/python scripts/run_isolated_tests.py --metadata-json "$metadata_dir/result.json" --timeout-seconds 12600 -- .venv/bin/python -m pytest -q --ignore=tests/test_isolated_database_runner.py --cov=app --cov-report=term-missing --cov-fail-under=78)) python3 scripts/check_stale_artifact_contracts.py python3 scripts/check_markdown_links.py diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-internal-review-evidence.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-internal-review-evidence.md new file mode 100644 index 00000000..fb982e78 --- /dev/null +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-internal-review-evidence.md @@ -0,0 +1,60 @@ +# Internal Review Evidence: WS-ART-001-03B3B3A + +Reviewed against trusted main: `081dfe70` + +Reviewed at: `2026-07-31` + +## Candidate + +Hidden shared OOXML container validation for server-classified DOCX, PPTX, and +XLSX guide sources. The candidate installs only the approved hash-bound +`defusedxml` wheel and does not activate a document adapter, AUTH action, +sufficiency invocation, or contributor-submission behavior. + +## Deterministic Evidence + +- Ruff, approved-dependency gate, and `uv lock --check`: PASS; +- focused OOXML, extraction, format-classification, and architecture suite: + 136 passed; +- OOXML module coverage: 93.92 percent, above the 90 percent subsystem floor; +- canonical CI lane inventory: 31 passed; +- stale artifact-contract scan, Markdown links, and `git diff --check`: PASS; +- repository-wide coverage and all hosted semantic lanes remain delegated to + the GitHub Backend gate; no local full-suite run was used. + +## Reviewer Results + +| Reviewer | Result | Blocking findings | +|---|---|---| +| architecture | PASS | none | +| security/auth | PASS | none | +| QA/test | PASS | none | +| senior engineering | PASS | none | +| product/ops | PASS WITH LOW RISKS | none | +| reuse/dedup | PASS WITH LOW RISKS | none | +| CI integrity | PASS | none | +| test delta | PASS | none | +| docs | PASS | none | + +## Material Repairs + +- replaced root-only acceptance with an exact-format positive passive-part + policy and prevented directories from impersonating required parts; +- inspect content-type and relationship metadata with strict passive + allowlists, exact passive image MIME/extension matching, safe OPC target + resolution, and explicit DTD/entity/external-reference parser denial; +- bounded corrupt member reads and detected prefixed nested archives; +- reused the classifier's canonical limits, required markers, and EOCD parser; +- proved metadata rejections perform zero entry-body reads and tightened every + bounded failure assertion identified by test-delta review. + +## Accepted Low Risks + +- The OOXML-specific executable list intentionally exceeds the generic ZIP + detector list; later adapter work should keep the common subset explicit. +- Hosted repository coverage and exact installed-wheel behavior remain final + publication evidence. + +Valid findings addressed: yes + +Open sub-agent sessions: none diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md new file mode 100644 index 00000000..6f2e2e27 --- /dev/null +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md @@ -0,0 +1,97 @@ +# 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 `defusedxml` dependency 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 + +- Added the exact hash-bound `defusedxml==0.7.1` wheel and lock evidence. +- Added central-directory-first OOXML validation with exact format markers, + passive package-part/content-type/relationship policies, and bounded XML. +- Reused the classifier's canonical limits, markers, and EOCD facts so + classification and child validation cannot drift. +- Added a typed isolated-worker loader that maps every security failure to the + existing bounded extraction protocol without registering an adapter. +- Assigned focused tests to `shared_foundations` and documented the boundary. + +## Design chosen and alternatives rejected + +The existing format classifier owns server classification and shared ZIP facts; +the isolated child owns `defusedxml` parsing and strict OOXML validation. Future +DOCX, 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 + +- All metadata is inspected before any body read; a spy proves zero reads for + encrypted, duplicate, unsafe-path, unknown/cross-format, marker-conflict, + special-entry, and executable metadata failures. +- Exact 2,000/2,001 entry, central-directory, decompressed-byte, compression, + and relationship-size boundaries are covered. +- Symlinks, traversal, encryption, ZIP64/multidisk, nested/prefixed archives, + macros, embeddings, executables, DTD/entities including UTF-16, active MIME + or relationship metadata, and external/escaping targets reject fail closed. +- Only the exact classified format root and passive OPC parts are accepted; + directory entries cannot satisfy required file markers. +- Corrupt local headers and body reads map to stable bounded failures. + +## Tests and checks + +- Ruff, dependency gate, and lock check — pass. +- Focused OOXML/extraction/classification/architecture suite — 136 passed. +- OOXML module coverage — 93.92 percent. +- CI lane inventory — 31 passed. +- Stale-contract scan, Markdown links, and `git diff --check` — pass. +- Hosted Backend/Agent Gates retain repository-wide coverage and semantic-lane + 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_foundations` +lane. 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. diff --git a/backend/app/modules/artifacts/guide_extraction_worker.py b/backend/app/modules/artifacts/guide_extraction_worker.py index 1cbc2e57..a0934625 100644 --- a/backend/app/modules/artifacts/guide_extraction_worker.py +++ b/backend/app/modules/artifacts/guide_extraction_worker.py @@ -188,6 +188,19 @@ def bounded_extract(payload: bytes) -> str: return bounded_extract +def _load_ooxml_security() -> Callable[[bytes, str], object]: + """Load the approved shared OOXML validator for later format adapters.""" + from app.modules.artifacts.guide_ooxml import OoxmlSecurityFailure, validate_ooxml + + def bounded_validate(payload: bytes, detected_format: str) -> object: + try: + return validate_ooxml(payload, detected_format=detected_format) + except OoxmlSecurityFailure as exc: + raise ExtractionFailure(exc.status, exc.code) from exc + + return bounded_validate + + def main() -> int: """Apply resource/isolation controls and emit one bounded JSON result.""" detected_format = sys.argv[1] if len(sys.argv) == 2 else "" diff --git a/backend/app/modules/artifacts/guide_formats.py b/backend/app/modules/artifacts/guide_formats.py index b970df97..0620da85 100644 --- a/backend/app/modules/artifacts/guide_formats.py +++ b/backend/app/modules/artifacts/guide_formats.py @@ -27,6 +27,29 @@ ".ps1", ".scr", } +OOXML_REQUIRED_MARKERS = { + "docx": frozenset({"[Content_Types].xml", "_rels/.rels", "word/document.xml"}), + "pptx": frozenset({"[Content_Types].xml", "_rels/.rels", "ppt/presentation.xml"}), + "xlsx": frozenset({"[Content_Types].xml", "_rels/.rels", "xl/workbook.xml"}), +} + + +def zip_directory_facts(source: BinaryIO | BytesIO) -> tuple[int, int]: + """Read bounded EOCD facts before ZipFile allocates its entry inventory.""" + source.seek(0, 2) + size = source.tell() + source.seek(max(0, size - 65_557)) + tail = source.read(65_557) + marker = tail.rfind(b"PK\x05\x06") + if marker < 0 or len(tail) - marker < 22: + raise zipfile.BadZipFile("missing end of central directory") + if tail[marker + 4 : marker + 8] != b"\x00\x00\x00\x00": + raise zipfile.BadZipFile("multi-disk archive") + entry_count = int.from_bytes(tail[marker + 10 : marker + 12], "little") + central_directory_bytes = int.from_bytes(tail[marker + 12 : marker + 16], "little") + if entry_count == 0xFFFF or central_directory_bytes == 0xFFFFFFFF: + raise zipfile.BadZipFile("zip64 archive is unsupported") + return entry_count, central_directory_bytes @dataclass(frozen=True, slots=True) @@ -162,12 +185,9 @@ def _inspect_zip(self, reader: BinaryIO) -> GuideFormatResult: reader.seek(0) with zipfile.ZipFile(reader) as archive: names = {info.filename for info in archive.infolist()} - markers = { - "docx": {"[Content_Types].xml", "_rels/.rels", "word/document.xml"}, - "pptx": {"[Content_Types].xml", "_rels/.rels", "ppt/presentation.xml"}, - "xlsx": {"[Content_Types].xml", "_rels/.rels", "xl/workbook.xml"}, - } - matches = [name for name, required in markers.items() if required <= names] + matches = [ + name for name, required in OOXML_REQUIRED_MARKERS.items() if required <= names + ] facts = self._bounded_zip_facts(state) if len(matches) > 1: return GuideFormatResult("zip", "ambiguous", facts) @@ -185,7 +205,7 @@ def _inspect_archive( state["depth"] = max(state["depth"], depth) if depth > self._limits.maximum_nesting_depth: return "limit_exceeded" - entry_count, central_directory_bytes = self._zip_directory_facts(source) + entry_count, central_directory_bytes = zip_directory_facts(source) if ( state["entries"] + entry_count > self._limits.maximum_entries or central_directory_bytes > self._limits.maximum_central_directory_bytes @@ -257,24 +277,6 @@ def _inspect_archive( return nested_result return None - @staticmethod - def _zip_directory_facts(source: BinaryIO | BytesIO) -> tuple[int, int]: - """Read the bounded EOCD before ZipFile allocates its entry inventory.""" - source.seek(0, 2) - size = source.tell() - source.seek(max(0, size - 65_557)) - tail = source.read(65_557) - marker = tail.rfind(b"PK\x05\x06") - if marker < 0 or len(tail) - marker < 22: - raise zipfile.BadZipFile("missing end of central directory") - if tail[marker + 4 : marker + 8] != b"\x00\x00\x00\x00": - raise zipfile.BadZipFile("multi-disk archive") - entry_count = int.from_bytes(tail[marker + 10 : marker + 12], "little") - central_directory_bytes = int.from_bytes(tail[marker + 12 : marker + 16], "little") - if entry_count == 0xFFFF or central_directory_bytes == 0xFFFFFFFF: - raise zipfile.BadZipFile("zip64 archive is unsupported") - return entry_count, central_directory_bytes - @staticmethod def _is_audio_video_signature(header: bytes) -> bool: return ( diff --git a/backend/app/modules/artifacts/guide_ooxml.py b/backend/app/modules/artifacts/guide_ooxml.py new file mode 100644 index 00000000..87c1c18c --- /dev/null +++ b/backend/app/modules/artifacts/guide_ooxml.py @@ -0,0 +1,319 @@ +"""Shared bounded OPC/OOXML container security for isolated guide adapters.""" + +from __future__ import annotations + +from dataclasses import dataclass +from io import BytesIO +import posixpath +from pathlib import PurePosixPath +from urllib.parse import urlsplit +import stat +import unicodedata +import zipfile + +from defusedxml import ElementTree +from defusedxml.common import DefusedXmlException + +from app.modules.artifacts.guide_formats import ( + OOXML_REQUIRED_MARKERS, + GuideFormatLimits, + zip_directory_facts, +) + + +_GUIDE_FORMAT_LIMITS = GuideFormatLimits() +MAXIMUM_ENTRIES = _GUIDE_FORMAT_LIMITS.maximum_entries +MAXIMUM_CENTRAL_DIRECTORY_BYTES = _GUIDE_FORMAT_LIMITS.maximum_central_directory_bytes +MAXIMUM_DECOMPRESSED_BYTES = _GUIDE_FORMAT_LIMITS.maximum_decompressed_bytes +MAXIMUM_COMPRESSION_RATIO = _GUIDE_FORMAT_LIMITS.maximum_compression_ratio +MAXIMUM_RELATIONSHIP_BYTES = 1024 * 1024 +_FORMAT_ROOT = {"docx": "word", "pptx": "ppt", "xlsx": "xl"} +_REQUIRED = OOXML_REQUIRED_MARKERS +_EXECUTABLE_SUFFIXES = ( + ".bat", ".bin", ".cmd", ".com", ".dll", ".exe", ".jar", ".js", ".msi", + ".ps1", ".py", ".scr", ".sh", ".vbs", ".zip", ".docm", ".pptm", ".xlsm", +) +_PASSIVE_PART_SUFFIXES = frozenset( + {".xml", ".rels", ".png", ".jpg", ".jpeg", ".gif", ".bmp", ".tif", ".tiff", ".emf", ".wmf", ".odttf"} +) +_COMMON_PREFIXES = ("_rels/", "docprops/", "customxml/") +_ACTIVE_METADATA_TOKENS = ( + "activex", "attachedtemplate", "altchunk", "control", "embeddedpackage", + "oleobject", "vbaproject", "vba", "macroenabled", +) +_PASSIVE_CONTENT_TYPE_PREFIXES = ( + "application/vnd.openxmlformats-officedocument.", + "application/vnd.openxmlformats-package.", +) +_PASSIVE_CONTENT_TYPES = frozenset({"application/xml", "text/xml"}) +_PASSIVE_IMAGE_CONTENT_TYPES = { + "image/bmp": frozenset({".bmp"}), + "image/gif": frozenset({".gif"}), + "image/jpeg": frozenset({".jpg", ".jpeg"}), + "image/png": frozenset({".png"}), + "image/tiff": frozenset({".tif", ".tiff"}), + "image/x-emf": frozenset({".emf"}), + "image/x-wmf": frozenset({".wmf"}), +} +_COMMON_RELATIONSHIPS = frozenset( + { + "core-properties", "custom-properties", "customxml", "customxmlprops", + "extended-properties", "image", "officeDocument", "thumbnail", "theme", + } +) +_FORMAT_RELATIONSHIPS = { + "docx": frozenset( + { + "chart", "comments", "commentsExtended", "commentsIds", "diagramColors", + "diagramData", "diagramLayout", "diagramQuickStyle", "endnotes", "fontTable", + "footer", "footnotes", "glossaryDocument", "header", "hyperlink", "numbering", + "people", "settings", "styles", "stylesWithEffects", "themeOverride", "webSettings", + } + ), + "pptx": frozenset( + { + "chart", "commentAuthors", "comments", "diagramColors", "diagramData", + "diagramLayout", "diagramQuickStyle", "handoutMaster", "hyperlink", "notesMaster", + "notesSlide", "presProps", "slide", "slideLayout", "slideMaster", "tableStyles", + "tags", "themeOverride", "viewProps", + } + ), + "xlsx": frozenset( + { + "calcChain", "chart", "chartsheet", "comments", "dialogsheet", "drawing", + "hyperlink", "person", "pivotCacheDefinition", "pivotCacheRecords", "pivotTable", + "sharedStrings", "styles", "table", "threadedComment", "worksheet", + } + ), +} + + +@dataclass(frozen=True, slots=True) +class OoxmlPackageFacts: + """Bounded structural facts safe to pass to a later format adapter.""" + + detected_format: str + entry_count: int + decompressed_bytes: int + + +class OoxmlSecurityFailure(Exception): + """Carry one bounded container-security outcome.""" + + def __init__(self, status: str, code: str) -> None: + super().__init__(code) + self.status = status + self.code = code + + +def _directory_facts(payload: bytes) -> tuple[int, int]: + try: + entries, directory_bytes = zip_directory_facts(BytesIO(payload)) + except zipfile.BadZipFile as exc: + message = str(exc) + code = "ooxml_invalid_directory" + if message == "multi-disk archive": + code = "ooxml_multidisk" + elif message == "zip64 archive is unsupported": + code = "ooxml_zip64" + raise OoxmlSecurityFailure("malformed", code) from exc + if entries > MAXIMUM_ENTRIES or directory_bytes > MAXIMUM_CENTRAL_DIRECTORY_BYTES: + raise OoxmlSecurityFailure("limit_exceeded", "ooxml_directory_limit") + return entries, directory_bytes + + +def _normalized_name(value: str) -> str: + if "\\" in value: + raise OoxmlSecurityFailure("malformed", "ooxml_invalid_path") + normalized = unicodedata.normalize("NFC", value) + path = PurePosixPath(normalized) + if ( + not normalized + or normalized.startswith("/") + or (path.parts and ":" in path.parts[0]) + or any(part in {"", ".", ".."} for part in path.parts) + ): + raise OoxmlSecurityFailure("malformed", "ooxml_invalid_path") + return normalized.rstrip("/") + + +def _part_is_allowed(name: str, *, detected_format: str) -> bool: + lower = name.casefold() + if lower == "[content_types].xml": + return True + if not ( + lower.startswith(_COMMON_PREFIXES) + or lower.startswith(f"{_FORMAT_ROOT[detected_format]}/") + ): + return False + return any(lower.endswith(suffix) for suffix in _PASSIVE_PART_SUFFIXES) + + +def _directory_is_allowed(name: str, *, detected_format: str) -> bool: + lower = f"{name.casefold().rstrip('/')}/" + allowed_roots = (*_COMMON_PREFIXES, f"{_FORMAT_ROOT[detected_format]}/") + return any(lower == root or lower.startswith(root) for root in allowed_roots) + + +def _has_active_metadata(value: str) -> bool: + folded = value.casefold() + return any(token in folded for token in _ACTIVE_METADATA_TOKENS) + + +def _relationship_target_is_external(value: str, *, relationship_part: str) -> bool: + target = value.strip() + if "\\" in target or "\x00" in target: + return True + parsed = urlsplit(target) + if parsed.scheme or parsed.netloc or target.startswith("/"): + return True + source_directory = ( + "" if relationship_part == "_rels/.rels" else relationship_part.rsplit("/_rels/", 1)[0] + ) + resolved = posixpath.normpath(posixpath.join(source_directory, parsed.path)) + return resolved == ".." or resolved.startswith("../") + + +def _content_type_is_passive( + value: str, + *, + part_name: str, + extension: str, +) -> bool: + folded = value.strip().casefold() + if not folded or _has_active_metadata(folded): + return False + if folded in _PASSIVE_IMAGE_CONTENT_TYPES: + declared_suffix = PurePosixPath(part_name.casefold()).suffix + default_suffix = f".{extension.casefold().lstrip('.')}" if extension else "" + suffix = declared_suffix or default_suffix + return suffix in _PASSIVE_IMAGE_CONTENT_TYPES[folded] + return folded in _PASSIVE_CONTENT_TYPES or folded.startswith( + _PASSIVE_CONTENT_TYPE_PREFIXES + ) + + +def _relationship_type_is_passive(value: str, *, detected_format: str) -> bool: + kind = value.rstrip("/").rsplit("/", 1)[-1] + return kind in (_COMMON_RELATIONSHIPS | _FORMAT_RELATIONSHIPS[detected_format]) + + +def validate_ooxml(payload: bytes, *, detected_format: str) -> OoxmlPackageFacts: + """Validate one exact classified OOXML package without extracting content.""" + if detected_format not in _FORMAT_ROOT: + raise OoxmlSecurityFailure("ambiguous", "ooxml_classification_conflict") + expected_entries, _ = _directory_facts(payload) + try: + archive = zipfile.ZipFile(BytesIO(payload)) + except (OSError, ValueError, zipfile.BadZipFile) as exc: + raise OoxmlSecurityFailure("malformed", "ooxml_invalid_directory") from exc + with archive: + infos = archive.infolist() + if len(infos) != expected_entries: + raise OoxmlSecurityFailure("malformed", "ooxml_directory_conflict") + names: dict[str, zipfile.ZipInfo] = {} + decompressed = 0 + for info in infos: + name = _normalized_name(info.filename) + folded = name.casefold() + if folded in names: + raise OoxmlSecurityFailure("malformed", "ooxml_duplicate_path") + names[folded] = info + lower = name.casefold() + mode = info.external_attr >> 16 + if info.flag_bits & 0x1: + raise OoxmlSecurityFailure("malformed", "ooxml_encrypted_entry") + if stat.S_IFMT(mode) not in {0, stat.S_IFREG, stat.S_IFDIR}: + raise OoxmlSecurityFailure("malformed", "ooxml_special_entry") + if info.is_dir(): + # Directory records are optional OPC metadata and may not impersonate parts. + if name.casefold() in { + marker.casefold() for markers in _REQUIRED.values() for marker in markers + }: + raise OoxmlSecurityFailure("malformed", "ooxml_required_marker_missing") + if not _directory_is_allowed(name, detected_format=detected_format): + raise OoxmlSecurityFailure("malformed", "ooxml_unknown_package_part") + continue + if ( + lower.endswith(_EXECUTABLE_SUFFIXES) + or "vbaproject" in lower + or "/embeddings/" in f"/{lower}" + ): + raise OoxmlSecurityFailure("malformed", "ooxml_active_content") + root = lower.split("/", 1)[0] + if root in set(_FORMAT_ROOT.values()) - {_FORMAT_ROOT[detected_format]}: + raise OoxmlSecurityFailure("ambiguous", "ooxml_classification_conflict") + if not _part_is_allowed(name, detected_format=detected_format): + raise OoxmlSecurityFailure("malformed", "ooxml_unknown_package_part") + decompressed += info.file_size + if decompressed > MAXIMUM_DECOMPRESSED_BYTES: + raise OoxmlSecurityFailure("limit_exceeded", "ooxml_decompressed_limit") + if info.file_size and ( + info.compress_size == 0 + or info.file_size > info.compress_size * MAXIMUM_COMPRESSION_RATIO + ): + raise OoxmlSecurityFailure("limit_exceeded", "ooxml_compression_ratio") + canonical_names = frozenset(names) + required = frozenset(name.casefold() for name in _REQUIRED[detected_format]) + if not required <= canonical_names: + raise OoxmlSecurityFailure("malformed", "ooxml_required_marker_missing") + other_markers = { + marker.casefold() + for format_name, markers in _REQUIRED.items() + if format_name != detected_format + for marker in markers - {"[Content_Types].xml", "_rels/.rels"} + } + if canonical_names & other_markers: + raise OoxmlSecurityFailure("ambiguous", "ooxml_classification_conflict") + + for name, info in names.items(): + if info.is_dir(): + continue + try: + body = archive.read(info) + except (OSError, RuntimeError, ValueError, zipfile.BadZipFile) as exc: + raise OoxmlSecurityFailure("malformed", "ooxml_entry_read_failed") from exc + if zipfile.is_zipfile(BytesIO(body)): + raise OoxmlSecurityFailure("malformed", "ooxml_nested_archive") + if not (name.endswith(".xml") or name.endswith(".rels")): + continue + if name.endswith(".rels") and len(body) > MAXIMUM_RELATIONSHIP_BYTES: + raise OoxmlSecurityFailure("limit_exceeded", "ooxml_relationship_limit") + if b"=0.29,<1.0", "botocore==1.43.0", "celery[redis]>=5.4,<6.0", + "defusedxml @ https://files.pythonhosted.org/packages/07/6c/aa3f2f849e01cb6a001cd8554a88d4c77c5c1a31c95bdf1cf9301e6d9ef4/defusedxml-0.7.1-py2.py3-none-any.whl#sha256=a352e7e428770286cc899e2542b6cdaedb2b4953ff269a210103ec58f6198a61", "fastapi>=0.115,<1.0", "httpx>=0.27,<1.0", "PyJWT[crypto]>=2.13,<3.0", diff --git a/backend/scripts/run_test_lanes.py b/backend/scripts/run_test_lanes.py index 43751f39..cba4b824 100644 --- a/backend/scripts/run_test_lanes.py +++ b/backend/scripts/run_test_lanes.py @@ -107,6 +107,7 @@ class TestLane: "tests/test_guide_formats.py", "tests/test_guide_extraction.py", "tests/test_guide_extractor_dependencies.py", + "tests/test_guide_ooxml.py", "tests/test_guide_pdf.py", "tests/test_local_artifact_store.py", "tests/test_s3_artifact_store.py", diff --git a/backend/tests/test_artifact_architecture.py b/backend/tests/test_artifact_architecture.py index ada5a685..ab7daff6 100644 --- a/backend/tests/test_artifact_architecture.py +++ b/backend/tests/test_artifact_architecture.py @@ -583,3 +583,26 @@ def test_pdf_parser_dependency_is_confined_to_the_format_adapter() -> None: ): adapter_importers.add(path.relative_to(APP_ROOT).as_posix()) assert adapter_importers == {"modules/artifacts/guide_extraction_worker.py"} + + +def test_ooxml_parser_dependency_and_adapter_are_confined_to_the_isolated_worker() -> None: + dependency_importers: set[str] = set() + adapter_importers: set[str] = set() + for path in APP_ROOT.rglob("*.py"): + relative = path.relative_to(APP_ROOT).as_posix() + for node in ast.walk(_tree(path)): + if isinstance(node, ast.Import): + if any(alias.name.split(".", 1)[0] == "defusedxml" for alias in node.names): + dependency_importers.add(relative) + if any( + alias.name == "app.modules.artifacts.guide_ooxml" for alias in node.names + ): + adapter_importers.add(relative) + elif isinstance(node, ast.ImportFrom) and node.module is not None: + if node.module.split(".", 1)[0] == "defusedxml": + dependency_importers.add(relative) + if node.module == "app.modules.artifacts.guide_ooxml": + adapter_importers.add(relative) + + assert dependency_importers == {"modules/artifacts/guide_ooxml.py"} + assert adapter_importers == {"modules/artifacts/guide_extraction_worker.py"} diff --git a/backend/tests/test_guide_extraction.py b/backend/tests/test_guide_extraction.py index 75ef9ea0..32dd954c 100644 --- a/backend/tests/test_guide_extraction.py +++ b/backend/tests/test_guide_extraction.py @@ -89,6 +89,16 @@ def __call__(self, *_args): worker_module._extract(b"\xff", "plain_text") +def test_ooxml_loader_maps_security_failures_without_activating_an_adapter() -> None: + validate = worker_module._load_ooxml_security() + with pytest.raises(worker_module.ExtractionFailure) as raised: + validate(b"not-a-package", "docx") + assert (raised.value.status, raised.value.code) == ( + "malformed", + "ooxml_invalid_directory", + ) + + @pytest.mark.parametrize( ("failure", "status", "error_code"), [ diff --git a/backend/tests/test_guide_ooxml.py b/backend/tests/test_guide_ooxml.py new file mode 100644 index 00000000..d34ae1a4 --- /dev/null +++ b/backend/tests/test_guide_ooxml.py @@ -0,0 +1,369 @@ +"""Focused proofs for the shared bounded OOXML security boundary.""" + +from __future__ import annotations + +from io import BytesIO +import stat +import zipfile + +import pytest + +import app.modules.artifacts.guide_ooxml as ooxml_module +from app.modules.artifacts.guide_formats import GuideFormatLimits, OOXML_REQUIRED_MARKERS +from app.modules.artifacts.guide_ooxml import OoxmlSecurityFailure, validate_ooxml + + +def _package( + *, + detected_format: str = "docx", + additions: dict[str, bytes] | None = None, + external: bool = False, +) -> bytes: + marker = {"docx": "word/document.xml", "pptx": "ppt/presentation.xml", "xlsx": "xl/workbook.xml"}[detected_format] + relationships = ( + b'' + if external + else b"" + ) + members = { + "[Content_Types].xml": b"", + "_rels/.rels": relationships, + marker: b"", + **(additions or {}), + } + output = BytesIO() + with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: + for name, body in members.items(): + archive.writestr(name, body) + return output.getvalue() + + +def _failure(payload: bytes, *, detected_format: str = "docx") -> tuple[str, str]: + with pytest.raises(OoxmlSecurityFailure) as raised: + validate_ooxml(payload, detected_format=detected_format) + return raised.value.status, raised.value.code + + +def test_ooxml_validator_inherits_classifier_markers_and_limits() -> None: + limits = GuideFormatLimits() + assert ooxml_module._REQUIRED is OOXML_REQUIRED_MARKERS + assert ooxml_module.MAXIMUM_ENTRIES == limits.maximum_entries + assert ( + ooxml_module.MAXIMUM_CENTRAL_DIRECTORY_BYTES + == limits.maximum_central_directory_bytes + ) + assert ooxml_module.MAXIMUM_DECOMPRESSED_BYTES == limits.maximum_decompressed_bytes + assert ooxml_module.MAXIMUM_COMPRESSION_RATIO == limits.maximum_compression_ratio + + +@pytest.mark.parametrize("detected_format", ["docx", "pptx", "xlsx"]) +def test_valid_exact_classified_packages_pass_without_extraction(detected_format: str) -> None: + result = validate_ooxml(_package(detected_format=detected_format), detected_format=detected_format) + assert result.detected_format == detected_format + assert result.entry_count == 3 + + +@pytest.mark.parametrize( + ("additions", "status", "code"), + [ + ({"../escape.xml": b""}, "malformed", "ooxml_invalid_path"), + ({"unknown/data.xml": b""}, "malformed", "ooxml_unknown_package_part"), + ({"word/embeddings/object.bin": b"x"}, "malformed", "ooxml_active_content"), + ({"word/nested.zip": b"PK\x05\x06" + b"\0" * 18}, "malformed", "ooxml_active_content"), + ({"word/bad.xml": b"]>&e;"}, "malformed", "ooxml_unsafe_xml"), + ({"word/bare_dtd.xml": b""}, "malformed", "ooxml_unsafe_xml"), + ], +) +def test_rejects_unsafe_members(additions, status: str, code: str) -> None: + with pytest.raises(OoxmlSecurityFailure) as raised: + validate_ooxml(_package(additions=additions), detected_format="docx") + assert (raised.value.status, raised.value.code) == (status, code) + + +def test_rejects_external_relationships() -> None: + with pytest.raises(OoxmlSecurityFailure) as raised: + validate_ooxml(_package(external=True), detected_format="docx") + assert raised.value.code == "ooxml_external_relationship" + + +@pytest.mark.parametrize("target", ["../evil.xml", "../../evil.xml"]) +def test_root_relationship_cannot_traverse_above_package(target: str) -> None: + relationships = ( + f'' + ).encode() + assert _failure(_package(additions={"_rels/.rels": relationships})) == ( + "malformed", + "ooxml_external_relationship", + ) + + +@pytest.mark.parametrize( + ("relationship", "expected_code"), + [ + ( + b'', + "ooxml_external_relationship", + ), + ( + b'', + "ooxml_active_content", + ), + ], +) +def test_rejects_implicit_external_and_active_relationship_metadata( + relationship: bytes, + expected_code: str, +) -> None: + assert _failure( + _package(additions={"word/_rels/document.xml.rels": relationship}) + ) == ("malformed", expected_code) + + +def test_rejects_active_content_type_under_a_passive_filename() -> None: + content_types = ( + b'' + ) + assert _failure( + _package(additions={"[Content_Types].xml": content_types, "word/payload.xml": b""}) + ) == ("malformed", "ooxml_active_content") + + +def test_rejects_svg_content_type_disguised_as_an_xml_part() -> None: + content_types = ( + b'' + ) + assert _failure( + _package( + additions={ + "[Content_Types].xml": content_types, + "word/media/active.xml": b"", + } + ) + ) == ("malformed", "ooxml_active_content") + + +@pytest.mark.parametrize("kind", ["aFChunk", "package", "externalLink"]) +def test_rejects_non_passive_relationship_types(kind: str) -> None: + relationship = ( + f'' + ).encode() + assert _failure( + _package(additions={"word/_rels/document.xml.rels": relationship}) + ) == ("malformed", "ooxml_active_content") + + +def test_utf16_dtd_is_rejected_by_parser_policy() -> None: + body = ''.encode("utf-16") + assert _failure(_package(additions={"word/utf16.xml": body})) == ( + "malformed", + "ooxml_unsafe_xml", + ) + + +@pytest.mark.parametrize( + ("name", "code"), + [ + ("/absolute.xml", "ooxml_invalid_path"), + ("C:/drive.xml", "ooxml_invalid_path"), + ("word\\backslash.xml", "ooxml_invalid_path"), + ("word/vbaProject.bin", "ooxml_active_content"), + ("word/run.js", "ooxml_active_content"), + ("word/run.sh", "ooxml_active_content"), + ("word/unknown.any", "ooxml_unknown_package_part"), + ("word/media/active.svg", "ooxml_unknown_package_part"), + ("ppt/slides/slide1.xml", "ooxml_classification_conflict"), + ], +) +def test_rejects_path_and_executable_variants(name: str, code: str) -> None: + expected_status = "ambiguous" if code == "ooxml_classification_conflict" else "malformed" + assert _failure(_package(additions={name: b""})) == (expected_status, code) + + +def test_rejects_case_collisions_and_special_entries() -> None: + collision = _package(additions={"word/A.xml": b"", "word/a.XML": b""}) + assert _failure(collision) == ("malformed", "ooxml_duplicate_path") + + output = BytesIO() + with zipfile.ZipFile(output, "w") as archive: + for name, body in { + "[Content_Types].xml": b"", + "_rels/.rels": b"", + "word/document.xml": b"", + }.items(): + archive.writestr(name, body) + link = zipfile.ZipInfo("word/link.xml") + link.create_system = 3 + link.external_attr = (stat.S_IFLNK | 0o777) << 16 + archive.writestr(link, b"target") + assert _failure(output.getvalue()) == ("malformed", "ooxml_special_entry") + + +def test_rejects_missing_and_conflicting_markers() -> None: + missing = _package() + with zipfile.ZipFile(BytesIO(missing), "r") as source: + members = {info.filename: source.read(info) for info in source.infolist()} + members.pop("word/document.xml") + output = BytesIO() + with zipfile.ZipFile(output, "w") as archive: + for name, body in members.items(): + archive.writestr(name, body) + assert _failure(output.getvalue()) == ("malformed", "ooxml_required_marker_missing") + conflict = _package(additions={"ppt/presentation.xml": b""}) + assert _failure(conflict) == ("ambiguous", "ooxml_classification_conflict") + assert _failure(_package(), detected_format="opaque") == ( + "ambiguous", + "ooxml_classification_conflict", + ) + + +def test_directory_cannot_impersonate_required_file_marker() -> None: + output = BytesIO() + with zipfile.ZipFile(output, "w") as archive: + archive.writestr("[Content_Types].xml", b"") + archive.writestr("_rels/.rels", b"") + archive.writestr("word/document.xml/", b"") + assert _failure(output.getvalue()) == ("malformed", "ooxml_required_marker_missing") + + +@pytest.mark.parametrize("directory", ["unknown/", "ppt/"]) +def test_unknown_and_cross_format_directory_roots_fail_closed(directory: str) -> None: + assert _failure(_package(additions={directory: b""})) == ( + "malformed", + "ooxml_unknown_package_part", + ) + + +def test_rejects_nested_archive_magic_without_archive_suffix() -> None: + nested = _package(additions={"word/inner.xml": b""}) + sfx = _package(additions={"word/media/blob.xml": b"MZstub" + nested}) + assert _failure(sfx) == ("malformed", "ooxml_nested_archive") + + +def test_rejects_bounded_size_ratio_and_relationship_limits( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(ooxml_module, "MAXIMUM_DECOMPRESSED_BYTES", 8) + assert _failure(_package()) == ("limit_exceeded", "ooxml_decompressed_limit") + monkeypatch.setattr(ooxml_module, "MAXIMUM_DECOMPRESSED_BYTES", 128 * 1024 * 1024) + monkeypatch.setattr(ooxml_module, "MAXIMUM_COMPRESSION_RATIO", 1) + assert _failure(_package(additions={"word/large.xml": b"x" * 1_000})) == ( + "limit_exceeded", + "ooxml_compression_ratio", + ) + monkeypatch.setattr(ooxml_module, "MAXIMUM_COMPRESSION_RATIO", 100) + monkeypatch.setattr(ooxml_module, "MAXIMUM_RELATIONSHIP_BYTES", 4) + assert _failure(_package()) == ("limit_exceeded", "ooxml_relationship_limit") + + +def test_rejects_invalid_directory_multidisk_and_zip64() -> None: + assert _failure(b"not-a-zip") == ("malformed", "ooxml_invalid_directory") + payload = bytearray(_package()) + marker = payload.rfind(b"PK\x05\x06") + payload[marker + 4 : marker + 8] = b"\x01\x00\x00\x00" + assert _failure(bytes(payload)) == ("malformed", "ooxml_multidisk") + payload = bytearray(_package()) + marker = payload.rfind(b"PK\x05\x06") + payload[marker + 10 : marker + 12] = b"\xff\xff" + assert _failure(bytes(payload)) == ("malformed", "ooxml_zip64") + + +def test_entry_boundary_is_exact_2000_and_2001() -> None: + accepted = {f"word/items/{index}.xml": b"" for index in range(1_997)} + result = validate_ooxml(_package(additions=accepted), detected_format="docx") + assert result.entry_count == 2_000 + rejected = {**accepted, "word/items/over.xml": b""} + assert _failure(_package(additions=rejected)) == ( + "limit_exceeded", + "ooxml_directory_limit", + ) + + +def test_exact_and_one_over_decompressed_and_directory_bounds( + monkeypatch: pytest.MonkeyPatch, +) -> None: + payload = _package() + baseline = validate_ooxml(payload, detected_format="docx") + monkeypatch.setattr(ooxml_module, "MAXIMUM_DECOMPRESSED_BYTES", baseline.decompressed_bytes) + assert validate_ooxml(payload, detected_format="docx") == baseline + monkeypatch.setattr( + ooxml_module, "MAXIMUM_DECOMPRESSED_BYTES", baseline.decompressed_bytes - 1 + ) + assert _failure(payload) == ("limit_exceeded", "ooxml_decompressed_limit") + + monkeypatch.setattr(ooxml_module, "MAXIMUM_DECOMPRESSED_BYTES", 128 * 1024 * 1024) + marker = payload.rfind(b"PK\x05\x06") + directory_bytes = int.from_bytes(payload[marker + 12 : marker + 16], "little") + monkeypatch.setattr(ooxml_module, "MAXIMUM_CENTRAL_DIRECTORY_BYTES", directory_bytes) + validate_ooxml(payload, detected_format="docx") + monkeypatch.setattr(ooxml_module, "MAXIMUM_CENTRAL_DIRECTORY_BYTES", directory_bytes - 1) + assert _failure(payload) == ("limit_exceeded", "ooxml_directory_limit") + + +def test_encrypted_central_directory_flag_rejects_before_body_read() -> None: + payload = bytearray(_package()) + marker = payload.find(b"PK\x01\x02") + flags = int.from_bytes(payload[marker + 8 : marker + 10], "little") | 0x1 + payload[marker + 8 : marker + 10] = flags.to_bytes(2, "little") + assert _failure(bytes(payload)) == ("malformed", "ooxml_encrypted_entry") + + +def test_all_metadata_rejections_happen_before_any_body_read( + monkeypatch: pytest.MonkeyPatch, +) -> None: + encrypted = bytearray(_package()) + marker = encrypted.find(b"PK\x01\x02") + flags = int.from_bytes(encrypted[marker + 8 : marker + 10], "little") | 0x1 + encrypted[marker + 8 : marker + 10] = flags.to_bytes(2, "little") + + special = BytesIO() + with zipfile.ZipFile(special, "w") as archive: + archive.writestr("[Content_Types].xml", b"") + archive.writestr("_rels/.rels", b"") + archive.writestr("word/document.xml", b"") + link = zipfile.ZipInfo("word/link.xml") + link.create_system = 3 + link.external_attr = (stat.S_IFLNK | 0o777) << 16 + archive.writestr(link, b"target") + + payloads = [ + bytes(encrypted), + _package(additions={"word/A.xml": b"", "word/a.XML": b""}), + _package(additions={"../escape.xml": b""}), + _package(additions={"unknown/data.xml": b""}), + _package(additions={"ppt/presentation.xml": b""}), + special.getvalue(), + _package(additions={"word/run.sh": b"echo unsafe"}), + ] + reads = 0 + original_read = zipfile.ZipFile.read + + def observed_read(self, *args, **kwargs): + nonlocal reads + reads += 1 + return original_read(self, *args, **kwargs) + + monkeypatch.setattr(zipfile.ZipFile, "read", observed_read) + for payload in payloads: + with pytest.raises(OoxmlSecurityFailure): + validate_ooxml(payload, detected_format="docx") + assert reads == 0 + + +def test_directory_inventory_conflict_is_rejected() -> None: + payload = bytearray(_package()) + marker = payload.rfind(b"PK\x05\x06") + payload[marker + 10 : marker + 12] = (2).to_bytes(2, "little") + assert _failure(bytes(payload)) == ("malformed", "ooxml_directory_conflict") + + +def test_corrupt_local_member_header_has_bounded_failure() -> None: + payload = bytearray(_package()) + marker = payload.find(b"PK\x03\x04") + name_length = int.from_bytes(payload[marker + 26 : marker + 28], "little") + assert name_length > 0 + payload[marker + 30] ^= 1 + assert _failure(bytes(payload)) == ("malformed", "ooxml_entry_read_failed") diff --git a/backend/uv.lock b/backend/uv.lock index 381460e5..12e5e584 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -730,6 +730,14 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/aa/50/a9caea39ad19c431c1a3f8a31114df65b260cdfe67786b6c7e7c040c4c44/cryptography-49.0.0-pp311-pypy311_pp73-win_amd64.whl", hash = "sha256:be9fcb48a55f023493482827d4f459bd263cc20efde64f204b97c123201850c6", size = 3783731, upload-time = "2026-06-12T20:02:43.319Z" }, ] +[[package]] +name = "defusedxml" +version = "0.7.1" +source = { url = "https://files.pythonhosted.org/packages/07/6c/aa3f2f849e01cb6a001cd8554a88d4c77c5c1a31c95bdf1cf9301e6d9ef4/defusedxml-0.7.1-py2.py3-none-any.whl" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/07/6c/aa3f2f849e01cb6a001cd8554a88d4c77c5c1a31c95bdf1cf9301e6d9ef4/defusedxml-0.7.1-py2.py3-none-any.whl", hash = "sha256:a352e7e428770286cc899e2542b6cdaedb2b4953ff269a210103ec58f6198a61" }, +] + [[package]] name = "distro" version = "1.9.0" @@ -2654,6 +2662,7 @@ dependencies = [ { name = "asyncpg" }, { name = "botocore" }, { name = "celery", extra = ["redis"] }, + { name = "defusedxml" }, { name = "fastapi" }, { name = "httpx" }, { name = "pydantic-settings" }, @@ -2683,6 +2692,7 @@ requires-dist = [ { name = "asyncpg", specifier = ">=0.29,<1.0" }, { name = "botocore", specifier = "==1.43.0" }, { name = "celery", extras = ["redis"], specifier = ">=5.4,<6.0" }, + { name = "defusedxml", url = "https://files.pythonhosted.org/packages/07/6c/aa3f2f849e01cb6a001cd8554a88d4c77c5c1a31c95bdf1cf9301e6d9ef4/defusedxml-0.7.1-py2.py3-none-any.whl" }, { name = "docstr-coverage", marker = "extra == 'dev'", specifier = ">=2.3,<3.0" }, { name = "fastapi", specifier = ">=0.115,<1.0" }, { name = "httpx", specifier = ">=0.27,<1.0" }, diff --git a/docs/spec_artifact_storage_service.md b/docs/spec_artifact_storage_service.md index d1dab97b..baf087ea 100644 --- a/docs/spec_artifact_storage_service.md +++ b/docs/spec_artifact_storage_service.md @@ -1274,6 +1274,21 @@ format-scoped approved imports without changing the baseline silently. explicitly prohibited parser set across runtime, optional, and dependency-group declarations. +The shared OOXML security boundary is validation-only and does not extract +DOCX, PPTX, or XLSX content. For an exact server classification it permits +`[Content_Types].xml`, passive XML/relationship parts under `_rels/`, +`docProps/`, and `customXml/`, and passive XML, relationship, image, or font +parts under only the matching `word/`, `ppt/`, or `xl/` format root. Other +format roots and arbitrary extensions fail closed. It inspects EOCD and +central-directory metadata before reading any body; then it rejects unsafe +XML, external or absolute relationship targets, active content-type and +relationship metadata, nested archives (including prefixed archives), macros, +embedded objects, executable-capable parts, unknown parts, and conflicting +format markers. Directory entries cannot satisfy required file markers. Fixed limits are 2,000 +entries, 8 MiB central-directory metadata, 128 MiB decompressed bytes, 100:1 +compression ratio, and 1 MiB per relationships part, within the existing child +CPU, wall-time, memory, descriptor, and process limits. + For 03B3A the isolation contract is descriptor-only parsing after trusted imports, enforced by a default-deny Linux libseccomp profile with an explicit syscall allowlist plus fixed resource limits: 32 MiB input, 4 MiB From 45132ffc0bfe6feed04e3c88caed830a641763d4 Mon Sep 17 00:00:00 2001 From: Abiorh001 Date: Fri, 31 Jul 2026 13:01:45 +0100 Subject: [PATCH 2/2] fix(artifacts): close root relationship traversal gap --- .../REVIEW_LOG.md | 3 ++ ...RT-001-03B3B3A-external-review-response.md | 33 +++++++++++++++++++ .../WS-ART-001-03B3B3A-pr-trust-bundle.md | 7 ++-- backend/app/modules/artifacts/guide_ooxml.py | 13 ++++++-- backend/tests/test_guide_ooxml.py | 8 +++++ 5 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-external-review-response.md diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md index 26804e34..185b546a 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md @@ -280,3 +280,6 @@ - Architecture, security, QA, senior engineering, product/ops, reuse, CI, test-delta, and docs review tracks pass after repair. Hosted GitHub and CodeRabbit evidence remain the publication gate. +- CodeRabbit's one valid external finding closes non-canonical root + relationship parts and fails closed on malformed relationship-part shapes; + focused security and test-delta re-review cover the repair. diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-external-review-response.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-external-review-response.md new file mode 100644 index 00000000..0f225f01 --- /dev/null +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-external-review-response.md @@ -0,0 +1,33 @@ +# WS-ART-001-03B3B3A External Review Response + +## Comments addressed + +- CodeRabbit correctly found that non-canonical root relationship parts such as + `_rels/foo.rels` could use an incorrect relationship base. Root package + relationships are now limited to `_rels/.rels`; nested root `_rels/` + directories are rejected; and relationship target resolution fails closed + for every relationship-part shape other than `_rels/.rels` or + `/_rels/.rels`. +- Focused regressions cover non-canonical root relationship files/directories + and existing root traversal cases. + +## Comments deferred + +- CodeRabbit's generic docstring warning is superseded by the repository-owned + hosted docstring gate, which passed on the reviewed head. + +## Human decisions needed + +None. + +## Commands rerun + +- focused OOXML security and coverage suite; +- Ruff; +- dependency, lock, lane, stale-contract, Markdown-link, and diff gates; +- hosted Backend and Agent Gates on the repaired head. + +## Remaining risks + +The complete hosted rerun and exact-head human approval remain publication +gates. No OOXML adapter or AUTH action is activated by this repair. diff --git a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md index 6f2e2e27..510b1f2c 100644 --- a/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md +++ b/.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3A-pr-trust-bundle.md @@ -79,8 +79,11 @@ were repaired and re-reviewed. ## External review -Pending CodeRabbit and hosted GitHub Backend/Agent Gates on the published PR -head. +CodeRabbit found one valid non-canonical root-relationship base issue. Root +relationships are now restricted to `_rels/.rels`, malformed relationship-part +shapes fail closed, and focused regressions cover the repair. Its generic +docstring warning is superseded by the passing repository-owned hosted gate. +Hosted GitHub Backend/Agent Gates must pass again on the repaired PR head. ## Remaining risks and follow-up diff --git a/backend/app/modules/artifacts/guide_ooxml.py b/backend/app/modules/artifacts/guide_ooxml.py index 87c1c18c..aa0a1548 100644 --- a/backend/app/modules/artifacts/guide_ooxml.py +++ b/backend/app/modules/artifacts/guide_ooxml.py @@ -141,6 +141,8 @@ def _part_is_allowed(name: str, *, detected_format: str) -> bool: lower = name.casefold() if lower == "[content_types].xml": return True + if lower.startswith("_rels/") and lower != "_rels/.rels": + return False if not ( lower.startswith(_COMMON_PREFIXES) or lower.startswith(f"{_FORMAT_ROOT[detected_format]}/") @@ -151,6 +153,8 @@ def _part_is_allowed(name: str, *, detected_format: str) -> bool: def _directory_is_allowed(name: str, *, detected_format: str) -> bool: lower = f"{name.casefold().rstrip('/')}/" + if lower.startswith("_rels/"): + return lower == "_rels/" allowed_roots = (*_COMMON_PREFIXES, f"{_FORMAT_ROOT[detected_format]}/") return any(lower == root or lower.startswith(root) for root in allowed_roots) @@ -167,9 +171,12 @@ def _relationship_target_is_external(value: str, *, relationship_part: str) -> b parsed = urlsplit(target) if parsed.scheme or parsed.netloc or target.startswith("/"): return True - source_directory = ( - "" if relationship_part == "_rels/.rels" else relationship_part.rsplit("/_rels/", 1)[0] - ) + if relationship_part == "_rels/.rels": + source_directory = "" + elif "/_rels/" in relationship_part: + source_directory = relationship_part.rsplit("/_rels/", 1)[0] + else: + return True resolved = posixpath.normpath(posixpath.join(source_directory, parsed.path)) return resolved == ".." or resolved.startswith("../") diff --git a/backend/tests/test_guide_ooxml.py b/backend/tests/test_guide_ooxml.py index d34ae1a4..5eda72ba 100644 --- a/backend/tests/test_guide_ooxml.py +++ b/backend/tests/test_guide_ooxml.py @@ -237,6 +237,14 @@ def test_unknown_and_cross_format_directory_roots_fail_closed(directory: str) -> ) +@pytest.mark.parametrize("name", ["_rels/foo.rels", "_rels/nested/"]) +def test_noncanonical_root_relationship_parts_fail_closed(name: str) -> None: + assert _failure(_package(additions={name: b""})) == ( + "malformed", + "ooxml_unknown_package_part", + ) + + def test_rejects_nested_archive_magic_without_archive_suffix() -> None: nested = _package(additions={"word/inner.xml": b""}) sfx = _package(additions={"word/media/blob.xml": b"MZstub" + nested})