[WIP] Hardening the interpolation of parameters containing special characters#587
[WIP] Hardening the interpolation of parameters containing special characters#587jcsirot wants to merge 1 commit intodocker-archive-public:masterfrom
Conversation
internal/packager/init.go
Outdated
| } | ||
| vars, err := compose.ExtractVariables(composeRaw, compose.ExtrapolationPattern) | ||
| vars, err := compose.ExtractVariables(composeRaw, compose.ComposeExtrapolationPattern) | ||
| fmt.Println(vars) |
…. and -) not allowed by the original compose file format more robust. Docker App is now using 2 different regexps when parsing a .dockerapp file and processing a compose file to generate a .dockerapp Signed-off-by: Jean-Christophe Sirot <jean-christophe.sirot@docker.com>
jcsirot
force-pushed
the
fix-var-substitution-regex
branch
from
164e4d0 to
72b27fc
Compare
Codecov Report
@@ Coverage Diff @@
## master #587 +/- ##
=========================================
+ Coverage 70.74% 72.55% +1.8%
=========================================
Files 55 54 -1
Lines 3247 2827 -420
=========================================
- Hits 2297 2051 -246
+ Misses 649 511 -138
+ Partials 301 265 -36
Continue to review full report at Codecov.
|
| substitution = "[_a-z][._a-z0-9]*(?::?[-?][^}]*)?" | ||
| delimiter = "\\$" | ||
| substitution = "[_a-z][._a-z0-9-]*" | ||
| composeSubstitution = "[_a-z][._a-z0-9]*(?::?[-?][^}]*)?" |
There was a problem hiding this comment.
Shoud we add the . here? The cli doesn't have it, I'm not sure we really need it
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think the test is bogus since env variables can only be alphanumeric and _.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So one could say that it’s not a valid compose file :)
4 participants
- What I did
Fixes #574
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
TBD