append frame-src config in test mode

[chbonser]

Addresses #151

Note this works in my case because my app has frame-src defined. From the failing tests it appears appendSourceList does not handle the case of a missing frame-src properly. Should appendSourceList get smarter or should this problem get solved via another approach?

[jelhan]

Thanks so a lot for working on this! Looks good to me. Haven't had the time to look at the failing test yet.

From the failing tests it appears appendSourceList does not handle the case of a missing frame-src properly. Should appendSourceList get smarter or should this problem get solved via another approach?

It should already be that smart:

ember-cli-content-security-policy/lib/utils.js

Lines 150 to 153 in 43527ed

	if (!Array.isArray(value)) {
	// initialize source list with an copy of default-src (see above)
	policyObject[directiveName] = policyObject['default-src'] ? policyObject['default-src'].slice() : [];
	}

[rwjblue]

@chbonser - Have you had any time to debug the failure here?

[jelhan]

@rwjblue I fear it's related to the buggy ember-cli-addon-tests setup. I working on the side on a replacement for that one but haven't had the time to finish it yet.

[jelhan]

Closed and reopened to trigger another Travis run after test suite was refactored in #154.

[jelhan]

Getting a way more helpful error in CI now:

  1) e2e: provides test support
       includes frame-src required by testem
         includes frame-src required by testem in CSP delivered by meta tag:
     AssertionError: expected 'default-src \'none\'; script-src \'self\' \'nonce-abcdefg\' localhost:49741 0.0.0.0:49741; font-src \'self\'; connect-src \'self\' ws://localhost:49741 ws://0.0.0.0:49741; img-src \'self\'; style-src \'self\'; media-src \'self\'; frame-src \'none\' \'self\';' to include 'frame-src \'self\';'
      at Context.<anonymous> (node-tests/e2e/test-support-test.js:232:38)

  2) e2e: provides test support
       includes frame-src required by testem
         includes frame-src required by testem in CSP delivered by HTTP header:
     AssertionError: expected 'default-src \'none\'; script-src \'self\' \'nonce-abcdefg\' localhost:49741 0.0.0.0:49741; font-src \'self\'; connect-src \'self\' ws://localhost:49741 ws://0.0.0.0:49741; img-src \'self\'; style-src \'self\'; media-src \'self\'; frame-src \'none\' \'self\';' to include 'frame-src \'self\';'
      at Context.<anonymous> (node-tests/e2e/test-support-test.js:244:30)
      at process._tickCallback (internal/process/next_tick.js:68:7)

The test tries to find the string frame-src 'self';. But the CSP includes frame-src 'none' 'self';'.

This highlights a bug in appendSourceList utility. It does not remove an existing 'none' keyword if applying an expression.

[jelhan]

Closing and reopening again after #156 has been merged. It should address the bug described in my last comment. Let's see.

[jelhan]

It's working. 👏

To avoid a regression I would prefer to have test coverage. There are already some tests coverage for frame-src in tests:

ember-cli-content-security-policy/node-tests/e2e/test-support-test.js

Lines 210 to 246 in 8c4067a

	describe('includes frame-src required by testem', function() {
	before(async function() {
	await setConfig(testProject, {
	delivery: ['header', 'meta'],
	reportOnly: false,
	});
	await testProject.startEmberServer({
	port: '49741',
	});
	});
	after(async function() {
	await testProject.stopEmberServer();
	await removeConfig(testProject);
	});
	it('includes frame-src required by testem in CSP delivered by meta tag', async function() {
	let testsIndexHtml = await testProject.readFile('dist/tests/index.html', 'utf8');
	let [,cspInTestsIndexHtml] = testsIndexHtml.match(CSP_META_TAG_REG_EXP);
	expect(cspInTestsIndexHtml).to.include("frame-src 'self';");
	});
	it('includes frame-src required by testem in CSP delivered by HTTP header', async function() {
	let responseForTests = await request({
	url: 'http://localhost:49741/tests',
	headers: {
	'Accept': 'text/html'
	}
	});
	let cspForTests = responseForTests.headers['content-security-policy'];
	expect(cspForTests).to.include("frame-src 'self';");
	});
	});

An additional test should be added that it does not remove existing frame-src directive but appends to it.

@chbonser: Do you have time to add such a test or should I take over? Totally understand if you don't have time or motivation to do so after two months.

[chbonser]

@jelhan I am buried in other things at the moment but I'll try to squeeze in time later this week to add the test you mentioned.

[jelhan]

Rebased and added a failing tests. Thanks a lot @chbonser for debugging and providing the original fix! I'm sorry that it took that long to get it in.

[chbonser]

Thanks @jelhan. Sorry I wasn't able to add the extra test coverage!