629850 - If Camino is set as the default app for an unsupported protocol (mailto, feed), accessing URIs for those protocols wedges browser

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. In Camino's Preferences, ensure Camino.app is the Default Feed Reader.
2. Go to http://news.google.com
3. In Camino's Location Bar, click the RSS icon and select "Subscribe".

AR:
Camino opens an empty tab with the feed: URL. Trying to close that tab or open another tab will open more empty tabs with the feed: URL.

*** If Camino is configured to "Load the pages the were open before quitting", then Camino will become wedged every time the app is quit and launched! I had to manually delete my ~/Library/Application Support/Camino/WindowState.plist file to rescue Camino.

ER:
View the feed (or at least not wedge the browser).

I can repro this behavior with Camino 2.0.6 and 2.1a1.

Comment 1

[Chris Peterson [:cpeterson]]

Is this problem related to bug 435517?

Comment 2

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

How in the world are you selecting Camino as the default feed reader?

Is something broken on 10.6 that allows selecting any app, rather than just those that register for "feed"?  (Coincidentally, gand came in the channel today to report a similar bug; somehow Camino had been configured as his default mail client.)

Ordinarily I'd tend to write this off, but this is too much.  Stuart, is there any way we can detect this loop for unsupported protocols and break it?

Comment 3

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

(In reply to comment #1)
> Is this problem related to bug 435517?

Er, I mid-aired with you, but no; those are separate apps (GoogleFeedHandler.app, MyYahooFeedHandler.app), not Camino.app.

Comment 4

[Chris Peterson [:cpeterson]]

I can apparently select any app as my default feed reader. The Default Feed Reader dropdown menu lists some seemingly correct apps (e.g. Mail.app and Safari.app), but if I choose "Other..." I can select any app. I am using Mac OS X 10.6.

My current feed reader is Photo Booth.app. :) When Camino tries to subscribe to a feed, Photo Booth.app is launched (without any error messages).

Comment 5

[philippe (part-time)]

Yeah, on both my 10.6 machines anything that is an app. can be selected.

Comment 6

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

Hrm, I wonder if we've never attempted to validate anything coming back from "Other…"?  I have a vague memory of us refusing to set things if they didn't support feed, but I just checked 1.6.11 on 10.3.9 and successfully set iPhoto as my default reader there :P  And in 2.1a1 on 10.5.8 as well :P

But even if we do try to validate the apps returned from "Other…", it's not going to stop other apps (e.g. Mail) from setting us as the default app.

So we need to find some way to break the cycle :(

Comment 7

[Stuart Morgan]

We could probably validate schemes against a whitelist in our GURL handler. I can't think of any cases offhand where letting through a scheme we don't understand would be useful.

Comment 8

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

That sounds promising.  We'll want to make sure we don't break POSIX file paths (which I think are the only things that come in that aren't URLs; I'd have to check).

I might be able to take this…

Comment 9

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

Attachment: Fix for GURL handling of unsupported protocols

Chris, you want to do the review here?

This wasn't quite as hard as I feared it might be.  We need to check the for a scheme, and, if one exists, makes sure it's a protocol we support (http, https, ftp, and file).  If the scheme isn't one of those, then bail.  (If there's no scheme, someone's most likely sent us an AppleEvent with the "url" of "www.apple.com" or "/Users/smokey/Desktop/test.html", and we'll continue to handle--or not handle--any such "url"s as we always have.)

There's one caveat to this fix, and that this doesn't handle URLs coming in via the Service (MainController openURL:) or the -url command-line parameter (MainController init).  However, 1) both of those are sort-of edge cases and 2) this patch is all that's necessary to prevent the wedging/infinite loop; we may try to load problematic URLs from those sources once, but when they come back to us, they'll go through this code path and be stopped.

If we think it's important to fix those two other pathways, I'm happy to patch them, too (with some advice on what's safe to call that early in MainController's init or another place to short-circuit that load).

Comment 10

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

Attachment: AppleScript used in testing

This is an AppleScript filled with URLs that I used as part of my testing (supplemented by some webloc, file opening, and "open" testing).

Comment 11

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

David, do you still have that copy of Dreamweaver you used to help us with bug 403346?

