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

Added exec template function. #1931

Closed
wants to merge 1 commit into from

Conversation

sba1
Copy link

@sba1 sba1 commented Mar 7, 2016

A new template function called exec is added. This function can be used to
include the standard output of an external command. Commands that are
allowed to be executed must be listed within the newly introduced
execWhiteList setting.

This is very similar to PR #1548, especially the idea of the white list was
inspired by that request.


if !commandAccepted {
jww.ERROR.Printf("Executing %s is not allowed. Check execWhiteList settings.", str)
return "", nil
Copy link
Member

Choose a reason for hiding this comment

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

Return an error instead of logging it. Easier to test, for once.

Copy link
Author

Choose a reason for hiding this comment

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

  • if !commandAccepted {
  •    jww.ERROR.Printf("Executing %s is not allowed. Check
    
    execWhiteList settings.", str)
  •    return "", nil
    

Return an error instead of logging it.

Done. The test for the failure case looks a bit awkward now, let me know
if it can be improved (never did anything with Go before).

@sba1 sba1 force-pushed the exec-template-function branch 3 times, most recently from d3790d0 to 9e833ec Compare March 7, 2016 21:27
viper.Set("ExecWhitelist", []string{})

in := "{{exec \"echo\" \"test\"}}"
expectedErrSuffix := fmt.Sprintf("Executing echo is not allowed. Check execWhiteList settings.")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use fmt.Sprintf here.

A new template function called exec is added. This function can be used to
include the standard output of an external command. Commands that are
allowed to be executed must be listed within the newly introduced
execWhiteList setting.
@moorereason
Copy link
Contributor

lgtm

CircleCI is dumb.

@digitalcraftsman
Copy link
Member

Does this pull request replace #847?

@sba1
Copy link
Author

sba1 commented Mar 12, 2016

Am 2016-03-12 12:41, schrieb digitalcraftsman:

Does this pull request replace #847 [1]?

Seems so. At least in parts. It doesn't add the short code yet (though I
have this in my local branch).

@bep
Copy link
Member

bep commented Mar 12, 2016

As neither of these solves the fundamental security issue, it doesn't really matter.

@sba1
Copy link
Author

sba1 commented Mar 12, 2016

As neither of these solves the fundamental security issue, it doesn't
really matter.

In fact, I was not aware of PR #847 and the discussion when creating
this PR, however, as in the final version of this request (which was not
responded though), a white list config string for the global
configuration was introduced following your suggestion. This is also the
case in this PR as described in the doc and commit message. If this is
not correct (in fact my knowledge about Hugo internals is little) I'm
open for suggestions.

For instance, another possibility would be to add a new command line
parameter to hugo to enable this feature.

@bep
Copy link
Member

bep commented Mar 12, 2016

@digitalcraftsman suggested in another issue that we add a savetofile template func. Combine that with exec and you will have the perfect platform for sophisticated Trojan horses.

A whitelist and a --unsafe flag is the minimum here. But that is, in my eyes, still not enough. People make mistakes and suddenly the unsafe boolean is switched and the whitelist check could be bypassed by terminating the string with an emoji :-) ...

@digitalcraftsman
Copy link
Member

Especially in this combination I can understand your concerns.

@sba1
Copy link
Author

sba1 commented Mar 12, 2016 via email

@bep
Copy link
Member

bep commented Mar 12, 2016

Unless someone has some really enlightening to add, I suggest we close this and related issues/PRs -- and focus on what we can do. A cross-platform cat (file reader) would be a good start.

@sba1
Copy link
Author

sba1 commented Mar 12, 2016 via email

@bep
Copy link
Member

bep commented Mar 12, 2016

http://discuss.gohugo.io/ is a better place for discussions.

@bep
Copy link
Member

bep commented Jun 26, 2017

I'm closing this now. We may revisit this in the future. Maybe.

@bep bep closed this Jun 26, 2017
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants