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

win_shell - fix space escaping for custom executable #57455

Merged
merged 1 commit into from Jun 16, 2019

Conversation

Projects
None yet
5 participants
@jborean93
Copy link
Contributor

commented Jun 6, 2019

SUMMARY

Currently if you use win_shell with an executable that contains a space in the path it will fail because we don't quote the value. This PR simply quotes all values as there is no harm doing that even when it isn't needed.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_shell

@ansibot

This comment has been minimized.

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

What happens if the value already has double quotes? won't they close and open the ones you added and thus make the value not quoted?

@ansibot ansibot removed the needs_triage label Jun 6, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@ShachafGoldstein I thought that would be an issue but because the type of the execution option is path it will actually fail out saying the path is invalid. That means that right now you cannot even quote the path to escape spaces. We also check if the path ends with .exe at https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_shell.ps1#L90 so if the path was quoted then it would be "C:\path".exe which probably is another issue in itself.

I could add the ArgvParser util but it's just extra code that is not really necessary in this case.

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

shipit

@Sephtex

This comment has been minimized.

Copy link

commented Jun 6, 2019

Tested the change, and it works as expected now, thanks @jborean93 for the fast solution and feedback!
@ShachafGoldstein even if the path is quotted (single/double) it isn't any problem, as it's potentially the value of yaml that is passed, or you'll need to do something like "'{{ path }}'", but that would be quite invalid and wouldn't even work as you'll get an error than an invalid path has been specified. And this is what we would even expect.

@ansibot ansibot added shipit and removed core_review labels Jun 6, 2019

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@jborean93 jborean93 merged commit d0c4914 into ansible:devel Jun 16, 2019

1 check passed

Shippable Run 126285 status is SUCCESS.
Details

@jborean93 jborean93 deleted the jborean93:win_shell-space branch Jun 16, 2019

jborean93 added a commit to jborean93/ansible that referenced this pull request Jun 16, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

Backport PR to stable-2.8 #57922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.