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

Don't double-encode URLs before parsing them #33

Merged
merged 1 commit into from
Sep 6, 2013
Merged

Don't double-encode URLs before parsing them #33

merged 1 commit into from
Sep 6, 2013

Conversation

s4y
Copy link
Contributor

@s4y s4y commented Jul 17, 2013

#7 (316) wasn't a bug. When someone constructs a mvim: URL, they should end up escaping the unsafe characters in the path twice: once for the file: URL (e.g. file://Users/steve/foo%20bar/baz.txt) and again for the mvim: URL.

Escaping the whole thing inside MacVim, like this fix did, breaks legit URLs. Here's an example in Python:

import urllib, webbrowser
url = "mvim://open?" + urllib.urlencode({
    "url": "file://" + urllib.pathname2url('/Users/steve/foo bar/baz.txt')
})
print(url) # -> mvim://open?url=file%3A%2F%2F%2FUsers%2Fsidney%2Ffoo%2520bar%2Fbaz.txt
webbrowser.open(url) # Should work, currently fails

In ObjC, the slashes and colon aren't escaped (equally valid as what Python does) but the space does get correctly double-escaped, so same breakage:

NSLog(@"mvim://open?url=%@", [
    [[NSURL fileURLWithPath:@"/Users/sidney/foo bar/baz.txt"] absoluteString]
    stringByAddingPercentEscapesUsingEncoding:NSUTF8StringEncoding
]); // -> mvim://open?url=file://localhost/Users/sidney/foo%2520bar/baz.txt

<https://code.google.com/p/macvim/issues/detail?id=316> wasn't a bug.

When you're constructing a URL, you *should* end up escaping URL-unsafe
characters twice: once for the `file:` URL and once for the `mvim:` URL.
Escaping it inside MacVim breaks legit URLs with escapes at the `mvim:` level.
b4winckler added a commit that referenced this pull request Sep 6, 2013
Don't double-encode URLs before parsing them
@b4winckler b4winckler merged commit 0f3e96b into b4winckler:master Sep 6, 2013
@b4winckler
Copy link
Owner

Thanks! For some reason I forgot about to review this pull request...sorry about that.

@s4y
Copy link
Contributor Author

s4y commented Sep 7, 2013

Sweet! No worries.

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.

2 participants