565253 - Opening a non-punycoded Unicode/IDN URL via GetURLCommand handling can fail to load the site

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: OS Integration, defect)

Description

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

This is the Camino version of bug 468005, which I've failed to file for two years now :P

Opening a non-punycoded Unicode URL (e.g. http://www.☃.net/ or http://✪df.ws/fab ) via AppleEvents (external app, open -a) fails.

We get a %-escaped URL from the AppleEvent, which we pass to Gecko, and Gecko brings up an error page for the %-escaped URL (we then unescape the URL correctly in the location bar, though, which makes our behavior appear all the more confusing to user).

STR:
1. Open Terminal.app.
2. $ open -a Camino http://☃.net/

AR: Error page.

ER: http://☃.net/ (http://xn--n3h.net/) loads.

However, /Applications/Camino.app/Contents/MacOS/Camino -url http://☃.net/ *is* opened successfully; I'm not sure where the command-line NSUserDefaults code diverges from the GetURLCommand code.

When I NSLog urlString for open -a Camino http://✪df.ws/fab , I get

urlString = http://%E2%9C%AAdf.ws/fab

(I don't see anything at all logged when I use the NSUserDefaults key, so it must enter the picture somewhere else.)

We then call [mainController showURL:urlString]; and leave GetURLCommand

and I'm out of debugging time for this tonight.

No, I think I've maybe figured this out (thanks murph!):

murph in bug 364497 comment 5: 

> There are two different locations in our code where external URLs are opened. 
> URLs dropped onto the dock icon come in by way of Apple Events and are handled
> by GetURLCommand.mm.  Clicking on an external link with Camino as the default
> browser, or informing Finder to open a saved URL with Camino, is handled by the
> NSApplication delegate method application:openFile: in MainController.mm.
 
http://mxr.mozilla.org/camino/source/src/application/MainController.mm#930

We decode the NSURL in openFile:, and, for instance, dragging http://✪df.ws/fab from the Terminal to the Dock icon gets us a URL we open correctly (and no NSLogging, so I'm fairly we're hitting the application:openFile: path and not the GetURLCommand path).  

Coincidentally, we also decode for file URLs we get via GetURLCommand; I think we just want to decode *everything* that comes in via GetURLCommand.  (If it's safe enough for everything that goes through application:openFile:, it's safe enough for GetURL, right?)

Although for some reason that doesn't seem to actually work :(

Comment 1

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

Attachment: Fix

OK, with a lot of debugging (and some help from Stuart), I figured out what's going on (and what code handles what URL entry point):

1) openURL: in MainController handles URL-only drags to the Dock icon (in addition to the Open URL service); the pasteboard gives us unescaped Unicode URLs.

2) MainController's init checks NSUserDefaults for the -url key; whatever provides that to us also gives us unescaped Unicode URLs.

3) Only the poor ol' GURL AppleEvent sends us escaped URLs for Unicode/IDNs (which Gecko is unable to handle; probably ultimately a Gecko bug, but not what I signed up for!).  However, just to make it more fun, only (apparently) the "open" command in the Terminal and links from external apps come as %escaped URLs; the "open location" GURL AppleScript command sends us unescaped URLs (at least on 10.5) just like user-defaults and the pasteboard :P

Anyway, given all that, the fix here is to unescape non-file URLs coming in via GetURLCommand and be done with it.

Comment 2

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

Stuart, since the problem here is only (I think) the %-escaped host, maybe we should only unescape the host ([NSURL host] actually does that for us on 10.5, it seems) and then put the URL back together with the unescaped host?

It's more work (and I don't know how to do all of the gymnastics required for splitting and reassembling in Cocoa), but it seems a whole lot safer; it should prevent us from touching the rest of the URL and hitting a dangerous case or unanticipated dangerous case, given that we can get both escaped and unescaped URLs via GURL events.

Comment 3

[Stuart Morgan]

Reconstructing URLs with NSURL is a PITA. So [NSURL host] doesn't actually give you the same string that the URL contains? That's annoying, since it means we can't easily approximate your suggestion by unescaping the whole URL if the host appears to need it.

Maybe we should use raw string fiddling instead of using NSURL to replace the host. We'll have to be careful about passwords and ports, but still probably easier to get right.

Comment 4

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

Comment on attachment 444997 [details] [diff] [review]
Fix

Cancelling the review request here since I think we've agreed the proposed alternative is better.

Comment 5

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

Also setting this back to NEW; I'll probably keep poking at it, but someone better-versed in NSString gymnastics should feel free to steal it from me.

Comment 6

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

Bug 309226 has lots of discussion of IDN-decoding options available; whatever we use there, we should use here, too.