Bug 949533 (csp-legacy-removal)

Remove non-standard CSP parser and X-Content-Security-Policy header support

RESOLVED FIXED in mozilla33

Status

()

enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: geekboy, Assigned: geekboy)

Tracking

(Blocks 2 bugs)

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Once roll-out and adoption of CSP 1.0 has begun, we should remove support for the legacy implementation.  Ideally, we could do this at the same time as fixing bug 925004 (rewriting it in C++).

Deprecation warnings have been in the product since releasing CSP 1.0 (per the W3C spec), so it's time to start removing it.
Depends on: 952737
Summary: Remove (deprecate) non-standard CSP parser and X-Content-Security-Policy header support → Remove non-standard CSP parser and X-Content-Security-Policy header support
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
No longer depends on: CSP, csp-w3c-1.0, 952737
Depends on: 988616
Blocks: 994166
No longer blocks: 925004
Blocks: 991466
No longer blocks: 994166
Posted patch remove-x-support (obsolete) — Splinter Review
Well, here's a first whack.

Still needs to delete the x- header mochitests (requires bug 988616) and requires the new backend to land with reporting support.
Attachment #8408617 - Flags: feedback?(grobinson)
Comment on attachment 8408617 [details] [diff] [review]
remove-x-support

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

You should also remove the security.csp.speccompliant pref definition from browser/app/profile/firefox.js and mobile/android/app/mobile.js.

