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 at.present #28187

Merged
merged 1 commit into from Oct 29, 2015
Merged

fix at.present #28187

merged 1 commit into from Oct 29, 2015

Conversation

sjansen
Copy link
Contributor

@sjansen sjansen commented Oct 21, 2015

Because cmd contains a pipe, it must be run by a real shell.

Because cmd contains a pipe, it must be run by a real shell.
@cachedout
Copy link
Contributor

You are correct here that we need to set the python_shell flag for this pipe to be processed correctly. However, we need to also ensure that the command in question is first safe to run on the shell and doesn't contain other shell-isms which might do unexpected sneaky things. There are a number of examples for sanitizing input using shlex.split in other modules. Let me know how that sounds. Thanks!

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Oct 22, 2015
@jfindlay
Copy link
Contributor

@sjansen, how would you feel about instead changing

if tag:
    cmd = '{0} "### SALT: {4}\n{1}" | {2} {3}'.format(echo_cmd,
        job, binary, timespec, tag)
else:
    cmd = '{0} "{1}" | {2} {3}'.format(echo_cmd, name, binary, timespec)
...
ret['comment'] = __salt__['cmd.run']('{0}'.format(cmd), runas=user)

to

if tag:
    stdin = '### SALT: {0}\n{1}'.format(tag, job)
else:
    stdin = name
cmd = '{0} {1}'.format(binary, timespec)
...
ret['comment'] = __salt__['cmd.run'](cmd, stdin=stdin, runas=user)

for example?

@jfindlay jfindlay added Minor Change Platform Relates to OS, containers, platform-based utilities like FS, system based apps State-Module labels Oct 23, 2015
@sjansen
Copy link
Contributor Author

sjansen commented Oct 24, 2015

Nice! Much better.

@cachedout
Copy link
Contributor

@sjansen Are you going to incorporate the suggestions from @jfindlay ? If so, please let me know when this is ready to go. Thanks!

cachedout pushed a commit that referenced this pull request Oct 29, 2015
@cachedout cachedout merged commit 4304001 into saltstack:2015.8 Oct 29, 2015
@cachedout
Copy link
Contributor

@jfindlay I have merged this. If you would like to follow-up with additional changes, please feel free.

@sjansen Sorry for the delay here. We've got this in now. Thanks much.

jfindlay added a commit to jfindlay/salt that referenced this pull request Nov 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged Platform Relates to OS, containers, platform-based utilities like FS, system based apps State-Module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants