Closed Bug 991466 Opened 6 years ago Closed 6 years ago

CSP in C++: Remove specCompliant flag everywhere after removing the old parser

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: ckerschb, Assigned: geekboy)

References

Details

Attachments

(2 files, 1 obsolete file)

The new parser does not support the 'X-Content-Security-Policy' header, therefore we can remove the specCompliant flag everywhere, e.g.
> void appendPolicy(in AString policyString, in nsIURI selfURI,
>                    in boolean reportOnly, in boolean specCompliant);
Depends on: 925004
No longer depends on: 925004
Depends on: csp-legacy-removal
Blocks: 925004
Attached patch remove-specCompliant (obsolete) — Splinter Review
First whack at removing specCompliant.  This will probably suffer some bitrot depending on when some of the other removals (and test changes) land.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
This patch removes spec-compliant arguments from everywhere and updates the tests accordingly to use spec-compliant policies.  This applies to mozilla-central now that bug 949533 has landed.
Attachment #8433665 - Attachment is obsolete: true
Attachment #8449785 - Flags: review?(mozilla)
Comment on attachment 8449785 [details] [diff] [review]
remove-specCompliant

Review of attachment 8449785 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks good to me, but probably we also want to remove:
> security.csp.speccompliant
from all the tests since it's not needed anymore, see:
http://mxr.mozilla.org/mozilla-central/search?string=security.csp.speccompliant&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Please remove from testcases and flag me again!
Attachment #8449785 - Flags: review?(mozilla) → review-
As requested, this patch adds removal of that pref from the mochitests.  Applied with the previous patch, the tests still all pass.
Attachment #8449847 - Flags: review?(mozilla)
Comment on attachment 8449785 [details] [diff] [review]
remove-specCompliant

Reflagging this patch for review; the additional changes you requested are in the other patch on this bug and I don't think you requested changes to this patch.  Anything else to change?
Attachment #8449785 - Flags: review- → review?(mozilla)
Forgot to add: I put the test changes in a separate patch because it was very mechanical (pretty close to the same changes in each file) and there are > 30 files touched for that basic change.
Comment on attachment 8449847 [details] [diff] [review]
remove security.csp.speccompliant from mochitests

Review of attachment 8449847 [details] [diff] [review]:
-----------------------------------------------------------------

Exactly what I was looking for! Quick turnaround - I am impressed!
Attachment #8449847 - Flags: review?(mozilla) → review+
Comment on attachment 8449785 [details] [diff] [review]
remove-specCompliant

Review of attachment 8449785 [details] [diff] [review]:
-----------------------------------------------------------------

That one was ok before - since you incorporated the requested changes in a separate patch I can safely r+ this one!
Attachment #8449785 - Flags: review?(mozilla) → review+
Comment on attachment 8449785 [details] [diff] [review]
remove-specCompliant

For good measure, before landing I would also like jst to take a quick look at the changes to nsDocument.cpp (small changes, jst).
Attachment #8449785 - Flags: review?(jst)
Comment on attachment 8449785 [details] [diff] [review]
remove-specCompliant

Should we worry about extension compat issues here due to the nsIContentSecurityPolicy API change? If so, please add the appropriate keywords to this bug so the right folks have a chance to weigh in before we ship this.

r=jst
Attachment #8449785 - Flags: review?(jst) → review+
Thanks, jst!  I think we're ok with extension compat because the code *should* disregard the extra boolean argument (without error) from any js-callers of appendPolicy who pass it in.
Since inbound has been a bit fragile lately: https://tbpl.mozilla.org/?tree=Try&rev=842b8bf0883c
You need to log in before you can comment on or make changes to this bug.