485562 - nsStandardURL allows setting empty host even for URLTYPE_AUTHORITY URLs

Status: RESOLVED DUPLICATE of bug 996055

(Core :: Networking, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This leads to issues like the following:

1) Start with http://foo.com/bar/
2) SetHost("")
3) NewURI() the GetSpec()
4) Examine the new URI

EXPECTED RESULTS: Just like the old URI.

ACTUAL RESULTS: http://bar/ with "bar" as the host.

This is web-detectible via the hostname setter on anchors.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 2

[Biju]

var a = document.createElement('a');
a.setAttribute('href', 'http://example.org:123/foo?bar#baz');
a.hostname = "#j";
[a.hostname, a.port,a.href, a.protocol, a.host,a.pathname,a.search, a.hash];



Above code can also set host null.
whether proposed code will take care off such situation?

other characters are ?/:
similarly you can also set host as ! * etc

Comment 3

[Biju]

a.host = "#test";

also gives same result

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that patch won't help with those.  We should probably either escape such chars or not allow those sets, for all standard urls (not just URLTYPE_AUTHORITY ones).

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 369712 [details] [diff] [review]
Proposed fix

We should probably not allow creating URIs that have an empty host either:
js> ios.nsIIOService.newURI("http://", null, null).spec
http:///
js> ios.nsIIOService.newURI("http://", null, null).host



On the other hand, there's more weirdness:
js>  ios.nsIIOService.newURI("file://foo/", null, null).host
foo
js> ios.nsIIOService.newURI("file://foo/", null, null).host = "bar"
WARNING: cannot set host on no-auth url: file /home/cbiesinger/mozilla-central/netwerk/base/src/nsStandardURL.cpp, line 1369
uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIURI.host]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: typein :: <TOP_LEVEL> :: line 5"  data: no]

i.e. you can create a no-auth URL with a host, you just can't modify the host afterwards

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah.  Want me to generally add more checks about this stuff into the URI-init codepath?  Might break some file:// uses, maybe.  :(

Comment 7

[Jason Duell]

Some beginner questions:

Um, what is a URLTYPE_AUTHORITY URL exactly, anyway?  The source code doesn't really explain at all.

And why do we care that one can set the host to ""?  Isn't this just an internal programming error, or is this an exploit of some sort?

Comment 8

[Boris Zbarsky [:bzbarsky]]

URLTYPE_AUTHORITY is a url for a scheme for which we know there is a required authority section (e.g. HTTP).  See also the constants and comments in nsIStandardURL.idl.

> And why do we care that one can set the host to ""?

Because allowing that means the URL doesn't round-trip correctly.  For example, if I take http://example.com/foo.com and set host to "", then I get http:///foo.com, which is the same thing as http://foo.com/ as far as the world is concerned.  This is actually detectable from web JS via setting .hostname on HTML anchor elements to the empty string; see http://www.mail-archive.com/[e-mail]/msg14130.html (and maybe the rest of that thread too).

It can't be an "internal programming error" if it's exposed to web JS.  We could do the check in nsHTMLAnchorElemen (and nsHTMLAreaElement), I guess, but it makes more sense to do it centrally in nsStandardURL.

Comment 9

[Jason Duell]

> EXPECTED RESULTS: Just like the old URI.
> ACTUAL RESULTS: http://bar/ with "bar" as the host.

I guess I'm wondering why--if someone is dumb enough to set host to ""--we don't just barf on the URL when we try to load it.  As opposed to remembering what the host value was originally, and resetting to it.  And frankly, if we do wind up trying to load "http://foo.com", what's the harm?  The user made an error, and now they get the behavior they "asked" for.  No?

But I don't need to drag the conversation on, if the rest of you are all on the same page as to the desired behavior.  Do you still want my review of the current patch, or were you planning to add the additional checks?  Let me know.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Jason, the issue is not the loading of the url.  The issue is that if one stringifies it, one doesn't get a url with the right host anymore.  This is a problem.  Do see the thread I linked to.

In this case the "user" is web page JS, that has no idea of the details of our URL implementation.  All it knows is it's seeing inconsistent behavior.

I'll be adding more checks; I hope to get a patch up in the next few days.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Over to default owner, clearly.... ccing rjesup, since he's been interested in bugs like this before.

Comment 12

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 13

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This got fixed in bug 996055, with more or less my proposed fix, I think, but with different tests.

Valentin, are the tests here useful?

Comment 17

[Valentin Gosu [:valentin] (he/him)]

(In reply to Boris Zbarsky [:bzbarsky] from comment #16)

This got fixed in bug 996055, with more or less my proposed fix, I think, but with different tests.

Valentin, are the tests here useful?

Not in its current form, but I think we might be missing some tests for these codepaths.
I filed bug 1621877

Comment 18

[Boris Zbarsky [:bzbarsky]]

Perfect, thank you!