Using ipv6 downloads html file instead of rendering it

RESOLVED FIXED in Firefox 64

Status

()

defect
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: 2badel2, Assigned: jandem)

Tracking

({regression})

64 Branch
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 unaffected, firefox64+ fixed)

Details

Attachments

(1 attachment)

Reporter

Description

9 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180830143136

Steps to reproduce:

Start a local webserver with ipv6 support. For example:

From: https://gist.github.com/chrisklaiber/54511886e8e4c18126792fc634f44d57
$ echo 'Hi' > index.html
$ python2 -m SimpleHTTPServer6

Open http://[::]:8000 (the ipv6 localhost)


Actual results:

The file gets downloaded, even if it is valid html with text/html MIME type.


Expected results:

The file should have been rendered by the browser. That happens as expected when using the http://127.0.0.1:8000 URL, the ipv4 equivalent of localhost. http://localhost:8000/ also works as expected.

This issue was not present in the last nightly (63.0a1). I tested it with a new profile, and the bug is still present.

Build ID: 20180916100118
OS: Linux 4.15.0-3-amd64
Reporter

Comment 1

9 months ago
To clarify: I submitted this issue from a different machine, the bug does not affect Firefox 62, only Firefox 64.
Great bug report, many thanks!

[Tracking Requested - why for this release]:
Seems a pretty severe regression.
Blocks: 1487032
Component: Untriaged → XPConnect
Flags: needinfo?(jdemooij)
Keywords: regression
Product: Firefox → Core
Has Regression Range: --- → yes
Assignee

Comment 4

9 months ago
I'm looking into this.

We seem to be getting a `::` base domain in ContentPrincipal::GetSiteOrigin from GetBaseDomainHelper and I think the URI building code doesn't like that. I'll do more digging.
Assignee

Comment 5

9 months ago
ThirdPartyUtil::GetBaseDomain calls GetAsciiHost here:

https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/dom/base/ThirdPartyUtil.cpp#300

And nsStandardURL::Host then strips the '[' and ']' here:

https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/netwerk/base/nsStandardURL.h#563-572

So we end up with :: instead of [::] and when we pass that to SetHost later it fails. Not sure what the right fix is...
Honza, any suggestions for the right thing to do here?  Or who else on the necko team would know?  Valentin seems to be out right now...
Flags: needinfo?(honzab.moz)
Assignee

Comment 7

9 months ago
One option I considered is to call GetAsciiHost explicitly, compare that to the result of ThirdPartyUtil::GetBaseDomain, and if they're equal then we don't call SetHost, but it's pretty workaround-ish so I'd love input from a Necko peer.
Assignee

Updated

9 months ago
Assignee: nobody → jdemooij
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Note that OriginAttributes::SetFirstPartyDomain seems to have some special-casing for this stuff already, but it's assuming that GetBaseDomain (on the tld service, not ThirdPartyUtil) will fail for ipv6 addresses...  I guess ThirdPartyUtil::GetBaseDomain swallows that error and just returns the ip address with the brackets stripped off, in that case.  :(

What you could still do is call GetAsciiHost and then check net_IsValidIPv6Addr, I guess.
Can you please be more specific what you expect from GetAsciiHost to return, how is it exactly used (like compared to what) and where the failing call to SetHost is made?

Stripping [] from ascii host goes back to bug 103916.  I haven't found and honestly don't remember the reason why it's done so.
Assignee

Comment 10

