macvim url scheme with line number is broken

Describe the bug
mvim url scheme with line number is not working on the latest release (Snapshot 162)

To Reproduce
Detailed steps to reproduce the behavior:

open a url with line number param (e.g. mvim://open?url=file://%2FUsers%2Frahulagrawal%2FCode%2Flimebike-web%2Fspec%2Fservices%2Fbike_scan_processor_spec.rb&line=87)
With 162 installed, the file opens but not set to the line number
Downgrade to 161 and try the same url, it works correctly this time.

Expected behavior
File opens at the passed line number

Screenshots

Environment (please complete the following information):

Latest snapshot (162)
OS: macOS 10.15.1
Terminal: iTerm2, browser firefox.

Additional context
Potentially related to #1002 ?

@frarees

cc @frarees (Sorry if this is not related to your commit)

It is related to #1002 definitively. So I'm not sure if RFC 2396 defines what happens when a query value is a URI itself, if it need to be re-encoded or not. Is there somewhere a statement on this specific scenario?

In your case, not re-encoding (e.g. mvim://open?url=file:///Users/rahulagrawal/Code/limebike-web/spec/services/bike_scan_processor_spec.rb&line=87) works on 8.2.319+.

This is what I do on bash to detect the version. Not bulletproof, enough for my use case:

function urlenc
{
	/usr/bin/python -c "import sys, urllib as ul; print ul.quote(sys.argv[1],'/')" "$1"
}

FILE=$1
LINE=$2

MVIMVERSION=`/usr/local/bin/mvim --version | grep 'VIM - Vi IMproved' | cut -d ' ' -f 5`
# not 100% accurate, we could have several patch defs e.g. 1-200, 205-300, 302-500
MVIMPATCH=`/usr/local/bin/mvim --version | grep 'Included patches' | cut -d '-' -f 2`

ENC_FILE=`urlenc "$FILE"`

# pre 8.2.319
#   re-encode, since MacVim mistakenly decodes it
# see https://github.com/macvim-dev/macvim/pull/1002
if [[ $MVIMVERSION < '8.2' || $MVIMVERSION == '8.2' && $MVIMPATCH < '319' ]]; then
	ENC_FILE=`urlenc "$ENC_FILE"`
fi

URI="mvim://open?url=file://$ENC_FILE&line=$LINE"

Yeah that seems directly responsible for the issue. I’ll take a look tonight. We should add a regression test as well (which a lot of MacVim-specific features don’t have, unfortunately).

Fix macvim-dev#1020

@rahul342

@rahul342 can you check #1021 see if it works for you?

@frarees

@frarees Would I need to compile and build your branch to test? What's the quickest way to try your change?

Hmm I don’t think this needs additional testing. I tried it and it does work and it’s possible to test locally as well

Merge pull request #1021 from frarees/uri_encode

Sorry for the late response. There are build instruction on the wiki. Snapshot 163 will contain the changes, so you could wait for it to be released too.

MacVim Snapshot 163

Fix mvim:// protocol handler not working well with iTerm2

Fix external editor URL scheme improperly encoding file URLs

I made a workaround that should fix this. Will be out next releease.

The proper file protocol URL for /tmp/%2fz is file:///tmp/%252fz (You can try this with your own web browser). For /tmp/x&line=5 the URL should be file:///tmp/x%26line=5.

Agreed. But if you copy that file:// URL and paste it into the GitHub search box at the top of this page, and then look at the resulting URL, you will find that the way you pass it inside a querystring value, according to your browser, is to encode it again. And that's what we're doing here: the mvim: bit isn't a static string prefix, but a nested URL. Assuming your browser encodes as aggressively as mine, you'll also find that you can manually replace the :, /, and =, and the server will interpret the value the same way, because those are defined as being interchangeable in that position.

So, while I agree that it is unnecessary to encode the slashes because they never have significance after a ?, I maintain that it is equally valid for one level of / -> %2F transformation to have been applied -- because the value we're talking about is an encoding of the file: URL, not an encoding within the file: URL.

All of mvim://open?url=file:///tmp/x%2526line=5, mvim://open?url=file:%2F%2F%2Ftmp%2Fx%2526line=5, and mvim://open?url=%66%69%6c%65%3a%2f%2f%2f%74%6d%70%2f%78%25%32%36%6c%69%6e%65%3d%35 contain exactly the same url value (file:///tmp/x%26line=5, which we agree represents the file /tmp/x&line=5), and a piece of software generating any of them is behaving correctly.

Your suggested mvim://open?url=file:///tmp/x%26line=5 does not: any URL parser will state that that contains a url value of file:///tmp/x&line=5. We can get away with that one, because although it's not the file: URL we agree it should be, there's no other reasonable interpretation of that character in that position -- though it'd be a different story if the & was a ?. But in your last example, being insufficiently-encoded is a clear problem: mvim://open?url=file:///tmp/x%2526line=5 contains a url of file:///tmp/x%26line=5, which per above, you agree refers to /tmp/x&line=5, not /tmp/x%26line=5.

I appreciate a partial workaround, but I'm still extremely confident that #1002 was wrong in its premise, and there was never a bug to begin with. The URL for /tmp/foo bar is file:///tmp/foo%20bar, which has an mvim URL of mvim://open?url=file:///foo%2520bar, and that has worked just fine for the last 10 years.

I'm still extremely confident that #1002 was wrong in its premise, and there was never a bug to begin with.

That could totally be it. And since it's both breaking compatibility for some users and seems people have been happy for a long time before, I proposed to just go back to how it was (#1040). Either way, it's great that there's a discussion going on how to do this right.

This is how I made it work with the original implementation (161 and previous):

function urlenc
{
	/usr/bin/python -c "import sys, urllib as ul; print ul.quote(sys.argv[1],'/')" "$1"
}

FILE=$1
LINE=$2

ENC_FILE=`urlenc "$FILE"`
ENC_FILE=`urlenc "$ENC_FILE"`
URI="mvim://open?url=file://$ENC_FILE&line=$LINE"

As you see, I had to do the encoding twice, otherwise e.g. files with spaces would not work. Encoding twice felt wrong, but maybe I'm missing something?

@frarees

@frarees does this help it feel any less wrong?

FILE=$1
LINE=$2

ENC_FILE=`urlenc "$FILE"`
FILE_URL="file://$ENC_FILE"

ENC_URL=`urlenc "$FILE_URL"`

# Obviously unnecessary, because we know what characters are in there, but technically correct, and helpful for symmetry
ENC_LINE=`urlenc "$LINE"`

URI="mvim://open?url=$ENC_URL&line=$ENC_LINE"

@matthewd

@matthewd it does. It helps understand there's the file://<encoded contents> and the mvim://<encoded contents>, whereas in my example it feels like there are two encodings cause the program is decoding the same thing twice.

@matthewd

@matthewd There are a couple things going on here, but I think we should at least agree that this issue's original URL (the one currently generated by iTerm2) is not formed properly? It looks like this:

File: /tmp/file name.txt -> URL: file://%2Ftmp%2Ffile%20name.txt.

Either way the space should be encoded one more time than the slash. Either of the two below could work depending on the encoding scheme:

(No encoding) file:///tmp/file%20name.txt
(Encoding) file:%2F%2F%2Ftmp/file%2520name.txt or file:///tmp/file%2520name.txt.

I appreciate a partial workaround, but I'm still extremely confident that #1002 was wrong in its premise, and there was never a bug to begin with. The URL for /tmp/foo bar is file:///tmp/foo%20bar, which has an mvim URL of mvim://open?url=file:///foo%2520bar, and that has worked just fine for the last 10 years.

I kind of agree with you that most URL component parser will decode the query items and then we should theoretically be encoding a space in file name as %2520 to double-encode it so it decodes properly when being parsed by a URL component parser. Couple issues I see:

MacVim claims to be TextMate compatible, and TextMate doesn't do double decoding, so it expects a space to be %20.
iTerm2 also doesn't do double-encoding, and if you cmd-click on a file name with space, it will just encode it once to %20. That could potentially be fixed, but it does lead to an awkward situation where different people running different versions of iTerm2 and MacVim will have different results with their names not being respected. I'm not sure if there is a good solution for this.

On the other hand MacVim has always double-decoded and the recent change did break it. I guess I can see the value of reverting back to the original behavior and just ask iTerm2 to fix it for MacVim integration.

TextMate doesn't do double decoding, so it expects a space to be %20

Oh!

iTerm2 also doesn't do double-encoding, and if you cmd-click on a file name with space, it will just encode it once to %20

Oh oh!

Sorry, I definitely should've at least checked the latter. This has worked so reliably for me (both from iTerm and elsewhere) for so long, and I apparently just assumed I'd opened space-containing paths from iTerm along the way. 😔

Sooo.... my revised proposal is that MacVim should double-decode [and that ideally URL-building applications should double-encode], but that MacVim also accomodate under-encoded values encountered along the way. (I think this is slightly different from #1043, but I could easily be misreading that.)

Extract the 'url' value from the mvim: URL
Decode it as "normal"
You now have a file: URL with some characters still encoded (properly behaving client), but also possibly with some characters not (e.g. raw space, per your example)
Re-encode characters that are not permitted in a URL
Parse the result as a URL, to extract the path etc.

I think 3-5 there describes how a web browser behaves, for example: when given http://www/foo%20bar, it will request exactly that from the server; when given http://www/foo bar, it'll assume you meant the first one, and request the same thing.

In other words, double-encoding is the right thing, and extracting the parameter from the mvim: URL will always perform the initial decode -- MacVim just then chooses to be accomodating on the second decode, by not erroring when it sees a non-URL-valid symbol, and instead pretends such a character was in fact encoded.

I'm afraid I got a bit lost trying to figure out how to spell that in ObjC, but I think this does what I have in mind in Swift, if that's close enough to make sense?

import Foundation

let urlSet = CharacterSet.urlFragmentAllowed
                .union(.urlHostAllowed)
                .union(.urlPasswordAllowed)
                .union(.urlQueryAllowed)
                .union(.urlUserAllowed)
                .union(CharacterSet(charactersIn: "%"))

print("file:///tmp/foo%20bar baz".addingPercentEncoding(withAllowedCharacters: urlSet) ?? "?");

In this proposal, file:///tmp/foo bar baz, file:///tmp/foo%20bar baz, and file:///tmp/foo%20bar%20baz are all equivalent, and mean /tmp/foo bar baz. Consequently, so do both mvim://open?url=file:///tmp/foo%20bar%20baz and mvim://open?url=file:///tmp/foo%2520bar%2520baz.

The only way to reference a file literally named /tmp/foo%20bar is as file:///tmp/foo%2520bar, and thus mvim://open?url=file:///tmp/foo%252520bar. This is a breaking change from the pre-#1002 behaviour, where mvim://open?url=file:///tmp/foo%2520bar would previously have opened that file, but would now look for /tmp/foo bar. But in exchange for breaking previously-working URLs that reference a filename containing a % (or ?/#), we fix compatibility with existing clients that under-escape, for all other characters.

And we should then encourage iTerm to adopt double-escaping, because that's the only truly unambiguous way to communicate all possible paths, and is just The Right Thing.

Old/current iTerm single-escaped -> Old MacVim = (old behaviour) spaces break, basic paths work
Old/current iTerm single-escaped -> Proposed MacVim = all paths work unless containing %, ?, #
Future iTerm double-escaped -> Old MacVim = all paths work
Future iTerm double-escaped -> Proposed MacVim = all paths work

On first glance, this seems ok. It's kind of an ugly compromise that basically works either either method, and with special characters like "%" we need the double-encoding for it to work (I think spaces are the main benefits of it). It's a strictly looser set of allowed input than old MacVim.

I still need to think about this a little. Magic behaviors with ambiguities in APIs can sometimes leads to training users with sloppy expectations (since someone may try it and it works) but there is the benefit of working with existing tools.

Fix mvim:// protocol double-encoding behavior handling properly

Sorry for the delay, but I just filed #1055 which fixes these encoding issues as described above. Basically, it will use double-encoding by default, just like how MacVim used to work before the recent changes, but it will opportunistically support single-encoding as well to make it play nice with iTerm.

sounds great. thanks.

Navigation Menu

Repository navigation

macvim url scheme with line number is broken #1020

30 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

macvim url scheme with line number is broken #1020

Description

Activity

rahul342 commented on Mar 10, 2020

frarees commented on Mar 10, 2020

ychin commented on Mar 10, 2020

frarees commented on Mar 16, 2020

rahul342 commented on Mar 16, 2020

ychin commented on Mar 17, 2020

frarees commented on Mar 17, 2020

30 remaining items

ychin commented on Apr 20, 2020

matthewd commented on Apr 20, 2020

frarees commented on Apr 20, 2020

matthewd commented on Apr 20, 2020

frarees commented on Apr 20, 2020

ychin commented on Apr 20, 2020

matthewd commented on Apr 21, 2020

ychin commented on Apr 21, 2020

ychin commented on Jul 6, 2020

dx7 commented on Jul 6, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions