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

eirslett · 2021-04-13T21:36:10Z

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

patak-dev · 2021-04-13T21:55:33Z

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

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 · 2021-04-14T07:03:40Z

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 · 2021-04-14T07:38:10Z

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

eirslett · 2021-04-14T08:35:03Z

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 · 2021-04-14T08:41:38Z

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

rschristian · 2021-04-15T01:49:52Z

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 · 2021-04-15T09:15:23Z

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 · 2021-04-15T09:40:47Z

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

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 · 2021-04-15T10:08:13Z

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 · 2021-04-15T10:54:52Z

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 · 2021-04-16T14:27:27Z

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 · 2021-04-16T14:37:55Z

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 · 2021-04-16T21:30:43Z

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 · 2021-04-17T17:50:53Z

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 · 2021-04-17T18:15:50Z

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 · 2021-04-17T22:22:47Z

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

Shinigami92 · 2021-04-19T17:05:55Z

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 · 2021-04-20T15:30:08Z

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.

The security fix for listening only on 127.0.0.1 by default seems like it broke tests on Windows. Try using 0.0.0.0 for tests, and see if that fixes the tests.

…n Windows

docs/config/index.md

nihalgonsalves · 2021-05-02T15:27:42Z

packages/vite/src/node/preview.ts

+  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
+  }


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' }

nihalgonsalves · 2021-05-02T15:27:55Z

packages/vite/src/node/preview.ts

