326532 - Ampersand in bookmark keyworded search string doesn't get URL encoded properly

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[James Whitwell]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051229 Camino/1.0b2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051229 Camino/1.0b2

I have a bookmark with a keyword set up for Google search (the bookmark is http://www.google.com.au/search?hl=en&q=%s&btnG=Google+Search&meta= and the keyword is 'goo').  When I type the following into the address bar

goo at&t "hobbit" cpu

it incorrectly (I think) ends up as

http://www.google.com.au/search?hl=en&q=at&t%20%22hobbit%22%20cpu&btnG=Google+Search&meta=

with the '&' between the 'at' and 't' as the value of the 'q' parameter, which also causes the other parts of the 'q' value to be incorrectly interpreted by Google.

Reproducible: Always

Steps to Reproduce:
1. set up a bookmark with the location 'http://www.google.com.au/search?hl=en&q=%s&btnG=Google+Search&meta='
2. set the bookmark keyword to 'goo'
3. type 'goo at&t "hobbit" cpu' into the address bar

Actual Results:  
I got a search for the letters 'AT'

Expected Results:  
I should've got search results for 'AT&T "Hobbit" CPU'

Comment 1

[Chris Lawson (gone)]

I'm not totally sure we want to be sanitising the search string used with keywords. This works perfectly in the search field. If it's a really simple fix, I suppose it wouldn't hurt, but this isn't exactly a common problem.

cl

Comment 2

[Wevah (Nate Weaver)]

Simple workaround: Use a JavaScript bookmarklet with encodeURIComponent().

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

What does Firefox 1.5 do?

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is basically bug 181243 all over again, except the only character that's not being encoded correctly now is the ampersand; other characters (like the searches in bug 181243, or the question mark) are properly encoded.

Firefox encodes the ampersand, so we should be doing that, too.

Comment 5

[froodian (Ian Leue)]

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

Comment 6

[Chris Lawson (gone)]

Attachment: first shot at a patch, encodes & as %26

Are there any other characters besides & we should worry about? I couldn't think of any, but if there are, we might want to consider an alternative approach (although simply making a string of them is fairly easy), like stringByAddingPercentEscapesUsingEncoding: or something.

I think the set of characters that are valid URL characters AND need to be encoded in most search strings is pretty small, though.

cl

Comment 7

[Stuart Morgan]

Comment on attachment 221252 [details] [diff] [review]
first shot at a patch, encodes & as %26

Why would you want to roll a custom partial solution when stringByAddingPercentEscapesUsingEncoding: should do exactly what needs to be done here?

Comment 8

[Chris Lawson (gone)]

(In reply to comment #7)
> (From update of attachment 221252 [details] [diff] [review] [edit])
> Why would you want to roll a custom partial solution when
> stringByAddingPercentEscapesUsingEncoding: should do exactly what needs to be
> done here?
> 

Because it doesn't, that's why. The ampersand is a valid URL character, and none of the encodings supported by that method seem to encode the ampersand. (Trust me, I tried that first.)

cl

Comment 9

[Wevah (Nate Weaver)]

You should probably use the CoreFoundation function CFURLCreateStringByAddingPercentEscapes(), then:

return (NSString *)CFURLCreateStringByAddingPercentEscapes(kCFAllocatorDefault, (CFStringRef)resultString, NULL, NULL, kCFStringEncodingUTF8);

Comment 10

[Wevah (Nate Weaver)]

Apparently Apple's docs are (gasp!) wrong. The fourth argument should be CFSTR(";/?:@&=+$,") or some such.

Comment 11

[Chris Lawson (gone)]

Attachment: uses CFURLCreateString... as suggested

This takes care of the filed bug and heads off nine more possible unfiled bugs. (I don't think any of them are a problem at Google, but all could be with other search engines.)

cl

Comment 12

[Stuart Morgan]

Comment on attachment 221291 [details] [diff] [review]
uses CFURLCreateString... as suggested

Sorry, I didn't look carefully enough at stringByAddingPercentEscapesUsingEncoding:

On the other hand, you didn't test this patch, so we're even ;)  You need to escape |keywords|, not the entire URL; this completely breaks keywords as is (looks good if you move the escaping to keyword though).

Comment 13

[Chris Lawson (gone)]

Attachment: tested ;) and working patch

There we go. I was in a hurry to get to work this morning when I posted the other one and hadn't had a chance to test it. It was very thoroughly broken, though.

This one works and only percent-escapes the search string.

cl

Comment 14

[Stuart Morgan]

Comment on attachment 221387 [details] [diff] [review]
tested ;) and working patch

The escaping should be done inside the if; no point in doing work that won't be needed.

And since you are respinning anyway and like cleanup... expandKeyword:inString: is a terrible name for this function. The "keyword" argument is *not* the bookmark keyword, the keyword isn't the part being expanded, and the string isn't just any old string (which is kind of important to the whole escaping process). It would be great if you could rename this to something sensible like expandURL:withString: (swapping the argument order, obviously). You'd only have to touch two other files, and it would make the world a better place.

Comment 15

[Chris Lawson (gone)]

(In reply to comment #14)

> You'd only have
> to touch two other files, and it would make the world a better place.

What two? I can only find expandKeyword: in BookmarkFolder.mm.

cl

Comment 16

[Stuart Morgan]

The imaginary two--I somehow got the idea that the call was from outside BMF, so there and BMF.h.  That makes it even easier to make the world a better place!

Comment 17

[Chris Lawson (gone)]

Attachment: Fixed, fixed, and fixed

All previous comments fixed. Oh, and turns out expandKeyword wasn't declared in the header, so this was the only file I had to touch. Cool.

Comment 18

[Stuart Morgan]

Comment on attachment 221540 [details] [diff] [review]
Fixed, fixed, and fixed

r=me.

Whoever checks this in should do something about the insane spacing on the arguments to CFURLCreateStringByAddingPercentEscapes (I meant to mention it earlier, but it slipped my mind).  Lining arguments up is nice and all, but not when it makes the whole file 40 or 50 characters wider.

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 221540 [details] [diff] [review]
Fixed, fixed, and fixed

sr=pink

Comment 20

[Mike Pinkerton (not reading bugmail)]

fixed trunk and branch