feat(tagged-pdf): show the page agent the PDF's form field names (#483) - #500
Conversation
With tagged PDFs on, the upload reads the kept PDF's fields (`iris-pdf fields`) into fields.json by page. The page prompt lists them and asks for each control to be named after its field. A field with no named control is logged (`page_fields_missing`), not corrected yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All checks pass (install, typecheck, 1771 unit tests, e2e, actionlint, shellcheck). No workflow files touched. One blocking issue.
Blocking: a PDF field name reaches a name="…" attribute with no filter on characters that end the attribute
src/pipeline/fields.ts:49-52:
Give each field's control a name attribute holding the field's name EXACTLY as listed (the text inside the quotes).
The name is arbitrary PDF-authored text from iris-pdf fields and is never filtered. The JSON quoting at src/pipeline/fields.ts:47 protects the prompt line only — its own comment says so ("a quote or newline in it must not end the quote") — and test/form-fields.test.ts:32-36 pins that a name containing " and a newline is accepted and shown to the model.
This is the case src/util/pdf.ts:61-73 already decided, for link hrefs:
const UNSAFE_CHARS = /["'<>`\s\u0000-\u001f\u007f]/;
// ... the page agent is told to copy the URL EXACTLY — so `https://ok.example/a" onmouseover="alert(1)`
// would arrive whole in a document served as text/html. Nothing downstream sanitizes
// agent output; wrapDocument concatenates it verbatim.
// ... it holds whether or not a given poppler build escapes them and whether or not the model obeys "exactly".Field names are the same kind of input going into the same kind of place, and the document is served res.type("text/html").send(html) at src/routes/sessions.ts:556. I checked: assembly.ts does not strip attributes or handlers, and anchors.ts does not look at name, so there is nothing downstream.
Input that reaches it: with tagged_pdf.command set, upload a one-PDF session whose AcroForm has a field named x" onfocus="alert(1) (or any name containing ", <, >, or a control character). The field is read into fields.json, listed in the page prompt, and the instruction is to reproduce it verbatim. Whether the broken attribute lands depends on the model obeying "EXACTLY" — which is exactly the dependency pdf.ts refuses to take; the floor case is a malformed <input> in the delivered document, the ceiling is an event handler in it.
Fix that matches the existing rule: drop a field whose name matches pdf.ts's UNSAFE_CHARS (in keepPdfFields, or in pageFieldContext), and log the drop. iris-pdf cannot match such a field by name in the HTML anyway, so nothing usable is lost.
Non-blocking notes
- Nothing tells the agent not to add a control.
src/pipeline/fields.ts:50asserts "these are its fields on this page", and the only counter-instruction is about names ("Do not invent names for controls that are not listed"). For a field whose widget the page image does not show (a hidden or calculated AcroForm field), the pressure is to emit an input that is not on the page — arriving unlabeled, i.e. an axelabelviolation the review loop then "fixes" by inventing a label from nothing. Latent: it depends on whetheriris-pdf fieldslists invisible widgets, whichdocs/API.mddoes not say. One sentence in the section ("Do not add a control the image does not show") would close it before the probe runs. - Option lists are length-capped (
MAX_OPTION_CHARS = 40) but names are not (src/pipeline/fields.ts:15-19). Up to 40 names of unbounded length go into both the first pass and the correction pass of every form page. A per-name cap matching the options rule would bound the prompt. POST /v1/sessionsnow awaitsreadFieldsbefore answering (src/routes/sessions.ts:469), and that call's timeout is 60 s (src/util/taggedPdf.ts:88-91), spent while the in-flight upload slot is held. Every one-PDF upload on a tagged-PDF deployment pays an extra inline process run.
Accessibility impact: intended to be positive — fields get tagged where their controls sit — but the unfiltered field name can land as a malformed or handler-bearing attribute on a delivered form control, and a field with no visible widget can push the agent to emit an unlabeled input.
Review on #500: a PDF field name went verbatim into an instruction to copy it into name="…". Names with a quote, <, >, a backtick or a control character, or over 100 characters, are dropped at upload and counted. The section also says not to add a control the image does not show. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Blocking issue fixed: Notes:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
All checks pass (install, typecheck, unit, e2e, actionlint, shellcheck). No workflow files touched.
The blocking finding from 9785964 is fixed. src/pipeline/fields.ts:16-23 adds
const UNSAFE_NAME = /["'<>`\u0000-\u001f\u007f]/;
plus a 100-character cap, src/routes/sessions.ts:180 drops such fields before fields.json is written, and test/form-fields.test.ts:37-44 / test/tagged-pdf.test.ts:144-148 pin both the rule and the end-to-end drop (x" onfocus="alert(1) never reaches the file). Since fields.json is the only source of img.fields (src/pipeline/orchestrator.ts:68), filtering at write time covers both the first pass and the correction pass. Allowing a space is correct and the comment says why. The two earlier non-blocking notes are closed too: "Do not add a control the image does not show" (fields.ts:68) and the name-length bound.
Non-blocking notes
-
src/routes/sessions.ts:180—if (!usableFieldName(f?.name) || !Number.isInteger(f.page)) { unusable++; ... }counts a field with a perfectly good name but a badpageas an unusable name, whiledocs/API.md:4377-4379definesunusableas "names dropped because a quote,<,>, a backtick or a control character could end the attribute, or longer than 100 characters". That list also omits', whichUNSAFE_NAMErejects. Log-only, so no delivered output changes; it just makesform_fieldsambiguous when diagnosing a tagger that emits a non-integer page. A separate counter, or widening the doc sentence to "names dropped, and fields with no page number", closes it. -
Unchanged since
9785964, repeating once only for the record:POST /v1/sessionsstill awaitskeepPdfFields(src/routes/sessions.ts:473), so every one-PDF upload on a tagged-PDF deployment pays an inlineiris-pdf fieldsrun with a 60 s timeout while the upload slot is held.
Accessibility impact: positive — each form field can now be tagged where its control sits instead of at the end of the page, and the name that reaches the name="…" attribute is now restricted to characters that cannot break out of it, so a hostile PDF field name can no longer land as a malformed or handler-bearing attribute on a delivered control.
Review note on #500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Probe result (approved by the user, one run, about $0.29). The probe used iris-pdf's 1-page
What the run doesn't show: whether other form pages, or pages without fields, changed in quality. It's one page with no baseline run. Round 2 note 1 is fixed. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
All checks pass (install, typecheck, unit, e2e, actionlint, shellcheck). No workflow files touched. The only change since the approved 0c2ef00 is docs/API.md.
Prior note 1 is fixed. docs/API.md:4377-4379 now matches the code: the unusable list covers ' and the non-integer-page drop that src/routes/sessions.ts:180 counts (!usableFieldName(f?.name) || !Number.isInteger(f.page)), and the character list matches UNSAFE_NAME at src/pipeline/fields.ts:19. I checked the new page_fields_missing sentence too — missingFields runs on the first pass's innerHtml (src/pipeline/extraction.ts:5037), well before correctPage (src/pipeline/extraction.ts:5354), so "the names no control in the first pass's HTML has" and "a control a later correction removes is not reported here" are both accurate.
Non-blocking notes
-
page_fieldscarries the redraw flag —...(redrawn ? { redrawn: true } : {})atsrc/pipeline/extraction.ts:3758— but the two placesdocs/API.mdsays which events carry it have not grown to include it.docs/API.md:1957-1958:redrawn: trueis present when the reply was a page's second draw … The same flag appears for the same reason onpage_style_attributes,page_digit_groupsandpage_links.and the
page_fieldsbullet atdocs/API.md:4380listsimage,fieldsanddroppedonly. Log-only and docs-only, but someone grepping that enumeration to decide whether a doubledpage_fieldsline means two form pages or one redrawn page will get it wrong. Addingpage_fieldsto the list at 1958 closes it.
Accessibility impact: none — this push changes documentation only; the delivered-HTML behaviour is the one approved at 0c2ef00 (fields named where their controls sit, with names restricted to characters that cannot break out of the name="…" attribute).
Review note on #500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Note fixed: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
All checks pass (install, typecheck, unit, e2e, actionlint, shellcheck). No workflow files touched. The only change since the approved e1e9759 is docs/API.md (3 insertions, 2 deletions).
The one note from e1e9759 is fixed, both halves of it. docs/API.md:1959 now reads
The same flag appears for the same reason on
page_style_attributes,page_digit_groups,page_linksandpage_fields.
and the event's own bullet at docs/API.md:4380-4381 grew the field: "dropped (past the cap of 40) and, on a second draw, redrawn: true". That matches src/pipeline/extraction.ts:3754-3759, where page_fields spreads ...(redrawn ? { redrawn: true } : {}) exactly as page_links does at :3748, so a reader grepping either enumeration to tell a doubled page_fields line (one redrawn page) from two form pages now gets the right answer from both places.
No new findings. I am not repeating the inline-iris-pdf fields-on-upload note a third time; it is on record at 0c2ef00.
Accessibility impact: none — this push changes documentation only; the delivered-HTML behaviour is the one approved at 0c2ef00.
Iris Maintainer Agent here.
Closes #483.
With tagged PDFs on, the page agent now gets the PDF's form field names, so iris-pdf can tag each field where its control sits rather than at the end of the page.
iris-pdf fieldsis read intosessions/<id>/fields.jsonby page. A failure is logged (form_fields), and the run converts the same.src/pipeline/fields.ts: the prompt section (names JSON-quoted, cap 40, short option lists). A name that could end itsname="…"attribute is dropped at upload andmissingFields. The section goes in the first pass and the correction pass.page_fields_missing). No correction is bought for it until the probe shows the names match.agents/page.mdis unchanged. The instruction lives in the section, so pages without fields keep the same cached prefix.Tests:
test/form-fields.test.tsand one intest/tagged-pdf.test.ts. Removing the section, the quoting, thedata-nameguard or the unsafe-name filter each fails a test.npm test1772/1772, typecheck and e2e pass.Probe (one run, about $0.29): on iris-pdf's 1-page
form-acroform.pdf,field_not_in_htmlfell from every field to 1 of 7 (reset, which the page image doesn't show). Details in the PR comments.🤖 Generated with Claude Code