Skip to content

Fix SSH command escaping #633

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

Merged
merged 1 commit into from
Feb 7, 2022
Merged

Fix SSH command escaping #633

merged 1 commit into from
Feb 7, 2022

Conversation

buu700
Copy link
Contributor

@buu700 buu700 commented Feb 6, 2022

Addresses #632.

I haven't tested very thoroughly, and may be making some incorrect assumptions about how shellescape works, but I can confirm that this at least fixes the lima bash -c "$(echo -e '\n\techo balls\n')" example for me.

Copy link
Member

@jandubois jandubois 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 not sufficient, as it doesn't deal with commands that include quotes. E.g. it breaks:

$ lima bash -c 'echo "foo bar"'
foo bar

With the current patch is breaks like this:

$ lima bash -c 'echo "foo bar"'
bar': -c: line 1: unexpected EOF while looking for matching `''
bar': -c: line 2: syntax error: unexpected end of file

@buu700
Copy link
Contributor Author

buu700 commented Feb 6, 2022

All right, should be good now. Thanks @jandubois!

As an aside, both of these examples would be good tests to add for the future.

jandubois
jandubois previously approved these changes Feb 6, 2022
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

LGTM, but since you volunteered to add tests (😄 ), maybe add them to this PR?

@buu700
Copy link
Contributor Author

buu700 commented Feb 6, 2022

lol, all right, done. I confirmed that the new tests currently pass, and each one also fails as expected with the two previous versions of shell.go.

Edit: Fixed the tests (initially wrote them as unit tests by mistake).

@buu700 buu700 force-pushed the master branch 2 times, most recently from d90b8e1 to 3198780 Compare February 6, 2022 20:26
@AkihiroSuda
Copy link
Member

Thanks, but please sign the commit for DCO
https://github.com/apps/dco

(run git commit -a -s --amend, and make sure that the Signed-off-by: NAME <EMAIL> line with your real name is included in the commit message)

@buu700
Copy link
Contributor Author

buu700 commented Feb 7, 2022

Done.

@AkihiroSuda AkihiroSuda added this to the v0.8.3 milestone Feb 7, 2022
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks!

@jandubois jandubois merged commit ec77f97 into lima-vm:master Feb 7, 2022
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.

lima bash -c "$(echo -e '\n\techo balls\n')" doesn't work as expected
3 participants