from Myk in bug 564059: Regarding URL detection, on the other hand, in conversations with Brian, we came to the conclusion that it would be better to require URLs to be explicitly flagged by wrapping them in a URL() constructor, despite the additional API complexity, as that eliminates the risk of an addon specifying content it retrieved from a third party that it thinks is HTML but which is parseable as a URL (whether accidentally or maliciously) and thus unexpectedly causes the URL to get loaded. The URL constructor would be provided by a very simple API and initially expose only a toString() method (and, of course, the ability to introspect instances to determine that they are instances of URL).

Comment on attachment 444194 [details] [diff] [review] v1 Or Atul!

Comment on attachment 444194 [details] [diff] [review] v1 >diff --git a/packages/jetpack-core/docs/url.md b/packages/jetpack-core/docs/url.md >+<code>url.**URL**(*url*)</code> >+ >+The URL constructor allows verification and stringification of URLs. >+Any API in the SDK which takes a URL will only accept objects of >+this type. The `url` property is a string to be converted into a URL. >+If `url` is not a valid URI, this constructor will throw >+an exception. Nit: although one can restringify the URL once one has objectified it via this constructor, isn't "stringification" kind of the opposite of what this constructor does? Perhaps it would be better to say that the constructor creates an object that represents a URL (verifying that the provided string is a valid URL in the process). Nit: `url` property -> `url` parameter Nit: To more clearly distinguish between the parameter and the return value of the constructor, I would call the `url` parameter something different, like `spec`, `src`, or `ref`. >+<code>url.**toFilename**(*url*)</code> ... >+<code>url.**fromFilename**(*path*)</code> It doesn't need to be addressed in this patch, but since we are converting this low-level module into a high-level module (i.e. one that is exposed to add-on developers rather than just the developers of other APIs), we should consider the ramifications of continuing to provide toFilename/fromFilename functionality in this module. Perhaps it would make sense to move these methods to a "file" module, since importing such a module clearly means you want access to files, whereas importing a "url" module doesn't suggest that. Requesting feedback from Brian on this question. >+<code>url.**resolve**(*base*, *relative*)</code> This also doesn't need to be addressed in this patch, but we should fold this method into the URL constructor, giving that constructor an optional `base` parameter (in the second position) whose value, if present, is used to resolve the `url` parameter in the first position. Implementing that is trivial, as the nsIIOService that the module uses to resolve URLs actually handles absolute vs. relative URL cases transparently, ignoring the value of the base parameter if the value of the spec parameter is absolute, so it is not necessary to pre-determine whether or not a URL is absolute before calling the resolver (although we do need to make sure that the base URL, if provided, is converted to an nsIURI before being passed to the resolver). >+<code>url.**parse**(*url*)</code> This doesn't need to be addressed in this patch either, but this method too should be folded into the URL implementation, such that calling the URL constructor gives you an object with the same properties provided by the object returned by this method. >diff --git a/packages/jetpack-core/lib/url.js b/packages/jetpack-core/lib/url.js >+exports.URL = function url_URL(urlString) { >+ let uri = newURI(urlString); >+ return { >+ toString: function url_URL_toString() uri.spec >+ }; >+} The constructor of the returned object should be the exported function, such that |test.assertEqual(url.URL("h:foo").constructor, url.URL)| passes, so APIs that accept URLs can test that arguments they receive are URL objects. It should be possible to implement this by simply setting the "constructor" property of the returned object to the url_URL function. Nit: Also, ideally, instead of "url_URL", the exported function should be called simply "URL", which looks a bit nicer when serialized. >diff --git a/packages/jetpack-core/tests/test-url.js b/packages/jetpack-core/tests/test-url.js Nit: a few more useful tests (which all pass against the current patch): let a = url.URL("h:foo"); let b = url.URL(a); test.assertEqual(b.toString(), "h:foo", "a URL can be initialized from another URL"); test.assert(a !== b, "a URL initialized from another URL is not the same object"); test.assert(a == "h:foo", "toString is implicit when a URL is compared to a string via =="); test.assert(a + "" === "h:foo", "toString is implicit when a URL is concatenated to a string"); (Most of these use "assert" in order to be very specific about what kind of comparison is being done, as ==/!= and ===/!== have different behaviors wrt. toString methods.) r=myk with the constructor issue addressed (nits as you see fit). Note to self: make sure to file bugs on refactoring and deprecation of resolve, parse, and perhaps toFilename/fromFilename.

(In reply to comment #3) > >+exports.URL = function url_URL(urlString) { > >+ let uri = newURI(urlString); > >+ return { > >+ toString: function url_URL_toString() uri.spec > >+ }; > >+} > > The constructor of the returned object should be the exported function, such > that |test.assertEqual(url.URL("h:foo").constructor, url.URL)| passes, so APIs > that accept URLs can test that arguments they receive are URL objects. It > should be possible to implement this by simply setting the "constructor" > property of the returned object to the url_URL function. Now that I think about it, shouldn't this be using api-utils.publicConstructor?

(In reply to comment #4) > Now that I think about it, shouldn't this be using api-utils.publicConstructor? I.e.: function URL(spec) { let uri = newURI(spec); this.toString = function URL_toString() uri.spec; } exports.URL = require("api-utils").publicConstructor(URL); Among other things, this makes it possible to use instanceof to check if an object is a URL, which is even better than comparing constructors.

* most of myk's comments addressed, including the folding in of parse/resolve into the URL constructor. * updated the tests and docs TODO: * file a bug about moving file stuff into it's own module * update the current url module consumers

updates all consumers and tests to use the new api.

Comment on attachment 450023 [details] [diff] [review] v2.1 >+<code>url.**URL**(*sourceURL*, *base*)</code> Nit: I would call sourceURL simply "source" (to make it simpler, and so we don't imply it's a URL rather than merely a string we're parsing into a URL). >+The optional `base` parameter is a string that if present, is used >+to resolve the `sourceURL` parameter. Nit: it would be useful to mention that `base` is for resolving relative URLs into absolute ones. >+Parses the URL `sourceURL` and returns an object with following properties: Nit: "Parses the URL `sourceURL` and returns an object with following properties:" -> "URL objects have the following properties:" >+<code>url.**toFilename**(*url*)</code> ... >+<code>url.**fromFilename**(*path*)</code> Note: regarding the file module (not this bug! just thinking aloud), perhaps what makes the most sense is for URL objects to have a "file" property that is null if the URL does not represent a local file and is a File object with a path property if it does represent a local file, so the way to get a file path from a URL would be: URL("...").file.path. And conversely, the file module would provide a File constructor, to which one can pass a local file path, that returns a File object with a url property, so one can do File("...").url to get the URL from the file path. Alternately, we could make URL objects also be File objects via object composition, but that could get tricky (f.e. dealing with the two different meanings of "path").

http://hg.mozilla.org/labs/jetpack-sdk/rev/a5bee1d8ce36 filed bug 571293 with your comments about the file-related apis.

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product. To filter bugmail related to this change, filter on the word "looptid".