I know from the current Gecko/XULapp bug that DW's doing something strange with the OpenURL event that I haven't yet figured out, and while I don't think I've broken it with this patch, I'd feel better if I could get some affirmative tests.  I can spin you a build with this patch if you still have DW to test against.

Comment 12

[Chris Peterson [:cpeterson]]

Smokey, just two questions:

1. Rather than hard-coding the "known good" URL schemes in |performDefaultImplementation|, is there existing code that already has a "safe URL scheme" check? I imagine checking URL schemes is a common problem. I see a list of supported URL schemes in Camino's Info.plist CFBundleURLTypes.

2. You noted that your patch does not the code paths through Services and the -url command-line parameter. Is this because you are trying to be conservative (for Camino 2.0.7)? I tested that your fix does handle those cases (as you said), just less elegantly (since an empty tab titled "Loading..." is opened, but not wedged).

But your fix looks good to me. r=cpeterso.

Comment 13

[Chris Peterson [:cpeterson]]

If David doesn't have a copy of Dreamweaver, I can find one to test "preview in browser" with your fix. Just let me know.

Comment 14

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

(In reply to comment #12)
> 1. Rather than hard-coding the "known good" URL schemes in
> |performDefaultImplementation|, is there existing code that already has a "safe
> URL scheme" check? I imagine checking URL schemes is a common problem. I see a
> list of supported URL schemes in Camino's Info.plist CFBundleURLTypes.

The closest extant things are the is*URI checks in NSString+Utils (http://mxr.mozilla.org/camino/source/camino/src/extensions/NSString+Utils.m#80), but they're really doing different things than what this code needs.

While there's an elegance to reading the values out of the plist (and a slight maintainability win), I'd rather not do that because then it becomes too easy for someone to escape this very protection and shoot themselves in the foot (yes, we will have users editing the plist because they think we "forgot" mailto: or whatever).  Yes, it'd be their own fault, but it's a nasty enough bug and easily preventible by hard-coding, and we don't add protocol support very often ;)

> 2. You noted that your patch does not the code paths through Services and the
> -url command-line parameter. Is this because you are trying to be conservative
> (for Camino 2.0.7)? I tested that your fix does handle those cases (as you
> said), just less elegantly (since an empty tab titled "Loading..." is opened,
> but not wedged).

I did want to do the minimum required for fixing the infinite loop problem because we have a history of breaking something when we (including me, but also people at lot more skilled at this than me!) make changes to the codesites where we handle GURL and odoc events (and also because when Josh rewrote the equivalent XULapp code two years ago, he broke a bunch of stuff in ways that weren't understood, some of which still aren't fixed--so it's clearly "fragile" in not just Camino).

So that and, as I said before, I think both of the other entry points are edge-cases, so it's OK to handle them less elegantly as long as we break the loop.  But that's just my opinion ;)

Comment 15

[Chris Peterson [:cpeterson]]

That sounds good to me.

Comment 16

[Stuart Morgan]

Comment on attachment 508192 [details] [diff] [review]
Fix for GURL handling of unsupported protocols

Stealing, since I don't think pink will mind ;)

>+    if (urlScheme && !([urlScheme isEqualToString:@"http"] ||
>+          [urlScheme isEqualToString:@"https"] ||
>+          [urlScheme isEqualToString:@"ftp"] ||
>+          [urlScheme isEqualToString:@"file"])) {

Please change to

    if (urlScheme && !([urlScheme isEqualToString:@"http"] ||
                       [urlScheme isEqualToString:@"https"] ||
                       [urlScheme isEqualToString:@"ftp"] ||
                       [urlScheme isEqualToString:@"file"]))
    {

(The indentation for readability, the { for Camino style.)

I do think reading the values from the plist would be better, so we don't get out of sync if we add or remove schemes, but it's of minor importance, so it can just be filed as a low-priority follow-up bug. (Yes, it allows people to intentionally do really dumb things, but if people want to hand-edit the bundle they can make Camino much more broken; the case where we mess up later on is more important.)

Comment 17

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

Attachment: Final? patch I intend to land [landed, comment 18]

Per additional discussion on irc, we should use NSString+Utils’s isEqualToStringIgnoringCase to protect against rogue MAILTO: and fEed: URIs ;)

Since there's a fair amount of churn involved in the various changes now, here's the hopefully-final patch I'll land once we get some positive testing with Dreamweaver.

(I love that Bugzilla gives me a choice between marking an old patch obsolete when adding a "for-checkin" version and seeing who gave reviews :P )

I went ahead and filed the plist-reading follow-up as bug 630432, and an general "audit isEqualToString usage for isEqualToStringIgnoringCase" bug as bug 630431.

Comment 18

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

David confirmed via email that no new untoward behavior with Dreamweaver appears as a result of this patch, so landed as http://hg.mozilla.org/camino/rev/a11e90030225

(There's some untoward behavior currently that I was not aware of; I asked David to do a little bit of checking and then to file a bug on that.)

Comment 19

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

So, there's one very unhappy unintended consequence of this: 'open location "about:blank"' no longer works.

In particular, this breaks my own "Clone Tab" toolbar script (because I open about:blank in order to work around bug 395711, which is a pain, and which can't be worked around using GUI Scripting because of bug 448992).  

I don't know how many other toolbar scripts adopted my work-around, but I definitely don't think we want to take this on the branch without some sort of fix (either whitelisting about:blank specifically or about: in general, I guess), and absent those two bugs getting fixed, I don't think we want to break this work-around for 2.1, either.

Lisa, do any of your scripts use any other about: URIs or any other non-http/https/ftp/file: URLs?

Comment 20

[Stuart Morgan]

I'm fine with allowing any about: URL. sr=me for landing that.

Comment 21

[Lisa Thompson]

No, none of my distributed scripts open any about: URIs or non-http/https/ftp/file: URLs. But view-source: URLs might be another one to account for -- I've opened them from time to time in personal-use scripts.

Comment 22

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

(In reply to comment #21)
> non-http/https/ftp/file: URLs. But view-source: URLs might be another one to
> account for -- I've opened them from time to time in personal-use scripts.

I wrote a script to do that once, too.  Hrm.  I tested against it in my script on the idea of it coming in via "external" sources ("no one's going to include a view-source: link in an email message, because it won't work"), but forgot about the pseudo-internal usages like toolbar scripts.

What about "data:"?  Safari supports it via AppleEvent, and it should be safe in this case (it will be opening a new tab), right?  Currently, we allow ill-formed data: URIs, but we block well-formed (properly-encoded) ones (since we allow the !urlScheme through, because that's how we get POSIX and POSIX-like paths and "www.apple.com").

I guess, on further reflection, I'd be in favor of also allowing view-source: and data: in via GURL events (the former because of AppleScript use-case, and the latter for p-Safari and because it should be safe via GURL); Stuart?

Comment 23

[Stuart Morgan]

Yeah, those are fine too I think.

Comment 24

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

http://wiki.caminobrowser.org/Status_Meetings:2011-02-09:Log#16:23

Comment 25

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

Attachment: Better fix, handles the aforementioned protocols and protects against NSURL's failings

I spent a lot of time testing this after I uncovered the "data" discrepancy in comment 22 and reworking the code to handle "Postel's Law" URLs without any further regressions or hole through which rogue URLs could slip and still manage to cause this bug.

In addition to adding about:, view-source:, and data: to the supported protocols, I reworked the scheme checking to avoid NSURL's failures.  I feel bad about writing an smichaud-length comment in the middle, but the problem is complex enough that I'd like it to be clear to prevent future changes from accidentally regressing things, and the logic was tricky enough that it helped keep me straight writing stuff.

So, although it's a fairly small bit of code changing, there's enough churn here that this at least needs another go at sr.

A couple of questions:

1) Is there any way to split-and-indent
> ([[[urlString componentsSeparatedByString:@":"] objectAtIndex:0] rangeOfString:@"/"].location == NSNotFound))
?

2) Is there a way to avoid calling |[[urlString componentsSeparatedByString:@":"] objectAtIndex:0]| twice?  I can't call it and assign it to a variable outside the |if|, because outside the |if|, that will return "www.apple.com" for "www.apple.com" (similarly for a POSIX path), which results in that entire string getting set as |urlScheme| and then failing the "is this a supported protocol check", thereby regressing support for those two constructs.

Comment 26

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

Attachment: Expanded applescript used in further testing

The AppleScript contains about three times as many test strings now.

Comment 27

[Stuart Morgan]

Comment on attachment 511918 [details] [diff] [review]
Better fix, handles the aforementioned protocols and protects against NSURL's failings

>+        ([[[urlString componentsSeparatedByString:@":"] objectAtIndex:0] rangeOfString:@"/"].location == NSNotFound))

Cut this at one of the spaces, in the method chain, and indent four spaces past the (

To answer your other question, you could just store the array returned by componentsSeparatedByString:, since that's the expensive part, and call objectAtIndex:0. Alternately, you should store it as possibleScheme, and go from there.

>+      NSString* urlScheme = [[urlString componentsSeparatedByString:@":"] objectAtIndex:0];
>+      // Setting Camino as the default app for protocols we do not handle creates
>+      // an infinite loop when those URIs are opened; avoid that by only handling
>+      // supported protocols.
>+      if (urlScheme && !([urlScheme isEqualToStringIgnoringCase:@"http"] ||
>+                         [urlScheme isEqualToStringIgnoringCase:@"https"] ||
>+                         [urlScheme isEqualToStringIgnoringCase:@"ftp"] ||
>+                         [urlScheme isEqualToStringIgnoringCase:@"file"] ||
>+                         [urlScheme isEqualToStringIgnoringCase:@"data"] ||
>+                         [urlScheme isEqualToStringIgnoringCase:@"about"] ||
>+                         [urlScheme isEqualToStringIgnoringCase:@"view-source"]))

Let's add a lowercaseString to the end of the definition of urlSheme, so these can all just be isEqualToString:. No need to check urlScheme in the |if| any more either, in the new code.


Does NSURL not give us a scheme in the bad cases, or do we not get an NSURL at all? If the latter, I'd like to try using NSURL first, and fall back to this code only in cases where it ends up being nil.

Comment 28

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

(In reply to comment #27)
> Does NSURL not give us a scheme in the bad cases, or do we not get an NSURL at
> all? If the latter, I'd like to try using NSURL first, and fall back to this
> code only in cases where it ends up being nil.

We end up with a mix of the two. I think we can probably still handle the NSURL-but-no-scheme cases with NSURL and an |if (scheme)| check, and then fall back to the manual parsing just for the non-NSURL cases, though.  I'll play with it once I have the requisite block of time.

Comment 29

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

(We don't want to ship the regression to AppleScripts in b1.)

Comment 30

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

Attachment: Even better fix (v3): use NSURL if at all possible

I finally had time to run through some comprehensive testing on this new version.  This patch should (I hope) address the review comments and makes us try to use NSURL first and only fall back to manual parsing if necessary.

If NSURL gets us a scheme, we use that as the scheme; if it doesn't (either because the NSURL itself is null, e.g. from website-like strings, Postel's Law URLs, or because the scheme is null, e.g. POSIX paths), we try again with the manual parsing, and if we get a scheme there, we use that as our scheme.

If we have a non-null scheme, we check it against supported protocols; if we have no scheme at this point, we either have a website-like string or a raw POSIX path, so we need to allow them through.

Comment 31

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

(In reply to comment #30)
> if we
> have no scheme at this point

(if the scheme is null or nil, not strictly "no scheme")

Comment 32

[Stuart Morgan]

Comment on attachment 518805 [details] [diff] [review]
Even better fix (v3): use NSURL if at all possible

The approach looks good, just some structural suggestions:

>+    NSString* potentialScheme = [[NSURL URLWithString:urlString] scheme];
...
>+    NSURL* url = [NSURL URLWithString:urlString];

Move this last line to just above the potentialScheme line, and use |url| there, rather than making the NSURL twice.

>+    NSString* potentialScheme = [[NSURL URLWithString:urlString] scheme];
>+    NSString* urlScheme = nil;
>+    if (potentialScheme)
>+      urlScheme = [potentialScheme lowercaseString];

potentialScheme isn't necessary here; just do:

NSString* urlScheme = [[[NSURL URLWithString:urlString] scheme] lowercaseString];
and then change the else to |if (!urlScheme && ...)|

>+    else if ([urlString rangeOfString:@":"].location != NSNotFound) {
>+      potentialScheme = [[urlString componentsSeparatedByString:@":"] objectAtIndex:0];
>+      if ([potentialScheme rangeOfString:@"/"].location == NSNotFound)
>+        urlScheme = [potentialScheme lowercaseString];
>+    }

This part would be more efficient as:
  if (!urlScheme) {
    unsigned int colonLocation = [urlString rangeOfString:@":"].location;
    if (colonLocation != NSNotFound &&
        [urlString rangeOfString:@"/" options:0 range:NSMakeRange(0, colonLocation)])
    {
      urlScheme = [[urlString substringToIndex:colonLocation] lowercaseString];
    }
  }

(Doing componentsSeparatedByString will needless parse the whole string, and create an array of string objects you don't actually want.)

And to remove duplication, you can factor out the lowercaseString so that urlString isn't set with it in each line, and instead end with
  urlString = [urlString lowercaseString];

Comment 33

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

(In reply to comment #32)
>     unsigned int colonLocation = [urlString rangeOfString:@":"].location;
>     if (colonLocation != NSNotFound &&
>         [urlString rangeOfString:@"/" options:0 range:NSMakeRange(0,
> colonLocation)])

This doesn't build, and the error is so opaque it's not even funny :P

/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/appleevents/GetURLCommand.mm: In function ‘objc_object* -[GetURLCommand performDefaultImplementation](GetURLCommand*, objc_selector*)’:
/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/appleevents/GetURLCommand.mm:83: error: no match for ‘operator&&’ in ‘(colonLocation != 2147483647u) && #‘obj_type_ref’ not supported by dump_expr#<expression error>(((objc_object*)urlString), _OBJC_SELECTOR_REFERENCES_8, ((NSString*)((objc_object*)(&{(& __CFConstantStringClassReference), 1992, "/", 1l}))), 0u, NSMakeRange(0u, colonLocation))’
/Users/smokey/Camino/dev/192branch/Camino192Branch/camino/src/appleevents/GetURLCommand.mm:83: note: candidates are: operator&&(bool, bool) <built-in>
** BUILD FAILED **

Also, the docs on rangeOfString are lacking a bit in detail for my taste, too, so I'm not exactly sure what your suggestion is supposed to do, but don't we want a "!" or "!=" or "== NSNotFound" or something associated with [urlString rangeOfString:@"/"]?  That is, if there's a colon, the string-before-colon can't have a / in it for us to use string-before-colon as the scheme, because / is not legal in schemes (and, in effect, we're not in a scheme at all but some sort of website-like string, e.g. wiki.caminobrowser.org/User:sardisson).

Comment 34

[Stuart Morgan]

Sorry, sloppy review-coding. Yes, that part of the code should be 
[urlString rangeOfString:@"/" options:0 range:NSMakeRange(0, colonLocation)].location == NSNotFound
That should fix the error too (which boils down to the compiler saying "I have no idea how to treat an NSRange as a boolean).

Comment 35

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

Attachment: v3.1, address sr comments

I had started messing around with that line and finally had arrived at the same conclusion just before your comment ;)

This should address all the prior sr comments and be good, modulo any new style comments on my attempt to split up that long line that had been causing compile failures ;)

Comment 36

[Stuart Morgan]

Comment on attachment 519081 [details] [diff] [review]
v3.1, address sr comments

sr=smorgan

Comment 37

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

http://hg.mozilla.org/camino/rev/972aeec1b791

Comment 38

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

Attachment: Alternate AppleScript for use in testing

This is an additional AppleScript variant that I worked up while working on this; it allows for automating both forms of testing (although there were some slight differences in how certain strings were handled with the "open" command-line tool, so I continued to test "open" manually).

Comment 39

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

We should consider this if we do a 2.0.9; I just didn't want to cram it into 2.0.8 once I rediscovered it.

Comment 40

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

CAMINO_2_0_BRANCH in advance of 2.0.9.