Skip to content

Improve mysql2 instrumentation#948

Open
timokoessler wants to merge 9 commits intomainfrom
fix-mysql2
Open

Improve mysql2 instrumentation#948
timokoessler wants to merge 9 commits intomainfrom
fix-mysql2

Conversation

@timokoessler
Copy link
Member

@timokoessler timokoessler commented Feb 26, 2026

  • Fix pool instrumentation
  • Fix context confusion when using pool under higher concurrency
  • Fix missing instrumentation of .prepare function

Summary by Aikido

Security Issues: 0 🔍 Quality Issues: 3 Resolved Issues: 0

⚡ Enhancements

  • Added .prepare instrumentation and unified SQL argument extraction logic
  • Added mysql2 v3.18 test dependency and runtime version warning

🐛 Bugfixes

  • Fixed pool instrumentation to preserve context under higher concurrency

🔧 Refactors

  • Refactored prototype selection and split connection/pool function instructions
  • Pinned firewall-tester-action in CI workflow to specific commit

More info

@codecov
Copy link

codecov bot commented Feb 26, 2026

Codecov Report

❌ Patch coverage is 97.19101% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
library/sinks/MySQL2.ts 97.19% 5 Missing ⚠️

📢 Thoughts on this report? Let us know!

) as typeof import("mysql2-v3.12/promise");
) as typeof import("mysql2-v3.18/promise");

if (major >= 3 && minor >= 12) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Version check major >= 3 && minor >= 12 misclassifies higher major versions (e.g., 4.0) as outdated due to impossible semver ordering assumptions.

Details

✨ AI Reasoning
​The code is trying to determine whether a mysql2 version is new enough to skip outdated-version expectations. However, the condition combines major and minor with a strict AND in a way that only works for major version 3. If a future version has a higher major but a lower minor (for example 4.0), this branch incorrectly treats it as too old. That creates definitively wrong behavior in the test logic based solely on the condition itself.

🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info


// Not possible to fix in old version because of circular dependency issues:
// https://github.com/sidorares/node-mysql2/pull/3081
if (major >= 3 && minor >= 12) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The major >= 3 && minor >= 12 gate is logically wrong for semver and will skip this branch for newer major versions like 4.x.

Details

✨ AI Reasoning
​This block is intended to run for sufficiently new mysql2 versions, but the same major/minor condition is used again. Because it requires minor >= 12 even when major is already above 3, the code excludes valid newer versions from this path. The control-flow condition itself guarantees incorrect classification for some version values.

🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant