Allow disabling public - Githubissues

franciscop / server

:desktop_computer: Simple and powerful server for Node.js

https://serverjs.io/

MIT License

3.56k stars 171 forks source link

Allow disabling public #121

Closed reed1 closed 4 years ago

reed1 commented 4 years ago

Describe the bug For now public option defaults to 'public'. There's a section in the docs to disable public, but it doesn't work as intended:

public: false will produce error on starting server (UnhandledPromiseRejectionWarning: TypeError: root path required)
public: '' will actually makes the root directory of application all public, which is really dangerous

To Reproduce Steps to reproduce the behavior:

install server npm i server
setup simple index.js with {public: false} or {public: ''} as the server option
the bug will reproduce as written above

Expected behavior

public: false and public: '' should not give error and should not serve static files

CLAassistant commented 4 years ago

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

franciscop commented 4 years ago

Hey thanks for the PR! Totally agreed, this looks really dangerous. Couple of suggestions:

Use if(){} for more clarity. Can use if (!options.public) return; as well, as a guard clause.
Add a test for the empty string case. You can just copy+paste the one you added and change it to ''.

I'm going to evaluate it ASAP and might merge it without those changes because at a glance this seems very important.

reed1 commented 4 years ago

Ah this only intended to give you an idea, you can just close this and make changes as needed. Thanks for the framework btw

franciscop commented 4 years ago

Merging it for now, will improve it locally.

franciscop commented 4 years ago

Thanks for the PR!

franciscop commented 4 years ago

Published server@1.0.20 🎉