Add config options 'domain' #871

barbogast · 2015-09-02T08:38:02Z

The value will be used when the backend has to generate urls which
point to the app. Using the hostname sent in the HTTP header is
dangerous since an attacker could send a request with a spoofed
HTTP header (HTTP Host header attack).

If the config is not set a loud warning is printed on startup and
the hostname of the HTTP header is used nevertheless.

barbogast · 2015-09-02T08:44:15Z

PR for #847

This has one problem though: It breaks livereload in its current implementation.

The problem is that the port of the server has to be appended to the new config option, so the value is for example http://localhost:3000. Then in the template the livereload port is appended: http://localhost:3000:35729.

Omitting the port would break the URLs generated by the backend such as the passwort reset link. I also thought about just using config.domain + config.port to build the url. But this would break on Heroku. On Heroku config.port is set to some arbitary port even though the app is reachable under port 80.

On possibility would be to use config.domain.split(':')[0] to extract the host for livereload.

simison · 2015-09-02T09:51:43Z

This works for the Livereload:

<script type="text/javascript">
  document.write('\x3Cscript type="text/javascript" src="//' + window.location.hostname + ':35729/livereload.js">\x3C/script>');
</script>

...then it doesn't matter if the thing is served at localhost, docker, staging... JS always picks up the current domain from the address bar.

barbogast · 2015-09-02T11:17:02Z

Jup, that should work.

Hmm... is the content of app.locals considered as API? Then changing app.locals.host would be a breaking change.

codydaig · 2015-09-13T23:44:06Z

config/config.js

