Skip to content

Commit 28e59ac

Browse files
ostermanclaude
andcommitted
fix(workdir): sanitize nested component names in BuildPath
A component name containing "/" (e.g. a nested layout like ecs/cluster) made workdir.BuildPath produce a real extra subdirectory instead of a single path segment, since the name was interpolated into "<stack>-<name>" without escaping and then filepath.Join'd. That put the nested component's workdir one level deeper than a flat component's at the same stack. Any path computed relative to the workdir -- most visibly a relative `backend.local.path` template like `../../../.context/tfstate/...` -- therefore climbed to a different real ancestor for the nested component than for the flat one, silently writing state under a different root (<repo>/.workdir/.context/... instead of <repo>/.context/...) even though both components used the identical backend config. Sanitize the component name the same way internal/exec/terraform_generate_ backends.go already does for backend template context: replace "/" with "-" before building the workdir directory name. BuildPath is the single formula reused by the source provisioner and by internal/terraform_backend's JIT-workdir state lookup, so both pick up the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a7b992b commit 28e59ac

3 files changed

Lines changed: 65 additions & 7 deletions

File tree

internal/terraform_backend/terraform_backend_local_test.go

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -309,11 +309,15 @@ func TestReadTerraformBackendLocal_JITWorkdir(t *testing.T) {
309309
}
310310
}`
311311

312-
t.Run("state exists, no _workdir_path (describe path — provisioner not yet run)", func(t *testing.T) {
312+
// assertJITStateFound places stateJSON at
313+
// tempDir/.workdir/terraform/<workdirName>/terraform.tfstate.d/demo/terraform.tfstate
314+
// (the BuildPath formula for stack "demo"), then asserts
315+
// ReadTerraformBackendLocal finds it for a component whose
316+
// atmos_component/component is componentName.
317+
assertJITStateFound := func(t *testing.T, workdirName, componentName string) {
318+
t.Helper()
313319
tempDir := t.TempDir()
314-
// BuildPath("tempDir", "terraform", "null-label", "demo", sections) → tempDir/.workdir/terraform/demo-null-label.
315-
// workspace "demo" → terraform.tfstate.d/demo/terraform.tfstate.
316-
stateDir := filepath.Join(tempDir, ".workdir", "terraform", "demo-null-label", "terraform.tfstate.d", "demo")
320+
stateDir := filepath.Join(tempDir, ".workdir", "terraform", workdirName, "terraform.tfstate.d", "demo")
317321
require.NoError(t, os.MkdirAll(stateDir, 0o755))
318322
require.NoError(t, os.WriteFile(filepath.Join(stateDir, "terraform.tfstate"), []byte(stateJSON), 0o644))
319323

@@ -326,18 +330,33 @@ func TestReadTerraformBackendLocal_JITWorkdir(t *testing.T) {
326330
"workdir": map[string]any{"enabled": true},
327331
},
328332
"atmos_stack": "demo",
329-
"atmos_component": "null-label",
330-
"component": "null-label", // base component (metadata.component); also used by static fallback.
333+
"atmos_component": componentName,
334+
"component": componentName, // base component (metadata.component); also used by static fallback.
331335
"workspace": "demo",
332336
}
333337

334338
content, err := tb.ReadTerraformBackendLocal(config, &sections, nil)
335339
require.NoError(t, err)
336-
require.NotNil(t, content, "expected state file to be found at JIT workdir path")
340+
require.NotNil(t, content, "expected state file to be found at the JIT workdir path")
337341

338342
result, err := tb.ProcessTerraformStateFile(content)
339343
require.NoError(t, err)
340344
assert.Equal(t, "eg-test-demo", result["id"])
345+
}
346+
347+
t.Run("state exists, no _workdir_path (describe path — provisioner not yet run)", func(t *testing.T) {
348+
// BuildPath("tempDir", "terraform", "null-label", "demo", sections) → tempDir/.workdir/terraform/demo-null-label.
349+
assertJITStateFound(t, "demo-null-label", "null-label")
350+
})
351+
352+
t.Run("nested component name does not shift the workdir root", func(t *testing.T) {
353+
// BuildPath must sanitize "/" in the component name to a single path
354+
// segment (demo-ecs-cluster), not a real subdirectory (demo-ecs/cluster)
355+
// -- otherwise this nested component's workdir sits one level deeper
356+
// than a flat component's at the same stack, and any path computed
357+
// relative to it (e.g. a relative local backend path) silently climbs
358+
// to a different ancestor.
359+
assertJITStateFound(t, "demo-ecs-cluster", "ecs/cluster")
341360
})
342361

343362
t.Run("_workdir_path set (apply path — provisioner already ran)", func(t *testing.T) {

pkg/provisioner/workdir/types.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package workdir
33
import (
44
"fmt"
55
"path/filepath"
6+
"strings"
67
"time"
78

89
"github.com/cloudposse/atmos/pkg/perf"
@@ -137,6 +138,17 @@ func BuildPath(basePath, componentType, component, stack string, componentConfig
137138
workdirComponent = atmosComponent
138139
}
139140

141+
// A nested component name (e.g. "ecs/cluster") must not add an extra path
142+
// segment: filepath.Join would otherwise turn it into a real subdirectory,
143+
// making the workdir one level deeper than a flat component's at the same
144+
// stack. That silently changes how many ".." a relative backend path (or
145+
// any other path computed relative to the workdir) needs to reach the
146+
// same ancestor, so equivalent components would write state under
147+
// different roots solely because one component name contains "/". Mirror
148+
// the same sanitization already used for backend template context (see
149+
// internal/exec/terraform_generate_backends.go).
150+
workdirComponent = strings.ReplaceAll(workdirComponent, "/", "-")
151+
140152
workdirName := fmt.Sprintf("%s-%s", stack, workdirComponent)
141153
return filepath.Join(basePath, WorkdirPath, componentType, workdirName)
142154
}

pkg/provisioner/workdir/types_test.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,33 @@ func TestBuildPath(t *testing.T) {
7373
componentConfig: nil,
7474
want: []string{"helmfile", "prod-app"},
7575
},
76+
{
77+
// A nested component name (containing "/", e.g. an ecs/cluster
78+
// directory layout) must sanitize to a single path segment, not
79+
// add an extra directory level. Otherwise a relative backend
80+
// path template like `../../../` climbs a different number of
81+
// real ancestors for a nested component than for a flat one at
82+
// the same stack, silently writing state under a different root
83+
// (see docs/fixes for the regression this guards).
84+
name: "nested component name does not add a path segment",
85+
basePath: "/base",
86+
componentType: "terraform",
87+
component: "ecs/cluster",
88+
stack: "fixtures",
89+
componentConfig: map[string]any{},
90+
want: []string{"terraform", "fixtures-ecs-cluster"},
91+
},
92+
{
93+
name: "nested atmos_component instance name does not add a path segment",
94+
basePath: "/base",
95+
componentType: "terraform",
96+
component: "ecs/cluster",
97+
stack: "fixtures",
98+
componentConfig: map[string]any{
99+
"atmos_component": "ecs/cluster-inherited-instance",
100+
},
101+
want: []string{"terraform", "fixtures-ecs-cluster-inherited-instance"},
102+
},
76103
}
77104

78105
for _, tt := range tests {

0 commit comments

Comments
 (0)