Closed Bug 1477248 Opened 2 years ago Closed 2 years ago

browser.search.search: Remove requireUserInput

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: robwu, Assigned: evilpie)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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);
})
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?
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.
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
Closed: 2 years ago
Resolution: --- → WONTFIX
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 → ---
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.
Flags: needinfo?(mozilla)
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: nobody → evilpies
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+
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
(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.
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+
(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.
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+
Keywords: dev-doc-needed
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
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
Attachment #9010574 - Attachment description: Landed patch to requireUserInput → Landed patch to remove requireUserInput
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?
https://hg.mozilla.org/mozilla-central/rev/ada62d24880b
https://hg.mozilla.org/mozilla-central/rev/a15b3cd9ec97
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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
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.