Closed Bug 1291458 Opened 3 years ago Closed 3 years ago

Update documentation for nsILoadInfo

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 3 obsolete files)

Within Bug 1105556 we are setting the right loadingPrincipal for docshell loads where we changed the initially introduced concept that the loadingPrincipal can never be null.

We should update the documentation for loadingPrincipal within nsILoadInfo to reflect that behavior.
Assignee: nobody → ckerschb
Blocks: 1105556
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
Comment on attachment 8777111 [details] [diff] [review]
bug_1291458_update_documentation_loadingprincipal.patch

Tanvi, I think Jonas is not going to review those bits anymore, any chance you could have a look?
Attachment #8777111 - Flags: review?(jonas) → review?(tanvi)
Summary: Update documentation for loadingPrincipal within nsILoadinfo → Update documentation for nsILoadInfo
Comment on attachment 8777111 [details] [diff] [review]
bug_1291458_update_documentation_loadingprincipal.patch

I suppose we should use the documentation Jonas sent out last week. I will update that and flag you for review again.
Attachment #8777111 - Flags: review?(tanvi)
Priority: -- → P2
Tanvi, I updated all the documentation for loadingPrincipal, triggeringPrincipal and loadingNode/loadingDocument within:
+++ b/netwerk/base/NetUtil.jsm
+++ b/netwerk/base/nsIChannel.idl
+++ b/netwerk/base/nsIIOService.idl
+++ b/netwerk/base/nsIIOService2.idl
+++ b/netwerk/base/nsILoadInfo.idl
+++ b/netwerk/base/nsNetUtil.h
+++ b/netwerk/protocol/websocket/nsIWebSocketChannel.idl

The only thing that's missing is the triggeringPrincipal for toplevel loads. If we are going to change/modify behavior within Bug 1289097, then I would also update the documentation for triggeringPricnipal within that patch.

For now I suppose it's crucial that we land those documentation updates.
Attachment #8777111 - Attachment is obsolete: true
Attachment #8790198 - Flags: review?(tanvi)
Comment on attachment 8790198 [details] [diff] [review]
bug_1291458_update_documentation_for_loadinfo.patch

Breaking this into multiple comments.  Here is the first review on loadingNode and loadingPrincipal.

