Closed Bug 375314 Opened 13 years ago Closed 12 years ago

discriminate ping and xmlhttprequests in content policy check

Categories

(Core :: Image Blocking, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: chpe, Assigned: gaubugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch proposed patch (obsolete) — Splinter Review
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 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.
Attachment #259617 - Flags: review?(jst)
Attached patch updated patch (obsolete) — Splinter Review
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 ] ?
Attachment #259617 - Attachment is obsolete: true
Attachment #261606 - Flags: review?(jst)
Attachment #259617 - Flags: review?(jst)
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 on attachment 261606 [details] [diff] [review]
updated patch

r- based on Wladimirs comments above.
Attachment #261606 - Flags: review?(jst) → review-
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.
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.
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.
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.
Ok, makes sense. Then you do want to block subrequests in the nodataprotocol policy
Attached patch Patch v2 and mochitest testcase (obsolete) — Splinter Review
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.
Assignee: nobody → trev.moz
Attachment #261606 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #264625 - Flags: review?(jst)
Comment on attachment 264625 [details] [diff] [review]
Patch v2 and mochitest testcase

r=jst
Attachment #264625 - Flags: review?(jst) → review+
Attachment #264625 - Flags: superreview?(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.
Attachment #264625 - Flags: superreview?(bzbarsky) → superreview?(peterv)
Attachment #264625 - Flags: superreview?(peterv) → superreview+
Whiteboard: [checkin needed]
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.
Attachment #264625 - Attachment is obsolete: true
Attachment #268676 - Flags: superreview+
Attachment #268676 - Flags: review+
Checked in the new patch. Clearing checkin-needed status.
Whiteboard: [checkin needed]
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.
(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.
Is there still something to do here, or could this be marked as FIXED?
No, as far as I know this bug is fixed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: abp
Depends on: 478528
You need to log in before you can comment on or make changes to this bug.