Common Pitfalls

  • Revision slug: Common_Pitfalls
  • Revision title: Common Pitfalls
  • Revision id: 198232
  • Created:
  • Creator: Bzbarsky
  • Is current revision? No
  • Comment /* Doing Incorrect Security Checks */

Revision Content

There are some common pitfalls that should be avoided when writing either extensions or core Mozilla code. These are the sort of things that super-review should be catching for code that goes into the tree; avoiding these to start with saves super-reviewers a lot of effort.


Creating URI objects incorrectly

In almost all cases, when creating a URI object you want to use the newURI method on the nsIIOService interface, like so:

JS:

 try {
   var ioServ = Components.classes["@mozilla.org/network/io-service;1"]
                          .getService(Components.interfaces.nsIIOService);
   var uriObj = ioServ.newURI(uriString, uriCharset, baseURI);
 } catch (e) {
   // May want to catch NS_ERROR_MALFORMED_URI for some applications
 }

C++:

 nsresult rv;
 nsCOMPtr<nsIIOService> ioServ =
   do_GetService("@mozilla.org/network/io-service;1");
 NS_ENSURE_SUCCESS(rv, rv);
 nsCOMPtr<nsIURI> uriObj;
 rv = ioServ->NewURI(uriString, uriCharset, baseURI,
                      getter_AddRefs(uriObj));
 if (NS_FAILED(rv)) {
   // May want to handle NS_ERROR_MALFORMED_URI for
   // some applications
   return rv;
 }

or, if the code can include nsNetUtil.h:

 nsCOMPtr<nsIURI> uriObj;
 nsresult rv = 
   NS_NewURI(getter_AddRefs(uriObj), uriString, uriCharset, baseURI);

In all cases the baseURI can be null if the uriString should be treated as an absolute URI and uriCharset can be null if there is no clear origin charset for the string (eg it's something the user typed in the URL bar).

The one exception to all of the above, that is the one case in which creating a specific URI class via createInstance is acceptable, is when one is implementing a protocol handler's newURI method. In that case you may not be able to use the I/O service because that service calls the newURI method on the appropriate protocol handler, which may be your own handler.

Doing Incorrect Security Checks For URI Loads

Before loading a URI, one of two security checks needs to be performed:

  1. If the URI is a string that will be loaded via passing the string to nsIWebNavigation::LoadURI, the one must call CheckLoadURIStrWithPrincipal and pass the principal that the URI originates from as the first argument and the URI string as the second argument.
  2. In all other cases, one should call CheckLoadURIWithPrincipal and pass the principal that the URI originates from as the first argument and an nsIURI object representing the URI as the second argument. If there is no nsIURI object on hand, it is secure to call CheckLoadURIStrWithPrincipal, but this will perform a more stringent security check than is strictly necessary.

All other security checks (including calls to CheckLoadURI and CheckLoadURIStr) are incorrect, because they do not properly capture the originating principal. Those methods should be removed.

Revision Source

<p>There are some common pitfalls that should be avoided when writing either extensions or core Mozilla code.  These are the sort of things that super-review should be catching for code that goes into the tree; avoiding these to start with saves super-reviewers a lot of effort.
</p><p><br>
</p>
<h3 name="Creating_URI_objects_incorrectly"> Creating URI objects incorrectly </h3>
<p>In almost all cases, when creating a URI object you want to use the <code>newURI</code> method on the <code>nsIIOService</code> interface, like so:
</p><p>JS:  
</p>
<pre class="eval"> try {
   var ioServ = Components.classes["@mozilla.org/network/io-service;1"]
                          .getService(Components.interfaces.nsIIOService);
   var uriObj = ioServ.newURI(uriString, uriCharset, baseURI);
 } catch (e) {
   // May want to catch NS_ERROR_MALFORMED_URI for some applications
 }
</pre>
<p>C++:
</p>
<pre class="eval"> nsresult rv;
 nsCOMPtr&lt;nsIIOService&gt; ioServ =
   do_GetService("@mozilla.org/network/io-service;1");
 NS_ENSURE_SUCCESS(rv, rv);
 nsCOMPtr&lt;nsIURI&gt; uriObj;
 rv = ioServ-&gt;NewURI(uriString, uriCharset, baseURI,
                      getter_AddRefs(uriObj));
 if (NS_FAILED(rv)) {
   // May want to handle NS_ERROR_MALFORMED_URI for
   // some applications
   return rv;
 }
</pre>
<p>or, if the code can include nsNetUtil.h:
</p>
<pre class="eval"> nsCOMPtr&lt;nsIURI&gt; uriObj;
 nsresult rv = 
   NS_NewURI(getter_AddRefs(uriObj), uriString, uriCharset, baseURI);
</pre>
<p>In all cases the <code>baseURI</code> can be null if the <code>uriString</code> should be treated as an absolute URI and <code>uriCharset</code> can be null if there is no clear origin charset for the string (eg it's something the user typed in the URL bar).
</p><p>The one exception to all of the above, that is the one case in which creating a specific URI class via <code>createInstance</code> is acceptable, is when one is implementing a protocol handler's <code>newURI</code> method.  In that case you may not be able to use the I/O service because that service calls the <code>newURI</code> method on the appropriate protocol handler, which may be your own handler.
</p>
<h3 name="Doing_Incorrect_Security_Checks_For_URI_Loads"> Doing Incorrect Security Checks For URI Loads </h3>
<p>Before loading a URI, one of two security checks needs to be performed:
</p>
<ol><li> If the URI is a string that will be loaded via passing the string to <code>nsIWebNavigation::LoadURI</code>, the one must call <code>CheckLoadURIStrWithPrincipal</code> and pass the principal that the URI originates from as the first argument and the URI string as the second argument.
</li><li> In all other cases, one should call <code>CheckLoadURIWithPrincipal</code> and pass the principal that the URI originates from as the first argument and an <code>nsIURI</code> object representing the URI as the second argument.  If there is no <code>nsIURI</code> object on hand, it is secure to call <code>CheckLoadURIStrWithPrincipal</code>, but this will perform a more stringent security check than is strictly necessary.
</li></ol>
<p>All other security checks (including calls to <code>CheckLoadURI</code> and <code>CheckLoadURIStr</code>) are incorrect, because they do not properly capture the originating principal.  Those methods should be removed.
</p>
Revert to this revision