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

WorkflowEventBinding Parameter from json doesn't work with % symbol #7042

Closed
jrobertson-insite opened this issue Oct 22, 2021 · 3 comments
Closed
Assignees
Labels

Comments

@jrobertson-insite
Copy link
Contributor

jrobertson-insite commented Oct 22, 2021

Summary

In a WorkflowEventBinding Parameter, the value is passed through Go's fmt library more than once. This causes all of Go's fmt specifiers to be applied to the user specified input.

In my case when the parameter has a value of payload.sqlQuery, and payload.sqlQuery has a value of SELECT * FROM test WHERE Name LIKE '%MyName%' we see this value in the actual workflow submitted as SELECT * FROM test WHERE Name LIKE '%!M(MISSING)yName%!(NOVERB)'.

I'm unsure if this is intended or not. If you submit the same workflow through the UI with the same values you send in via json it gives the value without going through fmt twice and trying to parse the percent modifiers.

Example

WorkflowEventBinding Parameter Input (via json): %MyName%

Submitted Workflow Parameter Input (from WorkflowEventBinding)

actual: %!M(MISSING)yName%!(NOVERB)
expected: %MyName%

The real world use case for this is passing an sql query into a workflow event binding. Where we use the % symbol in the LIKE clause. To work around we're just manually encoding the percent symbol on the sender's side.

What version of Argo Workflows are you running?
v3.2.2

Diagnostics

See test cases on my fork

Or this can be used to reproduce locally.

apiVersion: argoproj.io/v1alpha1
kind: WorkflowEventBinding
metadata:
  name: test-binding
spec:
  event:
    selector: discriminator == "test"
  submit:
    workflowTemplateRef:
      name: someTemplate
    metadata:
    arguments:
      parameters:
      - name: myQuery
        valueFrom:
          event: payload.sqlQuery

JSON payload for event submitted to argo workflows

{
  "sqlQuery": "SELECT * FROM test WHERE Name LIKE '%MyName%'"
}

What executor are you running? Docker/K8SAPI/Kubelet/PNS/Emissary
Emissary

Logs from the workflow controller:

No relevant logs. Workflow is submitted with myQuery variable as
SELECT * FROM test WHERE Name LIKE '%!M(MISSING)yName%!(NOVERB)'
rather than what I expect
SELECT * FROM test WHERE Name LIKE '%MyName%'


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

@alexec
Copy link
Contributor

alexec commented Oct 23, 2021

This smells of injection vulnerability.

@alexec alexec added the type/security Security related label Oct 23, 2021
@alexec
Copy link
Contributor

alexec commented Oct 25, 2021

The fix for this, I think, is this:

func (i Item) Format(s fmt.State, _ rune) {
	_, _ = fmt.Fprintf(s, "%s", i.String()) //nolint
}

@jrobertson-insite you seem to have a good handle on the code and tests? Would you like to submit a PR?

@alexec alexec removed triage type/security Security related labels Oct 25, 2021
@alexec alexec removed their assignment Oct 25, 2021
@jrobertson-insite
Copy link
Contributor Author

Sure. I'll have a deeper look and submit a PR.

jrobertson-insite added a commit to jrobertson-insite/argo-workflows that referenced this issue Oct 28, 2021
…7042

Signed-off-by: Jonathan Robertson <jrobertson@insitesoft.com>
@alexec alexec closed this as completed in c5de76b Oct 28, 2021
@alexec alexec mentioned this issue Nov 5, 2021
25 tasks
@sarabala1979 sarabala1979 mentioned this issue Dec 15, 2021
73 tasks
sarabala1979 pushed a commit that referenced this issue Dec 15, 2021
)

Signed-off-by: Jonathan Robertson <jrobertson@insitesoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants