Skip to content

Commit

Permalink
Fix accidental mutation of shared cty.Paths in ValueMarks funcs
Browse files Browse the repository at this point in the history
Go's `append()` reserves the right to mutate its primary argument in-place, and
expects the caller to assign its return value to the same variable that was
passed as the primary argument. Due to what was almost definitely a typo
(followed by copy-paste mishap), the configschema `Block.ValueMarks` and
`Object.ValueMarks` functions were treating it like an immutable function that
returns a new slice.

In rare and hard-to-reproduce cases, this was causing bizarre malfunctions when
marking sensitive schema attributes in deeply-nested block structures --
omitting the marks for some sensitive values (🚨), and marking other entire
blocks as sensitive (which is supposed to be impossible). The chaotic and
unreliable nature of the bugs is likely related to `append()`'s automatic slice
reallocation behavior (if the append operation overflows the original array
allocation, the resulting behavior can _look_ immutable), but there might be
other contributing factors too.

This commit fixes existing instances of the problem, and wraps the desired
copy-and-append behavior in a helper function to simplify handling shared parent
paths in an immutable way.
  • Loading branch information
nfagerlund committed Jan 19, 2023
1 parent 5ac0375 commit 83428c9
Showing 1 changed file with 18 additions and 16 deletions.
34 changes: 18 additions & 16 deletions internal/configs/configschema/marks.go
Expand Up @@ -7,6 +7,15 @@ import (
"github.com/zclconf/go-cty/cty"
)

// copyAndExtendPath returns a copy of a cty.Path with some additional
// `cty.PathStep`s appended to its end, to simplify creating new child paths.
func copyAndExtendPath(path cty.Path, nextSteps ...cty.PathStep) cty.Path {
newPath := make(cty.Path, len(path), len(path)+len(nextSteps))
copy(newPath, path)
newPath = append(newPath, nextSteps...)
return newPath
}

// ValueMarks returns a set of path value marks for a given value and path,
// based on the sensitive flag for each attribute within the schema. Nested
// blocks are descended (if present in the given value).
Expand All @@ -17,9 +26,7 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks {
for name, attrS := range b.Attributes {
if attrS.Sensitive {
// Create a copy of the path, with this step added, to add to our PathValueMarks slice
attrPath := make(cty.Path, len(path), len(path)+1)
copy(attrPath, path)
attrPath = append(path, cty.GetAttrStep{Name: name})
attrPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name})
pvm = append(pvm, cty.PathValueMarks{
Path: attrPath,
Marks: cty.NewValueMarks(marks.Sensitive),
Expand All @@ -41,9 +48,7 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks {
}

// Create a copy of the path, with this step added, to add to our PathValueMarks slice
attrPath := make(cty.Path, len(path), len(path)+1)
copy(attrPath, path)
attrPath = append(path, cty.GetAttrStep{Name: name})
attrPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name})

pvm = append(pvm, attrS.NestedType.ValueMarks(val.GetAttr(name), attrPath)...)
}
Expand All @@ -61,17 +66,18 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks {
}

// Create a copy of the path, with this step added, to add to our PathValueMarks slice
blockPath := make(cty.Path, len(path), len(path)+1)
copy(blockPath, path)
blockPath = append(path, cty.GetAttrStep{Name: name})
blockPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name})

switch blockS.Nesting {
case NestingSingle, NestingGroup:
pvm = append(pvm, blockS.Block.ValueMarks(blockV, blockPath)...)
case NestingList, NestingMap, NestingSet:
for it := blockV.ElementIterator(); it.Next(); {
idx, blockEV := it.Element()
morePaths := blockS.Block.ValueMarks(blockEV, append(blockPath, cty.IndexStep{Key: idx}))
// Create a copy of the path, with this block instance's index
// step added, to add to our PathValueMarks slice
blockInstancePath := copyAndExtendPath(blockPath, cty.IndexStep{Key: idx})
morePaths := blockS.Block.ValueMarks(blockEV, blockInstancePath)
pvm = append(pvm, morePaths...)
}
default:
Expand Down Expand Up @@ -100,9 +106,7 @@ func (o *Object) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks {
switch o.Nesting {
case NestingSingle, NestingGroup:
// Create a path to this attribute
attrPath := make(cty.Path, len(path), len(path)+1)
copy(attrPath, path)
attrPath = append(path, cty.GetAttrStep{Name: name})
attrPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name})

if attrS.Sensitive {
// If the entire attribute is sensitive, mark it so
Expand All @@ -127,9 +131,7 @@ func (o *Object) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks {
// of the loops: index into the collection, then the contained
// attribute name. This is because we have one type
// representing multiple collection elements.
attrPath := make(cty.Path, len(path), len(path)+2)
copy(attrPath, path)
attrPath = append(path, cty.IndexStep{Key: idx}, cty.GetAttrStep{Name: name})
attrPath := copyAndExtendPath(path, cty.IndexStep{Key: idx}, cty.GetAttrStep{Name: name})

if attrS.Sensitive {
// If the entire attribute is sensitive, mark it so
Expand Down

0 comments on commit 83428c9

Please sign in to comment.