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

fix: Compare only the URL path component to expected value when proxying HMR websocket (fix #53) #52

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

rhurmala
Copy link
Contributor

In versions 5.4.12 and 6.0.9 vite introduced several fixes to vulnerabilities in HMR websocket.

Amongst other changes, a query parameter token was added to the HMR websocket upgrade request.
This broke the HMR proxy feature invike-node. Socket connection was not established, and the browser request was left pending until timing out.

This PR suggests a backwards compatible approach to accommodate these changes.

Following steps were taken:

  • Edit devServerPlugin.ts
  • Edit vitedependencies to the minimum patch version with the aforementioned security fixes in all package.json -files
  • Run pnpm install && pnpm run build && pnpm run test and confirm successful run
  • Revert to original package.json -files and run pnpm run reset && pnpm install && pnpm run build && pnpm run test to confirm successful run with previously used vite versions

package.json modifications are not necessary to fix the issue, but are included for replicating the steps above

}

function isHMRProxyRequest(req: IncomingMessage) {
const url = new URL(`http://example.com${req.url}`)
Copy link
Member

Choose a reason for hiding this comment

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

URL constructor have a native (more robust) way to handle URL base. See https://developer.mozilla.org/en-US/docs/Web/API/URL/URL#parameters

Suggested change
const url = new URL(`http://example.com${req.url}`)
const url = new URL(req.url, "http://example.com")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check out this commit which switched to use the two argument constructor as you suggested.
The case where req.url === undefined is handled explicitly.
Semantics remain the same if one assumes VITE_HMR_PATH to always be a path starting with /.

@magne4000
Copy link
Member

@rhurmala Thanks for the PR!

@brillout Merge this at your conveniance, I'm not sure how everything is conflicting with your current refactoring.

@brillout
Copy link
Member

👍 It doesn't conflict so feel free to merge whenever you want. I recommend to edit the title of this PR to add the fix: prefix — when squash merging GitHub will suggest the PR title as commit message + (#52). (That's one reason why I like to squash merge PRs even if they have only one commit.)

@brillout
Copy link
Member

Also for appending (fix #53), for neat CHANGELOG.md entries.

@brillout
Copy link
Member

I don't have any major code refactoring going on for Vike extensions. (Only transforming them into real Vike extensions, which should't conflict with other changes.)

…ssing req.url explicitly as a non-match, whereas previously url.pathname would have implicitly resolved to '/'.
@rhurmala rhurmala changed the title Compare only the URL path component to expected value when proxying HMR websocket fix: Compare only the URL path component to expected value when proxying HMR websocket (#53) Jan 30, 2025
@rhurmala rhurmala changed the title fix: Compare only the URL path component to expected value when proxying HMR websocket (#53) fix: Compare only the URL path component to expected value when proxying HMR websocket (#52) Jan 30, 2025
@rhurmala rhurmala changed the title fix: Compare only the URL path component to expected value when proxying HMR websocket (#52) fix: Compare only the URL path component to expected value when proxying HMR websocket (fix #53) Jan 30, 2025
@rhurmala rhurmala requested a review from magne4000 January 30, 2025 16:26
@rhurmala
Copy link
Contributor Author

👍 It doesn't conflict so feel free to merge whenever you want. I recommend to edit the title of this PR to add the fix: prefix — when squash merging GitHub will suggest the PR title as commit message + (#52). (That's one reason why I like to squash merge PRs even if they have only one commit.)

I added the prefix and issue reference to the PR title.
I don't have a 'merge' button available however, so I'll just have to wait patiently. 😄

@magne4000 magne4000 merged commit 5ef5ee6 into vikejs:main Jan 30, 2025
2 checks passed
@magne4000
Copy link
Member

Released as 0.3.2 🚀

@brillout
Copy link
Member

This PR suggests a backwards compatible

I reverted the peer dep version change (a0a2b38). (It's a small breaking change. Personally I think it's fine for a 0.x package but some users expect rigorous semver.)

@magne4000 I went ahead and applied the optional peer dep trick: 4ab3b87.

@magne4000 @snake-py I've just seen that Vite exports searchForWorkspaceRoot() which seems quite exhaustive; we can copy its implementation for our eject endeavour. Or even better publish a standalone npm package search-workspace-root to share with Vite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants