Closed
Bug 1453012
Opened 6 years ago
Closed 6 years ago
Need to block chrome URLS when we block about: URLs
Categories
(Firefox :: Enterprise Policies, enhancement)
Firefox
Enterprise Policies
Tracking
()
RESOLVED
FIXED
Firefox 60
People
(Reporter: mkaply, Assigned: mkaply)
References
Details
Attachments
(2 files)
3.35 KB,
patch
|
Felipe
:
feedback+
|
Details | Diff | Splinter Review |
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
We currently block about: URLS in the redirector, but these can be bypassed by typing the chrome URL directly in the URL bar (chrome://global/content/config.xul). We need some mechanism to block that as well. For 60, maybe we should just have the JS files for the various feature bail early if the policy is not enabled? While we think of a longer term solution? Affects about:config about:profiles about:support about:debugging about:telemetry
Assignee | ||
Comment 1•6 years ago
|
||
This is a Firefox 60 only fix for this problem because content policy changed.
Assignee | ||
Comment 2•6 years ago
|
||
Comment on attachment 8967104 [details] [diff] [review] Blocking chrome pages using content policy obviously I need to do something different with the "registerContentPolicy" Right now it's only in about:config. This is for feedback, not review
Attachment #8967104 -
Flags: review?(felipc) → feedback?(felipc)
Comment 3•6 years ago
|
||
Comment on attachment 8967104 [details] [diff] [review] Blocking chrome pages using content policy Review of attachment 8967104 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/enterprisepolicies/Policies.jsm @@ +848,5 @@ > + > +let ContentPolicy = { > + shouldLoad: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) { > + if (aContentLocation.scheme == "chrome") { > + if (aRequestOrigin && aRequestOrigin.spec == "chrome://browser/content/browser.xul") { I think you can replace checking the aRequestOrigin, by checking if aContentType is TYPE_DOCUMENT or TYPE_SUBDOCUMENT. (so that it means it's being attempted to loaded in a tab or an iframe). @@ +849,5 @@ > +let ContentPolicy = { > + shouldLoad: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) { > + if (aContentLocation.scheme == "chrome") { > + if (aRequestOrigin && aRequestOrigin.spec == "chrome://browser/content/browser.xul") { > + if ((!Services.policies.isAllowed("about:config") && No need to list all these options. I think that once at least one about: page is blocked, we can activate this content policy to block *all* chrome:// URLs. so your registerContentPolicy() entry point function could be called something like blockAllChromeURLs(). And make the ContentPolicy object name be more specific about its purpose.. And add at least a small comment about it @@ +891,5 @@ > + ContentPolicy.contractID, > + ContentPolicy); > + > + let cm = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager); > + cm.addCategoryEntry("content-policy", ContentPolicy.contractID, ContentPolicy.contractID, false, true); (I'm not an specialist on this, but it looks on par with other stuff in the tree)
Attachment #8967104 -
Flags: feedback?(felipc) → feedback+
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8967115 [details] Bug 1453012 - Block all chrome URLS if about: policy is active. https://reviewboard.mozilla.org/r/235764/#review241564 Code analysis found 4 defects in this patch: - 4 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/enterprisepolicies/EnterprisePolicies.js:470 (Diff revision 1) > } > > +// This policy blocks all chrome:// URLs from being loaded in the browser window. > +// It is only activated when an about: page is blocked. > +let ChromeURLBlockPolicy = { > + shouldLoad: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) { Error: Expected method shorthand. [eslint: object-shorthand] ::: browser/components/enterprisepolicies/EnterprisePolicies.js:478 (Diff revision 1) > + aRequestOrigin.spec == "chrome://browser/content/browser.xul") { > + return Ci.nsIContentPolicy.REJECT_REQUEST; > + } > + return Ci.nsIContentPolicy.ACCEPT; > + }, > + shouldProcess: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) { Error: Expected method shorthand. [eslint: object-shorthand] ::: browser/components/enterprisepolicies/EnterprisePolicies.js:483 (Diff revision 1) > + shouldProcess: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) { > + return Ci.nsIContentPolicy.ACCEPT; > + }, > + classDescription: "Policy Engine Content Policy", > + contractID: "@mozilla-org/policy-engine-content-policy-service;1", > + classID: Components.ID('{ba7b9118-cabc-4845-8b26-4215d2a59ed7}'), Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/enterprisepolicies/EnterprisePolicies.js:485 (Diff revision 1) > + }, > + classDescription: "Policy Engine Content Policy", > + contractID: "@mozilla-org/policy-engine-content-policy-service;1", > + classID: Components.ID('{ba7b9118-cabc-4845-8b26-4215d2a59ed7}'), > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPolicy]), > + createInstance: function(outer, iid) { Error: Expected method shorthand. [eslint: object-shorthand]
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8967115 [details] Bug 1453012 - Block all chrome URLS if about: policy is active. https://reviewboard.mozilla.org/r/235764/#review241600
Attachment #8967115 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 8967115 [details] Bug 1453012 - Block all chrome URLS if about: policy is active. Approval Request Comment [Feature/Bug causing the regression]: Enterprise Policy [User impact if declined]: User can bypass blocked pages [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Not in nightly [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: Non [Is the change risky?]: No [Why is the change risky/not risky?]: Adds new content policy only in cases where about: is blocked [String changes made/needed]: None This change is only going on beta right now because nsIContentPolicy was completely rewritten for 61, so any patch we do won't be close. So this patch is only intended as a stopgap on ESR 60.
Attachment #8967115 -
Flags: approval-mozilla-beta?
Comment 9•6 years ago
|
||
Comment on attachment 8967115 [details] Bug 1453012 - Block all chrome URLS if about: policy is active. enterprise policies fix, approved for 60.0b12
Attachment #8967115 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8f64d3c436b3
status-firefox60:
--- → fixed
Updated•6 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee | ||
Comment 12•6 years ago
|
||
No, it fell off my radar. I honestly wasn't terribly stressed because things are fine on the ESR and we're still getting RR to the point where it's fully usable for enterprise. I'll open a new bug and target for 62.
Flags: needinfo?(mozilla)
Updated•6 years ago
|
Comment 13•6 years ago
|
||
Anything left to do here in this bug or can we close it?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 14•6 years ago
|
||
We're all done here. Additional work was done in https://bugzilla.mozilla.org/show_bug.cgi?id=1471355 and it's already uplifted to 62.
Flags: needinfo?(mozilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•