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

Fix windows cmd quoting #666

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Conversation

jsturtevant
Copy link
Contributor

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description of change

In #651 we found that quoting was not working properly. The following would fail:

 wrap a powershell with quotes - expect 0 because travis does not restrict anonymous logins:
    exec: powershell -noprofile -noninteractive -command "(get-itemproperty -path 'HKLM:/SYSTEM/CurrentControlSet/Control/Lsa/').restrictanonymous"

Not using quotes worked for that use case but for more complex scenarios quoting is necessary. Digging into it a bit more I found https://golang.org/pkg/os/exec/#Command:

On Windows, processes receive the whole command line as a single string and do their own parsing. Command combines and quotes Args into a command line string with an algorithm compatible with applications using CommandLineToArgvW (which is the most common way). Notable exceptions are msiexec.exe and cmd.exe (and thus, all batch files), which have a different unquoting algorithm. In these or other similar cases, you can do the quoting yourself and provide the full command line in SysProcAttr.CmdLine, leaving Args empty.

This PR uses the second approach.

cc: @petemounce

@jsturtevant
Copy link
Contributor Author

the codeclimate issue is: exported function NewCommandForWindowsCmd should have comment or be unexported

The NewCommand was missing a comment and I copied that style. Should I update the new function with a comment?

Copy link
Collaborator

@petemounce petemounce left a comment

Choose a reason for hiding this comment

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

Great find, and I really appreciate the revisit!

@aelsabbahy
Copy link
Member

@petemounce is this good to go? I might cut a release tomorrow.. so if this is ready I can merge it prior.

Awesome work tracking this down @jsturtevant !

@petemounce
Copy link
Collaborator

@aelsabbahy yes, I think this looks good.

@jsturtevant
Copy link
Contributor Author

getting into the next release would be awesome. Do I need to address the codeclimate error?

@aelsabbahy
Copy link
Member

If you want to address it, you can. Honestly, the code is littered with that error and I need to clean it up across the board one day.. I won't hold you to a standard I haven't implemented myself =)

@aelsabbahy
Copy link
Member

aelsabbahy commented Dec 18, 2020

Merging, so I can cut a release with this and other changes.

Thanks again for spending the time to track this down!

@aelsabbahy aelsabbahy merged commit 6e5d3e3 into goss-org:master Dec 18, 2020
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.

3 participants