Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Adding host configuration feature #1101

Merged
merged 1 commit into from
Dec 25, 2015
Merged

Adding host configuration feature #1101

merged 1 commit into from
Dec 25, 2015

Conversation

farajfarook
Copy link
Contributor

Adding the functionality of configuring the host to start the server.

By default set this to 127.0.0.1 as the best practice is to run the server locally and expose the port via a proxy like nginx.

@lirantal lirantal self-assigned this Dec 17, 2015
@lirantal lirantal added this to the 0.5.0 milestone Dec 17, 2015
@lirantal
Copy link
Member

Thanks @farajfarook!

Why not bind to 0.0.0.0 all ips?

Also, please make sure you follow up on our code commit guidelines: https://github.com/meanjs/mean/blob/master/CONTRIBUTING.md#commit-message-guidelines

@codydaig
Copy link
Member

@farajfarook I would agree with @lirantal that it should be bound to 0.0.0.0 by default. However, I'd also recommend putting this in the production env file, but commented out with a note saying binding it to 127.0.0.1 is much safer in production.

@farajfarook
Copy link
Contributor Author

@lirantal You are welcome, and thank you for the opportunity to contribute. I made the required changes to my commit according to the guidelines and as per @codydaig 's suggestion.

@@ -7,6 +7,7 @@ module.exports = {
certificate: './config/sslcerts/cert.pem'
},
port: process.env.PORT || 8443,
host: process.env.HOST || '127.0.0.1',
Copy link
Member

Choose a reason for hiding this comment

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

@farajfarook we should be consistent and set the production config to 0.0.0.0 too.
IMO it's not that much of a security risk because users usually clone on their own machine and set it up later to run on production usually using build tools.

More than that, nowadays people use docker (or other types of VMs) and it is required to bind to a non-loopback host/ip to make the app accessible.

@lirantal
Copy link
Member

@farajfarook I added another comment, once we're done and decided upon this please squash your commits to just one and we'll merge.

Thanks again for contributing!

Adding the functionality of configuring the host to bind the server. By
default this is set to 0.0.0.0.
@farajfarook
Copy link
Contributor Author

@lirantal Did the changes as per your comment and squashed the commits. 👍 Please merge.

@lirantal
Copy link
Member

thanks I'll review and merge later on.

@codydaig
Copy link
Member

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants