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

String sniffing fixes for URLs #1594

Merged
merged 6 commits into from Sep 19, 2013
Merged

String sniffing fixes for URLs #1594

merged 6 commits into from Sep 19, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Sep 9, 2013

In working with Howard on #1553, I ran across a bug with this search URL:

http://en.wikipedia.org/wiki/Special:Search?search=***

while adding a port would make it work as expected

http://en.wikipedia.org:80/wiki/Special:Search?search=***

You can test this by running ⌘⎋ on both of those and checking the default action.

So, I added some unit tests for various colon placement combinations, then changed the string sniffing code so the tests would pass.

In a nutshell, all I did was split on / first before splitting on :. That way, we only worry about colons in the hostname:port part of the URL. All other colons are ignored.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 17, 2013

If none of the beautiful URL testing tests fail now, then that's good enough for me :D

Is it worth adding a test for a search URL with a port only?

http://google.com:80/?searching=***

That works now, but it may be worth testing it for the future... maybe

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 17, 2013

Is it worth adding a test for a search URL with a port only?

Can't hurt. I might also take this opportunity to add a small pet feature relating to URLs.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 17, 2013

Ooooh I'm waiting! My merge finger is frozen :)

On 17 Medi 2013, at 11:29, Rob McBroom notifications@github.com wrote:

Is it worth adding a test for a search URL with a port only?

Can't hurt. I might also take this opportunity to add a small pet feature relating to URLs.


Reply to this email directly or view it on GitHub.

skurfer added 3 commits Sep 17, 2013
Easier to read and modify.
qsapp/ will become http://www.qsapp.com/

The URL pattern is localized, so it can potentially be used in other countries.

Tests for this type of input were also added.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 17, 2013

OK, it's ready to be looked over.

// Check the port (if it exists). URLs may contain multiple colons, so we take the first occurance of it. e.g. http://google.com:80/?q=this_is_a_:
NSString *port = [[[colonComponents objectAtIndex:1] componentsSeparatedByString:@"/"] objectAtIndex:0];
NSString *port = [colonComponents objectAtIndex:1];
Copy link
Member

@tiennou tiennou Sep 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the stance on object-literal access ? e.g colonComponents[1]. I'm using that now, and it sure feels leaner. (There's one above and one below)

Copy link
Member Author

@skurfer skurfer Sep 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love all the new syntax. Long overdue. And we've agreed to allow it since 10.6 was dropped.

Copy link
Member

@pjrobertson pjrobertson Sep 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I've added info to the wiki page:
http://qsapp.com/wiki/Github

I haven't mentioned indentation, but just referenced the LLVM docs ;-)

@tiennou
Copy link
Member

@tiennou tiennou commented Sep 17, 2013

Other than that, looks good. Nice to see a test ;-).

EDIT: I'll try to keep the IRC alive and cozy tonight, if you guys are around.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 18, 2013

+1 for the tests and localisation :)

Strange, qsapp/ doesn't work for me in Safari (6.0.5). Is there something I need to enable?
Also - I'm guessing you just copied what Safari does, using http:// and www. - the other day I was thinking about QS's way at assuming a URL is http:// (and not https://). Not very security conscious ;-)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 18, 2013

Strange, qsapp/ doesn't work for me in Safari (6.0.5). Is there something I need to enable?

I think we talked about this before. I couldn't remember if it didn't work for you or if it wasn't useful since it assumes a U.S. URL. I suspect they didn't make the behavior internationally friendly. So once again, QS has more to offer than the built-in tools. 😃

Maybe try changing to the U.S. in system prefs? (If you even care to see it. I think you get the idea.) Safari on iOS also does this if you want to give that a try. In fact, if you type "qsapp" in Mobile Safari and hit Go, you might see it change to "qsapp/" for a fraction of a second before being expanded.

Something else I realized while working on this: We shouldn't be allowing URLs where one of the components begins or ends with - (though it is allowed in the middle). I just didn't have the motivation to deal with it. Now that we're 10.7+, I think there's an NSRegularExpression we could use, but that would be a pretty big rewrite. Should we just leave it for now?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 18, 2013

Now you mention it I do seem to remember. Works on iOS. Kind of a quirky thing though.

Does this mean that when I type this/that into QS in text mode, this/ will be changed to a URL briefly?
Maybe a hidden pref?

Should we just leave it for now?

I'm fine with that. It's much more robust than Apple's URL sniffing anyways :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 18, 2013

Does this mean that when I type this/that into QS in text mode, this/ will be changed to a URL briefly?

Yes, I suppose it will. Is that really a problem, though?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 19, 2013

Not especially, just checking ;-)

I know I won't use this (well, I can't in Safari either, so it's not normal for me), so it would be a minor annoyance, but nothing major.
I guess if we have any negative feedback from the community we can make it a hidden pref or something

(FYI: I tried changing my language to 'English' and my 'region' to US, but the page still doesn't load in Safari. It's not some extension you have is it?)

On 19 Medi 2013, at 02:49, Rob McBroom notifications@github.com wrote:

Does this mean that when I type this/that into QS in text mode, this/ will be changed to a URL briefly?

