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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Oct 24, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/vite/src/node/server/openBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,7 @@ async function startBrowserProcess(
if (openedBrowser) {
// Try our best to reuse existing tab with AppleScript
await execAsync(
`osascript openChrome.applescript "${encodeURI(
url,
)}" "${openedBrowser}"`,
`osascript openChrome.applescript "${url}" "${openedBrowser}"`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
cwd: join(VITE_PACKAGE_DIR, 'bin'),
},
Expand Down