fix(assets): bound what the run pays, not what the document claims (0.10.1)

A second independent review read `230d1cb` -- the commit that closed the
`v0.10.0` review's two MAJOR findings -- and found one of them open. The
bound read `/Width` and `/Height`, which an untrusted document writes, while
`get_data()` pays for the stream beside them; `/Length` is the COMPRESSED
length and the two numbers are independent.

Re-measured here on `ed8d9d7` before anything changed, in its own
interpreter: a 408 516-byte PDF declaring 1x1 and carrying 400 MB of deflated
zeros was CARRIED, no rejection, 891 904 000 B peak RSS. After: 0 carried,
`asset_too_large`, 57 065 472 B. At 1,2 GB of zeros, 2 436 MB -> 64 569 344 B
-- the cost no longer scales with the bomb. End to end through the CLI with
the shipped defaults: 838 000 640 B and an asset written -> exit 0,
79 650 816 B, `0 carried of 1 found`, no `assets/`.

Three numbers are bounded now, not one: what a container DECLARES, what a
carried FILE measures (`read_image`, so a 49 MP PNG of 47 705 bytes is not
passed on to a consumer), and what a PDF stream DECOMPRESSES to
(`assets.inflated_size`, a chunk at a time, output discarded, before
`get_data()`). The limit is stated rather than implied: the stream
measurement runs where `FlateDecode` is the first filter and the document is
not encrypted; every other chain is a check on the decoded length AFTER the
decode, a counted refusal and not a bounded one.

A non-positive declared dimension is `asset_size_invalid`, its own code,
raised before the stream is read. `-1 x 40000000000` is a NEGATIVE pixel
count, under which every `>` bound read as satisfied, so the check returned
silently and the refusal arrived from `encode_png` as
`asset_samples_invalid`. Its own code because a publisher shipping a picture
bigger than this package carries and a dictionary written to be read wrong
are different facts about a document.

Two smaller findings in the line that says what is missing, both introduced
by the first fix: the address was written twice, once bare, and a linkifying
renderer autolinks a bare URL -- written once now, in one code span; and
`label` became a dead parameter, so the figure's caption was dropped, a
regression against 0.10.0. It is written again in the `-- <label>` form a
carried pointer uses.

Version bumped to 0.10.1 across all ten places. Nine were unbound and stale:
four README install lines naming the previous release, two prose lines, the
"current tag" entry, `uv.lock`, and a CHANGELOG whose 0.10.1 content sat
under `[Unreleased]`. Two new packaging tests bind them to `__version__`, and
the README's guard tag to `[tool.uv.sources]`.

Every test was red first. The fate of every image is identical with and
without the new bound on three K2 PDFs carrying 800 images (464/464, 311/311
with the same 12 rejections, 25/25), and the second inflate is below the
noise floor there. 0 shipped artifacts move: no bundle under `examples/`,
`skills/` or `tests/fixtures/` carries an image pointer at all, measured
against a known-positive control.

`asset_too_large` was undocumented in the error registry; both codes are
there now. `tools/okf_accounting_gate.py` gains the new code in its closed
list -- one string, no behaviour change, stated because that file belongs to
another order.

Suite 2141 passed / 1 skipped, ruff + format + mypy --strict clean.
Report: docs/2026-09-18-bildestien-holder-0-10-1.md

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

2026-09-18 13:41:18 +02:00

7 KiB

Raw Blame History

Two findings in the image path, closed before push (0.10.1)

Order 20260917T135034Z-4840022792, from an independent review of v0.10.0 (~/.claude/docs/2026-09-17-review-okf-0.10.0.local.md, LOCAL-ONLY). The review's verdict was "safe to push, with reservations": no blocker, and byte-identity, determinism and --no-assets parity all hold. Both findings land with the shipped defaults (--assets on, --gate guard-trusted-source), and both are new in 0.10.0, because before it no reader read an <img> attribute or opened an image stream.

Both repros were rebuilt here as tests before anything was fixed (tests/test_asset_limits.py, 17 tests).

MAJOR-1: a remote reference was a live markdown image link

Reproduced:

render_missing('https://collect.example.net/p.gif?u=S', href=…)
-> '![fig](https://collect.example.net/p.gif?u=S)\nImage: … (not carried: …)'

The first line is a live image link to an address the document's author controls, query string included. This package opens no socket. A consumer that renders the bundle, or an agent that fetches what it renders, does — which turns "this bundle was opened" into a beacon (and a server-side consumer into an SSRF). Measured with guard 1.4.0: PRESET_USER_UPLOAD fails the document secure, PRESET_TRUSTED_SOURCE — the build's default — persists it.

Fixed in render_missing: a reference with an href is written as Image: <name> (not carried: <reason>) address:

`` — inert text, with the address still stated, because a reader has to know what stood there.

