Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

injectQuery incorrectly double-encodes query string parameters #18244

Open
7 tasks done
mykwillis opened this issue Sep 30, 2024 · 1 comment · May be fixed by #18246
Open
7 tasks done

injectQuery incorrectly double-encodes query string parameters #18244

mykwillis opened this issue Sep 30, 2024 · 1 comment · May be fixed by #18246
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@mykwillis
Copy link

Describe the bug

My code uses import.meta.url to read a query string paramter from the URL used to load my module in a browser <script> tag. This works reliably in all environments except that, when HMR is used, the url query string is corrupted when HMR adds a "t=" parameter (presumably to bust the cache).

I tracked this down to an issue with the injectQuery method.

For example, the src given here:

        <script
            type="module"
            src="/src/embed.ts?id=66cf6a5a47fe7364e779d3c9&url=https%3A%2F%2Fnewrobotoverlords.com%2Fnotebooklms-a-new-way-to-learn-and-understand%2F"
            async
        ></script>

Ends up being read as import.meta.url as:

http://localhost:5173/src/embed.ts?t=1727723109376&id=66cf6a5a47fe7364e779d3c9&url=https%253A%252F%252Fnewrobotoverlords.com%252Fnotebooklms-a-new-way-to-learn-and-understand%252F

(Note that a t parameter has been added, and the url parameter has been incorrectly doubly-encoded, or at least has extra "%25" characters added to it.)

Reproduction

https://github.com/mykwillis/vite/tree/inject-query-bug

Steps to reproduce

pnpm i
pnpm run test-unit

System Info

System:
    OS: macOS 14.6.1
    CPU: (12) arm64 Apple M2 Max
    Memory: 1.11 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.20.4 - ~/.nvm/versions/node/v18.20.4/bin/node
    npm: 10.7.0 - ~/.nvm/versions/node/v18.20.4/bin/npm
    pnpm: 9.11.0 - ~/.nvm/versions/node/v18.20.4/bin/pnpm
  Browsers:
    Chrome: 129.0.6668.70
    Safari: 17.6
  npmPackages:
    @vitejs/release-scripts: ^1.3.2 => 1.3.2 
    rollup: ^4.22.5 => 4.22.5 
    vite: workspace:* => 6.0.0-beta.1

Used Package Manager

pnpm

Logs

No response

Validations

@mykwillis
Copy link
Author

mykwillis commented Sep 30, 2024

It seems this bug was introduced with #2614

I wrote this test as part of the submitted branch, which fails and exhibits the problem:

  test('path with url-encoded path as query parameter', () => {
    const src = '/src/module.ts?url=https%3A%2F%2Fusr.vite%2F'
    const expected = '/src/module.ts?t=1234&url=https%3A%2F%2Fusr.vite%2F'
    expect(injectQuery(src, 't=1234')).toEqual(expected)
  })
 FAIL  packages/vite/src/node/__tests__/utils.spec.ts > injectQuery > path with url-encoded path as query parameter
AssertionError: expected '/src/module.ts?t=1234&url=https%253A%…' to deeply equal '/src/module.ts?t=1234&url=https%3A%2F…'

Expected: "/src/module.ts?t=1234&url=https%3A%2F%2Fusr.vite%2F"
Received: "/src/module.ts?t=1234&url=https%253A%252F%252Fusr.vite%252F"

 ❯ packages/vite/src/node/__tests__/utils.spec.ts:120:40
    118|     const src = '/src/module.ts?url=https%3A%2F%2Fusr.vite%2F'
    119|     const expected = '/src/module.ts?t=1234&url=https%3A%2F%2Fusr.vite%2F'
    120|     expect(injectQuery(src, 't=1234')).toEqual(expected)
       |                                        ^
    121|   })
    122| })

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 1, 2024
@sapphi-red sapphi-red linked a pull request Oct 1, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants