From be9e9720d5c0ff96c6e3f6d4b65ec97a7e8281ee Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 9 Apr 2020 09:16:45 +0100 Subject: [PATCH 1/5] Introduce class `TestFile` and use it. --- .../CWE-295/DisabledCertificateCheck.ql | 2 +- ql/src/filters/ClassifyFiles.ql | 2 +- ql/src/semmle/go/frameworks/Testing.qll | 28 +++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql b/ql/src/Security/CWE-295/DisabledCertificateCheck.ql index 1adeb5be67c..268a9a5bf09 100644 --- a/ql/src/Security/CWE-295/DisabledCertificateCheck.ql +++ b/ql/src/Security/CWE-295/DisabledCertificateCheck.ql @@ -115,5 +115,5 @@ where becomesPartOf*(base, init) ) and // exclude results in test code - exists(File fl | fl = w.getFile() | not fl = any(TestCase tc).getFile()) + exists(File fl | fl = w.getFile() | not fl instanceof TestFile) select w, "InsecureSkipVerify should not be used in production code." diff --git a/ql/src/filters/ClassifyFiles.ql b/ql/src/filters/ClassifyFiles.ql index 120ee3100a8..792e9832ec2 100644 --- a/ql/src/filters/ClassifyFiles.ql +++ b/ql/src/filters/ClassifyFiles.ql @@ -21,7 +21,7 @@ string generatorCommentRegex() { predicate classify(File f, string category) { // tests - f = any(TestCase tc).getFile() and + f instanceof TestFile and category = "test" or // vendored code diff --git a/ql/src/semmle/go/frameworks/Testing.qll b/ql/src/semmle/go/frameworks/Testing.qll index 33548a8720e..022b74b5db4 100644 --- a/ql/src/semmle/go/frameworks/Testing.qll +++ b/ql/src/semmle/go/frameworks/Testing.qll @@ -40,3 +40,31 @@ module TestCase { } } } + +/** + * A file that contains test cases or is otherwise used for testing. + * + * Extend this class to refine existing models of testing frameworks. If you want to model new + * frameworks, extend `TestFile::Range` instead. + */ +class TestFile extends File { + TestFile::Range self; + + TestFile() { this = self } +} + +/** Provides classes for working with test files. */ +module TestFile { + /** + * A file that contains test cases or is otherwise used for testing. + * + * Extend this class to model new testing frameworks. If you want to refine existing models, + * extend `TestFile` instead. + */ + abstract class Range extends File { } + + /** A file containing at least one test case. */ + private class FileContainingTestCases extends Range { + FileContainingTestCases() { this = any(TestCase tc).getFile() } + } +} From e30e5685b2be0578b77f5a1eacdce57f0e9e4657 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 9 Apr 2020 09:40:25 +0100 Subject: [PATCH 2/5] Fix recognition of `Test`, `Benchmark`, and `Example` as test cases. --- ql/src/semmle/go/frameworks/Testing.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ql/src/semmle/go/frameworks/Testing.qll b/ql/src/semmle/go/frameworks/Testing.qll index 022b74b5db4..47a65346402 100644 --- a/ql/src/semmle/go/frameworks/Testing.qll +++ b/ql/src/semmle/go/frameworks/Testing.qll @@ -27,15 +27,15 @@ module TestCase { /** A `go test` style test (including benchmarks and examples). */ private class GoTestFunction extends Range, FuncDef { GoTestFunction() { - getName().regexpMatch("Test[^a-z].*") and + getName().regexpMatch("Test(?![a-z]).*") and getNumParameter() = 1 and getParameter(0).getType().(PointerType).getBaseType().hasQualifiedName("testing", "T") or - getName().regexpMatch("Benchmark[^a-z].*") and + getName().regexpMatch("Benchmark(?![a-z]).*") and getNumParameter() = 1 and getParameter(0).getType().(PointerType).getBaseType().hasQualifiedName("testing", "B") or - getName().regexpMatch("Example[^a-z].*") and + getName().regexpMatch("Example(?![a-z]).*") and getNumParameter() = 0 } } From d5c8570bfca8c3124617c9d81a549c24666e1b54 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 9 Apr 2020 09:40:53 +0100 Subject: [PATCH 3/5] Recognise imports of well-known testing frameworks. --- ql/src/semmle/go/frameworks/Testing.qll | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ql/src/semmle/go/frameworks/Testing.qll b/ql/src/semmle/go/frameworks/Testing.qll index 47a65346402..e2940dd14d7 100644 --- a/ql/src/semmle/go/frameworks/Testing.qll +++ b/ql/src/semmle/go/frameworks/Testing.qll @@ -67,4 +67,19 @@ module TestFile { private class FileContainingTestCases extends Range { FileContainingTestCases() { this = any(TestCase tc).getFile() } } + + /** A file that imports a well-known testing framework. */ + private class FileImportingTestingFramework extends Range { + FileImportingTestingFramework() { + exists(string pkg, ImportSpec is | + is.getPath() = pkg and + is.getFile() = this + | + pkg = "net/http/httptest" or + pkg = "gen/thrifttest" or + pkg = "github.com/onsi/ginkgo" or + pkg = "github.com/onsi/gomega" + ) + } + } } From 1bf835f1566f94f6dbee4357b1bb793c3a753307 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 9 Apr 2020 09:41:02 +0100 Subject: [PATCH 4/5] Add tests. --- .../ClassifyFiles/ClassifyFiles.expected | 2 ++ .../filters/ClassifyFiles/TestCase.expected | 6 ++++ .../filters/ClassifyFiles/TestCase.ql | 4 +++ .../filters/ClassifyFiles/httptest.go | 5 +++ .../filters/ClassifyFiles/test1.go | 31 +++++++++++++++++++ 5 files changed, 48 insertions(+) create mode 100644 ql/test/query-tests/filters/ClassifyFiles/TestCase.expected create mode 100644 ql/test/query-tests/filters/ClassifyFiles/TestCase.ql create mode 100644 ql/test/query-tests/filters/ClassifyFiles/httptest.go create mode 100644 ql/test/query-tests/filters/ClassifyFiles/test1.go diff --git a/ql/test/query-tests/filters/ClassifyFiles/ClassifyFiles.expected b/ql/test/query-tests/filters/ClassifyFiles/ClassifyFiles.expected index b0173466116..378d4fc0882 100644 --- a/ql/test/query-tests/filters/ClassifyFiles/ClassifyFiles.expected +++ b/ql/test/query-tests/filters/ClassifyFiles/ClassifyFiles.expected @@ -1,2 +1,4 @@ | hello2.go:0:0:0:0 | hello2.go | generated | | hello.go:0:0:0:0 | hello.go | generated | +| httptest.go:0:0:0:0 | httptest.go | test | +| test1.go:0:0:0:0 | test1.go | test | diff --git a/ql/test/query-tests/filters/ClassifyFiles/TestCase.expected b/ql/test/query-tests/filters/ClassifyFiles/TestCase.expected new file mode 100644 index 00000000000..84ef0a244e4 --- /dev/null +++ b/ql/test/query-tests/filters/ClassifyFiles/TestCase.expected @@ -0,0 +1,6 @@ +| test1.go:5:1:5:26 | function declaration | +| test1.go:7:1:7:27 | function declaration | +| test1.go:15:1:15:31 | function declaration | +| test1.go:17:1:17:32 | function declaration | +| test1.go:25:1:25:17 | function declaration | +| test1.go:27:1:27:18 | function declaration | diff --git a/ql/test/query-tests/filters/ClassifyFiles/TestCase.ql b/ql/test/query-tests/filters/ClassifyFiles/TestCase.ql new file mode 100644 index 00000000000..6eea0fa0355 --- /dev/null +++ b/ql/test/query-tests/filters/ClassifyFiles/TestCase.ql @@ -0,0 +1,4 @@ +import go + +from TestCase tc +select tc diff --git a/ql/test/query-tests/filters/ClassifyFiles/httptest.go b/ql/test/query-tests/filters/ClassifyFiles/httptest.go new file mode 100644 index 00000000000..4ae19dc8d9c --- /dev/null +++ b/ql/test/query-tests/filters/ClassifyFiles/httptest.go @@ -0,0 +1,5 @@ +package main + +import "net/http/httptest" + +func setup(server *httptest.Server) {} diff --git a/ql/test/query-tests/filters/ClassifyFiles/test1.go b/ql/test/query-tests/filters/ClassifyFiles/test1.go new file mode 100644 index 00000000000..2139c420ca9 --- /dev/null +++ b/ql/test/query-tests/filters/ClassifyFiles/test1.go @@ -0,0 +1,31 @@ +package main + +import "testing" + +func Test(t *testing.T) {} // test case + +func Test2(t *testing.T) {} // test case + +func TestMustHaveSingleParameter(t *testing.T, b bool) {} // not a test case + +func TestParameterMustHaveCorrectType(b *testing.B) {} // not a test case + +func TestsDoNotLookLikeThis(t *testing.T) {} // not a test case + +func Benchmark(b *testing.B) {} // test case + +func Benchmark2(b *testing.B) {} // test case + +func BenchmarkMustHaveSingleParameter(b *testing.B, flag bool) {} // not a test case + +func BenchmarkParameterMustHaveCorrectType(t *testing.T) {} // not a test case + +func BenchmarksDoNotLookLikeThis(b *testing.B) {} // not a test case + +func Example() {} // test case + +func Example2() {} // test case + +func ExampleMustNotHaveParameter(t *testing.T) {} // not a test case + +func ExamplesDoNotLookLikeThis() {} // not a test case From d344687f5289adfc033076828986c1e1e812f8a9 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 9 Apr 2020 09:41:09 +0100 Subject: [PATCH 5/5] Add change note. --- change-notes/1.24/analysis-go.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 4ee3612d750..627330bda5c 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -4,8 +4,9 @@ * Alert suppression can now be done with single-line block comments (`/* ... */`) as well as line comments (`// ...`). * Analysis of flow through fields has been improved. -* More sources of untrusted input as well as vulnerable sinks are modelled, which may lead to more results from the security queries. +* Detection of test code has been improved. LGTM will not show alerts in test code by default. * Go 1.14 library changes have been modeled. +* More sources of untrusted input as well as vulnerable sinks are modelled, which may lead to more results from the security queries. ## New queries