267487 - addSearchEngine should install search plugins from https sites as well as http

Status: RESOLVED FIXED

(SeaMonkey :: Search, enhancement)

Description

[helenbroadie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041103

The addSearchEngine function from /components/nsSidebar.js appears to only allow
users to install src & image files for search plugins from URLs which start
"http://".  I think it would be helpful if they could also install from
"https://".  For example, for websites which would like users to authenticate
themselves before downloading search plugins.

Reproducible: Always
Steps to Reproduce:
1. Create a test page which offers a search plugin to install which is accessed
by an "https" URL
2. Try and install the search plugin
Actual Results:  
Mozilla gave me the following JavaScript errors: 

Error: [Exception... "Not enough arguments [nsIPromptService.alert]"  nsresult:
"0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame ::
file:///<MOZILLA INSTALL DIR>/components/nsSidebar.js :: anonymous :: line 283"
 data: no]
Source File: file:///<MOZILLA INSTALL DIR>/components/nsSidebar.js
Line: 283

Error: uncaught exception: [Exception... "Not enough arguments
[nsIPromptService.alert]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"
 location: "JS frame :: file:///<MOZILLA INSTALL DIR>/components/nsSidebar.js ::
anonymous :: line 283"  data: no]

(with <MOZILLA INSTALL DIR> substituted for appropriate value)

No other error was displayed to the user but the plugin was not installed

Expected Results:  
The plugin should have been installed.

At the very least though, there should have been some more useful user error.

I edited my NsSidebar.js to give the right arguments for the
nsIPromptService.alert and then it gave me the following JS error: 

Error: uncaught exception: [Exception... "'Illegal value' when calling method:
[nsISidebar::addSearchEngine]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"
 location: "JS frame :: https://<LOCATION OF SEARCH PLUGIN INSTALL PAGE> ::
addEngine :: line 24"  data: no]

(again, with <LOCATION OF SEARCH PLUGIN INSTALL PAGE> substituted)

Comment 1

[Cornelius]

I experienced the same silent failing in linux as well. It would be very useful
to be able to install search plugins over https.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Comment 2

[Robert Strong (they/them - no direct email)]

*** Bug 291652 has been marked as a duplicate of this bug. ***

Comment 3

[Mark Poticha]

Hello,

i'v the problem also and hier is my patch proposal. It works fine for me.

------------------8<----------------
    try
    {
        // make sure using HTTP or HTTPS (for both engine as well as icon URLs)

        if (engineURL.search(/^http[s]+:\/\//i) == -1)
        {
            debug ("must use HTTP or HTTPS to fetch search engine file");
            throw Components.results.NS_ERROR_INVALID_ARG;
        }

        if (iconURL.search(/^http[s]+:\/\//i) == -1)
        {
            debug ("must use HTTP or HTTPS to fetch search icon file");
            throw Components.results.NS_ERROR_INVALID_ARG;
        }
------------------8<----------------

Comment 4

[Mark Poticha]

Attachment: add the HTTPS handling possibility of addSearchEngine

Comment 5

[Pedro Morais]

HTTPS still doesn't work in Deer Park Beta 1. Patch look ok.

Comment 6

[O. Atsushi (Torisugari)]

Attachment: Patch for both firefox and seamonkey

/^http[s]+:/ matches httpsssss://www.example.com/, so that /^https?:/ would be
a better expression.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 197819 [details] [diff] [review]
Patch for both firefox and seamonkey

I'm not going to complain about the way we're messing with URIs in this code.
:-/

  if (engineURL.search(foo) == -1)

while you're here, please change the style to:

  if (!foo.test(engineURL.search))

r=mano otherwise.

Comment 8

[O. Atsushi (Torisugari)]

Attachment: Patch for both firefox and seamonkey

(In reply to comment #7)
Comments addressed.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 198283 [details] [diff] [review]
Patch for both firefox and seamonkey

Why did you bracket the regexp(s)?

Comment 10

[O. Atsushi (Torisugari)]

(In reply to comment #9)
> (From update of attachment 198283 [details] [diff] [review])
> Why did you bracket the regexp(s)?

Just an emphasis of the js objects, as they includes comma and so on.
If there were a variable "i" near the block, /.../i.test(...)
would also be very confusing. I like 2+(2*2) rather than 2+2*2,
even if it's obvious to most of those who read it. I think
if(str.match(exp)){...} is easier to read later, but you like
if(exp.test(str)){...}, then I added brackets.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 198283 [details] [diff] [review]
Patch for both firefox and seamonkey

OK then.

>+            throw "Invalid search engine URL";
...
>+            throw "Invalid search icon URL";

s/Invalid/Unsupported.

r=mano

Comment 12

[O. Atsushi (Torisugari)]

Attachment: Patch for both firefox and seamonkey

per comment 11.
Thank you.

Comment 13

[[e-mail]]

Comment on attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

I don't suppose you could fix the alert to while you're at it?

Comment 14

[O. Atsushi (Torisugari)]

(In reply to comment #13)
> (From update of attachment 198986 [details] [diff] [review] [edit])
> I don't suppose you could fix the alert to while you're at it?

Uhm, you mean localizing, right? It sounds a good idea.

On the other hand, I don't understand why we need such a lot of
debug messages, which are hidden to normal users.

Comment 15

[O. Atsushi (Torisugari)]

I filed bug 312560 to address alert.

Comment 16

[Asaf Romano (gone)]

Checking in mozilla/browser/components/sidebar/src/nsSidebar.js;
/cvsroot/mozilla/browser/components/sidebar/src/nsSidebar.js,v  <--  nsSidebar.js
new revision: 1.11; previous revision: 1.10
done
Checking in mozilla/xpfe/components/sidebar/src/nsSidebar.js;
/cvsroot/mozilla/xpfe/components/sidebar/src/nsSidebar.js,v  <--  nsSidebar.js
new revision: 1.38; previous revision: 1.37
done

Comment 17

[Ian Neal]

Comment on attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

a=me for SM1.0b, one more needed

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

a=me for sm1.0b for the mozilla/xpfe/components/sidebar/src/nsSidebar.js file

Comment 19

[[e-mail]]

SeaMonkey-only portion of patch checked in to the 1.8 branch.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

This didn't make the branches, so triggered bug 335614 when addons.m.o started serving plugins using https. This has been baking on the trunk for a long time. It's definitely needed for 2.0, though probably not for 1.5.0.x since it's been broken in several released milestones from that branch anyways.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> This didn't make the branches, so triggered bug 335614 when addons.m.o started
> serving plugins using https.

Sorry, I meant "triggered bug 335602".

Comment 22

[Mike Connor [:mconnor]]

This should be very low risk and fixes a usability issue on the 1.5.0.x branch, so we should really take it

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Checked in on the 1.8 branch for Firefox 2.
mozilla/browser/components/sidebar/src/nsSidebar.js 	1.10.8.2

Comment 24

[:Gavin Sharp [email: [e-mail]]]

*** Bug 336456 has been marked as a duplicate of this bug. ***

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

approved for 1.8.0 branch -- land ASAP please!

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Landed on the 1.8.0 branch for Firefox 1.5.0.4.
mozilla/browser/components/sidebar/src/nsSidebar.js 	1.10.14.1