The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8final

Status

SeaMonkey
Search
--
enhancement
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: helenbroadie, Assigned: O. Atsushi (Torisugari))

Tracking

({fixed-seamonkey1.0, fixed1.8.0.4, fixed1.8.1})

1.8 Branch
mozilla1.8final
fixed-seamonkey1.0, fixed1.8.0.4, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
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
*** Bug 291652 has been marked as a duplicate of this bug. ***

Comment 3

12 years ago
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

12 years ago
Created attachment 193108 [details] [diff] [review]
add the HTTPS handling possibility of addSearchEngine

Comment 5

12 years ago
HTTPS still doesn't work in Deer Park Beta 1. Patch look ok.
(Assignee)

Comment 6

12 years ago
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.
(Assignee)

Updated

12 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Updated

12 years ago
Attachment #197819 - Flags: review?(bugs.mano)
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.
Attachment #197819 - Flags: review?(bugs.mano) → review+
(Assignee)

Comment 8

12 years ago
Created attachment 198283 [details] [diff] [review]
Patch for both firefox and seamonkey

(In reply to comment #7)
Comments addressed.
(Assignee)

Updated

12 years ago
Attachment #197819 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #198283 - Flags: review?(bugs.mano)
Comment on attachment 198283 [details] [diff] [review]
Patch for both firefox and seamonkey

Why did you bracket the regexp(s)?
(Assignee)

Comment 10

12 years ago
(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 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
Attachment #198283 - Flags: review?(bugs.mano) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

per comment 11.
Thank you.
Assignee: search → torisugari
Attachment #198283 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #198986 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 13

12 years ago
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?
Attachment #198986 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 14

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #193108 - Attachment is obsolete: true
(Assignee)

Comment 15

12 years ago
I filed bug 312560 to address alert.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha

Comment 17

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

a=me for SM1.0b, one more needed
a=me for sm1.0b for the mozilla/xpfe/components/sidebar/src/nsSidebar.js file

Comment 19

11 years ago
SeaMonkey-only portion of patch checked in to the 1.8 branch.
Whiteboard: fixed-seamonkey1.0
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
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.
Attachment #198986 - Flags: approval1.8.0.4?
Attachment #198986 - Flags: approval-branch-1.8.1?(mconnor)
(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".

Updated

11 years ago
Attachment #198986 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
This should be very low risk and fixes a usability issue on the 1.5.0.x branch, so we should really take it
Checked in on the 1.8 branch for Firefox 2.
mozilla/browser/components/sidebar/src/nsSidebar.js 	1.10.8.2
Keywords: fixed1.8.1
*** Bug 336456 has been marked as a duplicate of this bug. ***
Comment on attachment 198986 [details] [diff] [review]
Patch for both firefox and seamonkey

approved for 1.8.0 branch -- land ASAP please!
Attachment #198986 - Flags: approval1.8.0.4? → approval1.8.0.4+
Landed on the 1.8.0 branch for Firefox 1.5.0.4.
mozilla/browser/components/sidebar/src/nsSidebar.js 	1.10.14.1
Keywords: fixed1.8.0.4
Target Milestone: mozilla1.9alpha → mozilla1.8final
Version: Trunk → 1.8 Branch
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.