>diff --git a/netwerk/base/NetUtil.jsm b/netwerk/base/NetUtil.jsm
>--- a/netwerk/base/NetUtil.jsm
>+++ b/netwerk/base/NetUtil.jsm
>@@ -219,67 +219,73 @@ this.NetUtil = {
>      *        {
>      *          uri:
>      *            The full URI spec string, nsIURI or nsIFile to create the
>      *            channel for.
>      *            Note that this cannot be an nsIFile if you have to specify a
>      *            non-default charset or base URI.  Call NetUtil.newURI first if
>      *            you need to construct an URI using those options.
>      *          loadingNode:
>-     *            The loadingDocument of the channel.
>-     *            The element or document where the result of this request will
>-     *            be used.  This is the document/element that will get access to
>-     *            the result of this request.  For example for an image load,
>-     *            it's the document in which the image will be loaded.  And for
>-     *            a CSS stylesheet it's the document whose rendering will be
>-     *            affected by the stylesheet.
>-     *            If possible, pass in the element which is performing the load.
>-     *            But if the load is coming from a JS API (such as
>-     *            XMLHttpRequest) or if the load might be coalesced across
>-     *            multiple elements (such as for <img>) then pass in the
>-     *            Document node instead.

>-     *            For loads that are not related to any document, such as loads
>-     *            coming from addons or internal browser features, omit this
>-     *            property and specify a loadingPrincipal or
>-     *            loadUsingSystemPrincipal instead.
Should we add this last sentence back in?  Or just call out top level documents and workers, as you have done in the new version.  What about addons?

>+     *            This is the Node where the resulting resource will be used.
>+     *            I.e. it is the Node which will get access to the result of
>+     *            the request. (Where "get access to" might simply mean "embed"
>+     *            depending on the type of resource that is loaded).
>+     *            For example for an <img>/<video> it is the image/video
>+     *            element. For document loads inside <iframe> and <frame>s,
>+     *            the LoadingNode is the <iframe>/<frame> element. 
Is this true?  I thought we only mass in the actual element for audio and video.  For other loads, we pass in the document that the img/frame/etc is contained in instead of the eactual img/frame/etc element.  But maybe this has changed recently.

> For an
>+     *            XMLHttpRequest, it is the Document which contained the
>+     *            JS which initiated the XHR. For a stylesheet, it is the
>+     *            <link rel=stylesheet> node.
>+     *            For loads triggered by the HTML pre-parser, the LoadingNode
>+     *            is the Document which is currently being parsed.
>+     *            For loads of top-level navigations, and for loads originating
>+     *            from workers, the LoadingNode is null.
Any other cases it can be null?  Example: it can be null in the parent process.

>+     *            If the LoadingNode is non-null, then the LoadingPrincipal is
>+     *            the principal of the LoadingNode.
>      *          loadingPrincipal:
>-     *            The loadingPrincipal of the channel.
>-     *            The principal of the document where the result of this request
>-     *            will be used.
>-     *            This defaults to the principal of aLoadingNode, so when
>-     *            aLoadingNode is passed this can be left as null. However for
>-     *            loads where aLoadingNode is null this argument must be passed.
>-     *            For example for loads from a WebWorker, pass the principal of
>-     *            that worker.  For loads from an addon or from internal browser
>-     *            features, pass the system principal.
>-     *            This principal should almost always be the system principal if
>-     *            loadingNode is omitted, in which case you can use the
>-     *            useSystemPrincipal property.  The only exception to this is
>-     *            for loads from WebWorkers since they don't have any nodes to
>-     *            be passed as loadingNode.
>-     *            Please note, loadingPrincipal is *not* the principal of the
>-     *            resource being loaded, but rather the principal of the context
>-     *            where the resource will be used.


>+     *            This is the principal of the network request's
>+     *            caller/requester where the resulting resource will be used.
The loadingPrincipal isn't necessarily the caller/requester.  Change this back to:
The principal of the document where the result of this request will be used.

>+     *            I.e. it is the principal which will get access to the result
>+     *            of the request. (Where "get access to" might simply mean
>+     *            "embed" depending on the type of resource that is loaded).
>+     *            For example for an image, it is the principal of the document
>+     *            where the image is rendered. For a stylesheet it is the
>+     *            principal of the document where the stylesheet will be applied.
>+     *            So if document at http://a.com/page.html loads an image from
>+     *            http://b.com/pic.jpg, then loadingPrincipal will be
>+     *            http://a.com/page.html.
Add back: For loads from a WebWorker, pass the principal of that worker.

>+     *            For <iframe> and <frame> loads, the LoadingPrincipal is the
>+     *            principal of the parent document. 

> For loads for top-level
>+     *            navigations, the LoadingPrincipal is null.
>+     *            For all loads except loads for top-level navigations, the
>+     *            LoadingPrincipal is never null.

Change to:
The LoadingPrincipal defaults to the principal of LoadingNode, so when
aLoadingNode is passed this argument does not need to be passed in. However for
loads where aLoadingNode is null this argument must be passed.  The only exception is for top-level loads and navigations.
For top-level loads and navigations, the LoadingPrincipal doesn't exist.  Top level loads are the only case where no LoadingPrincipal or LoadingNode exist; for all other loads, the LoadingPrincipal will never be null.
Comment on attachment 8790198 [details] [diff] [review]
bug_1291458_update_documentation_for_loadinfo.patch

>diff --git a/netwerk/base/NetUtil.jsm b/netwerk/base/NetUtil.jsm
>--- a/netwerk/base/NetUtil.jsm
>+++ b/netwerk/base/NetUtil.jsm
>@@ -219,67 +219,73 @@ this.NetUtil = {
>      *          triggeringPrincipal:
The triggeringPrincipal changes look great!


>diff --git a/netwerk/base/nsIChannel.idl b/netwerk/base/nsIChannel.idl
>--- a/netwerk/base/nsIChannel.idl
>+++ b/netwerk/base/nsIChannel.idl
>@@ -329,19 +329,23 @@ interface nsIChannel : nsIRequest
>      * Implementations should throw NS_ERROR_NOT_AVAILABLE if the header either
>      * doesn't exist for this type of channel or is empty.
>      *
>      * @deprecated Use contentDisposition/contentDispositionFilename instead.
>      */
>     readonly attribute ACString contentDispositionHeader;
> 
>     /**
>-     * The nsILoadInfo for this load.  This is immutable for the
>-     * lifetime of the load and should be passed through across
>-     * redirects and the like.
>+     * The LoadInfo object contains information about a network load, why it
>+     * was started, and how we plan on using the resulting response.
>+     * If a network request is redirected, the new channel will receive a new
>+     * LoadInfo object. The new object will contain mostly the same
>+     * information as the pre-redirect one, but updated as appropriate. 

> For
>+     * exact information about what is updated, see documentation on
>+     * individual properties.
Change to: For detailed information about what parts of LoadInfo are updated on redirect, see documentation on individual properties.

>      */
Comment on attachment 8790198 [details] [diff] [review]
bug_1291458_update_documentation_for_loadinfo.patch

r+ with the changes I've mentioned.  Thanks Chris!

>diff --git a/netwerk/base/nsILoadInfo.idl b/netwerk/base/nsILoadInfo.idl
>--- a/netwerk/base/nsILoadInfo.idl
>+++ b/netwerk/base/nsILoadInfo.idl
>@@ -179,79 +179,108 @@ interface nsILoadInfo : nsISupports

>   [noscript, notxpcom, nostdcall, binaryname(TriggeringPrincipal)]
>   nsIPrincipal binaryTriggeringPrincipal();
> 
>   /**
>-   * The loadingDocument of the channel.
>+   * This is the ownerDocument of the LoadingNode. Unless the LoadingNode
>+   * is a Document, in which case the LoadingDocument is the same as the
>+   * LoadingNode.
>    *
>-   * The loadingDocument of a channel is the document that requested the
>-   * load of the resource. It is *not* the resource itself, it's the
>-   * resource's caller or requester in which the load is happening.
>-   *
>-   * <script> example: Assume a document who's origin is http://a.com embeds
>-   * a script from http://b.com. The loadingDocument for the channel
>-   * associated with the http://b.com script load is the document with origin
>-   * http://a.com
>-   *
>-   * <iframe> example: Assume a document with origin http://a.com embeds
>-   * <iframe src="http://b.com">. The loadingDocument for the channel associated
>-   * with the http://b.com network request is the document who's origin is
>-   * http://a.com. Now assume the iframe to http://b.com then further embeds
>-   * <script src="http://c.com">. The loadingDocument for the channel associated
>-   * with the http://c.com network request is the iframe with origin http://b.com.
>-   *
>-   * Warning: The loadingDocument can be null!

>+   * For loads of top-level navigations, and for loads originating from
>+   * workers, the LoadingDocument is null.
Change to: For top-level loads, top-level navigations, and loads originating from workers, the LoadingDocument is null.

>+   * 
>+   * If the LoadingDocument is non-null, then the LoadingPrincipal is the
>+   * principal of the LoadingDocument.
Change to:
When the LoadingDocument is not null, the LoadingPrincipal is set to the principal of the LoadingDocument

>    */
>   readonly attribute nsIDOMDocument loadingDocument;
Attachment #8790198 - Flags: review?(tanvi) → review+
Tanvi, thanks for the review. I incorporated your suggestions, but one thing remains unclear, which is what you pointed out regarding the LoadingNode. In fact, we only pass the element for audio and video as far as I know, but we pass the containing element (document) in other cases. I suggest we could change 'document' to 'subresource' in which case the documentation would be correct again. Before landing the documentation I was wondering what you think about the following text for LoadingNode:

     *            This is the Node where the resulting resource will be used.
     *            I.e. it is the Node which will get access to the result of
     *            the request. (Where "get access to" might simply mean "embed"
     *            depending on the type of resource that is loaded).
     *            For example for an <audio>/<video> it is the audio/video
     *            element. For subresource loads inside <iframe> and <frame>s,
     *            the LoadingNode is the <iframe>/<frame> element. For an
     *            XMLHttpRequest, it is the Document which contained the
     *            JS which initiated the XHR. For a stylesheet, it is the
     *            <link rel=stylesheet> node.

What do you think? Are we fine with that or are we missing something? Thanks!
Flags: needinfo?(tanvi)
Attachment #8791173 - Flags: review+
Attachment #8790198 - Attachment is obsolete: true
Comment on attachment 8791173 [details] [diff] [review]
bug_1291458_update_documentation_for_loadinfo.patch

Review of attachment 8791173 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure that it's a good idea that we're duplicating so much of this info in so many places. One solution would be to make sure that nsILoadInfo.idl as all the detailed docs, then elsewere we could simply refer to nsILoadInfo.idl.

So for example in nsIIOService.idl we could do

@param aLoadingNode, aLoadingPrincipal, aTriggeringPrincipal, aSecurityFlags, aContentPolicyType
  These will be used as values for the nsILoadInfo object on the created channel. 
  For details, see nsILoadInfo in nsILoadInfo.idl


I think it was my idea originally to duplicate this info in multiple places. But I think it's made it much harder to make these docs be good, which is quite important.

::: netwerk/base/nsIChannel.idl
@@ +339,5 @@
> +     * If a network request is redirected, the new channel will receive a new
> +     * LoadInfo object. The new object will contain mostly the same
> +     * information as the pre-redirect one, but updated as appropriate.
> +     * For detailed information about what parts of LoadInfo are updated on
> +     * redirect, see documentation on individual properties.

Can we add this info to the top of nsILoadInfo.idl
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #9)
> I'm not sure that it's a good idea that we're duplicating so much of this
> info in so many places.

Agreed, it's hard to keep docs up to date in 5+ different locations. I updated all of the documentation consumers to point to nsILoadInfo.

> ::: netwerk/base/nsIChannel.idl
> > +     * If a network request is redirected, the new channel will receive a new
> > +     * LoadInfo object. The new object will contain mostly the same
> > +     * information as the pre-redirect one, but updated as appropriate.
> > +     * For detailed information about what parts of LoadInfo are updated on
> > +     * redirect, see documentation on individual properties.
> 
> Can we add this info to the top of nsILoadInfo.idl

Done!


The only thing missing is the appropriate documentation for loadingNode, see Comment 8. Then we are done here!
Attachment #8791173 - Attachment is obsolete: true
Attachment #8791896 - Flags: review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Created attachment 8791173 [details] [diff] [review]
> bug_1291458_update_documentation_for_loadinfo.patch
> 
> Tanvi, thanks for the review. I incorporated your suggestions, but one thing
> remains unclear, which is what you pointed out regarding the LoadingNode. In
> fact, we only pass the element for audio and video as far as I know, but we
> pass the containing element (document) in other cases. I suggest we could
> change 'document' to 'subresource' in which case the documentation would be
> correct again. Before landing the documentation I was wondering what you
> think about the following text for LoadingNode:
> 
>      *            This is the Node where the resulting resource will be used.
>      *            I.e. it is the Node which will get access to the result of
>      *            the request. (Where "get access to" might simply mean
> "embed"
>      *            depending on the type of resource that is loaded).
>      *            For example for an <audio>/<video> it is the audio/video
>      *            element. For subresource loads inside <iframe> and
> <frame>s,
>      *            the LoadingNode is the <iframe>/<frame> element. For an
>      *            XMLHttpRequest, it is the Document which contained the
>      *            JS which initiated the XHR. For a stylesheet, it is the
>      *            <link rel=stylesheet> node.

What does this last sentence mean?

If a.com loads a stylesheet b.com, then the loadingNode for the b.com load is the a.com document.  If stylesheet b.com loads a font from c.com, then the loadingNode/loadingPrincipal for the font load is the a.com document and the triggeringPrincipal for the font load is b.com.

Hence, for a stylesheet load, I don't think the loadingNode is the <link rel=stylsheet> node.

I also would like to see one more change, which you'll see in the next comment.  Thanks Christoph!
Flags: needinfo?(tanvi)
Comment on attachment 8791896 [details] [diff] [review]
bug_1291458_update_documentation_for_loadinfo.patch

>diff --git a/netwerk/base/nsILoadInfo.idl b/netwerk/base/nsILoadInfo.idl
>--- a/netwerk/base/nsILoadInfo.idl
>+++ b/netwerk/base/nsILoadInfo.idl
>   /**
>-   * The loadingPrincipal is the principal that is responsible for the load.
>-   * It is *NOT* the principal tied to the resource/URI that this
>-   * channel is loading, it's the principal of the resource's
>-   * caller or requester. For example, if this channel is loading
>-   * an image from http://b.com that is embedded in a document
>-   * who's origin is http://a.com, the loadingPrincipal is http://a.com.
>+   * This is the principal of the network request's caller/requester where
>+   * the resulting resource will be used. I.e. it is the principal which
>+   * will get access to the result of the request. (Where "get access to"
>+   * might simply mean "embed" depending on the type of resource that is
>+   * loaded).
>    *
>-   * The loadingPrincipal will never be null.
>+   * For example for an image, it is the principal of the document where
>+   * the image is rendered. For a stylesheet it is the principal of the
>+   * document where the stylesheet will be applied.
>+   *
>+   * So if document at http://a.com/page.html loads an image from
>+   * http://b.com/pic.jpg, then loadingPrincipal will be
>+   * http://a.com/page.html.
>+   *
>+   * For <iframe> and <frame> loads, the LoadingPrincipal is the principal
>+   * of the parent document. For loads for top-level navigations, the
>+   * LoadingPrincipal is null.
Instead of "For loads for top-level navigations", can you say "For top-level loads", since a top level load may not be the result of a navigation.

>+   *
>+   * For all loads except loads for top-level navigations, the
s/loads for top-level navigations/top-level loads/

>+   * LoadingPrincipal is never null.
>    */
>   readonly attribute nsIPrincipal loadingPrincipal;
> 


>   /**
>-   * The loadingDocument of the channel.
>+   * This is the ownerDocument of the LoadingNode. Unless the LoadingNode
>+   * is a Document, in which case the LoadingDocument is the same as the
>+   * LoadingNode.
>    *
>-   * The loadingDocument of a channel is the document that requested the
>-   * load of the resource. It is *not* the resource itself, it's the
>-   * resource's caller or requester in which the load is happening.
>+   * For loads of top-level navigations, and for loads originating from
>+   * workers, the LoadingDocument is null.
Same here.
s/loads of top-level navigations/top-level loads/


>   /**
>    * A C++-friendly version of loadingDocument (loadingNode).
>-   * This is the node most proximally responsible for the load.
>+   * This is the Node where the resulting resource will be used. I.e. it is
>+   * the Node which will get access to the result of the request. (Where
>+   * "get access to" might simply mean "embed" depending on the type of
>+   * resource that is loaded).
>+   *
>+   * For example for an <img>/<video> it is the image/video element. For
>+   * document loads inside <iframe> and <frame>s, the LoadingNode is the
>+   * <iframe>/<frame> element. For an XMLHttpRequest, it is the Document
>+   * which contained the JS which initiated the XHR. For a stylesheet, it
>+   * is the <link rel=stylesheet> node.
>+   *
>+   * For loads triggered by the HTML pre-parser, the LoadingNode is the
>+   * Document which is currently being parsed.
>+   *
>+   * For loads of top-level navigations, and for loads originating from
>+   * workers, the LoadingNode is null.
s/loads of top-level navigations/top-level loads/

>+   *
>+   * If the LoadingNode is non-null, then the LoadingPrincipal is the
>+   * principal of the LoadingNode.
>    */
>   [noscript, notxpcom, nostdcall, binaryname(LoadingNode)]
>   nsINode binaryLoadingNode();
>
https://hg.mozilla.org/mozilla-central/rev/008c6f3dc239
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
> Instead of "For loads for top-level navigations", can you say "For top-level
> loads", since a top level load may not be the result of a navigation.

When is a top-level load not a navigation? I think the term navigation is used even when clicking a bookmark or when typing into the url-bar.


Another thing that I think would be good to state, and which I had forgotten to write about in the proposal I sent to Christoph, is when one should, or should not, use the SystemPrincipal as loadingPrincipal/triggeringPrincipal.

Basically I think we should add something like the following to the loadingPrincipal and triggeringPrincipal description:

If the loadingPrincipal (change to triggeringPrincipal in the triggeringPrincipal description) is the system principal, no security checks will be done at all, not during the initial load, and not during redirects. This includes not doing any nsIContentPolicy checks or any CheckLoadURI checks.

Because of this, never set the loadingPrincipal (change to triggeringPrincipal in the triggeringPrincipal description) to the system principal when the URI to be loaded is controlled by a webpage.

If the loadingPrincipal and triggeringPrincipal are both codebase-principals, then we will at least call into nsIContentPolicies. This happens even if the uri to be loaded is same-origin with the loadingPrincipal and triggeringPrincipal.
> Hence, for a stylesheet load, I don't think the loadingNode is the <link
> rel=stylsheet> node.

The docs that I originally sent to christoph intended to describe how I think things *should* work. I.e. I think in many cases we should chase down the correct loadingNode rather than simply use the document.

Certainly this should be done as follow-up bugs though.
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #15)
> Another thing that I think would be good to state, and which I had forgotten
> to write about in the proposal I sent to Christoph, is when one should, or
> should not, use the SystemPrincipal as loadingPrincipal/triggeringPrincipal.

I filed Bug 1305996 to add that.
Depends on: 1305996
You need to log in before you can comment on or make changes to this bug.