From bbfec2d532518e6536b9924d1f039abd07c6f2ef Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 14 Nov 2024 13:25:10 -0500 Subject: [PATCH 1/2] pass all marks through conditional expressions --- hclsyntax/expression.go | 31 ++++++++++++++------------- hclsyntax/expression_template_test.go | 4 ++-- hclsyntax/expression_test.go | 2 +- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 577a50fa..94977460 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -788,21 +788,24 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic }) return cty.UnknownVal(resultType), diags } - if !condResult.IsKnown() { - // we use the unmarked values throughout the unknown branch - _, condResultMarks := condResult.Unmark() - trueResult, trueResultMarks := trueResult.Unmark() - falseResult, falseResultMarks := falseResult.Unmark() - // use a value to merge marks - _, resMarks := cty.DynamicVal.WithMarks(condResultMarks, trueResultMarks, falseResultMarks).Unmark() + // Now that we have all three values, collect all the marks for the result. + // Since it's possible that a condition value could be unknown, and the + // consumer needs to deal with any marks from either branch anyway, we must + // always combine them for consistent results. + condResult, condResultMarks := condResult.Unmark() + trueResult, trueResultMarks := trueResult.Unmark() + falseResult, falseResultMarks := falseResult.Unmark() + var resMarks []cty.ValueMarks + resMarks = append(resMarks, condResultMarks, trueResultMarks, falseResultMarks) + if !condResult.IsKnown() { trueRange := trueResult.Range() falseRange := falseResult.Range() // if both branches are known to be null, then the result must still be null if trueResult.IsNull() && falseResult.IsNull() { - return cty.NullVal(resultType).WithMarks(resMarks), diags + return cty.NullVal(resultType).WithMarks(resMarks...), diags } // We might be able to offer a refined range for the result based on @@ -841,7 +844,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic ref = ref.NumberRangeUpperBound(hi, hiInc) } - return ref.NewValue().WithMarks(resMarks), diags + return ref.NewValue().WithMarks(resMarks...), diags } if trueResult.Type().IsCollectionType() && falseResult.Type().IsCollectionType() { @@ -867,7 +870,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic } ref = ref.CollectionLengthLowerBound(lo).CollectionLengthUpperBound(hi) - return ref.NewValue().WithMarks(resMarks), diags + return ref.NewValue().WithMarks(resMarks...), diags } } @@ -875,7 +878,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic if trueRange.DefinitelyNotNull() && falseRange.DefinitelyNotNull() { ret = ret.RefineNotNull() } - return ret.WithMarks(resMarks), diags + return ret.WithMarks(resMarks...), diags } condResult, err := convert.Convert(condResult, cty.Bool) @@ -892,8 +895,6 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic return cty.UnknownVal(resultType), diags } - // Unmark result before testing for truthiness - condResult, _ = condResult.UnmarkDeep() if condResult.True() { diags = append(diags, trueDiags...) if convs[0] != nil { @@ -916,7 +917,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic trueResult = cty.UnknownVal(resultType) } } - return trueResult, diags + return trueResult.WithMarks(resMarks...), diags } else { diags = append(diags, falseDiags...) if convs[1] != nil { @@ -939,7 +940,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic falseResult = cty.UnknownVal(resultType) } } - return falseResult, diags + return falseResult.WithMarks(resMarks...), diags } } diff --git a/hclsyntax/expression_template_test.go b/hclsyntax/expression_template_test.go index 31198458..92c11afe 100644 --- a/hclsyntax/expression_template_test.go +++ b/hclsyntax/expression_template_test.go @@ -318,14 +318,14 @@ trim`, cty.UnknownVal(cty.String).Refine().NotNull().StringPrefixFull(strings.Repeat("_", 128)).NewValue(), 0, }, - { // marks from uninterpolated values are ignored + { // all marks are passed through to ensure result is always consistent `hello%{ if false } ${target}%{ endif }`, &hcl.EvalContext{ Variables: map[string]cty.Value{ "target": cty.StringVal("world").Mark("sensitive"), }, }, - cty.StringVal("hello"), + cty.StringVal("hello").Mark("sensitive"), 0, }, { // marks from interpolated values are passed through diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index b50dc137..cb1132e3 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -2175,7 +2175,7 @@ EOT }).Mark("sensitive"), }, }, - cty.NumberIntVal(1), + cty.NumberIntVal(1).Mark("sensitive"), 0, }, { // auto-converts collection types From b48ba6ecbfd16da22e3ad5476427744f9bee47b1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 14 Nov 2024 14:27:10 -0500 Subject: [PATCH 2/2] pass marks through unknown ForExpr values Some of the unknown value paths within ForExpr were dropping marks from the result. --- hclsyntax/expression.go | 43 ++++++++++++++++++++++++++---------- hclsyntax/expression_test.go | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 94977460..f4c3a6d7 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -1430,9 +1430,9 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { }) return cty.DynamicVal, diags } - if !collVal.IsKnown() { - return cty.DynamicVal, diags - } + + // Grab the CondExpr marks when we're returning early with an unknown + var condMarks cty.ValueMarks // Before we start we'll do an early check to see if any CondExpr we've // been given is of the wrong type. This isn't 100% reliable (it may @@ -1460,6 +1460,9 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { }) return cty.DynamicVal, diags } + + _, condMarks = result.Unmark() + _, err := convert.Convert(result, cty.Bool) if err != nil { diags = append(diags, &hcl.Diagnostic{ @@ -1478,6 +1481,10 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { } } + if !collVal.IsKnown() { + return cty.DynamicVal.WithMarks(append(marks, condMarks)...), diags + } + if e.KeyExpr != nil { // Producing an object var vals map[string]cty.Value @@ -1518,6 +1525,12 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { known = false continue } + + // Extract and merge marks from the include expression into the + // main set of marks + _, includeMarks := includeRaw.Unmark() + marks = append(marks, includeMarks) + include, err := convert.Convert(includeRaw, cty.Bool) if err != nil { if known { @@ -1541,7 +1554,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { // Extract and merge marks from the include expression into the // main set of marks - includeUnmarked, includeMarks := include.Unmark() + includeUnmarked, _ := include.Unmark() marks = append(marks, includeMarks) if includeUnmarked.False() { // Skip this element @@ -1566,6 +1579,10 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { known = false continue } + + _, keyMarks := keyRaw.Unmark() + marks = append(marks, keyMarks) + if !keyRaw.IsKnown() { known = false continue @@ -1588,8 +1605,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { continue } - key, keyMarks := key.Unmark() - marks = append(marks, keyMarks) + key, _ = key.Unmark() val, valDiags := e.ValExpr.Value(childCtx) diags = append(diags, valDiags...) @@ -1619,7 +1635,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { } if !known { - return cty.DynamicVal, diags + return cty.DynamicVal.WithMarks(marks...), diags } if e.Group { @@ -1665,6 +1681,12 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { known = false continue } + + // Extract and merge marks from the include expression into the + // main set of marks + _, includeMarks := includeRaw.Unmark() + marks = append(marks, includeMarks) + if !includeRaw.IsKnown() { // We will eventually return DynamicVal, but we'll continue // iterating in case there are other diagnostics to gather @@ -1690,10 +1712,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { continue } - // Extract and merge marks from the include expression into the - // main set of marks - includeUnmarked, includeMarks := include.Unmark() - marks = append(marks, includeMarks) + includeUnmarked, _ := include.Unmark() if includeUnmarked.False() { // Skip this element continue @@ -1706,7 +1725,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { } if !known { - return cty.DynamicVal, diags + return cty.DynamicVal.WithMarks(marks...), diags } return cty.TupleVal(vals).WithMarks(marks...), diags diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index cb1132e3..df11d28c 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1143,6 +1143,49 @@ upper( }).Mark("sensitive"), 0, }, + { + `{ for k, v in things: k => v if k != unknown_secret }`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "things": cty.MapVal(map[string]cty.Value{ + "a": cty.True, + "b": cty.False, + "c": cty.False, + }), + "unknown_secret": cty.UnknownVal(cty.String).Mark("sensitive"), + }, + }, + cty.DynamicVal.Mark("sensitive"), + 0, + }, + { + `[ for v in things: v if v != unknown_secret ]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "things": cty.TupleVal([]cty.Value{ + cty.StringVal("a"), + cty.StringVal("b"), + }), + "unknown_secret": cty.UnknownVal(cty.String).Mark("sensitive"), + }, + }, + cty.DynamicVal.Mark("sensitive"), + 0, + }, + { + `[ for v in things: v if v != secret ]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "things": cty.TupleVal([]cty.Value{ + cty.UnknownVal(cty.String).Mark("mark"), + cty.StringVal("b"), + }), + "secret": cty.StringVal("b").Mark("sensitive"), + }, + }, + cty.DynamicVal.WithMarks(cty.NewValueMarks("mark", "sensitive")), + 0, + }, { `[{name: "Steve"}, {name: "Ermintrude"}].*.name`, nil,