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

Also minify CSPs (fixes #947) #975

Closed
wants to merge 6 commits into from
Closed

Also minify CSPs (fixes #947) #975

wants to merge 6 commits into from

Conversation

ENT8R
Copy link

@ENT8R ENT8R commented Oct 28, 2018

I hope everything is OK. (I didn't take a look into the whole codebase and don't know if this would be the right place to add it...)

@XhmikosR
Copy link
Collaborator

Please fix the test failures and rebase against upstream/master.

@ENT8R
Copy link
Author

ENT8R commented Mar 22, 2019

I fixed the indentation issues with fa17d5c
Github IDE screwed something up there... Sorry for that.

@XhmikosR
Copy link
Collaborator

XhmikosR commented Mar 22, 2019

Looks good now. One thing I don't know though because I haven't dug deep into the codebase myself either, is if we need to lowercase tags, their names and/or their values before the comparisons.

@alexlamsl
Copy link
Collaborator

HTTP headers are case insensitive, plus the follow two MDN links listed <meta> usage using different case schemes:
https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta

So I'd say we need to do a lower case comparison here. Also, can we get a test case or two for this new feature?

Just add a new section at the end of tests/minifier.js like so:

QUnit.test('minify Content-Security-Policy', function(assert) {
  assert.equal(minify(...), ...);
  ...
});

@ENT8R
Copy link
Author

ENT8R commented Mar 23, 2019

So now there are also some test cases and a case insensitive check with e066084

@alexlamsl alexlamsl closed this Mar 24, 2019
@alexlamsl alexlamsl reopened this Mar 24, 2019
@alexlamsl
Copy link
Collaborator

Changes LGTM − now if I can just figure out how to get the Cursed Integration job to start for this PR...

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 this pull request may close these issues.

3 participants