File name escaping seems to fail

[AntJanus]

I'm running into this issue and can't seem to find a solution. When setting a source or destination that has spaces in it, it causes all kinds of issues and for a variety of reasons:

1. When entering a name with spaces, the spaces are escaped but the entire string isn't put into quotes. Simply escaping the name doesn't work. There's an SO question on this
2. When entering a name with spaces and trying to manually quote it, the quotes get escaped (which makes sense but doesn't offer a quick workaround)

[AntJanus]

I created a fork and it's far from perfect but here are a few situations it fails (and node-rsync currently fails):

1. when transferring at any point and escaping, there need to be quotes
2. when transferring from remote to local, escaping local paths can cause issues, escaping remote can cause issues
3. when transferring from local to remote, remote has to be escaped.

I'm currently doing this:

1. escape all paths unless it contains @ which designates it as remote
2. when going from local to remote, using the s argument.

It'd be great to be able to override the default escaping mechanism (such as by specifying a "true/false" flag in the function) and having "escape and quote all" default.

[AntJanus]

Okay, so I haven't been able to figure it out..at all. The results I'm getting are rather inconsistent. Anyways, I'd love some help on this. I don't mind writing the PR or implementation but how is escaping supposed to work? I read that you're supposed to quote AND escape the name like so:

'file\ name\ with\ space.pdf'

But this morning, my version of rsync ended up uploading a file with the slashes included.

// CC @mattijs

[mattijs]

Thank you for taking the time to try and figure this out. I'm not quite sure myself what the best solution is here. I did a quick test with a file and directory with spaces in them. It seems to work fine when the entire source and/or destination are quoted and have the spaces escaped, like so:

$ rsync "a\ text.file" "[email protected]:b\ dir"

Testing this is going to be hard cause it might also depend on the shell the command is executed in (I did not include that in my small test). Some work has to be done to figure out proper test cases that are know to work. These test cases will probably have to be figured out manually but running a bunch of commands on a machine.

[AntJanus]

So here's what I found out over the past few months. First: the PR I opened has been working without a hitch since the last commit. I'm using the repo to transfer files uploaded from Heroku to another server (and transfer them back). I'm dealing with a fair number of files in all kinds of different naming formats.

Here's the sample command I'm using to retrieve files:

var rsync = new Rsync()
          .shell(`ssh -o StrictHostKeyChecking=no -i ${sshKeyPath}`)
          .flags('az')
          .set('remove-source-files')
          .source([`${remoteUser}@${currentEnv.dataServer}:${cssPath}`, `${remoteUser}@${currentEnv.dataServer}:${htmlPath}`])
          .destination('/tmp/')
        ;

and to save them:

   var rsync = new Rsync()
      .shell(`ssh -o StrictHostKeyChecking=no -i ${process.cwd()}/provisioning/files/ssh/id_rsa`)
      .flags('azs')
      .source(`${req.files.file.path}`) //upload path of a file
      .destination(`${remoteUser}@${envConfig.dataServer}:${app.locals.path)}`)
    ;

I never truly got it to work so right now, I have to post-process the files that I upload so that they have a nice name. Otherwise, I can either setup the escaping to allow me to retrieve files or upload them but I can't do both reliably with the same code. I kept getting into a situation where escaping one way worked for uploading to a remote server but downloading made it fail and vice-versa.

There's some weird thing with escaping where you escape one way for src vs. dest and another way local vs remote. Each combination of the two produced a failure effect.

To explain the PR:

var match = filename.match('@'); // match to see if the server is remote

//if it is remote, escape everything
if (match && match.length > 0) {
   //escape quotes, backticks, apostrophes, $, &, and bunch of other stuff because people use those in EVERYTHING for some reason.
   return "'" + filename.replace(/(["'`\s\\\(\)\\$\\&])/g,'\\$1') + "'";
}

// if it is not remote, simply quote it because that works
return "'" + filename + "'";
```

Another major caveat to this PR (other than not reliably escaping for that weird grid of `src`/`dest`/`local`/`remote` combination), is that variable expansion essentially doesn't work due to the use of quotes.

[mattijs]

Thanks for the detailed explanation.

The thing I don't like about the current solution is that it relies on the @ character in the string. I'd rather have a single way of treating each file path used in source or destination. This basically comes down to quoting all files, not just the ones containing an @ character.

Like you pointed out shell expansion does not work when quoting the file names. I wanted to hold on to this for convenience but I'm starting to get convinced that this is just not a good idea. We are constructing the command from JavaScript anyways so why not do file lookups from JavaScript too (through glob for example). Also #40 kind of killed escaping already (partially).

I kept getting into a situation where escaping one way worked for uploading to a remote server but downloading made it fail and vice-versa.

This is odd. Could you provide a reproducible test case for this (using local files) so we can see if we can determine how this should work.

There's some weird thing with escaping where you escape one way for src vs. dest and another way local vs remote. Each combination of the two produced a failure effect.

I don't fully get this one but I think this should be solved by just always escaping file names with quotes. In order to do this escapeFileArg should be modified but beware that this is also used for include/exclude patterns. Some of the tests expected output should be changed in order to accommodate the changes. I would be nice if you could keep #37 in mind and maybe even get a little start with that.

[jasonschmedes]

I got around this by quoting the remote path myself.

For example:

const rsync = new Rsync()
    .set('rsh', 'ssh -p22')
    .source(`${username}@${host}:"${source}"`)
    .destination(destination)

[AntJanus]

Awesome!

I still use my snippet up above in production lol. And it works!