Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rejig URL string sniffing to be more robust #1312

Merged
merged 3 commits into from Jan 15, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 6, 2013

  • Don't use Apple's URLWithString method, it's flaky
  • support for ports in the URL
  • support for whitespace in the path (e.g. http://google.com/?q=my search string )
  • support for 'localhost' URLs
  • Better validation of IP addresses
  • Correctly apply the scheme when it doesn't exist in the URL string
  • Tidy up the sniffString: method to allow for tests after the URL test (it doesn't necessarily have to be last anymore)

A carry on from the work at #1246

* Don't use Apple's URLWithString method, it's flaky
* support for ports in the URL
* support for whitespace in the path (e.g. http://google.com/?q=my search string )
* support for 'localhost' URLs
* Better validation of IP addresses
* Correctly apply the scheme when it doesn't exist in the URL string
* Tidy up the sniffString: method to allow for tests after the URL test (it doesn't necessarily have to be last anymore)
@pjrobertson
Copy link
Member Author

pjrobertson commented Jan 6, 2013

I've also added another two commits that update the list of valid TLDs to the latest list from Iana (and a script to easily parse this list, will save you a few seconds :) )

@skurfer
Copy link
Member

skurfer commented Jan 11, 2013

This seems to work really well.

Since I complained about Safari 6, I've learned that you can simulate the old behavior by appending /. What would you think about adding that ability to Quicksilver? That is, typing "qsapp/" would be treated as the URL "http://www.qsapp.com". So if the last character is a slash, we could check the rest of the string to see if it's a valid domain name. I made a note of the rules in the Remote Hosts source. One thing I don't know is whether or not the Safari behavior is localized. For example, if you type "cnn/", does it take you to cnn.com, or cnn.co.uk?

@pjrobertson
Copy link
Member Author

pjrobertson commented Jan 11, 2013

Good, I'm glad :)
I think the moral of the story is... don't be scared to move away from
Apple's methods, they're not always right.

Since I complained about Safari 6, ...

Are you running the 10.8.3 preview? I'm on Safari 6.0.2 and what you
describe doesn't work.
Typing google/ or cnn/ takes me to 'page not found'. Are you sure it's not
a DNS thing your host is doing? Try changing your DNS to
opendnshttp://www.opendns.com (servers
are 208.67.222.222 and 208.67.220.220) and what do you get. It's what I
use, and I get taken to the openDNS 'page not found' splash as I say.

Tbh, even if it is a Safari thing, I wouldn't really like what you describe
anyway. I don't see why .com should be the default (OK, most of our users
are from the US) and I don't think I've ever heard of that 'feature'
before, so who's it going to help? (Except maybe you ;-) )

On 11 January 2013 15:04, Rob McBroom notifications@github.com wrote:

This seems to work really well.

Since I complained about Safari 6, I've learned that you can simulate the
old behavior by appending /. What would you think about adding that
ability to Quicksilver? That is, typing "qsapp/" would be treated as the
URL "http://www.qsapp.com". So if the last character is a slash, we could
check the rest of the string to see if it's a valid domain name. I made a
note of the rules in the Remote Hosts sourcehttps://github.com/skurfer/RemoteHosts-qsplugin/blob/master/RemoteHostsSource.m#L86.
One thing I don't know is whether or not the Safari behavior is localized.
For example, if you type "cnn/", does it take you to can.com, or cnn.co.uk
?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1312#issuecomment-12147654.

@skurfer
Copy link
Member

skurfer commented Jan 11, 2013

Are you sure it's not a DNS thing your host is doing?

Can't be. Doesn't work in Chrome, but does work in iOS, which moves around to all sorts of different DNS servers. (By "works", I mean if you type "qsapp", you'll see it briefly change to "qsapp/" then to the full URL.)

Maybe it's hard-coded to .com and just disabled in places where that doesn't make sense. I'm on 6.0.2 as well (on 10.8.2).

I don't see why .com should be the default (OK, most of our users are from the US) and I don't think I've ever heard of that 'feature' before, so who's it going to help?

Well, if we implement it and promote it, maybe it will come to be thought of as yet another cool Quicksilver feature. :-)

(Except maybe you ;-) )

You act as if that's not enough justification. I'm confused. (And there are few things I hate more than wanting to use a smiley inside parens. I usually rearrange huge paragraphs to avoid making the choice. One of the 21st century's greatest challenges. :-) ) ←Ugh. Horrible.

@pjrobertson
Copy link
Member Author

pjrobertson commented Jan 11, 2013

Can't be. Doesn't work in Chrome, but does work in iOS, which moves
around to all sorts of different DNS servers. (By "works", I mean if you
type "qsapp", you'll see it briefly change to "qsapp/" then to the full
URL.)

Interesting, still can't get it to work here. Can't see any options in the
prefs or menu bar either.

Well, if we implement it and promote it, maybe it will come to be thought
of as yet another cool Quicksilver feature. :-)

True :P
As usual, you've convinced me. If you're keen, feel free to go for it.

And there are few things I hate more than wanting to use a smiley inside
parens

I think that may have been the first time I've done it, I've finally
succumbed. C'mon, it's not all that bad? As long as you put a space in it's
fine :)

(Except maybe you ;-) )

You act as if that's not enough justification. I'm confused.

That was an "if you want to do it, go for it, but I'm not fussed" comment.
Feel free!

On 11 January 2013 16:26, Rob McBroom notifications@github.com wrote:

Are you sure it's not a DNS thing your host is doing?

Can't be. Doesn't work in Chrome, but does work in iOS, which moves around
to all sorts of different DNS servers. (By "works", I mean if you type
"qsapp", you'll see it briefly change to "qsapp/" then to the full URL.)

Maybe it's hard-coded to .com and just disabled in places where that
doesn't make sense. I'm on 6.0.2 as well (on 10.8.2).

I don't see why .com should be the default (OK, most of our users are from
the US) and I don't think I've ever heard of that 'feature' before, so
who's it going to help?

Well, if we implement it and promote it, maybe it will come to be thought
of as yet another cool Quicksilver feature. :-)

(Except maybe you ;-) )

You act as if that's not enough justification. I'm confused. (And there
are few things I hate more than wanting to use a smiley inside parens. I
usually rearrange huge paragraphs to avoid making the choice. One of the
21st century's greatest challenges. :-) ) ←Ugh. Horrible.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1312#issuecomment-12151598.

@skurfer
Copy link
Member

skurfer commented Jan 11, 2013

OK, I'll look into it after this is merged.

skurfer added a commit that referenced this pull request Jan 15, 2013
Rejig URL string sniffing to be more robust
@skurfer skurfer merged commit 7a66039 into quicksilver:master Jan 15, 2013
@pjrobertson pjrobertson deleted the sniffURLs branch Jan 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants