Repository navigation
fix(safety): exclude PDO method calls from exec() false-positive detection - #28
Conversation
Resolves #19 The exec() danger pattern used a simple word-boundary regex that matched any call whose name ended in 'exec', including PHP PDO method calls such as $pdo->exec() and PDO::exec(). Apply negative look-behinds for '->' and '::' so that only standalone function invocations (the global PHP exec() / Python exec()) are flagged. Method-call forms are safe SQL-execution APIs and must not generate false-positive safety warnings. Changes: - semantic_code_intelligence/llm/safety.py: update exec() pattern - semantic_code_intelligence/tests/test_phase12.py: add 6 regression tests
M9nx
left a comment
There was a problem hiding this comment.
Request changes
Inline comment on semantic_code_intelligence/llm/safety.py line 26:
This lookbehind only excludes ->exec and ::exec when the operator is immediately adjacent to exec. PHP permits whitespace around these operators, so valid calls such as $pdo -> exec($sql) and SomeClass :: exec($sql) are still reported as dynamic code execution. Please make the matching token-aware or normalize whitespace around ->/:: before applying the rule, and add regression tests for both whitespace-separated forms.
The current tests cover the compact forms and pass, but they do not cover this valid PHP formatting case.
|
@M9nx This PR fixes issue #19 — the PHP safety scanner was generating false positives for legitimate PDO method calls like Fix is minimal and safe:
Ready for review and merge. 🙏 |
|
@elhussienysabry Following up on your comment: the compact cases are fixed, but the current regex still flags valid PHP when whitespace surrounds the call operator. POC against commit from semantic_code_intelligence.llm.safety import SafetyValidator
validator = SafetyValidator()
for code in ["$pdo -> exec($sql)", "PDO :: exec($sql)"]:
print(code, validator.is_safe(code))Output: These are still false positives because the lookbehinds only match |
Addresses M9nx review on PR #28. The previous fix used fixed-width negative lookbehinds (?<!->) and (?<!::) that only excluded exec() when the operator was immediately adjacent. PHP permits whitespace around -> and :: so spaced forms such as $pdo -> exec($sql) and SomeClass :: exec($sql) were still being reported as dynamic code execution (false positives). Fix: introduce _PHP_OPERATOR_WS pre-processing in SafetyValidator.validate() that collapses any surrounding whitespace around -> and :: to their compact forms before applying the pattern list. This makes the lookbehinds work correctly for all valid PHP spacing styles. Tests added for whitespace-separated forms: - $pdo -> exec($sql) (arrow with spaces) - $connection -> exec($sql) (arrow with spaces) - PDO :: exec($stmt) (double-colon with spaces) - SomeClass :: exec($sql) (double-colon with spaces) - result = exec(user_input) (bare exec still flagged)
|
Thanks for the review @M9nx! You're absolutely right — the fixed-width lookbehinds didn't handle whitespace around Fix applied in the latest commit: Instead of trying to extend the lookbehinds (which require fixed width in Python's _PHP_OPERATOR_WS = re.compile(r'\s*(->|::)\s*')
# In validate(), per line:
normalised = _PHP_OPERATOR_WS.sub(r'\1', line)This collapses New regression tests added for spaced forms:
All 9 test cases pass. Ready for re-review! 🙏 |
Summary
Fixes #19
The PHP safety scanner was reporting false positives for legitimate PDO method calls like
$pdo->exec()andPDO::exec(), flagging them as dynamic code execution findings.Root Cause
The
exec()danger pattern used a simple word-boundary regex:This matched any
exec(— including PHP PDO method calls like$connection->exec($sql).Fix
Added negative lookbehinds for
->and::so only standalone function calls are flagged:exec('cmd')exec(user_code)$pdo->exec($sql)$connection->exec($sql)PDO::exec($stmt)Files Changed
semantic_code_intelligence/llm/safety.py— 1-line regex fixsemantic_code_intelligence/tests/test_phase12.py— 6 regression tests