278773 - Various toolkit code incorrectly constructs URIs

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Unless you're implementing a protocol, creating an nsIURI object by
instantiating an nsStandardURL and setting the spec is almost certainly the
wrong thing to do.  This will lead to bugs for various URIs, ranging from data:
and javascript: to jar: (and we don't really want to break signed jars any more
than we have to, please).  Especially troubling, to me, are the cases where the
incorrectly-created URI object is then used in security checks.  This could
cause real issues in both directions (checks failing when they should succeed,
and the other way around).

The URL in the url field shows a number of hits for the contractid in various
tookit files.  Spot-checks show that every single case I've looked at should be
using either nsIIOService.newURI or nsIIOService.newFileURI (the latter in
toolkit/mozapps/extensions/src/nsExtensionManager.js.in, for example).

It may be that the hits under toolkit/ don't belong in the toolkit product
(since none of the components seem relevant), but I can't tell for sure, so
filing this just in case and picking a random component.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bug 278772 is the equivalent Firefox bug.

Comment 2

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

Attachment: Patch v1

Comment 3

[Darin Fisher]

Comment on attachment 176378 [details] [diff] [review]
Patch v1

>Index: toolkit/mozapps/downloads/content/downloads.js

>     for (var i = 0; i < numXPInstallItems;) {
>       // Pretty Name
>       var displayName = aParams.GetString(i++);
>       
>       // URI
>+      var uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                          .getService(Components.interfaces.nsIIOService)
>+                          .newURI(aParams.GetString(i++), null, null);

I think it would be wise to move the getService call out of 
the body of the loop.  Otherwise, you are doing duplicate
work for each iteration.

looks good otherwise.  r=darin

Comment 4

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

Attachment: Revised (checked in)