Closed Bug 1112782 Opened 5 years ago Closed 5 years ago

Update Redirect handling for CSP

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 1094156, CSP
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+
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 ;-)
You need to log in before you can comment on or make changes to this bug.