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

SFTP transfer crashes on paths with "%" characters #1348

Closed
natecode opened this issue Jun 29, 2015 · 6 comments
Closed

SFTP transfer crashes on paths with "%" characters #1348

natecode opened this issue Jun 29, 2015 · 6 comments

Comments

@natecode
Copy link

When transferring files from a host with get(), an exception is triggered if any path has a "%" code in it.

Fatal error: get() encountered an exception while downloading
Underlying exception:
    %d format: a number is required, not dict

To reproduce, create a directory on the remote host with % in the file:

ssh remote 'mkdir -p top/%a; touch top/%a/%d'
mkdir out

The run the following script to try to retrieve this dir:

from fabric.api import settings, get
with settings:
   get('top/*', 'out')

The issue is in the % expansion within sftp, where it passes a dict of args before calling abspath() in sftp.py:

        local_path = os.path.abspath(escaped_path % path_vars )
@natecode
Copy link
Author

This happens in all versions of Fabric that I've tried. The only workaround I found was to disable % expansion in this code path. Rather than try to escape %'s in the input, it's probably better to move to str.format(), which is a little more easy to work with.

@bitprophet
Copy link
Member

Is there a reason doing normal %-formatting escaping (using double-percents, %%, IIRC) won't suffice here?

@natecode
Copy link
Author

natecode commented Jul 4, 2015

Because when doing a recursive get() from a remote directory, you can't know what's under the directory without listing its contents first?

This can happen if there is any name with %d in it at an arbitrary depth from the root dir you're transferring. This is something Fabric should definitely handle for the user since recursively listing and then %-escaping each file entry and passing it to get() individually seems like a poor user experience.

bspink added a commit to bspink/fabric that referenced this issue Aug 5, 2015
@bspink
Copy link

bspink commented Aug 5, 2015

The patch I wrote didn't change to using the format method for strings as mentioned above, as that would break functionality for anyone with any of the specially-handled formatted strings. I just updated the existing regex to be more specific and escape anything except for the aforementioned special strings.

@natecode
Copy link
Author

natecode commented Aug 5, 2015

@bspink Thank you, I tested this patch and it fixes the case I listed in the issue. I look forward to updating when this is next released.

Cheers!

@bitprophet
Copy link
Member

Rolling into the PR! edit: which is #1361, no auto-link was created, thx GH

bitprophet pushed a commit that referenced this issue Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants