1477248 - browser.search.search: Remove requireUserInput

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Rob Wu [:robwu]]

The browser.search.search API currently requires user input.

This user interaction should not be required, because the API is not dangerous (and no more dangerous than performing a navigation).

Having this user gesture requirement reduces the usefulness of the API, and extensions could ignore the browser.search API and open a hard-coded search engine, which reduces the control of users.

As an example of a use case, consider an extension that adds a menu item to start searches in a private browsing window or container tab, i.e.:

browser.menus.onClicked.addListener(async (info, tab) {
   let win = await browser.windows.create({incognito: true});
   let tabId = win.tabs[0].id;
   browser.search.search("...", info.selectionText);
})

Comment 1

[Rob Wu [:robwu]]

I found out about this new API and the unnecessary user gesture requirement because of this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1398833#c6

Comment 2

[Mike Kaply [:mkaply]]

I honestly don't remember the reason that I added the user input requirement. It might have been an earlier version of the API that did more.

Does my use of tabs create mean that the search doesn't work if you don't have the tabs permission?

Comment 3

[Rob Wu [:robwu]]

The tabs permission is only needed to view the URL, title and icon of tabs.

tabs.create can be used without permissions.
In the example above (change ") {" to ") => {" by the way) , browser.search.search does not work because the user gesture is lost the moment that another async API was invoked.

Comment 4

[Mike Conca [:mconca]]

The requirement for user input is to prevent search hijacking and monetization of search without user knowledge. Without this requirement, malicious extensions can use the search API to perform searches on any installed engine as often as they like. The potential for abuse outweighs the limitations that the requirement imposes.

Comment 5

[Tom S [:evilpie]]

See also https://github.com/tridactyl/tridactyl/issues/792#issuecomment-417839540. This changes makes browser.search useless to that addon.

Comment 6

[Tom S [:evilpie]]

I don't see the justification for the abuse argument. Unless this is a POST based search engine, the extension can just use browser.tabs.create and add our magic parameters to the search query.

Comment 7

[Tom S [:evilpie]]

See also bug 1352598 comment 138

Comment 8

[Mike Kaply [:mkaply]]

I'll remove. We had a reason initially but I don't remember the justification.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Tom Schuster [:evilpie] from comment #6)
> I don't see the justification for the abuse argument. Unless this is a POST
> based search engine, the extension can just use browser.tabs.create and add
> our magic parameters to the search query.

I agree, unless we were to prevent opening tabs to search engines via tabs.create and only allow it through the search api.  That's probably not a great idea to do, but haven't thought it through.

Comment 10

[Tom S [:evilpie]]

Attachment: browser.search.search: Remove requireUserInput. r?

Comment 11

[Tom S [:evilpie]]

Comment on attachment 9006479 [details] [diff] [review]
browser.search.search: Remove requireUserInput. r?

We should also uplift this change to Firefox 63.

Comment 12

[Shane Caraveo (:mixedpuppy)]

I think mconca should chime in prior to this landing.

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 9006479 [details] [diff] [review]
browser.search.search: Remove requireUserInput. r?

LGTM. Need the feedback from mconca.

Comment 14

[Tom S [:evilpie]]

Too bad that mconca seems to be away a few more days, I was hoping to get this into Beta as soon as possible.

Comment 15

[Shane Caraveo (:mixedpuppy)]

We could ask mconnor as well, but I'm not clear around the urgency to get this into beta.

Comment 16

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> We could ask mconnor as well, but I'm not clear around the urgency to get
> this into beta.

If we decide to drop the user interaction requirement, then getting it into Beta (63) is desirable because Fiefox 63 is the first release that supports the browser.search API. The patch is trivial and not having to account for the user gesture requirement makes using the API considerably easier.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Ok, that makes sense.  However since mconca disagreed with removal, I want product to chime in.  Either him or mconnor.

Comment 18

[Tom S [:evilpie]]

Any news?

Comment 19

[Mike Conca [:mconca]]

