sensu / sensu-go

Simple. Scalable. Multi-cloud monitoring.
https://sensu.io
MIT License
1.02k stars 176 forks source link

sensuctl fails to create check commands containing `&` #397

Closed portertech closed 6 years ago

portertech commented 6 years ago

sensuctl check create cannot create checks with commands containing &, e.g. echo output && exit 1. sensuctl ends up creating a check with a command string containing the unicode equivalent, e.g. echo output \u0026\u0026 exit 1.

Escaping & on input has no effect, e.g. echo output \&\& exit 1 -> echo output \\\u0026\\\u0026 exit 1.

portertech commented 6 years ago

sensuctl handler create also fails to create pipe handlers with commands containing >, e.g. cat \u003e /tmp/foobar.

jamesdphillips commented 6 years ago

@portertech interactive or flags? both?

portertech commented 6 years ago

@jamesdphillips so far, just interactive, I'll test flags 👍

portertech commented 6 years ago

@jamesdphillips also happens with flags :(

grepory commented 6 years ago

Let's see if this affects creation directly via the API (@portertech).

Otherwise, it's probably some kind of input encoding thing in the CLI.

mercul3s commented 6 years ago

@portertech where are you seeing the unicode after creating the check? Testing this locally - I added a few commands with |, && and > and did not see funny characters in sensuctl output.

portertech commented 6 years ago

@mercul3s sensuctl check list.

portertech commented 6 years ago

I am using zsh.

mercul3s commented 6 years ago

Ah, I am not using zsh and sensuctl check list does not contain unicode. Will take a look with zsh.

palourde commented 6 years ago

I don't think it has nothing to do with zsh but rather JSON encoding:

 $ sensuctl check list --format none
       Name               Command          Interval   Subscriptions   Handlers   Assets   Publish?
 ──────────────── ─────────────────────── ────────── ─────────────── ────────── ──────── ──────────
  check_unicode    echo output && exit 1         60   linux                               true
$ sensuctl check list --format json
[
  {
    "name": "check_unicode",
    "command": "echo output \u0026\u0026 exit 1",
    [...]
palourde commented 6 years ago

TIL you can't stop json.Marshal() from escaping these characters because the source code enforces it:

err := e.marshal(v, encOpts{escapeHTML: true})

It basically means we would have to use our own Marshaler... For example, we could rewrite the PrintJSON function like this (there's definitively a way to write this in a much cleaner way):

func PrintJSON(r interface{}, io io.Writer) error {
    buffer := &bytes.Buffer{}
    encoder := json.NewEncoder(buffer)
    encoder.SetEscapeHTML(false)

    if err := encoder.Encode(r); err != nil {
        return err
    }

    var prettyJSON bytes.Buffer
    if err := json.Indent(&prettyJSON, buffer.Bytes(), "", "  "); err != nil {
        return err
    }

    fmt.Fprintf(io, "%s\n", prettyJSON.Bytes())
    return nil
}

Which result in:

$ sensuctl check info check_unicode --format json
{
  "name": "check_unicode",
  "command": "echo output && exit 1",
  [...]

Sorry, I went down the rabbit hole, I'm out!

portertech commented 6 years ago

@palourde so this only effects the JSON output and not how the check operates within Sensu itself?

mercul3s commented 6 years ago

Looks like there is an encoder to override this functionality: https://github.com/golang/go/commit/ab52ad894f453a02153fb2bc106d08c47ba643b6

palourde commented 6 years ago

@mercul3s That's actually what I'm using, if you look at the snippet above, I'm using encoder.SetEscapeHTML(false)

@portertech That's actually a really good question.

Since we do a json.Marshal in the store (e.g. for checks), the following is stored in etcd:

/sensu.io/checks/default/default/check_unicode
{"name":"check_unicode","interval":60,"subscriptions":["linux"],"command":"echo output \u0026\u0026 exit 1","handlers":[],"runtime_assets":[],"environment":"default","organization":"default","publish":true}

However, since we encode the struct between components, it doesn't matter how we store it. Now, I'm not sure if the agent is able to properly run the command, definitively something to try! I think it would be fine since we don't dump any JSON when executing the command but I'm not 100% sure. Otherwise, we would have to use the same technique as above.

mercul3s commented 6 years ago

@palourde oh right, I missed that!