+    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)}`)
+          })
+      )
+    }


This can be extracted too

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

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

Co-authored-by: Nihal Gonsalves <nihal@nihalgonsalves.com>

…xes the unit tests on Windows" This reverts commit 2fde214.

…ests" This reverts commit 491d0c0.

eirslett · 2021-05-03T22:25:41Z

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)

packages/vite/src/node/preview.ts

patak-dev · 2021-05-04T06:55:42Z

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 ::.

Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>

packages/vite/src/node/preview.ts

Shinigami92 · 2021-05-04T16:34:54Z

packages/vite/src/node/server/index.ts

+  } else {
+    hostname = options.host as string


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

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

The as string just hides the TypeScript safeness here ⚠️

eirslett · 2021-05-04T21:26:18Z

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.

patak-dev · 2021-05-05T04:13:32Z

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

Co-authored-by: antfu <hi@antfu.me>

lubomirblazekcz · 2021-05-11T10:03:33Z

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

Shinigami92 · 2021-05-11T10:25:07Z

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 · 2021-05-11T13:16:05Z

Issue here #3355

Co-authored-by: Nihal Gonsalves <nihal@nihalgonsalves.com> Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>

Co-authored-by: antfu <hi@antfu.me>

eirslett mentioned this pull request Apr 13, 2021

fix(server): Security fix: only listen to private network address by default #2865

Closed

Shinigami92 added p4-important Violate documented behavior or significantly improves performance (priority) security labels Apr 14, 2021

Shinigami92 previously approved these changes Apr 14, 2021

View reviewed changes

eirslett dismissed Shinigami92’s stale review via f2187ef April 16, 2021 14:26

eirslett force-pushed the bugfix-security-use-host-param-correctly branch from 75f68bb to 33a4792 Compare April 21, 2021 17:50

eirslett added 6 commits May 2, 2021 10:56

fix(server): Listen only to 127.0.0.1 by default

c61214c

fix(server): Adjust CLI output regarding which IPs the server listens to

1377b03

feat(cli): The --host flag can be run without arguments

a388e27

feat(cli): Tweak --host parameter: be even more explicit about defaults

af7b24d

fix(tests): listen on host 0.0.0.0 when running tests

491d0c0

The security fix for listening only on 127.0.0.1 by default seems like it broke tests on Windows. Try using 0.0.0.0 for tests, and see if that fixes the tests.

fix(tests): Try listening on :: to see if that fixes the unit tests o…

2fde214

…n Windows

nihalgonsalves reviewed May 2, 2021

View reviewed changes

eirslett and others added 4 commits May 3, 2021 22:56

improvement to docs

46cd6e9

Co-authored-by: Nihal Gonsalves <nihal@nihalgonsalves.com>

fix(tests): Revert "fix(tests): Try listening on :: to see if that fi…

c0fd438

…xes the unit tests on Windows" This reverts commit 2fde214.

fix(tests): Revert "fix(tests): listen on host 0.0.0.0 when running t…

7e4063e

…ests" This reverts commit 491d0c0.

fix(tests): Server config listen to :: by default in all tests

890e41c

antfu self-requested a review May 4, 2021 04:54

antfu requested changes May 4, 2021

View reviewed changes

packages/vite/src/node/preview.ts Outdated Show resolved Hide resolved

eirslett and others added 2 commits May 4, 2021 17:03

Update packages/vite/src/node/preview.ts

dec1334

Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>

fix(server): Use hostname undefined instead of 0.0.0.0 or ::

940f525

Shinigami92 reviewed May 4, 2021

View reviewed changes

patak-dev approved these changes May 4, 2021

View reviewed changes

antfu approved these changes May 5, 2021

View reviewed changes

antfu merged commit 1e604d5 into vitejs:main May 5, 2021

Shinigami92 mentioned this pull request May 5, 2021

refactor: rewrite after #2977 #3269

Merged

9 tasks

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

refactor: rewrite after #2977 (#3269)

d2b3ba1

Co-authored-by: antfu <hi@antfu.me>

antfu mentioned this pull request May 9, 2021

fix: disable dotfiles serving via /@fs/ by default #3298

Closed

9 tasks

This was referenced May 12, 2021

fix(server): hostname defaults to localhost, fix #3355 #3382

Closed

fix(server): hostname defaults to localhost, fix #3355 #3383

Merged

fi3ework pushed a commit to fi3ework/vite that referenced this pull request May 22, 2021

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

37717ab

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

refactor: rewrite after vitejs#2977 (vitejs#3269)

77a4332

Co-authored-by: antfu <hi@antfu.me>

mrkishi mentioned this pull request Feb 24, 2022

vite's localhost is now incompatible with node's #7075

Closed

7 tasks

bluwy mentioned this pull request Oct 13, 2023

chore: remove server.host option for e2e test #14606

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eirslett commented Apr 13, 2021

patak-dev commented Apr 13, 2021

Shinigami92 left a comment

Shinigami92 commented Apr 14, 2021

patak-dev commented Apr 14, 2021

eirslett commented Apr 14, 2021

Shinigami92 commented Apr 14, 2021 •

edited

Loading

rschristian commented Apr 15, 2021

patak-dev commented Apr 15, 2021

patak-dev commented Apr 15, 2021 •

edited

Loading

patak-dev commented Apr 15, 2021

antfu commented Apr 15, 2021

eirslett commented Apr 16, 2021 •

edited

Loading

patak-dev commented Apr 16, 2021

patak-dev commented Apr 16, 2021

eirslett commented Apr 17, 2021

patak-dev commented Apr 17, 2021

eirslett commented Apr 17, 2021

Shinigami92 commented Apr 19, 2021

eirslett commented Apr 20, 2021

nihalgonsalves May 2, 2021 •

edited

Loading

nihalgonsalves May 2, 2021

patak-dev May 2, 2021

nihalgonsalves May 2, 2021

eirslett commented May 3, 2021

patak-dev commented May 4, 2021

Shinigami92 May 4, 2021

Shinigami92 May 5, 2021

eirslett commented May 4, 2021

patak-dev commented May 5, 2021

lubomirblazekcz commented May 11, 2021

Shinigami92 commented May 11, 2021

lubomirblazekcz commented May 11, 2021

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

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

Conversation

eirslett commented Apr 13, 2021

patak-dev commented Apr 13, 2021

Shinigami92 left a comment

Choose a reason for hiding this comment

Shinigami92 commented Apr 14, 2021

patak-dev commented Apr 14, 2021

eirslett commented Apr 14, 2021

Shinigami92 commented Apr 14, 2021 • edited Loading

rschristian commented Apr 15, 2021

patak-dev commented Apr 15, 2021

patak-dev commented Apr 15, 2021 • edited Loading

patak-dev commented Apr 15, 2021

antfu commented Apr 15, 2021

eirslett commented Apr 16, 2021 • edited Loading

patak-dev commented Apr 16, 2021

patak-dev commented Apr 16, 2021

eirslett commented Apr 17, 2021

patak-dev commented Apr 17, 2021

eirslett commented Apr 17, 2021

Shinigami92 commented Apr 19, 2021

eirslett commented Apr 20, 2021

nihalgonsalves May 2, 2021 • edited Loading

Choose a reason for hiding this comment

nihalgonsalves May 2, 2021

Choose a reason for hiding this comment

patak-dev May 2, 2021

Choose a reason for hiding this comment

nihalgonsalves May 2, 2021

Choose a reason for hiding this comment

eirslett commented May 3, 2021

patak-dev commented May 4, 2021

Shinigami92 May 4, 2021

Choose a reason for hiding this comment

Shinigami92 May 5, 2021

Choose a reason for hiding this comment

eirslett commented May 4, 2021

patak-dev commented May 5, 2021

lubomirblazekcz commented May 11, 2021

Shinigami92 commented May 11, 2021

lubomirblazekcz commented May 11, 2021

Shinigami92 commented Apr 14, 2021 •

edited

Loading

patak-dev commented Apr 15, 2021 •

edited

Loading

eirslett commented Apr 16, 2021 •

edited

Loading

nihalgonsalves May 2, 2021 •

edited

Loading