Skip to content

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

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

Merged
petebacondarwin merged 1 commit into from
Feb 1, 2024
Merged

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

Show file tree
Hide file tree
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
10 changes: 10 additions & 0 deletions .changeset/smart-owls-jog.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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

@petebacondarwin petebacondarwin Feb 1, 2024

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export async function startPreviewServer({
accessTokenRef,
});

await waitForPortToBeAvailable(port, {
await waitForPortToBeAvailable(port, ip, {
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
Expand Down Expand Up @@ -295,7 +295,7 @@ export function usePreviewServer({
return;
}

waitForPortToBeAvailable(port, {
waitForPortToBeAvailable(port, ip, {
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
Expand Down Expand Up @@ -636,9 +636,13 @@ function createStreamHandler(
*/
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";
Copy link
Contributor Author

@Lekensteyn Lekensteyn Jan 31, 2024

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:

workers-sdk/packages/wrangler/src/dev/proxy.ts

Line 169 in 749fa3c

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 @@ export async function waitForPortToBeAvailable(
doReject(err);
}
});
server.listen(port, () =>
server.listen(port, host, () =>
terminator
.terminate()
.then(doResolve, () =>
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/user/user.ts
View file
Open in desktop
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