V2.8.42#911
Merged
Merged
Conversation
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prepares the 2.8.42 release by bumping version references across the project, adding the v2.8.42 release notes, and optimizing --dumpdir CSV export by skipping particularly expensive sys/information/performance schema tables (including some AWS RDS-related tables).
Changes:
- Bump version to
2.8.42in core script and documentation/version markers. - Optimize
dump_csv_files()to skip heavy tables/views during--dumpdirexports and log what gets skipped. - Add release notes for
v2.8.42and update the changelog entry for the new version (currently incomplete).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
USAGE.md |
Updates displayed version to 2.8.42. |
releases/v2.8.42.md |
Adds release notes for v2.8.42 (note: contains an accuracy issue about perltidy). |
README.md |
Updates the version badge/link to v2.8.42. |
POTENTIAL_ISSUES |
Renames an audit section to v2.8.42 (but content appears to correspond to v2.8.41). |
mysqltuner.pl |
Bumps tunerversion and updates --dumpdir logic to skip expensive tables/views (minor formatting cleanup needed). |
MEMORY_DB.md |
Updates “Current Version” and relabels “Recent Audits” to v2.8.42 (but the listed items appear to be v2.8.41 content). |
CURRENT_VERSION.txt |
Updates current version to 2.8.42. |
Changelog |
Adds a 2.8.42 header, but the entry doesn’t yet reflect the actual changes in this PR. |
Comments suppressed due to low confidence (1)
mysqltuner.pl:11250
- This block has the same trailing whitespace after
orand also inconsistent indentation inside theif(theinfoprint/nextare indented further than the surrounding code). Please run perltidy or normalize the indentation so the dumpdir skip logic matches the project’s formatting.
if ( $info_s_table =~ /INNODB_BUFFER_PAGE/ or
$info_s_table =~ /RDS_CONTROL_PERFORMANCE_INSIGHTS_STATUS/ or
$info_s_table =~ /RDS_METRICS_COUNTER/ or
$info_s_table =~ /RDS_METRICS_GAUGE/) {
infoprint("SKIPPING $info_s_table");
next;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11228
to
+11230
| for my $sys_view ( select_array('use sys;show tables;') ) { | ||
| if ( $sys_view =~ /innodb_buffer_stats/ ) { | ||
| if ( $sys_view =~ /innodb_buffer_stats/ or | ||
| $sys_view =~ /schema_table_statistics_with_buffer/ ) { |
Comment on lines
151
to
154
| ## [2026-04-13 Audit] Development v2.8.42 | ||
|
|
||
| ### [v2.8.41] Laboratory Verification | ||
| ### [v2.8.42] Laboratory Verification | ||
|
|
|
|
||
| ### Recent Audits | ||
| - **v2.8.41**: | ||
| - **v2.8.42**: |
|
|
||
| 2.8.42 2026-05-17 | ||
|
|
||
| - chore: bump version to 2.8.42 |
| - **CVE Database Refresh**: Massive update to `vulnerabilities.csv`, adding 69 new vulnerability definitions and updating existing mappings for better security diagnostics. | ||
| - **Feature Inventory Sync**: Updated `FEATURES.md` to accurately reflect the full set of implemented features after running automated discovery. | ||
| - **Dumpdir Performance Boost**: Significantly accelerated `--dumpdir` analysis by excluding resource-intensive internal tables (e.g., `INNODB_BUFFER_PAGE`) and AWS-specific metrics (`RDS_METRICS_*`, `RDS_CONTROL_*`) that often cause timeouts in managed environments. | ||
| - **Code Maintenance**: Applied `perltidy` to `mysqltuner.pl` to ensure consistent formatting and adhere to project style guidelines. |
…date documentation and tests
jmrenouard
added
bug
enhancement
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
tests/test_issue_900.t (1)
55-56: ⚡ Quick winFixed temp filenames can make tests race each other.
Using deterministic filenames under
tests/is great for nostalgia, less great for parallel runs and flaky cleanup. PreferFile::Tempfor unique, auto-cleaned files.Also applies to: 104-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_issue_900.t` around lines 55 - 56, Replace the fixed test-state filename with a unique temporary file created via File::Temp: instead of setting $temp_state_file = File::Spec->catfile('tests', '.mysqltuner_test_state'), use File::Temp (e.g., File::Temp->new or tempfile()) to create a temp file and assign its filename to $temp_state_file so each test gets a unique path and the file is auto-removed; update other occurrences that reference $temp_state_file (the same spots around the earlier and later uses) to use the temp file handle or its filename and ensure proper cleanup by letting File::Temp scope/UNLINK handle removal.build/release_gen.py (1)
68-75: ⚡ Quick winBare
except:clauses mask all exceptions.The naked
except:blocks at lines 68, 74, 121, and 128 will catch everything - includingKeyboardInterrupt,SystemExit, and typos in your variable names. I've debugged enough "why isn't my script responding to Ctrl-C" mysteries to know this pattern.Since you're clearly catching
subprocess.CalledProcessErrorfrom the git commands, make it explicit:- except: + except subprocess.CalledProcessError:That said, for a build script that needs to degrade gracefully, this is more of a code hygiene suggestion than a blocker. The fallback messages are reasonable.
Also applies to: 119-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/release_gen.py` around lines 68 - 75, Replace the naked "except:" handlers around the git subprocess calls with explicit exception classes so you don't swallow system-level exceptions; specifically catch subprocess.CalledProcessError and FileNotFoundError (and optionally OSError) for the blocks that call subprocess.check_output (the code working with prev_tag and commits via git describe/log), and apply the same explicit catches to the other similar try/except blocks later in the file (the blocks around subprocess.check_output that produce the "Initial release or no previous tag found." and related fallback messages). Keep the existing fallback return strings but only return them when the expected subprocess/file errors are raised, re-raising or letting through KeyboardInterrupt/SystemExit and truly unexpected exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Changelog`:
- Around line 3-10: The Changelog entry for version 2.8.42 shows date 2026-05-25
which conflicts with the PR description that says 2026-05-17; confirm the
authoritative release date and make a consistent change: if the official release
date is 2026-05-25, update the PR description to 2026-05-25; if the official
date is 2026-05-17, update the Changelog entry in "Changelog" (version header
for 2.8.42) to 2026-05-17 and also correct the date inside releases/v2.8.42.md
so both the Changelog and releases/v2.8.42.md match the confirmed release date
for version 2.8.42.
In `@documentation/AUTHENTICATION_PLUGINS.md`:
- Line 10: Update the documentation row for the `mysql_old_password` entry in
AUTHENTICATION_PLUGINS.md: change the algorithm label from "SHA-1 (Old)" to
"legacy pre-4.1 hash" and ensure `mysql_native_password` keeps the
"double-SHA-1" wording so the table accurately distinguishes the legacy pre-4.1
hashing from the double-SHA-1 native method.
In `@documentation/REFERENCES.md`:
- Line 35: Replace the filesystem URI in the REFERENCES.md entry for
"**MySQLTuner-perl Specification**" (the link currently using
"file:///documentation/AUTHENTICATION_PLUGINS.md") with a repo-relative Markdown
link to documentation/AUTHENTICATION_PLUGINS.md (e.g. [Naming Conventions &
Style Compatibility](documentation/AUTHENTICATION_PLUGINS.md)) so the link
resolves correctly in rendered docs; update the link target text if needed but
keep the same visible label.
In `@mysqltuner.pl`:
- Around line 3048-3050: The current open( my $fh, '>', $state_file ) call
writes a predictable /tmp path and can follow symlinks; fix by writing to a
private temporary file and atomically renaming it into place (or at minimum
reject symlinks) before writing: check that $state_file is not a symlink (using
Perl's -l or lstat) and then create a temp file in the same directory (e.g.
using File::Spec->catfile(dirname($state_file), ...) or File::Temp), set its
mode to 0600, write the "$uptime:$total_attempts\n" to that temp file via the
$fh used in the diff, close it and then rename() the temp file to $state_file to
replace it atomically; ensure any intermediate failures unlink the temp file.
- Around line 4539-4557: The badprint call in the password-check loop (inside
the block using $target_hash, `@passwords`, `@variants` and computing $computed)
prints the matched dictionary password ($v), leaking credentials; change the
message passed to badprint (and similar calls around the other matches) to
indicate a dictionary/weak password was matched for User '$user'\@'$host'
without including $v, and keep the push to `@generalrec` but ensure no variable
containing the plaintext candidate (e.g., $v or any element from
`@passwords/`@variants) is interpolated into logs, reports, or dump artifacts;
apply the same change to the other occurrences noted (the other badprint calls
around the 4601-4613 and 4687-4698 regions).
- Around line 2835-2836: The current adjust_aborted_connects() call mutates
%mystat before assigning it to $result{'Status'}, which changes upstream counter
names/values; instead, preserve raw upstream counters by removing the in-place
mutation: ensure %mystat is copied unchanged into $result{'Status'} and have
adjust_aborted_connects() return a computed/derived value (or write its output
into a separate key such as $result{'Derived'}->{'Aborted_connects_adjusted'})
so callers can use the normalized metric without altering the original
Aborted_connects or Connections fields; apply the same change to the other call
site that invokes adjust_aborted_connects() (the block around the second
invocation).
- Around line 2998-3011: get_state_file_path currently builds the state filename
only from $opt{host}, $opt{port}, and $opt{socket} which causes different
SSH/container targets that proxy to 127.0.0.1:3306 to collide; update
get_state_file_path to incorporate transport-specific identity (e.g., append an
SSH identifier like $opt{ssh_host} or a container identifier when running in
container mode) into $filename before returning it so state files are unique per
target/transport while still sanitizing any added values the same way as
$host/$port/$socket.
In `@tests/auth_plugin_checks.t`:
- Around line 48-49: The current tests use the postfix "ok(... ) for
`@main`::secrec" which performs zero assertions when `@main`::secrec is empty;
change them to first assert the array is non-empty (e.g. ok(scalar `@main`::secrec
> 0, "... has entries")) and then iterate explicitly (foreach my $s
(`@main`::secrec) { ok($s =~ /Migrate to 'caching_sha2_password'/, 'Recommendation
for caching_sha2_password') }) so each element is checked and at least one
assertion runs; apply the same pattern to the other similar check(s) referencing
`@main`::secrec and ensure the consolidated check against `@main`::generalrec still
uses is(scalar(grep { ... } `@main`::generalrec), 1, ...) after confirming the
source arrays are non-empty.
In `@tests/test_issue_900.t`:
- Line 19: Replace the hard-coded require './mysqltuner.pl' in
tests/test_issue_900.t with a path-stable require that builds an absolute path
relative to the test file (e.g., use FindBin or File::Spec to compute the test
file's directory and then require the mysqltuner.pl from that computed path);
update the require call (the one containing './mysqltuner.pl') so the test no
longer depends on being run from the repo root.
---
Nitpick comments:
In `@build/release_gen.py`:
- Around line 68-75: Replace the naked "except:" handlers around the git
subprocess calls with explicit exception classes so you don't swallow
system-level exceptions; specifically catch subprocess.CalledProcessError and
FileNotFoundError (and optionally OSError) for the blocks that call
subprocess.check_output (the code working with prev_tag and commits via git
describe/log), and apply the same explicit catches to the other similar
try/except blocks later in the file (the blocks around subprocess.check_output
that produce the "Initial release or no previous tag found." and related
fallback messages). Keep the existing fallback return strings but only return
them when the expected subprocess/file errors are raised, re-raising or letting
through KeyboardInterrupt/SystemExit and truly unexpected exceptions.
In `@tests/test_issue_900.t`:
- Around line 55-56: Replace the fixed test-state filename with a unique
temporary file created via File::Temp: instead of setting $temp_state_file =
File::Spec->catfile('tests', '.mysqltuner_test_state'), use File::Temp (e.g.,
File::Temp->new or tempfile()) to create a temp file and assign its filename to
$temp_state_file so each test gets a unique path and the file is auto-removed;
update other occurrences that reference $temp_state_file (the same spots around
the earlier and later uses) to use the temp file handle or its filename and
ensure proper cleanup by letting File::Temp scope/UNLINK handle removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb3974a2-4ae5-4602-9279-6a0baac4861c
📒 Files selected for processing (17)
COMMIT_AND_RELEASE.mdCURRENT_VERSION.txtChangelogFEATURES.mdMEMORY_DB.mdPOTENTIAL_ISSUESREADME.mdROADMAP.mdUSAGE.mdbuild/release_gen.pydocumentation/AUTHENTICATION_PLUGINS.mddocumentation/REFERENCES.mdmysqltuner.plreleases/v2.8.42.mdtests/auth_plugin_checks.ttests/sql_modeling.ttests/test_issue_900.t
Comment on lines
+3
to
+10
| 2.8.42 2026-05-25 | ||
|
|
||
| - chore: bump version to 2.8.42. | ||
| - fix: resolve fake aborted connections count increase during password strength checks (#900). | ||
| - fix: resolve EOF metadata corruption and duplicated configurations in Emacs block (#904). | ||
| - ci: optimize release notes generation to isolate branch changes. | ||
| - perf: optimize --dumpdir performance by excluding heavy RDS/Aurora and internal metrics. | ||
|
|
There was a problem hiding this comment.
Date discrepancy in release metadata.
The Changelog entry is dated 2026-05-25, but the PR description states "Release 2.8.42 dated 2026-05-17". I've seen enough version-bump commits to know this usually means someone copy-pasted the wrong date somewhere.
Which date is the official release date? If it's 2026-05-25, the PR description should be updated. If it's 2026-05-17, this Changelog date and the corresponding releases/v2.8.42.md date need correction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Changelog` around lines 3 - 10, The Changelog entry for version 2.8.42 shows
date 2026-05-25 which conflicts with the PR description that says 2026-05-17;
confirm the authoritative release date and make a consistent change: if the
official release date is 2026-05-25, update the PR description to 2026-05-25; if
the official date is 2026-05-17, update the Changelog entry in "Changelog"
(version header for 2.8.42) to 2026-05-17 and also correct the date inside
releases/v2.8.42.md so both the Changelog and releases/v2.8.42.md match the
confirmed release date for version 2.8.42.
| | Plugin Name | Description | Algorithm | Security Level | Deprecated / Obsolete | Present in MySQL | Present in MariaDB | | ||
| | :--- | :--- | :--- | :--- | :--- | :--- | :--- | | ||
| | `mysql_native_password` | Historical default authentication method. | SHA-1 | Low | Yes (Removed from MySQL 8.4+ / Deprecated in MariaDB) | ✅ (Obsolete) | ✅ (Historical default) | | ||
| | `mysql_old_password` | Ancient pre-4.1 authentication method. | SHA-1 (Old) | Very Low | Yes (Removed) | ❌ | ✅ (Obsolete) | |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What hashing algorithm does the MySQL/MariaDB mysql_old_password plugin use, and is it SHA-1 or a legacy pre-4.1 hash format?
💡 Result:
The mysql_old_password plugin uses a legacy, insecure pre-4.1 password hashing algorithm, not SHA-1 [1][2][3]. This algorithm produces a 16-byte hash value [1][4][5]. In contrast, the newer 4.1+ authentication method (mysql_native_password) utilizes a double-SHA-1 hashing process and produces a 41-byte hash value [1][4][6]. The mysql_old_password plugin and its associated pre-4.1 hashing method are considered obsolete and insecure [2][3][7]. Support for this plugin and the pre-4.1 hash format was removed in MySQL 5.7.5 [1][5]. While it remains available in some environments for backward compatibility, it is strongly recommended to migrate to more secure authentication plugins such as caching_sha2_password or ed25519 [2][8][9].
Citations:
- 1: https://dev.mysql.com/doc/refman/5.7/en/password-hashing.html
- 2: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-mysql_old_password
- 3: https://dev.mysql.com/doc/refman/5.7/en/old-native-pluggable-authentication.html
- 4: https://www.dbi-services.com/blog/the-evolution-of-mysql-authentication-mechanism/
- 5: https://docs.oracle.com/cd/E17952_01/mysql-5.7-en/password-hashing.html
- 6: https://mariadb.org/history-of-mysql-mariadb-authentication-protocols/
- 7: https://dev.mysql.com/doc/refman/5.7/en/account-upgrades.html
- 8: https://github.com/mariadb-corporation/mariadb-docs/blob/main/server/reference/plugins/authentication-plugins/authentication-plugin-mysql_old_password.md
- 9: https://dev.mysql.com/doc/refman/9.1/en/sha256-pluggable-authentication.html
Fix documentation: mysql_old_password is not SHA-1 (it’s pre-4.1 legacy hashing).
Line 10 labels mysql_old_password as SHA-1 (Old), but that plugin uses the legacy pre-4.1 password hashing algorithm (not SHA-1). Adjust the table wording to “legacy pre-4.1 hash” (and reserve “double-SHA-1” for mysql_native_password) so the security story isn’t told with the wrong algorithm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@documentation/AUTHENTICATION_PLUGINS.md` at line 10, Update the documentation
row for the `mysql_old_password` entry in AUTHENTICATION_PLUGINS.md: change the
algorithm label from "SHA-1 (Old)" to "legacy pre-4.1 hash" and ensure
`mysql_native_password` keeps the "double-SHA-1" wording so the table accurately
distinguishes the legacy pre-4.1 hashing from the double-SHA-1 native method.
| - **MySQL**: [Primary Key Optimization](https://dev.mysql.com/doc/refman/8.4/en/optimizing-primary-keys.html) - Surrogate keys, UUID indexing, and index traversal efficiency. | ||
| - **MySQL**: [JSON Datatype Indexing](https://dev.mysql.com/doc/refman/8.4/en/create-table-secondary-indexes.html#json-column-indirect-index) - Secondary indexing via virtual generated columns. | ||
| - **MariaDB**: [Invisible Indexes KB](https://mariadb.com/kb/en/invisible-indexes/) - Hiding indexes to test query planner changes. | ||
| - **MySQLTuner-perl Specification**: [Naming Conventions & Style Compatibility](file:///documentation/AUTHENTICATION_PLUGINS.md) - Summary of naming styles. |
There was a problem hiding this comment.
file:/// link will 404 in rendered docs
Line 35 uses a local filesystem URI, so readers get a broken jump instead of your carefully curated wisdom. Use a repo-relative markdown link.
Suggested fix
-- **MySQLTuner-perl Specification**: [Naming Conventions & Style Compatibility](file:///documentation/AUTHENTICATION_PLUGINS.md) - Summary of naming styles.
+- **MySQLTuner-perl Specification**: [Naming Conventions & Style Compatibility](./AUTHENTICATION_PLUGINS.md) - Summary of naming styles.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **MySQLTuner-perl Specification**: [Naming Conventions & Style Compatibility](file:///documentation/AUTHENTICATION_PLUGINS.md) - Summary of naming styles. | |
| - **MySQLTuner-perl Specification**: [Naming Conventions & Style Compatibility](./AUTHENTICATION_PLUGINS.md) - Summary of naming styles. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@documentation/REFERENCES.md` at line 35, Replace the filesystem URI in the
REFERENCES.md entry for "**MySQLTuner-perl Specification**" (the link currently
using "file:///documentation/AUTHENTICATION_PLUGINS.md") with a repo-relative
Markdown link to documentation/AUTHENTICATION_PLUGINS.md (e.g. [Naming
Conventions & Style Compatibility](documentation/AUTHENTICATION_PLUGINS.md)) so
the link resolves correctly in rendered docs; update the link target text if
needed but keep the same visible label.
Comment on lines
+2835
to
2836
| adjust_aborted_connects(); | ||
| $result{'Status'} = \%mystat; |
There was a problem hiding this comment.
Keep upstream counters raw; normalize in derived fields instead.
adjust_aborted_connects() rewrites Aborted_connects and Connections in %mystat before they are stored in $result{'Status'}. That makes the exported status section diverge from actual SHOW STATUS output, which is going to surprise any DBA or downstream parser who expects the real counters rather than the haunted version. Keep the raw values intact and compute an adjusted metric separately.
As per coding guidelines, "MySQL/MariaDB/Percona variable names and status counters must match upstream exactly."
Also applies to: 3029-3036
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mysqltuner.pl` around lines 2835 - 2836, The current
adjust_aborted_connects() call mutates %mystat before assigning it to
$result{'Status'}, which changes upstream counter names/values; instead,
preserve raw upstream counters by removing the in-place mutation: ensure %mystat
is copied unchanged into $result{'Status'} and have adjust_aborted_connects()
return a computed/derived value (or write its output into a separate key such as
$result{'Derived'}->{'Aborted_connects_adjusted'}) so callers can use the
normalized metric without altering the original Aborted_connects or Connections
fields; apply the same change to the other call site that invokes
adjust_aborted_connects() (the block around the second invocation).
Comment on lines
+2998
to
+3011
| sub get_state_file_path { | ||
| my $tmpdir = $ENV{TEMP} || $ENV{TMP} || '/tmp'; | ||
| my $host = $opt{host} || 'localhost'; | ||
| my $port = $opt{port} || '3306'; | ||
| my $socket = $opt{socket} || ''; | ||
|
|
||
| $host =~ s/[^a-zA-Z0-9_\.-]/_/g; | ||
| $port =~ s/[^a-zA-Z0-9_\.-]/_/g; | ||
| $socket =~ s/[^a-zA-Z0-9_\.-]/_/g; | ||
|
|
||
| my $filename = ".mysqltuner_${host}_${port}"; | ||
| $filename .= "_${socket}" if $socket ne ''; | ||
|
|
||
| return File::Spec->catfile( $tmpdir, $filename ); |
There was a problem hiding this comment.
State-file identity collapses different SSH/container targets into one bucket.
This key only uses host, port, and socket. In SSH/container mode those often fall back to the same 127.0.0.1:3306, so different instances reuse the same state file and inherit each other’s failed-attempt baseline. The result is delightfully wrong aborted-connect math. Include transport-specific identity such as ssh-host and container in the filename.
Suggested fix
sub get_state_file_path {
my $tmpdir = $ENV{TEMP} || $ENV{TMP} || '/tmp';
my $host = $opt{host} || 'localhost';
my $port = $opt{port} || '3306';
my $socket = $opt{socket} || '';
+ my $ssh = $opt{'ssh-host'} || '';
+ my $ctr = $opt{'container'} || '';
$host =~ s/[^a-zA-Z0-9_\.-]/_/g;
$port =~ s/[^a-zA-Z0-9_\.-]/_/g;
$socket =~ s/[^a-zA-Z0-9_\.-]/_/g;
+ $ssh =~ s/[^a-zA-Z0-9_\.-]/_/g;
+ $ctr =~ s/[^a-zA-Z0-9_\.-]/_/g;
my $filename = ".mysqltuner_${host}_${port}";
$filename .= "_${socket}" if $socket ne '';
+ $filename .= "_ssh_${ssh}" if $ssh ne '';
+ $filename .= "_ctr_${ctr}" if $ctr ne '';
return File::Spec->catfile( $tmpdir, $filename );
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mysqltuner.pl` around lines 2998 - 3011, get_state_file_path currently builds
the state filename only from $opt{host}, $opt{port}, and $opt{socket} which
causes different SSH/container targets that proxy to 127.0.0.1:3306 to collide;
update get_state_file_path to incorporate transport-specific identity (e.g.,
append an SSH identifier like $opt{ssh_host} or a container identifier when
running in container mode) into $filename before returning it so state files are
unique per target/transport while still sanitizing any added values the same way
as $host/$port/$socket.
Comment on lines
+3048
to
+3050
| if ( open( my $fh, '>', $state_file ) ) { | ||
| print $fh "$uptime:$total_attempts\n"; | ||
| close($fh); |
There was a problem hiding this comment.
Please don’t open('>') a predictable file in /tmp and hope for the best.
Writing the state file through a stable temp-path name will happily follow a symlink. For a script that is often run with elevated privileges, that is a rather expensive trust exercise. Write to a private temp file with 0600 permissions and rename() it into place, or at least reject symlinks before opening.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mysqltuner.pl` around lines 3048 - 3050, The current open( my $fh, '>',
$state_file ) call writes a predictable /tmp path and can follow symlinks; fix
by writing to a private temporary file and atomically renaming it into place (or
at minimum reject symlinks) before writing: check that $state_file is not a
symlink (using Perl's -l or lstat) and then create a temp file in the same
directory (e.g. using File::Spec->catfile(dirname($state_file), ...) or
File::Temp), set its mode to 0600, write the "$uptime:$total_attempts\n" to that
temp file via the $fh used in the diff, close it and then rename() the temp file
to $state_file to replace it atomically; ensure any intermediate failures unlink
the temp file.
Comment on lines
+4539
to
+4557
| my $target_hash = uc($auth_string); | ||
| my $found_weak = 0; | ||
| foreach my $pass (@passwords) { | ||
| $pass =~ s/\s//g; | ||
| chomp($pass); | ||
| my @variants = ( $pass, uc($pass), ucfirst($pass) ); | ||
| foreach my $v (@variants) { | ||
| my $computed = '*' | ||
| . uc( | ||
| Digest::SHA::sha1_hex( | ||
| Digest::SHA::sha1($v) | ||
| ) | ||
| ); | ||
| if ( $computed eq $target_hash ) { | ||
| badprint | ||
| "User '$user'\@'$host' is using weak password: $v (checked offline)"; | ||
| push( @generalrec, | ||
| "Set up a Secure Password for '$user'\@'$host' user." | ||
| ); |
There was a problem hiding this comment.
Don’t print the password you just proved is valid.
These badprint calls echo the matched dictionary value into stdout, --outputfile, reports, and dumpdir artifacts. That turns a weak-password check into a credential leak with extra steps. Report that a dictionary password matched; don’t hand the password back like a souvenir.
Suggested fix
- badprint
-"User '$user'\@'$host' is using weak password: $v (checked offline)";
+ badprint
+"User '$user'\@'$host' is using a weak password from the configured dictionary (checked offline)";
@@
- badprint
-"User '$target_user' is using weak password: $v";
+ badprint
+"User '$target_user' is using a weak password from the configured dictionary";
@@
- badprint
-"User '$target_user' is using weak password: $v";
+ badprint
+"User '$target_user' is using a weak password from the configured dictionary";Also applies to: 4601-4613, 4687-4698
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mysqltuner.pl` around lines 4539 - 4557, The badprint call in the
password-check loop (inside the block using $target_hash, `@passwords`, `@variants`
and computing $computed) prints the matched dictionary password ($v), leaking
credentials; change the message passed to badprint (and similar calls around the
other matches) to indicate a dictionary/weak password was matched for User
'$user'\@'$host' without including $v, and keep the push to `@generalrec` but
ensure no variable containing the plaintext candidate (e.g., $v or any element
from `@passwords/`@variants) is interpolated into logs, reports, or dump
artifacts; apply the same change to the other occurrences noted (the other
badprint calls around the 4601-4613 and 4687-4698 regions).
Comment on lines
+48
to
+49
| ok(grep(/Migrate to 'caching_sha2_password'/, $_), 'Recommendation for caching_sha2_password') for @main::secrec; | ||
| is(scalar(grep { $_ eq "Migrate to 'caching_sha2_password' for 1 user(s)" } @main::generalrec), 1, 'Consolidated recommendation pushed to generalrec'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find assertion patterns that can execute zero checks.
rg -nP "ok\\(grep\\([^\\n]+\\)\\s*,[^\\n]+\\)\\s*for\\s+\`@main`::secrec" tests/auth_plugin_checks.tRepository: major/MySQLTuner-perl
Length of output: 308
Fix zero-iteration test assertions on empty @main::secrec (lines 48-49, also 91-92)
ok(... ) for @main::secrec runs zero checks when @main::secrec is empty, so the subtest can pass while doing absolutely nothing (Perl’s “nothing to see here” magic).
Suggested fix
- ok(grep(/Migrate to 'caching_sha2_password'/, $_), 'Recommendation for caching_sha2_password') for `@main`::secrec;
+ ok(grep({ /Migrate to 'caching_sha2_password'/ } `@main`::secrec), 'Recommendation for caching_sha2_password');
- ok(grep(/Migrate to 'ed25519' or 'unix_socket'/, $_), 'Recommendation for ed25519/unix_socket') for `@main`::secrec;
+ ok(grep({ /Migrate to 'ed25519' or 'unix_socket'/ } `@main`::secrec), 'Recommendation for ed25519/unix_socket');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/auth_plugin_checks.t` around lines 48 - 49, The current tests use the
postfix "ok(... ) for `@main`::secrec" which performs zero assertions when
`@main`::secrec is empty; change them to first assert the array is non-empty (e.g.
ok(scalar `@main`::secrec > 0, "... has entries")) and then iterate explicitly
(foreach my $s (`@main`::secrec) { ok($s =~ /Migrate to 'caching_sha2_password'/,
'Recommendation for caching_sha2_password') }) so each element is checked and at
least one assertion runs; apply the same pattern to the other similar check(s)
referencing `@main`::secrec and ensure the consolidated check against
`@main`::generalrec still uses is(scalar(grep { ... } `@main`::generalrec), 1, ...)
after confirming the source arrays are non-empty.
| { | ||
| local @ARGV = (); | ||
| no warnings 'redefine'; | ||
| require './mysqltuner.pl'; |
There was a problem hiding this comment.
Use a path-stable require for mysqltuner.pl.
Line 19 hard-codes ./mysqltuner.pl, so this test only works when launched from repo root. Future you (or CI variants) may enjoy a surprise failure.
Suggested fix
{
local `@ARGV` = ();
no warnings 'redefine';
- require './mysqltuner.pl';
+ my $script_dir = dirname(File::Spec->rel2abs(__FILE__));
+ require File::Spec->catfile($script_dir, '..', 'mysqltuner.pl');
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_issue_900.t` at line 19, Replace the hard-coded require
'./mysqltuner.pl' in tests/test_issue_900.t with a path-stable require that
builds an absolute path relative to the test file (e.g., use FindBin or
File::Spec to compute the test file's directory and then require the
mysqltuner.pl from that computed path); update the require call (the one
containing './mysqltuner.pl') so the test no longer depends on being run from
the repo root.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
2.8.42 2026-05-25