Pass loadURIOptions dictionary for docshell loads

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P2
normal
RESOLVED FIXED
4 months ago
21 days ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks 4 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 4 obsolete attachments)

Currently we individually pass a
* triggeringPrincipal
* referrer
* referrer policy

from the front end into docshell. Soon we also need to pass a CSP and most likely we would like to pass more such 'security' args to docshell in the foreseen future.

Instead of having to change all intermediate functions and pass yet another argument we then only have to update fields within that struct.
(Assignee)

Updated

4 months ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
(Assignee)

Comment 1

4 months ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> Currently we individually pass a
> * triggeringPrincipal
> * referrer
> * referrer policy

I just thought of another member for that struct:
* the storagePrincipal, which we would like to use to separate browsing contexts is a good candidate to live in that struct.
(Assignee)

Comment 2

4 months ago
Boris, as discussed at the AllHands it makes sense to pass a 'security struct' from the frontend code into docshell. Before going further down that route though, I wanted to make sure I am not missing anything.

Here is my design:

1) Created a new nsIDocSecArgs.idl which holds triggeringPrincipal, CSP, referrer, referrerPolicy (later also a storagePrincipal).

2) Changed LoadUriWithOptions() as well as LoadURI() to take that new struct from (1) instead of individual arguments.

3) Within DoURILoad() we use the newly passed aCsp instead of querying the CSP from the triggeringPrincipal (but also asserting that they are the same). Please note that it's that CSP argument we can then use within Bug 965637 for new documents.

Obviously there are a few things missing, e.g. before passing the newly created docSecArgs object, I need to push through the CSP to some more API layers, e.g. from tabbrowser.xml. Also, I need to expose an .init() for the docSecArgs and a few other things...

Modulo the above, does that all look acceptable to you or am I missing something?
Flags: needinfo?(bzbarsky)
> Created a new nsIDocSecArgs.idl

I'm not sure about the naming there...

And actually, I'm not 100% sure why we want a new XPCOM type here.  Seems like we could just use jsval in the xpidl and convert to a WebIDL dictionary on the C++ side.  That would let JS consumers pass in options objects in a natural way, right?  Do we need a refcounted type on the C++ side, or would having a struct that we can pass around (a WebIDL dictionary) be fine?
 
Apart from that, this seems fine, though.  I'd like Kyle to take a look as well, to see how this works with the changes he's planning.
Flags: needinfo?(bzbarsky) → needinfo?(kyle)
(Assignee)

Comment 4

4 months ago
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)
> > Created a new nsIDocSecArgs.idl
> I'm not sure about the naming there...

The name is an easy fix, if you have any preference, let me know!
 
> And actually, I'm not 100% sure why we want a new XPCOM type here.  Seems
> like we could just use jsval in the xpidl and convert to a WebIDL dictionary
> on the C++ side.  That would let JS consumers pass in options objects in a
> natural way, right?  Do we need a refcounted type on the C++ side, or would
> having a struct that we can pass around (a WebIDL dictionary) be fine?

I think it's not that simple unfortunately, while I agree it would be preferable to generate a webidl dictionariy, there are members within that struct that make this hard. We have nsIPrincipal which would translate to Principal in webidl, so that part is fine, but I don't know what to do about nsIContentSecurityPolicy - is there even a non trivial solution?

Additionally, do we need to care about future members we would like to add to that dictionary?

I am more than happy to incorporate your suggestions but also wanted to make sure that we are not missing anything here.
Flags: needinfo?(bzbarsky)
I don't think this should interrupt the changes I'm making at the moment, as this is a subset of what we later include in nsDocShellLoadState, so we'll probably end up subsuming it in the LoadState creation in LoadURI. We'd need to possible add the storage principal to LoadState, depending on where it's needed in the load.
Flags: needinfo?(kyle)
> but I don't know what to do about nsIContentSecurityPolicy - is there even a non trivial solution?

You can use XPCOM types in webidl.  That's how "Principal" works.  You just put:

  interface Foo;

in your .webidl file to declare the XPCOM interface as an interface and add:

  addExternalIface('Foo', nativeType='nsIContentSecurityPolicy',
                   headerFile='nsIContentSecurityPolicy.h', notflattened=True)

to Bindings.conf to map the name "Foo" to the right XPCOM interface.  You could just use "nsIContentSecurityPolicy" for "Foo", or "ContentSecurityPolicy", or whatever.

> Additionally, do we need to care about future members we would like to add to that dictionary?

I'm not sure what you're asking, exactly...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 7

4 months ago
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)
> > but I don't know what to do about nsIContentSecurityPolicy - is there even a non trivial solution?
> 
> You can use XPCOM types in webidl.  That's how "Principal" works.

