feat(upload)!: opt-in form fields via lvt-upload-with

[adnaan]

Closes the client half of livetemplate/livetemplate#452.

The problem

A Proxied upload auto-fires on file selection — there is no submit for the user to review — and the client POSTed the entire enclosing form to the upload endpoint. The only guard was a denylist excluding type="password", so a CSRF token, a hidden secret, or an autocomplete="current-password" text input all still travelled.

A denylist fails open: anything the author didn't anticipate leaks silently.

The change

Inverted to opt-in. A field travels only when marked lvt-upload-with:

<form>
  <input type="hidden" name="id" value="{{.Record.ID}}" lvt-upload-with />
  <input type="hidden" name="csrf" value="{{.CSRFToken}}" />
  <input type="file" lvt-upload="scan" />
</form>

id reaches OnUpload; csrf does not. Failing to mark a needed field now surfaces as a missing value in the handler — a visible bug — rather than as a leak.

Names are collected from the marked elements and FormData still does the serializing, so successful-control semantics (unchecked boxes, disabled controls, multi-<select>) remain the browser's job rather than something reimplemented here.

Two contract questions raised in the client#130 review are answered in code comments:

• Name collision — the file part wins over a marked field of the same name (set, not append).
• Multi-file — each file is POSTed as its own request, so marked fields ride along with every one; each request reaches OnUpload self-describing.

Verification

Four tests cover the contract. Crucially, they were run against the old denylist implementation to confirm they discriminate — 3 of the 4 fail there, for the right reason:

✕ serializes only the lvt-upload-with fields ...
    expect(fd.get("csrf")).toBeNull()
    Received: "tok-abc"          <- the leak, reproduced
✕ sends no form fields at all when a form marks none
    +   "id",  +   "note"        <- unmarked fields travelling
✕ opts in a whole radio group when any one member is marked

A test that passes both before and after would prove nothing; these fail before and pass after.

Full suite: 819 passed, 39 suites, tsc --noEmit clean.

Browser-verified end-to-end against this exact bundle via the upload-modes chromedp e2e in livetemplate/docs (all 4 tests, WS-up and WS-disabled paths).

Compatibility

Breaking, and deliberately not phased — the project is alpha with no external users, so this collapses straight to the end state rather than shipping a deprecation window.

Note the docs PR is safe to merge in either order: the marker is an inert unknown attribute to the currently-published client, which still delivers the field via the old denylist.

Out of scope

Field-carrying is multipart-only; the WS chunked path (upload_chunk/upload_complete) never carried form fields and still doesn't. That transport asymmetry is pre-existing and unchanged here — worth a separate look if marked fields should reach chunked uploads too.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG

[claude]

Review

What this does: Flips form-field carrying on a Proxied auto-upload from a denylist (exclude type="password") to an explicit opt-in (lvt-upload-with). Correctly closes the leak described in the linked issue — a CSRF token, hidden secret, or autocomplete="current-password" text input previously rode along silently since a Proxied upload fires without a submit moment.

Correctness

• appendFormFields logic checks out: collect names carrying lvt-upload-with from form.elements, then filter new FormData(form).entries() down to that set, excluding lvt-action and File values. Delegating to native FormData serialization for the actual values (rather than reimplementing) is the right call — it keeps successful-control semantics (unchecked boxes, disabled controls, radio/checkbox groups) as the browser's job.
• The set() (not append()) on the file part correctly makes the file win a name collision against a marked field — documented in a comment, and the invariant is enforceable by inspection, not just by test.
• Marking by name rather than by element means opting in one radio/checkbox in a group opts in the whole group; this is a little surprising at first glance but is intentional and clearly documented, and the new test (opts in a whole radio group when any one member is marked) locks in the behavior.
• Marking is form-scoped, not upload-scoped — a field marked once travels with every upload triggered from that form, including each individual request when a selection produces multiple files. Also intentional and documented; worth double-checking the field lands in the migration docs (docs repo) since it's a subtle multi-file/multi-trigger implication that's easy to miss when migrating a form.

Security

• The new default (opt-out → opt-in / fail-closed) is strictly more restrictive than the old denylist, so there's no regression risk — worst case is a missing field, which surfaces as a visible bug in OnUpload rather than a silent leak.

Tests

