From 85f4a77da09b63ba71b94dc2c37ee0adf4cc3cc7 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 19 Dec 2022 09:12:08 -0800 Subject: [PATCH] types: Ensure List/Map/Object/Set Attributes/AttributeTypes/Elements returns cannot mutate underlying data (#591) Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/556 Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/582 This will ensure that the `Attributes()`, `AttributeTypes()`, and `Elements()` methods return a copy of the underlying map or slice of data, rather than a direct reference to the map or slice. This also prevents `Object`-based plan modification from returning a panic since the updated `Object` `Attributes()` implementation will return an empty map instead of a `nil` map. Provider implementations should always rely on `IsNull()` and `IsUnknown()` for verifying whether types are known, rather than a nil comparison. This is considered a bug fix for the intended behavior of these type implementations rather than a breaking change as it standardizes type handling expectations. New unit testing failures before code updates: ``` --- FAIL: TestMapValueElements_immutable (0.00s) /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/types/basetypes/map_test.go:604: unexpected Elements mutation --- FAIL: TestNestedAttributeObjectPlanModify (0.00s) --- FAIL: TestNestedAttributeObjectPlanModify/response-planvalue-unknown-to-known-nested (0.00s) panic: assignment to entry in nil map [recovered] panic: assignment to entry in nil map goroutine 35 [running]: testing.tRunner.func1.2({0x1010f0180, 0x10115b648}) /opt/homebrew/Cellar/go/1.19.4/libexec/src/testing/testing.go:1396 +0x1c8 testing.tRunner.func1() /opt/homebrew/Cellar/go/1.19.4/libexec/src/testing/testing.go:1399 +0x378 panic({0x1010f0180, 0x10115b648}) /opt/homebrew/Cellar/go/1.19.4/libexec/src/runtime/panic.go:884 +0x204 github.com/hashicorp/terraform-plugin-framework/internal/fwserver.NestedAttributeObjectPlanModify({_, _}, {_, _}, {{{0x1400010d190, 0x1, 0x1}}, {0x1, {0x1400010d1b0, 0x1, ...}}, ...}, ...) /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification.go:1748 +0x510 ``` --- .changelog/591.txt | 15 ++ .../attribute_plan_modification_test.go | 152 ++++++++++++++++++ types/basetypes/list.go | 9 +- types/basetypes/list_test.go | 15 +- types/basetypes/map.go | 12 +- types/basetypes/map_test.go | 15 +- types/basetypes/object.go | 34 +++- types/basetypes/object_test.go | 51 +++++- types/basetypes/set.go | 9 +- types/basetypes/set_test.go | 15 +- 10 files changed, 303 insertions(+), 24 deletions(-) create mode 100644 .changelog/591.txt diff --git a/.changelog/591.txt b/.changelog/591.txt new file mode 100644 index 000000000..bec27fd0d --- /dev/null +++ b/.changelog/591.txt @@ -0,0 +1,15 @@ +```release-note:bug +types/basetypes: Prevented value mutation via the `ListValue`, `MapValue`, and `SetValue` type `Elements()` method return +``` + +```release-note:bug +types/basetypes: Prevented type mutation via the `ObjectType` type `AttributeTypes()` method return +``` + +```release-note:bug +types/basetypes: Prevented value mutation via the `ObjectValue` type `AttributeTypes()` and `Attributes()` method returns +``` + +```release-note:bug +resource/schema/planmodifier: Prevented `assignment to entry in nil map` panic for `Object` type plan modifiers +``` diff --git a/internal/fwserver/attribute_plan_modification_test.go b/internal/fwserver/attribute_plan_modification_test.go index e98497cef..6c206e63e 100644 --- a/internal/fwserver/attribute_plan_modification_test.go +++ b/internal/fwserver/attribute_plan_modification_test.go @@ -7764,6 +7764,158 @@ func TestNestedAttributeObjectPlanModify(t *testing.T) { ), }, }, + "response-planvalue-unknown-to-known": { + object: testschema.NestedAttributeObjectWithPlanModifiers{ + PlanModifiers: []planmodifier.Object{ + testplanmodifier.Object{ + PlanModifyObjectMethod: func(ctx context.Context, req planmodifier.ObjectRequest, resp *planmodifier.ObjectResponse) { + resp.PlanValue = types.ObjectValueMust( + map[string]attr.Type{ + "testattr": types.StringType, + }, + map[string]attr.Value{ + "testattr": types.StringValue("newtestvalue"), + }, + ) + }, + }, + }, + }, + request: planmodifier.ObjectRequest{ + Config: tfsdk.Config{ + Raw: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + }, + }, + map[string]tftypes.Value{ + "test": tftypes.NewValue( + tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + nil, + ), + }, + ), + Schema: fwSchema, + }, + ConfigValue: types.ObjectNull( + map[string]attr.Type{"testattr": types.StringType}, + ), + Path: path.Root("test"), + PathExpression: path.MatchRoot("test"), + Plan: tfsdk.Plan{ + Raw: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + }, + }, + map[string]tftypes.Value{ + "test": tftypes.NewValue( + tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + tftypes.UnknownValue, + ), + }, + ), + Schema: fwSchema, + }, + PlanValue: types.ObjectUnknown( + map[string]attr.Type{"testattr": types.StringType}, + ), + State: testState, + StateValue: fwValue, + }, + response: &ModifyAttributePlanResponse{ + AttributePlan: types.ObjectUnknown( + map[string]attr.Type{"testattr": types.StringType}, + ), + }, + expected: &ModifyAttributePlanResponse{ + AttributePlan: types.ObjectValueMust( + map[string]attr.Type{ + "testattr": types.StringType, + }, + map[string]attr.Value{ + "testattr": types.StringValue("newtestvalue"), + }, + ), + }, + }, + "response-planvalue-unknown-to-known-nested": { + object: testschema.NestedAttributeObject{ + Attributes: map[string]fwschema.Attribute{ + "testattr": testschema.AttributeWithStringPlanModifiers{ + Required: true, + PlanModifiers: []planmodifier.String{ + testplanmodifier.String{ + PlanModifyStringMethod: func(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { + resp.PlanValue = types.StringValue("newtestvalue") // should win over object + }, + }, + }, + }, + }, + }, + request: planmodifier.ObjectRequest{ + Config: tfsdk.Config{ + Raw: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + }, + }, + map[string]tftypes.Value{ + "test": tftypes.NewValue( + tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + nil, + ), + }, + ), + Schema: fwSchema, + }, + ConfigValue: types.ObjectNull( + map[string]attr.Type{"testattr": types.StringType}, + ), + Path: path.Root("test"), + PathExpression: path.MatchRoot("test"), + Plan: tfsdk.Plan{ + Raw: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "test": tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + }, + }, + map[string]tftypes.Value{ + "test": tftypes.NewValue( + tftypes.Object{AttributeTypes: map[string]tftypes.Type{"testattr": tftypes.String}}, + tftypes.UnknownValue, + ), + }, + ), + Schema: fwSchema, + }, + PlanValue: types.ObjectUnknown( + map[string]attr.Type{"testattr": types.StringType}, + ), + State: testState, + StateValue: fwValue, + }, + response: &ModifyAttributePlanResponse{ + AttributePlan: types.ObjectUnknown( + map[string]attr.Type{"testattr": types.StringType}, + ), + }, + expected: &ModifyAttributePlanResponse{ + AttributePlan: types.ObjectValueMust( + map[string]attr.Type{ + "testattr": types.StringType, + }, + map[string]attr.Value{ + "testattr": types.StringValue("newtestvalue"), + }, + ), + }, + }, "response-private": { object: testschema.NestedAttributeObjectWithPlanModifiers{ PlanModifiers: []planmodifier.Object{ diff --git a/types/basetypes/list.go b/types/basetypes/list.go index 1b4b96100..7acc4c15e 100644 --- a/types/basetypes/list.go +++ b/types/basetypes/list.go @@ -309,10 +309,13 @@ type ListValue struct { state attr.ValueState } -// Elements returns the collection of elements for the List. Returns nil if the -// List is null or unknown. +// Elements returns a copy of the collection of elements for the List. func (l ListValue) Elements() []attr.Value { - return l.elements + // Ensure callers cannot mutate the internal elements + result := make([]attr.Value, 0, len(l.elements)) + result = append(result, l.elements...) + + return result } // ElementsAs populates `target` with the elements of the ListValue, throwing an diff --git a/types/basetypes/list_test.go b/types/basetypes/list_test.go index 9c5b8eae9..12d63f0f4 100644 --- a/types/basetypes/list_test.go +++ b/types/basetypes/list_test.go @@ -589,11 +589,11 @@ func TestListValueElements(t *testing.T) { }, "null": { input: NewListNull(StringType{}), - expected: nil, + expected: []attr.Value{}, }, "unknown": { input: NewListUnknown(StringType{}), - expected: nil, + expected: []attr.Value{}, }, } @@ -612,6 +612,17 @@ func TestListValueElements(t *testing.T) { } } +func TestListValueElements_immutable(t *testing.T) { + t.Parallel() + + value := NewListValueMust(StringType{}, []attr.Value{NewStringValue("original")}) + value.Elements()[0] = NewStringValue("modified") + + if !value.Equal(NewListValueMust(StringType{}, []attr.Value{NewStringValue("original")})) { + t.Fatal("unexpected Elements mutation") + } +} + func TestListValueElementType(t *testing.T) { t.Parallel() diff --git a/types/basetypes/map.go b/types/basetypes/map.go index 6171633b7..4dc73607c 100644 --- a/types/basetypes/map.go +++ b/types/basetypes/map.go @@ -314,10 +314,16 @@ type MapValue struct { state attr.ValueState } -// Elements returns the mapping of elements for the Map. Returns nil if the -// Map is null or unknown. +// Elements returns a copy of the mapping of elements for the Map. func (m MapValue) Elements() map[string]attr.Value { - return m.elements + // Ensure callers cannot mutate the internal elements + result := make(map[string]attr.Value, len(m.elements)) + + for key, value := range m.elements { + result[key] = value + } + + return result } // ElementsAs populates `target` with the elements of the MapValue, throwing an diff --git a/types/basetypes/map_test.go b/types/basetypes/map_test.go index 0de66af2e..1da9deae1 100644 --- a/types/basetypes/map_test.go +++ b/types/basetypes/map_test.go @@ -571,11 +571,11 @@ func TestMapValueElements(t *testing.T) { }, "null": { input: NewMapNull(StringType{}), - expected: nil, + expected: map[string]attr.Value{}, }, "unknown": { input: NewMapUnknown(StringType{}), - expected: nil, + expected: map[string]attr.Value{}, }, } @@ -594,6 +594,17 @@ func TestMapValueElements(t *testing.T) { } } +func TestMapValueElements_immutable(t *testing.T) { + t.Parallel() + + value := NewMapValueMust(StringType{}, map[string]attr.Value{"test": NewStringValue("original")}) + value.Elements()["test"] = NewStringValue("modified") + + if !value.Equal(NewMapValueMust(StringType{}, map[string]attr.Value{"test": NewStringValue("original")})) { + t.Fatal("unexpected Elements mutation") + } +} + func TestMapValueElementType(t *testing.T) { t.Parallel() diff --git a/types/basetypes/object.go b/types/basetypes/object.go index 74ee62ed2..87bf83ccd 100644 --- a/types/basetypes/object.go +++ b/types/basetypes/object.go @@ -50,9 +50,16 @@ func (o ObjectType) WithAttributeTypes(typs map[string]attr.Type) attr.TypeWithA } } -// AttributeTypes returns the type's attribute types. +// AttributeTypes returns a copy of the type's attribute types. func (o ObjectType) AttributeTypes() map[string]attr.Type { - return o.AttrTypes + // Ensure callers cannot mutate the value + result := make(map[string]attr.Type, len(o.AttrTypes)) + + for key, value := range o.AttrTypes { + result[key] = value + } + + return result } // TerraformType returns the tftypes.Type that should be used to @@ -356,15 +363,28 @@ func (o ObjectValue) As(ctx context.Context, target interface{}, opts ObjectAsOp }, path.Empty()) } -// Attributes returns the mapping of known attribute values for the Object. -// Returns nil if the Object is null or unknown. +// Attributes returns a copy of the mapping of known attribute values for the Object. func (o ObjectValue) Attributes() map[string]attr.Value { - return o.attributes + // Ensure callers cannot mutate the internal attributes + result := make(map[string]attr.Value, len(o.attributes)) + + for name, value := range o.attributes { + result[name] = value + } + + return result } -// AttributeTypes returns the mapping of attribute types for the Object. +// AttributeTypes returns a copy of the mapping of attribute types for the Object. func (o ObjectValue) AttributeTypes(_ context.Context) map[string]attr.Type { - return o.attributeTypes + // Ensure callers cannot mutate the internal attribute types + result := make(map[string]attr.Type, len(o.attributeTypes)) + + for name, typ := range o.attributeTypes { + result[name] = typ + } + + return result } // Type returns an ObjectType with the same attribute types as `o`. diff --git a/types/basetypes/object_test.go b/types/basetypes/object_test.go index 9b8e71e4e..9314e7f5f 100644 --- a/types/basetypes/object_test.go +++ b/types/basetypes/object_test.go @@ -12,6 +12,17 @@ import ( "github.com/hashicorp/terraform-plugin-go/tftypes" ) +func TestObjectTypeAttributeTypes_immutable(t *testing.T) { + t.Parallel() + + typ := ObjectType{AttrTypes: map[string]attr.Type{"test": StringType{}}} + typ.AttributeTypes()["test"] = BoolType{} + + if !typ.Equal(ObjectType{AttrTypes: map[string]attr.Type{"test": StringType{}}}) { + t.Fatal("unexpected AttributeTypes mutation") + } +} + func TestObjectTypeTerraformType_simple(t *testing.T) { t.Parallel() result := ObjectType{ @@ -932,11 +943,11 @@ func TestObjectValueAttributes(t *testing.T) { }, "null": { input: NewObjectNull(map[string]attr.Type{"test_attr": StringType{}}), - expected: nil, + expected: map[string]attr.Value{}, }, "unknown": { input: NewObjectUnknown(map[string]attr.Type{"test_attr": StringType{}}), - expected: nil, + expected: map[string]attr.Value{}, }, } @@ -955,6 +966,24 @@ func TestObjectValueAttributes(t *testing.T) { } } +func TestObjectValueAttributes_immutable(t *testing.T) { + t.Parallel() + + value := NewObjectValueMust( + map[string]attr.Type{"test": StringType{}}, + map[string]attr.Value{"test": NewStringValue("original")}, + ) + expected := NewObjectValueMust( + map[string]attr.Type{"test": StringType{}}, + map[string]attr.Value{"test": NewStringValue("original")}, + ) + value.Attributes()["test"] = NewStringValue("modified") + + if !value.Equal(expected) { + t.Fatal("unexpected Attributes mutation") + } +} + func TestObjectValueAttributeTypes(t *testing.T) { t.Parallel() @@ -994,6 +1023,24 @@ func TestObjectValueAttributeTypes(t *testing.T) { } } +func TestObjectValueAttributeTypes_immutable(t *testing.T) { + t.Parallel() + + value := NewObjectValueMust( + map[string]attr.Type{"test": StringType{}}, + map[string]attr.Value{"test": NewStringValue("original")}, + ) + expected := NewObjectValueMust( + map[string]attr.Type{"test": StringType{}}, + map[string]attr.Value{"test": NewStringValue("original")}, + ) + value.AttributeTypes(context.Background())["test"] = BoolType{} + + if !value.Equal(expected) { + t.Fatal("unexpected AttributeTypes mutation") + } +} + func TestObjectValueToTerraformValue(t *testing.T) { t.Parallel() type testCase struct { diff --git a/types/basetypes/set.go b/types/basetypes/set.go index 5ffb88163..3167172eb 100644 --- a/types/basetypes/set.go +++ b/types/basetypes/set.go @@ -341,10 +341,13 @@ type SetValue struct { state attr.ValueState } -// Elements returns the collection of elements for the Set. Returns nil if the -// Set is null or unknown. +// Elements returns a copy of the collection of elements for the Set. func (s SetValue) Elements() []attr.Value { - return s.elements + // Ensure callers cannot mutate the internal elements + result := make([]attr.Value, 0, len(s.elements)) + result = append(result, s.elements...) + + return result } // ElementsAs populates `target` with the elements of the SetValue, throwing an diff --git a/types/basetypes/set_test.go b/types/basetypes/set_test.go index 6cf618abd..692ae6072 100644 --- a/types/basetypes/set_test.go +++ b/types/basetypes/set_test.go @@ -866,11 +866,11 @@ func TestSetValueElements(t *testing.T) { }, "null": { input: NewSetNull(StringType{}), - expected: nil, + expected: []attr.Value{}, }, "unknown": { input: NewSetUnknown(StringType{}), - expected: nil, + expected: []attr.Value{}, }, } @@ -889,6 +889,17 @@ func TestSetValueElements(t *testing.T) { } } +func TestSetValueElements_immutable(t *testing.T) { + t.Parallel() + + value := NewSetValueMust(StringType{}, []attr.Value{NewStringValue("original")}) + value.Elements()[0] = NewStringValue("modified") + + if !value.Equal(NewSetValueMust(StringType{}, []attr.Value{NewStringValue("original")})) { + t.Fatal("unexpected Elements mutation") + } +} + func TestSetValueElementType(t *testing.T) { t.Parallel()