Skip to content

Route Markdown conversion through ZIP admission - #62

Merged
M3gA-Mind merged 2 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/sr001-convert-admission
Nov 11, 2026
Merged

M3gA-Mind merged 2 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/sr001-convert-admission

Conversation

@M3gA-Mind

Copy link
Copy Markdown
Collaborator

Closes #61.

markdown::convert opened Office archives with a plain ZipArchive::new, so a document that intake::extract refuses was read in full. That covered a part over 8 MiB, more than 2048 members, and an oversized or ambiguous central directory. Archives now open through intake's zip_preflight and admitted reader, the same path extract uses. Each declared part is capped at intake's 8 MiB, and the 64 MiB aggregate cap stays.

Behaviour changes:

  • convert refuses archives that intake's admission refuses, and any part declared over 8 MiB.
  • Encrypted unrelated members are still skipped rather than refused, so a deck with encrypted notes keeps its slides. This is pinned by the existing encrypted_unrelated_parts_do_not_hide_valid_slides.
  • XLSX gets the same preflight before calamine opens it. calamine still opens the bytes with its own reader.

Public API: none. MAX_DECOMPRESSED_BYTES is unchanged.

Tests (convert path):

  • a 12 MiB part: refused (fails on main);
  • 3000 members: refused (fails on main);
  • a nested DOCX behind a method-99 primary record: never yields the nested text. This is a guard; it passes on main too.

Validation (our Linux CI runner):

  • On 53224a48dc: clippy clean, 227 passed.
  • Red control with main's ooxml.rs and intake/mod.rs: the 12 MiB-part and 3000-member tests fail ("unwrap_err on an Ok value"), and the nested guard passes.
  • On the formatted head 8adfa5c: fmt clean. Merged with the dependency batch: fmt, clippy and --locked tests pass, 227 passed.
  • Merged with the hayro-free batch 3ff26dd: fmt, clippy and cargo test --locked --all-features all pass (227 passed, 0 failed), and cargo +1.88 check --locked --all-targets --all-features passes.

Branch: M3gA-Mind/tinydocs fix/sr001-convert-admission @ 8adfa5c (pushed).

markdown::convert opened Office archives with a plain ZipArchive, so a
document that intake::extract refuses (a part over 8 MiB, more than 2048
members, an oversized or ambiguous central directory) was read in full.
Open them through intake's preflight and admitted reader, and cap each
declared part at intake's 8 MiB as well as the 64 MiB aggregate.

Encrypted unrelated members stay skipped rather than refused, as before.

Closes tinyhumansai#61
@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 35e100fe-3d7a-471d-8ded-7f9e68d7230b
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@tinysweeper

tinysweeper Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Reviewed head: f2f0050c91a7ac610bcc01505299470dea2c5253

Findings

Previously reported and still active

  • Assert the error case in the nested-archive test instead of silently skipping

Advisory notes

  • high: Assert the error case in the nested-archive test instead of silently skipping (src/markdown/mod_tests.rs:459)
    Problem: The test's name promises `never_yields_its_text_when_the_primary_directory_is_invalid`, but the only assertion path taken is a successful `convert(...).unwrap()`. If `convert` star
    Suggested fix:
    No concrete fix was supplied.
  • high: Assert the error case in the nested-archive test instead of silently skipping ((pull request description))
    Problem: This test only proves the nested text is absent when conversion *succeeds* — any refusal makes `unwrap()` panic, but only on success is the nested-archive guard actually exercised,
    Suggested fix:
    No concrete fix was supplied.

Verdict: Ready for maintainer review · Priority: none
Coverage: Reviewed 4 of 4 files.

Diagnostics and supporting details

Full summary

Pull request purpose (title): Route Markdown conversion through ZIP admission. This change closes #61 by making `markdown::convert` open Office archives through intake's bounded ZIP admission instead of a plain `ZipArchive::new`, which previously let a document that `intake::extract` refuses be read in full. Word/PowerPoint OOXML readers now open archives via a new `pub(crate)` `admitted_archive` helper that runs intake's `zip_preflight` and admitted reader; each declared part is capped at intake's 8 MiB alongside the existing 64 MiB aggregate cap; XLSX gets the same preflight before calamine opens the bytes with its own reader; and encrypted unrelated members are still skipped rather than refused. There is no public API change and `MAX_DECOMPRESSED_BYTES` is unchanged. Three convert-path tests are added, and the nested-archive guard has been rewritten in this push to use `convert(...).unwrap()` and assert both that the outer text is present and that the nested secret is absent. The critique, commits and description lanes each report one prior finding still open on the nested-archive test. Separately, the latest-push delta is unavailable — the review state lacks a trusted base-to-head diff comparison, so nothing here is presented as a push delta.

Review snapshot

Change surface Files Review signal Count
Production 2 Active findings 1
Tests 1 Noted findings 2
Documentation 1 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

`markdown::convert` now routes Office archive opening through intake's admission path. In `src/intake/mod.rs`, `MAX_PART` (8 MiB) is widened to `pub(crate)` and a new markdown-gated `pub(crate) fn admitted_archive` runs `zip_admission::zip_preflight` then `zip_admission::open_admitted_zip`, the same sequence `extract` uses. In `src/markdown/ooxml.rs`, `ooxml::open` calls `admitted_archive` rather than `ZipArchive::new`, and its declared-size check is generalized to `declared_sizes`, which returns both the largest and the summed declared uncompressed sizes from the central directory and refuses the archive when the largest exceeds `MAX_PART` or the total exceeds `MAX_DECOMPRESSED_BYTES`. The per-entry streaming cap in `read_parts` moves from `MAX_DECOMPRESSED_BYTES` to `MAX_PART`. `src/markdown/README.md` documents the ordered admission sequence, the 8 MiB per-member cap and the skip-rather-than-refuse handling of encrypted members. Three tests are added to `src/markdown/mod_tests.rs`, including a nested-archive guard that is now written to unwrap the outer conversion and assert both the presence of the outer text and the absence of the nested secret. One prior critique finding on that nested test remains open. Separately: the latest-push delta is unavailable — the review state lacks a trusted base-to-head diff comparison.

