Update Redirect handling for CSP

RESOLVED FIXED in mozilla37

Status

()

Core
DOM: Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1094156#c35 we should refactor the redirect handling case for CSP and account for top-level redirects. In other words, we should whitelist TYPE_DOCUMENT.
(Assignee)

Updated

3 years ago
Blocks: 1094156, 493857
(Assignee)

Comment 1

3 years ago
Created attachment 8538112 [details] [diff] [review]
bug_1112782_csp_redirects.patch

Sid, as discussed on IRC, I refactored the internal CSP mapping of content types to directives. Do you think we should also account for TYPE_REFRESH, see also the regular shouldLoad case:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#140

Also, I simplified the way we query the loadingPrincipal. This simplification is totally valid since we added that assertion in LoadInfo:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadInfo.cpp#37

I verified that all testcases still work and also the e10s testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=1094156#c35 works now.

Did I forget something?
Attachment #8538112 - Flags: review?(sstamm)
Comment on attachment 8538112 [details] [diff] [review]
bug_1112782_csp_redirects.patch

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

TYPE_REFRESH: we should let that fall through to the default/error case.  We don't need to handle it explicitly in CSP_ContentTypeToDirective.

LoadInfo to the rescue!  Looks good, Chris.
Attachment #8538112 - Flags: review?(sstamm) → review+
(Assignee)

Comment 3

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f46d4d9d1dc
Target Milestone: --- → mozilla37
(Assignee)

Comment 4

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/d302447734ac
Assignee: nobody → mozilla

Comment 5

3 years ago
Comment on attachment 8538112 [details] [diff] [review]
bug_1112782_csp_redirects.patch

>   int16_t aDecision = nsIContentPolicy::ACCEPT;
...
>-  if (aDecision == 1)
>+  if (aDecision == true) {
Actually I don't think either of these are correct ;-)
https://hg.mozilla.org/mozilla-central/rev/1f46d4d9d1dc
https://hg.mozilla.org/mozilla-central/rev/d302447734ac
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Duplicate of this bug: 1113196

Updated

3 years ago
Duplicate of this bug: 1111339
You need to log in before you can comment on or make changes to this bug.