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

Evaluating template in workflow replaced << with &lt;< #415

Closed
invidian opened this issue Jan 12, 2021 · 2 comments · Fixed by #416
Closed

Evaluating template in workflow replaced << with &lt;< #415

invidian opened this issue Jan 12, 2021 · 2 comments · Fixed by #416
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@invidian
Copy link
Contributor

invidian commented Jan 12, 2021

Expected Behaviour

Workflow properly renders template without replacing any characters.

Current Behaviour

When template includes sequence like <<, it gets replaced into `<<.

Possible Solution

Use text/template instead of html/template when rendering workflows.

Steps to Reproduce (for bugs)

Unit test to reproduce:

func TestRenderTemplateNotHTML(t *testing.T) {
        validTemplate := `
version: "0.1"
name: hello_world_workflow
global_timeout: 600
tasks:
  - name: "hello world<<"
    worker: "{{.device_1}}"
    actions:
    - name: "hello_world"
      image: hello-world
      timeout: 60
`
        templateID := uuid.New().String()
        hwAddress := []byte("{\"device_1\":\"08:00:27:00:00:01\"}")

        result, err := RenderTemplate(templateID, validTemplate, hwAddress)
        if err != nil {
                t.Fatalf("rendering should not fail")
        }

        expected := "hello world<<"

        if !strings.Contains(result, expected) {
                t.Fatalf("Result should contain %q, got \n\n%s", expected, result)
        }
}

Context

https://twitter.com/_invidian/status/1339504259340091392

Found while working on tinkerbell/cluster-api-provider-tinkerbell#17.

Your Environment

Tinkerbell sandbox from version tinkerbell/playground@be40a7b.

@gianarb gianarb added the kind/bug Categorizes issue or PR as related to a bug. label Jan 12, 2021
@gianarb
Copy link
Contributor

gianarb commented Jan 12, 2021

I agree with John Arundel https://twitter.com/bitfield/status/1339506837230632960 is not a bug it is a security-conscious decision.

I am joking thank you for opening this issue

@gianarb gianarb self-assigned this Jan 12, 2021
@invidian
Copy link
Contributor Author

Added sample unit test to reproduce. Perhaps one in the code could be more comprehensive.

@mergify mergify bot closed this as completed in #416 Jan 15, 2021
mergify bot added a commit that referenced this issue Jan 15, 2021
## Description

Apparently, we imported the wrong package, we need to use text no HTML rules.

## Why is this needed

Wrong import, we do not need to escape following HTML rules.
Fixes: #415 

## How Has This Been Tested?
 Unit test


## Checklist:

I have:

- [x] updated the documentation and/or roadmap (if required)
- [x] added unit or e2e tests
- [x] provided instructions on how to upgrade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants