Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

[WIP] Hardening the interpolation of parameters containing special characters #587

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions e2e/commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,17 @@ func testRenderApp(appPath string, env ...string) func(*testing.T) {
}
}

func TestRenderParametersWithSpecialChars(t *testing.T) {
cmd, cleanup := dockerCli.createTestCmd()
defer cleanup()
dir := fs.NewDir(t, "")
defer dir.Remove()

appPath := filepath.Join("testdata", "parameters-with-special-chars")
cmd.Command = dockerCli.Command("app", "render", filepath.Join(appPath, "my.dockerapp"))
result := icmd.RunCmd(cmd).Assert(t, icmd.Success)
golden.Assert(t, result.Stdout(), filepath.Join("parameters-with-special-chars", "expected.golden"))
}
func TestRenderFormatters(t *testing.T) {
cmd, cleanup := dockerCli.createTestCmd()
defer cleanup()
Expand Down
9 changes: 9 additions & 0 deletions e2e/testdata/parameters-with-special-chars/expected.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: "3.0"
services:
test:
command:
- echo
- brave
- gnu
- world
image: alpine:latest
16 changes: 16 additions & 0 deletions e2e/testdata/parameters-with-special-chars/my.dockerapp
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
version: 0.1.0
name: myapp
maintainers:
- name: dev
email: [email protected]
---
version: "3.0"
services:
test:
image: alpine:latest
command: echo ${texts.first} ${texts.second-1} ${texts.second-2}
---
texts:
first: "brave"
second-1: "gnu"
second-2: "world"
12 changes: 10 additions & 2 deletions internal/compose/compose.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,25 @@ import (
)

const (
delimiter = "\\$"
substitution = "[_a-z][._a-z0-9]*(?::?[-?][^}]*)?"
delimiter = "\\$"
substitution = "[_a-z][._a-z0-9-]*"
composeSubstitution = "[_a-z][._a-z0-9]*(?::?[-?][^}]*)?"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shoud we add the . here? The cli doesn't have it, I'm not sure we really need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a true question. I added the . to make this test pass https://github.com/docker/app/blob/8d52f6d458163e8d5cac6147f6cb4c54092168af/internal/packager/init_test.go#L47
But I'm not sure if the tested usecase is legit

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the test is bogus since env variables can only be alphanumeric and _.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dots are necessary for the indentation levels in the parameters yaml file.
As in: https://github.com/docker/app/blob/8d52f6d458163e8d5cac6147f6cb4c54092168af/internal/packager/init_test.go#L75-L78

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but here we are talking about variable substitution that the cli does, it can substitute an environment variable inside a compose file (using ${VARIABLE:-default_value} notation. And an environment variable can only have [a-zA-Z_]+. No dots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test here is verifying the conversion of the valid compose file into a dockerapp file. But the compose file syntax does not support the dot notation.

Copy link
Contributor

Choose a reason for hiding this comment

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

So one could say that it’s not a valid compose file :)

)

var (
patternString = fmt.Sprintf(
"%s(?i:(?P<escaped>%s)|(?P<named>%s)|{(?P<braced>%s)}|(?P<invalid>))",
delimiter, delimiter, substitution, substitution,
)
composePatternString = fmt.Sprintf(
"%s(?i:(?P<escaped>%s)|(?P<named>%s)|{(?P<braced>%s)}|(?P<invalid>))",
delimiter, delimiter, composeSubstitution, composeSubstitution,
)
// ExtrapolationPattern is the variable regexp pattern used to interpolate or extract variables when rendering
ExtrapolationPattern = regexp.MustCompile(patternString)
// ComposeExtrapolationPattern is the compose original variable regexp pattern used to interpolate or extract
// variables when rendering. This pattern is only used when a docherapp file is initialized from a compose file
ComposeExtrapolationPattern = regexp.MustCompile(composePatternString)
)

// Load applies the specified function when loading a slice of compose data
Expand Down
2 changes: 1 addition & 1 deletion internal/packager/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func initFromComposeFile(name string, composeFile string) error {
}
}
}
vars, err := compose.ExtractVariables(composeRaw, compose.ExtrapolationPattern)
vars, err := compose.ExtractVariables(composeRaw, compose.ComposeExtrapolationPattern)
if err != nil {
return errors.Wrap(err, "failed to parse compose file")
}
Expand Down
1 change: 0 additions & 1 deletion internal/packager/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ services:
image: image1
ports:
- ${ports.service1:-9001}

service2:
image: image2
ports:
Expand Down