Add an API to nsIProtocolHandler to give per-URI flags

RESOLVED FIXED in Firefox 42

Status

()

Core
Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Followup from bug 1161831.

The problem is that, right now, nsIProtocolHandler::flags is an attribute, which means that all URIs of a given protocol need to have the same flags. But sometimes, we want different behavior for different URIs of a given protocol.

For the about: case, we solve it with this silly nested moz-safe-about: URI setup. For moz-extension, I just added an explicit check for this in nsScriptSecurityManager in bug 1161831.

Boris and I decided that the sanest thing to do here is to migrate from flags being a getter to being a method, and passing in the actual URI in question, which allows us to make the decision there.
(Assignee)

Updated

3 years ago
Flags: needinfo?(bobbyholley)
We definitely don't want to have to call out to addons every time that a URI is created, since eventually we'll want to support nsIURI creation off the main thread.

Beyond that I'm not hugely opinionated.
(Assignee)

Comment 2

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #1)
> We definitely don't want to have to call out to addons every time that a URI
> is created, since eventually we'll want to support nsIURI creation off the
> main thread.

Sure - this is just a question of changing the API of nsIProtocolHandler to pass in an extra argument when requesting the protocol flags. This should be fine, unless we need those protocol flags during URI creation, and currently cache them somehow? I haven't looked into it too much.
That could be ok. Though I suspect we wouldn't be able to expose that ability to addons, i.e. addons would be limited to having the same flags for all URIs. But that sounds fine.
(Assignee)

Comment 4

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #3)
> That could be ok. Though I suspect we wouldn't be able to expose that
> ability to addons, i.e. addons would be limited to having the same flags for
> all URIs. But that sounds fine.

Can you explain why? I'm planning on adding this as a QI to some nsIProtocolHandlerWithDynamicFlags, which addons presumably could implement if they wanted to (but probably wouldn't). I could make that builtinclass though, just to be safe.
Because I'd like to enable us to interact with the network stack off the main thread. That surely will mean getting URI flags off the main thread. Which isn't possible as long as the API is synchronous since we couldn't call into the addon from other threads.

I guess we technically could block the thread until we do a roundtrip to the main thread to call into the addon to get the URI flags, but that doesn't seem like acceptable perf.

Note that even the current workarounds of creating nested URIs doesn't work for addons if we enable URI creation off the main thread. At least given my current plan. See "Hooks for custom protocols" in https://etherpad.mozilla.org/BetterNeckoSecurityHooks
(Assignee)

Comment 6

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #5)
> Because I'd like to enable us to interact with the network stack off the
> main thread. That surely will mean getting URI flags off the main thread.
> Which isn't possible as long as the API is synchronous since we couldn't
> call into the addon from other threads.

Right - my question in comment 2 is how this is different than the status quo (where addons implement nsIProtocolHandler in JS, including the flags getter). My guess is that you're comfortable assuming that .flags never changes, and caching it?

> I guess we technically could block the thread until we do a roundtrip to the
> main thread to call into the addon to get the URI flags, but that doesn't
> seem like acceptable perf.

Naw. I'll just make this [builtinclass].
My plan is to deprecate nsIProtcolHandler and replace it with something that doesn't have a .newURI or .newChannel function and instead is more declarative. See the etherpad for details.

But marking it [builtinclass] sounds perfect.
(Assignee)

Comment 8

3 years ago
Created attachment 8639602 [details] [diff] [review]
Bug 1186152 - Implement nsIProtocolHandlerWithDynamicFlags and use it for moz-extension. v1

Bug 1186152 - Implement nsIProtocolHandlerWithDynamicFlags and use it for moz-extension. v1
Attachment #8639602 - Flags: review?(bzbarsky)
Comment on attachment 8639602 [details] [diff] [review]
Bug 1186152 - Implement nsIProtocolHandlerWithDynamicFlags and use it for moz-extension. v1

>+    // around. Calling this on a scheme with dynamic flags will through.

"throw"

>+  *aFlags = URI_STD | URI_IS_LOCAL_RESOURCE | (loadableByAnyone ? 0 : URI_DANGEROUS_TO_LOAD);

You need URI_LOADABLE_BY_ANYONE if loadableByAnyone, no?  I thought we asserted that one of the 5 required security flags was set....

r=me with that fixed
Attachment #8639602 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #10)
> You need URI_LOADABLE_BY_ANYONE if loadableByAnyone, no?

Good catch.

> I thought we asserted that one of the 5 required security flags was set....

Nope: https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/netwerk/base/nsIProtocolHandler.idl#l164
Oh.  Well, we should declare it to be "mozilla 2.0" and stop loading URIs entirely if they don't have one of these flags set!  ;)
https://hg.mozilla.org/mozilla-central/rev/b72d4867ae69
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.