Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,22 @@
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.
- 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.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
`<directory>/_rels/<part>.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.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
# 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

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

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.
13 changes: 13 additions & 0 deletions backend/app/modules/artifacts/guide_extraction_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""
Expand Down
52 changes: 27 additions & 25 deletions backend/app/modules/artifacts/guide_formats.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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 (
Expand Down
Loading
Loading