Skip to content

fix: the mysql manager utility constructs sql querie... in... #28262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
orbisai0security wants to merge 2 commits into from
+42 −11
Closed

fix: the mysql manager utility constructs sql querie... in... #28262

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f6d7e6
fix: V-001 security vulnerability
orbisai0security May 30, 2026
df296de
Address review feedback (3 comments)
orbisai0security May 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 42 additions & 11 deletions e2e/helpers/environment/service-managers/mysql-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,15 @@ export class MySQLManager {
async createDatabase(databaseName: string): Promise<void> {
debug('Creating database:', databaseName);

await this.exec('mysql -uroot -proot -e "CREATE DATABASE IF NOT EXISTS \\`' + databaseName + '\\`;"');
await this.exec('mysql -uroot -proot -e "CREATE DATABASE IF NOT EXISTS \\`' + this.escapeSqlIdentifier(databaseName) + '\\`;"');

debug('Database created:', databaseName);
}

async dropDatabase(database: string): Promise<void> {
debug('Dropping database if exists:', database);

await this.exec('mysql -uroot -proot -e "DROP DATABASE IF EXISTS \\`' + database + '\\`;"');
await this.exec('mysql -uroot -proot -e "DROP DATABASE IF EXISTS \\`' + this.escapeSqlIdentifier(database) + '\\`;"');

debug('Database dropped (if existed):', database);
}
Expand All @@ -92,7 +92,8 @@ export class MySQLManager {
try {
debug('Finding all test databases to clean up...');

const query = `SELECT schema_name FROM information_schema.schemata WHERE schema_name LIKE 'ghost_%' AND schema_name NOT IN ('ghost_testing', 'ghost_e2e_base', '${DEV_PRIMARY_DATABASE}')`;
const escapedDevPrimaryDb = this.escapeSqlString(DEV_PRIMARY_DATABASE);
const query = "SELECT schema_name FROM information_schema.schemata WHERE schema_name LIKE 'ghost_%' AND schema_name NOT IN ('ghost_testing', 'ghost_e2e_base', '" + escapedDevPrimaryDb + "')";
const output = await this.exec(`mysql -uroot -proot -N -e "${query}"`);

const databaseNames = this.parseDatabaseNames(output);
Expand All @@ -110,7 +111,7 @@ export class MySQLManager {
async createSnapshot(sourceDatabase: string = 'ghost_testing', outputPath: string = '/tmp/dump.sql'): Promise<void> {
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));
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 31, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 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 50

Repository: 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 || true

Repository: 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 50

Repository: 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.


logging.info('Database snapshot created');
}
Expand All @@ -119,7 +120,7 @@ export class MySQLManager {
try {
debug('Deleting MySQL snapshot:', snapshotPath);

await this.exec(`rm -f ${snapshotPath}`);
await this.exec('rm -f ' + this.escapeShellArg(snapshotPath));

debug('MySQL snapshot deleted');
} catch (error) {
Expand All @@ -131,7 +132,7 @@ export class MySQLManager {
async restoreDatabaseFromSnapshot(database: string, snapshotPath: string = '/tmp/dump.sql'): Promise<void> {
debug('Restoring database from snapshot:', database);

await this.exec('mysql -uroot -proot ' + database + ' < ' + snapshotPath);
await this.exec('mysql -uroot -proot ' + this.escapeShellArg(database) + ' < ' + this.escapeShellArg(snapshotPath));

debug('Database restored from snapshot:', database);
}
Expand Down Expand Up @@ -167,8 +168,8 @@ export class MySQLManager {
debug('Updating site_uuid in database settings:', database, siteUuid);

const command = 'mysql -uroot -proot -e "UPDATE \\`' +
database + '\\`.settings SET value=\'' +
siteUuid + '\' WHERE \\`key\\`=\'site_uuid\';"';
this.escapeSqlIdentifier(database) + '\\`.settings SET value=\'' +
this.escapeSqlString(siteUuid) + '\' WHERE \\`key\\`=\'site_uuid\';"';

await this.exec(command);

Expand All @@ -178,13 +179,14 @@ export class MySQLManager {
async updateStripeSettings(database: string, secretKey: string, publishableKey: string): Promise<void> {
debug('Updating Stripe settings in database:', database);

const escapedSecretKey = secretKey.replace(/'/g, '\\\'');
const escapedPublishableKey = publishableKey.replace(/'/g, '\\\'');
const escapedSecretKey = this.escapeSqlString(secretKey);
const escapedPublishableKey = this.escapeSqlString(publishableKey);
const escapedDatabase = this.escapeSqlIdentifier(database);

// Use INSERT ... ON DUPLICATE KEY UPDATE so this works whether or not
// the settings rows already exist. In dev mode the base DB is empty
// (only schema, no seeded rows), so a plain UPDATE would be a no-op.
const command = 'mysql -uroot -proot -e "INSERT INTO \\`' + database + '\\`.settings ' +
const command = 'mysql -uroot -proot -e "INSERT INTO \\`' + escapedDatabase + '\\`.settings ' +
'(id, \\`group\\`, \\`key\\`, value, type, flags, created_at, updated_at) VALUES ' +
'(SUBSTRING(REPLACE(UUID(), \'-\', \'\'), 1, 24), \'members\', \'stripe_secret_key\', \'' + escapedSecretKey + '\', \'string\', NULL, NOW(), NOW()), ' +
'(SUBSTRING(REPLACE(UUID(), \'-\', \'\'), 1, 24), \'members\', \'stripe_publishable_key\', \'' + escapedPublishableKey + '\', \'string\', NULL, NOW(), NOW()) ' +
Expand All @@ -195,6 +197,35 @@ export class MySQLManager {
debug('Stripe settings updated in database');
}

/**
* Validates and returns a safe SQL identifier (database/table/column name).
* Only allows alphanumeric characters, underscores, and hyphens to prevent
* SQL and shell injection through backtick-quoted identifiers.
*/
private escapeSqlIdentifier(name: string): string {
if (!/^[a-zA-Z0-9_-]+$/.test(name)) {
throw new Error(`Unsafe SQL identifier contains invalid characters: ${name}`);
}
return name;
}

/**
* Escapes a SQL string value by doubling single quotes.
* This is the standard SQL escaping method and works regardless of
* the NO_BACKSLASH_ESCAPES SQL mode.
*/
private escapeSqlString(value: string): string {
return value.replace(/'/g, "''");
}

/**
* Escapes a shell argument by wrapping it in single quotes.
* Any single quotes within the argument are properly escaped using the '\'' pattern.
*/
private escapeShellArg(arg: string): string {
return "'" + arg.replace(/'/g, "'\\''") + "'";
}

private async exec(command: string) {
const container = this.docker.getContainer(this.containerName);
return await this.execInContainer(container, command);
Expand Down
Loading