Closed
Bug 949533
(csp-legacy-removal)
Opened 11 years ago
Closed 10 years ago
Remove non-standard CSP parser and X-Content-Security-Policy header support
Categories
(Core :: Security, enhancement)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: geekboy, Assigned: geekboy)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
geekboy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
geekboy
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
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 | ||
Updated•11 years ago
|
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #8408617 -
Flags: feedback?(grobinson)
Comment 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
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).
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
And for ease of review, I'll split the "delete this file" changes out into a separate patch.
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Apply this patch with the -changes patch (this patch is just file removals).
Attachment #8440872 -
Flags: review?(grobinson)
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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 15•10 years ago
|
||
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 16•10 years ago
|
||
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-
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
This should apply cleanly. Garrett?
Attachment #8440872 -
Attachment is obsolete: true
Attachment #8444684 -
Flags: review?(grobinson)
Updated•10 years ago
|
Attachment #8444684 -
Flags: review?(grobinson) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Added r=grobinson to commit message.
Attachment #8444684 -
Attachment is obsolete: true
Attachment #8446016 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f70027b9330b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3662513dd7
Target Milestone: --- → mozilla33
Comment 25•10 years ago
|
||
GREAT SUCCESS ! :D
Comment 26•10 years ago
|
||
ZOMG, amazing :)
Comment 27•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a3662513dd7
https://hg.mozilla.org/mozilla-central/rev/f70027b9330b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•