Ah, I didn't know it's that simple.

> > Additionally, do we need to care about future members we would like to add to that dictionary?
> 
> I'm not sure what you're asking, exactly...

I didn't know we can use XPCOM types in webidl which renders this second part of the question obsolete.
(Assignee)

Comment 8

4 months ago
Boris, just wanted to run this WIP-patch by you real quick to make sure I am not missing anything. Here is what I did:

1 - I created a LoadArgs.webidl file which holds a dictionary for all the arguments.

2 - I updated loadURI() within nsIWebNavigation.idl by the following argument:
      in jsval aLoadArgs,

Using a jsval allows to use .Init() to actually create the dictionary on the C++ side. The downside however is that we have to create a JSVal for all the C++ callers of LoadURI(). There are are a few of C++ callers which now create a JSVal from the dictionary only to then reverse that operation within LoadURI(). Would it be possible to create an overloaded function of LoadURI? One that takes a jsval and the other that takes dom::LoadArgs as an argument so we don't have to deal with JSVals in C++ land. Or alternatively one that is only available within C++ and then we only forward the .idl one to the C++ one?

What do you think?
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

4 months ago
Attachment #9032748 - Attachment is obsolete: true
Yes, you could add a loadURI overload that takes the WebIDL dictionary directly.  Something like this, in nsIWebNavigation.idl at the top:

  [ref] native LoadArgs(const mozilla::dom::LoadArgs);

and then in the actual interface definition:


  [implicit_jscontext,binaryname(LoadURIFromScript)]
  void loadURI(in AString                  aURI,
               in unsigned long            aLoadFlags,
               in nsIURI                   aReferrer,
               in jsval                    aLoadArgs,
               in nsIInputStream           aPostData,
               in nsIInputStream           aHeaders,
               [optional] in nsIPrincipal  aTriggeringPrincipal);
               in nsIInputStream           aHeaders);

  // Same thing, but for C++ callers.
  [nostdcall]
  void loadURI(in AString                  aURI,
               in unsigned long            aLoadFlags,
               in nsIURI                   aReferrer,
               in LoadArgs                 aLoadArgs,
               in nsIInputStream           aPostData,
               in nsIInputStream           aHeaders,
               [optional] in nsIPrincipal  aTriggeringPrincipal);
               in nsIInputStream           aHeaders);

The [nostdcall] is not strictly needed, but if you do that you can just have it returning "nsresult" instead of NS_IMETHODIMP.  The binaryname bit _is_ needed, because you don't want them having the same name on the C++ side, since that would make the vtable unpredictable.

Then the jsval version (which in C++ is called LoadURIFromScript) will create a dictionary from the given val and call the dictionary version, and C++ callers can call the dictionary version directly.
Flags: needinfo?(bzbarsky)

Comment 10

3 months ago
Without wanting to derail this bug, if we're adding a new API anyway, any chance we could move the other pile of arguments on the new struct so the new API just takes a URI plus an options webidl dictionary? It'd make a lot of this type of code a lot more readable (at least in JS).
(Assignee)

Comment 11

