cli: Diagnostics can include collections with sensitive elements
We previously had a shallow IsMarked call in compactValueStr's caller but then a more-conservative deep ContainsMarked call inside compactValueStr with a different resulting message. As well as causing an inconsistency in messages, this was also a bit confusing because it made it seem like a non-sensitive collection containing a sensitive element value was wholly sensitive, making the debug information in the diagnostic messages not trustworthy for debugging certain varieties of problem. I originally considered just removing the redundant check in compactValueStr here, but ultimately I decided to keep it as a sort of defense in depth in case a future refactoring disconnects these two checks. This should also serve as a prompt to someone making later changes to compactValueStr to think about the implications of sensitive values in there, which otherwise wouldn't be mentioned at all. Disclosing information about a collection containing sensitive values is safe here because compactValueStr only discloses information about the value's type and element keys, and neither of those can be sensitive in isolation. (Constructing a map with sensitive keys reduces to a sensitive overall map.)
This commit is contained in:
parent
751fba49a7
commit
8f233cde4c
|
@ -317,10 +317,20 @@ func compactValueStr(val cty.Value) string {
|
||||||
// helpful but concise messages in diagnostics. It is not comprehensive
|
// helpful but concise messages in diagnostics. It is not comprehensive
|
||||||
// nor intended to be used for other purposes.
|
// nor intended to be used for other purposes.
|
||||||
|
|
||||||
if val.ContainsMarked() {
|
if val.IsMarked() {
|
||||||
|
// We check this in here just to make sure, but note that the caller
|
||||||
|
// of compactValueStr ought to have already checked this and skipped
|
||||||
|
// calling into compactValueStr anyway, so this shouldn't actually
|
||||||
|
// be reachable.
|
||||||
return "(sensitive value)"
|
return "(sensitive value)"
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// WARNING: We've only checked that the value isn't sensitive _shallowly_
|
||||||
|
// here, and so we must never show any element values from complex types
|
||||||
|
// in here. However, it's fine to show map keys and attribute names because
|
||||||
|
// those are never sensitive in isolation: the entire value would be
|
||||||
|
// sensitive in that case.
|
||||||
|
|
||||||
ty := val.Type()
|
ty := val.Type()
|
||||||
switch {
|
switch {
|
||||||
case val.IsNull():
|
case val.IsNull():
|
||||||
|
|
|
@ -360,6 +360,62 @@ func TestNewDiagnostic(t *testing.T) {
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
"error with source code subject and expression referring to a collection containing a sensitive value": {
|
||||||
|
&hcl.Diagnostic{
|
||||||
|
Severity: hcl.DiagError,
|
||||||
|
Summary: "Wrong noises",
|
||||||
|
Detail: "Biological sounds are not allowed",
|
||||||
|
Subject: &hcl.Range{
|
||||||
|
Filename: "test.tf",
|
||||||
|
Start: hcl.Pos{Line: 2, Column: 9, Byte: 42},
|
||||||
|
End: hcl.Pos{Line: 2, Column: 26, Byte: 59},
|
||||||
|
},
|
||||||
|
Expression: hcltest.MockExprTraversal(hcl.Traversal{
|
||||||
|
hcl.TraverseRoot{Name: "var"},
|
||||||
|
hcl.TraverseAttr{Name: "boop"},
|
||||||
|
}),
|
||||||
|
EvalContext: &hcl.EvalContext{
|
||||||
|
Variables: map[string]cty.Value{
|
||||||
|
"var": cty.ObjectVal(map[string]cty.Value{
|
||||||
|
"boop": cty.MapVal(map[string]cty.Value{
|
||||||
|
"hello!": cty.StringVal("bleurgh").Mark("sensitive"),
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
&Diagnostic{
|
||||||
|
Severity: "error",
|
||||||
|
Summary: "Wrong noises",
|
||||||
|
Detail: "Biological sounds are not allowed",
|
||||||
|
Range: &DiagnosticRange{
|
||||||
|
Filename: "test.tf",
|
||||||
|
Start: Pos{
|
||||||
|
Line: 2,
|
||||||
|
Column: 9,
|
||||||
|
Byte: 42,
|
||||||
|
},
|
||||||
|
End: Pos{
|
||||||
|
Line: 2,
|
||||||
|
Column: 26,
|
||||||
|
Byte: 59,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
Snippet: &DiagnosticSnippet{
|
||||||
|
Context: strPtr(`resource "test_resource" "test"`),
|
||||||
|
Code: (` foo = var.boop["hello!"]`),
|
||||||
|
StartLine: (2),
|
||||||
|
HighlightStartOffset: (8),
|
||||||
|
HighlightEndOffset: (25),
|
||||||
|
Values: []DiagnosticExpressionValue{
|
||||||
|
{
|
||||||
|
Traversal: `var.boop`,
|
||||||
|
Statement: `is map of string with 1 element`,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
"error with source code subject and unknown string expression": {
|
"error with source code subject and unknown string expression": {
|
||||||
&hcl.Diagnostic{
|
&hcl.Diagnostic{
|
||||||
Severity: hcl.DiagError,
|
Severity: hcl.DiagError,
|
||||||
|
|
|
@ -0,0 +1,31 @@
|
||||||
|
{
|
||||||
|
"severity": "error",
|
||||||
|
"summary": "Wrong noises",
|
||||||
|
"detail": "Biological sounds are not allowed",
|
||||||
|
"range": {
|
||||||
|
"filename": "test.tf",
|
||||||
|
"start": {
|
||||||
|
"line": 2,
|
||||||
|
"column": 9,
|
||||||
|
"byte": 42
|
||||||
|
},
|
||||||
|
"end": {
|
||||||
|
"line": 2,
|
||||||
|
"column": 26,
|
||||||
|
"byte": 59
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"snippet": {
|
||||||
|
"context": "resource \"test_resource\" \"test\"",
|
||||||
|
"code": " foo = var.boop[\"hello!\"]",
|
||||||
|
"start_line": 2,
|
||||||
|
"highlight_start_offset": 8,
|
||||||
|
"highlight_end_offset": 25,
|
||||||
|
"values": [
|
||||||
|
{
|
||||||
|
"traversal": "var.boop",
|
||||||
|
"statement": "is map of string with 1 element"
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
}
|
Loading…
Reference in New Issue