Repository navigation
Conversation
#732, slice 2) The attachment inventory from slice 1 (#893) named a docx image only by its relationship id. The id is now resolved through word/_rels/document.xml.rels (ECMA-376 Part 2 §9.3, Annex A.3) to the package part holding the bytes, and that part is streamed through SHA-256. Linked images, absent parts and Confluence images keep part/digest/size empty; state stays unsupported until a later slice stores the bytes. Malformed relationships fail before any write, like the main part. Refs #732 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYr4VjkzmxX2THRjfx7
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 285257e0ae
ℹ️ 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".
| f"{path} relationships part is {info.file_size} bytes, " | ||
| f"exceeds {max_bytes}-byte cap" | ||
| ) | ||
| raw = archive.read(info) |
There was a problem hiding this comment.
Wrap relationships-member read failures in DocumentReadError
When a DOCX has a readable main document but a corrupted relationships member, archive.read(info) can raise zipfile.BadZipFile directly (reproduced with a CRC mismatch). _open() only wraps failures while opening the container, so this exception escapes ingest_document.handler, which catches only DocumentReadError and DocumentParseError, instead of returning the expected document_read_failed result. Wrap member-read failures in DocumentReadError, as the existing main-part reader does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmé et corrigé dans 94f9948 (fixup de 285257e). archive.read(info) sur word/_rels/document.xml.rels est désormais enveloppé : BadZipFile/OSError/EOFError deviennent DocumentReadError, ce qui donne document_read_failed au handler. Nouveau test test_a_relationships_part_whose_crc_fails_is_refused : il échoue avec zipfile.BadZipFile: Bad CRC-32 avant le correctif et passe après.
Generated by Claude Code
…and size (#732, slice 2)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94f9948963
ℹ️ 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".
| except (zipfile.BadZipFile, OSError, EOFError) as exc: | ||
| raise DocumentReadError(f"cannot read {part} in {path}: {exc}") from exc |
There was a problem hiding this comment.
Wrap decompression errors in DocumentReadError
When a DOCX has a readable main document but an invalid DEFLATE block in a referenced image, member.read() raises zlib.error, which this exception handler does not catch. Unlike the previously reported CRC mismatch, which is now covered, corrupting a compressed block reproduces an uncaught decompression exception. This escapes ingest_document.handler instead of returning document_read_failed, since that handler catches only DocumentReadError and DocumentParseError. Catch and wrap zlib.error here and in read_docx_relationships_xml, whose member-read exception handler has the same gap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmé et corrigé dans 580a9ea (fixup de 285257e), à la racine plutôt que cas par cas : les deux lectures de membre passent maintenant par _stream_member, qui convertit en DocumentReadError tout ce que zipfile peut lever en lisant ou décompressant un membre (BadZipFile, EOFError, zlib.error, lzma.LZMAError, NotImplementedError pour une méthode non supportée, RuntimeError pour un membre chiffré, OSError). Nouveau test paramétré test_a_member_with_a_corrupt_deflate_stream_is_refused (image et part de relations) : il échoue avec zlib.error: invalid block type avant le correctif et passe après.
Generated by Claude Code
…and size (#732, slice 2)
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 580a9ea646
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| with _open(path) as archive: | ||
| names = set(archive.namelist()) | ||
| for part in sorted(set(parts) & names): | ||
| found[part] = _digest_member(archive, part, path) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Security: Cap decompressed media before hashing
When a DOCX from an untrusted author is ingested, every referenced ZIP member is streamed to EOF here without a per-member or aggregate decompressed-byte limit. A 522,508-byte proof archive with one 512 MiB media member was accepted and fully hashed; multiple or LZMA members can scale the work further. Streaming bounds RAM and the batch semaphore limits concurrent ingests, but neither bounds CPU, and caller cancellation leaves the worker running. Reject oversized ZipInfo.file_size values and enforce a cumulative byte budget while streaming.
SECURITY.md reference: SECURITY.md:L5-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Constat confirmé par lecture du code ; décision du propriétaire nécessaire avant tout push.
Ce que j'ai tracé. ZipExtFile lit exactement ZipInfo.file_size octets décompressés (au-delà, le flux est tronqué et le CRC vérifié). La somme des file_size des parts référencées borne donc le travail de digest_docx_parts. Aujourd'hui, aucune borne ne s'applique à cette somme. La mémoire reste bornée (lecture par blocs de 64 KiB), mais le temps CPU croît avec la taille déclarée. La cause tient donc en une ligne : il faut une borne sur sum(info.file_size), vérifiée avant de lire, plus le refus d'un membre dont file_size dépasse le reste du budget.
Pourquoi je ne pousse pas. Le correctif exige une constante, et la règle du dépôt (« no source, no implementation », # source: sur chaque constante) m'interdit d'en inventer une. Je n'ai pas pu lire la limite documentée de Word : support.microsoft.com et learn.microsoft.com sont bloqués par le réseau de cet environnement.
À trancher (cdeust) :
- Réutiliser le plafond d'ADR-0516 (
MAX_DOCUMENT_BYTES, 1 MiB) par part média. C'est sourcé, mais une capture d'écran de plus de 1 MiB resterait non identifiée, avec un état explicite à définir (failedou champs vides). - Fixer un budget cumulé par document (par exemple la limite de taille totale documentée par Microsoft pour un document Word avec images), enregistré dans un ADR comme source. Je l'implémente dès que la valeur et sa source sont posées.
- Accepter le risque dans le périmètre de SECURITY.md (l'outil n'ingère qu'un chemin local que l'utilisateur fournit lui-même) et clore ce fil avec cette justification.
Generated by Claude Code
|
CI autopilot : les deux échecs Reste ouvert : le fil de sécurité sur le plafond des parts média, qui attend la décision du propriétaire. Generated by Claude Code |
|
Claude finished @cdeust's task in 2m 13s —— View job Review 1 of 3 of PR #961: approve, no Blockers
The posted review lists three Notes that need no action before merge:
I could not run lint, the craftsmanship gate or the tests, because those commands need an approval this job does not have. The CI Verdict: approve |
Review 1 of 3: PR #961 (head
|
…and size (#732, slice 2) fixup! feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) unit (run 42750861159): test_every_storage_primitive_is_an_audited_site read archive.open(name) as open-write, the first positional argument of an open on an unresolved receiver being taken as its mode. The member is opened for reading: say so with mode="r".
|
CI autopilot :
Generated by Claude Code |
|
Claude finished @cdeust's task in 1m 54s —— View job Review 2 of 3: PR #961, approve with no Blockers
The commit changes nothing at runtime, because The media-size cap thread still waits on the owner's decision. Verdict: approve |
Review 2 of 3: PR #961 (head
|
…and size (#732, slice 2) unit (run 42755857850): the mutation gate (ADR-1122) left 12 survivors and found no test importing ingest_document_media. - docx_relationships: the all-'..' guard was redundant (popping word/ already yields ''), so it is now 'if not named'. Id and Target are read without a default and a relationship missing either is skipped; a missing-Target test pins it. - docx_media_reader: the tests assert that the docx path appears in read errors, and that every read asks for at most 64 KiB. mode="r" (the ZipFile.open default) and 'UTF-8' vs 'utf-8' are registered equivalents. - ingest_document_media: the lookup has no default; tests_py/handlers/test_ingest_document_media.py covers it directly.
|
CI autopilot :
Generated by Claude Code |
|
Mise à jour (CI autopilot) : dépassé. Le conflit a été résolu par Generated by Claude Code |
Co-Authored-By: Claude Fable 5.2 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYsWnKSjpX4oNV19PCF Signed-off-by: Claude <[email protected]>
|
Claude finished @cdeust's task in 2m 26s —— View job Review 3 of 3: PR #961, approve with no Blockers
All of the PR is accepted, and no file or commit carries an open Blocker. The review lists three Notes that need no action before merge:
I could not run lint, the craftsmanship gate, pytest or mutmut here, so the reviewers traced the code by reading it. The Verdict: approve |
Review 3 of 3: PR #961 (head
|
|
CI autopilot : toute la CI de L'autopilot ne peut pas le résoudre : le push avec lease d'un rebase est refusé par le classifieur de l'environnement, et la routine interdit Generated by Claude Code |
|
Continued in #978. Generated by Claude Code |
Summary
Refs #732 (tranche 2 ; l'issue reste ouverte).
Cause racine.
La tranche 1 (#893) inventoriait chaque image embarquée avec son id de relation docx (
r:embed/r:id), sans résoudre cet id vers la part du paquet qui porte les octets. Rien ne permettait donc d'identifier les octets d'une image, prérequis de leur conservation (critère « bytes, digest, position and document version »).core/docx_relationships.py(pur) :word/_rels/document.xml.rels→{id: part}pour chaque relation interne, cible résolue selon ECMA-376 Part 2 Annex A.3 (relative àword/, absolue depuis la racine, segments./..). Les ciblesTargetMode="External"et vides sont absentes. XML malformé →DocumentParseError.infrastructure/docx_media_reader.py: lit la part de relations (plafondMAX_DOCUMENT_BYTESd'ADR-0516, UTF-8) et passe chaque part nommée en SHA-256 par blocs de 64 KiB (mémoire bornée).handlers/ingest_document_media.py: composition ; chaque attachment résolu reçoitpart,digest(sha256:<hex>),size. Image liée, part absente, image Confluence : champs vides ("","",null).stateresteunsupported: les octets sont identifiés, pas conservés.docs/module-inventory.mdet CHANGELOG mis à jour.Type of change
Test plan
pytest tests_py/core/test_docx_relationships.py tests_py/infrastructure/test_docx_media_reader.py tests_py/handlers/test_ingest_document*.py+ tous les tests qui référencentingest_document/DocumentAttachment/module-inventory: 178 passed, 3 skipped.pytest tests_py/core/+ tests document : 3961 passed, 162 skipped, 1 échec préexistant surmain(test_wiki_coverage_dashboard.py::test_unreadable_page_is_skipped_not_fatal, environnement root).ruff check/ruff format --check(0.17.3) sur les fichiers touchés : OK.python scripts/check_craftsmanship.py: OK.Mutations visées par les nouveaux tests : résolution
..au-dessus de la racine, cible vide ou faite de seuls segments de point,TargetModeexterne vs interne, relation sansId, élément hors espace de noms OPC, plafond exact (max_byteségal / dépassé), lecture par blocs au-delà de 64 KiB, part vide (taille 0), CRC invalide, part absente du zip.Audit notes
Non vérifié.
scripts/mutation_check.shn'a pas pu tourner en local (téléchargement detorchrefusé par le proxy de l'environnement).unitcomplet de la CI non rejoué.Tranches restantes de #732 (hors de cette PR).
preserved-only) avec une référence de source accessible, côté stockage.extractedetfailedexplicites.https://claude.ai/code/session_018kyZYr4VjkzmxX2THRjfx7
Generated by Claude Code