This change was discussed with the search PM and Eng leads (:mconnor and :mikedeboer). Both are comfortable with the change. I've asked :mconnor to respond here with his approval and, pending that action, this patch can land.

Comment 20

[Mike Connor [:mconnor]]

Agreed on removing the restriction. There's some legit concerns about search hijacking, but I don't think this restriction is the way to resolve those.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9006479 [details] [diff] [review]
browser.search.search: Remove requireUserInput. r?

blocking review while I take a look at the test changes.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Can this patch be moved to phab?

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9006479 [details] [diff] [review]
browser.search.search: Remove requireUserInput. r?

I think that test_search_notab, test_search_from_options_page, test_search_from_sidebar tests can be removed.  

test_search and test_search_default_engine can remove the onclicked handling, and dont forget the browser_action in manifest.

With those changes, r+

Comment 24

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #23)
> Comment on attachment 9006479 [details] [diff] [review]
> browser.search.search: Remove requireUserInput. r?
> 
> I think that test_search_notab, test_search_from_options_page,

test_search_notab is the only test that verifies that a new tab is opened when tabId is not set.
That test coverage should be preserved, for example by merging test_search and test_search_notab.

The test_search_default_engine at the end already tests the browser.search.search method with the tabId set.

Comment 25

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #24)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #23)
> > Comment on attachment 9006479 [details] [diff] [review]
> > browser.search.search: Remove requireUserInput. r?
> > 
> > I think that test_search_notab, test_search_from_options_page,
> 
> test_search_notab is the only test that verifies that a new tab is opened
> when tabId is not set.

Good catch.  most of these tests are highly replicated, could probably all move into a single test and test: no tabId, tabId, search engine name, default search engine.

Comment 26

[Tom S [:evilpie]]

Attachment: Bug 1477248 - Simplify tests.r ?

Unifying the custom search engine and default search engine test seemed to complicated for me.

I will set up phabricator locally at some later date. (php really?)

Comment 27

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9010437 [details] [diff] [review]
Bug 1477248 - Simplify tests.r ?

Thanks!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada62d24880b
browser.search.search: Remove requireUserInput. r=mkaply
https://hg.mozilla.org/integration/mozilla-inbound/rev/a15b3cd9ec97
Simplify tests. r=mixedpuppy

Comment 29

[Tom S [:evilpie]]

Attachment: Landed patch to remove requireUserInput

I split out the test change from "browser.search.search: Remove requireUserInput. r?" that got reviewed by mkaply. We should uplift this patch instead. (As committed to m-i)

Comment 30

[Tom S [:evilpie]]

Comment on attachment 9010574 [details] [diff] [review]
Landed patch to remove requireUserInput

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352598 (initial landing of this feature)
[User impact if declined]: API is useless for many extensions. WebExtensions have to explicitly check for the Firefox version. This is a new API in beta, so we have the chance to fix this before release.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Removes an additional restriction to this webextension API that is unnecessary
[String changes made/needed]:

Comment 31

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ada62d24880b
https://hg.mozilla.org/mozilla-central/rev/a15b3cd9ec97

Comment 32

[Pascal Chevrel:pascalc]

Comment on attachment 9010574 [details] [diff] [review]
Landed patch to remove requireUserInput

Minimal patch fixing a well defined bug, uplift approved for 63 beta 9, thanks.

Comment 33

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cef6e1feaaa3
https://hg.mozilla.org/releases/mozilla-beta/rev/7c24f95da514

Comment 34

[Tom S [:evilpie]]

Removed the sentence about user actions on https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/search/search.

Comment 35

[Chris Mills [:cmills]]

(In reply to Tom Schuster [:evilpie] from comment #34)
> Removed the sentence about user actions on
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> search/search.

I've added a note to the Fx 64 rel notes to cover this too:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#Changes_for_add-on_developers

Comment 36

[Tom S [:evilpie]]

I think this is rather confusing, this change was uplifted to 63.