Closed Bug 999656 Opened 6 years ago Closed 5 years ago
CSP in CPP: Refactor mapping of frame-ancestors to TYPE-DOCUMENT
In the new CSP implementation we currently map frame-ancestors to TYPE-Document. We should refactor that part and come up with a better mapping.
Yep, we should clean this up before calling frame-ancestors "done".
Hey Chris: how do you like this direction? I still have to clean it up a bit and see if I can't factor more out, but the CSP tests pass with this patch.
Attachment #8531735 - Flags: feedback?(mozilla)
Attachment #8531735 - Attachment is patch: true
Comment on attachment 8531735 [details] [diff] [review] first whack Review of attachment 8531735 [details] [diff] [review]: ----------------------------------------------------------------- Wohoo - we should have used constants in the idl from the beginning instead of introducing enums. This really cleans things up quite a bit. Also, I like the way you cleaned up the permits-functions. Great work! Thanks! ::: dom/interfaces/security/nsIContentSecurityPolicy.idl @@ +23,1 @@ > [scriptable, uuid(dc86b261-5e41-4cab-ace3-a0278f5a7ec7)] Don't forget to update the uuid since function signatures changed. ::: dom/security/nsCSPContext.cpp @@ +179,5 @@ > nonce, > // aExtra is only non-null if > // the channel got redirected. > (aExtra != nullptr), > + false, nit: maybe add a comment after false // fall back to default-dir or something! ::: dom/security/nsCSPUtils.cpp @@ +146,5 @@ > + > + case nsIContentPolicy::TYPE_MEDIA: > + return nsIContentSecurityPolicy::MEDIA_SRC_DIRECTIVE; > + > + case nsIContentPolicy::TYPE_DOCUMENT: Even though we can/should leave TYPE_DOCUMENT here, that case statement should never get triggered for TYPE_DOCUMENT. See: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#142 I think we should keep it here anyway though. @@ +152,5 @@ > + return nsIContentSecurityPolicy::FRAME_SRC_DIRECTIVE; > + > + // BLock XSLT as script, see bug 910139 > + case nsIContentPolicy::TYPE_XSLT: > + return nsIContentSecurityPolicy::SCRIPT_SRC_DIRECTIVE; You could move the case statement up right after TYPE_SCRPT;
Attachment #8531735 - Flags: feedback?(mozilla) → feedback+
I pulled out the common code from the various places in nsCSPContext that loop through all the policies, and put that into a permitsInternal function. That way if we have bugs in policy-checks, it will be easy to track down a single place where it could happen. I think this is ready. Chris: what do you think?
Comment on attachment 8534417 [details] [diff] [review] bug999656-refactor-mappings Review of attachment 8534417 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot for cleaning that up. Really like to see those changes happening, especially moving all the directive constants into the *.idl. ::: dom/security/nsCSPUtils.cpp @@ +148,5 @@ > + > + case nsIContentPolicy::TYPE_MEDIA: > + return nsIContentSecurityPolicy::MEDIA_SRC_DIRECTIVE; > + > + // TYPE_DOCUMENT shouldn't be used since it's specifically whitelistedby Nit: missing whitespace between whitelisted by @@ +175,5 @@ > + // CSP can not block csp reports, fall through to error > + case nsIContentPolicy::TYPE_CSP_REPORT: > + // Fall through to error for all other directives > + default: > + NS_ASSERTION(false, "Can not map nsContentPolicyType to CSPDirective"); I think we should use a MOZ_ASSERT(false) in the default case. That should seriously not be allowed to work. ::: dom/security/nsCSPUtils.h @@ +54,5 @@ > #define STYLE_NONCE_VIOLATION_OBSERVER_TOPIC "Inline Style had invalid nonce" > #define SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC "Inline Script had invalid hash" > #define STYLE_HASH_VIOLATION_OBSERVER_TOPIC "Inline Style had invalid hash" > > +// these strings map to the CSPDirectives in nsIContentSecurityPolicy You have that nice "Note" comment in the *.idl, please also add a similar comment here tha t you have to add a new CSPDirective in the *.idl whenever you add a new directive to that array.
Attachment #8534417 - Flags: review?(mozilla) → review+
Thanks, Chris. Carrying over r=ckerschb.
Inbound is open and closed and I just can't time it right today so I'm going to add the checkin-needed keyword here.
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.