Last Comment Bug 390295 - Searchplugin with digit in url cuts off url
: Searchplugin with digit in url cuts off url
Status: RESOLVED FIXED
: fixed-seamonkey1.1.10
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Linux
: -- minor (vote)
: ---
Assigned To: Bruno 'Aqualon' Escherl
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-31 06:36 PDT by Klaus Fuerstberger
Modified: 2008-03-31 07:09 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Support URLs with digits and IP addresses as searchRoot (1.12 KB, patch)
2008-03-27 08:49 PDT, Bruno 'Aqualon' Escherl
mnyromyr: review+
Details | Diff | Splinter Review
allow schema://host as searchRoot (1.12 KB, patch)
2008-03-28 15:06 PDT, Bruno 'Aqualon' Escherl
neil: review+
neil: superreview+
kairo: approval‑seamonkey1.1.10+
Details | Diff | Splinter Review

Description Klaus Fuerstberger 2007-07-31 06:36:12 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.8.1.5) Gecko/20070716 SeaMonkey/1.1.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.8.1.5) Gecko/20070716 SeaMonkey/1.1.3

I have a metager2.src with the content below.
If yout hit the search button of the empty addressbar to call the searchsite the url 
http://www.metager2.de/search.php is cut off to http://www.metager/

###################################################################
<search
   version="1.0"
   name="MetaGer2"
   description="MetaGer2 Meta-Suchmaschine"
   method="GET"
   action="http://www.metager2.de/search.php"
   update="http://www.metager2.de/mozilla/metager2.src"
   updateCheckDays=1
   queryEncoding="iso-8859-1"
   queryCharset="iso-8859-1"
>

<input name="q" user>

</search>

<browser
    update="http://www.metager2.de/mozilla/metager2.src"
    updateIcon="http://www.metager2.de/mozilla/metager2.gif"
    updateCheckDays="3"
>
###################################################################

Reproducible: Always

Steps to Reproduce:
1. Install the searchplugin http://www.metager2.de/mozilla.php
2. Make the searchplugin metager2 the default searchengine in preferences
3. Hit the browser searchbutton with an empty addressbar

Actual Results:  
The URL ist cut off to http://metager/


The sidebar search works.
Comment 1 Klaus Fuerstberger 2007-07-31 06:41:20 PDT
This patch solves the problem:
xpfe/browser/resources/content/navigator.js

# diff navigator.js navigator.js.DIST
1239c1239
<           searchRoot = searchURL.match(/[a-z]+:\/\/[a-z.-]+/);
---
>           searchRoot = searchURL.match(/[a-z]+:\/\/[a-z0-9.-]+/);
Comment 2 Misak Khachatryan 2008-03-27 00:55:21 PDT
Klaus, please attach your patch to bug and request review for it from respective module owner.
Comment 3 Klaus Fuerstberger 2008-03-27 03:17:46 PDT
Hi Misak,

the bug still exist with SeaMonkey 1.1.8. I don't know what do more than reporting the bug here. I also don't know a module owner, because the code is in a general component. The simple patch is posted above, so what should I attach more?

Bye Klaus
Comment 4 Misak Khachatryan 2008-03-27 04:25:08 PDT
Hi Klaus,

Here is the two useful links for you, if you have time of course:

http://www.mozilla.org/hacking/
http://www.mozilla.org/owners.html

Also you can visit IRC channel #seamonkey at irc.mozilla.org and ask developers what to do with patch. I'm sure, You'll find help there.
Comment 5 Misak Khachatryan 2008-03-27 04:27:36 PDT
Here another link - http://www.seamonkey-project.org/dev/project-areas
Seems Mnyromir is who you need. You can catch him on irc
Comment 6 Klaus Fuerstberger 2008-03-27 05:04:02 PDT
Hi Misak,

I think it should be enough for a "normal" user to file a bug report, as I'm not a developer. As far as I know this is the right place for mozilla bug reports. I also think that the developers search and work on the bug reports, so why should I bother them on IRC? The subject of this bug report is clear and the right developer should be able to find this report.

But you are always welcome to do anything with this bug report and the patch :-)

IMHO we should stop, filling this bugreport with general discussions about the handling of mozilla bug reporting. If you furthermore wish to discuss this, feel free to send me a private mail.

