From c9706b6e45dd60c602af61fd861242d3f949c878 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Sun, 21 May 2023 15:14:34 +0200 Subject: [PATCH 01/11] Implementation of fields with capability type analyser-a --- lint/capability_fields_analyser.go | 103 ++++++++++++++++ lint/capability_fields_analyser_test.go | 157 ++++++++++++++++++++++++ 2 files changed, 260 insertions(+) create mode 100644 lint/capability_fields_analyser.go create mode 100644 lint/capability_fields_analyser_test.go diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go new file mode 100644 index 00000000..a5ad9b0a --- /dev/null +++ b/lint/capability_fields_analyser.go @@ -0,0 +1,103 @@ +/* + * Cadence-lint - The Cadence linter + * + * Copyright 2019-2022 Dapper Labs, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package lint + +import ( + "github.com/onflow/cadence/runtime/ast" + "github.com/onflow/cadence/tools/analysis" +) + +func DetectCapabilityType(typeToCheck ast.Type) bool { + const capabilityTypeLiteral = "Capability" + switch upcastedType := typeToCheck.(type) { + case *ast.NominalType: + return upcastedType.Identifier.Identifier == capabilityTypeLiteral + case *ast.OptionalType: + return DetectCapabilityType(upcastedType.Type) + case *ast.VariableSizedType: + return DetectCapabilityType(upcastedType.Type) + case *ast.ConstantSizedType: + return DetectCapabilityType(upcastedType.Type) + case *ast.DictionaryType: + return DetectCapabilityType(upcastedType.KeyType) || DetectCapabilityType(upcastedType.ValueType) + case *ast.FunctionType: + return false + case *ast.ReferenceType: + return DetectCapabilityType(upcastedType.Type) + case *ast.RestrictedType: + return false + case *ast.InstantiationType: + return DetectCapabilityType(upcastedType.Type) + default: + panic("Unknown type") + } +} + +var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { + + elementFilter := []ast.Element{ + (*ast.FieldDeclaration)(nil), + } + + return &analysis.Analyzer{ + Description: "Detects public fields with Capability type", + Requires: []*analysis.Analyzer{ + analysis.InspectorAnalyzer, + }, + Run: func(pass *analysis.Pass) interface{} { + inspector := pass.ResultOf[analysis.InspectorAnalyzer].(*ast.Inspector) + + location := pass.Program.Location + report := pass.Report + + inspector.Preorder( + elementFilter, + func(element ast.Element) { + + field, ok := element.(*ast.FieldDeclaration) + if !ok { + return + } + if field.Access == ast.AccessPublic && DetectCapabilityType(field.TypeAnnotation.Type) { + report( + analysis.Diagnostic{ + Location: location, + Range: ast.NewRangeFromPositioned(nil, element), + Category: UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + ) + + } + + }, + ) + + return nil + }, + } +})() + +func init() { + RegisterAnalyzer( + "public-capability-field", + CapabilityFieldAnalyzer, + ) +} diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go new file mode 100644 index 00000000..72a39d94 --- /dev/null +++ b/lint/capability_fields_analyser_test.go @@ -0,0 +1,157 @@ +/* + * Cadence-lint - The Cadence linter + * + * Copyright 2019-2022 Dapper Labs, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package lint_test + +import ( + "testing" + + "github.com/onflow/cadence/runtime/ast" + "github.com/onflow/cadence/tools/analysis" + "github.com/stretchr/testify/require" + + "github.com/onflow/cadence-tools/lint" +) + +func TestPublicCapabilityFieldInContract(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract ExposingCapability { + pub let my_capability : Capability? + init() { + self.my_capability = nil + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 48, Line: 3, Column: 7}, + EndPos: ast.Position{Offset: 82, Line: 3, Column: 41}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + }, + diagnostics, + ) +} + +func TestPublicDictionaryFieldWithCapabilityValueType(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract ExposingCapability { + pub let my_capability : {String: Capability?} + init() { + self.my_capability = {"key": nil} + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 48, Line: 3, Column: 7}, + EndPos: ast.Position{Offset: 92, Line: 3, Column: 51}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + }, + diagnostics, + ) +} + +func TestPublicArrayFieldWithConcreteCapabilityType(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract ExposingCapability { + pub resource interface MySecretStuffInterface {} + pub let array_capability : [Capability<&{MySecretStuffInterface}>] + init() { + self.array_capability = [self.account.link<&{MySecretStuffInterface}>(/public/Stuff, target: /storage/Stuff)!] + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 104, Line: 4, Column: 7}, + EndPos: ast.Position{Offset: 169, Line: 4, Column: 72}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + }, + diagnostics, + ) +} + +func TestPrivateCapabilityAndNonCapabilityTypeFieldInContract(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract ExposingCapability { + priv let my_capability : Capability? + pub let data: Int + init() { + self.my_capability = nil + self.data = 42 + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic(nil), + diagnostics, + ) +} From 634cfcf1fe5c3ac2aff62522e514605c41aef969 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 17 Jul 2023 23:10:02 +0200 Subject: [PATCH 02/11] Add a capability field analyser --- lint/capability_fields_analyser.go | 20 ++++++++++---------- lint/capability_fields_analyser_test.go | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index a5ad9b0a..0a1a7ba6 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -1,7 +1,7 @@ /* * Cadence-lint - The Cadence linter * - * Copyright 2019-2022 Dapper Labs, Inc. + * Copyright 2019-2023 Dapper Labs, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -24,26 +24,26 @@ import ( ) func DetectCapabilityType(typeToCheck ast.Type) bool { - const capabilityTypeLiteral = "Capability" - switch upcastedType := typeToCheck.(type) { + const capabilityTypeName = "Capability" + switch downcastedType := typeToCheck.(type) { case *ast.NominalType: - return upcastedType.Identifier.Identifier == capabilityTypeLiteral + return downcastedType.Identifier.Identifier == capabilityTypeName case *ast.OptionalType: - return DetectCapabilityType(upcastedType.Type) + return DetectCapabilityType(downcastedType.Type) case *ast.VariableSizedType: - return DetectCapabilityType(upcastedType.Type) + return DetectCapabilityType(downcastedType.Type) case *ast.ConstantSizedType: - return DetectCapabilityType(upcastedType.Type) + return DetectCapabilityType(downcastedType.Type) case *ast.DictionaryType: - return DetectCapabilityType(upcastedType.KeyType) || DetectCapabilityType(upcastedType.ValueType) + return DetectCapabilityType(downcastedType.KeyType) || DetectCapabilityType(downcastedType.ValueType) case *ast.FunctionType: return false case *ast.ReferenceType: - return DetectCapabilityType(upcastedType.Type) + return DetectCapabilityType(downcastedType.Type) case *ast.RestrictedType: return false case *ast.InstantiationType: - return DetectCapabilityType(upcastedType.Type) + return DetectCapabilityType(downcastedType.Type) default: panic("Unknown type") } diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go index 72a39d94..174ec92d 100644 --- a/lint/capability_fields_analyser_test.go +++ b/lint/capability_fields_analyser_test.go @@ -1,7 +1,7 @@ /* * Cadence-lint - The Cadence linter * - * Copyright 2019-2022 Dapper Labs, Inc. + * Copyright 2019-2023 Dapper Labs, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -139,10 +139,10 @@ func TestPrivateCapabilityAndNonCapabilityTypeFieldInContract(t *testing.T) { ` pub contract ExposingCapability { priv let my_capability : Capability? - pub let data: Int + pub let data: Int init() { self.my_capability = nil - self.data = 42 + self.data = 42 } } `, From a3adda09f177de1a1001ff20777f7056a770ff6b Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Sat, 22 Jul 2023 01:33:33 +0200 Subject: [PATCH 03/11] Added check fo rstruct --- lint/capability_fields_analyser.go | 61 ++++++++++- lint/capability_fields_analyser_test.go | 131 ++++++++++++++++++++++++ 2 files changed, 191 insertions(+), 1 deletion(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index 0a1a7ba6..e0673eaf 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -20,6 +20,7 @@ package lint import ( "github.com/onflow/cadence/runtime/ast" + "github.com/onflow/cadence/runtime/common" "github.com/onflow/cadence/tools/analysis" ) @@ -49,6 +50,38 @@ func DetectCapabilityType(typeToCheck ast.Type) bool { } } +func CollectCompositeIdentifiers(inspector *ast.Inspector) (map[string]bool, map[ast.Identifier]bool) { + compositeIdsCapabilitiesPropery := make(map[string]bool) + fieldsInStruct := make(map[ast.Identifier]bool) + inspector.Preorder( + []ast.Element{(*ast.CompositeDeclaration)(nil)}, + func(element ast.Element) { + switch declaration := element.(type) { + case *ast.CompositeDeclaration: + { + if declaration.CompositeKind != common.CompositeKindStructure { + return + } + for _, d := range declaration.Members.Declarations() { + fd, ok := d.(*ast.FieldDeclaration) + if !ok { + return + } + if fd.Access != ast.AccessPublic { + return + } + if fd.Access == ast.AccessPublic && DetectCapabilityType(fd.TypeAnnotation.Type) { + fieldsInStruct[fd.Identifier] = true + compositeIdsCapabilitiesPropery[declaration.Identifier.Identifier] = true + } + } + } + } + }, + ) + return compositeIdsCapabilitiesPropery, fieldsInStruct +} + var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { elementFilter := []ast.Element{ @@ -62,9 +95,9 @@ var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { }, Run: func(pass *analysis.Pass) interface{} { inspector := pass.ResultOf[analysis.InspectorAnalyzer].(*ast.Inspector) - location := pass.Program.Location report := pass.Report + compositeIdsCapabilitiesPropery, fieldsInStruct := CollectCompositeIdentifiers(inspector) inspector.Preorder( elementFilter, @@ -74,6 +107,32 @@ var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { if !ok { return } + + nt, ok := field.TypeAnnotation.Type.(*ast.NominalType) + if ok { + hasCapability, found := compositeIdsCapabilitiesPropery[nt.Identifier.Identifier] + + if found { + if hasCapability && field.Access == ast.AccessPublic { + report( + analysis.Diagnostic{ + Location: location, + Range: ast.NewRangeFromPositioned(nil, element), + Category: UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + ) + return + } + } + } + _, found := fieldsInStruct[field.Identifier] + if found { + //if its in a struct it is treated by another check + return + } + if field.Access == ast.AccessPublic && DetectCapabilityType(field.TypeAnnotation.Type) { report( analysis.Diagnostic{ diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go index 174ec92d..259b8481 100644 --- a/lint/capability_fields_analyser_test.go +++ b/lint/capability_fields_analyser_test.go @@ -155,3 +155,134 @@ func TestPrivateCapabilityAndNonCapabilityTypeFieldInContract(t *testing.T) { diagnostics, ) } +func TestImplicitCapabilityLeakViaArray(t *testing.T) { + + t.Parallel() + + t.Run("valid", func(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract MyContract { + priv let myCapArray: [Capability] + + init() { + self.myCapArray = [] + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic(nil), + diagnostics, + ) + }) +} + +func TestImplicitCapabilityLeakViaStruct2(t *testing.T) { + t.Run("valid", func(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract MyContract { + pub resource Counter { + priv var count: Int + + init(count: Int) { + self.count = count + } + } + + pub struct ContractData { + pub var owner: Capability + init(cap: Capability) { + self.owner = cap + } + } + + priv var contractData: ContractData + + init(){ + self.contractData = ContractData(cap: + self.account.getCapability<&Counter>(/public/counter) + ) + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic(nil), + diagnostics, + ) + }) +} + +func TestImplicitCapabilityLeakViaStruct3(t *testing.T) { + + t.Parallel() + t.Run("leak via struct field", func(t *testing.T) { + + diagnostics := testAnalyzers(t, + ` + pub contract MyContract { + pub resource Counter { + priv var count: Int + + init(count: Int) { + self.count = count + } + } + + pub struct ContractData { + pub var owner: Capability + init(cap: Capability) { + self.owner = cap + } + } + + pub struct ContractDataIgnored { + pub var owner: Capability + init(cap: Capability) { + self.owner = cap + } + } + + pub var contractData: ContractData + + init(){ + self.contractData = ContractData(cap: + self.account.getCapability<&Counter>(/public/counter) + ) + } + }`, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 484, Line: 25, Column: 6}, + EndPos: ast.Position{Offset: 517, Line: 25, Column: 39}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + }, + diagnostics, + ) + }) +} From 12562e9d43bb74eecbc63138f4fb659876f206d8 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Sat, 22 Jul 2023 02:22:37 +0200 Subject: [PATCH 04/11] Added struct with pub capasbiolities in a rec detector --- lint/capability_fields_analyser.go | 62 ++++++++++-------------------- 1 file changed, 20 insertions(+), 42 deletions(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index e0673eaf..a8e34563 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -24,35 +24,36 @@ import ( "github.com/onflow/cadence/tools/analysis" ) -func DetectCapabilityType(typeToCheck ast.Type) bool { +func DetectCapabilityType(typeToCheck ast.Type, structWithPubCapabilities map[string]struct{}) bool { const capabilityTypeName = "Capability" switch downcastedType := typeToCheck.(type) { case *ast.NominalType: - return downcastedType.Identifier.Identifier == capabilityTypeName + _, found := structWithPubCapabilities[downcastedType.Identifier.Identifier] + return downcastedType.Identifier.Identifier == capabilityTypeName || found case *ast.OptionalType: - return DetectCapabilityType(downcastedType.Type) + return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) case *ast.VariableSizedType: - return DetectCapabilityType(downcastedType.Type) + return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) case *ast.ConstantSizedType: - return DetectCapabilityType(downcastedType.Type) + return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) case *ast.DictionaryType: - return DetectCapabilityType(downcastedType.KeyType) || DetectCapabilityType(downcastedType.ValueType) + return DetectCapabilityType(downcastedType.KeyType, structWithPubCapabilities) || DetectCapabilityType(downcastedType.ValueType, structWithPubCapabilities) case *ast.FunctionType: return false case *ast.ReferenceType: - return DetectCapabilityType(downcastedType.Type) + return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) case *ast.RestrictedType: return false case *ast.InstantiationType: - return DetectCapabilityType(downcastedType.Type) + return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) default: panic("Unknown type") } } -func CollectCompositeIdentifiers(inspector *ast.Inspector) (map[string]bool, map[ast.Identifier]bool) { - compositeIdsCapabilitiesPropery := make(map[string]bool) - fieldsInStruct := make(map[ast.Identifier]bool) +func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string]struct{}, map[ast.Identifier]struct{}) { + structWithPubCapabilities := make(map[string]struct{}) + fieldsInStruct := make(map[ast.Identifier]struct{}) inspector.Preorder( []ast.Element{(*ast.CompositeDeclaration)(nil)}, func(element ast.Element) { @@ -63,23 +64,23 @@ func CollectCompositeIdentifiers(inspector *ast.Inspector) (map[string]bool, map return } for _, d := range declaration.Members.Declarations() { - fd, ok := d.(*ast.FieldDeclaration) + field, ok := d.(*ast.FieldDeclaration) if !ok { return } - if fd.Access != ast.AccessPublic { + if field.Access != ast.AccessPublic { return } - if fd.Access == ast.AccessPublic && DetectCapabilityType(fd.TypeAnnotation.Type) { - fieldsInStruct[fd.Identifier] = true - compositeIdsCapabilitiesPropery[declaration.Identifier.Identifier] = true + if field.Access == ast.AccessPublic && DetectCapabilityType(field.TypeAnnotation.Type, structWithPubCapabilities) { + fieldsInStruct[field.Identifier] = struct{}{} + structWithPubCapabilities[declaration.Identifier.Identifier] = struct{}{} } } } } }, ) - return compositeIdsCapabilitiesPropery, fieldsInStruct + return structWithPubCapabilities, fieldsInStruct } var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { @@ -97,43 +98,20 @@ var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { inspector := pass.ResultOf[analysis.InspectorAnalyzer].(*ast.Inspector) location := pass.Program.Location report := pass.Report - compositeIdsCapabilitiesPropery, fieldsInStruct := CollectCompositeIdentifiers(inspector) + structTypesPublicCapability, fieldsInStruct := CollectStructsWithPublicCapabilities(inspector) inspector.Preorder( elementFilter, func(element ast.Element) { - field, ok := element.(*ast.FieldDeclaration) if !ok { return } - - nt, ok := field.TypeAnnotation.Type.(*ast.NominalType) - if ok { - hasCapability, found := compositeIdsCapabilitiesPropery[nt.Identifier.Identifier] - - if found { - if hasCapability && field.Access == ast.AccessPublic { - report( - analysis.Diagnostic{ - Location: location, - Range: ast.NewRangeFromPositioned(nil, element), - Category: UpdateCategory, - Message: "It is an anti-pattern to have public Capability fields.", - SecondaryMessage: "Consider restricting access.", - }, - ) - return - } - } - } _, found := fieldsInStruct[field.Identifier] if found { - //if its in a struct it is treated by another check return } - - if field.Access == ast.AccessPublic && DetectCapabilityType(field.TypeAnnotation.Type) { + if field.Access == ast.AccessPublic && DetectCapabilityType(field.TypeAnnotation.Type, structTypesPublicCapability) { report( analysis.Diagnostic{ Location: location, From d98af875031e38bba0de91b6583c8e6adb1f4595 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Sat, 22 Jul 2023 12:38:59 +0200 Subject: [PATCH 05/11] Aded tets from Nazim --- lint/capability_fields_analyser_test.go | 219 ++++++++++++++---------- 1 file changed, 128 insertions(+), 91 deletions(-) diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go index 259b8481..5546fecb 100644 --- a/lint/capability_fields_analyser_test.go +++ b/lint/capability_fields_analyser_test.go @@ -28,38 +28,68 @@ import ( "github.com/onflow/cadence-tools/lint" ) -func TestPublicCapabilityFieldInContract(t *testing.T) { +func TestCapabilityFieldInContract(t *testing.T) { t.Parallel() - diagnostics := testAnalyzers(t, - ` - pub contract ExposingCapability { - pub let my_capability : Capability? - init() { - self.my_capability = nil - } - } - `, - lint.CapabilityFieldAnalyzer, - ) + t.Run("public capability field", func(t *testing.T) { - require.Equal( - t, - []analysis.Diagnostic{ - { - Range: ast.Range{ - StartPos: ast.Position{Offset: 48, Line: 3, Column: 7}, - EndPos: ast.Position{Offset: 82, Line: 3, Column: 41}, + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract ExposingCapability { + pub let my_capability : Capability? + init() { + self.my_capability = nil + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 44, Line: 3, Column: 5}, + EndPos: ast.Position{Offset: 78, Line: 3, Column: 39}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", }, - Location: testLocation, - Category: lint.UpdateCategory, - Message: "It is an anti-pattern to have public Capability fields.", - SecondaryMessage: "Consider restricting access.", }, - }, - diagnostics, - ) + diagnostics, + ) + }) + + t.Run("private capability field", func(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract ExposingCapability { + priv let my_capability : Capability? + pub let data: Int + init() { + self.my_capability = nil + self.data = 42 + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic(nil), + diagnostics, + ) + }) } func TestPublicDictionaryFieldWithCapabilityValueType(t *testing.T) { @@ -131,91 +161,58 @@ func TestPublicArrayFieldWithConcreteCapabilityType(t *testing.T) { ) } -func TestPrivateCapabilityAndNonCapabilityTypeFieldInContract(t *testing.T) { - - t.Parallel() - - diagnostics := testAnalyzers(t, - ` - pub contract ExposingCapability { - priv let my_capability : Capability? - pub let data: Int - init() { - self.my_capability = nil - self.data = 42 - } - } - `, - lint.CapabilityFieldAnalyzer, - ) - - require.Equal( - t, - []analysis.Diagnostic(nil), - diagnostics, - ) -} func TestImplicitCapabilityLeakViaArray(t *testing.T) { t.Parallel() - t.Run("valid", func(t *testing.T) { + t.Run("public leaking capability in an array", func(t *testing.T) { t.Parallel() diagnostics := testAnalyzers(t, ` - pub contract MyContract { - priv let myCapArray: [Capability] - - init() { - self.myCapArray = [] - } - } - `, + pub contract MyContract { + pub let myCapArray: [Capability] + init() { + self.myCapArray = [] + } + } + `, lint.CapabilityFieldAnalyzer, ) require.Equal( t, - []analysis.Diagnostic(nil), + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 38, Line: 3, Column: 6}, + EndPos: ast.Position{Offset: 69, Line: 3, Column: 37}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + }, diagnostics, ) }) -} -func TestImplicitCapabilityLeakViaStruct2(t *testing.T) { - t.Run("valid", func(t *testing.T) { + t.Run("private nonleaking capability in an array", func(t *testing.T) { t.Parallel() diagnostics := testAnalyzers(t, ` - pub contract MyContract { - pub resource Counter { - priv var count: Int - - init(count: Int) { - self.count = count - } - } - - pub struct ContractData { - pub var owner: Capability - init(cap: Capability) { - self.owner = cap - } - } - - priv var contractData: ContractData - - init(){ - self.contractData = ContractData(cap: - self.account.getCapability<&Counter>(/public/counter) - ) - } - } - `, + pub contract MyContract { + priv let myCapArray: [Capability] + + init() { + self.myCapArray = [] + } + } + `, lint.CapabilityFieldAnalyzer, ) @@ -227,11 +224,11 @@ func TestImplicitCapabilityLeakViaStruct2(t *testing.T) { }) } -func TestImplicitCapabilityLeakViaStruct3(t *testing.T) { - - t.Parallel() +func TestImplicitCapabilityLeakViaStruct(t *testing.T) { t.Run("leak via struct field", func(t *testing.T) { + t.Parallel() + diagnostics := testAnalyzers(t, ` pub contract MyContract { @@ -285,4 +282,44 @@ func TestImplicitCapabilityLeakViaStruct3(t *testing.T) { diagnostics, ) }) + t.Run("valid", func(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub contract MyContract { + pub resource Counter { + priv var count: Int + + init(count: Int) { + self.count = count + } + } + + pub struct ContractData { + pub var owner: Capability + init(cap: Capability) { + self.owner = cap + } + } + + priv var contractData: ContractData + + init(){ + self.contractData = ContractData(cap: + self.account.getCapability<&Counter>(/public/counter) + ) + } + } + `, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic(nil), + diagnostics, + ) + }) } From 74665adcca00eca352fd64ffd6fbc849b6abf789 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 24 Jul 2023 13:06:17 +0200 Subject: [PATCH 06/11] Removed superfluous condition --- lint/capability_fields_analyser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index a8e34563..d14cd58d 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -71,7 +71,7 @@ func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string] if field.Access != ast.AccessPublic { return } - if field.Access == ast.AccessPublic && DetectCapabilityType(field.TypeAnnotation.Type, structWithPubCapabilities) { + if DetectCapabilityType(field.TypeAnnotation.Type, structWithPubCapabilities) { fieldsInStruct[field.Identifier] = struct{}{} structWithPubCapabilities[declaration.Identifier.Identifier] = struct{}{} } From 8e5deb07763d4c1faef180221a8ff9101db77f1a Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 28 Aug 2023 17:49:47 +0200 Subject: [PATCH 07/11] Applied minor improvments --- lint/capability_fields_analyser.go | 5 +---- lint/capability_fields_analyser_test.go | 3 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index d14cd58d..0a36a773 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -65,10 +65,7 @@ func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string] } for _, d := range declaration.Members.Declarations() { field, ok := d.(*ast.FieldDeclaration) - if !ok { - return - } - if field.Access != ast.AccessPublic { + if !ok || field.Access != ast.AccessPublic { return } if DetectCapabilityType(field.TypeAnnotation.Type, structWithPubCapabilities) { diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go index 5546fecb..403787d6 100644 --- a/lint/capability_fields_analyser_test.go +++ b/lint/capability_fields_analyser_test.go @@ -225,6 +225,8 @@ func TestImplicitCapabilityLeakViaArray(t *testing.T) { } func TestImplicitCapabilityLeakViaStruct(t *testing.T) { + t.Parallel() + t.Run("leak via struct field", func(t *testing.T) { t.Parallel() @@ -282,6 +284,7 @@ func TestImplicitCapabilityLeakViaStruct(t *testing.T) { diagnostics, ) }) + t.Run("valid", func(t *testing.T) { t.Parallel() From b14fe58fe008d9f19029d76daa280b504b41e917 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 28 Aug 2023 18:14:30 +0200 Subject: [PATCH 08/11] Renamed structWithPubCapabilities --- lint/capability_fields_analyser.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index 0a36a773..08ffd18c 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -24,35 +24,35 @@ import ( "github.com/onflow/cadence/tools/analysis" ) -func DetectCapabilityType(typeToCheck ast.Type, structWithPubCapabilities map[string]struct{}) bool { +func DetectCapabilityType(typeToCheck ast.Type, compositesWithPubCapabilities map[string]struct{}) bool { const capabilityTypeName = "Capability" switch downcastedType := typeToCheck.(type) { case *ast.NominalType: - _, found := structWithPubCapabilities[downcastedType.Identifier.Identifier] + _, found := compositesWithPubCapabilities[downcastedType.Identifier.Identifier] return downcastedType.Identifier.Identifier == capabilityTypeName || found case *ast.OptionalType: - return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) + return DetectCapabilityType(downcastedType.Type, compositesWithPubCapabilities) case *ast.VariableSizedType: - return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) + return DetectCapabilityType(downcastedType.Type, compositesWithPubCapabilities) case *ast.ConstantSizedType: - return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) + return DetectCapabilityType(downcastedType.Type, compositesWithPubCapabilities) case *ast.DictionaryType: - return DetectCapabilityType(downcastedType.KeyType, structWithPubCapabilities) || DetectCapabilityType(downcastedType.ValueType, structWithPubCapabilities) + return DetectCapabilityType(downcastedType.KeyType, compositesWithPubCapabilities) || DetectCapabilityType(downcastedType.ValueType, compositesWithPubCapabilities) case *ast.FunctionType: return false case *ast.ReferenceType: - return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) + return DetectCapabilityType(downcastedType.Type, compositesWithPubCapabilities) case *ast.RestrictedType: return false case *ast.InstantiationType: - return DetectCapabilityType(downcastedType.Type, structWithPubCapabilities) + return DetectCapabilityType(downcastedType.Type, compositesWithPubCapabilities) default: panic("Unknown type") } } func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string]struct{}, map[ast.Identifier]struct{}) { - structWithPubCapabilities := make(map[string]struct{}) + compositesWithPubCapabilities := make(map[string]struct{}) fieldsInStruct := make(map[ast.Identifier]struct{}) inspector.Preorder( []ast.Element{(*ast.CompositeDeclaration)(nil)}, @@ -68,16 +68,16 @@ func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string] if !ok || field.Access != ast.AccessPublic { return } - if DetectCapabilityType(field.TypeAnnotation.Type, structWithPubCapabilities) { + if DetectCapabilityType(field.TypeAnnotation.Type, compositesWithPubCapabilities) { fieldsInStruct[field.Identifier] = struct{}{} - structWithPubCapabilities[declaration.Identifier.Identifier] = struct{}{} + compositesWithPubCapabilities[declaration.Identifier.Identifier] = struct{}{} } } } } }, ) - return structWithPubCapabilities, fieldsInStruct + return compositesWithPubCapabilities, fieldsInStruct } var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { From df8d514c388752b9a19e8b2edf60cf6d729bc405 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 28 Aug 2023 23:09:01 +0200 Subject: [PATCH 09/11] Capture all composite declarations --- lint/capability_fields_analyser.go | 12 ++-- lint/capability_fields_analyser_test.go | 78 +++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index 08ffd18c..d501da68 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -51,7 +51,7 @@ func DetectCapabilityType(typeToCheck ast.Type, compositesWithPubCapabilities ma } } -func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string]struct{}, map[ast.Identifier]struct{}) { +func CollectCompositesWithPublicCapabilities(inspector *ast.Inspector) (map[string]struct{}, map[ast.Identifier]struct{}) { compositesWithPubCapabilities := make(map[string]struct{}) fieldsInStruct := make(map[ast.Identifier]struct{}) inspector.Preorder( @@ -60,16 +60,16 @@ func CollectStructsWithPublicCapabilities(inspector *ast.Inspector) (map[string] switch declaration := element.(type) { case *ast.CompositeDeclaration: { - if declaration.CompositeKind != common.CompositeKindStructure { - return - } for _, d := range declaration.Members.Declarations() { field, ok := d.(*ast.FieldDeclaration) if !ok || field.Access != ast.AccessPublic { return } if DetectCapabilityType(field.TypeAnnotation.Type, compositesWithPubCapabilities) { - fieldsInStruct[field.Identifier] = struct{}{} + if declaration.CompositeKind != common.CompositeKindContract { + fieldsInStruct[field.Identifier] = struct{}{} + } + compositesWithPubCapabilities[declaration.Identifier.Identifier] = struct{}{} } } @@ -95,7 +95,7 @@ var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { inspector := pass.ResultOf[analysis.InspectorAnalyzer].(*ast.Inspector) location := pass.Program.Location report := pass.Report - structTypesPublicCapability, fieldsInStruct := CollectStructsWithPublicCapabilities(inspector) + structTypesPublicCapability, fieldsInStruct := CollectCompositesWithPublicCapabilities(inspector) inspector.Preorder( elementFilter, diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go index 403787d6..0dbc98d5 100644 --- a/lint/capability_fields_analyser_test.go +++ b/lint/capability_fields_analyser_test.go @@ -326,3 +326,81 @@ func TestImplicitCapabilityLeakViaStruct(t *testing.T) { ) }) } + +func TestImplicitCapabilityLeakViaResource(t *testing.T) { + t.Parallel() + + t.Run("leak via resource field", func(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub resource MyResource { + pub let my_capability : Capability? + init() { + self.my_capability = nil + } + } + pub contract MyContract { + + // Declare a public field using the MyResource resource + pub var myResource: @MyResource? + + // Initialize the contract + init() { + self.myResource <- nil + } + }`, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic{ + { + Range: ast.Range{ + StartPos: ast.Position{Offset: 209, Line: 11, Column: 3}, + EndPos: ast.Position{Offset: 240, Line: 11, Column: 34}, + }, + Location: testLocation, + Category: lint.UpdateCategory, + Message: "It is an anti-pattern to have public Capability fields.", + SecondaryMessage: "Consider restricting access.", + }, + }, + diagnostics, + ) + }) + t.Run("valid", func(t *testing.T) { + + t.Parallel() + + diagnostics := testAnalyzers(t, + ` + pub resource MyResource { + pub let my_capability : Capability? + init() { + self.my_capability = nil + } + } + pub contract MyContract { + + // Declare a public field using the MyResource resource + priv var myResource: @MyResource? + + // Initialize the contract + init() { + self.myResource <- nil + } + }`, + lint.CapabilityFieldAnalyzer, + ) + + require.Equal( + t, + []analysis.Diagnostic(nil), + diagnostics, + ) + }) +} From fb685d0e05ec924298c5da60ea01547975ecf421 Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 28 Aug 2023 23:32:06 +0200 Subject: [PATCH 10/11] Added more comments --- lint/capability_fields_analyser.go | 9 ++++++--- lint/capability_fields_analyser_test.go | 2 -- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index d501da68..2546d011 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -53,7 +53,7 @@ func DetectCapabilityType(typeToCheck ast.Type, compositesWithPubCapabilities ma func CollectCompositesWithPublicCapabilities(inspector *ast.Inspector) (map[string]struct{}, map[ast.Identifier]struct{}) { compositesWithPubCapabilities := make(map[string]struct{}) - fieldsInStruct := make(map[ast.Identifier]struct{}) + fieldsInComposite := make(map[ast.Identifier]struct{}) inspector.Preorder( []ast.Element{(*ast.CompositeDeclaration)(nil)}, func(element ast.Element) { @@ -67,7 +67,10 @@ func CollectCompositesWithPublicCapabilities(inspector *ast.Inspector) (map[stri } if DetectCapabilityType(field.TypeAnnotation.Type, compositesWithPubCapabilities) { if declaration.CompositeKind != common.CompositeKindContract { - fieldsInStruct[field.Identifier] = struct{}{} + // public capability fields in contracts are not included in this set as later + // on it is used to exclude false positive. And while a struct with a public capability + // can be a false positive a contract with a public capability is always an anti pattern. + fieldsInComposite[field.Identifier] = struct{}{} } compositesWithPubCapabilities[declaration.Identifier.Identifier] = struct{}{} @@ -77,7 +80,7 @@ func CollectCompositesWithPublicCapabilities(inspector *ast.Inspector) (map[stri } }, ) - return compositesWithPubCapabilities, fieldsInStruct + return compositesWithPubCapabilities, fieldsInComposite } var CapabilityFieldAnalyzer = (func() *analysis.Analyzer { diff --git a/lint/capability_fields_analyser_test.go b/lint/capability_fields_analyser_test.go index 0dbc98d5..e17392b5 100644 --- a/lint/capability_fields_analyser_test.go +++ b/lint/capability_fields_analyser_test.go @@ -386,10 +386,8 @@ func TestImplicitCapabilityLeakViaResource(t *testing.T) { } pub contract MyContract { - // Declare a public field using the MyResource resource priv var myResource: @MyResource? - // Initialize the contract init() { self.myResource <- nil } From c070b6ea2ad337d90fbeb5a8f0726f4bd2dfabae Mon Sep 17 00:00:00 2001 From: Boris Kozlov Date: Mon, 28 Aug 2023 23:34:55 +0200 Subject: [PATCH 11/11] Improved or compariosn --- lint/capability_fields_analyser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lint/capability_fields_analyser.go b/lint/capability_fields_analyser.go index 2546d011..ef9d6378 100644 --- a/lint/capability_fields_analyser.go +++ b/lint/capability_fields_analyser.go @@ -29,7 +29,7 @@ func DetectCapabilityType(typeToCheck ast.Type, compositesWithPubCapabilities ma switch downcastedType := typeToCheck.(type) { case *ast.NominalType: _, found := compositesWithPubCapabilities[downcastedType.Identifier.Identifier] - return downcastedType.Identifier.Identifier == capabilityTypeName || found + return found || downcastedType.Identifier.Identifier == capabilityTypeName case *ast.OptionalType: return DetectCapabilityType(downcastedType.Type, compositesWithPubCapabilities) case *ast.VariableSizedType: