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 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..e2940dd14d7 100644 --- a/ql/src/semmle/go/frameworks/Testing.qll +++ b/ql/src/semmle/go/frameworks/Testing.qll @@ -27,16 +27,59 @@ 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 } } } + +/** + * 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() } + } + + /** 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" + ) + } + } +} 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