Pinned as a property, not a string. FOREIGN_IMAGE_LINK matches any markdown image whose target is not this bundle's own assets/. It is asserted over the two readers that resolve references (HTML, STS), over three shapes of remote address (https:, //host, upper-case scheme), and over a whole built bundle of the shipped fixture inbox. The known-positive beside it: a local image still produces a pointer block that IMAGE_POINTER matches, and a data: URI image is still carried.

The tier asymmetry is the guard's own question and was sent to llm-ingestion-pipeline-security with the repro (20260917T221801Z-428505178). Nothing was built there.

MAJOR-2: nothing bounded a declared image size

Reproduced with the review's own generator, rebuilt in the test file: a PDF declaring one grayscale image of compressed zeros.

declared	PDF file	peak RSS
3 000 x 3 000	9.6 KB	83 MB
8 000 x 8 000	63 KB	276 MB (review's measurement)

The cost is linear in the pixel count, so 50 000 x 50 000 is several GB. One document — malicious, defective, or a legitimately enormous scan — could take a whole batch build with it, before any gate, because the guard never sees image bytes.

The bound is read off the corpora, not chosen. Over the 4 828 image objects of the 43-document reference corpus the largest is 4 515 x 4 128 (18.6 MP, a landscape drawing). Over R761's 109 delivered pictures the largest is 2 072 x 656 (1.4 MP).

MAX_IMAGE_PIXELS = 40 000 000 (2.1x the largest measured).
MAX_IMAGE_BYTES = 256 MiB of samples.
Over either: asset_too_large, counted like every other refusal, with the declared size in the reason. Never a silent skip and never a killed build.

Checked on what the container DECLARES, before anything is decompressed. stream.get_data() is what pays for the bomb, so the declared Width and Height are read first. The order is observable, not asserted: the test feeds a PDF whose image stream is corrupt AND whose declared size is over the bound. Decoding first gives asset_pdf_unsupported; reading the size first gives asset_too_large. encode_png refuses the same size on its own, so the encoder does not trust its caller.

CORRECTION, 2026-09-18 — this paragraph claimed a defence this round did not build. A declared size and a decompressed stream size are two independent numbers: /Length is the COMPRESSED length, and nothing in the dictionary states what get_data() returns. A second independent review measured a 408 516-byte PDF declaring 1x1 and carrying 400 MB of deflated zeros being CARRIED, with no rejection, at 892 MB of peak RSS — the same failure mode this round set out to close, reached through the other number. The bound was real and it bound the wrong thing. What this round DID close is the declared size and the data: URI; the stream itself is bounded from docs/2026-09-18-bildestien-holder-0-10-1.md, and the paragraph below about a verbatim file is corrected there too.

The data: URI, which the review flagged and did not measure, is closed by the same bound, checked on the payload length before decoding (base64 expands by 3/4). Measured: refused with asset_too_large as a row.

Not bounded, and stated rather than hidden: an image FILE carried verbatim is never decoded, so its cost is its own file size. (Corrected 2026-09-18: it is bounded now. This package pays nothing for such a file, but a 7 000 x 7 000 PNG of 47 705 bytes written into a bundle hands the consumer the same bomb with 7000x7000 px printed beside it, and the README's own first sentence about this bound said it was refused.)

The determinism defect PM added to this order

pdfminer names an inline image (BI … EI) from id() of a Python object. Measured 2026-09-17: two concept files of the reference corpus differed between two builds of one commit, in the pointer line (Image: page-3-6344095824 against page-3-4555034320). That breaks the bit-exact rebuild invariant.

Fixed: a name pdfminer derived from id() — all digits, no resource name — is replaced by the image's position on the page (page-3-inline-2). The test builds a PDF with an inline image and compares two extractions with the _pdf_pages cache cleared between them.

The three MINOR findings

images: N counts pointer blocks, not unique pictures (12 pointers to 2 files is images: 12). Documented in the README rather than changed: the number is the count of places a picture stands, dedup is on content, and changing it would move bundle bytes and lose the placement count.
A concept that is only a pointer block is persisted as substantive. Documented: "degenerate" means zero characters after stripping whitespace, and a pointer block is text. Changing the definition is a spec question.
data: URI: closed, see above.

Verification

tests/test_asset_limits.py: 17 tests, both repros red first.
Full suite 2 045 passed, 1 skipped; ruff, ruff format --check and mypy --strict clean. Re-run after git add against a clean tree.
R761 unchanged (PM's measurement, repeated): --gate none gives 50 carried of 50 found, 50 files in assets/, and diff -rq against the bundle built before this change reports no difference.
tools/okf_accounting_gate.py: green on all six rows.

Proposal

Version 0.10.1. No tag, no release, no push — the content accounting sits in the same tree and PM wants the order of the two settled first.

7 KiB Raw Blame History