stripe-archive / safesql

Static analysis tool for Golang that protects against SQL injections
MIT License
564 stars 47 forks source link

Adds ability to ignore false positives #16

Closed codyl-stripe closed 4 years ago

codyl-stripe commented 4 years ago

Summary

This addresses https://github.com/stripe/safesql/issues/1 and now allows users to override a false positive by providing a comment //nolint:safesql.

When a statement is ignored, it will still be logged but will not cause the analyzer to exit with an exit code of 1 if all found statements are ignored.

Testing

I created a simple file of

package testdata

import (
    "database/sql"
    "fmt"
)

func main() {
    fmt.Println(query("'test' OR 1=1"))
}

const GetAllQuery = "SELECT COUNT(*) FROM t WHERE arg=%s"

func query(arg string) error {
    db, err := sql.Open("postgres", "postgresql://test:test@test")
    if err != nil {
        return err
    }

    query := fmt.Sprintf(GetAllQuery, arg)
    //nolint:safesql
    row := db.QueryRow(query)
    var count int
    if err := row.Scan(&count); err != nil {
        return err
    }

    row = db.QueryRow(fmt.Sprintf(GetAllQuery, "Catch me please?")) //nolint:safesql
    if err := row.Scan(&count); err != nil {
        return err
    }

    return nil
}

And ran the linter which now produces the following output and returns a status code of 1:

- /Users/codyl/go/src/github.com/stripe/safesql/testdata/injected.go:22:20 is potentially unsafe but ignored by comment
- /Users/codyl/go/src/github.com/stripe/safesql/testdata/injected.go:28:19 is potentially unsafe but ignored by comment

I also tested with removing 1 of the comments and confirmed that the analyzer does return an exit code of 1.

Further work

In a follow up PR I will split out the code to allow for easier testing, add some tests, and also allow it to be imported into more extensive linters.