For the unit tests: this bug should have a test to remove all of the x- unit tests (once they've been split, so bug 988616 has to land first). Additionally, some tests will be removed completely because they test the behavior when sites serve both headers (e.g. test_dual_headers_warning.html).

::: content/base/src/contentSecurityPolicy.js
@@ +80,5 @@
>    /* shouldn't see this one */
>    csp._MAPPINGS[cp.TYPE_REFRESH]           =  null;
>  
>    /* categorized content types */
> +  /* NOTE: order in this array is important !!! */

Why? (include in comment)

@@ +85,1 @@
>    // These are the same in old and new CSP's so just use the new mappings.

Remove this comment
Attachment #8408617 - Flags: feedback?(grobinson) → feedback+
When we remove the x-header support, these files should go (see bug 988616 comment 35):
> browser/devtools/webconsole/test/test-bug-821877-csperrors.html
> browser/devtools/webconsole/test/test-bug-821877-csperrors.html^headers^
> browser/devtools/webconsole/test/browser_webconsole_bug_821877_csp_errors.js
> content/base/test/unit/test_csputils.js
> content/base/test/unit/test_bug558431.js
> browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js
> browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html
> browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html^headers^

And these test files should be updated to test CSP:
> dom/workers/test/csp_worker.js
> dom/workers/test/test_csp.html
> dom/workers/test/test_csp.html^headers^
> dom/workers/test/test_csp.js
> content/base/test/unit/test_cspreports.js
> browser/devtools/webconsole/test/test_bug_770099_violation.html
> browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js
> browser/devtools/webconsole/test/test_bug_770099_violation.html^headers^

We should consider porting test_csputils.js and test_csp_ignores_path.js to the new parser (though looks like a lot of that is already in TestCSPParser.cpp).
Now that bug 858787 has landed and the CSP tests have been split into xcsp and csp directories (thanks, Garrett!), nothing should require us keeping around X- header support (except the headers themselves).  We can now begin removing the legacy header support (but not yet the js implementation of CSP 1.0).
Depends on: 858787
Posted patch remove-x-support (obsolete) — Splinter Review
Added a bunch more test removal stuff to the patch (thanks grobinson for splitting the tests!) and pushed to try with new backend enabled (we should probably try it without the new backend too before long):
https://tbpl.mozilla.org/?tree=Try&rev=b52a8d35b36a
Attachment #8408617 - Attachment is obsolete: true
Looks like this b-c test broke:
browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js, possibly because the warning posted to the console does not equal the warning expected (maybe default port isn't printed out).

And potentially a B2G ICS Emulator crash in Mochitest 9, though that could be a random orange.  Retriggering.

After those fixes, I'll run it on try again and then flag for review.
And for ease of review, I'll split the "delete this file" changes out into a separate patch.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6)
> Looks like this b-c test broke:
> browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js,
> possibly because the warning posted to the console does not equal the
> warning expected (maybe default port isn't printed out).

Had to patch CSPUtils.jsm to avoid printing the default port when it's used (:80, for example) and had to update the new backend to use a different string for report-only violations by threading the report-only flag to the CSPReportSenderRunnable.  Pretty minor changes.

> And potentially a B2G ICS Emulator crash in Mochitest 9, though that could
> be a random orange.  Retriggering.

It was random.  New patches coming shortly.
Posted patch remove-x-support-changes (obsolete) — Splinter Review
Removing code paths for x-csp support (stop reading the header and stop using the non-spec-compliant bits).
Attachment #8440047 - Attachment is obsolete: true
Attachment #8440871 - Flags: review?(grobinson)
Posted patch remove-x-support-deletions (obsolete) — Splinter Review
Apply this patch with the -changes patch (this patch is just file removals).
Attachment #8440872 - Flags: review?(grobinson)
These new patches are on try, using the old backend (security.csp.newbackend.enable is set to false as on current mozilla-central).
https://tbpl.mozilla.org/?tree=Try&rev=5d3558e8ad27
Looks like I missed some xpcshell tests that assume default ports would be created by CSPSource.toString() (which this patch changes so they don't).  That's an easy fix in test_csputils.js and test_csp_ignores_path.js to just remove :443 and :80 from the "expected output" strings.

Like this:
-      var parsed = "http://foo.bar:21 https://ras.bar:443";
+      var parsed = "http://foo.bar:21 https://ras.bar";

Our new backend (nsCSP*.cpp) should not return the port with stringified sources when the port is equal to the default port.  Also, these two xpcshell tests are not run with the new backend since they test CSPUtils.jsm directly.

I'll update the patches with these two xpcshell test changes after Garrett's review.
Comment on attachment 8440871 [details] [diff] [review]
remove-x-support-changes

Hey Johnny: flagging you for review on the nsDocument.cpp changes.  Feel free to review other stuff too if you feel so inclined.
Attachment #8440871 - Flags: review?(jst)
Comment on attachment 8440871 [details] [diff] [review]
remove-x-support-changes

r=jst for the dom pieces here.
Attachment #8440871 - Flags: review?(jst) → review+
Comment on attachment 8440871 [details] [diff] [review]
remove-x-support-changes

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

You also need to remove the reference to the speccompliant pref from b2g/app/b2g.js. r+ with that fixed and the comments addressed.

::: content/base/src/CSPUtils.jsm
@@ +1249,5 @@
>      if (this.scheme)
>        s = s + this.scheme + "://";
>      if (this._host)
>        s = s + this._host;
> +    if (this.port && gIoService.getProtocolHandler(this.scheme).defaultPort != this.port)

I'm assuming this is for spec-compliance, because CSP 1.0 4.11 [0] says the report should use URI-reference from RFC 3986, 3.2.3 [1], which says the default port should be omitted. I also think this improves readability.

Just making a note here since the reason for this change was non-obvious.

[0] http://www.w3.org/TR/CSP/#report-uri
[1] http://www.ietf.org/rfc/rfc3986.txt

::: content/base/src/contentSecurityPolicy.js
@@ +105,4 @@
>    // TODO : EventSource will be here and also will use connect-src
>    // after we fix Bug 802872 - CSP should restrict EventSource using the connect-src
>    // directive. For background see Bug 667490 - EventSource should use the same
>    // nsIContentPolicy type as XHR (which is fixed)

Side note: we should remove this comment, bug 802872 has been resolved.

It's a little confusing: EventSource triggers Content Policy checks with type TYPE_DATAREQUEST, but that's an alias for TYPE_XMLHTTPREQUEST [0], so we use the same logic as for XHR and everything works.

We can remove this as part of this patch (might as well, IMO), or do it in a follow up.

[0] http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl#108

::: content/base/src/nsCSPContext.cpp
@@ +887,5 @@
>          nsString blockedDataChar16 = NS_ConvertUTF8toUTF16(blockedDataStr);
>          const char16_t* params[] = { mViolatedDirective.get(),
>                                       blockedDataChar16.get() };
> +        CSP_LogLocalizedStr(mReportOnly ? NS_LITERAL_STRING("CSPROViolationWithURI").get()
> +                                        : NS_LITERAL_STRING("CSPViolationWithURI").get(),

All of the changes to this file are equivalent to the patch for bug 1011058. Did you mean to include them here? I don't think these are appropriate to include in this patch, as they are unrelated to the description. Instead, we should just land bug 1011058 independently.
Attachment #8440871 - Flags: review?(grobinson) → review+
Comment on attachment 8440872 [details] [diff] [review]
remove-x-support-deletions

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

This patch doesn't apply cleanly:

1 out of 1 hunks FAILED -- saving rejects to file content/base/test/xcsp/test_CSP_frameancestors.html.rej
patch failed, unable to continue (try -v)

It's look pretty good, but I'm going to wait until it applies cleanly to review it again.
Attachment #8440872 - Flags: review?(grobinson) → review-
(In reply to Garrett Robinson [:grobinson] from comment #16)
> Comment on attachment 8440872 [details] [diff] [review]
> remove-x-support-deletions
> 
> Review of attachment 8440872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch doesn't apply cleanly:
> 
> 1 out of 1 hunks FAILED -- saving rejects to file
> content/base/test/xcsp/test_CSP_frameancestors.html.rej
> patch failed, unable to continue (try -v)

test_CSP_frameancestors.html changed, so the removal failed (file is different).  I'll update the patch, but basically the patch just removes that file.
(In reply to Garrett Robinson [:grobinson] from comment #15)
> Comment on attachment 8440871 [details] [diff] [review]
> 
> ::: content/base/src/CSPUtils.jsm
> I'm assuming this is for spec-compliance, because CSP 1.0 4.11 [0] says the
> report should use URI-reference from RFC 3986, 3.2.3 [1], which says the
> default port should be omitted. I also think this improves readability.
> 
> Just making a note here since the reason for this change was non-obvious.

I put a note in a comment so it's clearer in the code.  Nice explanation!

> ::: content/base/src/contentSecurityPolicy.js
> @@ +105,4 @@
> >    // TODO : EventSource will be here and also will use connect-src
...
> We can remove this as part of this patch (might as well, IMO), or do it in a
> follow up.

Good call!  Will do it in this bug.

> ::: content/base/src/nsCSPContext.cpp
> All of the changes to this file are equivalent to the patch for bug 1011058.
> Did you mean to include them here? I don't think these are appropriate to
> include in this patch, as they are unrelated to the description. Instead, we
> should just land bug 1011058 independently.

Ah, yeah, I spaced.  We need those changes for tests to succeed with the new backend enabled (see comment 8).  I'll remove those changes from this patch, though we'll have to fix the new backend before turning it on or the same tests will fail.
Fixed as per Garrett's most excellent comments and re-exported.  Carrying over r=jst,grobinson.
Attachment #8440871 - Attachment is obsolete: true
Attachment #8444682 - Flags: review+
Posted patch remove-x-support-deletions (obsolete) — Splinter Review
This should apply cleanly.  Garrett?
Attachment #8440872 - Attachment is obsolete: true
Attachment #8444684 - Flags: review?(grobinson)
Attachment #8444684 - Flags: review?(grobinson) → review+
Added r=grobinson to commit message.
Attachment #8444684 - Attachment is obsolete: true
Attachment #8446016 - Flags: review+
Duplicate of this bug: 952737
Blocks: 925004
No longer blocks: 952737
GREAT SUCCESS ! :D
ZOMG, amazing :)
Duplicate of this bug: 992382
You need to log in before you can comment on or make changes to this bug.