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(security): docsify serve should only listen on localhost #52

Closed
wants to merge 2 commits into from
Closed

Conversation

solymosi
Copy link

The local development server spawned by docsify serve listens on all interfaces by default. This might be a security concern when working on an insecure local network.

This PR restricts the server to localhost / 127.0.0.1.

@anikethsaha
Copy link
Member

Thanks for the PR 👍
I think it does fallback to the localhost by default.
Can you give some more info or any reference for this?

@anikethsaha
Copy link
Member

perhaps we can add an optional flag for the serve command where it will set this host and port if required. Need to discuss this a little

@solymosi
Copy link
Author

solymosi commented Nov 18, 2019

In my testing the server was listening on 0.0.0.0, at least on Windows.
I support the idea of an optional flag.

In any case, the console message should not be hard-coded to say localhost but instead the actual IP being listened on. Let me see if I can make these changes in the next few days.

@anikethsaha
Copy link
Member

In my testing the server was listening on 0.0.0.0, at least on Windows

Strange 😕 I uses windows too but never faced this. Will try to re-create it if possible.

In any case, the console message should not be hard-coded to say localhost but instead the actual IP being listened on. Let me see if I can make these changes in the next few days.

Yup, this need to be done. I would lean more towards the optional flag cause in general the default behavior of a CLI should be changed using flags.
Feel free to investigate and send a new PR. Or I guess you can rebase and squash this one too

@trusktr
Copy link
Member

trusktr commented Jan 20, 2020

The Node.js doc states it defaults to 0.0.0.0.

@solymosi Can you link to a document explaining what the security issue is? The Node.js doc, and the Wiki page it links to, both don't mention anything regarding security.

@trusktr
Copy link
Member

trusktr commented Jan 20, 2020

I don't really see any security concern here: https://serverfault.com/questions/78048/whats-the-difference-between-ip-address-0-0-0-0-and-127-0-0-1

Docsify is intended to be served to the public. The administrator any machine where Docsify is served will need to take care to restrict exposure of the site as they wish.

To that end, having an option to specify the host would be convenient (rather than a hard-coded value of 127.0.0.1). It can be a new host parameter to that function.

function (path, openInBrowser, port, livereloadPort, host = '0.0.0.0') {

or similar.

@trusktr
Copy link
Member

trusktr commented Jan 20, 2020

@solymosi Would you be willing to make that update?

@solymosi
Copy link
Author

solymosi commented Feb 4, 2020

I've made the requested changes. Also added an --ip option for docsify serve which defaults to 0.0.0.0.

@solymosi
Copy link
Author

solymosi commented Feb 4, 2020

@solymosi Can you link to a document explaining what the security issue is? The Node.js doc, and the Wiki page it links to, both don't mention anything regarding security.

I was initially made aware of the fact that Docsify actually listens on 0.0.0.0 instead of localhost when I received a vulnerability alert from our corporate network scanning service that I had an HTTP service running on my development machine that was exposing a .git folder. This turned out to be a Docsify instance that I was running locally while I was working on some documentation. This is why I mentioned this being a potential security concern, since it could lead to sensitive data exposure from a developer machine that is connected to an untrusted network.

I think displaying the actual listen host (i.e. 0.0.0.0 by default) and making it configurable is a good compromise, since it is a non-breaking change but it still lets more security-conscious users change the IP to 127.0.0.1 if they want.

@solymosi
Copy link
Author

Any updates on this?

@anikethsaha
Copy link
Member

Hey ! sorry for the delay, Lets do few things before merging it.

  • Docs update : added notes in the read (ignore if already done)
  • what do you think of making the default to localhost and then it can be override based on our choice using the flag --ip which is introduced in this PR

@solymosi
Copy link
Author

@anikethsaha Both done ✔️

@slikts
Copy link

slikts commented Mar 8, 2020

I use an external machine for development, so this change is an inconvenience with little to no security benefit.

@anikethsaha
Copy link
Member

It will be in for next release !

@atharva3010
Copy link

I need this feature to run docsify on a whitelisted hostname. Any updates?

@solymosi solymosi closed this by deleting the head repository Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants