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

fix(server): Listen only to 127.0.0.1 by default #2977

Merged
merged 12 commits into from
May 5, 2021

Conversation

eirslett
Copy link
Contributor

This pull request supersedes #2865, after discussion that one should rely on the --host parameter to determine public/private network interface setup.

@patak-dev
Copy link
Member

Copying my comment from the prev PR for reference:
I think we should implement the same scheme as sirv: https://github.com/lukeed/sirv/tree/master/packages/sirv-cli#network-access

For security reasons, sirv-cli does not expose your server to the network by default. This means that your machine, and only your machine, will be able to access the localhost server.
If, however, your coworker wants to access the server from their computer, or you want to preview your work on a mobile device, you must use the --host flag. Only then will your server be accessible to other devices on the same network.
Using --host without a value is equivalent to --host 0.0.0.0, which is makes it discoverable publicly. You may customize this by passing a different value – but you probably don't need to!

@eirslett We should add an equivalent paragraph to the docs. And this PR doesn't implement support for --host without a value being equivalent to --host 0.0.0.0, no?
If the logic in preview.ts and server/index.ts is shared, we may want to take the opportunity to create a helper now that it is getting more complex. In the case of preview, it is not a big security issue but since sirv is doing this for its server, I agree that it would be good to be consistent.

@Shinigami92 Shinigami92 added p4-important Violate documented behavior or significantly improves performance (priority) security labels Apr 14, 2021
Shinigami92
Shinigami92 previously approved these changes Apr 14, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

suggestion (non-blocking): The only thing I see here is why do we have so huge duplicate code between server and preview? Can this be shared or e.g. re-called from the server impl?

@Shinigami92
Copy link
Member

Also it seems there are some failing tests 🤔
https://github.com/vitejs/vite/pull/2977/checks?check_run_id=2337683223#step:10:16

At least on windows 🤔 @patak-js Do you have windows? Can you reproduce this on your machine?

@patak-dev
Copy link
Member

@Shinigami92 I can reproduce this on my machine, but I don't understand how this is related to this PR. Head is green.

@eirslett
Copy link
Contributor Author

Yes, we should refactor it, my suggestion is that since this is a security fix, we could merge it (after code review) and do the other improvements in a separate PR. The code duplication should definitely be refactored, and supporting --host without arguments sounds like possibly a good idea.

My suspicion is that Windows tries to use the public IP in the tests, but by default the server now only listens to 127.0.0.1, which might be the reason why the tests fail?

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 14, 2021

@eirslett you should consider to create a PR/branch on top of eirslett:bugfix-security-use-host-param-correctly

@rschristian
Copy link
Contributor

And this PR doesn't implement support for --host without a value being equivalent to --host 0.0.0.0, no?

FWIW, Luke mentioned he only made that the default due to a few complaints about having to type more to expose. You certainly could make the argument that forcing users to be more explicit when exposing could be of benefit.

Just something to consider.

@patak-dev
Copy link
Member

Ok, I agree with @rschristian. We can have --host default to --host 0.0.0.0 if there are a lot of users asking for the shortcut, it is safer to avoid it.

@patak-dev
Copy link
Member

patak-dev commented Apr 15, 2021

About the hint message, I think that it could be less intrusive. I think we should copy sirv here:

image

It could be:

    Network:      Use `--host` to expose

Instead of add if we don't default to 0.0.0.0 in this PR. We shouldn't change the Network: and Local: tags.

@patak-dev
Copy link
Member

The failing example in windows is working well when launching the dev server, but not in the jest tests. This may be an issue with untilUpdated. Just in case, @eirslett would you merge main in the PR?

@antfu
Copy link
Member

antfu commented Apr 15, 2021

Agree to keep the Network: for hinting. My preference:

  > Local:    http://localhost:4000/
  > Network:  (use `--host` to expose)

Where the (...) is colored with gray or dim.

@eirslett
Copy link
Contributor Author

eirslett commented Apr 16, 2021

There we go:

  • CLI output looks like @antfu suggested
  • The --host parameter can be used without arguments

🤞 Let's hope that the tests pass...

@patak-dev
Copy link
Member

