-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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: default host to localhost
instead of 127.0.0.1
#8543
Changes from 8 commits
ada4446
aa8d5ca
74e0279
aed60eb
3b08484
3eb8a99
dd7448b
7b88dff
67d22bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import type { | |
OutgoingHttpHeaders as HttpServerHeaders | ||
} from 'http' | ||
import type { ServerOptions as HttpsServerOptions } from 'https' | ||
import { promises as dns } from 'dns' | ||
import type { Connect } from 'types/connect' | ||
import { isObject } from './utils' | ||
import type { ProxyOptions } from './server/middlewares/proxy' | ||
|
@@ -184,9 +185,16 @@ export async function httpServerStart( | |
logger: Logger | ||
} | ||
): Promise<number> { | ||
return new Promise((resolve, reject) => { | ||
let { port, strictPort, host, logger } = serverOptions | ||
let { port, strictPort, host, logger } = serverOptions | ||
|
||
// This could be removed when Vite only supports Node 17+ because verbatim=true is default | ||
// https://github.com/nodejs/node/pull/39987 | ||
if (host === 'localhost') { | ||
const addr = await dns.lookup('localhost', { verbatim: true }) | ||
host = addr.address | ||
} | ||
Comment on lines
+190
to
+195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I confirmed this part with Node v18.3.0 + Windows. |
||
|
||
return new Promise((resolve, reject) => { | ||
const onError = (e: Error & { code?: string }) => { | ||
if (e.code === 'EADDRINUSE') { | ||
if (strictPort) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -726,28 +726,29 @@ export interface Hostname { | |
name: string | ||
} | ||
|
||
const wildcardHosts = new Set([ | ||
'0.0.0.0', | ||
'::', | ||
'0000:0000:0000:0000:0000:0000:0000:0000' | ||
]) | ||
|
||
export function resolveHostname( | ||
optionsHost: string | boolean | undefined | ||
): Hostname { | ||
let host: string | undefined | ||
if (optionsHost === undefined || optionsHost === false) { | ||
// Use a secure default | ||
host = '127.0.0.1' | ||
host = 'localhost' | ||
} else if (optionsHost === true) { | ||
// If passed --host in the CLI without arguments | ||
host = undefined // undefined typically means 0.0.0.0 or :: (listen on all IPs) | ||
} else { | ||
host = optionsHost | ||
} | ||
|
||
// Set host name to localhost when possible, unless the user explicitly asked for '127.0.0.1' | ||
// Set host name to localhost when possible | ||
const name = | ||
(optionsHost !== '127.0.0.1' && host === '127.0.0.1') || | ||
host === '0.0.0.0' || | ||
host === '::' || | ||
host === undefined | ||
? 'localhost' | ||
: host | ||
host === undefined || wildcardHosts.has(host) ? 'localhost' : host | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not safe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Maybe adding a warning that denotes ports might be overridden? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, a warning should be good enough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sapphi-red I was confused by the message, as you may know. As a Vite user, I would expect it to say "security" and be listed as a warning (not informational). Also while the "ports might be overridden" makes sense in the context of this thread, it lead me in a wrong direction. I know this already shipped, but a suggestion: The "more information" should include a way to permanently silence the warning. My 2c... Just that you know, anyone running Vite under Docker Compose will need to enable the Disclaimer. I don't claim to understand the whole change in play here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! This message is not intended as a security warning. The purpose of this message is to inform the user that a server other than Vite might respond when accessing the output address. Because it is a confusing behavior. I'll create a PR to improve this message. |
||
|
||
return { host, name } | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm using
dns.promises
sincedns/promises
is supported from Node 15+.https://nodejs.org/dist/latest-v16.x/docs/api/dns.html#dns-promises-api