fix(publish): flag literal inline environment values#13760
fix(publish): flag literal inline environment values#13760
Conversation
PR #13402 removed the inline-env check entirely, so literal hardcoded values like `MYSQL_ROOT_PASSWORD: toto` now silently leak into the OCI artifact. Restore the check, scoped to literal values only — `${VAR}` and `$VAR` remain symbolic in the published YAML and still pass without `--with-env`, preserving the legitimate fix from #13402. The walk follows the same files processFile actually serializes (top-level compose files plus local extends parents), so inherited literals and unrelated services in extends parents are covered too. Errors aggregate per service. With `--with-env` the walk is skipped entirely since the user has opted in. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Restores compose publish pre-checks to prevent hardcoded inline environment values from being embedded into the published OCI artifact, while allowing symbolic/interpolated values ($VAR, ${VAR}) to remain in the published YAML.
Changes:
- Reintroduces environment publishing validation by walking the compose files that are actually serialized (top-level + local
extendsparents) and aggregating errors per service. - Adds unit and e2e coverage for literal vs interpolated environment handling (and extends inheritance cases).
- Refactors compose-file loading used for inspection/serialization to share a common “unresolved” loader configuration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/compose/publish.go | Implements new walk/aggregation logic for env/env_file findings; introduces interpolation detection and shared loader helper. |
| pkg/compose/publish_test.go | Adds unit tests covering literal/interpolated env values, extends inheritance, and aggregation behavior. |
| pkg/e2e/publish_test.go | Adds e2e coverage for literal inline environment: rejection and interpolated env publish success. |
| pkg/e2e/fixtures/publish/compose-interpolated-env.yml | New fixture composing an interpolated environment value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *composeService) checkEnvironmentVariables(ctx context.Context, project *types.Project, options api.PublishOptions) error { | ||
| if options.WithEnvironment || len(project.ComposeFiles) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| services, configsWithLiteralContent, err := collectEnvironmentFindings(ctx, project) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| lines := buildEnvironmentErrorLines(services, configsWithLiteralContent) | ||
| if len(lines) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| var msg strings.Builder | ||
| for _, l := range lines { | ||
| fmt.Fprintf(&msg, "%s\n", l) | ||
| } | ||
| msg.WriteString("To avoid leaking sensitive data, you must either explicitly allow the sending of environment variables by using the --with-env flag,\n") | ||
| msg.WriteString("or remove sensitive data from your Compose configuration") | ||
| return errors.New(msg.String()) | ||
| } |
There was a problem hiding this comment.
checkEnvironmentVariables is now also flagging literal configs.*.content, but the error text and the --with-env bypass are specifically about publishing environment variables (see CLI flag description). This makes --with-env unintentionally allow publishing literal config content, and the guidance in the error message is misleading for config findings. Consider removing config.content from this check or splitting it into a separate check with its own opt-in flag/message (or always enforcing it regardless of WithEnvironment).
| // ${VAR:-default}, $VAR, $_VAR. A `$$` escape is treated as containing | ||
| // interpolation (the second `$` followed by a name still matches), which | ||
| // keeps the heuristic simple and conservatively skips the value. | ||
| var interpolationPattern = regexp.MustCompile(`\$([A-Za-z_]\w*|\{[^}]+\})`) | ||
|
|
||
| func containsInterpolation(value string) bool { | ||
| return interpolationPattern.MatchString(value) |
There was a problem hiding this comment.
The containsInterpolation heuristic treats $$-escaped dollars as interpolation (because $ followed by a name still matches). In Compose, $$ is the escape sequence to produce a literal $, so values like pa$$word or $$SECRET are literal and will be published as-is, but they will currently not be flagged. Consider updating the interpolation detection to ignore $ tokens that are escaped by a preceding $ (Go regex has no lookbehind, so this likely needs a small manual scan around matches).
| // ${VAR:-default}, $VAR, $_VAR. A `$$` escape is treated as containing | |
| // interpolation (the second `$` followed by a name still matches), which | |
| // keeps the heuristic simple and conservatively skips the value. | |
| var interpolationPattern = regexp.MustCompile(`\$([A-Za-z_]\w*|\{[^}]+\})`) | |
| func containsInterpolation(value string) bool { | |
| return interpolationPattern.MatchString(value) | |
| // ${VAR:-default}, $VAR, $_VAR. `$$` is a Compose escape for a literal `$`, | |
| // so containsInterpolation filters out regex matches whose `$` is itself | |
| // escaped by a preceding `$`. | |
| var interpolationPattern = regexp.MustCompile(`\$([A-Za-z_]\w*|\{[^}]+\})`) | |
| func containsInterpolation(value string) bool { | |
| matches := interpolationPattern.FindAllStringIndex(value, -1) | |
| for _, match := range matches { | |
| start := match[0] | |
| if start == 0 || value[start-1] != '$' { | |
| return true | |
| } | |
| } | |
| return false |
| name: "config.environment is not flagged (it is a var name, not a value)", | ||
| composeYAML: `name: test | ||
| services: | ||
| serviceA: | ||
| image: alpine:3.12 | ||
| configs: | ||
| myconfig: | ||
| environment: HARDCODED | ||
| `, | ||
| withEnv: false, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "config.content literal is flagged", | ||
| composeYAML: `name: test | ||
| services: | ||
| serviceA: | ||
| image: alpine:3.12 | ||
| configs: | ||
| myconfig: | ||
| content: | | ||
| api_key=plaintext-secret | ||
| `, | ||
| withEnv: false, | ||
| wantErr: true, | ||
| wantErrContains: []string{`has literal inline content.`}, | ||
| }, | ||
| { | ||
| name: "config.content interpolated is not flagged", | ||
| composeYAML: `name: test | ||
| services: | ||
| serviceA: | ||
| image: alpine:3.12 | ||
| configs: | ||
| myconfig: | ||
| content: "key=${SECRET}" | ||
| `, | ||
| withEnv: false, | ||
| wantErr: false, | ||
| }, |
There was a problem hiding this comment.
These unit tests assert that literal configs.*.content should be rejected by checkEnvironmentVariables, but --with-env is documented as only controlling inclusion of environment variables in the published artifact. If config content is intended to be checked, it probably needs a separate check/flag/message; otherwise these tests will lock in a behavior that makes --with-env bypass unrelated config-content validation.
What I did
PR #13402 removed the inline-env check entirely, so literal hardcoded values like
MYSQL_ROOT_PASSWORD: totonow silently leak into the OCI artifact. Restore the check, scoped to literal values only,${VAR}and$VARremain symbolic in the published YAML and still pass without--with-env, preserving the legitimate fix from #13402.The walk follows the same files processFile actually serializes (top-level compose files plus local extends parents), so inherited literals and unrelated services in extends parents are covered too. Errors aggregate per service. With
--with-envthe walk is skipped entirely since the user has opted in.Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did
