Policy: SearchEngines.Add cannot add effective search engine with POST method

VERIFIED FIXED in Firefox 67

Status

()

P2
normal
VERIFIED FIXED
10 months ago
16 days ago

People

(Reporter: yuki, Assigned: yuki)

Tracking

Trunk
Firefox 67
Points:
---

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox62 wontfix, firefox67 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
Policy Engine now allows me to add search engine via policies.json (bug 1428948):

--------------
{
  "policies": {
    "SearchEngines": {
      "Add": [
        {
          "Name":               "Name1",
          "IconURL":            "https://example.com/favicon1.ico",
          "Alias":              "ex1",
          "Description":        "Description1",
          "Method":             "GET",
          "URLTemplate":        "https://example.com/search1?q={searchTerms}",
          "SuggestURLTemplate": "https://example.com/suggest1?q={searchTerms}"
        },
        {
          "Name":               "Name2",
          "IconURL":            "https://example.com/favicon2.ico",
          "Alias":              "ex2",
          "Description":        "Description2",
          "Method":             "POST",
          "URLTemplate":        "https://example.com/search2?q={searchTerms}",
          "SuggestURLTemplate": "https://example.com/suggest2?q={searchTerms}"
        }
      ]
    }
  }
}
--------------

However, search engine added with "POST" method does nothing.


Steps to reproduce:

 1. Save above example as a file "distribution/policies.json".
 2. Start Firefox.
 3. Go to about:config and set "devtools.chrome.enabled" and
    "devtools.debugger.remote-enabled" to "true".
 3. Start Browser Toolbox (Ctrl-Shift-Alt-I) and choose the "Network" tab.
 3. Focus to the location bar in the browser window.
 4. Type "ex2 foo" and hit Enter key.

Expected result:
Something "POST" HTTP request is logged.

Actual result:
Nothing is logged.


Additional information:

Schema definition:
https://dxr.mozilla.org/mozilla-esr60/source/browser/components/enterprisepolicies/schemas/policies-schema.json#562
Logic to add new search engine based on the given policy:
https://dxr.mozilla.org/mozilla-esr60/source/browser/components/enterprisepolicies/Policies.jsm#649
Implementation of the method called to add new search engine:
https://dxr.mozilla.org/mozilla-esr60/source/toolkit/components/search/nsSearchService.js#3853
Interface definition of the method:
https://dxr.mozilla.org/mozilla-esr60/source/netwerk/base/nsIBrowserSearchService.idl#327

After I researched, I realized that there is no field to define POST parameters in the schema, method implementation, and the interface. Thus I think we need to do two changes:

 1. Add new fields definiing POST parameters to the schema.
 2. Add new parameters to the method receiving definitions of POST parameters.
(Assignee)

Updated

10 months ago
status-firefox-esr60: --- → affected
(Assignee)

Comment 1

10 months ago
I've confirmed this problem on Firefox ESR60.0.1.
(Assignee)

Comment 2

10 months ago
There is a related topic: bug 1463680 for the encoding of parameters.
(Assignee)

Comment 3

10 months ago
Oops, the bug number was wrong. Correct: bug 1463684
How does a POST engine properly work through other means (i.e., not through addEngineWithDetails?)

My understanding is that the search query is encoded as is and sent in the post data, isn't it?

https://dxr.mozilla.org/mozilla-esr60/source/toolkit/components/search/nsSearchService.js#1131
Flags: needinfo?(yuki)
Priority: -- → P2
(Assignee)

Comment 5

