Rust: Update SqlxQuery, SqlxExecute to use getCanonicalPath #19802
Conversation
|
I managed to make some progress; will continue investigations tomorrow. |
|
Just merged |
| let _ = conn.execute(safe_query_2.as_str()).await?; // $ sql-sink | ||
| let _ = conn.execute(safe_query_3.as_str()).await?; // $ sql-sink | ||
| let _ = conn.execute(unsafe_query_1.as_str()).await?; // $ sql-sink Alert[rust/sql-injection]=args1 | ||
| let _ = conn.execute(safe_query_1.as_str()).await?; // $ MISSING: sql-sink |
There was a problem hiding this comment.
While we are able to infer that conn has type PoolConnection, we cannot currently infer that is in fact has type PoolConnection<MySql> (#19954 or similar is needed for that).
Once we are able to do that, we need implicit dereferencing via
impl<DB: Database> Deref for PoolConnection<DB> {
type Target = DB::Connection;
fn deref(&self) -> &Self::Target {
&self.live.as_ref().expect(EXPECT_MSG).raw
}
}
to get to the type &MySqlConnection, and then via impl<'c> Executor<'c> for &'c mut MySqlConnection we can find execute.
There was a problem hiding this comment.
I've added explicit types to some of the test cases and that is indeed not enough by itself, but it should increase the chances we see a change in test results as improvements are made.
I'm not sure whether we should aim to merge this PR (i.e. accept the regression for now) or wait for type inference to catch up. Guess I'll bring it out of draft and start a DCA run to help decide...
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Rust SQLx framework modeling by migrating from manual class-based implementations to use getCanonicalPath rather than getResolvedPath. The change also transitions from hand-written classes to generated model files using the sink model framework.
- Removes custom
SqlxQueryandSqlxExecuteclasses from the framework implementation - Adds a new model file defining SQL injection sinks for SQLx functions
- Updates test expectations to reflect the new detection patterns with different line numbers and sink identifiers
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/frameworks/Sqlx.qll | Removes the entire custom SQLx implementation file |
| rust/ql/lib/codeql/rust/frameworks/sqlx.model.yml | Adds new model definitions for SQLx SQL injection sinks |
| rust/ql/lib/codeql/rust/Frameworks.qll | Removes import of the now-deleted Sqlx.qll file |
| rust/ql/test/query-tests/security/CWE-089/sqlx.rs | Updates test comments to reflect missing/changed detection behavior |
| rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected | Updates expected test results with new sink patterns and model IDs |
| rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/PathResolutionConsistency.expected | Updates expected line numbers for consistency checks |
|
OK, at this point I feel we need to get this merged and just fix up any remaining issues afterwards. I have just fixed the merge conflict. I'm making a note of regressions in the paths-related-regressions issue. |
Update
SqlxQuery,SqlxExecuteto usegetCanonicalPathrather thangetResolvedPath.At present we lose some results, presumably for similar reasons as we lose similar results in #19268 . I'd prefer we address the issue (and do a DCA run) before merging this.
@hvitved