Klaus
Comment 7 Misak Khachatryan 2008-03-27 06:16:18 PDT
Ok Klaus, I get your point. I'll try to poke Mnyromir to look at your patch.
I just doing some cleanup of bugs, that don't have activity long time, and want make Seamonkey better. With your patch, it should be better a bit. Thanks and sorry.
Comment 8 Bruno 'Aqualon' Escherl 2008-03-27 08:49:05 PDT
Created attachment 312045 [details] [diff] [review]
Support URLs with digits and IP addresses as searchRoot

The solution suggested by Klaus in comment#1 matches any alphadigit combinations as part of searchRoot (like metager2.de and also pure IP addresses).

The patch I made is trunk only, don't know if we want it for branch.
Comment 9 Karsten Düsterloh 2008-03-27 15:20:25 PDT
Comment on attachment 312045 [details] [diff] [review]
Support URLs with digits and IP addresses as searchRoot

>         // Get a search URL and guess that the front page of the site has a search form.
>         var searchDS = Components.classes["@mozilla.org/rdf/datasource;1?name=internetsearch"]
>                                  .getService(Components.interfaces.nsIInternetSearchService);
>         var searchURL = searchDS.GetInternetSearchURL(searchEngineURI, "ABC", 0, 0, {value:0});
>         if (searchURL) {
>-          searchRoot = searchURL.match(/[a-z]+:\/\/[a-z.-]+/);
>+          searchRoot = searchURL.match(/[a-z]+:\/\/[a-z0-9.-]+/);
>           if (searchRoot) {
>             openTopWin(searchRoot + "/");
>             return;
>           }

My goodness, this code is just evil guessing! Any IDN or uppercase letters and it's doomed. And it won't work with arbitrary protocols, either.
A slightly better regex, imo, would be this:

/^[a-z-]+:\/\/[a-z0-9.-]+/i

This ties the match to the beginning of the string, allows protocols like our internal mailbox-message:// and ignores the character case. IDN still won't work, but they can be made working by replacing them with their punycode.

r=me with that.
Comment 10 Bruno 'Aqualon' Escherl 2008-03-27 17:37:19 PDT
How about one of those suggestions:

/* matching all between :// and / */
/^[a-z-]+:\/\/[^/]*/

/* matches additional all kind of characters in the protocol part */
/^[^]+:\/\/[^/]*/

I think the first one would be sufficient, but I don't know very much about regex.
Comment 11 Bruno 'Aqualon' Escherl 2008-03-28 15:06:17 PDT
Created attachment 312381 [details] [diff] [review]
allow schema://host as searchRoot

Neil: Mnyromyr suggested the final regex, so asking you for review :)
Comment 12 neil@parkwaycc.co.uk 2008-03-29 10:48:26 PDT
Comment on attachment 312381 [details] [diff] [review]
allow schema://host as searchRoot

bz or biesi would probably prefer this to go via the IO service but I'm sure it's good enough for what we need.
Comment 13 Frank Wein [:mcsmurf] 2008-03-30 09:30:15 PDT
Checking in navigator.js;
/cvsroot/mozilla/suite/browser/navigator.js,v  <--  navigator.js
new revision: 1.619; previous revision: 1.618
done
Comment 14 Bruno 'Aqualon' Escherl 2008-03-30 09:39:02 PDT
Comment on attachment 312381 [details] [diff] [review]
allow schema://host as searchRoot

Do we want this in branch? 

Pro:
Only a small change to allow sites like metager2.de.

Contra:
The only use case is clicking on the search button with no userinput in the urlbar.
Comment 15 Klaus Fuerstberger 2008-03-30 09:53:48 PDT
This are two pro's:

Only a small change *and* it has a use case :-)

Thanks for the changes!

Bye Klaus
Comment 16 Robert Kaiser 2008-03-30 15:16:45 PDT
Comment on attachment 312381 [details] [diff] [review]
allow schema://host as searchRoot

From what I see, this fixes at least one known broken case, has no security risk, doesn't break any currently working cases, and seems to have no substantial risk to cause problems, so everything clear for a branch fix ;-)
a=me for 1.1.10 (1.8 branch)
Comment 17 Robert Kaiser 2008-03-31 07:09:47 PDT
This has been checked in by mcsmurf on trunk (comment #13), and I just checked it into 1.8 branch per Aqaualon's request on IRC.

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