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

v2 put/get don't perform tilde homedir expansion #1653

Closed
pint12 opened this Issue Aug 28, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@pint12

pint12 commented Aug 28, 2017

i have set
remote= '~/temp'
put api translated it to '/home/user/~/temp'
that caused the process to fail .

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 29, 2017

Member

Thanks for the report! I probably haven't gotten around to porting the remote equivalent of os.path.expanduser from v1. Just looked and yup, it's very basic right now. Made myself a todo.

Member

bitprophet commented Aug 29, 2017

Thanks for the report! I probably haven't gotten around to porting the remote equivalent of os.path.expanduser from v1. Just looked and yup, it's very basic right now. Made myself a todo.

@bitprophet bitprophet added this to the 2.0 milestone Aug 29, 2017

@bitprophet bitprophet modified the milestones: 2.0, 2.0.x May 29, 2018

@bitprophet bitprophet changed the title from fabric2 - does not except ~ to v2 put/get don't perform tilde homedir expansion Jul 23, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 23, 2018

Member

I recall one reason this wasn't done yet is it entails running shell commands in the background, which can rarely (but frustratingly) cause unexpected issues – imagine a user accidentally misconfiguring something related to command execution (or the remote server having some issue along the same lines) and then having shell exec errors bubble up from what they thought was a "simple" file transfer.

However given we had to implement it that way in v1, I assume there's no standard way of having the SFTP server perform expansion on our behalf, so we may be stuck doing the same thing here. Gonna double check first tho.

If we do have to go that road I wonder if there are any ways to make it cleaner...for example having an opt-in expand_tildes kwarg or something, so that users have to understand they are turning additional behavior on. I don't really like that idea – it's a lot of friction for a very common necessity – but it still occurred to me.

Possibly best is to "amend" any exceptions that bubble up from the expansion step to make it clear they happened internally; or at least, to add very strict tests that ensure we always get a clear traceback so the user or a maintainer will see "ah yes, this is the internal tilde expansion – try running a regular command first to see what's going wrong"?

Member

bitprophet commented Jul 23, 2018

I recall one reason this wasn't done yet is it entails running shell commands in the background, which can rarely (but frustratingly) cause unexpected issues – imagine a user accidentally misconfiguring something related to command execution (or the remote server having some issue along the same lines) and then having shell exec errors bubble up from what they thought was a "simple" file transfer.

However given we had to implement it that way in v1, I assume there's no standard way of having the SFTP server perform expansion on our behalf, so we may be stuck doing the same thing here. Gonna double check first tho.

If we do have to go that road I wonder if there are any ways to make it cleaner...for example having an opt-in expand_tildes kwarg or something, so that users have to understand they are turning additional behavior on. I don't really like that idea – it's a lot of friction for a very common necessity – but it still occurred to me.

Possibly best is to "amend" any exceptions that bubble up from the expansion step to make it clear they happened internally; or at least, to add very strict tests that ensure we always get a clear traceback so the user or a maintainer will see "ah yes, this is the internal tilde expansion – try running a regular command first to see what's going wrong"?

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 23, 2018

Member

Reading the RFCs, I'm reminded that technically one does not usually need the tilde with SFTP, since the remote CWD always starts out in the connecting user's home directory. The only time this is not true is if one is using the (Paramiko-specific; not part of the SFTP spec; implemented purely locally) SFTPClient.chdir method, and Fabric doesn't expose that anyways.

From draft 13, section 6 of the spec:

File names starting with a slash are "absolute", and are relative to
the root of the file system. Names starting with any other character
are relative to the user's default directory (home directory).

Member

bitprophet commented Jul 23, 2018

Reading the RFCs, I'm reminded that technically one does not usually need the tilde with SFTP, since the remote CWD always starts out in the connecting user's home directory. The only time this is not true is if one is using the (Paramiko-specific; not part of the SFTP spec; implemented purely locally) SFTPClient.chdir method, and Fabric doesn't expose that anyways.

From draft 13, section 6 of the spec:

File names starting with a slash are "absolute", and are relative to
the root of the file system. Names starting with any other character
are relative to the user's default directory (home directory).

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 23, 2018

Member

Given that, one option may be to do something like if path.startswith("~/"): path = path[2:], i.e. to trim that tilde and leave the path relative. I don't love this either since it feels pretty fragile.

And in any case, the "just rely on CWD being $HOME" tactic is itself also a bit fragile, eg I can imagine various setups where users' contexts are set elsewhere (though in this case, such an SFTP server may not ALLOW paths outside that root).

