Skip to content

Commit 9b956f0

Browse files
ostermanclaude
andcommitted
fix: discard invalid component pro: map instead of passing it through
CodeRabbit review (PR #2883, discussion_r3729413928): extractComponentSections assigned result.ComponentPro before schema.DecodeComponentPro validated it, so a typo'd pro: {enable: false} (meant "enabled") still flowed downstream as the raw map. pro.ResolveSection ignores unrecognized keys, so the typo silently resolved to Pro's default-enabled behavior -- the opposite of what the user wrote. Now the raw map is only assigned after decoding succeeds; the warning stays non-fatal so sibling components still process. Updated TestProcessStackConfig_ProSectionUnknownKeyDoesNotBlockProcessing to assert the pro: section is absent (not silently enabling Pro) rather than passed through unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent af4fb71 commit 9b956f0

2 files changed

Lines changed: 17 additions & 13 deletions

File tree

internal/exec/stack_processor_process_stacks_helpers_extraction.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,15 +64,19 @@ func extractComponentSections(opts *ComponentProcessorOptions, result *Component
6464
log.Warn("component 'pro' section must be a map, ignoring it",
6565
"error", fmt.Errorf("%w: 'components.%s.%s.pro' in the file '%s'", errUtils.ErrInvalidComponentPro, opts.ComponentType, opts.Component, opts.StackName))
6666
} else {
67-
result.ComponentPro = componentPro
68-
// Strict-decode as a validation-only side channel: catches an unrecognized key
69-
// (e.g. "enable" instead of "enabled") the same way `atmos validate stacks`
70-
// would via JSON Schema, without requiring that separate command to have been
71-
// run first. Non-fatal for the same reason as the type check above -- ComponentPro
72-
// stays the raw map either way, so downstream resolution is unaffected.
67+
// Strict-decode before assigning: catches an unrecognized key (e.g. "enable"
68+
// instead of "enabled") the same way `atmos validate stacks` would via JSON
69+
// Schema, without requiring that separate command to have been run first.
70+
// Discarding the raw map on failure (rather than assigning it anyway) matters --
71+
// downstream resolution ignores unknown keys, so a `pro: {enable: false}` typo
72+
// would otherwise silently fall through to Pro's enabled-by-default behavior,
73+
// the opposite of what the user wrote. Non-fatal for the same reason as the type
74+
// check above: one component's typo must not abort sibling components.
7375
if _, err := schema.DecodeComponentPro(componentPro); err != nil {
7476
log.Warn("component 'pro' section has an unrecognized field, check for a typo (e.g. 'enable' instead of 'enabled')",
7577
"component", opts.Component, "type", opts.ComponentType, "file", opts.StackName, "error", err)
78+
} else {
79+
result.ComponentPro = componentPro
7680
}
7781
}
7882
}

internal/exec/stack_processor_process_stacks_test.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1635,10 +1635,11 @@ func TestProcessStackConfig_MalformedComponentProDoesNotBlockSiblings(t *testing
16351635

16361636
// TestProcessStackConfig_ProSectionUnknownKeyDoesNotBlockProcessing guards against a typo'd
16371637
// pro: key (e.g. "enable" instead of "enabled") having any fatal effect. schema.DecodeComponentPro
1638-
// rejects the unknown key so a warning gets logged (see extractComponentSections), but that must
1639-
// stay a warning, not an error -- and the raw pro: map must still pass through unchanged so
1640-
// pro.ResolveSection's existing default-enabled-unless-explicit-false behavior is unaffected by
1641-
// this validation-only side channel.
1638+
// rejects the unknown key so a warning gets logged (see extractComponentSections), and that must
1639+
// stay a warning, not an error. But the invalid raw map must also be discarded rather than passed
1640+
// through: pro.ResolveSection ignores unrecognized keys, so a passed-through `{enable: false}`
1641+
// would silently resolve to Pro's default-enabled behavior -- the opposite of the user's evident
1642+
// intent to disable it.
16421643
func TestProcessStackConfig_ProSectionUnknownKeyDoesNotBlockProcessing(t *testing.T) {
16431644
atmosConfig := &schema.AtmosConfiguration{}
16441645

@@ -1680,9 +1681,8 @@ func TestProcessStackConfig_ProSectionUnknownKeyDoesNotBlockProcessing(t *testin
16801681
require.True(t, ok, "terraform components should be present")
16811682
typo, ok := terraformSection["typo"].(map[string]any)
16821683
require.True(t, ok, "typo component should still resolve")
1683-
proSection, ok := typo[cfg.ProSectionName].(map[string]any)
1684-
require.True(t, ok, "raw pro: map must still pass through despite the unrecognized key")
1685-
assert.Equal(t, map[string]any{"enable": false}, proSection, "the strict decode is validation-only and must not mutate or strip the raw section")
1684+
_, hasPro := typo[cfg.ProSectionName]
1685+
assert.False(t, hasPro, "an invalid pro: map must be discarded, not passed through as if it enabled Pro by default")
16861686
}
16871687

16881688
// componentHooks extracts the merged hooks section for a terraform component

0 commit comments

Comments
 (0)