Expand SQLi sinks #41
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR expands SQL injection detection to additional Go SQL packages by adding stubs, tests, and updating the analyzer and README.
- Added mock implementations and test cases for xorm, go-pg, rqlite, squirrel, sqlx, etc.
- Extended
injectableSQLMethodsandimportslogic to cover more SQL libraries and improved query-argument handling. - Updated README to list newly supported packages and fixed minor typos.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/injection/testdata/src/xorm.io/xorm/mock.go | Added xorm engine and session stubs for new SQLi tests |
| sql/injection/testdata/src/[n–v]/main.go | Introduced test apps triggering SQL sinks in various packages |
| sql/injection/injection.go | Expanded sinks list, improved imports recursion and arg logic |
| sql/injection/injection_test.go | Added TestN–TestV for new testdata directories |
| README.md | Listed additional supported SQL packages and fixed typos |
Comments suppressed due to low confidence (1)
README.md:84
- [nitpick] For better readability, sort the list of supported SQL packages alphabetically or group them by category (e.g., drivers, ORM libraries).
Supported SQL packages include:
| var userControlledValues = taint.NewSources( | ||
| // Function (and method) calls | ||
| // Function (and method) calls that are user controlled | ||
| // over the netork. These are all taken into account as |
There was a problem hiding this comment.
Fix spelling of 'netork' to 'network'.
| // over the netork. These are all taken into account as | |
| // over the network. These are all taken into account as |
| if strings.HasSuffix(imp.Path(), pkg) { | ||
| imported = true | ||
| break | ||
| if strings.HasSuffix(p.Path(), pkg) { |
There was a problem hiding this comment.
[nitpick] Consider using exact import path matching (e.g., p.Path() == pkg) instead of suffix matching to avoid accidental false positives when package paths share common suffixes.
| if strings.HasSuffix(p.Path(), pkg) { | |
| if p.Path() == pkg { |
| "(*github.com/jinzhu/gorm.DB).Raw", | ||
| "(*github.com/jinzhu/gorm.DB).Exec", | ||
| "(*github.com/jinzhu/gorm.DB).Order", | ||
| // GORM v2 |
There was a problem hiding this comment.
[nitpick] The injectableSQLMethods list is growing large and repetitive; consider grouping method patterns per package or generating this list programmatically to simplify future maintenance.
|
|
||
| // Skip the context argument, if using a *Context query variant. | ||
| if strings.HasPrefix(queryEdge.Site.Value().Call.Value.String(), "Context") { | ||
| if strings.HasSuffix(queryEdge.Site.Value().Call.Value.String(), "Context") { |
There was a problem hiding this comment.
[nitpick] Relying on string suffix matching may be brittle; consider inspecting the method's name or signature directly (e.g., using Signature().Name()) to accurately detect Context variants like QueryContext, ExecContext, etc.
| if strings.HasSuffix(queryEdge.Site.Value().Call.Value.String(), "Context") { | |
| if strings.Contains(queryEdge.Site.Common().Signature().Name(), "Context") { |
Summary
Testing
go fmt ./...go test ./...https://chatgpt.com/codex/tasks/task_e_6867eda11a8483319c076a37c0dc5edf