771550 - Add a readonly origin attribute to nsIURI (aka stop computing origin everywhere!)

Status: NEW

(Core :: Networking, enhancement, P3)

Description

[Mounir Lamouri (:mounir)]

So right now, we have 3 major points to get origin:
- nsIPrincipal.origin
- nsSecurityManager::GetOriginFromURI
- nsContentUtils::Get{ASCII,UTF}Origin()

And we also have other origin computations completely wrong like:
https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/apphelper.js#102
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#485

I'm not sure why nsIURI doesn't simply provide an origin attribute that will allow anyone to get the URI's origin.

BTW, the main difference between the first 3 implementations is how they handle file:// and data URIs.
This is what they do:
* nsContentUtils:
 - file://:  null
 - data URI: null
* Principal:
 - file://:  the spec (file:///tmp/foo.html for example)
 - data URI: a cryptic string that isn't the same after reboot (like: moz-nullprincipal:{28970565-9f0d-4bde-a5be-c6ea56dbf3c2}). I guess nsNullPrincipalURI is doing that.
* nsScriptSecurityManager:
 - file://:  file://
 - data URI: the spec (data:text/html,<input> for example)

Is there a preferred behaviour? I would tend to say that we should return the full spec for data URIs and file:// but I guess some people have a better understanding of those issues.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The nsContentUtils API returns origin as defined in the HTML5 spec.  It should be used if, and only if, implementing something that uses that definition of origin.

Note that the distinction between origin and ASCII origin is nontrivial; converting between them once in string form is not actually possible iirc.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch

Boris, I remember you told me something needs to be updated if we update nsIURI.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 640289 [details] [diff] [review]
Patch

Yes, you need to update the hackery at http://hg.mozilla.org/mozilla-central/file/78e058c75a38/xpcom/io/nsBinaryStream.cpp#l729

You also need to document which exact concept of "origin" this property is returning and probably get some developer docs added so extensions can be updated as needed.

For jar, why would you not return the origin of your inner URI?

For nsStandardURL, I believe your code could return different origins for http://foo/ and http://foo:80/, which I think is wrong.  Though nsScripSecurityManager does something similar, so maybe we canonicalize the port before this point or something?

For nsStandardURL, hardcoding the schemes which have no hosts also seems wrong.

Furthermore, depending on what you're trying to do with the origin, I think there are other schemes (imap:// say) where the right origin is the whole URI...  See NS_SecurityCompareURIs.

Simple nested URIs should presumably forward on to the thing nested inside?  Again, this _really_ depends on what the origin is being used for.  And that brings us back to there being at least 3 distinct concepts of "origin" around.

What should origin be for blob: URIs or other impls of nsIURIWithPrincipal?

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 640289 [details] [diff] [review]
> Patch
> 
> Yes, you need to update the hackery at
> http://hg.mozilla.org/mozilla-central/file/78e058c75a38/xpcom/io/
> nsBinaryStream.cpp#l729

Thanks.

> You also need to document which exact concept of "origin" this property is
> returning and probably get some developer docs added so extensions can be
> updated as needed.

I think we would be quite close to https://tools.ietf.org/html/rfc6454#section-4. We could try to follow that.
The purpose would be to use origin mostly for security checks.

> For jar, why would you not return the origin of your inner URI?

You mean for:
jar:http://example.org/foobar.jar!/foo.html
returning:
http://example.org
?

That would make sense.

> For nsStandardURL, I believe your code could return different origins for
> http://foo/ and http://foo:80/, which I think is wrong.  Though
> nsScripSecurityManager does something similar, so maybe we canonicalize the
> port before this point or something?

Seems like nsStandardURL::Init() takes a default port. If the port in the URL spec is equal to the default port, the class will consider it as not present. IOW, it is the same. I have added tests to check this.

> For nsStandardURL, hardcoding the schemes which have no hosts also seems
> wrong.

For 'chrome', I don't see any other solution.
We could check if Host.IsEmpty() but that would have some edge cases.
We could use mURLType but I'm not sure I got all of them. URLTYPE_NO_AUTHORITY is for stuff like file:// I guess. I'm not sure what is the difference for the two others.
Actually, for file://, we could use mSupportsFileURL and then support more cases (like ressources://).

> Furthermore, depending on what you're trying to do with the origin, I think
> there are other schemes (imap:// say) where the right origin is the whole
> URI...  See NS_SecurityCompareURIs.

Given that I would like this to be used for security checks, maybe we should have the same behaviour. However, how can we do that without special casing?

> Simple nested URIs should presumably forward on to the thing nested inside? 

Did that.

> What should origin be for blob: URIs or other impls of nsIURIWithPrincipal?

Anything that would make the blob distinct from anything else. Maybe the spec?

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I guess it's not perfect yet but comments are welcome :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I think we would be quite close to https://tools.ietf.org/html/rfc6454#section-4.

That section returns an object, not a string, fwiw.  This algorithm more closely corresponds to our nsIPrincipal than what you're doing here...

> The purpose would be to use origin mostly for security checks.

OK.  I think it's _very_ important to document _exactly_ what this origin thing is and what you can and can't expect from it.  And in particular what uses are correct and what uses are not.

I would really like to understand how we plan to use this thing before I can comment intelligently on the desired behavior.  Note that for "file://" the story for security checks is ... somewhat complicated.  Like it depends on more than the URI.

> You mean for:
> jar:http://example.org/foobar.jar!/foo.html
> returning:
> http://example.org

Yes.

> URLTYPE_NO_AUTHORITY is for stuff like file:// I guess.

Yes.  But chrome:// doesn't use that, right?

> I'm not sure what is the difference for the two others.

The difference is how the parsing works, as documented in the IDL.

I agree that using the URI type is suboptimal here.  I wonder whether this would make sense as a protocol handler flag?

> Anything that would make the blob distinct from anything else. Maybe the spec?

If it's unique, I guess that can work, yes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 641118 [details] [diff] [review]
Patch v2

r- pending those comments.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 9

[Kershaw Chang [:kershaw]]

Hi Valentin,

Do you think if we still need to implement this?

Thanks.

Comment 10

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

Probably a nice to have - but the differences in behaviour noted in the above comments are still there and need to be addressed.
It would also be nice to get in line with https://url.spec.whatwg.org/#origin