browser.search.search: Remove requireUserInput

RESOLVED FIXED in Firefox 63

Status

enhancement
P3
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: robwu, Assigned: evilpie)

Tracking

({dev-doc-complete})

unspecified
mozilla64
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed, firefox64 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
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);
})
(Reporter)

Comment 1

9 months ago
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
See Also: → 1398833
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?
(Reporter)

Comment 3

9 months ago
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.
(Reporter)

Updated

9 months ago
Blocks: 1352598
See Also: → 1476415
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.
Severity: normal → enhancement
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → WONTFIX
(Assignee)

Comment 5

8 months ago
See also https://github.com/tridactyl/tridactyl/issues/792#issuecomment-417839540. This changes makes browser.search useless to that addon.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 6

8 months ago
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.
(Assignee)

Updated

8 months ago
Flags: needinfo?(mozilla)
(Assignee)

Comment 7

8 months ago
See also bug 1352598 comment 138
I'll remove. We had a reason initially but I don't remember the justification.
Flags: needinfo?(mozilla)
(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.
(Assignee)

Updated

8 months ago
Assignee: nobody → evilpies
(Assignee)

Comment 11

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

We should also uplift this change to Firefox 63.
Attachment #9006479 - Flags: review?(mozilla)
I think mconca should chime in prior to this landing.
Flags: needinfo?(mconca)
Comment on attachment 9006479 [details] [diff] [review]
browser.search.search: Remove requireUserInput. r?

LGTM. Need the feedback from mconca.
Attachment #9006479 - Flags: review?(mozilla) → review+
(Assignee)

Comment 14

8 months ago
Too bad that mconca seems to be away a few more days, I was hoping to get this into Beta as soon as possible.
We could ask mconnor as well, but I'm not clear around the urgency to get this into beta.
Flags: needinfo?(mconnor)
Priority: -- → P3
(Reporter)

Comment 16

8 months ago
(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.
Ok, that makes sense.  However since mconca disagreed with removal, I want product to chime in.  Either him or mconnor.
(Assignee)

Comment 18

7 months ago
Any news?
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.
Flags: needinfo?(mconca)
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.
Flags: needinfo?(mconnor)
Comment on attachment 9006479 [details] [diff] [review]
browser.search.search: Remove requireUserInput. r?

blocking review while I take a look at the test changes.
Attachment #9006479 - Flags: review?(mixedpuppy)
Can this patch be moved to phab?
Flags: needinfo?(evilpies)
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+
Attachment #9006479 - Flags: review?(mixedpuppy) → review+
(Reporter)

Comment 24

7 months ago
(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.
(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.
(Assignee)

Comment 26

7 months ago
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?)
Flags: needinfo?(evilpies)
Attachment #9010437 - Flags: review?(mixedpuppy)
Comment on attachment 9010437 [details] [diff] [review]
Bug 1477248 - Simplify tests.r ?

Thanks!
Attachment #9010437 - Flags: review?(mixedpuppy) → review+
(Assignee)

Updated

7 months ago
Keywords: dev-doc-needed

Comment 28

7 months ago
Pushed by evilpies@gmail.com:
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
(Assignee)

Comment 29

7 months ago
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)
Attachment #9006479 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #9010574 - Attachment description: Landed patch to requireUserInput → Landed patch to remove requireUserInput
(Assignee)

Comment 30

7 months ago
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]:
Attachment #9010574 - Flags: approval-mozilla-beta?

Comment 31

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ada62d24880b
https://hg.mozilla.org/mozilla-central/rev/a15b3cd9ec97
Status: REOPENED → RESOLVED
Last Resolved: 9 months ago7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Updated

7 months ago
Flags: qe-verify-
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.
Attachment #9010574 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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
(Assignee)

Comment 36

6 months ago
I think this is rather confusing, this change was uplifted to 63.
You need to log in before you can comment on or make changes to this bug.