bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
3.86k stars 224 forks source link

ssh URI parser objects to percent-encoded @ in username #979

Closed samushenkov closed 7 months ago

samushenkov commented 7 months ago

Hello guys. I decided to try using unison app to sync my local folder with remote one.

Here is what I faced:

> ssh username@live.com@some-pc-name unison -version
< unison version 2.53.3 (ocaml 4.14.0)

> unison -testServer "absolute-win-path" ssh://username@live.com@some-pc-name//absolute-win-path
< ill-formed root specification ssh://username@live.com@some-pc-name//absolute-win-path

If I try some other username (without @) then it goes further and asks to enter password.

OS: win 11 pro / 22621.2715 Unison client: unison version 2.53.3 (ocaml 4.14.0)

gdt commented 7 months ago

The URI specification does not permit @ in the user-info field, so your ssh: URI is indeed malformed. Thus this seems not a bug.

Also, your bug report doesn't comply with https://github.com/bcpierce00/unison/wiki/Reporting-Bugs-and-Feature-Requests as it leaves out local unison version and operating system. Did you find that page on the way to opening an issue?

samushenkov commented 7 months ago

@gdt, you are right, I didn't see this page. Actually I thought that an issue is obvious one and doesn't require any extra information. Sorry for that.

Just tried to connect from linux (wsl) to windows machine - works fine.

> ssh -V
< OpenSSH_8.7p1, OpenSSL 3.0.7 1 Nov 2022

So 2 different clients can parse username with @ character without any problem. Btw, microsoft now forces users to use online accounts (email-like), so others can face this issue too.

gdt commented 7 months ago

@gdt, you are right, I didn't see this page. Actually I thought that an issue is obvious one and doesn't require any extra information. Sorry for that.

In this case it didn't really matter, but in general we have trouble with incomplete reports, and also with questions.

Just tried to connect from linux (wsl) to windows machine - works fine.

> ssh -V
< OpenSSH_8.7p1, OpenSSL 3.0.7 1 Nov 2022

So 2 different clients can parse username with @ character without any problem.

The ssh command line does not use ssh: URIs, so that isn't really surprising. ssh documents

ssh connects and logs into the specified destination, which may be
specified as either [user@]hostname or a URI of the form
ssh://[user@]hostname[:port].

which doesn't really allow or disallow @ in user or hostname.

I believe that @ can be logically in an ssh URI, but has to be escaped. See https://www.rfc-editor.org/rfc/rfc3986 for details. I think %40 might be what you need.

Btw, microsoft now forces users to use online accounts (email-like), so others can face this issue too.

Wow; I had no idea (but I am a MS non-user).

samushenkov commented 7 months ago

Just gave it a try:

> ssh ssh://username%40live.com@some-pc-name

SSH works perfect, but in this case I need to specify ssh uri to work.

> unison -testServer "absolute-win-path" ssh://username%40live.com@some-pc-name//absolute-win-path
< Fatal error: There's a problem with one of the roots:
< ill-formed root specification ssh://username%40live.com@some-pc-name//absolute-win-path

The same issue. I think escape character % is not allowed too.

gdt commented 7 months ago

That might be a real bug then. I retitled the issue, and added a defect label. I added a windows label, as it is obvious that @ does not belong in account names, and it appears to be a windows-only issue at least for now. (It's kind of like thinking it's ok to put spaces in file names :-)

See src/clroot.ml for the parsing code. Glancing really quickly I am not surprised that it doesn't handle the full complexity of URIs. However, I think some characters that are legal in user-info are missing from the grammar, and you can probably add % in userAtRegexp.

gdt commented 7 months ago

Now that I see there is a regexp that is clearly at odds with the standards, i dropped the windows label.

tleedjarv commented 7 months ago

I believe it's an unmotivated limitation. I've opened a PR to fix this specific use case (also accept the @ directly). I think even more should be accepted in the username but that's something for another day.