Add helmetOptions to config

[madmod]

Allow disabling frameguard so that MM can be embedded in iframes by setting "frameguard" in config.js to false. It also allows for advanced configuration of frameguard as explained in the helmet docs.

Originally this was intended to only allow disabling frameguard, but at the suggestion of @roramirez I have modified it to allow setting any helmet options from config.js by specifying the helmetOptions parameter.

[roramirez]

Can be better using a options like electronOptions for this case to using with helmet parameters.

[madmod]

@roramirez Good point. I have changed it to be helmetOptions, allowing any kind of helmet configuration.

[roramirez]

Is missing Changelog, README documentation and if not set the helmetOption in the configuration file.

[nhubbard]

I can understand the changelog fix and docs, but why would we need to put this in the README?

[roramirez]

I mean on this part of README https://github.com/MichMich/MagicMirror#configuration

[nhubbard]

Just a quick comment on the implementation of helmetOptions.

[nhubbard]

I would not go for straight config.helmetOptions, but instead specify a helmet config with some sane defaults if the helmetOptions key is empty.

Something like this:

if(config.helmetOptions) {
    app.use(helmet(config.helmetOptions));
} else {
    app.use(helmet({
        dnsPrefetchControl: true,
        frameguard: false,
        hidePoweredBy: true,
        hsts: false,
        ieNoOpen: false,
        noSniff: true,
        xssFilter: true
    });
}

[roramirez]

it's look good for me

[cowboysdude]

If I'm thinking right here couldn't a user just add this to their module to block their module from being put in an iframe even if MM has frameguard disabled?

var frameguard = require('frameguard')
app.use(frameguard({ action: 'deny' }))

I'm not thinking they would or even see why they would but I suppose they could ...

What would that do to MM if they do that an a user has frameguard disabled?

[madmod]

@nhubbard helmet takes care of setting sane defaults for us, and I think allowing it to do so would be better since specifying the defaults in MM would be redundant. (Unless we want to override some of them by default in MM, but I don't think that is necessary.)

@cowboysdude The issue this PR solves is allowing MM to be shown in an iframe. My use for this feature is to show it on a Chromecast. It doesn't have any effect on MM modules using iframes.

[nhubbard]

@madmod I think we should put the defaults I proposed in. It's not that I disagree with you; the defaults used by helmet include some items that don't apply to us, like HSTS and Key Pinning.

[roramirez]

Well .. after thinking more calm this, we need keep the same behavior at today. If nothing setting for helmetOptions we not should be add be default values.

If we set the defaults values there possibility to get more troubles, some external modules can not work anymore ... and us trying to solve, apply fixes here and there.

So... if somebody want to include the values by default need a policy or plan for this. First of all, add a warning or notice by log where it said it the exists of the new configuration option in an the future will be apply the default values ...if not this is an open windows to get more troubles.

[MichMich]

I agree with @roramirez. I thing the default situation should be no change to the current configuration.

[MichMich]

Guys, what do I need to do with this PR? Will it be changed?

[nhubbard]

@MichMich Since the debate was settled, I think we can merge it.

[roramirez]

Still not ready to merge.

[MichMich]

@roramirez What needs to change before we can merge?

[roramirez]

@MichMich Need add the changelog, README and not set values by default if the configuration isn't set.

@madmod Are you want to add these changes?

[MichMich]

I'm going to close this one due to inactivity. Feel free to reopen.