fix: the mysql manager utility constructs sql querie... in...#28262
fix: the mysql manager utility constructs sql querie... in...#28262orbisai0security wants to merge 2 commits into
Conversation
Automated security fix generated by OrbisAI Security
WalkthroughThis PR adds centralized escaping helpers (SQL identifier, SQL string, shell argument) and updates MySQLManager to use them across database CREATE/DROP, test-database cleanup, snapshot create/delete/restore, and settings updates (site_uuid and Stripe keys), replacing raw interpolation and ad-hoc quote handling. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@e2e/helpers/environment/service-managers/mysql-manager.ts`:
- Around line 95-97: The file still concatenates untrusted variables into shell
commands and SQL strings (e.g., in dropAllTestDatabases()/the SELECT query,
CREATE DATABASE, DROP DATABASE, the mysqldump call, restore logic, and the
UPDATE that sets siteUuid), leaving SQL and command-injection vectors; replace
all shell-based mysql/mysqldump usage with the mysql2 Promise client and use
parameterized queries (execute with ? placeholders) for
SELECT/CREATE/DROP/UPDATE, and for dumps/restores invoke mysqldump/restore via
safe APIs that avoid shell interpolation (or use the client to export/import
data), ensuring every variable (databaseName, sourceDatabase, outputPath,
snapshotPath, siteUuid) is never concatenated into a shell command or raw SQL
string. Ensure functions mentioned (dropAllTestDatabases, dropDatabases,
createDatabase, dropDatabase, any dump/restore helpers, and the UPDATE that sets
siteUuid) are refactored to use mysql2 connections and parameter binding instead
of template literals and this.exec.
- Line 95: The current escaping of DEV_PRIMARY_DATABASE into escapedDevPrimaryDb
uses backslash escapes (replace(/'/g, "\\'")) which is fragile across
shell/MySQL modes; change it to standard SQL quote-doubling by replacing single
quotes with two single quotes (e.g., replace(/'/g, "''")) or, even better, stop
manual escaping entirely and use a MySQL client with parameterized queries
(replace the code that builds raw SQL strings using escapedDevPrimaryDb with
parameter binding via your client library), updating any call sites that
reference escapedDevPrimaryDb accordingly.
🪄 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: 791391ff-8de2-48ef-86f2-9dfc17183ac1
📒 Files selected for processing (1)
e2e/helpers/environment/service-managers/mysql-manager.ts
|
✅ Review Feedback Addressed I've automatically addressed 3 review comment(s): The reviewer identified two main issues:
The fix adds three private helper methods:
These helpers are then applied consistently across all methods that construct SQL queries or shell commands. Files modified:
The changes have been pushed to this PR branch. Please review! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/helpers/environment/service-managers/mysql-manager.ts (1)
182-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't put Stripe keys on the error-message path.
This command still embeds both Stripe keys verbatim, and
execInContainer()includescommandin the thrown error. If the insert fails,setupTestDatabase()will log those secrets into CI output.🤖 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 `@e2e/helpers/environment/service-managers/mysql-manager.ts` around lines 182 - 193, The command currently embeds Stripe secrets (escapedSecretKey, escapedPublishableKey) into the `command` string used by `execInContainer()` (called from `setupTestDatabase()`), which can cause secrets to be logged on error; instead, stop concatenating the secret values into `command` and pass them into MySQL without putting them in the logged command—for example, write the INSERT SQL to a temporary file or feed it via STDIN inside the container and supply the secrets via environment variables or stdin (not via the `command` string), or use a here-doc that exports the secrets inside the container before running `mysql`; update the code around `escapeSqlString`/`escapedSecretKey`/`escapedPublishableKey`/`escapedDatabase` and the `command` construction so the produced `command` no longer contains the secret values and only references non-sensitive environment variable names or reads from a temp file/stdin when calling `execInContainer()`.
🤖 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 `@e2e/helpers/environment/service-managers/mysql-manager.ts`:
- Line 114: The mysqldump/mysql restore commands in createSnapshot() and
restoreDatabaseFromSnapshot() currently pass the database name as a positional
arg which can be interpreted as an option if it starts with “-”; update both
methods in mysql-manager.ts to harden input by (a) validating the database
identifier (create/introduce/ reuse a helper like escapeSqlIdentifier() that
rejects any name starting with '-' and only allows safe chars) and (b) add a
'--' end-of-options marker immediately before the escaped database argument in
the exec invocations (e.g. ... mysqldump [opts] -- --escapedDbName ...) so even
if validation misses something the CLI won’t treat the DB name as an option.
Ensure you reference and update the exec calls in createSnapshot() and
restoreDatabaseFromSnapshot() and use existing escapeShellArg() only for quoting
after validation.
---
Outside diff comments:
In `@e2e/helpers/environment/service-managers/mysql-manager.ts`:
- Around line 182-193: The command currently embeds Stripe secrets
(escapedSecretKey, escapedPublishableKey) into the `command` string used by
`execInContainer()` (called from `setupTestDatabase()`), which can cause secrets
to be logged on error; instead, stop concatenating the secret values into
`command` and pass them into MySQL without putting them in the logged
command—for example, write the INSERT SQL to a temporary file or feed it via
STDIN inside the container and supply the secrets via environment variables or
stdin (not via the `command` string), or use a here-doc that exports the secrets
inside the container before running `mysql`; update the code around
`escapeSqlString`/`escapedSecretKey`/`escapedPublishableKey`/`escapedDatabase`
and the `command` construction so the produced `command` no longer contains the
secret values and only references non-sensitive environment variable names or
reads from a temp file/stdin when calling `execInContainer()`.
🪄 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: c4feb501-5183-4113-8a1b-1635638566aa
📒 Files selected for processing (1)
e2e/helpers/environment/service-managers/mysql-manager.ts
| logging.info('Creating database snapshot...'); | ||
|
|
||
| await this.exec(`mysqldump -uroot -proot --opt --single-transaction ${sourceDatabase} > ${outputPath}`); | ||
| await this.exec('mysqldump -uroot -proot --opt --single-transaction ' + this.escapeShellArg(sourceDatabase) + ' > ' + this.escapeShellArg(outputPath)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target file and relevant sections
FILE="e2e/helpers/environment/service-managers/mysql-manager.ts"
echo "== File exists? =="
ls -l "$FILE" || true
echo "== Lines around 100-150 =="
nl -ba "$FILE" | sed -n '80,170p'
echo "== Grep for escapeSqlIdentifier usage in file =="
rg -n "escapeSqlIdentifier" "$FILE" || true
echo "== Grep for escapeShellArg usage in file =="
rg -n "escapeShellArg" "$FILE" || true
echo "== Locate escapeSqlIdentifier implementation (by searching repo) =="
rg -n "escapeSqlIdentifier\\(" -S . --glob='*.ts' --glob='*.tsx' | head -n 50
echo "== Locate escapeShellArg implementation (by searching repo) =="
rg -n "escapeShellArg\\(" -S . --glob='*.ts' --glob='*.tsx' | head -n 50Repository: TryGhost/Ghost
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="e2e/helpers/environment/service-managers/mysql-manager.ts"
echo "== Lines around 100-150 =="
# print with simple line numbers via awk
awk 'NR>=80 && NR<=170 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,120p'
echo "== Grep for escapeSqlIdentifier usage in file =="
rg -n "escapeSqlIdentifier" "$FILE" || true
echo "== Grep for escapeShellArg usage in file =="
rg -n "escapeShellArg" "$FILE" || true
echo "== Locate escapeSqlIdentifier implementation (repo-wide, TS/TSX) =="
rg -n "function escapeSqlIdentifier|escapeSqlIdentifier\\s*\\(" -S . --glob='*.ts' --glob='*.tsx' | head -n 50 || true
echo "== Locate escapeShellArg implementation (repo-wide, TS/TSX) =="
rg -n "function escapeShellArg|escapeShellArg\\s*\\(" -S . --glob='*.ts' --glob='*.tsx' | head -n 50 || trueRepository: TryGhost/Ghost
Length of output: 6570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="e2e/helpers/environment/service-managers/mysql-manager.ts"
echo "== Lines around escapeSqlIdentifier/escapeShellArg (195-260) =="
awk 'NR>=190 && NR<=260 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,120p'Repository: TryGhost/Ghost
Length of output: 3263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "createSnapshot\\(" -S e2e/helpers/environment/service-managers/mysql-manager.ts e2e | head -n 50
rg -n "restoreDatabaseFromSnapshot\\(" -S e2e/helpers/environment/service-managers/mysql-manager.ts e2e | head -n 50
rg -n "dropDatabase\\(" -S e2e/helpers/environment/service-managers/mysql-manager.ts e2e | head -n 50
rg -n "new MySQLManager|MySQLManager" -S e2e | head -n 50Repository: TryGhost/Ghost
Length of output: 2517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="e2e/helpers/environment/environment-manager.ts"
echo "== Lines around call sites in $FILE (50-120) =="
awk 'NR>=1 && NR<=140 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,140p'
FILE2="e2e/helpers/environment/service-managers/mysql-manager.ts"
echo "== Lines around MySQLManager usage and databaseName flow (1-100) =="
awk 'NR>=1 && NR<=110 {printf "%d:%s\n", NR, $0}' "$FILE2" | sed -n '1,110p'Repository: TryGhost/Ghost
Length of output: 10100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\\.createSnapshot\\(" -S e2e . --glob='*.ts' --glob='*.tsx'
rg -n "\\.restoreDatabaseFromSnapshot\\(" -S e2e . --glob='*.ts' --glob='*.tsx'Repository: TryGhost/Ghost
Length of output: 534
Harden mysqldump/mysql snapshot & restore against option-looking database names
In e2e/helpers/environment/service-managers/mysql-manager.ts, createSnapshot() (line 114) and restoreDatabaseFromSnapshot() (line 135) pass the database as a positional argument using escapeShellArg(). Shell quoting doesn’t prevent mysql/mysqldump from interpreting arguments that start with -- as options (quotes are removed by sh -c before the MySQL CLI sees them). Current call sites use safe names (ghost_e2e_base and ghost_e2e_<uuid>), but these methods should still validate/guard the input (e.g., use stricter escapeSqlIdentifier() validation that rejects leading -, and/or add a -- end-of-options marker before the database argument).
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 114-114: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@e2e/helpers/environment/service-managers/mysql-manager.ts` at line 114, The
mysqldump/mysql restore commands in createSnapshot() and
restoreDatabaseFromSnapshot() currently pass the database name as a positional
arg which can be interpreted as an option if it starts with “-”; update both
methods in mysql-manager.ts to harden input by (a) validating the database
identifier (create/introduce/ reuse a helper like escapeSqlIdentifier() that
rejects any name starting with '-' and only allows safe chars) and (b) add a
'--' end-of-options marker immediately before the escaped database argument in
the exec invocations (e.g. ... mysqldump [opts] -- --escapedDbName ...) so even
if validation misses something the CLI won’t treat the DB name as an option.
Ensure you reference and update the exec calls in createSnapshot() and
restoreDatabaseFromSnapshot() and use existing escapeShellArg() only for quoting
after validation.
|
This is only used for e2e tests. Please do not open this again. |
2 participants
Summary
Fix critical severity security issue in
e2e/helpers/environment/service-managers/mysql-manager.ts.Vulnerability
V-001e2e/helpers/environment/service-managers/mysql-manager.ts:95Description: The MySQL manager utility constructs SQL queries and shell commands using string concatenation and template literals with unsanitized variables. At line 187, a database name is directly interpolated into a shell command executing MySQL, and at line 95, a variable is interpolated into a SQL query without parameterization.
Evidence
Exploitation scenario: Manipulate the database name variable (via environment configuration or test parameters) to inject SQL commands.
Scanner confirmation: multi_agent_ai rule
V-001flagged this pattern.Production code: This file is in the production codebase, not test-only code.
Threat Model Context
This is a Node.js library - vulnerabilities affect downstream consumers who use this package.
Changes
e2e/helpers/environment/service-managers/mysql-manager.tsVerification
Automated security fix by OrbisAI Security