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

feat: insert -- between RSH and rsync commands #656

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheoTechnicguy
Copy link

@TheoTechnicguy TheoTechnicguy commented Oct 28, 2024

Hi there!

I ran into some minor issue whilst tinkering with rsync and a new remote shell program. I noticed that rsync does not add the now common -- indicating the end of parsable arguments to the program between the RSH flags and rsync command.

Effectively, this changes the generated command from

ssh [opts] <destination> rsync [opts] src dst

to

ssh [opts] <destination> -- rsync [opts] src dst

To the best of my knowledge, this practice is a POSIX-compatible convention, in use by virtually all applications in my daily use.

I have tested this change with the test scripts (2 tests skipped: crtimes and protected-regular) and with the current SSH implementation (OpenSSH_9.9p1, OpenSSL 3.3.2 3 Sep 2024; I can assert, however, that I have used this practice for 3+ years with OpenSSH). As such, I do not believe there to be any significant impact; nevertheless, I would be very appreciative of input about situations I did not think about. Do you know any other commonly used remote shells I should test?

Furthermore, I was unable to find contributing guidance, so please do not hesitate to indicate code formatting, commenting or other requirements I did not follow.

Cheers and many thanks for a utility I have had the pleasure of using for years (and will continue to)!


Edit: I did some digging and traced back the origin of the -- terminator.

Nowadays, CLI program options not always support single-dash syntax and
do not require flags to be set before positional arguments. To ensure
that future remote shells do not interpret rsync flags, this commit
inserts a double dash (`--`) before the transmitted rsync command.
Copy link
Contributor

@charmitro charmitro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is safe to merge. IMHO It protects against future remote shell updates.

Before patch:

$ ./rsync/rsync -av --verbose notes [email protected]:~/
opening connection using: ssh -l charmitro rpi.local rsync --server -vvlogDtpre.iLsfxCIvu . "~/"  (9 args)

After patch:

$ ./rsync/rsync -av --verbose notes [email protected]:~/
opening connection using: ssh -l charmitro rpi.local -- rsync --server -vvlogDtpre.iLsfxCIvu . "~/"  (10 args)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants