Provide shell-escaped parameters for `script:`

[coryrc]

Feature request

Provide $(params.<name>.escaped) where the value has been escaped à la printf '%q' (see this or this.

It is currently impossible to safely or robustly use variable expansion safely in script:; i.e. imagine trying to do echo "$(params.name)" where someone made their name "$(rm -fR /).

Use case

Allow script:s to work robustly in the face of special characters.

[coryrc]

This could also be accomplished with the same error checking on Task creation if inputs were available like results i.e. /tekton/params/hash holds the contents of parameter var and the path is available via $(params.var.path).

[coryrc]

My current workaround is using env var:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: test-substitution-
spec:
  params:
    - name: in
      value: "\"`date`\""
  taskSpec:
    params:
      - name: in
    steps:
      - image: debian:buster
        env:
          - name: DANGER
            value: $(params.in)
        # The first line runs date
        # The second does not
        script: |
          echo "$(params.in)"
          echo "${DANGER}"

[bobcatfish]

Hey @coryrc ! I'm wondering what we would do in the case where someone DOES want to allow passing arbitrary commands into the script.

I wonder if your env var solution is the most reasonable - another thing that comes to mind is if we allowed Tasks to express validation (kinda related to #1393)

@ImJasonH might have some opinions here

[imjasonh]

Hey @coryrc ! I'm wondering what we would do in the case where someone DOES want to allow passing arbitrary commands into the script.

In that case the Task author can use the default, $(params.foo), instead of $(params.foo.escaped), if I'm understanding correctly.

The downside there is that it requires the Task author to remember to ask for .escaped, and it could lead to confusion about what kind of escaping it is -- URL-encoding? SQL escaping?

I'm mostly +1 toward it, with good documentation and examples.

[coryrc]

In that case the Task author can use the default, $(params.foo), instead of $(params.foo.escaped), if I'm understanding correctly.

Yes

The downside there is that it requires the Task author to remember to ask for .escaped, and it could lead to confusion about what kind of escaping it is -- URL-encoding? SQL escaping?

I'm mostly +1 toward it, with good documentation and examples.

Is this a change that I should make a TEP for?

[coryrc]

And "shell-escaped" might be a better name

[imjasonh]

Is this a change that I should make a TEP for?

Yeah, I think so, if only to get broader eyes/feedback on it than trying to ping people into this thread.

It's fairly straightforward, so the TEP shouldn't be too long. A few motivating use cases like you have up there ☝️ and a word or two about how you'd shell-escape. Let me know if I can help, and/or assign me when you share the TEP.

[skaegi]

In your example you can maybe just use single quotes if you want to make sure you get the literal value:

script: |
          echo "$(params.in)"
          echo "${DANGER}"
          echo '$(params.in)'

Although the syntax looks like shell variable expansion it really is just doing a direct and very simple string substitution. We already have teams doing script in node and python which will have different encoding rules so I'm a bit hesitant to jump on the +1 wagon here.

[coryrc]

In your example you can maybe just use single quotes if you want to make sure you get the literal value:

script: |
          echo "$(params.in)"
          echo "${DANGER}"
          echo '$(params.in)'

This will break if params.in is '; rm -fR /;'

[skaegi]

Sorry @coryrc, I just saw this and well... ouch. I wonder if there is a generic alternative to $(params.<name>.escaped) -- fwiw I am now onboard and trying to figure out the best solution. We can continue at ... https://github.com/tektoncd/community/pull/208

[tekton-robot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[bobcatfish]

Judging by our discussions around TEP-0012 which proposed this, I think it makes sense to close this issue for now (see https://github.com/tektoncd/community/pull/208#issuecomment-707242339)