Fix Fetch dropping repeated form fields and leaking src into history (2.x)

CHANGELOG.md

@@ -11,6 +11,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - **@studiometa/ui-motion:** add a new `@studiometa/ui-motion` package with the `Motion`, `MotionScrollTimeline`, `MotionSequence` and `MotionView` components to animate elements declaratively with [Motion](https://motion.dev) ([#628](https://github.com/studiometa/ui/pull/628), [#629](https://github.com/studiometa/ui/pull/629), [#630](https://github.com/studiometa/ui/pull/630), [#633](https://github.com/studiometa/ui/pull/633), [#636](https://github.com/studiometa/ui/pull/636), [#637](https://github.com/studiometa/ui/pull/637), [#638](https://github.com/studiometa/ui/pull/638), [#640](https://github.com/studiometa/ui/pull/640), [#641](https://github.com/studiometa/ui/pull/641))
 - **DataBind:** add the `data-bind:if` virtual binding to render `<template>` content conditionally, announcing each change with the bubbling `dom-update` protocol event ([#626](https://github.com/studiometa/ui/pull/626), [#634](https://github.com/studiometa/ui/pull/634))
 
+### Fixed
+
+- **Fetch:** send every value of a repeated GET form field instead of the last one, so a checkbox group or a `<select multiple>` no longer reaches the server with one of its values
+- **Fetch:** push the element's own destination in history rather than the fetched URL, so a `src` pointing at a lighter endpoint no longer leaks into the address bar — see the new `historyUrl` getter
+
 ### Changed
 
 - **Dialog:** make the `open` and `close` events bubble and extendable with `event.detail.waitUntil()`, which also accepts a transitioner ([#627](https://github.com/studiometa/ui/pull/627), [#635](https://github.com/studiometa/ui/pull/635))

packages/docs/reference/items/Fetch/js-api.md

@@ -141,7 +141,43 @@ Returns the global `fetch` function.
 
 - Return: `URL`
 
-Resolves the request URL. The base is the [`src` option](#src) when it is set, otherwise the element's own destination: a link's `href`, a form's `action`, or the current location as a last resort. For a form with `method="get"`, the form data is then folded onto that base as URL parameters (fields set on top, so a fixed query in `src` is preserved).
+Resolves the request URL. The base is the [`src` option](#src) when it is set, otherwise the element's own destination: a link's `href`, a form's `action`, or the current location as a last resort. For a form with `method="get"`, the form data is then folded onto that base as URL parameters.
+
+Folding replaces what the base URL carried under the same name, and keeps every value a name carries. So a fixed query in `src` survives alongside the live fields, a field of the same name overrides it, and a control with several values — a checkbox group, a `<select multiple>` — sends all of them:
+
+```html
+<!-- ?genre=rock&genre=jazz&section=results -->
+<form action="/search" method="get" data-component="Fetch" data-option-src="/search/suggest?genre=stale&section=results">
+  <input type="checkbox" name="genre" value="rock" checked />
+  <input type="checkbox" name="genre" value="jazz" checked />
+</form>
+```
+
+### `historyUrl`
+
+- Return: `URL`
+
+Resolves the URL the address bar should show, which is not always the one that was requested. The [`src` option](#src) says *what to request*; this says *what the navigation is*.
+
+Without `src` the two are identical. With it, history follows the element's own destination — a link's `href`, a form's `action` folded with its form data — so a lighter endpoint can serve the request without leaking into the URL a visitor copies:
+
+```html
+<a
+  href="/projects/page/2?orderby=title"
+  data-component="Fetch"
+  data-option-history
+  data-option-src="/projects/page/2?orderby=title&sections=listing">
+  2
+</a>
+```
+
+Clicking that requests the `sections=listing` URL and pushes `/projects/page/2?orderby=title`.
+
+A URL passed explicitly to [`fetch(url)`](#fetch-url-url-string-requestinit-requestinit) is pushed as given: a caller that named a URL meant that URL.
+
+::: tip
+On a back or forward navigation the component re-fetches `window.location.href`, which is now the pushed URL rather than the `src` one. Keep the [`selector`](#selector) matching elements that exist in **both** responses — the full page and the lighter endpoint — or the two directions will not update the same regions.
+:::
 
 ### `requestInit`
 

packages/tests/Fetch/Fetch.spec.ts

@@ -133,6 +133,96 @@ describe('The Fetch class', () => {
       expect(fetch.url.href).toBe('https://example.com/submit?foo=bar');
     });
 
+    it('should keep every value of a repeated GET form field', async () => {
+      // A checkbox group is repeated names by design. Setting each field on top of the
+      // last left one value, so ticking a second box changed nothing.
+      const one = h('input', { type: 'checkbox', name: 'genre[]', value: 'rock', checked: true });
+      const two = h('input', { type: 'checkbox', name: 'genre[]', value: 'jazz', checked: true });
+      const form = h('form', { action: 'https://example.com/search', method: 'get' }, [one, two]);
+      const fetch = new Fetch(form);
+      await mount(fetch);
+
+      expect(fetch.url.searchParams.getAll('genre[]')).toEqual(['rock', 'jazz']);
+    });
+
+    it('should keep every value of a repeated field that has no brackets', async () => {
+      // The bracket suffix is a PHP convention, not an HTML one: a repeated name is
+      // repeated whether or not it ends in `[]`, so the fold cannot key on its shape.
+      // `<select multiple>` is the other everyday case and is not covered here, because
+      // happy-dom puts only the first selected option in FormData.
+      const one = h('input', { name: 'genre', value: 'rock' });
+      const two = h('input', { name: 'genre', value: 'jazz' });
+      const form = h('form', { action: 'https://example.com/search', method: 'get' }, [one, two]);
+      const fetch = new Fetch(form);
+      await mount(fetch);
+
+      expect(fetch.url.searchParams.getAll('genre')).toEqual(['rock', 'jazz']);
+    });
+
+    it('should let repeated GET form fields replace a conflicting query in `src`', async () => {
+      // Both halves at once: the base's stale value goes, and both live values stay.
+      (window as any).happyDOM.setURL('https://example.com/');
+      const one = h('input', { type: 'checkbox', name: 'genre[]', value: 'rock', checked: true });
+      const two = h('input', { type: 'checkbox', name: 'genre[]', value: 'jazz', checked: true });
+      const form = h(
+        'form',
+        {
+          action: 'https://example.com/search',
+          method: 'get',
+          dataOptionSrc: '/search/suggest?genre[]=stale&section=keep',
+        },
+        [one, two],
+      );
+      const fetch = new Fetch(form);
+      await mount(fetch);
+
+      expect(fetch.url.searchParams.getAll('genre[]')).toEqual(['rock', 'jazz']);
+      expect(fetch.url.searchParams.get('section')).toBe('keep');
+    });
+
+    it('should have a `historyUrl` getter following the link `href` rather than `src`', async () => {
+      (window as any).happyDOM.setURL('https://example.com/');
+      const anchor = h('a', {
+        href: 'https://example.com/projects/page/2?orderby=title',
+        dataOptionSrc: '/projects/page/2?orderby=title&sections=listing',
+      });
+      const fetch = new Fetch(anchor);
+      await mount(fetch);
+
+      expect(fetch.url.searchParams.get('sections')).toBe('listing');
+      expect(fetch.historyUrl.href).toBe('https://example.com/projects/page/2?orderby=title');
+    });
+
+    it('should fold GET form data onto the `action` for `historyUrl`', async () => {
+      (window as any).happyDOM.setURL('https://example.com/');
+      const input = h('input', { name: 'q', value: 'live' });
+      const form = h(
+        'form',
+        {
+          action: 'https://example.com/search',
+          method: 'get',
+          dataOptionSrc: '/search/suggest?sections=results',
+        },
+        [input],
+      );
+      const fetch = new Fetch(form);
+      await mount(fetch);
+
+      // The address bar has to show what the no-JS submit would have produced, filters
+      // included — the bare action would drop them.
+      expect(fetch.historyUrl.href).toBe('https://example.com/search?q=live');
+      expect(fetch.url.searchParams.get('sections')).toBe('results');
+    });
+
+    it('should have a `historyUrl` equal to `url` when there is no `src`', async () => {
+      const input = h('input', { name: 'foo', value: 'bar' });
+      const form = h('form', { action: 'https://example.com/submit', method: 'get' }, [input]);
+      const fetch = new Fetch(form);
+      await mount(fetch);
+
+      expect(fetch.historyUrl.href).toBe(fetch.url.href);
+    });
+
     it('should have a `requestInit` getter', async () => {
       const headers = { 'x-foo': 'bar' };
       const init = { method: 'post' };
@@ -665,6 +755,49 @@ describe('The Fetch class', () => {
       historySpy.mockRestore();
     });
 
+    it('should push the element destination rather than the fetched `src`', async () => {
+      (window as any).happyDOM.setURL('https://example.com/');
+      const anchor = h('a', {
+        href: 'https://example.com/projects/page/2?orderby=title',
+        dataOptionSrc: '/projects/page/2?orderby=title&sections=listing',
+        dataOptionHistory: true,
+      });
+      const fetch = new Fetch(anchor);
+      const clientSpy = vi.fn(() => Promise.resolve(new Response('<div id="test">content</div>')));
+      vi.spyOn(fetch, 'client', 'get').mockImplementation(() => clientSpy);
+      const historySpy = vi.spyOn(window.history, 'pushState');
+      historySpy.mockImplementation(() => undefined);
+
+      await mount(fetch);
+      await fetch.fetch();
+
+      // Requested the lighter endpoint…
+      expect(clientSpy).toHaveBeenCalledWith(
+        new URL('https://example.com/projects/page/2?orderby=title&sections=listing'),
+        expect.objectContaining({ signal: expect.any(AbortSignal) }),
+      );
+      // …and left a URL somebody can copy.
+      expect(historySpy).toHaveBeenCalledWith({}, '', '/projects/page/2?orderby=title');
+      historySpy.mockRestore();
+    });
+
+    it('should push a URL given to `fetch()` rather than the element destination', async () => {
+      (window as any).happyDOM.setURL('https://example.com/');
+      const anchor = h('a', { href: 'https://example.com/from-href', dataOptionHistory: true });
+      const fetch = new Fetch(anchor);
+      const clientSpy = vi.fn(() => Promise.resolve(new Response('<div id="test">content</div>')));
+      vi.spyOn(fetch, 'client', 'get').mockImplementation(() => clientSpy);
+      const historySpy = vi.spyOn(window.history, 'pushState');
+      historySpy.mockImplementation(() => undefined);
+
+      await mount(fetch);
+      await fetch.fetch('/called-explicitly');
+
+      // A caller that named a URL meant that URL, in the address bar as well.
+      expect(historySpy).toHaveBeenCalledWith({}, '', '/called-explicitly');
+      historySpy.mockRestore();
+    });
+
     it('should not push history on popstate', async () => {
       const anchor = h('a', { href: 'https://example.com', dataOptionHistory: true });
       const fetch = new Fetch(anchor);

packages/ui/src/Fetch/Fetch.ts

@@ -144,35 +144,104 @@ export class Fetch<T extends BaseProps = BaseProps>
   }
 
   /**
-   * The URL to use for the request.
+   * The URL history should be given for the request in flight.
+   *
+   * Set by `fetch()`, which is the only place that knows whether the URL came from the
+   * element or from a caller. `update()` may also be called directly, so it falls back to
+   * the URL it was given.
    *
-   * The base URL is the `src` option when it is set, otherwise the element's own destination:
-   * a form's `action`, a link's `href`, or the current location as a last resort. For a GET
-   * form, the form data is then folded onto that base with each field `set` on top — so an
-   * explicit `src` can carry a fixed query (e.g. `?section_id=…`) that survives alongside the
-   * live form fields, with form fields winning on conflict.
+   * @private
    */
-  get url(): URL {
-    const { $el, isForm, isLink, $options } = this;
-
-    const url = $options.src
-      ? new URL($options.src, window.location.href)
-      : isForm
-        ? new URL(($el as HTMLFormElement).action)
-        : isLink
-          ? new URL(($el as HTMLAnchorElement).href)
-          : new URL(window.location.href);
-
-    if (isForm && ($el as HTMLFormElement).method.toLowerCase() === 'get') {
-      // @ts-expect-error URLSearchParams accepts FormData as parameter in the browser.
-      for (const [key, value] of new URLSearchParams(new FormData($el))) {
-        url.searchParams.set(key, value);
+  __historyUrl: URL | undefined;
+
+  /**
+   * The element's own destination: a form's `action`, a link's `href`, or the current
+   * location as a last resort.
+   *
+   * @private
+   */
+  get __destination(): string {
+    const { $el, isForm, isLink } = this;
+
+    if (isForm) {
+      return ($el as HTMLFormElement).action;
+    }
+
+    if (isLink) {
+      return ($el as HTMLAnchorElement).href;
+    }
+
+    return window.location.href;
+  }
+
+  /**
+   * Resolve a base URL and fold a GET form's fields onto it.
+   *
+   * Fields replace what the base URL carried for the same name, and several values under
+   * one name are all kept: the first field of a given name deletes the base's values, and
+   * every field then appends. A single `set` per field would have done the first half and
+   * silently dropped the second, so a checkbox group or a `<select multiple>` — whose whole
+   * purpose is repeated names — reached the server with one of its values.
+   *
+   * @private
+   */
+  __resolveUrl(base: string): URL {
+    const { $el, isForm } = this;
+    const url = new URL(base, window.location.href);
+
+    if (!isForm || ($el as HTMLFormElement).method.toLowerCase() !== 'get') {
+      return url;
+    }
+
+    const overridden = new Set<string>();
+
+    // @ts-expect-error URLSearchParams accepts FormData as parameter in the browser.
+    for (const [key, value] of new URLSearchParams(new FormData($el))) {
+      if (!overridden.has(key)) {
+        url.searchParams.delete(key);
+        overridden.add(key);
       }
+
+      url.searchParams.append(key, value);
     }
 
     return url;
   }
 
+  /**
+   * The URL to use for the request.
+   *
+   * The base URL is the `src` option when it is set, otherwise the element's own
+   * destination. For a GET form, the form data is then folded onto that base — so an
+   * explicit `src` can carry a fixed query (e.g. `?section_id=…`) that survives alongside
+   * the live form fields, with form fields winning on conflict.
+   */
+  get url(): URL {
+    return this.__resolveUrl(this.$options.src || this.__destination);
+  }
+
+  /**
+   * The URL the address bar should show, which is not always the one that was requested.
+   *
+   * The `src` option answers "what to request"; this answers "what this navigation is". A
+   * link may point at a page and fetch a lighter endpoint that renders the same regions:
+   *
+   * ```html
+   * <a href="/projects/page/2?orderby=title"
+   *   data-component="Fetch"
+   *   data-option-history
+   *   data-option-src="/projects/page/2?orderby=title&sections=listing">2</a>
+   * ```
+   *
+   * Pushing the requested URL there would put `sections=listing` in the address bar and in
+   * anything a visitor copies out of it. So history follows the element's own destination,
+   * folded with the same form data, and falls back to the requested URL whenever there is
+   * no `src` to diverge from — which is every element that does not set one.
+   */
+  get historyUrl(): URL {
+    return this.__resolveUrl(this.__destination);
+  }
+
   /**
    * Option for the fetch request.
    */
@@ -285,8 +354,19 @@ export class Fetch<T extends BaseProps = BaseProps>
    * `URL` object resolved against the current location so the history and view-transition
    * paths — which rely on `url.pathname` and `url.searchParams` — stay safe.
    */
-  async fetch(url: URL | string = this.url, requestInit: RequestInit = {}) {
-    const normalizedUrl = url instanceof URL ? url : new URL(url, window.location.href);
+  async fetch(url?: URL | string, requestInit: RequestInit = {}) {
+    // Whether the URL came from the element or from the caller, which is what decides
+    // where history goes: an explicit `fetch('/somewhere')` is a navigation the caller
+    // named, and substituting the element's own destination for it would be a surprise.
+    const fromElement = url === undefined;
+    const normalizedUrl = fromElement
+      ? this.url
+      : url instanceof URL
+        ? url
+        : new URL(url, window.location.href);
+
+    this.__historyUrl = fromElement ? this.historyUrl : normalizedUrl;
+
     const { FETCH_EVENTS } = this.constructor;
     this.$emit(FETCH_EVENTS.BEFORE_FETCH, { instance: this, url: normalizedUrl, requestInit });
 
@@ -425,7 +505,8 @@ export class Fetch<T extends BaseProps = BaseProps>
 
     if (history) {
       if (requestInit?.headers?.[this.__headerNames.X_TRIGGERED_BY] !== 'popstate') {
-        historyPush({ path: url.pathname, search: url.searchParams });
+        const target = this.__historyUrl ?? url;
+        historyPush({ path: target.pathname, search: target.searchParams });
       }
       domScheduler.write(() => {
         if (fragment.title) {