• Good discipline: the PR notes the new tests were run against the old implementation to confirm 3 of 4 fail there for the right reason, which is exactly what makes them worth trusting.
• Coverage is thorough: opt-in fields travel, unmarked stays put, lvt-action and marked file inputs can't be hijacked, and the radio/checkbox group semantics are pinned down explicitly.

Nits (non-blocking)

• The WS chunked upload path's pre-existing inability to carry any form fields is correctly called out as out of scope, but since this PR closes the security gap for the multipart path, it might be worth a tracking issue so the asymmetry doesn't get forgotten.

No blocking issues found — this looks correct, well-tested, and appropriately scoped as a breaking change given the alpha status.

[adnaan]

Thanks — both nits actioned:

Chunked-path asymmetry → filed as livetemplate/livetemplate#508 so it doesn't get lost. Agreed it's more visible now than before: #452 turned field-carrying into an explicit author-declared contract, and an explicit contract that quietly doesn't hold on one transport is more surprising than the incidental behaviour it replaced.

Multi-file / multi-trigger implication in the docs → covered in livetemplate/docs#120, which documents both the form-scoped marking ("a marked field travels with every upload fired from its form") and the per-request multi-file behaviour. I also added a test this round (f036c04) locking the multi-file contract — it was documented in three places but every test drove a single file, so a regression to first-only would have slipped through.

[claude]

Review

What it does: Flips Proxied-upload form-field serialization from a denylist (exclude type="password") to an opt-in allowlist (lvt-upload-with). Correctly closes the leak: since a Proxied upload auto-fires on file selection with no submit checkpoint, any co-located field the denylist didn't anticipate (CSRF token, hidden secret, etc.) previously rode along silently.

Correctness

• appendFormFields logic is sound: collects opted-in names from form.elements by attribute presence, then lets native FormData(form) do the actual serialization — so successful-control semantics (unchecked boxes, disabled, radio/checkbox groups, multi-select) stay correct for free rather than being reimplemented.
• lvt-action is explicitly excluded even if marked, preventing a malicious/careless mark from clobbering the caller-set action via formData.append after formData.set already wrote it.
• File-typed values are excluded from the marked set, so a marked file input can't double-send its bytes as a value field.
• set() (not append()) for the actual file part means a name collision between a marked field and the upload field correctly gives the file precedence — documented in code and matches the PR's stated contract.
• Multi-file selections re-serialize marked fields per request, matching the one-file-per-POST transport model.

Test coverage

Good — and notably, the PR description states the new/changed tests were run against the old implementation first and 3 of 4 failed there for the right reason (the leak reproduced), which is convincing evidence the tests actually discriminate old vs. new behavior rather than just passing trivially. The radio-group and multi-file-request tests cover the trickier edge cases well.

Minor observations (non-blocking)

• The WS chunked upload path (upload_chunk/upload_complete) still doesn't carry any form fields — correctly called out as pre-existing/out-of-scope, but worth a follow-up issue since it's an asymmetry a user could easily trip over after this change (Proxied gets opt-in fields, chunked gets none).
• No package.json version bump in this diff for a change explicitly marked BREAKING — assuming that's handled at release time rather than per-PR in this repo, but flagging in case it's expected here.
• Doc comments are thorough (arguably slightly verbose for a private method), but they clearly capture the non-obvious contract decisions (name collision, multi-file re-send, form-scoped vs. upload-scoped marking), which is valuable given this is a security-sensitive default change.

Overall: a well-scoped, well-tested fix for a real fail-open vulnerability, with a sensible fail-closed replacement. No bugs found.

[adnaan]

Round 2 raised no new functional issue, so stopping here rather than pushing another round. Answering the two new nits by reply:

No package.json version bump — correct, that's handled at release time by release.sh, not per-PR. Bumping in the PR would conflict with every other PR in flight and would claim a version that isn't published yet.

Doc comments verbose for a private method — deliberate, and I'd keep them. What's recorded there isn't how the method works (that's readable from six lines of code) but why the default is off: the next person to look at this will be someone who marked a field, forgot, and is debugging a missing value in OnUpload — and the tempting fix from that seat is to make it implicit again. The comment is the argument against reintroducing the fail-open behaviour, which is exactly the kind of thing that gets re-litigated once it's no longer in anyone's head.

Chunked-path asymmetry — already filed as livetemplate/livetemplate#508 after round 1.