T09 · Insecure Skill Coding Practices
- Location
SKILL.md:456- Finding
Unsafe SQL Interpolation Recommended by PHP 8 Migration Templates
- Content
View full analysis
Vulnerability Details
File Location:
SKILL.md, lines 456-493; additional concrete examples appear at lines 796, 820, 847, and 1126
Vulnerability Type: SQL injection through direct interpolation of unvalidated values
Risk Level: HighThe Skill recommends inserting array values directly into SQL statements as part of its PHP 8 compatibility migration procedure.
php // Recommended numeric-field syntax $empire->query("select ... where classid={$r['classid']} and id={$r['id']}"); $empire->query("update ... set classid={$add['classid']}"); // Recommended INSERT syntax $empire->query( "insert into {$dbtbpre}enewsztinfo(ztid,cid,classid,id,newstime,mid,isgood) " . "values('$ztid','$zcid',{$r['classid']},{$r['id']},{$r['newstime']},'$mid','0');" );Additional concrete migration examples use the same unsafe pattern:
php $finfor = $empire->fetch1( "select " . ReturnSqlFtextF($mid) . " from {$dbtbpre}ecms_" . $tbname . "_data_" . $r['stb'] . " where id={$r['id']} limit 1" ); $empire->query( "update {$dbtbpre}ecms_" . $tbname . " set onclick=onclick+1 where id={$add['id']} limit 1" ); $next_r = $empire->fetch1( "select isurl,titleurl,classid,id,title from {$dbtbpre}ecms_" . $class_r[$add['classid']]['tbname'] . " FORCE INDEX (idx_classid_id_asc) where id>{$add['id']} " . "and classid={$add['classid']} order by id limit 1" );Technical Analysis
The migration correctly identifies that unquoted PHP array keys are incompatible with PHP 8, but its proposed SQL repair directly interpolates values such as
$r['id'],$r['classid'],$r['newstime'], and$add['id']without parameter binding, integer conversion, or explicit validation.Curly-brace interpolation only resolves PHP string parsing. It does not provide SQL escaping or establish that a value is numeric. If any interpolated value is derived from an HTTP request, i ...[truncated 2246 chars]
- Remediation
View remediation
Remediation Suggestions
- Replace the documented direct-interpolation pattern with prepared statements and bound parameters wherever the database abstraction layer supports them.
php $stmt = $database->prepare( 'SELECT ... WHERE classid = :classid AND id = :id' ); $stmt->execute([ ':classid' => (int) $r['classid'], ':id' => (int) $r['id'], ]);- If the legacy EmpireCMS database API cannot use prepared statements, validate and cast every numeric value before constructing the query:
php $id = (int) ($r['id'] ?? 0); $classid = (int) ($r['classid'] ?? 0); $empire->query( "select ... where classid=$classid and id=$id" );- Validate identifiers against their expected domain. Reject missing, negative, zero, or out-of-range values where they are not valid:
php $id = filter_var($r['id'] ?? null, FILTER_VALIDATE_INT, [ 'options' => ['min_range' => 1], ]); if ($id === false) { throw new InvalidArgumentException('Invalid content identifier'); }-
Continue to quote and escape string fields according to the database driver's rules, preferably through bound parameters. Do not treat curly-brace interpolation as an escaping mechanism.
-
For dynamic table names such as
$tbnameor$r['stb'], use a strict allowlist because table identifiers generally cannot be protected using ordinary value placeholders:
php if (!in_array($tbname, $allowedTableNames, true)) { throw new InvalidArgumentException('Invalid table name'); }-
Update every similar example in
SKILL.md, including the concrete examples around lines 796, 820, 847, and 1126, so Agents are not given conflicting secure and insecure guidance. -
Add a mandatory migration verification step that traces the source of every SQL value and performs security tests using malformed numeric input. Review the final code with static analysis and application-level SQL injection testing.
...[truncated 215 chars]