Yes, I suppose it will. Is that really a problem, though?


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 19, 2013

For reference: http://hints.macworld.com/article.php?story=20120919001200353

On 19 Sep 2013, at 17:13, Patrick Robertson robertson.patrick@gmail.com wrote:

Not especially, just checking ;-)

I know I won't use this (well, I can't in Safari either, so it's not normal for me), so it would be a minor annoyance, but nothing major.
I guess if we have any negative feedback from the community we can make it a hidden pref or something

(FYI: I tried changing my language to 'English' and my 'region' to US, but the page still doesn't load in Safari. It's not some extension you have is it?)

On 19 Medi 2013, at 02:49, Rob McBroom notifications@github.com wrote:

Does this mean that when I type this/that into QS in text mode, this/ will be changed to a URL briefly?

Yes, I suppose it will. Is that really a problem, though?


Reply to this email directly or view it on GitHub.

pjrobertson added a commit that referenced this issue Sep 19, 2013
@pjrobertson pjrobertson merged commit 44c239b into master Sep 19, 2013
@pjrobertson pjrobertson deleted the sniffSearch branch Sep 19, 2013
@tiennou
Copy link
Member

@tiennou tiennou commented Sep 19, 2013

Yeah, I actually suspected DNS settings too and was about to point that out.

Le 19 sept. 2013 à 10:14, Patrick Robertson notifications@github.com a écrit :

For reference: http://hints.macworld.com/article.php?story=20120919001200353

On 19 Sep 2013, at 17:13, Patrick Robertson robertson.patrick@gmail.com wrote:

Not especially, just checking ;-)

I know I won't use this (well, I can't in Safari either, so it's not normal for me), so it would be a minor annoyance, but nothing major.
I guess if we have any negative feedback from the community we can make it a hidden pref or something

(FYI: I tried changing my language to 'English' and my 'region' to US, but the page still doesn't load in Safari. It's not some extension you have is it?)

On 19 Medi 2013, at 02:49, Rob McBroom notifications@github.com wrote:

Does this mean that when I type this/that into QS in text mode, this/ will be changed to a URL briefly?

Yes, I suppose it will. Is that really a problem, though?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 19, 2013

I don't have .com as a search domain, but I think that trick would have always worked. Nothing to do with a specific browser.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 19, 2013

Seems it is a domain thing - but only in Safari ?! :S

I use this: http://www.opendns.com/technology/dnscrypt/

I just disabled it and now I get the wonderful .com completion ;-)
quickly goes and re-enables DNSCrypt, for safety reasons of course :P

On 19 Sep 2013, at 23:31, Rob McBroom notifications@github.com wrote:

I don't have .com as a search domain, but I think that trick would have always worked. Nothing to do with a specific browser.


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 19, 2013

P.S. If you're prone to DNS poisoning (the EU and US fast approaching the the same levels of web restrictions as China, who use it) then DNSCrypt works a treat

On 19 Sep 2013, at 23:45, Patrick Robertson robertson.patrick@gmail.com wrote:

Seems it is a domain thing - but only in Safari ?! :S

I use this: http://www.opendns.com/technology/dnscrypt/

I just disabled it and now I get the wonderful .com completion ;-)
quickly goes and re-enables DNSCrypt, for safety reasons of course :P

On 19 Sep 2013, at 23:31, Rob McBroom notifications@github.com wrote:

I don't have .com as a search domain, but I think that trick would have always worked. Nothing to do with a specific browser.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 19, 2013

If you're prone to DNS poisoning (the EU and US fast approaching the the same levels of web restrictions as China, who use it) then DNSCrypt works a treat

Sounds cool, but I have things at home and especially at work that aren't in public DNS, so OpenDNS wouldn't be able to resolve them. :-(

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 19, 2013

I was gonna say that OpenDNS has a feature for that - it's called 'shortcuts' http://blog.opendns.com/2007/04/22/shortcut-the-web/
But they've disabled it for new users since it was apparently buggy.

DNSCrypt does have a setting for bypassing certain domains as well:

Your problem sounds interesting - do you have your own DNS for your custom domain resolving, or just a hosts file edit or something?

On 20 Sep 2013, at 00:36, Rob McBroom notifications@github.com wrote:

If you're prone to DNS poisoning (the EU and US fast approaching the the same levels of web restrictions as China, who use it) then DNSCrypt works a treat

Sounds cool, but I have things at home and especially at work that aren't in public DNS, so OpenDNS wouldn't be able to resolve them. :-(


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 20, 2013

I have a DNS server at home that all the devices in house use. No tricky redirection or spoofing for me. :-)

At work, we have “internal” and public-facing DNS. Pretty much everything I touch only exists internally.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 21, 2013

I guessing more complicated world than the one I live it. All I need is GitHub and Facebook (although not in China :P)

Interesting though :)

On 20 Medi 2013, at 22:42, Rob McBroom notifications@github.com wrote:

I have a DNS server at home that all the devices in house use. No tricky redirection or spoofing for me. :-)

At work, we have “internal” and public-facing DNS. Pretty much everything I touch only exists internally.


Reply to this email directly or view it on GitHub.

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

3 participants