Skip to content

Conversation

@ibash
Copy link
Contributor

@ibash ibash commented May 25, 2014

This fixes #42.
Platform.js has a bug where it was mis-detecting IE11 as Firefox, that has a PR here: bestiejs/platform.js#57

This includes code to detect IE11 correctly until the platform.js PR is merged in (alternatively, you can wait for that request to be released and bump the platform.js version).

This PR also includes code that fixes some typos with CSP test suite, headers are sent back lowercased so the test assert(res.header['X-Content-Security-Policy'] === undefined); would always return true.

ibash added 2 commits May 25, 2014 11:36
Headers are lower cased and the agent name was changed to the key of the AGENTS objects.
The sandbox directive was added to IE 10 / 11 when no security policy was specified.
This only sets the sandbox directive when it's explicitly defined.
Furthermore, platform.js was mis-detecting IE11 as Firefox.
This adds stop-gap detection of IE11 until this pull request
is merged into platform.js: bestiejs/platform.js#57

Fixes #42
@ibash
Copy link
Contributor Author

ibash commented May 26, 2014

It looks like I was mistaken about the user agent IE11 uses, going to correct.
Also, it looks like platform.js hasn't been upated on npm in 2 years. This means IE11 detecting will fail https://www.npmjs.org/package/platform.

This pull request will detect IE11 for you, or you can wait until platform.js is updated and support IE11 then.

@EvanHahn
Copy link
Member

I think it'd be good to merge this now to support IE11 ASAP and push a patch version, and then when/if platform is updated, we'll update again with another patch version. @evilpacket, your opinion?

@ibash
Copy link
Contributor Author

ibash commented May 27, 2014

@EvanHahn / @evilpacket -- the pull request I did on platform.js prompted them to update the npm repo. They now detect IE11 (but not IE11 masked as other browsers).

I bumped the version of platform.js listed in package.json so this no longer has IE 11 specific detection.

@EvanHahn EvanHahn closed this in 3780bf5 Jun 1, 2014
@EvanHahn
Copy link
Member

EvanHahn commented Jun 1, 2014

@ibash, I cleaned some of this up but I mostly copy-pasted your code into 3780bf5. Could you make sure I fixed things?

@ibash
Copy link
Contributor Author

ibash commented Jun 2, 2014

@EvanHahn The tests for IE/Safari are still broken.
Try this:

  1. Comment out the IE block in the switch/case: https://github.com/evilpacket/helmet/blob/master/lib/middleware/csp.js#L87
  2. Add this line to the test/csp.js before the assert on line 217, so you can see the headers: console.log(JSON.stringify(res.header)); -- here: https://github.com/evilpacket/helmet/blob/master/test/csp.js#L217
  3. Run npm test

Expected:
Tests should fail

Actual:
Tests pass

The problem is that the headers are actually returned lower case -- so the assert is asserting something that is always true.

Instead of:

                assert(res.header['X-WebKit-CSP'] === undefined);
                assert(res.header['Content-Security-Policy'] === undefined);
                assert(res.header['X-Content-Security-Policy'] === undefined);

You need:

                assert(res.header['x-webkit-csp'] === undefined);
                assert(res.header['content-security-policy'] === undefined);
                assert(res.header['x-content-security-policy'] === undefined);

The same applies for this test:

        it("doesn't set the header for IE " + version + " if sandbox isn't specified", function (done) {

Instead of:

                assert(res.header[ua.header] === undefined);

It needs to be:

                assert(res.header[ua.header.toLowerCase()] === undefined);

@ibash
Copy link
Contributor Author

ibash commented Jun 2, 2014

With those changes you'll see that the tests fail (as they should).

@ibash
Copy link
Contributor Author

ibash commented Jun 2, 2014

@EvanHahn Fixing the tests: #49 which show that the fixes aren't working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Impossible to disable sandbox for CSP header in IE

2 participants