1395820 - Acitivity Stream doesn't work on about:Home (note capitalization - not about:home)

Status: RESOLVED WONTFIX

(Firefox :: New Tab Page, defect, P5)

Description

[Phil K]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170831220208

Steps to reproduce:

When I type in about:Home (by mistake, not about:home) into URL bar, I am prompted whether to allow the site to store data on my computer. I presume this should not happen as I am not prompted to do so when opening about:home


Actual results:

User is asked "Will you allow Home to store data on your computer? with options "Allow Storing Data" and "Don't allow" 


Expected results:

This prompt should not pop up.

Comment 1

[Johann Hofmann [:johannh]]

Interesting. I can reproduce this, all the way to release. From a quick glance this could be related to the checks we're doing here: https://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/quota/ActorsParent.cpp#5429

But I might be wrong. janv probably knows.

Comment 2

[Jan Varga [:janv]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> Interesting. I can reproduce this, all the way to release. From a quick
> glance this could be related to the checks we're doing here:
> https://searchfox.org/mozilla-central/rev/
> f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/quota/ActorsParent.cpp#5429
> 
> But I might be wrong. janv probably knows.

You are right, but I would assume that about:Home or any other variant gets normalized to about:home before passed to quota manager, etc.

Comment 3

[Phil K]

With the New Tab page in 57 onwards, about:Home does not ask for permissions, but Top Sites and Highlights do not show up. This happens on 57 (developer edition) and 58. I presume this bug should be closed then?

Comment 4

[Johann Hofmann [:johannh]]

Hm, not sure. I'll leave that to the activity stream people.

Comment 5

[Ed Lee :Mardak]

Looks like the page isn't getting messages. (No `addMessageListener` or `sendAsyncMessage`.)

Probably because `RemotePageManager` does a case sensitive check `registeredURLs.has(url)`:
https://searchfox.org/mozilla-central/source/toolkit/modules/RemotePageManager.jsm#546-549

It does do some fuzzy url matching with `window.document.documentURI.replace(/[\#|\?].*$/, "");` so making it case insensitive might be acceptable. A potential security issue if someone were expecting a case sensitive match for some reason… (although it would be limited to the non-domain portion of the URL as domains, e.g., https://LOCALHOST/foo == https://localhost/foo but https://localhost/FOO != https://localhost/foo)

A related question of if we do support case insensitive matching, what do we report to listeners? The originally requested RemotePage url (about:home) or the actual current location (about:Home#foo) ?

Comment 6

[Dave Townsend [:mossop]]

(In reply to Ed Lee :Mardak from comment #5)
> Looks like the page isn't getting messages. (No `addMessageListener` or
> `sendAsyncMessage`.)
> 
> Probably because `RemotePageManager` does a case sensitive check
> `registeredURLs.has(url)`:
> https://searchfox.org/mozilla-central/source/toolkit/modules/
> RemotePageManager.jsm#546-549
> 
> It does do some fuzzy url matching with
> `window.document.documentURI.replace(/[\#|\?].*$/, "");` so making it case
> insensitive might be acceptable. A potential security issue if someone were
> expecting a case sensitive match for some reason… (although it would be
> limited to the non-domain portion of the URL as domains, e.g.,
> https://LOCALHOST/foo == https://localhost/foo but https://localhost/FOO !=
> https://localhost/foo)

I guess it's reasonable to do case insensitive matching on the hostname.

> A related question of if we do support case insensitive matching, what do we
> report to listeners? The originally requested RemotePage url (about:home) or
> the actual current location (about:Home#foo) ?

I'd normalize the hostname to lower-case, that way the caller doesn't need to think about it.

Comment 7

[Ed Lee :Mardak]

Ha ha. Meh. At least with `nsIURI`, "about:Home" `prePath` is "about:" while "https://LOCALHOST/foo"'s is "https://localhost" (because about URIs technically don't have `host`s.)

If we're fixing this, I think it might make sense to just special case loaded about: urls as lower case as special logic is needed to handle about: vs http/s: protocols anyway instead of some common `nsIURI.*` usage.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

How does nsIURI come into play here?

Comment 9

[Ed Lee :Mardak]

It's better security-wise if we aren't manually parsing urls and splitting them as slightly differing behaviors can lead to unexpected exploits. Although I suppose the code is already doing a regex match for # and ? to chop off the query and ref.

Comment 11

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 12

[Johann Hofmann [:johannh]]

I can still reproduce this. Did you mean to set WONTFIX instead?