Closed Bug 1634650 Opened 4 years ago Closed 4 years ago

Whitelisting domain suffixes for search queries

Categories

(Firefox :: Address Bar, enhancement, P2)

Product:
Firefox
Component:
Address Bar
Type:
enhancement
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 78
Iteration:
78.1 - May 4 - May 17
Tracking Flags:
Tracking Status
firefox78 --- fixed

People

(Reporter: filip.stamcar, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1634650 - Add whitelisting of domain suffixes for URIFixup. r=harry
47 bytes, text/x-phabricator-request
Details | Review

Currently, it is only possible to whitelist complete domains and prevent them from redirecting to search using browser.fixup.domainwhitelist.yourdomain.yoursuffix.

It should also be possible to also whitelist whole suffixes/TLDs. This is useful in case of local development using reserved TLDs (bug 1634592) because users don't need to manually whitelist every domain. It is also useful with non-IANA-managed TLDs like OpenNIC or extensions that add support for domains of some other project (like .eth or .bit).

Although reserved TLDs and OpenNIC domains use a custom DNS server, so Firefox can use that to offer users to automatically whitelist a certain domain, this can't be said for extension-based domains. For such domains, the only way of accessing them is to manually whitelist every domain in about:config or manually type http:// before the domain.

It should also be possible to also whitelist whole suffixes/TLDs. With this, users could just add, for example, .local, .libre or .eth as a whitelisted suffix and all search queries with that suffix will be interpreted as website/domain request.

Additionally, API for whitelisting suffixes/TLDs could be exposed to extensions. This would be useful in cases where extensions are used for handling custom domains (like MetaMask for .eth, PeerName or Blockchain DNS for .bit, .emc and few others...). Without such API, users would have to install such extension and them manually whitelist suffix/TLD that extension uses. With such API, users would just need to install the extension, and extension would automatically register TLDs that it handles as whitelisted domain TLDs.

I have some idea how whitelisting suffixes could look like:

Pref could be something like browser.fixup.suffixwhitelist.yoursuffix, so all searches which end with .yoursuffix would be considered as URLs. Additionally, if pref is set to, for example browser.fixup.suffixwhitelist.local.domain, searches which end with .local.domain would be considered as URLs, but searches ending on just .domain won't be.

For extensions API for whitelisting TLDs, there are more approaches:

  1. If an extension requires host permission (in the manifest) for specific domain/URL, automatically treat that URL as whitelisted domain/suffix. For example, if extension requires *://*.bit/*, automatically treat all searches ending with .bit as whitelisted suffixes, and if extension requires *://*.local.domain, treat searches ending with .local.domain as URLs. The benefit is that it doesn't require changing most of the extensions. However, the problem with this is that it is not really configurable. But much configuration is probably not really needed here, because if extension wants to access websites on some suffix, we can assume that that suffix contains websites.

  2. Providing another manifest permission similar to host permission, which is only used to tell Firefox that specific suffix should be whitelisted. The advantage is that it is more configurable. Disadvantages are that the extension would need to be updated to start using it, and would probably result in duplicated configuration/permissions (you would need to set both normal host permission and this new "suffix permission" in manifest). And in most, if not all, cases, separate configuration probably won't be needed.

  3. Exposing API to JavaScript in some way. The advantage is that it is dynamic. Disadvantage is also that is dynamic, which means that is is probably harder to handle it, and can also cause security and usability problems. And probably also results in duplicated configuration.

Of those three approaches, I most like first, because it doesn't result in too much configuration and can work with existing extensions.

Blocks: qb-results-papercuts
Status: UNCONFIRMED → NEW
Points: --- → 3
Ever confirmed: true
Priority: -- → P2

We should implement the whitelist prefs here, For clarity I think we should go with browser.fixup.domainsuffixwhitelist.yoursuffix
(I would have preferred a more readable name but that train left so we're stuck with this).
By default we should whitelist the reserved TLDs from bug 1634592.
It may be useful to also have a policy.

I don't think we can add suffix providers as prefs though, we'd need an updated list in the product, like the PSL, to support an high number of suffixes, we don't have such a list.
A WebExtension API is an interesting idea, but honestly it should be filed separately. I must note though that currently the isDomainWhitelisted API we use is synchronous and it would not cope well with the async WebExt API, thus someone should either fix all the isDomainWhitelisted call points (not trivial) or figure out an API that just updates an in-memory list asynchronously, and accept that some hits may fail. Though can't WebExt already intercept traffic? If so it looks like it would be possible to redirect it already.
Anyway, please file any WebExt request apart, we don't have the resources to do it here.

Mike, do we have a policy for browser.fixup.domainwhitelist? could you please point to the code for it?
Is it just managed though autoconfig?

Flags: needinfo?(mozilla)
Blocks: 1635062
Assignee: nobody → mak
Blocks: 1180329
No longer blocks: qb-results-papercuts
Status: NEW → ASSIGNED
Iteration: --- → 78.1 - May 4 - May 17

We don't have a policy for this, it's being managed through autoconfig.

The only fixup policy we have is:

browser.fixup.dns_first_for_single_words

although there have been requests for others.

Flags: needinfo?(mozilla)

I guess either policies or a WebExt API as requested in bug 1635062 may be useful to setup suffixes collections.
Atm we don't have the resources in the Search team to look after those, hopefully the community can pick up those items and provide solutions that would be nicer than autoconfig.

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/57d78ba110be
Add whitelisting of domain suffixes for URIFixup. r=harry

it looks intermittent from treeherder and there's bug 1614678 filed about it... did you respin and assigned to my bug because the previous changeset had no failures at all?

Flags: needinfo?(mak) → needinfo?(dluca)

The problem is I don't see anything in test_refresh_firefox.py that may be affected by my changes...

Ok I see the retriggers, I'm totally puzzled, I think some timing changes made an intermittent a bit more frequent, for now I don't have other ideas.

Flags: needinfo?(dluca)

(In reply to Marco Bonardo [:mak] from comment #9)

it looks intermittent from treeherder and there's bug 1614678 filed about it... did you respin and assigned to my bug because the previous changeset had no failures at all?

Note that this bug is about an exit code of 0 whilst yours show an exit code of 11. As Gijs mentioned this is a crash. Interesting that no minidump files are found or processed.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #12)

Note that this bug is about an exit code of 0 whilst yours show an exit code of 11. As Gijs mentioned this is a crash. Interesting that no minidump files are found or processed.

It must be a crash happening due to some timing changes, we can't exclude this isn't the same failure with different outcomes, due to the code changes in the last 2 months.
My changes should only affect page loads starting from user input, and afaict there's nothing like that in this test.
Without any kind of stack though I wonder how we can figure out anything, and this is also an intermittent, there are green runs on and after my push.
It seems to only affect Linux 18.04 x64 shippable opt, that makes me think if we can't get a useful stack, we may have to skip-if = os == "linux" && bits == 64 && !debug, but of course I'd prefer to find what's up.

Interesting. I wonder if this might be something related to bug 1607713, which is a high intermittent crash with the "new" Ubuntu 18.04 jobs.

Maybe we could try to run the Marionette job with the old 16.04 image on try and check if the test passes fine there with a couple of retriggers. I would assume we have to add a worker setting like for our puppeteer jobs to the Marionette Taskcluster configs.

Flags: needinfo?(egao)

So, we are in testResetEverything.
The restart we crash on is apparently the one after the first checkProfile.

One thing I don't understand, by looking at a PASS log, is why after the first self.checkProfile(expect_sync_user=True) and the restart, I cannot find traces of the second self.checkProfile(has_migrated=True, expect_sync_user=True), I instead see a TEST-PASS just after the restart.
For example, looking for the string "services.sync.username" in the log, I see it set by self.createProfileData() and then get by the first self.checkProfile(expect_sync_user=True), then I don't see it again, but the second self.checkProfile call should get it again, it's like we don't run the second checkProfile at all.

I downloaded the build and tried to run the test on Ubuntu 18.04, I was never able to crash it. I'll file a separate bug for it and likely disable on that platform there, we'll have to figure out why there's no minidump, and whether this is related to bug 1607713. But There's no change I can do to the patch to avoid this, afaict.

See Also: → 1636706

As a heads up, if you're still interested in running Marionette with the old 16.04 image (still in-tree), you could remove these clauses and it will revert: https://searchfox.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests.py#621-623

Flags: needinfo?(egao)

I'll do a Try push with those lines removed and annotate the results in bug 1636706

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/0230d23f652f
Add whitelisting of domain suffixes for URIFixup. r=harry

https://hg.mozilla.org/mozilla-central/rev/0230d23f652f

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78

Thank you for fixing this! How do you feel about uplifting this, so this is in the same release as Bug 1080682?

Flags: needinfo?(mak)

I can't uplift atm, there's a crash bug that I'm still investigating and that makes me nervous and doubtful about uplifts.

Flags: needinfo?(mak)

Is it possible this is causing a regression in golinks as per bug 1642435?

(In reply to Sagar from comment #23)

Is it possible this is causing a regression in golinks as per bug 1642435?

not this bug.

See Also: → 1731034
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: