Closed Bug 1453012 Opened 2 years ago Closed 2 years ago

Need to block chrome URLS when we block about: URLs

Categories

(Firefox :: Enterprise Policies, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed
firefox61 --- wontfix

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(2 files)

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
This is a Firefox 60 only fix for this problem because content policy changed.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8967104 - Flags: review?(felipc)
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 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 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 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+
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 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+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Did we ever sort anything out for 61+ here?
Flags: needinfo?(mozilla)
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)
Blocks: 1471355
Anything left to do here in this bug or can we close it?
Flags: needinfo?(mozilla)
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.