684746 - Data Manager doesn't support many content blocker permissions

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, enhancement)

Description

[[e-mail]]

Currently Gecko's content blocker supports the following permissions:
other		(Don't know)
script		 Load Scripts
image		 Load Images (already supported)
stylesheet	 Load Stylesheets
object		 Load Plug-ins
document	 Load Pages
subdocument	 Load Frames
refresh		 Refresh Pages
xbl		 Load XBL (if allowXULXBL is set)
ping		 Allow <a ping> requests
xmlhttprequest	 Allow XMLHttpRequests
objectsubrequest Allow Plug-ins to make requests
dtd		 Load DTDs
font		(Download fonts; not yet supported, see bug 684726)
media		(Play audio/video; not yet supported, see bug 684726)

So for instance you could block scripts from facebook.net or objects from youtube.com this way.

Note that the block applies to the location of the resource, rather than the location of the document, although you can apparently set the permission type to 3 which maps to same-domain only.

I'm not sure which of these would be useful to end users so I will attach proof of concept for scripts, stylesheets and plug-ins.

Comment 1

[[e-mail]]

Attachment: Proof of concept

Comment 2

[Robert Kaiser]

Comment on attachment 558332 [details] [diff] [review]
Proof of concept

I don't think this is a solution, it's just a hack that lets a few things look better. I don't like how inconsistent it looks to me at first glance, but I'll take more of a look when I have some free time (which unfortunately could be a long time to come, given that I'm flying off on Friday for slightly more than a week and then have another weekend flying off somewhere else - Mozilla makes me busy nowadays).

Comment 3

[H. Hofer]

So, is someone else allowed to review this?

It would be _very_ useful to have those detailed permission settings (esp. scripts and objects) in SeaMonkey!

I applied the the patch to trunk, and it works nicely. IMHO, the DM doesn't look more inconsistent with it than it does anyway ;-)

Comment 4

[Robert Kaiser]

(In reply to H. Hofer from comment #3)
> So, is someone else allowed to review this?

Me telling I'm not convinced of the approach is no reason for putting it on someone else. Also, it's not like those things would be complete unsupported right now. This is a minor change after all.

(In reply to H. Hofer from comment #3)
> IMHO, the DM doesn't
> look more inconsistent with it than it does anyway ;-)

Feel free to report additional bugs for inconsistencies you find - I think it's rather consistent in itself, but it surely has room for improvement.

Comment 5

[H. Hofer]

(In reply to Robert Kaiser (:[e-mail]) from comment #2 and comment #4)
> > which unfortunately could be a long time to come
> Me telling I'm not convinced of the approach is no reason for putting it on
> someone else.

It's not about being you being convinced - it's about your spare time...

> Also, it's not like those things would be complete unsupported
> right now. This is a minor change after all.

Well how _are_ they supported? I cannot see a possibility to specifically
block or allow scripts and objects from the DM UI.

Comment 6

[Robert Kaiser]

(In reply to H. Hofer from comment #5)
> Well how _are_ they supported? I cannot see a possibility to specifically
> block or allow scripts and objects from the DM UI.

From all I know, you can do that from other UI - and if not, then we never had any UI for it at all before. And DM can display all such permissions that are set once you have added them in some way. There are multiple ways we can enhance DM, and the patch here is a beginning, but as I said, it looks too hacky for me at a first glance, we need to do better. We should not allow this code to become hacky, we should extend it as cleanly and flexibly as possible.

Comment 7

[Robert Kaiser]

Comment on attachment 558332 [details] [diff] [review]
Proof of concept

>+      // Look for an nsContentBlocker permission
>+      var action = Services.prefs.getIntPref("permissions.default." + aType);
>+      // XXX Support 3 = same domain only
>+      if (action < 1 || action > 2)
>+	action = Services.perms.ALLOW_ACTION;
>+      return action;

So, I'm minusing this for two reasons: 1) We should implement the "same domain" stuff correctly right away (see how the cookie part has been done) and 2) I don't understand what this |if| is all about.

The rest of this approach looks reasonable to me after thinking about it some more.

Comment 8

[[e-mail]]

(In reply to Robert Kaiser from comment #7)
> 1) We should implement the "same domain" stuff correctly right away
I see what you mean now, we should have three radio buttons for those preferences, so that you can choose same-domain objects only.

> 2) I don't understand what this |if| is all about.
nsContentBlocker ignores values outside the range 1 to 3, defaulting to 1.

Comment 9

[[e-mail]]

Attachment: Proposed patch

With support for the Allow for Same Domain option.

Comment 10

[Robert Kaiser]

Comment on attachment 604690 [details] [diff] [review]
Proposed patch

>+richlistitem.permission[type="script"],
>+richlistitem.permission[type="image"],
>+richlistitem.permission[type="stylesheet"],
>[...]

Can we add a comment before this to explain why they're all in this different non-default group?

>+const SAME_DOMAIN = 3;

Bah, can't we point to some global place here just like we do with the others? :(


>-      let permTypes = ["allowXULXBL", "cookie", "geo", "image", "install",
>-                       "password", "popup"];
>+      let permTypes = ["allowXULXBL", "cookie", "geo",
>+                       "script", "image", "stylesheet", "object",
>+                       "install", "password", "popup"];

I had alphabetical order here, the new order sounds strange here...

>+      // Look for an nsContentBlocker permission
>+      var action = Services.prefs.getIntPref("permissions.default." + aType);
>+      if (action < Services.perms.ALLOW_ACTION || action > SAME_DOMAIN)
>+        action = Services.perms.ALLOW_ACTION;
>+      return action;

I don't like how we implicitly assume the values to be integers of certain values here but never clearly state that.
Can't we make this more similar to how we're doing it for the others? That could even be something like

if (defaultPref == 3)
  return SAME_DOMAIN;
if (defaultPref == 2)
  return Services.perms.DENY_ACTION;
return Services.perms.ALLOW_ACTION;

> <!ENTITY perm.Allow "Allow">
> <!ENTITY perm.AllowSession "Allow for Session">
> <!ENTITY perm.Block "Block">
>+<!ENTITY perm.AllowSameDomain "Allow for Same Domain">

Nit: put this before .Block so that it's grouped with .AllowSession


I'm going back and forth between + and - in my head, but I'm minusing this just because I'd like to see it again with those comments fixed, mainly for the default pref stuff.

(This patch just made me realize that we already can just block scripts or plugins for certain sites with just permission settings... How neat!)

Comment 11

[[e-mail]]

Attachment: Addressed review comments

Comment 12

[Robert Kaiser]

Comment on attachment 610840 [details] [diff] [review]
Addressed review comments

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

Thanks a lot for the patch!

::: suite/common/dataman/dataman.js
@@ +57,5 @@
>                                     "nsIIDNService");
>  
> +// From nsContentBlocker.cpp
> +const NOFOREIGN = 3;
> +

Bah, too bad it's not exposed in the original place in a way we can access it. :(

@@ +1332,5 @@
> +          return Services.perms.ALLOW_ACTION;
> +      }
> +    } catch (e) {
> +      return Services.perms.UNKNOWN_ACTION;
> +    }

Yes, I like that much better, thanks. :)

Comment 13

[[e-mail]]

Pushed changeset d9bc5710b9e2 to comm-central.