Bug 1960237 - Only offer "Add Search Engine" for forms with an explicit action attribute. r=search-reviewers,scunnane

Differential Revision: https://phabricator.services.mozilla.com/D247775

browser/actors/ContextMenuChild.sys.mjs

@@ -241,7 +241,7 @@ export class ContextMenuChild extends JSWindowActorChild {
           !node.name ||
           (method != "POST" && method != "GET") ||
           node.form.enctype != "application/x-www-form-urlencoded" ||
-          formData.entries().some(([k, v]) => !k && typeof v != "string")
+          formData.values().some(v => typeof v != "string")
         ) {
           // This should never happen since these conditions are checked in
           // `isTargetASearchEngineField`.

browser/components/search/test/browser/browser_addSearchEngineFromForm.js

@@ -3,6 +3,10 @@
 
 "use strict";
 
+const { SpellCheckHelper } = ChromeUtils.importESModule(
+  "resource://gre/modules/InlineSpellChecker.sys.mjs"
+);
+
 ChromeUtils.defineLazyGetter(this, "UrlbarTestUtils", () => {
   const { UrlbarTestUtils: module } = ChromeUtils.importESModule(
     "resource://testing-common/UrlbarTestUtils.sys.mjs"
@@ -66,12 +70,6 @@ const URL_UTF_8 =
 const URL_WINDOWS1252 =
   "https://example.org/browser/browser/components/search/test/browser/test_windows1252.html";
 
-add_setup(async function () {
-  await SpecialPowers.pushPrefEnv({
-    set: [["browser.urlbar.update2.engineAliasRefresh", true]],
-  });
-});
-
 async function addEngine(browser, selector, name, alias) {
   let contextMenu = document.getElementById("contentAreaContextMenu");
   let addEngineItem = document.getElementById("context-add-engine");
@@ -164,7 +162,27 @@ async function navigateToCharset(charset) {
   }
 }
 
-add_task(async function () {
+function postDataToString(postData) {
+  if (!postData) {
+    return undefined;
+  }
+  let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].createInstance(
+    Ci.nsIBinaryInputStream
+  );
+  binaryStream.setInputStream(postData.data);
+
+  return binaryStream
+    .readBytes(binaryStream.available())
+    .replace("searchTerms", "%s");
+}
+
+add_setup(async function () {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.urlbar.update2.engineAliasRefresh", true]],
+  });
+});
+
+add_task(async function testAddingEngines() {
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
   let browser = tab.linkedBrowser;
 
@@ -187,7 +205,7 @@ add_task(async function () {
       "Submission URI is correct"
     );
     Assert.equal(
-      postDataToString(submission.postData),
+      SearchTestUtils.getPostDataString(submission),
       args.expectedPost,
       "Submission post data is correct."
     );
@@ -202,16 +220,56 @@ add_task(async function () {
   BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
 
-function postDataToString(postData) {
-  if (!postData) {
-    return undefined;
-  }
-  let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].createInstance(
-    Ci.nsIBinaryInputStream
-  );
-  binaryStream.setInputStream(postData.data);
+add_task(async function testSearchFieldDetection() {
+  let form = document.createElement("form");
+  form.method = "GET";
+  form.action = "/";
 
-  return binaryStream
-    .readBytes(binaryStream.available())
-    .replace("searchTerms", "%s");
-}
+  let input = document.createElement("input");
+  input.type = "search";
+  input.name = "q";
+  form.appendChild(input);
+
+  let isSearchField = SpellCheckHelper.isTargetASearchEngineField(
+    input,
+    window
+  );
+  Assert.equal(isSearchField, true, "Is search field initially");
+
+  // We test mostly non search fields here since valid search fields
+  // are already tested in testAddingEngines.
+  delete form.removeAttribute("action");
+  isSearchField = SpellCheckHelper.isTargetASearchEngineField(input, window);
+  Assert.equal(isSearchField, false, "Missing action means no search field");
+
+  form.action = "/";
+
+  form.method = "dialog";
+  isSearchField = SpellCheckHelper.isTargetASearchEngineField(input, window);
+  Assert.equal(isSearchField, false, "Method=dialog means no search field");
+
+  form.method = "POST";
+
+  delete input.removeAttribute("name");
+  isSearchField = SpellCheckHelper.isTargetASearchEngineField(input, window);
+  Assert.equal(isSearchField, false, "Missing name means no search field");
+
+  input.name = "q";
+
+  input.type = "url";
+  isSearchField = SpellCheckHelper.isTargetASearchEngineField(input, window);
+  Assert.equal(isSearchField, false, "Url input means no search field");
+
+  input.type = "text";
+
+  let fileInput = document.createElement("input");
+  fileInput.type = "file";
+  fileInput.name = "file-input";
+  form.appendChild(fileInput);
+  isSearchField = SpellCheckHelper.isTargetASearchEngineField(input, window);
+  Assert.equal(isSearchField, false, "File input means no search field");
+
+  fileInput.remove();
+  isSearchField = SpellCheckHelper.isTargetASearchEngineField(input, window);
+  Assert.equal(isSearchField, true, "Is search field again");
+});

toolkit/components/search/tests/SearchTestUtils.sys.mjs

@@ -761,6 +761,26 @@ class _SearchTestUtils {
       reader.readAsDataURL(blob);
     });
   }
+
+  /**
+   * Extracts post data string from an nsISearchSubmission.
+   * If there is no post data, returns null.
+   *
+   * @param {?nsISearchSubmission} submission
+   * @returns {?string}
+   */
+  getPostDataString(submission) {
+    if (!submission.postData) {
+      return null;
+    }
+
+    let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].createInstance(
+      Ci.nsIBinaryInputStream
+    );
+    binaryStream.setInputStream(submission.postData.data);
+
+    return binaryStream.readBytes(binaryStream.available());
+  }
 }
 
 export const SearchTestUtils = new _SearchTestUtils();

toolkit/components/search/tests/xpcshell/test_getSubmission_encoding.js

@@ -110,23 +110,9 @@ add_task(async function test_encoding_of_spaces() {
   let submission = engine.getSubmission("f o o");
   Assert.equal(submission.uri.spec, "https://example.com/user");
   Assert.equal(
-    postDataToString(submission.postData),
+    SearchTestUtils.getPostDataString(submission),
     "q=f+o+o",
     "Encodes spaces in body as +."
   );
   await Services.search.removeEngine(engine);
 });
-
-function postDataToString(postData) {
-  if (!postData) {
-    return undefined;
-  }
-  let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].createInstance(
-    Ci.nsIBinaryInputStream
-  );
-  binaryStream.setInputStream(postData.data);
-
-  return binaryStream
-    .readBytes(binaryStream.available())
-    .replace("searchTerms", "%s");
-}

toolkit/modules/InlineSpellChecker.sys.mjs

@@ -473,21 +473,39 @@ export var SpellCheckHelper = {
     );
   },
 
+  /**
+   * Returns whether the element is counted as a search engine field.
+   *
+   * @param {HTMLInputElement} aNode
+   *   The input to check.
+   * @param {Window} window
+   *   The element's window.
+   * @returns {boolean}
+   *   Whether it should count as a search engine field.
+   */
   isTargetASearchEngineField(aNode, window) {
-    if (!window.HTMLInputElement.isInstance(aNode)) {
+    if (
+      !window.HTMLInputElement.isInstance(aNode) ||
+      (aNode.type != "text" && aNode.type != "search") ||
+      aNode.readOnly ||
+      !aNode.name ||
+      !aNode.form
+    ) {
       return false;
     }
 
     let form = aNode.form;
-    if (!form || aNode.type == "password" || !aNode.name) {
-      return false;
-    }
     let method = form.method.toUpperCase();
 
     return (
+      // Forms without an explicit action often don't work, see Bug 1960237.
+      form.hasAttribute("action") &&
+      // The only other method is dialog.
       (method == "GET" || method == "POST") &&
+      // SearchEngine objects currently only support urlencoded requests.
       form.enctype == "application/x-www-form-urlencoded" &&
-      new FormData(form).entries().every(([k, v]) => k && typeof v == "string")
+      // Don't allow forms with file inputs.
+      new FormData(form).values().every(v => typeof v == "string")
     );
   },