Features

  • Modified — Markdown OOXML archives open through intake's bounded ZIP admission: Word and PowerPoint conversion passes untrusted archives through the same preflight and admitted reader that `intake::extract` uses, so a document intake refuses can no longer be read in full by the Markdown path; central-directory size, member count, name handling and no-fallback-to-embedded-footers all apply. (src/intake/mod.rs, src/markdown/ooxml.rs)
  • Added — `admitted_archive` helper reusing intake's admission: Provides a `pub(crate)`, markdown-feature-gated entry point that runs `zip_preflight` and returns the admitted `ZipArchive`, so other readers of untrusted archives cannot fall back to an unadmitted directory. (src/intake/mod.rs)
  • Modified — Per-entry streaming read cap narrowed to `MAX_PART`: `read_parts` caps each streamed XML member at 8 MiB instead of the 64 MiB aggregate limit, so an archive that declares small sizes but streams more cannot force an unbounded allocation. (src/markdown/ooxml.rs fn read_parts<R: Read + Seek>()
  • Internal refactor — `MAX_PART` widened to `pub(crate)`: Exposes the existing 8 MiB per-member constant to the Markdown reader without changing its value or any public API surface. (src/intake/mod.rs)
  • Modified — Markdown documentation describes the new admission guarantees: Readers are told that archives first pass the same bounded ZIP admission as intake, then are admitted only if no member declares more than 8 MiB and the aggregate declared expansion is at most 64 MiB, and that encrypted members are skipped rather than refused, so a deck with encrypted notes still yields its slides. (src/markdown/README.md, src/markdown/README.md retains TinyMemory OfficeConverter semantics, including form-feed PDF page)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Before merge

  • Address carried finding Assert the error case in the nested-archive test instead of silently skipping.
Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The new tests all appear to exercise real intake caps; the prior finding about the nested-archive test silently skipping the error case still stands, since the rewritten test still asserts only on a successful conversion of the outer document. (1 finding added by a second pass) (1 earlier finding(s) still open)

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The answered security lane reports that no model findings survived falsification: the change exposes intake's bounded ZIP admission and per-member cap to the Markdown path without sending untrusted input to a dangerous sink, and without introducing authentication/authorisation, network, deserialization, path-traversal, secret-leakage or widened CI-permission problems. `src/markdown/README.md` is prose and was not security-reviewed by that lane.
  • Lane summary: No model findings survived falsification. (1 earlier finding(s) still open)

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The answered tests lane reports the new tests are worth keeping: both refusal tests assert on the specific admission-limit messages, giving real regression protection, and the nested-archive test asserts the outer text is present while the nested secret is absent — with one gap remaining in that nested test.
  • Lane summary: The new tests are worth keeping: both refusal tests assert on the specific admission-limit message (not merely that conversion failed), and the nested-archive test asserts the outer text is present while the nested secret is absent. One gap remains in that nested test, so it gets a finding below. (1 earlier finding(s) still open)

commits

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The answered commits lane reports no sensitive content in the committed files (with one earlier finding still open, per its summary).
  • Lane summary: No sensitive content in committed files. (1 earlier finding(s) still open)

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The answered description lane reports that the nested-archive test now unwraps the conversion and asserts presence of the outer text and absence of the inner text, while keeping its prior finding open: an `Err` from the admission path still passes the test without exercising the asserted guard, unlike the two refusal tests which do assert the error case.
  • Lane summary: The prior finding stands: the nested-archive test still calls `convert(...).unwrap()` and only asserts that the outer text is present and the inner text absent, so an `Err` from the admission path (or any other refusal) passes the test without exercising the asserted guard. The two new refusal tests do assert the error case, but this one does not. (1 earlier finding(s) still open)
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.005192
  • Tokens: 312703 input · 6294 output · 52224 cached · 397 embedding
  • Updated: 2026-11-11T12:27:00Z
Head State Pass summary
8adfa5ca22c0 changes requested 1 active finding(s), 0 resolved finding(s) (at 2026-11-11T11:57:36Z)
f2f0050c91a7 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-11-11T12:27:00Z)

tinysweeper 0.1.2

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: 1 lane(s) blocking, worst finding is high.

Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.

Diagnostics

Total spend: $0.0218

Comment thread src/markdown/mod_tests.rs Resolved
@tinysweeper tinysweeper Bot added the priority: p1 Next. Wrong behaviour a user will hit, or a security weakness behind a condition. label Nov 11, 2026
The test only checked the text when convert succeeded, so an error would
have passed silently. Conversion succeeds here (the corrupt record is on a
member DOCX never reads), so require that, require the outer text, and
refuse the nested text.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previously-blocking findings are resolved. Clearing the changes request.

Diagnostics

Total spend: $0.0052

@tinysweeper tinysweeper Bot added priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. and removed priority: p1 Next. Wrong behaviour a user will hit, or a security weakness behind a condition. labels Nov 11, 2026
@M3gA-Mind
M3gA-Mind merged commit 0b97088 into tinyhumansai:main Nov 11, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

markdown::convert skips the ZIP admission checks that intake::extract enforces

1 participant