+ */
+var validateDomainIsSet = function (config) {
+  if(!config.app.domain){
+    console.error(chalk.red('+ Important warning: config.domain is empty. To prevent HTTP Host header attacks please set config.domain.'));


To be consistent, I don't think this should be console.error, just console.log.

Right.
@Koblaid I also don't think we need to be completely verbose with the specific security attacks but just mention that for security reasons the domain should be explicitly

codydaig · 2015-09-14T15:17:01Z

What about: If a domain isn't set in the config, just default to req.hostname?

barbogast · 2015-09-16T08:08:30Z

Alright, I just pushed some changes. The domain is now retrieved via req.headers.host which includes the port and should also work behind a proxy.

AFAICS the remaining issue is the hard-coded port in development.js. Omitting the port in config.domain would break urls generated by the app since the port has to be included.

So we could:

We could remove the default value in development.js. That would trigger the 'domain is not set' warning
1. We could ommit the warning if NODE_ENV=development
We could add a new config variable config.defaultPort and use it in default.js for config.port and in develepment.js for config.domain

lirantal · 2015-09-19T07:41:07Z

modules/core/server/views/layout.server.view.html

@@ -56,7 +56,9 @@

  {% if livereload %}
  <!--Livereload script rendered -->
-  <script type="text/javascript" src="{{host}}:35729/livereload.js"></script>
+  <script type="text/javascript">
+    document.write('\x3Cscript type="text/javascript" src="//' + window.location.hostname + ':35729/livereload.js">\x3C/script>');


what's the reason for doing it like this? is that in order to take the "real" hostname from the URL rather than the config variable? If so, isn't there a cleaner way of doing it instead of document.write?

Hehe, I didn't really like the code either, but was too lazy to really check it. Would this be better?

var scriptTag = document.createElement('script'); scriptTag.setAttribute('type', 'text/javascript'); scriptTag.setAttribute('src','//' + window.location.hostname + ':35729/livereload.js'); document.head.appendChild(scriptTag);

Yes, that's much better if we actually need to change it like that.

is that in order to take the "real" hostname from the URL rather than the config variable?

Yep. If testing/doing development behind proxy (e.g. Nginx) setup inside Vagrant/Docker, livereload would not work.

Can we get this line changed to be better?

Yep.
@Koblaid we're still waiting for you to change that line to your recommendation at https://github.com/meanjs/mean/pull/871/files#r39917678

Just pushed

lirantal · 2015-09-19T07:44:21Z

@Koblaid about the port number issue, we already have a port number config option, can't we use it? for example:
domain: 'localhost' + port

or will that not solve the issue?

barbogast · 2015-09-19T08:07:42Z

This works on localhost but it will break if the app is running for example on Heroku. Heroku sets the PORT environment variable to some arbitary port, while the app is reachable with port 80 from the outside.

simison · 2015-09-19T16:32:13Z

@Koblaid that's true also for any proxy configuration, say Nginx/Phusion Passenger.

lirantal · 2015-10-02T17:10:33Z

@Koblaid see my latest comment for this PR: #871 (comment)

barbogast · 2015-10-10T11:49:05Z

@lirantal Oh, sorry, at first I didn't know that you were still waiting for an answer and than I was busy ;-)

Well, I don't know how it could work out of the box without hard-coding the domain in development mode. (By the way: the default port is also hard-coded in default.js, isn't that the same thing?)

We could drop the domain line in development.js but then the user would be greeted with the "domain config is empty" warning, until he sets the option.

Or we could separate the default port in default.js so we would have two port variables:

one under which the node instance is reachable
one under which the site is reachable from the outside

Than the first one could be used to start up the server and the second one could be used to dynamically generate the domain.

But I think that would only make things more complicated.

lirantal · 2015-10-15T16:03:24Z

@Koblaid see my latest comments on the code section to finalize this PR.
@ilanbiala do you consider this a breaking change due to the domain/livereload change?

barbogast · 2015-10-17T08:04:26Z

Alright, I hope its ok now. The livereload-tag is now generated as proposed, and the domain-config in development.js and test.js is set to a default value but overwritable by the environment variable.

I wasn't sure about including the port into the default value for the domain-config in develepment.js and test.js. Should I remove it? I think it's necessary for URLs generated from the config to be correct. But I can still remove it if you like.

lirantal · 2015-10-17T16:38:56Z

config/env/development.js

@@ -22,7 +22,8 @@ module.exports = {
    }
  },
  app: {
-    title: defaultEnvConfig.app.title + ' - Development Environment'
+    title: defaultEnvConfig.app.title + ' - Development Environment',
+    domain: process.env.DOMAIN || 'localhost:3000'


you can do here:

domain: process.env.DOMAIN || 'localhost:' + defaultEnvConfig.port;

I think it's better because this way you don't hard code the port number.

ilanbiala · 2015-10-17T16:41:24Z

Still needs to be rebased before we can even consider merging it.

lirantal · 2015-10-17T16:41:58Z

@Koblaid see my latest comments about the port and:

update the PR to solve the conflicts
squash

can you think of adding some test to check the domain configuration works or this is just plain config option that has no point in testing?

The value will be used when the backend has to generate urls which point to the app. Using the hostname sent in the HTTP header is dangerous since an attacker could send a request with a spoofed HTTP header (HTTP Host header attack). If the config is not set a loud warning is printed on startup and the hostname of the HTTP header is used nevertheless.

barbogast · 2015-10-26T07:03:19Z

@lirantal

Changed development.js according to your latest comment
rebased on master
squashed

ilanbiala · 2015-10-26T13:50:18Z

@Koblaid can you add any meaningful tests?

barbogast · 2015-10-26T14:38:22Z

Hmm... I didn't see any existing tests for the configuration. Are there any?

If not, what kind of tests do you have in mind? Checking if the configuration is correctly created for the different modes? Or that the created configuration acutally works for the different setups (with proxy, without proxy, ...)?

Anyway, I think creating a test infrastructure for the configuration should be a separate issue/PR.

ilanbiala · 2015-10-26T16:47:54Z

@Koblaid tests always come with the implementation, that way we don't forget about them. Add whatever tests you see as meaningful and we'll go from there.

mleanos · 2015-10-27T00:38:14Z

@Koblaid We've defined configuration tests here. Perhaps a test could be written that is similar to some of these?
https://github.com/meanjs/mean/blob/master/modules/core/tests/server/core.server.config.tests.js

codydaig · 2015-11-03T17:07:14Z

@Koblaid Any update on this?

mleanos · 2015-11-04T21:16:45Z

I think it makes sense to move this to the 0.5.0 Milestone since there hasn't been any movement on this for over a week.

Also, it seems that the Token Auth implementation might revolve around the domain/host options. We might just want to address this once we have a better idea on what Token Auth changes on the server-side.

codydaig · 2015-11-04T21:55:10Z

I second @mleanos that we should push this back.

lirantal · 2015-11-07T08:05:39Z

Sure let's move to 0.5.0 so it doesn't block anything (I don't see how it's related to the token auth though)

mleanos · 2015-11-12T21:17:17Z

@lirantal I thought the domain/host may be related because in the proposed PR for Token Auth ( #1040 ), there were some changes to the API base URL; perhaps this isn't really related, but wanted to point it out.

lirantal · 2015-12-27T09:34:14Z

@mleanos @codydaig can we discuss this again or?

mleanos · 2015-12-27T13:41:11Z

@lirantal Sure. I think the changes introduced in #1101 are related. Correct me if I'm wrong.

Is there anything in particular that you'd like to discuss?

lirantal · 2015-12-28T06:55:59Z

@Melanos right, and I think we need to somehow merge this PR with the changes made with #1101

lirantal · 2016-01-16T15:50:29Z

@Koblaid would you like to make the changes required here to accommodate for the livereload update?

codydaig · 2016-01-30T05:22:38Z

#1101 was merged in. What's left to do on this PR?

lirantal · 2016-01-30T23:36:06Z

@codydaig let's close it for now.
@Koblaid can re-open/comment in the future if required.

Generic DOMAI configuration environment variable, useful for setting links to an app in reset email templates, and other cases. Fixes meanjs#871 and meanjs#847

Generic DOMAI configuration environment variable, useful for setting links to an app in reset email templates, and other cases. Fixes #871 and #847

codydaig reviewed Sep 13, 2015
View reviewed changes

lirantal added the enhance:feature label Sep 15, 2015

lirantal added this to the 0.4.2 milestone Sep 15, 2015

lirantal self-assigned this Sep 15, 2015

lirantal reviewed Sep 19, 2015
View reviewed changes

lirantal reviewed Oct 17, 2015
View reviewed changes

barbogast force-pushed the dont-use-host-from-http-header branch from 4c53837 to 987dfd0 Compare October 26, 2015 07:01

codydaig added the issue:bug label Nov 6, 2015

lirantal modified the milestones: 0.5.0, 0.4.2 Nov 7, 2015

lirantal closed this Jan 30, 2016

lirantal mentioned this pull request Aug 31, 2016

Adds a generic DOMAIN configuration environment #1469

Merged

Add config options 'domain' #871

Add config options 'domain' #871

Conversation

barbogast commented Sep 2, 2015

barbogast commented Sep 2, 2015

simison commented Sep 2, 2015

barbogast commented Sep 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codydaig commented Sep 14, 2015

barbogast commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lirantal commented Sep 19, 2015

barbogast commented Sep 19, 2015

simison commented Sep 19, 2015

lirantal commented Oct 2, 2015

barbogast commented Oct 10, 2015

lirantal commented Oct 15, 2015

barbogast commented Oct 17, 2015

Choose a reason for hiding this comment

ilanbiala commented Oct 17, 2015

lirantal commented Oct 17, 2015

barbogast commented Oct 26, 2015

ilanbiala commented Oct 26, 2015

barbogast commented Oct 26, 2015

ilanbiala commented Oct 26, 2015

mleanos commented Oct 27, 2015

codydaig commented Nov 3, 2015

mleanos commented Nov 4, 2015

codydaig commented Nov 4, 2015

lirantal commented Nov 7, 2015

mleanos commented Nov 12, 2015

lirantal commented Dec 27, 2015

mleanos commented Dec 27, 2015

lirantal commented Dec 28, 2015

lirantal commented Jan 16, 2016

codydaig commented Jan 30, 2016

lirantal commented Jan 30, 2016