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

More secure defaults. #1004

Merged
merged 3 commits into from
Sep 6, 2017
Merged

More secure defaults. #1004

merged 3 commits into from
Sep 6, 2017

Conversation

qistoph
Copy link
Contributor

@qistoph qistoph commented Aug 25, 2017

Related to #950. More secure suggestion.

Without config, listen only on looback interface. In sample config
listen on any interface, but use an IP whitelist.

Copy link
Contributor

@roramirez roramirez left a comment

Choose a reason for hiding this comment

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

Could be add some tests?

@MichMich
Copy link
Collaborator

MichMich commented Aug 29, 2017

Maybe the comments should reflect what the default value is when no address is given?

@qistoph
Copy link
Contributor Author

qistoph commented Sep 1, 2017

@MichMich, something like this? dfcbfad

Without config, listen only on looback interface. In sample config
listen on any interface, but use an IP whitelist.

Related to MagicMirrorOrg#950
@@ -9,7 +9,11 @@
*/

var config = {
address: "localhost",
address: "", // Address to listen on, can be:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need keep the localhost value for config sample... and only add comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, sounds fine to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you update this in the PR?

@qistoph
Copy link
Contributor Author

qistoph commented Sep 4, 2017

I've been working on some test cases for these configuration options, but keep running into issues...

https://github.com/qistoph/MagicMirror/tree/issue950_2

It seems that starting the app from the global namespace of mocha, leaves traces that influence later test cases. So I've tried to run them in a sandbox, but that is starting to be quite some work...

https://github.com/qistoph/MagicMirror/tree/issue950_sandbox

Do you maybe have any suggestions @roramirez ?

As proposed in review
@MichMich MichMich merged commit e5ead9e into MagicMirrorOrg:develop Sep 6, 2017
@roramirez
Copy link
Contributor

@qistoph why not use as e2e tests?

@roramirez
Copy link
Contributor

I pushed the fix
It's need set a valid file for defaults if not app get a exception and the tests will be fail
qistoph#1

We need now fix the conflict with version tests.

:)

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

Successfully merging this pull request may close these issues.

3 participants