Skip to content

feat: arm a create-new form's save by the form the run typed into - #1543

Closed
abrichr wants to merge 5 commits into
mainfrom
claude/create-form-identity
Closed

abrichr wants to merge 5 commits into
mainfrom
claude/create-form-identity

Conversation

@abrichr

@abrichr abrichr commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

The third onboarding blocker from #1447: a save on a create-new form compiled with no identity check, and qualification and clinical-write refuse an unarmed save. #1537 fixes the other two.

The problem

A form that creates a record (Trac's New Ticket, NetBox's Add Device) shows no existing record next to its save button. The identity band around Create ticket holds only the button's own label, so the save compiled unarmed ("the only readable row text is the target's own label"). Qualification then refused it ("save step … has no armed identity check").

The documented remedy, record --identifier FIELD, doesn't work here. I recorded a Trac-shaped form with --identifier field_summary, and the save was armed by a pixel crop of the summary field. Replay with a new summary then halted on an identity mismatch, because the crop pictures the demo's own value.

The fix

The record a create-form save commits is the one the run typed into the form. So the demonstrated form, all of it, is the identity.

  • Recording. For a labelled submit button of the form that owns every field typed since the last save, the submit-binding observer (submit_binding.py) records three things:

    • the form's relation to those fields;
    • a fingerprint: the submission's origin and path, its method, and the sorted name/type list of every control it would send, hidden ones included;
    • the values of the fields the run didn't type into.
  • Compile. The step gets Anchor.form_identity (FormIdentity), which holds only:

    • the filled-field count and form_submit;
    • SHA-256 digests of the address and the control list, plus the method;
    • salted digests of the untouched values (the identity-template scheme).
  • Hidden tokens are never compared, and their values are never kept. Two kinds are exempt: ones named like a token (csrf, token, nonce, …), whose value changes on every page load (Trac's __FORM_TOKEN is one), and hidden values that changed while the demonstration was typed. Password values never leave the page.

  • Replay. The new top identity tier (mode form) checks, in the DOM, right before the click, that:

    • the button is still that form's submit button;
    • the form owns exactly the fields this run typed into, each holding exactly what was typed;
    • the address, method and control list match the demonstration;
    • every untouched field holds its demonstrated value.

    Anything else is a mismatch and halts. A backend without form_identity_at leaves identity unreadable, which halts.

  • During the click. A one-shot capture-phase submit guard cancels any submit that is not that form from that button (for example a click handler's requestSubmit() on another form), and the step halts.

  • Signal quorum. A signal-quorum identity policy and a form identity must both hold.

  • Policy and reports. is_identity_armed, the replayer's identity gates, the armed-step count and the fixup pass all accept form_identity.

Tests (real headless Chromium)

tests/test_create_form_identity.py has 17 cases. Each records a Trac-shaped New Ticket form with the interactive recorder, compiles it with the real compiler, and replays it with the real Replayer.

Verifies and creates the right ticket:

  • new values;
  • a button outside the form with form=;
  • a matching hidden parent or select;
  • a per-session CSRF token that differs on the new page load;
  • a hidden value that rotated during the demonstration.

Halts before the click, nothing submitted:

  • the six holes from the review:
    • an edit form that looks like the create form (id=42, /ticket/42);
    • an address and method changed after typing;
    • a hidden id added while typing;
    • a hidden parent on another record;
    • a select left on another record (both reported as "the form's 'pid' field … holds a different value");
    • a click handler that submits another form (cancelled by the guard);
  • a field rewritten after typing;
  • a button of another form;
  • a portal button against a form= recording;
  • reformatting controlled inputs (as you type: stopped by the typed-value read-back; on blur: stopped by the form check);
  • a keyed React remount, pinned as a known false halt.

The reviewer's unmodified probe (test_probe_1543.py), run against this commit: all six holes halt with nothing submitted, and the controls (plain, parent_ok, select_ok, search_present) verify and submit.

Regression, run locally with one worker: the submit-binding, interactive-recorder, recorder, id-less locator, create-form, policy, identity, qualification (without the long gate campaign), replayer, compiler and fixup suites. 1646 passed, 11 skipped, 0 failed. ruff and mypy are clean.

Limits

  • A script that submits with form.submit() or fetch sends no submit event, so the guard doesn't see it. LIMITS.md says so.
  • A field that reformats the typed value halts, by design. So does a field replaced by a keyed remount.
  • A program that should create under a different parent on each run halts until that parent is checked another way.
  • This covers web forms only, and it doesn't prove the app saved the record correctly; only a record check does.

Merge note

origin/main (with #1537) merged in cleanly, with no conflicts.

🤖 Generated with Claude Code

A form that creates a record (Trac's New Ticket, NetBox's Add Device)
shows no existing record next to its save button, so the save compiled
with no identity check and qualification refused it ("save step has no
armed identity check"). `record --identifier FIELD` didn't help: its pixel
crop pictures the demo's own value, so a run with a new value halted.

The record such a save commits is the one the run typed into the form.
The submit-binding observer now also reads a labelled control's form
relation, and the compiler gives a consequential labelled click that has
no other identity evidence, and is a button of the one form owning every
field typed since the last save, an Anchor.form_identity (counts and
tokens only). Before the click the replayer checks, in the DOM, that the
button's form holds exactly the fields this run typed into and that each
still holds the value the run left in it (the Playwright backend
snapshots each field after typing). A rewritten field, a missing field or
a button of another form halts before the click; a backend that can't
read the page leaves identity unreadable, which halts too.

Found by the onboarding-effort benchmark (#1447).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
abrichr and others added 2 commits October 25, 2026 10:01
…and portal saves

Review cases for React apps:
- A controlled input that reformats the value now halts. The Playwright
  backend records, after typing, whether the field holds exactly the typed
  text (a select: its chosen option's label), and the form check counts a
  field that didn't as changed. A field reformatted on every keystroke stops
  at the typed-value read-back; one reformatted on blur stops at the form
  check.
- A submit button outside the <form> element is armed when its form=
  attribute names the form. One with no form owner (a portal that submits
  from script) gets no form identity and stays unarmed, and a program
  recorded with form= halts before the click on such a page.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abrichr
abrichr marked this pull request as ready for review October 25, 2026 14:15
@abrichr
abrichr enabled auto-merge October 25, 2026 14:15
@abrichr
abrichr disabled auto-merge October 25, 2026 14:19
@abrichr

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

AI review (coordinator)

Verdict: blocking. Auto-merge left off. Form identity proves that the typed fields hold the typed values. It proves nothing about the rest of the form: hidden record ids, untouched defaults, or the action URL. So it arms an existing record's edit form and a create form scoped to the wrong parent, and the save goes through.

How I tested

I wrote real headless Chromium probes on 17a13f311, using the PR's own harness: record_interactive on a Trac-shaped New Ticket page, then compile_recording and the real Replayer + PlaywrightBackend. Each probe recorded on the clean page and replayed against a variant. I ran one worker under nice: 14 probes, about 85 s. The page's submit handler logs action, method and FormData.

Variant (replayed) Identity Run What was submitted
plain (control) form / verified success correct create
edit: same blank fields, hidden id=42, action=/ticket/42, title "Ticket #42 (edit)" form / verified success update of ticket 42
action_swap: script sets action=/ticket/42, method=post and adds hidden id=42 on description focus verified success POST /ticket/42 with id=42
late_hidden: script appends hidden id=42 on description input verified success id=42 sent with the save
parent_wrong: recorded with hidden pid=7, replayed with pid=9 verified success new ticket under pid 9
select_wrong: untouched "Patient" select, recorded on Ann (7), default on Bob (9) at replay verified success pid=9
click_other: Create button in the form, its click handler requestSubmit()s another form verified success other form submitted
search form + focus stolen to search box mismatch "not in the form that owns the filled fields" halted nothing (good)
React-style remount of the summary node on blur (same value / different value) mismatch "typed into 1 field(s) … had 2" halted nothing (good, fail-closed; false halt on keyed remounts is a limit to document)

Answers to the six questions:

  1. Edit form that looks like a create form: wrongly armed. Nothing compares the form's action, method or non-typed controls, so a record update passes as a create.
  2. Two forms with focus moving: OK. Mixed ownership halts, and so does a typed-into count mismatch.
  3. Pre-filled hidden id or wrong default: not enough. "Owns exactly the fields typed" counts only the typed fields. Every other successful control rides along unchecked.
  4. WeakMap across a re-render: fails closed. The new node is not in the fill store, so the count drops and the run halts. It never passes vacuously.
  5. Portal or form= resolving elsewhere: mostly OK. A form= pointing at another form halts. But a button inside the right form whose handler submits a different form is verified, and submit_role="form_control" (type=button) adds nothing, because its behaviour is script-defined.
  6. Action or method changed after typing: not detected.

There is also a smaller issue in _verify_identity. The form_identity early return comes before the qualification signal_quorum policy branch, so a step-level quorum policy is silently skipped for these steps.

Exact fixes

  1. Form fingerprint (structure only, no values). At record time, in OBSERVE_SUBMIT_BINDING_JS for the labelled/shared case, add these to the observation and to FormIdentity:

    • action_sha256: SHA-256 of the effective submission target. That is the submitter's formAction || form.action, reduced to origin plus path (no query or fragment).
    • method: the submitter's formMethod || form.method.
    • controls_sha256: SHA-256 of the sorted list of (name, type) for every successful control the submit would send. That is the keys of new FormData(form, submitter) joined with the control types, hidden included.

    At replay, _FORM_IDENTITY_AT_JS recomputes all three right before the click, and verify_form_identity halts on any difference. This closes edit, action_swap and late_hidden.

  2. Untouched value-bearing controls. Some controls the run did not type into still carry a non-empty value: hidden inputs, selects, checked radios and checkboxes. For each one, record a salted digest of its value (same salt scheme as identity_template) keyed by name. At replay, halt if any such control's value differs. Exempt only hidden controls whose value already differed between the recording's page load and its click, such as CSRF tokens; the recorder can snapshot at first fill and again at the click. This closes parent_wrong and select_wrong. A program that legitimately changes the parent per run then halts and needs a parent identity check, which is the correct fail-closed outcome.

  3. Bind the click to the checked form. Accept only submit_role == "form_submit" for form identity, and drop form_control. Before the click, install a one-shot window capture-phase submit listener. It calls preventDefault() and stopImmediatePropagation() and records a refusal when event.target !== checkedForm or event.submitter !== control, and the step then fails on that refusal. This closes click_other for requestSubmit and native submits. Add a LIMITS.md line that a form.submit() or fetch from script is not seen.

  4. Ordering. Move the form_identity branch below the signal_quorum branch, or AND them.

  5. Tests. Add the five HOLE rows above as test_create_form_identity.py cases that must halt before the click with created == [], plus a control showing that a CSRF-style rotating hidden token still verifies.

The parts that hold up well are the WeakMap snapshot (no value returns to Python), the dedupe and count check, the mixed-form refusal, and the fail-closed paths for an unreadable page or a remounted node.

abrichr added a commit that referenced this pull request Oct 25, 2026
Create now carries a form identity (bug 17, #1543). The run gate then refused seven unarmed field clicks on Medplum's placeholder-only form (bug 21), so the teach reaches those fields by keyboard from armed clicks; all 10 clicks are armed. Certification still refuses two key steps that only move the focus and assert no postcondition (bug 22). Evidence in results/medplum-phase-a/lease-2.

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

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

Tested this branch, merged with #1564, on Medplum 5.2.8's new-patient form (React and Mantine, synthetic data), from the second-installation work in #1548.

What worked

  • Re-recorded with this branch, the labeled "Create" button compiles with form_identity (filled_fields: 9, submit_role: form_submit) and identity_armed: true.
  • qualify's save-step precondition passes.
  • The run gate passes identity coverage (10/10) and effect coverage.

Gaps and questions the form showed (observed at record and compile time only; no qualification case or replay has run yet)

  1. 9 filled fields for 10 typed values. The demonstration typed 10 parameters, and the gender field is the likely one left out. It is a Mantine autocomplete: the clerk types the code, then ArrowDown and Enter. Choosing an option re-renders the field and drops the focus to <body>. If the field the run typed into is replaced, "the form owns exactly the fields this run typed into" may not hold at replay. Worth a test with a controlled combobox that re-renders on selection.
  2. Rows added by "Add" buttons. Telecom, Name and Address rows only exist after an "Add …" click. The form's field set grows during the run, so the owned field set at the Create click depends on how many rows were added.
  3. Native selects filled by typing. The Telecom system select gets its value from type-ahead (phone, email), not from a text input. Does a select count as a typed field for the "still holds the value the run left in it" check?
  4. Not seen here: no form= attribute, and no submit button rendered in a portal outside the <form>. Create is inside the form.

Separately, certification still refuses the program for an unrelated reason: two key steps assert no postcondition, because they only move the focus. That's tracked as bug 22 in #1548.

🤖 Generated with Claude Code

…s own submit

Review of #1543: form identity checked only that the typed fields kept
their typed values, so an edit form that looked like the create form, an
address, method or record id a script changed or added, a hidden parent
or an untouched select on another record, and a click handler that
submitted another form all verified and submitted wrong data.

- Fingerprint: the observer records, for a labelled submit button of the
  filled form, the submission's origin and path, its method, and the
  sorted name/type list of every control it would send (hidden ones
  included). FormIdentity keeps their SHA-256 and the run recomputes them
  right before the click; any difference halts.
- Untouched values: every field the run didn't type into is kept as a
  salted digest (the identity-template scheme) and compared. Hidden tokens
  are exempt: named like one (csrf, token, nonce...), since their value
  changes every page load, or changed while the demonstration was typed.
  Their values are never kept. Passwords never leave the page.
- Only a form's own submit button gets a form identity. Right before the
  click a one-shot capture-phase submit guard cancels any submit that isn't
  that form from that button, and the step halts.
- A signal-quorum identity policy and a form identity must now both hold.

Tests: the review's six holes halt before the click with nothing
submitted, a per-session CSRF token and a hidden value that rotated
during the demonstration still verify, and a keyed React remount is
pinned as a known false halt (LIMITS.md).

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

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

Addressed in 81d00f5, all five points:

  1. Fingerprint. For a labelled submit button of the filled form, the observer records the submission's origin and path, its method, and the sorted name/type list of every control it would send, hidden ones included. FormIdentity keeps SHA-256 digests of the address and the control list, plus the method. The run recomputes them right before the click and halts on any difference.
  2. Untouched values. Every field the run didn't type into is kept as a salted digest (the identity-template scheme) and compared. Hidden tokens are exempt, and their values are never kept. One change from what you proposed: besides hidden values that changed during the demonstration, the exemption also covers hidden controls named like a token (csrf|xsrf|token|nonce|authenticity|verification|viewstate|eventvalidation). A per-session token such as Trac's __FORM_TOKEN doesn't rotate within a recording but differs on every new session, so without this nearly every real CSRF-protected form would false-halt. Both exemptions are tested.
  3. Click binding. Only form_submit gets a form identity. A one-shot capture-phase submit guard cancels any submit that isn't the checked form from the checked button, and the step halts. LIMITS.md notes that form.submit() and fetch aren't seen.
  4. Ordering. A signal-quorum policy and a form identity must both hold.
  5. Tests. test_create_form_identity.py now has 17 cases: your six holes halt with nothing submitted, both kinds of rotating token still verify, and the keyed remount is pinned as a known false halt.

I also ran your unmodified test_probe_1543.py against 81d00f5: edit, action_swap, late_hidden, parent_wrong, select_wrong and click_other all halt with created == [], and plain, parent_ok, select_ok and search_present verify and submit.

Local regression with one worker: 1646 passed, 0 failed. Auto-merge is still off for your re-check.

`arg=self._fill_value_key` passes a per-backend page key (a uuid made at
runtime) and matched generic-api-key. The line is gone since 81d00f5;
the fingerprint pins commit, file, rule and line, so nothing else is
relaxed.

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

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

gitleaks: the failure was a false positive in this PR's first commit. arg=self._fill_value_key passes a per-backend page key (a uuid made at runtime) and matched generic-api-key. The line is gone since 81d00f5, but the scan covers history, so e74703a pins that one fingerprint (commit:file:rule:line) in .gitleaksignore. A full-history gitleaks git --log-opts=HEAD is clean locally.

The Medplum questions, answered from how the code works. None of these were run on Medplum, which has only been recorded and compiled so far.

  1. A combobox that re-renders on selection. If choosing the option replaces the element the run typed into with a new one, the form no longer owns the fields this run typed into, and the run halts before Create. That is the same false halt as the keyed remount, which is a pinned test and listed in LIMITS.md. It halts safely, but on Medplum it may halt every run. If the gender field shows up there, the next step is a real-app replay. I haven't started that, because new work is paused.
  2. Rows added by "Add" buttons. The fingerprint is taken at the Create click, so it includes every added row. A run that adds the same rows matches. A run that adds a different number of rows changes the field set and halts.
  3. A native select filled by type-ahead. Yes, it counts as a field the run typed into. It is registered when typed into, and it must end up with its chosen option's label equal to the typed text, so phone has to select the option labelled phone.
  4. form= and portals: both are covered by tests on this PR, though not seen on Medplum.

@abrichr

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

AI review (coordinator), re-review of 81d00f5

Verdict: still blocking. Auto-merge left off. All six original holes now halt with nothing submitted, and the qualification-quorum ordering is fixed. The new mechanisms have four gaps that let a wrong-record save through with identity verified.

What I tested

I used real headless Chromium with the PR's recorder, compiler, Replayer and PlaywrightBackend, one worker under nice. The page logs getAttribute('action') (query included), method and FormData(form, submitter).

Round 1, re-run (14 probes): edit, action_swap, late_hidden, parent_wrong, select_wrong and click_other all halt now, with created == []. Controls, search-steal and remount behave as before.

Round 2 (15 new probes). Each row records on one variant and replays on another.

Recorded on → replayed on Identity Submitted
action="/save?mode=new" → action="/save?id=42" verified GET /save?id=42
hidden pid="", click handler copies window.currentPid (7 → 9) verified pid=9
plain → hidden id=42 disabled, click handler enables it verified id=42
plain → click handler preventDefault() + setTimeout(300, other.requestSubmit()) verified other form
plain → click handler setTimeout: append hidden id=42, then form.requestSubmit(button) verified id=42
plain → correct submit, then setTimeout(300) submits the other form verified both forms
hidden pid set by script on first input (recorded 7) → set to 9 verified (exempt=['pid']) pid=9
hidden encounter_token=7 → 9 verified (exempt=['encounter_token']) 9
form=-associated hidden id=42 added outside the form mismatch (controls) nothing (good)
csrf aaa → bbb verified correct (good)
csrf + injected new hidden csrf_token=42 mismatch (controls) nothing (good)
plain → added empty <input type=file> verified empty file part (minor)

Answers to your four questions:

  • Controls fingerprint. It reads form.elements, so form=-associated controls are included. That works. Disabled controls are skipped, which matches what is submitted at check time, but a click handler can enable one afterwards, and nothing re-checks at submit (row 3). File inputs are skipped entirely (minor).
  • CSRF exemption. An agent-injected new hidden field is not exempt: the controls hash changes and the run halts. But exemption is granted (a) to any hidden field whose value changed during the recording's typing, which includes a record or parent id that a script assigns on first input, and (b) to any hidden field whose name matches token|verification|…, whatever its value. Both let a wrong id through.
  • setTimeout. The guard is not effective. form_submit_guard_refusal() is read and disarms right after the click returns, and the guard is one-shot. A submit scheduled for 300 ms later, or a second submit after the guarded one, runs unguarded.
  • Salt. The salt is per form identity (new_salt_hex() per decide_form_identity call) and is stored in the bundle as value_salt unless the env salt is set. That is the identity_template scheme. Low-entropy values (numeric ids, select options) can be recovered by enumeration from a bundle with a stored salt, and the recording's form_values event holds them in plaintext. This is consistent with the existing scheme, not a blocker, but worth one line in LIMITS.md.

Exact fixes

  1. Query in the action. Hashing origin plus path was my earlier suggestion, and it was wrong. Hash origin, path and the sorted query parameter names into action_sha256. Treat query parameter values like untouched control values: salted digests in value_digests under a ?name key, compared at replay.
  2. Re-check contents at submit time. At arm time, store page-side a canonical string of [...new FormData(form, control)] with exempt names removed, plus the effective action (with query) and method. In the capture-phase handler, recompute it and preventDefault() + stopImmediatePropagation() on any difference. This closes the store-copy, disabled-toggle and delayed-mutate rows.
  3. Keep the guard alive past the click. Don't remove it on the first submit, and don't disarm it in form_submit_guard_refusal(). Leave it armed until the page navigates or the step's postcondition and settle window ends. It should refuse every non-matching submit in that window, not only the first. Then read the refusal at the end of that window, and again at the start of the next step. Any recorded refusal makes the run a safety_halt, even if the step had already looked successful. This closes the two setTimeout rows and submit-then-other.
  4. Narrow the exemption. A hidden field is exempt only if its name matches the token pattern and it rotated during the recording and its recorded value looks like a token: at least 16 characters, not all digits, not empty before typing. Rotation alone, or a name match alone, no longer exempts it. A hidden field that goes from empty to a value while typing is compared by digest.
  5. Tests. Add the six bold rows as halt-before-click cases (or cancelled-submit cases for the delayed ones) with created == [], and keep csrf aaa→bbb verifying as the control.

The probes are in the review worktree: packages/flow/tests/test_probe_1543_r2.py, results in results_r2.jsonl.

@abrichr

abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

Replaced by #1581: the same change as one squashed commit on main, so the line gitleaks flagged never exists and no scanner finding is ignored. #1581 is a parked draft under the feature pause, with the re-review's open findings in its description.

@abrichr abrichr closed this Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant