Skip to content

Rust: Model mysql and mysql_async query sinks - #20631

Merged
geoffw0 merged 14 commits into
github:mainfrom
geoffw0:mysql
Oct 16, 2025
Merged

Rust: Model mysql and mysql_async query sinks#20631
geoffw0 merged 14 commits into
github:mainfrom
geoffw0:mysql

Conversation

@geoffw0

Copy link
Copy Markdown
Contributor

Model mysql and mysql_async query sinks (sources will follow in another PR).

CopilotAI review requested due to automatic review settings October 13, 2025 15:33
@geoffw0
geoffw0 requested review from a team as code ownersOctober 13, 2025 15:33
@geoffw0geoffw0 added the Rust Pull requests that update Rust code label Oct 13, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds SQL injection sink models for the mysql and mysql_async Rust libraries to improve security analysis coverage.

  • Introduces sink models for various query methods in both sync and async MySQL libraries
  • Includes comprehensive test coverage demonstrating SQL injection detection
  • Updates documentation to reflect the newly supported frameworks

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
rust/ql/test/query-tests/security/CWE-089/options.ymlAdds mysql and mysql_async dependencies for testing
rust/ql/test/query-tests/security/CWE-089/mysql.rsTest file demonstrating both safe and unsafe MySQL query patterns
rust/ql/test/query-tests/security/CWE-089/SqlInjection.expectedExpected test results showing proper SQL injection detection
rust/ql/lib/codeql/rust/frameworks/mysql.model.ymlSink models for synchronous mysql library query methods
rust/ql/lib/codeql/rust/frameworks/mysql-async.model.ymlSink models for asynchronous mysql_async library query methods
rust/ql/lib/change-notes/2025-10-10-mysql.mdChange note documenting the new library support
docs/codeql/reusables/supported-frameworks.rstDocumentation update listing the new supported frameworks

Comment threadrust/ql/lib/codeql/rust/frameworks/mysql.model.yml Outdated

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.

@geoffw0

Copy link
Copy Markdown
ContributorAuthor

DCA LGTM.

@paldepindpaldepind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Models look good to me. I got thrown off by some of the formatting so I'd suggest that we format the tests with rustfmt to make things more uniform and easier to read.

Comment threadrust/ql/test/query-tests/security/CWE-089/mysql.rs Outdated

// prepared queries (safe)
let stmt = conn.prep(prepared_query.as_str())?; // $ sql-sink
let _ : Vec<i64> = conn.exec(&stmt, (remote_string.as_str(),))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these commas here and below?

 here
v
let _ :Vec<i64> = conn.exec(&stmt,(remote_string.as_str(),))?;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's one of those funny corners of languages - this is required to disambiguate a tuple with one element from ordinary parenthesis in Rust. See here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Thanks.

Co-authored-by: Simon Friis Vindum <paldepind@github.com>
@geoffw0

Copy link
Copy Markdown
ContributorAuthor

I'd suggest that we format the tests with rustfmt to make things more uniform and easier to read.

I'm generally against formatting tests, at least uniformly, as we should aim to have a high diversity of code in tests and auto formatting reduces it. I'm happy to address specific areas of confusion, and perhaps even format whole files in certain cases - but I don't want us to get anywhere close to this being a requirement (in tests).

@geoffw0

Copy link
Copy Markdown
ContributorAuthor

I have formatted just this test file.

paldepind
paldepind previously approved these changes Oct 15, 2025
@paldepind

Copy link
Copy Markdown
Contributor

Thanks. From my point of view, I find auto-formatted code nicer to read and since formatting mostly only affect parsing, and rust_analyzer handles parsing, I'd think it wouldn't be something we'd have to test much. But that's just my preference :)

@geoffw0

Copy link
Copy Markdown
ContributorAuthor

I'd think it wouldn't be something we'd have to test much.

True. It's probably fine if 90% of tests are formatted for ease of reading.

@geoffw0

Copy link
Copy Markdown
ContributorAuthor

@paldepind would you mind re-approving this?

@geoffw0
geoffw0 merged commit 75a34a4 into github:mainOct 16, 2025
19 checks passed
@geoffw0

Copy link
Copy Markdown
ContributorAuthor

Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationRustPull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@geoffw0@paldepind