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

Throws in FastBoot if disabled or delivery does not include header #117

Closed
jelhan opened this issue Oct 10, 2019 · 1 comment · Fixed by #118
Closed

Throws in FastBoot if disabled or delivery does not include header #117

jelhan opened this issue Oct 10, 2019 · 1 comment · Fixed by #118

Comments

@jelhan
Copy link
Collaborator

jelhan commented Oct 10, 2019

The run-time configuration is only present if addon is enabled and delivery contains "header":

// provide configuration needed at run-time for FastBoot support (if needed)
// TODO: only inject if application uses FastBoot
// https://github.com/rwjblue/ember-cli-content-security-policy/issues/116
if (!this._config.enabled || !this._config.delivery.includes('header')) {
return {};
}

This is by design to prevent unnecessary bloat.

This run-time configuration is consumed by an instance initializer which is only present if the application depends on ember-cli-fastboot. It assert that the run-time configuration is present:

assert(
'Required configuration is available at run-time',
addonConfig.hasOwnProperty('reportOnly') && addonConfig.hasOwnProperty('policy')
);

If that assertion is stripped in a production build, the next line will throw with a TypeError cause config is undefined:

return config['ember-cli-content-security-policy'];

The instance initializer is present even if addon is disabled or delivery does not include "header". Since the run-time configuration is not available, it will throw.

This should be fixed by only including the instance initalizier in the build if required. This would also prevent us from pushing code to the consuming application that is not needed.

@jelhan
Copy link
Collaborator Author

jelhan commented Oct 10, 2019

This also shows an issue with the test coverage. There are tests that the CSP header is not set for these scenarios but that tests do not assert the FastBoot build was successful:

it('does not set CSP header if disabled', async function() {
await setConfig(app, { enabled: false });
await app.runEmberCommand('build');
await startServer();
let response = await request({
url: 'http://localhost:49742',
headers: {
'Accept': 'text/html'
},
});
expect(response.headers).to.not.include.key('content-security-policy-report-only');
});
it('does not set CSP header if delivery does not include header', async function() {
await setConfig(app, { delivery: ['meta'] });
await app.runEmberCommand('build');
await startServer();
let response = await request({
url: 'http://localhost:49742',
headers: {
'Accept': 'text/html'
},
});
expect(response.headers).to.not.include.key('content-security-policy-report-only');
});

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 a pull request may close this issue.

1 participant