No great options here so I am tempted to just go with the obvious after all. It never seemed to generate too many problems in v1 that I recall.

Member

bitprophet commented Jul 23, 2018

Given that, one option may be to do something like if path.startswith("~/"): path = path[2:], i.e. to trim that tilde and leave the path relative. I don't love this either since it feels pretty fragile.

And in any case, the "just rely on CWD being $HOME" tactic is itself also a bit fragile, eg I can imagine various setups where users' contexts are set elsewhere (though in this case, such an SFTP server may not ALLOW paths outside that root).

No great options here so I am tempted to just go with the obvious after all. It never seemed to generate too many problems in v1 that I recall.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 23, 2018

Member

Oh nice, I was living in the past, went to look at exactly what Fabric 1.x does now, and we replaced the echo $HOME or whatever with sftp.normalize(".") at some point (or I'm misremembering and the echo was elsewhere...)

Missed normalize on my initial skimming of Paramiko's SFTP APIs. This makes me happy!


On reflection though: this doesn't actually do anything "real"; it should behave exactly like removing the tilde in the first place! (Again, aside from use of that chdir method, which v1 never used and we currently don't use either.)

I also note Fabric 1's support for this was actually buggy; it replaced the first tilde (good) but did not check whether the user was trying stuff like ~foo/bar (i.e. specifying a different user's home, which definitely doesn't appear possible without shell stuff). There's probably an open issue for that, which is sad because I don't see a solution for it either (besides raising an explicit error in that case).

I'm now actually real tempted to just put in a doc warning saying "do not use tildes! just use relative paths!". Having extra code means extra docs (more than just the warning's amount of text) and opportunities for bugs (like the v1 bug mentioned above).

Member

bitprophet commented Jul 23, 2018

Oh nice, I was living in the past, went to look at exactly what Fabric 1.x does now, and we replaced the echo $HOME or whatever with sftp.normalize(".") at some point (or I'm misremembering and the echo was elsewhere...)

Missed normalize on my initial skimming of Paramiko's SFTP APIs. This makes me happy!


On reflection though: this doesn't actually do anything "real"; it should behave exactly like removing the tilde in the first place! (Again, aside from use of that chdir method, which v1 never used and we currently don't use either.)

I also note Fabric 1's support for this was actually buggy; it replaced the first tilde (good) but did not check whether the user was trying stuff like ~foo/bar (i.e. specifying a different user's home, which definitely doesn't appear possible without shell stuff). There's probably an open issue for that, which is sad because I don't see a solution for it either (besides raising an explicit error in that case).

I'm now actually real tempted to just put in a doc warning saying "do not use tildes! just use relative paths!". Having extra code means extra docs (more than just the warning's amount of text) and opportunities for bugs (like the v1 bug mentioned above).

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 23, 2018

Member

Only devil's advocate reason I can think of for going through with tilde expansion is compatibility with local file paths (i.e. the "run a nontrivial task against localhost w/o actually using SSH" use case, #98). In that sense, local code is much more likely to be incidentally chdir'd someplace outside $HOME and thus benefits from ~/xxx.

But this seems pretty specious to me.

Member

bitprophet commented Jul 23, 2018

Only devil's advocate reason I can think of for going through with tilde expansion is compatibility with local file paths (i.e. the "run a nontrivial task against localhost w/o actually using SSH" use case, #98). In that sense, local code is much more likely to be incidentally chdir'd someplace outside $HOME and thus benefits from ~/xxx.

But this seems pretty specious to me.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Went with "just update the docs informing users to use relative paths only".

Should somebody come along with an amazing reason why I'm an idiot (meaning both a use case requiring tildes instead of relative paths, and a method of performing expansion which is lightweight enough to be worth using) we can always revisit.

Member

bitprophet commented Jul 24, 2018

Went with "just update the docs informing users to use relative paths only".

Should somebody come along with an amazing reason why I'm an idiot (meaning both a use case requiring tildes instead of relative paths, and a method of performing expansion which is lightweight enough to be worth using) we can always revisit.

@bitprophet bitprophet closed this Jul 24, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

@pint12 to be super clear so you don't have to read all the above: please try simply using temp/ instead of ~/temp/, if you hadn't already. Should work fine!

Member

bitprophet commented Jul 24, 2018

@pint12 to be super clear so you don't have to read all the above: please try simply using temp/ instead of ~/temp/, if you hadn't already. Should work fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment