Improve SQL check extensibility, add support for pgx #834
Conversation
Remove hardwired assumption and heuristics on index of arg taking a SQL string, be explicit about it instead.
| @@ -4,6 +4,9 @@ package tools | |||
|
|
|||
| // nolint | |||
| import ( | |||
| _ "github.com/jackc/pgconn" | |||
There was a problem hiding this comment.
a blank import should be only in a main or test package, or have a comment justifying it
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
==========================================
- Coverage 74.35% 74.05% -0.31%
==========================================
Files 50 50
Lines 3124 3134 +10
==========================================
- Hits 2323 2321 -2
- Misses 735 743 +8
- Partials 66 70 +4
Continue to review full report at Codecov.
|
|
Marking as a draft, because the second commit is work in progress; otherwise complete but lacking in test coverage. Before completing it, I'd like to verify if the pgx support, once complete here, is something you'd welcome in gosec. |
|
Thanks for this contribution. I would keep the pgx stuff out of gosec since the tool is focused on standard Go package. Thanks for understanding. |
|
Sure. It's unfortunate though, as there doesn't seem to be anything else one could easily use or extend to craft these checks for non-stdlib packages. Anyway, I think the refactoring commit a18c3a0 still has value on its own. If you agree, I can submit it as another PR. |
|
@scop Thanks, I left some comments in the commit. I don't see it as a major improvement but rather a bit more complicated and not so obvious. If you can make it a bit more clear, keeping in mind that someone needs maybe at some point extend the method list, you can submit a PR. |
|
The whole point of that commit is to make it easier for someone at some point to extend the method list. Before my commit, one has to modify two places in the code, and if the function to be added has the query in argument index > 1, or doesn't conform to the "ends with Context => +1" heuristic, the current code wouldn't work for that at all. After that commit, one just adds the function name and its interesting argument index to sqlCallIdents, and it's done. Note that my second commit actually shows this in action: 567d75a -> just addition to the map and it's covered, no matter what the argument indexes are, no other code changes needed. |
|
Thanks for clarifications! Please send that commit in a different pull request and close this one. Thanks again! |
|
Done: #841 |
The SQL checks currently contain a number of hardwired/heuristic assumptions about the index of the argument taking an SQL string. The first commit here generalizes that.
The second one adds support for checking the SQL issues against the popular https://github.com/jackc/pgx family of PostgreSQL drivers and tools.