375314 - discriminate ping and xmlhttprequests in content policy check

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: proposed patch

Bug 323924 added a content policy check for pings, and bug 371123 added a content policy check for XMLHttpRequest. However they both use nsIContentPolicy::TYPE_OTHER as 'content type', making it impossible for content policies to explicitly check for them.
I propose to add TYPE_PING and TYPE_XMLHTTPREQUEST.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 259617 [details] [diff] [review]
proposed patch

Not sure who should review this; I chose jst because he reviewed the patches in the bugs referenced in comment 0.

Comment 2

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

This patch also adds a dedicated type for loads by plugins, from bug 375435 comment 13 and 14.

I'm not sure whether I should add the new types to the check in the NoData content policy [http://lxr.mozilla.org/seamonkey/source/content/base/src/nsNoDataProtocolContentPolicy.cpp#58 ] ?

Comment 3

[Wladimir Palant]

If we are at it already, we should add TYPE_XBL as well, see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLService.cpp&rev=1.231#539

XForms also uses TYPE_OTHER (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/xforms/nsXFormsUtils.cpp&rev=1.96#1567) but I am not sure whether/how this should be changed.

As to nsNoDataProtocolContentPolicy - I am not sure why it uses a blacklist when it should be a whitelist. Maybe it was meant as a quick and safe patch for the branch. I would change the check into:

  if (aContentType != TYPE_DOCUMENT &&
      aContentType != TYPE_SUBDOCUMENT &&
      aContentType != TYPE_OBJECT) {

Object subrequests are always loaded by us unlike the objects themselves. So there is no reason why there should be an exception for them. nsNoDataProtocolContentPolicy goes back to bug 181860, CC'ing sicking to comment.

Finally, you are changing nsIContentPolicy interface - you should give it another uuid.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 261606 [details] [diff] [review]
updated patch

r- based on Wladimirs comments above.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm worried about the TYPE_OBJECT_SUBREQUEST change, how is that different from TYPE_OBJECT?

The nsNoDataProtocolContentPolicy does uses a list of types to let through because it was assumed that for all, but a few, types we don't want to allow external protocols. Seems like that is true given the new things you're adding to it, with exception of TYPE_OBJECT_SUBREQUEST maybe.

But i'm fine with using a list of stuff to block instead. It is a safer approach.

Comment 6

[Wladimir Palant]

TYPE_OBJECT is the object itself, e.g. the Java applet being executed. TYPE_OBJECT_SUBREQUEST (maybe there is a better name for it?) would be any additional data requested by the plugin through the browser. E.g. in case of Flash that could be XML files, images or additional SWF files, we don't know the real type because this data is processed by the plugin. I don't think these things should be mixed together.

As to nsNoDataProtocolContentPolicy - I think you got it the wrong way round (or I didn't get your comment). The current implementation lists the types where external protocols should be disallowed. I propose to have the opposite - list the types where we want to allow external protocols for whatever reasons, a more robust solution. The list should only contain TYPE_DOCUMENT, TYPE_SUBDOCUMENT, and because of bug 346167 we need TYPE_OBJECT as well.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, yeah, you're right about nsNoDataProtocolContentPolicy, I thought I checked before commenting, but apparently I didn't. I agree it should be reversed.

I'm not sure how I feel about TYPE_OBJECT_SUBREQUEST, do you have a specific usecase? In many cases the difference between TYPE_OBJECT_SUBREQUEST and TYPE_OBJECT will be an implementation detail in the plugin, or even in the flash file.

Comment 8

[Wladimir Palant]

The use case in bug 375435 was Yahoo Maps. It loads quite some data including XML files for ad display that Adblock Plus should be able to intercept. However, Adblock Plus still needs to distinguish between the actual Flash file and anything loaded by it. E.g. if it collapses space left from blocked objects, it needs to set display:none on the <object> tag if the object has been blocked. But it should not do the same when blocking a subrequest of the same object - the object itself hasn't been blocked and should stay visible.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, makes sense. Then you do want to block subrequests in the nodataprotocol policy

Comment 10

[Wladimir Palant]

Attachment: Patch v2 and mochitest testcase

Christian, I hope you don't mind me taking this bug - this needs to be done. I added TYPE_XBL for XBL binding requests, fixed nsNoDataProtocolContentPolicy and generated a new uuid for the nsIContentPolicy interface. There is also a mochitest testcase now that will create different types of requests and check whether they generate the correct content policy calls. Everything but TYPE_OTHER, TYPE_REFRESH and TYPE_OBJECT_SUBREQUEST is tested. Object subrequests cannot be tested because they would require a plugin (Flash?) to be installed on the test system.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 264625 [details] [diff] [review]
Patch v2 and mochitest testcase

r=jst

Comment 12

[Boris Zbarsky [:bzbarsky]]

I suggest having biesi sr this if you expect to get it into 1.9.  I certainly won't be able to do it in any sort of reasonable timeframe.

Comment 13

[Kenneth Herron]

Patch v2 doesn't apply cleanly content/xbl/src/nsXBLService.cpp. Wladimir, could you update it? If the change is trivial, it shouldn't need another round of reviews.

Comment 14

[Wladimir Palant]

Attachment: Patch to be checked in (bitrot eliminated)

Comment 15

[Kenneth Herron]

Checked in the new patch. Clearing checkin-needed status.

Comment 16

[Kenneth Herron]

This patch appears to have broken the camino tinderbox "pawn"; see http://tinderbox.mozilla.org/Camino/. It's complaining about "too many open files" while building dependencies, which sounds like a bug in the build tools. I'll leave this patch in unless someone thinks it should be backed out.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> This patch appears to have broken the camino tinderbox "pawn"; see
> http://tinderbox.mozilla.org/Camino/. It's complaining about "too many open
> files" while building dependencies, which sounds like a bug in the build tools.

It is indeed, the Camino tinderboxen have been known to have this problem. They usually fix themselves after a few cycles, or a clobber. See also bug 335506.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Is there still something to do here, or could this be marked as FIXED?

Comment 19

[Wladimir Palant]

No, as far as I know this bug is fixed.