Skip to content

Commit 948d5e6

Browse files
UlisesGasconbjohansebas
UlisesGascon
and
bjohansebas
authored
fix(proxy): match the HMR upgrade path exactly like the ws server (#5678)
* test(proxy-option): extend HMR upgrade coverage to URL variants of the configured path Adds trailing slash, case variations, percent-encoded path, and leading double slash to the existing HMR/proxy isolation tests so that all URL shapes recognized as the HMR upgrade are treated consistently. * fix(proxy): normalize HMR upgrade path comparison Tighten the HMR-vs-proxy upgrade dispatch so URL variants of the configured HMR path are consistently recognized as the HMR socket: - lowercase comparison (case-insensitive) - decode percent-encoding before comparison - strip trailing slashes - collapse leading multiple slashes so //foo is treated as /foo instead of being parsed as a scheme-relative URL - wrap URL parsing in try/catch so a malformed req.url does not raise an uncaught exception * fix(proxy): enforce exact HMR upgrade path matching for websocket connections * fix(proxy): improve HMR upgrade path handling and user proxy integration * ci: pin node 24 * fix(test): update Node.js version matrix and adjust test naming for clarity * fix(proxy): enhance HMR upgrade handling by improving WebSocket server management --------- Co-authored-by: Sebastian Beltran <[email protected]>
1 parent 93e8996 commit 948d5e6

3 files changed

Lines changed: 205 additions & 4 deletions

File tree

‎.github/workflows/nodejs.yml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,13 @@ jobs:
6464
run: npx commitlint --from ${{ github.event.pull_request.head.sha }}~${{ github.event.pull_request.commits }} --to ${{ github.event.pull_request.head.sha }} --verbose
6565

6666
test:
67-
name: Test - ${{ matrix.os }} - Node v${{ matrix.node-version }}, Webpack ${{ matrix.webpack-version }} (${{ matrix.shard }})
67+
name: Test - ${{ matrix.os }} - Node v${{ matrix.node-version == '24.15' && '24.x' || matrix.node-version }}, Webpack ${{ matrix.webpack-version }} (${{ matrix.shard }})
6868

6969
strategy:
7070
matrix:
7171
os: [ubuntu-latest, windows-latest, macos-latest]
72-
node-version: [18.x, 20.x, 22.x, 24.x]
72+
# 24.15 is pinned on purpose: Node 24.16 is broken for Puppeteer. Do not bump to 24.x.
73+
node-version: [18.x, 20.x, 22.x, 24.15]
7374
shard: ["1/4", "2/4", "3/4", "4/4"]
7475
webpack-version: [latest]
7576

‎lib/Server.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1867,8 +1867,16 @@ class Server {
18671867

18681868
/** @type {S} */
18691869
(this.server).on("upgrade", (req, socket, head) => {
1870-
if (hmrPath && req.url) {
1871-
const { pathname } = new URL(req.url, "http://0.0.0.0");
1870+
if (hmrPath && typeof req.url === "string") {
1871+
// Match the configured HMR path exactly the same way the underlying
1872+
// WebSocket server (`ws`) does in `WebSocketServer#shouldHandle`: a
1873+
// raw, case-sensitive comparison of the request target with the query
1874+
// string stripped. Any normalization here would classify URL variants
1875+
// (`//ws`, `/WS`, …) as the HMR socket even though `ws` refuses them.
1876+
// https://github.com/websockets/ws/blob/8.18.3/lib/websocket-server.js#L214
1877+
const queryIndex = req.url.indexOf("?");
1878+
const pathname =
1879+
queryIndex !== -1 ? req.url.slice(0, queryIndex) : req.url;
18721880
if (pathname === hmrPath) {
18731881
return;
18741882
}

‎test/server/proxy-option.test.js‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -859,6 +859,198 @@ describe("proxy option", () => {
859859
});
860860
});
861861

862+
describe("HMR upgrade dispatching to user proxies", () => {
863+
let server;
864+
let backend;
865+
let backendWss;
866+
let backendUpgradeCount;
867+
868+
// Start a backend WebSocket server (the user proxy target) and a dev-server
869+
// proxying everything to it, with the given dev-server options merged in.
870+
const setup = async (devServerOptions) => {
871+
backendUpgradeCount = 0;
872+
873+
backend = http.createServer();
874+
backendWss = new WebSocketServer({ server: backend });
875+
backendWss.on("connection", () => {
876+
backendUpgradeCount += 1;
877+
});
878+
879+
await new Promise((resolve) => {
880+
backend.listen(port5, resolve);
881+
});
882+
883+
server = new Server(
884+
{
885+
hot: true,
886+
allowedHosts: "all",
887+
proxy: [
888+
{
889+
context: "/",
890+
target: `http://localhost:${port5}`,
891+
ws: true,
892+
},
893+
],
894+
port: port3,
895+
...devServerOptions,
896+
},
897+
webpack(config),
898+
);
899+
900+
await server.start();
901+
};
902+
903+
const teardown = async () => {
904+
for (const client of backendWss.clients) {
905+
client.terminate();
906+
}
907+
backendWss.close();
908+
backend.closeAllConnections();
909+
await server.stop();
910+
await new Promise((resolve) => {
911+
backend.close(resolve);
912+
});
913+
};
914+
915+
// Open a WebSocket to `path` and report whether the dev-server completed the
916+
// handshake (`opened`) and whether the upgrade was forwarded to the backend
917+
// proxy (`forwarded`).
918+
const probe = async (path) => {
919+
const before = backendUpgradeCount;
920+
921+
const ws = new WebSocket(`ws://localhost:${port3}${path}`);
922+
923+
// Resolve as soon as the socket reaches a terminal state instead of
924+
// waiting a fixed delay: `open` means the handshake completed, `error`
925+
// means it was rejected. The timeout is only a fallback in case neither
926+
// event ever fires, so it can be generous without slowing the happy path.
927+
const opened = await new Promise((resolve) => {
928+
const timer = setTimeout(() => resolve(false), 2000);
929+
930+
ws.once("open", () => {
931+
clearTimeout(timer);
932+
resolve(true);
933+
});
934+
ws.once("error", () => {
935+
clearTimeout(timer);
936+
resolve(false);
937+
});
938+
});
939+
940+
try {
941+
ws.close();
942+
} catch {
943+
// ignore close errors on already-failed sockets
944+
}
945+
946+
return { opened, forwarded: backendUpgradeCount > before };
947+
};
948+
949+
// Behavior shared by every WebSocket server implementation: the HMR socket
950+
// is served locally and never forwarded, while any path the HMR server does
951+
// not own falls through to the user proxy. SockJS serves its transport under
952+
// `/<prefix>/<server>/<session>/websocket`, not the bare `/ws`.
953+
const serverTypes = [
954+
{ type: "ws", hmrPath: "/ws", nonHmrPath: "/not-hmr" },
955+
{
956+
type: "sockjs",
957+
hmrPath: "/ws/000/abcd1234/websocket",
958+
nonHmrPath: "/not-hmr",
959+
},
960+
];
961+
962+
for (const { type, hmrPath, nonHmrPath } of serverTypes) {
963+
describe(`with webSocketServerType: ${type}`, () => {
964+
beforeAll(() => setup({ webSocketServer: type }));
965+
966+
afterAll(teardown);
967+
968+
it("serves the HMR upgrade locally and does not forward it to the proxy", async () => {
969+
const { opened, forwarded } = await probe(hmrPath);
970+
971+
expect(opened).toBe(true);
972+
expect(forwarded).toBe(false);
973+
});
974+
975+
it("forwards a non-HMR upgrade to the user proxy", async () => {
976+
const { forwarded } = await probe(nonHmrPath);
977+
978+
expect(forwarded).toBe(true);
979+
});
980+
});
981+
}
982+
983+
// `ws`-specific: the dispatch compares the path exactly the same way
984+
// `WebSocketServer#shouldHandle` does, so only the configured path (query
985+
// stripped) is the HMR socket; every other variant is forwarded.
986+
describe("with the `ws` server, path matching is exact", () => {
987+
beforeAll(() => setup({ webSocketServer: "ws" }));
988+
989+
afterAll(teardown);
990+
991+
it.each([
992+
["exact path", "/ws"],
993+
["path with query string", "/ws?token=1"],
994+
])("treats %s (%s) as the HMR upgrade path", async (_label, path) => {
995+
const { forwarded } = await probe(path);
996+
997+
expect(forwarded).toBe(false);
998+
});
999+
1000+
it.each([
1001+
["leading double slash", "//ws"],
1002+
["trailing slash", "/ws/"],
1003+
["uppercase", "/WS"],
1004+
["mixed case", "/wS"],
1005+
["percent-encoded path", "/%77%73"],
1006+
])("forwards %s (%s) to the user proxy", async (_label, path) => {
1007+
const { forwarded } = await probe(path);
1008+
1009+
expect(forwarded).toBe(true);
1010+
});
1011+
});
1012+
1013+
// The HMR path is read from the configured `webSocketServer` options, not a
1014+
// hardcoded `/ws`.
1015+
describe("with a custom `ws` path", () => {
1016+
beforeAll(() =>
1017+
setup({
1018+
webSocketServer: { type: "ws", options: { path: "/custom-hmr" } },
1019+
}),
1020+
);
1021+
1022+
afterAll(teardown);
1023+
1024+
it("treats the configured path (/custom-hmr) as the HMR upgrade path", async () => {
1025+
const { forwarded } = await probe("/custom-hmr");
1026+
1027+
expect(forwarded).toBe(false);
1028+
});
1029+
1030+
it("forwards the default path (/ws) once it is no longer the HMR path", async () => {
1031+
const { forwarded } = await probe("/ws");
1032+
1033+
expect(forwarded).toBe(true);
1034+
});
1035+
});
1036+
1037+
// With no HMR server there is no socket to protect, so the filter never
1038+
// engages and even `/ws` is forwarded to the user proxy.
1039+
describe("without a webSocketServer", () => {
1040+
beforeAll(() =>
1041+
setup({ hot: false, liveReload: false, webSocketServer: false }),
1042+
);
1043+
1044+
afterAll(teardown);
1045+
1046+
it("forwards /ws to the user proxy because there is no HMR socket to protect", async () => {
1047+
const { forwarded } = await probe("/ws");
1048+
1049+
expect(forwarded).toBe(true);
1050+
});
1051+
});
1052+
});
1053+
8621054
describe("should supports http methods", () => {
8631055
let server;
8641056
let req;

0 commit comments

Comments
 (0)