From 6906e57769a79fcf39955c4c0d1037434ea0c579 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sun, 21 Nov 2021 19:28:37 -0500 Subject: [PATCH] `GET` Submissions: don't merge into `searchParams` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The background --- According to the HTML Specification's [§ 4.10.21.3 Form submission algorithm][] section, submissions transmitted as `GET` requests [mutate the `[action]` URL][mutate], overriding any search parameters already encoded into the `[action]` value: > [Mutate action URL][algorithm] > --- > > Let pairs be the result of converting to a list of > name-value pairs with entry list. > > Let query be the result of running the > `application/x-www-form-urlencoded` serializer with pairs > and encoding. > > Set parsed action's query component to query. > > Plan to navigate to parsed action. [§ 4.10.21.3 Form submission algorithm]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm [algorithm]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#submit-mutate-action [mutate]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-algorithm:submit-mutate-action Form submissions made with `POST` requests, on the other hand, encode _both_ the `[action]` value's query parameters and any additionally encoded body data: > [Submit as entity body][post-submit] > --- > > … > > Plan to navigate to a new request whose URL is parsed > action, method is method, header list is > « (`Content-Type`, mimeType) », and body is body. [post-submit]: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#submit-body The problem --- Ever since [18585fc][] (and subsequently [hotwired/turbo#108][]), Turbo's `FetchRequest` has merged submission values from `
` elements with `[method="get"]` and without `[action]` attributes _into_ the current URL's query parameters. For example, consider the following forms rendered on the `/articles` page: ```html
``` Without Turbo, entering "Hotwire" as the search term into `input[name="q"]` and clicking the "Search" button would navigate the browser to `/articles?q=Hotwire`. Then, clicking the "Sort ascending" button would navigate the page to `/articles?order=asc`. Without Turbo, entering "Hotwire" as the search term into `input[name="q"]` and clicking the "Search" button navigates the browser to `/articles?q=Hotwire`. Then, clicking the "Sort ascending" button **navigates the page to `/articles?q=Hotwire&order=asc`**, effectively _merging_ values from the page's URL and the `
` element's fields. [18585fc]: https://github.com/hotwired/turbo/commit/18585fc95ead42e7a7f3f6f75274ae4308f40c15#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R135-R142 [hotwired/turbo#108]: https://github.com/hotwired/turbo/pull/108 The solution --- This commit modifies the way that `FetchRequest` constructs its `url: URL` and `body: FormData | URLSearchParams` properties. First, it _always_ assigns a `body` property, but _conditionally_ encodes that value into the `fetchOptions: RequestInit` property based on whether or not the request is an idempotent `GET`. Next, if constructed with a `body: URLSearchParams` that has entries, **replace** the `url: URL` instance's search params _entirely_ with those values, like the HTML Specification algorithm entails. If constructed with a `body: URLSearchParams` that is empty, pass the `url: URL` through and assign the property **without modifying it**. Additionally, this commit adds test cases to ensure that `POST` requests transmit data in both the body and the URL. While the previous multi-form merging behavior can be desirable, it is not the behavior outlined by the HTML Specification, so Turbo should not provide it out-of-the-box. Having said that, there are two ways for applications to restore that behavior: 1. declare a [turbo:before-fetch-request][] event listener to merge values _into_ the event's `detail.url` instance: ```js addEventListener("turbo:before-fetch-request", ({ target, detail: { url, fetchOptions: { method } } }) => { if (target instanceof HTMLFormElement && method == "GET") { for (const [ name, value ] of new URLSearchParams(window.location.search)) { // conditionally call `set` or `append`, // depending on your application's needs if (url.searchParams.has(name)) continue else url.searchParams.set(name, value) } } }) ``` 2. declare a [formdata][] event listener to merge values _into_ the submitted form's [FormData][] instance prior to entering the Turbo request pipeline: ```js addEventListener("submit", (event) => { if (event.defaultPrevented) return const { target, submitter } = event const action = submitter?.getAttribute("formaction") || target.getAttribute("action") if (target.method == "get" && !action) { target.addEventListener("formdata", ({ formData }) => { for (const [ name, value ] of new URLSearchParams(window.location.search)) { // conditionally call `set` or `append`, // depending on your application's needs if (formData.has(name)) continue else formData.set(name, value) } }, { once: true }) } }) ``` The conditionals in both cases could be omitted if applications controlled the behavior more directly, like by declaring a Stimulus controller and action (e.g. ``). [turbo:before-fetch-request]: https://turbo.hotwired.dev/reference/events [formdata]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/formdata_event [FormData]: https://developer.mozilla.org/en-US/docs/Web/API/FormData --- src/core/frames/frame_controller.ts | 8 ++-- src/http/fetch_request.ts | 19 ++++++---- src/tests/fixtures/form.html | 12 ++++++ src/tests/functional/form_submission_tests.ts | 37 +++++++++++++++++++ src/tests/server.ts | 4 +- 5 files changed, 67 insertions(+), 13 deletions(-) diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index 6a1cd9908..d53ae75b2 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -6,7 +6,7 @@ import { clearBusyState, getAttribute, parseHTMLDocument, markAsBusy } from "../ import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission" import { Snapshot } from "../snapshot" import { ViewDelegate } from "../view" -import { getAction, expandURL, urlsAreEqual, locationIsVisitable, Locatable } from "../url" +import { getAction, expandURL, urlsAreEqual, locationIsVisitable } from "../url" import { FormInterceptor, FormInterceptorDelegate } from "./form_interceptor" import { FrameView } from "./frame_view" import { LinkInterceptor, LinkInterceptorDelegate } from "./link_interceptor" @@ -86,7 +86,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest this.currentURL = this.sourceURL if (this.sourceURL) { try { - this.element.loaded = this.visit(this.sourceURL) + this.element.loaded = this.visit(expandURL(this.sourceURL)) this.appearanceObserver.stop() await this.element.loaded this.hasBeenLoaded = true @@ -235,8 +235,8 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest // Private - private async visit(url: Locatable) { - const request = new FetchRequest(this, FetchMethod.get, expandURL(url), new URLSearchParams, this.element) + private async visit(url: URL) { + const request = new FetchRequest(this, FetchMethod.get, url, url.searchParams, this.element) this.currentFetchRequest?.cancel() this.currentFetchRequest = request diff --git a/src/http/fetch_request.ts b/src/http/fetch_request.ts index 6e13fe1ba..fca4dc529 100644 --- a/src/http/fetch_request.ts +++ b/src/http/fetch_request.ts @@ -56,12 +56,10 @@ export class FetchRequest { this.delegate = delegate this.method = method this.headers = this.defaultHeaders - if (this.isIdempotent) { - this.url = mergeFormDataEntries(location, [ ...body.entries() ]) - } else { - this.body = body - this.url = location - } + this.body = body + this.url = this.isIdempotent && this.entries.length ? + mergeFormDataEntries(new URL(location.href), this.entries) : + location this.target = target } @@ -118,7 +116,7 @@ export class FetchRequest { credentials: "same-origin", headers: this.headers, redirect: "follow", - body: this.body, + body: this.isIdempotent ? null : this.body, signal: this.abortSignal, referrer: this.delegate.referrer?.href } @@ -155,6 +153,7 @@ export class FetchRequest { function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL { const currentSearchParams = new URLSearchParams(url.search) + deleteAll(url.searchParams) for (const [ name, value ] of entries) { if (value instanceof File) continue @@ -169,3 +168,9 @@ function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]) return url } + +function deleteAll(searchParams: URLSearchParams) { + for (const name of searchParams.keys()) { + searchParams.delete(name) + } +} diff --git a/src/tests/fixtures/form.html b/src/tests/fixtures/form.html index d31137e95..22e4e14b2 100644 --- a/src/tests/fixtures/form.html +++ b/src/tests/fixtures/form.html @@ -25,6 +25,18 @@

Form

+
+
+ +
+
+ + +
+
+ +
+
diff --git a/src/tests/functional/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts index 9df835722..f2f2c2f66 100644 --- a/src/tests/functional/form_submission_tests.ts +++ b/src/tests/functional/form_submission_tests.ts @@ -88,6 +88,15 @@ export class FormSubmissionTests extends TurboDriveTestCase { await this.nextEventNamed("turbo:load") } + async "test standard POST form submission merges values from both searchParams and body"() { + await this.clickSelector("#form-action-post-redirect-self-q-b") + await this.nextBody + + this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html") + this.assert.equal(await this.getSearchParam("q"), "b") + this.assert.equal(await this.getSearchParam("sort"), "asc") + } + async "test standard POST form submission toggles submitter [disabled] attribute"() { await this.clickSelector("#standard-post-form-submit") @@ -126,6 +135,34 @@ export class FormSubmissionTests extends TurboDriveTestCase { await this.nextEventNamed("turbo:load") } + async "test standard GET form submission does not incorporate the current page's URLSearchParams values into the submission"() { + await this.clickSelector("#form-action-self-submit") + await this.nextBody + + this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html") + this.assert.equal(await this.search, "?sort=asc") + + await this.clickSelector("#form-action-none-q-a") + await this.nextBody + + this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html") + this.assert.equal(await this.search, "?q=a", "navigates without omitted keys") + } + + async "test standard GET form submission does not merge values into the [action] attribute"() { + await this.clickSelector("#form-action-self-submit") + await this.nextBody + + this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html") + this.assert.equal(await this.search, "?sort=asc") + + await this.clickSelector("#form-action-self-q-b") + await this.nextBody + + this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html") + this.assert.equal(await this.search, "?q=b", "navigates without omitted keys") + } + async "test standard GET form submission toggles submitter [disabled] attribute"() { await this.clickSelector("#standard-get-form-submit") diff --git a/src/tests/server.ts b/src/tests/server.ts index 7c576a6a8..227865a46 100644 --- a/src/tests/server.ts +++ b/src/tests/server.ts @@ -19,12 +19,12 @@ router.use((request, response, next) => { router.post("/redirect", (request, response) => { const { path, sleep, ...query } = request.body - const pathname = path ?? "/src/tests/fixtures/one.html" + const { pathname, query: searchParams } = url.parse(path ?? request.query.path ?? "/src/tests/fixtures/one.html", true) const enctype = request.get("Content-Type") if (enctype) { query.enctype = enctype } - setTimeout(() => response.redirect(303, url.format({ pathname, query })), parseInt(sleep || "0", 10)) + setTimeout(() => response.redirect(303, url.format({ pathname, query: { ...query, ...searchParams } })), parseInt(sleep || "0", 10)) }) router.get("/redirect", (request, response) => {