It is still failing in Windows, but it should be because of our tests and not because of the feature itself. @eirslett maybe we can force to use the server.host option for test so we keep testing in the same environment as before this PR? Tests are working in head, so if we do that, this should also pass, no?

@patak-dev
Copy link
Member

Some findings about the failing tests:
Previous to this PR, we are passing undefined as the host by default ( httpServer.listen(port, undefined) )

  • This is equivalent to using httpServer.listen(port, '::'), I checked that the tests are passing in Windows passing this string.
  • If I force manually the use of httpServer.listen(port, '0.0.0.0'), the HMR tests in Windows fails.
  • Forcing manually the use of httpServer.listen(port, '127.0.0.1'), the HMR tests in Windows also fails.

I don't understand why using '0.0.0.0' or '127.0.0.1' doesn't work for windows. And I don't know if it is a good idea to force the tests to use '::' because we wouldn't be testing the default setup after this PR.

Other details:

  • I don't see in the PR, how is host being defined as '0.0.0.0' in case the value is omitted.
  • In the PR If the host is not '127.0.0.1', the cli output will not replace 127.0.0.1 with localhost properly.

@eirslett
Copy link
Contributor Author

I don't see in the PR, how is host being defined as '0.0.0.0' in case the value is omitted.

I think the host will then be undefined which, when passed to node.js/express, means it will listen on 0.0.0.0.

In the PR If the host is not '127.0.0.1', the cli output will not replace 127.0.0.1 with localhost properly.

I don't think it matters - the most valuable piece of information is the public IP address, which you can then pass on to coworkers, or your mobile phone, or whatever should access your dev server remotely.

@patak-dev
Copy link
Member

I don't see in the PR, how is host being defined as '0.0.0.0' in case the value is omitted.

I think the host will then be undefined which, when passed to node.js/express, means it will listen on 0.0.0.0.

If host is undefined, then hostname is going to be '127.0.0.1' with this PR, no?

const hostname = options.host || '127.0.0.1'

Note: Looks like if the system supports IPv6, passing undefined defaults to '::' instead of '0.0.0.0' (but sirv defaults to '0.0.0.0')

In the PR If the host is not '127.0.0.1', the cli output will not replace 127.0.0.1 with localhost properly.

I don't think it matters - the most valuable piece of information is the public IP address, which you can then pass on to coworkers, or your mobile phone, or whatever should access your dev server remotely.

Users may still want to use localhost and give the public IP to others at the same time, and this is how Vite works until this moment. Using --host after this PR should work the same way as it is now (so 127.0.0.1 should be replaced by localhost in the output)

@eirslett
Copy link
Contributor Author

I pushed a new change now, which makes the logic even more explicit.

@Shinigami92
Copy link
Member

Hey @eirslett, I just had a quick look over your PR

Some thoughts I had are:

  • Your are checking e.g. hostname === '127.0.0.1', would be 'loaclhost' also a valid value?
  • Is it possible to validate the users input that it is a valid IPv4 address?
  • Do you think IPv6 addresses could be also a valid input?

IPv6 support (if wanted) could be outsourced to another PR 🤔

@eirslett
Copy link
Contributor Author

Your are checking e.g. hostname === '127.0.0.1', would be 'loaclhost' also a valid value?

It's not really checking that, it's just setting the default value to 127.0.0.1.

Is it possible to validate the users input that it is a valid IPv4 address?

I'm sure it possible, but maybe that's a separate feature?

Do you think IPv6 addresses could be also a valid input?

Absolutely, there's not really anything in the way now for passing an IPv6 address as the --host parameter. This PR only sets the defaults. Anyways I think IPv6 is also a separate issue.

@eirslett eirslett force-pushed the bugfix-security-use-host-param-correctly branch from 75f68bb to 33a4792 Compare April 21, 2021 17:50
docs/config/index.md Outdated Show resolved Hide resolved
Comment on lines 44 to 53
let hostname: string
if (options.host === undefined) {
// Use a secure default
hostname = '127.0.0.1'
} else if (options.host === true) {
// The user probably passed --host in the CLI, without arguments
hostname = '0.0.0.0'
} else {
hostname = options.host as string
}
Copy link
Member

@nihalgonsalves nihalgonsalves May 2, 2021

Choose a reason for hiding this comment

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