3 months ago
(In reply to :Gijs (he/him) from comment #10)
> Without wanting to derail this bug, if we're adding a new API anyway, any
> chance we could move the other pile of arguments on the new struct so the
> new API just takes a URI plus an options webidl dictionary? It'd make a lot
> of this type of code a lot more readable (at least in JS).

Yeah, I am totally fine with that. Currently the .webidl looks like this:

+dictionary LoadArgs {
+  required Principal? triggeringPrincipal;
+  ContentSecurityPolicy csp;
+  URI referrerURI;
+  long referrerPolicy;
+};

What else would you like to have in there?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 12

3 months ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to :Gijs (he/him) from comment #10)
> > Without wanting to derail this bug, if we're adding a new API anyway, any
> > chance we could move the other pile of arguments on the new struct so the
> > new API just takes a URI plus an options webidl dictionary? It'd make a lot
> > of this type of code a lot more readable (at least in JS).
> 
> Yeah, I am totally fine with that. Currently the .webidl looks like this:
> 
> +dictionary LoadArgs {
> +  required Principal? triggeringPrincipal;
> +  ContentSecurityPolicy csp;
> +  URI referrerURI;
> +  long referrerPolicy;
> +};
> 
> What else would you like to have in there?

This seems fine for security-related stuff. The other thing is that we should be moving towards passing nsIURIs or webidl URLs to these types of APIs instead of strings, but I dunno how tricky that'd be.

`LoadArgs` is pretty generic, though, and if it's not specific to security then IMO load flags, postData and additional headers (which seem to be referenced in comment #9) could also go in there - basically everything besides the URI.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 13

3 months ago
(In reply to :Gijs (he/him) from comment #12)
> `LoadArgs` is pretty generic, though, and if it's not specific to security
> then IMO load flags, postData and additional headers (which seem to be
> referenced in comment #9) could also go in there - basically everything
> besides the URI.

Yeah that makes sense, updating the title of the bug to reflect that change as well.
Summary: Create 'security'-struct which we can pass from the front-end to docshell → Pass loadArgs dictionary for docshell loads
(Assignee)

Comment 14

3 months ago

After some intensive hacking I realized that CSP requires more changes. I am suggesting we provide the fundamental architecture for loadArgs within this Bug and do the CSP stuff within Bug 1518454.

Blocks: 1518454
(Assignee)

Comment 15

3 months ago

Boris,
let me summarize the changes within this patch:

  • We create a LoadArgs.webidl which contains a dictionary for arguments we want to pass to LoadURI().
  • We update nsIWebNavigation.idl and pass that new loadArgs object to LoadURIWithOptions and since LoadURI() was just forwarding calls to LoadURIWithOptions() I removed LoadURI completely.
  • Probably we can be a little smarter within the WebIDL regarding default values. For now, I only require a triggeringPrincipal and default the loadFlags to 0.
  • As mentioned, the CSP bits are getting pretty big sizewise, so let's do all the CSP related changes to the loadArgs dict within Bug 1518454.
  • Gijs offered his help to review the frontend bits if you are fine with those backend changes.

Thanks!

Attachment #9033973 - Attachment is obsolete: true
Attachment #9035079 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

3 months ago

Gijs,
please see the previous comment for some background. Additionally, I don't like the additional

  •  if (postData) {
    
  •    loadArgs.postStream = postData;
    
  •  }
    

instead of just adding let loadArgs = { ..., postStream: postData }, but if postData is null then I get the following error:

Console message: [JavaScript Error: "'postStream' member of LoadArgs is not an object."

Maybe it has to do with the defaults in the LoadArgs.webidl but I am not sure - would be really happy if we can clean that up.

Thanks!

Attachment #9035081 - Flags: review?(gijskruitbosch+bugs)

Comment 17

3 months ago

You should be able to use the ? suffix for properties on the dictionary that can be null, I think?

In fact, this means I'm a bit confused about the triggering principal:

required Principal? triggeringPrincipal;

Yeah, passing an explicitly null postStream seems fine so we should allow that.

In fact, this means I'm a bit confused about the triggering principal:

We currently have a mode where we allow null to be passed (when the "security.strict_security_checks.enabled" pref is false. That's the default on release, beta, and Android...

Comment on attachment 9035079 [details] [diff] [review]
bug_1513241_part_1_update_loadURI_interface.patch

>+++ b/docshell/base/nsDocShell.cpp
>+nsDocShell::LoadURIWithOptions(const nsAString& aURI,

Why not just call this loadURI? There is no more "non-options" version, and the fact that it has options is obvious from the options member...

>+                               const mozilla::dom::LoadArgs& aLoadArgs) {

This file has "using namespace mozilla::dom", so I don't think we need the full namespacing here.

>+  nsCOMPtr<nsIInputStream> postStream;
>+  if (aLoadArgs.mPostStream.WasPassed()) {
>+    postStream = aLoadArgs.mPostStream.Value();
>+  }

Since we should allow null for the postStream anyway, with null representing "no post data", the IDL should probably just have:

  InputStream? postStream = null;

and then this code can be:

  nsCOMPtr<nsIInputStream> postStream = aLoadArgs.mPostStream;
>+  if (aLoadArgs.mHeaders.WasPassed()) {

Same thing here; I think we should just allow passing null to represent "no header stream" and set that up as the default value.

>+  if (aLoadArgs.mBaseURI.WasPassed()) {

And probably same here; might as well allow passing null explicitly for the base URI.

I don't know whether we'd still want the headerStream and baseURI locals if we do that.

>+  if (aLoadArgs.mReferrerURI.WasPassed()) {

We probably want to allow passing null here too, actually. Especially since the IDL comments claim something about what happens if it's null. ;)

>+nsDocShell::LoadURIWithOptionsFromScript(const nsAString& aURI,
>+  dom::LoadArgs loadArgs;

Is the namespace prefixing needed?

>@@ -6905,22 +6917,22 @@ nsresult nsDocShell::EndPageLoad(nsIWebP
>+          dom::LoadArgs loadArgs;

Is the prefixing needed?

>+++ b/docshell/base/nsIDocShell.idl
>+interface nsIContentSecurityPolicy;

Hmm. Why is this needed?

>+++ b/docshell/base/nsIWebNavigation.idl
>+#include "mozilla/dom/LoadArgsBinding.h"

Why is this needed? The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

>+   * @param aLoadArgs
>+   *        A JSObject of the form LoadArgs.webidl holding info like e.g. the

"defined in LoadArgs.webidl", right?

>+++ b/dom/ipc/TabChild.cpp
> +  dom::LoadArgs loadArgs;

Do we need the prefixing?

>+      NS_ConvertUTF8toUTF16(aURI),loadArgs);

Missing space. Did you clang-format this? If not, please do.

>+++ b/dom/security/FramingChecker.cpp
>@@ -252,18 +253,21 @@ static bool ShouldIgnoreFrameOptions(nsI
>+          dom::LoadArgs loadArgs;

Do we need the prefixing?

>+++ b/dom/webidl/LoadArgs.webidl
>+dictionary LoadArgs {

This name seems very generic... How about LoadURIOptions (and rename the .webidl too).

>+   * The principal that initiated the load.
>+   */
>+  required Principal? triggeringPrincipal;

There was more documentation in the .idl. Is leaving it out purposeful?

I'm a bit torn on whether this should be in the dictionary or just an argument, given the "required" status. Especially given the C++ callers, who can easily forget to set the member...

That said, if we're planning to just pass around the entire LoadURIOptions struct as a whole, it makes sense to have this in the dictionary.

Maybe we should add a constructor to dictionaries with required args that takes those args... Maybe file a followup on that and reference this dictionary?

>+   * One of the REFERRER_POLICY_* constants from nsIHttpChannel.
>+   *  Normal case is REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE.

I know you just copied this comment, but seems to be the "normal case" is UNSET and then the internals will do something with that, right?

>+++ b/editor/composer/nsEditingSession.cpp
>+      dom::LoadArgs loadArgs;

Do we need the prefixing?

>+++ b/xpfe/appshell/nsWebShellWindow.cpp
>+    dom::LoadArgs loadArgs;

Do we need the prefixing?

r=me with those bits fixed. Thank you for doing this!

Attachment #9035079 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

3 months ago

Thanks Boris, I incorporated all of your suggestions, in particular:

  • Renamed the dictionary as well as the file to LoadURIOptions.webidl
  • Kept LoadURI() and ditched LoadURIWithOptions()
  • Removed namespace prefixing where possible

Regarding the triggeringPrincipal as part of the dictionary or having it as a separate argument

  • I prefer having it within the dictionary because as you have pointed out we can pass around the loadURIOptions struct as a whole. Additionally we have strong assertions within LoadURI() which enforce we have a triggeringPrincipal set. Please note that even if the pref security.strict_security_checks.enable is set to false we still assert that LoadURI gets a valid triggeringPrincipal.

Copying over r+ from bz!

Attachment #9035079 - Attachment is obsolete: true
Attachment #9035384 - Flags: review+
(Assignee)

Comment 21

3 months ago

Gijs, this is way cleaner now having the webidl updated with the default values. What do you think?

Attachment #9035081 - Attachment is obsolete: true
Attachment #9035081 - Flags: review?(gijskruitbosch+bugs)
Attachment #9035385 - Flags: review?(gijskruitbosch+bugs)

Why is this needed? The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

What happened to that review comment?

Flags: needinfo?(ckerschb)
(Assignee)

Comment 23

3 months ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #22)

Why is this needed? The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

What happened to that review comment?

Sorry for not pointing that out explicitly, but I didn't ignore your comment. If I remove |#include "mozilla/dom/LoadURIOptionsBinding.h"| from nsIWebNavigation.idl I get the following compile error:

0:31.02 /home/ckerschb/source/mc-dbg/dist/include/nsIWebNavigation.h:95:93: error: no type named 'LoadURIOptions' in namespace 'mozilla::dom'
0:31.02 JS_HAZ_CAN_RUN_SCRIPT virtual nsresult LoadURI(const nsAString& aURI, const mozilla::dom::LoadURIOptions & aLoadURIOptions) = 0;

Flags: needinfo?(ckerschb)
(Assignee)

Updated

3 months ago
Summary: Pass loadArgs dictionary for docshell loads → Pass loadURIOptions dictionary for docshell loads

Ah, I see. We don't automatically generate forward-decls for "native" bits.

Still, all we should need here is a forward-decl (inside the "%{C++" section), not an include, right?

(Assignee)

Comment 25

3 months ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #24)

Ah, I see. We don't automatically generate forward-decls for "native" bits.
Still, all we should need here is a forward-decl (inside the "%{C++" section), not an include, right?

Unfortunately not - I tried several things.
First approach: Simply doing:

%{ C++
class mozilla::dom::LoadURIOptions;
%}

causes this compile error:

In file included from /home/ckerschb/source/mc/uriloader/base/nsDocLoader.cpp:14:
0:04.64 /home/ckerschb/source/mc-dbg/dist/include/nsIWebNavigation.h:38:21: error: no class named 'LoadURIOptions' in namespace 'mozilla::dom'
0:04.64 class mozilla::dom::LoadURIOptions;

Second Approach:
If in nsIWebNavigation.idl I do:

webidl LoadURIOptions;

it produces the following code within dist/include/nsIWebNavigation.h:

namespace mozilla {
namespace dom {
class LoadURIOptions; /* webidl LoadURIOptions */
} // namespace dom
} // namespace mozilla

which then causes this warning.

1:42.28 struct LoadURIOptions : public DictionaryBase
1:42.28 ^
1:42.28 /home/ckerschb/source/mc-dbg/dist/include/nsIWebNavigation.h:39:1: note: did you mean struct here?
1:42.28 class LoadURIOptions; /* webidl LoadURIOptions */
1:42.28 ^~~~~
1:42.28 struct
1:47.07 1 warning generated.

TDIL because I was not aware that we can use forward declarations within *.idl. Do you want me to file a bug to generate forward declarations for native bits? Or how should we proceed?

Flags: needinfo?(bzbarsky)

First approach: Simply doing:

You want:

  %{C++
    namespace mozilla {
    namespace dom {
    struct LoadURIOptions;
    } // namespace dom
    } // namespace mozilla
  %}
Flags: needinfo?(bzbarsky)

Comment 27

3 months ago
Comment on attachment 9035385 [details] [diff] [review]
bug_1513241_part_2_frontend_consumers.patch

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

::: browser/base/content/browser.js
@@ +1075,5 @@
> +        triggeringPrincipal,
> +        loadFlags: flags,
> +        referrerURI,
> +        referrerPolicy,
> +        postStream: postData,

It's kind of unfortunate that we call this postStream consistently on the cpp site and postdata consistently on the js side, but I don't have great solutions apart from just living with this for now...

@@ +1130,5 @@
> +        loadFlags: flags,
> +        referrerURI,
> +        referrerPolicy,
> +        postStream: postData,
> +      };

Nit: just define this once before the try...catch and use it in both branches.

::: toolkit/components/remotebrowserutils/RemoteWebNavigation.js
@@ +92,5 @@
>      }
>  
>      this._sendMessage("WebNavigation:LoadURI", {
>        uri: aURI,
> +      flags: aLoadURIOptions.loadFlags,

Can you file a follow-up issue to make these object prop names match to make this slightly more ergonomic?
Attachment #9035385 - Flags: review?(gijskruitbosch+bugs) → review+

I would be fine with calling it postData on the C++ side, fwiw.

(Assignee)

Comment 29

3 months ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #26)

First approach: Simply doing:

You want:

  %{C++
    namespace mozilla {
    namespace dom {
    struct LoadURIOptions;
    } // namespace dom
    } // namespace mozilla
  %}

Thanks - that works!

(Assignee)

Comment 30

3 months ago

The code we are having at the moment causes some problems on Android [1], which is also why we only assert that we pass the right triggeringPrincipal for top-level loads if it's not an Android build. To accomodate, I changed the LoadURIOptions.webidl to

Principal? triggeringPrincipal = null;
so the principal is not required. Jonathan will fix that problem within Bug 1504968. I'll leave a comment within Bug 1504968 as well.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ab3e53d35dba0a5db9c20394a7000cd301bef7

Blocks: 1504968
(Assignee)

Comment 31

3 months ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #28)

I would be fine with calling it postData on the C++ side, fwiw.

I updated that part and also called it postData in the webidl.

(Assignee)

Updated

3 months ago
Blocks: 1519365
(Assignee)

Comment 32

3 months ago

(In reply to :Gijs (he/him) from comment #27)

 this._sendMessage("WebNavigation:LoadURI", {
   uri: aURI,
  •  flags: aLoadURIOptions.loadFlags,
    

Can you file a follow-up issue to make these object prop names match to make
this slightly more ergonomic?

Thanks Gijs, I incorporated your suggestions and filed Bug 1519365 to get those property names updated.

Comment 33

3 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae543902cda
Update loadURI interface and pass a loadURIOptions dictionary from frontend to docshell loads. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/71cb45922e34
Update frontend consumers of loadURI and pass loadURIOptions dictionary. r=gijs

Comment 34

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.