From b88d4924869083158c44a9466c29053e88bee248 Mon Sep 17 00:00:00 2001 From: Adnaan Badr Date: Sun, 19 Jul 2026 20:33:56 +0000 Subject: [PATCH 1/2] feat(upload)!: opt-in form fields via lvt-upload-with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Proxied upload auto-fires on file selection, so the enclosing form was POSTed to the upload endpoint with no submit-time moment for the user to notice. The denylist that guarded this excluded only type="password" — CSRF tokens, hidden secrets and autocomplete="current-password" text inputs all still rode along. Invert the default: a field travels only when marked lvt-upload-with. Forgetting to mark a field the handler needs now surfaces as a missing value in OnUpload — a visible bug — instead of a silent leak. Names are collected from the marked elements and FormData still does the serializing, so successful-control semantics (unchecked boxes, disabled controls, multi-selects) stay the browser's job. Also documents two contract questions raised in review: the file part wins a name collision with a marked field, and when one selection carries several files the marked fields ride along with every request so each reaches OnUpload self-describing. BREAKING CHANGE: form fields no longer travel with a Proxied upload unless marked lvt-upload-with. Add the attribute to each field an OnUpload handler reads (typically a record id). Refs livetemplate/livetemplate#452 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG --- CHANGELOG.md | 13 ++++++ tests/upload-handler.test.ts | 83 ++++++++++++++++++++++++++++++------ upload/upload-handler.ts | 57 ++++++++++++++++--------- 3 files changed, 119 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6e73da..e1c49e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to @livetemplate/client will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Changed + +- **BREAKING** feat(upload): form fields travel with a Proxied upload only when + marked `lvt-upload-with`, replacing the previous serialize-everything-except- + `type="password"` denylist (livetemplate#452). A Proxied upload auto-fires on + file selection, so the old default silently POSTed every co-located field — + CSRF tokens, hidden secrets — to the upload endpoint with no submit-time moment + for the user to notice. **Migration:** add `lvt-upload-with` to each field an + `OnUpload` handler reads (typically a record id); an unmarked field now + surfaces as a missing value in the handler rather than as a silent leak. + ## [v0.18.2] - 2026-07-15 ### Changes diff --git a/tests/upload-handler.test.ts b/tests/upload-handler.test.ts index 3b05048..fe20076 100644 --- a/tests/upload-handler.test.ts +++ b/tests/upload-handler.test.ts @@ -261,20 +261,16 @@ describe("UploadHandler", () => { ); }); - it("serializes the triggering input's form fields into a proxied upload, before the file", async () => { + // Fires a proxied auto-upload from the input named "doc" inside the given + // form markup and returns the FormData that was POSTed. + const proxiedUploadFormData = async (formHTML: string): Promise => { const postMultipart = jest.fn().mockResolvedValue(undefined); const proxiedHandler = new UploadHandler(mockSendMessage, { postMultipartUpload: postMultipart, }); - // The file input lives in a form carrying a record id (a value field) and - // a second, unrelated file input that must NOT become a value field. const form = document.createElement("form"); - form.innerHTML = - '' + - '' + - '' + - ''; + form.innerHTML = formHTML; const input = form.querySelector('input[name="doc"]') as HTMLInputElement; const file = createMockFile("scan.png", "imgdata", "image/png"); @@ -293,20 +289,81 @@ describe("UploadHandler", () => { }); await jest.runAllTimersAsync(); - const fd = postMultipart.mock.calls[0][0] as FormData; - // The record id rides along as a value field... + return postMultipart.mock.calls[0][0] as FormData; + }; + + it("serializes only the lvt-upload-with fields into a proxied upload, before the file", async () => { + const fd = await proxiedUploadFormData( + '' + + '' + + '' + + '' + + '' + + '' + ); + + // The marked record id rides along as a value field... expect(fd.get("id")).toBe("item-42"); // ...ordered before the streamed file part, so OnUpload sees it mid-stream. const keys = [...fd.keys()]; expect(keys.indexOf("id")).toBeLessThan(keys.indexOf("doc")); + + // Everything unmarked stays put. This is the assertion the opt-in contract + // exists for: under the old denylist the csrf token and the note both + // travelled, and only the password was special-cased out. + expect(fd.get("csrf")).toBeNull(); + expect(fd.get("note")).toBeNull(); + expect(fd.get("secret")).toBeNull(); + // The streamed file is still the file part; the other file input is not // serialized as a value field, and the action is untouched. expect(fd.get("doc")).toBeInstanceOf(File); expect(fd.get("other")).toBeNull(); expect(fd.get("lvt-action")).toBe("upload_doc_complete"); - // Password inputs are never serialized into the upload POST — a co-located - // credential must not ride along with an auto-fired upload. - expect(fd.get("secret")).toBeNull(); + }); + + it("sends no form fields at all when a proxied upload's form marks none", async () => { + const fd = await proxiedUploadFormData( + '' + + '' + + '' + ); + + // Unmarked is the default, so the POST carries the action and the file and + // nothing else — no field leaves the page without being asked to. + expect([...fd.keys()].sort()).toEqual(["doc", "lvt-action"]); + }); + + it("ignores lvt-upload-with on the reserved action and on file inputs", async () => { + const fd = await proxiedUploadFormData( + '' + + '' + + '' + ); + + // Marking cannot clobber the action the caller set... + expect(fd.getAll("lvt-action")).toEqual(["upload_doc_complete"]); + // ...nor smuggle a second file in as a value field. + expect(fd.get("other")).toBeNull(); + }); + + it("opts in a whole radio group when any one member is marked", async () => { + const fd = await proxiedUploadFormData( + '' + + '' + + '' + + '' + + '' + ); + + // Marking is by name, so the checked sibling travels even though the mark + // sits on the unchecked one... + expect(fd.get("kind")).toBe("photo"); + // ...the browser's successful-control rules still decide the value, which + // is why the marked-but-unchecked box sends nothing... + expect(fd.get("flag")).toBeNull(); + // ...and an entirely unmarked group stays put, checked or not. + expect(fd.get("tier")).toBeNull(); }); it("previews locally without uploading when mode is preview", async () => { diff --git a/upload/upload-handler.ts b/upload/upload-handler.ts index 2ec5e7d..4690b4c 100644 --- a/upload/upload-handler.ts +++ b/upload/upload-handler.ts @@ -426,7 +426,7 @@ export class UploadHandler { /** * Upload a file as a single multipart POST to the live URL, carrying the - * upload__complete action and the enclosing form fields. The server + * upload__complete action and any lvt-upload-with fields. The server * routes the part by the field's configured mode: a Proxied field streams the * bytes to OnUpload (zero local-disk staging), a Volume-with-Dir field stages * them to Dir. Independent of the WebSocket — Proxied always uses this, and @@ -453,9 +453,12 @@ export class UploadHandler { // Write value fields BEFORE the file part so the server resolves the // action regardless of multipart part ordering. formData.set("lvt-action", `upload_${entry.uploadName}_complete`); - // Then the triggering input's enclosing form fields (e.g. a record id), so + // Then the fields marked to travel with the upload (e.g. a record id), so // the server can associate the streamed bytes with a record in OnUpload. this.appendFormFields(formData, entry); + // set(), not append(): if a marked value field happens to share the upload + // field's name, the file replaces it. The file part is the whole point of + // the request, so it wins the collision. formData.set(entry.uploadName, entry.file, entry.file.name); await this.postMultipartUpload(formData, entry.abortController.signal); @@ -476,37 +479,49 @@ export class UploadHandler { } /** - * appendFormFields serializes the value fields of the form enclosing the - * upload's triggering input into formData, ahead of the file part — so a - * Proxied OnUpload handler can read them (e.g. a record id) mid-stream. Uses - * the browser's native form serialization (successful controls only), skipping - * File values (file inputs are sent as the streaming part) and the reserved - * lvt-action field. A no-op when the input is outside a form. + * appendFormFields serializes the opted-in value fields of the form enclosing + * the upload's triggering input into formData, ahead of the file part — so a + * Proxied OnUpload handler can read them (e.g. a record id) mid-stream. A + * no-op when the input is outside a form or nothing is marked. * - * A Proxied upload auto-fires on file selection (not an explicit submit), so it - * POSTs the whole enclosing form. Password inputs are excluded by name so - * credentials in a co-located form never ride along with an upload the user - * didn't deliberately submit; keep other sensitive controls out of a form that - * contains an auto-upload file input. (A broader opt-in model — only fields - * marked to travel — is tracked upstream as a safer-by-default follow-up.) + * Opt-in, not opt-out: only fields carrying `lvt-upload-with` travel. A + * Proxied upload auto-fires on file selection rather than on an explicit + * submit, so the user never gets a submit-time moment to notice what leaves + * the page. A denylist fails open — an unanticipated sensitive control (a CSRF + * token, a hidden secret, an `autocomplete="current-password"` text input) + * rides along silently. An allowlist fails closed: forget to mark the record + * id and OnUpload sees a missing field, which is a visible bug rather than a + * silent leak. + * + * The mark is form-scoped, not upload-scoped: a marked field travels with + * every upload fired from its form. Marking is by *name*, so marking any one + * member of a radio/checkbox group opts in the whole group — the browser's + * own successful-control rules then decide which values are actually sent. * * Values are read here, at upload time (after the upload_start round-trip), not * at file-selection time, so edits made between selecting the file and the * upload completing are reflected — the freshest form state wins. + * + * When one selection carries several files, each is POSTed as its own request, + * so the marked fields are re-serialized into every one — each request reaches + * OnUpload independently and must be self-describing. */ private appendFormFields(formData: FormData, entry: UploadEntry): void { const form = entry.sourceInput?.form; if (!form) return; - // Collect password field names to exclude. Casting to HTMLInputElement is safe - // for the type check: only has type="password";