Last Comment Bug 267487 - addSearchEngine should install search plugins from https sites as well as http
: addSearchEngine should install search plugins from https sites as well as http
Status: RESOLVED FIXED
: fixed-seamonkey1.0, fixed1.8.0.4, fixed1.8.1
Product: SeaMonkey
Classification: Client Software
Component: Search (show other bugs)
: 1.8 Branch
: All All
: -- enhancement with 1 vote (vote)
: mozilla1.8final
Assigned To: O. Atsushi (Torisugari)
:
Mentors:
: 291652 336456 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-11-03 09:09 PST by helenbroadie
Modified: 2008-07-31 01:19 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add the HTTPS handling possibility of addSearchEngine (723 bytes, patch)
2005-08-18 16:38 PDT, Mark Poticha
no flags Details | Diff | Splinter Review
Patch for both firefox and seamonkey (3.52 KB, patch)
2005-09-29 01:17 PDT, O. Atsushi (Torisugari)
asaf: review+
Details | Diff | Splinter Review
Patch for both firefox and seamonkey (5.05 KB, patch)
2005-10-03 01:53 PDT, O. Atsushi (Torisugari)
asaf: review+
Details | Diff | Splinter Review
Patch for both firefox and seamonkey (5.06 KB, patch)
2005-10-09 02:19 PDT, O. Atsushi (Torisugari)
neil: superreview+
mconnor: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description helenbroadie 2004-11-03 09:09:58 PST
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 2004-11-23 09:16:46 PST
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 [:rstrong] (use needinfo to contact me) 2005-04-23 19:42:17 PDT
*** Bug 291652 has been marked as a duplicate of this bug. ***
Comment 3 Mark Poticha 2005-08-18 16:30:08 PDT
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 2005-08-18 16:38:38 PDT
Created attachment 193108 [details] [diff] [review]
add the HTTPS handling possibility of addSearchEngine
Comment 5 Pedro Morais 2005-09-16 08:30:41 PDT
HTTPS still doesn't work in Deer Park Beta 1. Patch look ok.
Comment 6 O. Atsushi (Torisugari) 2005-09-29 01:17:40 PDT
Created attachment 197819 [details] [diff] [review]
Patch for both firefox and seamonkey

/^http[s]+:/ matches httpsssss://www.example.com/, so that /^https?:/ would be
a better expression.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-10-02 15:06:06 PDT
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) 2005-10-03 01:53:11 PDT
Created attachment 198283 [details] [diff] [review]
Patch for both firefox and seamonkey

(In reply to comment #7)
Comments addressed.
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-10-03 02:17:27 PDT
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) 2005-10-03 03:13:18 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-10-05 09:54:03 PDT
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) 2005-10-09 02:19:37 PDT
Created attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

per comment 11.
Thank you.
Comment 13 neil@parkwaycc.co.uk 2005-10-14 09:39:56 PDT
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) 2005-10-14 10:34:43 PDT
(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) 2005-10-15 08:35:54 PDT
I filed bug 312560 to address alert.
Comment 16 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-10-17 04:32:43 PDT
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 2005-12-14 08:50:39 PST
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) 2005-12-14 13:54:02 PST
a=me for sm1.0b for the mozilla/xpfe/components/sidebar/src/nsSidebar.js file
Comment 19 neil@parkwaycc.co.uk 2005-12-14 16:52:10 PST
SeaMonkey-only portion of patch checked in to the 1.8 branch.
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-03 10:18:49 PDT
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: gavin@gavinsharp.com] 2006-05-03 10:20:17 PDT
(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] 2006-05-03 10:22:46 PDT
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: gavin@gavinsharp.com] 2006-05-03 12:18:59 PDT
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: gavin@gavinsharp.com] 2006-05-03 12:27:44 PDT
*** Bug 336456 has been marked as a duplicate of this bug. ***
Comment 25 Daniel Veditz [:dveditz] 2006-05-03 13:47:14 PDT
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: gavin@gavinsharp.com] 2006-05-03 13:54:40 PDT
Landed on the 1.8.0 branch for Firefox 1.5.0.4.
mozilla/browser/components/sidebar/src/nsSidebar.js 	1.10.14.1

Note You need to log in before you can comment on or make changes to this bug.