Kjell Tore Guttormsen
0f308c1f56
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>
137 lines
7 KiB
Markdown
137 lines
7 KiB
Markdown
# 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=…)
|
|
-> '\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: `<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.
|