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

[wrangler] fix: listen on loopback for wrangler dev port check and login #4830

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions .changeset/smart-owls-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"wrangler": patch
---

fix: listen on loopback for wrangler dev port check and login

Avoid listening on the wildcard address by default to reduce the attacker's
surface and avoid firewall prompts on macOS.

Relates to #4430.
12 changes: 7 additions & 5 deletions packages/wrangler/src/__tests__/dev.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -879,12 +879,12 @@ describe("wrangler dev", () => {
writeWranglerToml({
main: "index.js",
dev: {
ip: "1.2.3.4",
ip: "::1",
},
});
fs.writeFileSync("index.js", `export default {};`);
await runWrangler("dev");
expect((Dev as jest.Mock).mock.calls[0][0].initialIp).toEqual("1.2.3.4");
expect((Dev as jest.Mock).mock.calls[0][0].initialIp).toEqual("::1");
expect(std.out).toMatchInlineSnapshot(`""`);
expect(std.warn).toMatchInlineSnapshot(`""`);
expect(std.err).toMatchInlineSnapshot(`""`);
Expand All @@ -894,12 +894,14 @@ describe("wrangler dev", () => {
writeWranglerToml({
main: "index.js",
dev: {
ip: "1.2.3.4",
ip: "::1",
},
});
fs.writeFileSync("index.js", `export default {};`);
await runWrangler("dev --ip=5.6.7.8");
expect((Dev as jest.Mock).mock.calls[0][0].initialIp).toEqual("5.6.7.8");
await runWrangler("dev --ip=127.0.0.1");
expect((Dev as jest.Mock).mock.calls[0][0].initialIp).toEqual(
"127.0.0.1"
);
expect(std.out).toMatchInlineSnapshot(`""`);
expect(std.warn).toMatchInlineSnapshot(`""`);
expect(std.err).toMatchInlineSnapshot(`""`);
Expand Down
16 changes: 11 additions & 5 deletions packages/wrangler/src/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -626,12 +626,16 @@ export async function startApiDev(args: StartDevOptions) {
};
}
/**
* Get an available TCP port number.
*
* Avoiding calling `getPort()` multiple times by memoizing the first result.
*/
function memoizeGetPort(defaultPort?: number) {
function memoizeGetPort(defaultPort: number, host: string) {
let portValue: number;
return async () => {
return portValue || (portValue = await getPort({ port: defaultPort }));
// Check a specific host to avoid probing all local addresses.
portValue = portValue ?? (await getPort({ port: defaultPort, host: host }));
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT - this can also be:

Suggested change
portValue = portValue ?? (await getPort({ port: defaultPort, host: host }));
portValue = portValue ?? (await getPort({ port: defaultPort, host }));

return portValue;
};
}
/**
Expand Down Expand Up @@ -705,14 +709,16 @@ async function validateDevServerSettings(
);

const { zoneId, host, routes } = await getZoneIdHostAndRoutes(args, config);
const getLocalPort = memoizeGetPort(DEFAULT_LOCAL_PORT);
const getInspectorPort = memoizeGetPort(DEFAULT_INSPECTOR_PORT);
const initialIp = args.ip || config.dev.ip;
const initialIpListenCheck = initialIp === "*" ? "0.0.0.0" : initialIp;
const getLocalPort = memoizeGetPort(DEFAULT_LOCAL_PORT, initialIpListenCheck);
const getInspectorPort = memoizeGetPort(DEFAULT_INSPECTOR_PORT, "localhost");

// Our inspector proxy server will be binding to the result of
// `getInspectorPort`. If we attempted to bind workerd to the same inspector
// port, we'd get a port already in use error. Therefore, generate a new port
// for our runtime to bind its inspector service to.
const getRuntimeInspectorPort = memoizeGetPort();
const getRuntimeInspectorPort = memoizeGetPort(0, "localhost");

if (config.services && config.services.length > 0) {
logger.warn(
Expand Down
10 changes: 7 additions & 3 deletions packages/wrangler/src/dev/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@
accessTokenRef,
});

await waitForPortToBeAvailable(port, {
await waitForPortToBeAvailable(port, ip, {

Check warning on line 157 in packages/wrangler/src/dev/proxy.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/dev/proxy.ts#L157

Added line #L157 was not covered by tests
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
Expand Down Expand Up @@ -295,7 +295,7 @@
return;
}

waitForPortToBeAvailable(port, {
waitForPortToBeAvailable(port, ip, {

Check warning on line 298 in packages/wrangler/src/dev/proxy.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/dev/proxy.ts#L298

Added line #L298 was not covered by tests
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
Expand Down Expand Up @@ -636,9 +636,13 @@
*/
export async function waitForPortToBeAvailable(
port: number,
host: string,
options: { retryPeriod: number; timeout: number; abortSignal: AbortSignal }
): Promise<void> {
return new Promise((resolve, reject) => {
if (host === "*") {
host = "0.0.0.0";

Check warning on line 644 in packages/wrangler/src/dev/proxy.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/dev/proxy.ts#L644

Added line #L644 was not covered by tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how I can trigger this code path as making wrangler dev --remote --ip "*" work also needed this check in packages/wrangler/src/dev.tsx.

Based on the following existing code, I added the explicit check for * here just in case:

if (ip === "::" || ip === "*" || ip === "0.0.0.0") {

}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
options.abortSignal.addEventListener("abort", () => {
const abortError = new Error("waitForPortToBeAvailable() aborted");
Expand Down Expand Up @@ -686,7 +690,7 @@
doReject(err);
}
});
server.listen(port, () =>
server.listen(port, host, () =>

Check warning on line 693 in packages/wrangler/src/dev/proxy.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/dev/proxy.ts#L693

Added line #L693 was not covered by tests
terminator
.terminate()
.then(doResolve, () =>
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/user/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,7 @@ export async function login(
}
});

server.listen(8976);
server.listen(8976, "localhost");
});
if (props?.browser) {
logger.log(`Opening a link in your default browser: ${urlToOpen}`);
Expand Down
Loading