10 months ago
(In reply to :Felipe Gomes (needinfo me!) from comment #4)
> How does a POST engine properly work through other means (i.e., not through
> addEngineWithDetails?)
> 
> My understanding is that the search query is encoded as is and sent in the
> post data, isn't it?

The POST body will become a application/x-www-form-urlencoded string like:

    q={searchTerm}&ie=UTF-8&...

To allow to define multiple parameters (because there can be multiple parameter values need to be encoded), I think the method need to accept enough information to define search engine like an OpenSearch plugin:
https://developer.mozilla.org/en-US/docs/Web/OpenSearch
Flags: needinfo?(yuki)
POST isn't supported for any of our search engine APIs, so it can't be added here easily without that change.

Comment 7

9 months ago
Would it be 'easier' to allow OpenSearch plugin XML files to be specified via the Search Engines policy instead of using its own method?
> Would it be 'easier' to allow OpenSearch plugin XML files to be specified via the Search Engines policy instead of using its own method?

We thought about that originally, but it seemed to make more sense to emulate the WebExtension behavior.

Can you try this again?

I added support for POST engines

https://bugzilla.mozilla.org/show_bug.cgi?id=1477076

Flags: needinfo?(yuki)
(Assignee)

Comment 10

a month ago

OK, the nsSearchService now supports registering of an POST search engine.
However there is still missing parameter to define postData for the engine via policies.

https://dxr.mozilla.org/mozilla-esr60/source/browser/components/enterprisepolicies/schemas/policies-schema.json#723

This sections needs to have a new parameter like:

"PostData": {
  "type": "string"
}

and the logic to register engines from policies ( https://dxr.mozilla.org/mozilla-esr60/source/browser/components/enterprisepolicies/Policies.jsm#807 ) needs to have postData: newEngine.PostData.

Flags: needinfo?(yuki)

Thanks for the investigation, Piro! Would you be interested in writing the patch for that? I'll happily review and land it

Flags: needinfo?(yuki)
(Assignee)

Comment 12

a month ago

OK, I may try that at the next week.

Flags: needinfo?(yuki)
(Assignee)

Updated

a month ago
Assignee: nobody → yuki
(Assignee)

Updated

a month ago
Attachment #9043212 - Flags: review?(felipc)

Comment 14

a month ago
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa4298251cf2
Allow to install search engine with POST method via Policy Engine r=Felipe

Backed out changeset fa4298251cf2 (bug 1463680) for browser-chrome failures at browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js

Backout: https://hg.mozilla.org/integration/autoland/rev/c3fde50a85f19db37677fd7aa8ab71b556b09d13

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=227918526&revision=fa4298251cf20d0bc0e9be7c872a72f13aa270d4

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227918526&repo=autoland&lineNumber=4955

06:39:41 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js | Sanity check the temporary file doesn't exist. - true == true -
06:39:41 INFO - Buffered messages finished
06:39:41 INFO - TEST-UNEXPECTED-FAIL | browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js | Specified search engine should be installed - Didn't expect null, but got it
06:39:41 INFO - Stack trace:
06:39:41 INFO - chrome://mochikit/content/browser-test.js:test_isnot:1319
06:39:41 INFO - chrome://mochitests/content/browser/browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:test_install_post_method_engine:269
06:39:41 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
06:39:41 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1097
06:39:41 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:995
06:39:41 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
06:39:41 INFO - Not taking screenshot here: see the one that was previously logged
06:39:41 INFO - TEST-UNEXPECTED-FAIL | browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:271 - TypeError: engine is null
06:39:41 INFO - Stack trace:
06:39:41 INFO - test_install_post_method_engine@chrome://mochitests/content/browser/browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:271:3
06:39:41 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1106:34
06:39:41 INFO - async
Tester_execTest@chrome://mochikit/content/browser-test.js:1097:16
06:39:41 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:995:9
06:39:41 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
06:39:41 INFO - Leaving test bound test_install_post_method_engine
06:39:41 INFO - TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js | Engine is inactive at the end of the test -
06:39:41 INFO - GECKO(861) | MEMORY STAT | vsize 4632MB | residentFast 616MB | heapAllocated 202MB
06:39:41 INFO - TEST-OK | browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js | took 8629ms
06:39:41 INFO - GECKO(861) | ++DOCSHELL 0x115ce8000 == 1 [pid = 863] [id = {b0e0321c-7dcb-b747-b862-b98b332101da}]
06:39:41 INFO - GECKO(861) | ++DOMWINDOW == 1 (0x10ee85800) [pid = 863] [serial = 75] [outer = 0x0]
06:39:41 INFO - GECKO(861) | ++DOMWINDOW == 2 (0x10eed3400) [pid = 863] [serial = 76] [outer = 0x10ee85800]
06:39:41 INFO - checking window state

Flags: needinfo?(yuki)
(Assignee)

Comment 16

a month ago

The failure didn't reproduced on the tryserver with a command line: ./mach try -b d -p macosx64 -u none -t none. Result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=823a09880619d7362b79004f06be216e8ad682f8
Hmm...

Flags: needinfo?(yuki)
(Assignee)

Comment 17

a month ago

Sorry I misunderstood usage of the TryChooser Syntax Builder. The command line should be: ./mach try -b d -p macosx64 -u all[10.10] -t none. Now I'm retrying.

(Assignee)

Comment 18

a month ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75cbffcc3426378fa729d53568f968e6d2ef1dd6
The error was not reported on the tryserver. Of course it doesn't happen on my local environment. Does anyone know what causes this failure?

I was able to recreate locally.

I believe this has to do with the new async search stuff.

Fix is pretty straightforward. Add these two lines to the new test:

https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js#220

// Get in line, because the Search policy callbacks are async.
await TestUtils.waitForTick();

with this change, the test is passing

(Assignee)

Comment 20

a month ago

(In reply to Mike Kaply [:mkaply] from comment #19)

I believe this has to do with the new async search stuff.

Thanks! I'm going to update the patch.

Comment 21

26 days ago
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa0aa1dc80b3
Allow to install search engine with POST method via Policy Engine r=Felipe

Comment 22

26 days ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 26 days ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Is this something needed on ESR60 or can it ride with release?

status-firefox62: affected → wontfix
Flags: needinfo?(mozilla)

This policy is ESR only, so this would be nice to have there.

Flags: needinfo?(mozilla)

Comment on attachment 9043212 [details]
Bug 1463680 - Allow to install search engine with POST method via Policy Engine

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Nice fix to search engine addition in ESR (IT's only available there)
  • User impact if declined: Policies can't use POST engines
  • Fix Landed on Version: 67
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Very small change to policy
  • String or UUID changes made by this patch:
Attachment #9043212 - Flags: review?(felipc) → approval-mozilla-esr60?
(Assignee)

Comment 26

19 days ago

The patch depends on the feature introduced by the bug 1477076, so its change is also need to be uplifted to ESR.

You're right, I forgot that. Do you mind if it waits to 68 esr?

Flags: qe-verify+
(Assignee)

Comment 28

16 days ago

I'm OK to wait for the next ESR 68. Currently there is no immediate need to use this feature on my cases.

QA Whiteboard: [qa-triaged]

I successfully reproduced the issue on Firefox 62.0a1 (2018-05-23) under Ubuntu 18.04 (x64) using the STR from Comment 0.

The issue is no longer reproducible on Firefox 67.0a1 (2019-03-03). Tests were performed under Ubuntu 18.04 (x64), Windows 10 (x64) and macOS 10.14.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
status-firefox67: fixed → verified

Comment on attachment 9043212 [details]
Bug 1463680 - Allow to install search engine with POST method via Policy Engine

Per comment 28.

Attachment #9043212 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
status-firefox-esr60: affected → wontfix
You need to log in before you can comment on or make changes to this bug.