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

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P2)

Description

[YUKI "Piro" Hiroshi]

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.

Comment 1

[YUKI "Piro" Hiroshi]

I've confirmed this problem on Firefox ESR60.0.1.

Comment 2

[YUKI "Piro" Hiroshi]

There is a related topic: bug 1463680 for the encoding of parameters.

Comment 3

[YUKI "Piro" Hiroshi]

Oops, the bug number was wrong. Correct: bug 1463684

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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

Comment 5

[YUKI "Piro" Hiroshi]

(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

Comment 6

[Mike Kaply [:mkaply]]

POST isn't supported for any of our search engine APIs, so it can't be added here easily without that change.

Comment 7

[James Pearson]

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

Comment 8

[Mike Kaply [:mkaply]]

> 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.

Comment 9

[Mike Kaply [:mkaply]]

Can you try this again?

I added support for POST engines

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

Comment 10

[YUKI "Piro" Hiroshi]

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.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

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

Comment 12

[YUKI "Piro" Hiroshi]

OK, I may try that at the next week.

Comment 13

[YUKI "Piro" Hiroshi]

Attachment: Bug 1463680 - Allow to install search engine with POST method via Policy Engine

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa4298251cf2
Allow to install search engine with POST method via Policy Engine r=Felipe

Comment 15

[Cristina Coroiu [:ccoroiu]]

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 - asyncTester_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

Comment 16

[YUKI "Piro" Hiroshi]

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...

Comment 17

[YUKI "Piro" Hiroshi]

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.

Comment 18

[YUKI "Piro" Hiroshi]

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?

Comment 19

[Mike Kaply [:mkaply]]

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

Comment 20

[YUKI "Piro" Hiroshi]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa0aa1dc80b3
Allow to install search engine with POST method via Policy Engine r=Felipe

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/aa0aa1dc80b3

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Mike Kaply [:mkaply]]

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

Comment 25

[Mike Kaply [:mkaply]]

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:

Comment 26

[YUKI "Piro" Hiroshi]

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

Comment 27

[Mike Kaply [:mkaply]]

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

Comment 28

[YUKI "Piro" Hiroshi]

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

Comment 29

[Catalin Sasca, Desktop QA [:csasca]]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Per comment 28.