9 months ago
(In reply to Honza Bambas (:mayhemer) from comment #9)
> Can you please be more specific what you expect from GetAsciiHost to return,
> how is it exactly used (like compared to what) and where the failing call to
> SetHost is made?

What we're trying to do is to get the base domain to turn something like foo.bar.baz.com into just baz.com. So we get the base domain here:

https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/caps/ContentPrincipal.cpp#428

Then see comment 5 for the GetAsciiHost call happening there. Then we pass the result of that to SetHostPort here:

https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/caps/ContentPrincipal.cpp#462

GetBaseDomain/GetAsciiHost returns just "::" and not "[::]", and SetHost{Port} doesn't like that.
Thanks, got it!  

OK, then I think the best way is to have a new r/o attr on nsIURI asciiHostRaw that will not strip the ipv6 brackets.

We probably need to keep GetHost() (.asciiHost) do what it does now, as I recall we have tried to remove that strip in the past and many things broke.
Flags: needinfo?(honzab.moz)
Alternately we could have a URI mutator thing that replaces the host with the base domain (and maybe clears out all the other state we're manually clearing out here?)...  That would push all the logic down into necko instead of having consumers have to know about this stuff.
Assignee

Comment 13

9 months ago
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)
> Alternately we could have a URI mutator thing that replaces the host with
> the base domain (and maybe clears out all the other state we're manually
> clearing out here?)... 

Yeah I like that. One problem with asciiHostRaw is that I have to audit the GetBaseDomain call graph, so I might look into the mutator thing first.
The mutator makes sense, yes.
I just found this while following some link on IRC: https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/caps/OriginAttributes.cpp#38. Looks like OriginAttributes::SetFirstPartyDomain also has code to handle this case.
Assignee

Comment 16

9 months ago
Boris, how do you feel about changing this code to call GetBaseDomain on the tld service instead of going via ThirdPartyUtil? Then we can handle NS_ERROR_HOST_IS_IP_ADDRESS and NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS ourselves and just not call SetHostAndPort in these cases.

However ThirdPartyUtil::GetBaseDomain has some extra checks that I'm not sure are relevant here:

https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/dom/base/ThirdPartyUtil.cpp#304-318

Adding a new method to the URI mutator is a bit annoying because GetBaseDomainHelper is also used by ContentPrincipal::GetBaseDomain and I want to avoid duplicating that...
Flags: needinfo?(bzbarsky)
> how do you feel about changing this code to call GetBaseDomain on the tld service instead of going via ThirdPartyUtil? 

Seems reasonable to me for GetSiteOrigin.  I'm not sure about ContentPrincipal::GetBaseDomain...

> However ThirdPartyUtil::GetBaseDomain has some extra checks that I'm not sure are relevant here:

I think agree they are not relevant.  You don't care about getting the "right" base domain in some sense; you just want to replace the hostname with the base domain if there is a base domain distinct from the hostname.  If the URL has no hostname already, that's fine with you.  At least for purposes of ContentPrincipal::GetSiteOrigin.  I'm less sure what the right thing is for ContentPrincipal::GetBaseDomain.

> because GetBaseDomainHelper is also used by ContentPrincipal::GetBaseDomain

So the things GetBaseDomainHelper does, other than calling GetBaseDomain on thirdpartyutil, are to check for file:// URIs and the URI_NORELATIVE flag.  I guess the mutator would have to do the same, one way or another...
Flags: needinfo?(bzbarsky)
Assignee

Comment 18

9 months ago
Thanks. I think I'll try splitting off the file:// + URI_NORELATIVE cases handled in GetBaseDomainHelper into a new helper function and then GetBaseDomain can use ThirdPartyUtil and GetSiteOrigin can use the TLD service directly. I think that will satisfy all constraints without duplicating too much...
Assignee

Comment 19

9 months ago
The problem is that we used ThirdPartyUtil.getBaseDomain and for IP addresses that
returns the host, and for IPv6 addresses GetHost strips the '[' and ']' brackets.
Then when we passed that IP address to SetHost, we failed because SetHost wants
the brackets to be present.

This patch changes GetSiteOrigin to call getBaseDomain on the TLD service instead,
so we can handle this case ourselves by not calling SetHost when we have an IP
address. GetBaseDomain still uses ThirdPartyUtil.

I tried to add a test for this (with an iframe + postMessage) but the mochitest
http server doesn't support IPv6.
Comment on attachment 9011025 [details]
Bug 1491728 - Fix ContentPrincipal::GetSiteOrigin to handle IPv6 addresses correctly. r?bzbarsky

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.
Attachment #9011025 - Flags: review+

Comment 22

9 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9bdbc378f037
Fix ContentPrincipal::GetSiteOrigin to handle IPv6 addresses correctly. r=bzbarsky
Assignee

Updated

9 months ago
Flags: needinfo?(jdemooij)

Comment 23

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bdbc378f037
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.