Since this is reused in both files, let's extract it to a function:

const getHostname(hostnameOption: string | boolean | undefined): string {
  if (typeof hostnameOption === 'string') {
    return hostnameOption
  }

  // `--host` without parameters
  if (hostnameOption === true) {
    return '0.0.0.0'
  }

  // secure loopback-only default
  return '127.0.0.1'
}

Comment on lines +63 to +85
if (hostname === '127.0.0.1') {
const url = `${protocol}://localhost:${chalk.bold(port)}${base}`
logger.info(` > Local: ${chalk.cyan(url)}`)
logger.info(` > Network: ${chalk.dim('use `--host` to expose')}`)
} else {
const interfaces = os.networkInterfaces()
Object.keys(interfaces).forEach((key) =>
(interfaces[key] || [])
.filter((details) => details.family === 'IPv4')
.map((detail) => {
return {
type: detail.address.includes('127.0.0.1')
? 'Local: '
: 'Network: ',
host: detail.address
}
})
.forEach(({ type, host }) => {
const url = `${protocol}://${host}:${chalk.bold(port)}${base}`
logger.info(` > ${type} ${chalk.cyan(url)}`)
})
)
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be extracted too

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but I think it is better if we do these refactorings in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏼 Okay to leave this one out, but it would be good to extract the added code (previous comment)

@eirslett
Copy link
Contributor Author

eirslett commented May 3, 2021

Looks like all the tests pass now! I'm not 100 % sure about making host: true mean hostname: undefined, but it's certainly possible. I have no idea why :: vs 0.0.0.0 should make a practical difference (or why the tests fail on Windows because of it)

@antfu antfu self-requested a review May 4, 2021 04:54
packages/vite/src/node/preview.ts Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member

I'm not 100 % sure about making host: true mean hostname: undefined, but it's certainly possible. I have no idea why :: vs 0.0.0.0 should make a practical difference (or why the tests fail on Windows because of it)

@eirslett let's do this change, and we can then use host: true in the tests instead of ::.

packages/vite/src/node/preview.ts Show resolved Hide resolved
packages/vite/src/node/preview.ts Show resolved Hide resolved
packages/vite/src/node/preview.ts Show resolved Hide resolved
Comment on lines +541 to +542
} else {
hostname = options.host as string
Copy link
Member

Choose a reason for hiding this comment

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

question: Is this also the case if options.host is set to false?
Cause I see that host is of type string | boolean | undefined

Copy link
Member

Choose a reason for hiding this comment

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

@patak-js as I mentioned here, what is with the options.host being false case?

image

The as string just hides the TypeScript safeness here ⚠️

@eirslett
Copy link
Contributor Author

eirslett commented May 4, 2021

I mean, yes, the code in there should probably be refactored, prettified, extracted, nitpicked and all, but this is first and foremost a security patch, and it's already been 29 days since I opened the first PR. I'm getting a little bit tired of all the bike shedding, sorry. All I wanted to do was to make Vite more secure.

@antfu antfu merged commit 1e604d5 into vitejs:main May 5, 2021
@patak-dev
Copy link
Member

Thanks for the PR @eirslett. For later reference, this was not merged before because tests were not passing til yesterday.

@Shinigami92 Shinigami92 mentioned this pull request May 5, 2021
9 tasks
@eirslett eirslett deleted the bugfix-security-use-host-param-correctly branch May 5, 2021 09:35
antfu added a commit that referenced this pull request May 8, 2021
Co-authored-by: antfu <hi@antfu.me>
@lubomirblazekcz
Copy link
Contributor

If you use host: true the open option returns undefined - http://undefined:3000/

@Shinigami92
Copy link
Member

If you use host: true the open option returns undefined - http://undefined:3000/

@evromalarkey Could you open an issue or maybe directly a PR for that?

@lubomirblazekcz
Copy link
Contributor

Issue here #3355

fi3ework pushed a commit to fi3ework/vite that referenced this pull request May 22, 2021
Co-authored-by: Nihal Gonsalves <nihal@nihalgonsalves.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
fi3ework pushed a commit to fi3ework/vite that referenced this pull request May 22, 2021
Co-authored-by: antfu <hi@antfu.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants