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.

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

Jump to bottom

fix: default host to localhost instead of 127.0.0.1 #8543

Merged
merged 9 commits into from Jun 14, 2022
Merged

fix: default host to localhost instead of 127.0.0.1 #8543

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ada4446
fix: default host to `localhost` instead of `127.0.0.1`
sapphi-red Jun 11, 2022
aa8d5ca
fix: move dns.lookup
sapphi-red Jun 11, 2022
74e0279
fix: use `dns.promises`
sapphi-red Jun 11, 2022
aed60eb
fix: IPv6 printServerUrls
sapphi-red Jun 11, 2022
3b08484
docs: update for default host change
sapphi-red Jun 11, 2022
3eb8a99
refactor: remove unused await
sapphi-red Jun 11, 2022
dd7448b
test: fix flaky test
sapphi-red Jun 11, 2022
7b88dff
refactor: apply suggestion
sapphi-red Jun 12, 2022
67d22bd
chore: add message for wildcard hosts
sapphi-red Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 23 additions & 2 deletions packages/vite/src/node/__tests__/utils.spec.ts
View file
Open in desktop
Expand Up @@ -49,9 +49,9 @@ describe('injectQuery', () => {
})

describe('resolveHostname', () => {
test('defaults to 127.0.0.1', () => {
test('defaults to localhost', () => {
expect(resolveHostname(undefined)).toEqual({
host: '127.0.0.1',
host: 'localhost',
name: 'localhost'
})
})
Expand All @@ -62,6 +62,27 @@ describe('resolveHostname', () => {
name: 'localhost'
})
})

test('accepts 0.0.0.0', () => {
expect(resolveHostname('0.0.0.0')).toEqual({
host: '0.0.0.0',
name: 'localhost'
})
})

test('accepts ::', () => {
expect(resolveHostname('::')).toEqual({
host: '::',
name: 'localhost'
})
})

test('accepts 0000:0000:0000:0000:0000:0000:0000:0000', () => {
expect(resolveHostname('0000:0000:0000:0000:0000:0000:0000:0000')).toEqual({
host: '0000:0000:0000:0000:0000:0000:0000:0000',
name: 'localhost'
})
})
})

test('ts import of file with .js extension', () => {
Expand Down
12 changes: 10 additions & 2 deletions packages/vite/src/node/http.ts
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ import type {
OutgoingHttpHeaders as HttpServerHeaders
} from 'http'
import type { ServerOptions as HttpsServerOptions } from 'https'
import dns from 'dns/promises'
import type { Connect } from 'types/connect'
import { isObject } from './utils'
import type { ProxyOptions } from './server/middlewares/proxy'
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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) {
Expand Down
11 changes: 9 additions & 2 deletions packages/vite/src/node/logger.ts
View file
Open in desktop
Expand Up @@ -164,6 +164,13 @@ export function printCommonServerUrls(
}
}

const loopbackHosts = new Set<string | undefined>([
'localhost',
'127.0.0.1',
'::1',
'0000:0000:0000:0000:0000:0000:0000:0001'
])
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved

function printServerUrls(
hostname: Hostname,
protocol: string,
Expand All @@ -173,15 +180,15 @@ function printServerUrls(
): void {
const urls: Array<{ label: string; url: string }> = []

if (hostname.host === '127.0.0.1') {
if (loopbackHosts.has(hostname.host)) {
urls.push({
label: 'Local',
url: colors.cyan(
`${protocol}://${hostname.name}:${colors.bold(port)}${base}`
)
})

if (hostname.name !== '127.0.0.1') {
if (hostname.name === 'localhost') {
urls.push({
label: 'Network',
url: colors.dim(`use ${colors.white(colors.bold('--host'))} to expose`)
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/server/index.ts
View file
Open in desktop
Expand Up @@ -561,7 +561,7 @@ async function startServer(

const options = server.config.server
const port = inlinePort ?? options.port ?? 5173
const hostname = resolveHostname(options.host)
const hostname = await resolveHostname(options.host)

const protocol = options.https ? 'https' : 'http'
const info = server.config.logger.info
Expand Down
17 changes: 9 additions & 8 deletions packages/vite/src/node/utils.ts
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This is not safe.
When you listen to 0.0.0.0:3000 in Vite, it is still possible to create a server that listens to 127.0.0.1:3000, and localhost:3000 will resolve to that server instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!
Though I'm not sure how to fix this.
Opening http://0.0.0.0:3000 in browsers does not work and any other network ip's could also be overridden by other servers.

Maybe adding a warning that denotes ports might be overridden?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, a warning should be good enough

Copy link
Member Author

Choose a reason for hiding this comment

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

image
I implemented like this. (maybe the message text is not friendly... I didn't come up with a good message.)

Copy link
Contributor

Choose a reason for hiding this comment

The 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:
"You are using a wildcard host. This means a service listening to 127.0.0.1:port on your system may hijack your traffic. More information: ".

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 host: true and thus will see the message. Docker Compose port forwarding does not work without it.

Disclaimer. I don't claim to understand the whole change in play here.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 }
}
Expand Down