fix(dev): prevent double URL encoding in server.open on macOS

[Sun79]

Description

Currently, when using server.open on macOS, the opened URL is double-encoded.

For example, with the following configuration:

{
  base: '/测试%23%24/',
  server: {
    open: './?key1=测试&key2=%23%24',
  },
}

The opened link becomes: http://localhost:5173/%25E6%25B5%258B%25E8%25AF%2595%2523%2524/?key1=%25E6%25B5%258B%25E8%25AF%2595&key2=%2523%2524
But the correct link should be: http://localhost:5173/%E6%B5%8B%E8%AF%95%23%24/?key1=%E6%B5%8B%E8%AF%95&key2=%23%24

[sapphi-red]

It seems this encodeURI was added in facebook/create-react-app#2076 to allow URLs containing ". I think we should do url.replaceAll('"', '%22') here.

[Sun79]

Thank you for your review! I have carefully checked all the code calling the openBrowser method. The URL obtained from server.resolvedUrls?.local[0] ?? server.resolvedUrls?.network[0] and the path retrieved from new URL(options.open, url).href both already encode the ". Is it still necessary to add url.replaceAll('"', '%22') to encode " in this method?

Looking forward to your feedback!

[sapphi-red]

Ah, then it should be ok. Thanks for checking!

[bluwy]

There's a spot where we don't use new URL(). Maybe we should add it while we're at it:

vite/packages/vite/src/node/shortcuts.ts

Lines 142 to 150 in b80daa7

	action(server) {
	const url =
	server.resolvedUrls?.local[0] ?? server.resolvedUrls?.network[0]
	if (url) {
	openBrowser(url, true, server.config.logger)
	} else {
	server.config.logger.warn('No URL available to open in browser')
	}
	},

This part is for the preview server, which it's handling it correctly this way, but not for its shortcuts:

vite/packages/vite/src/node/preview.ts

Lines 260 to 268 in b80daa7

	if (options.open) {
	const url = server.resolvedUrls?.local[0] ?? server.resolvedUrls?.network[0]
	if (url) {
	const path =
	typeof options.open === 'string' ? new URL(options.open, url).href : url
	openBrowser(path, true, logger)
	}
	}

[sapphi-red]

@bluwy Both handles resolvedUrls the same way (passes openBrowser without new URL()). Are you suggesting to do url = new URL(url).href?

[Sun79]

@bluwy Thank you for pointing that out!

I’ve checked the part of the code you mentioned, and the URL obtained from server.resolvedUrls?.local[0] ?? server.resolvedUrls?.network[0] is already URL-encoded. So, I think there is no need to use new URL() in this case.

[bluwy]

You're right I got it mixed up. I was suggesting adding:

const path = 
       typeof server.config.preview.open === 'string' ? new URL(server.config.preview.open, url).href : url 

It's a different bug now that I look at it. We can do in a separate PR