Add Psalm taint annotations for SQL injection analysis #78
Conversation
Reviewer's GuideAdds Psalm taint analysis annotations to SQL query and execution interfaces/implementations so that calls performing parameterized SQL are treated as escaping the Sequence diagram for taint-escaped SQL execution via SqlQuery and PerformSqlsequenceDiagram
actor User
participant InputSource
participant SqlQuery
participant PerformSql as PerformSqlInterface_impl
participant PDO as ExtendedPdoInterface
participant PDOStatement
User->>InputSource: provide_untrusted_parameters()
InputSource->>SqlQuery: getRow(sqlId, values)
SqlQuery->>PerformSql: perform(pdo, sqlId, sql, values)
PerformSql->>PDO: prepare_and_execute(sql, values)
PDO-->>PerformSql: PDOStatement
PerformSql-->>SqlQuery: PDOStatement
SqlQuery-->>InputSource: result_row
Class diagram for SQL query and execution interfaces with Psalm taint-escape annotationsclassDiagram
class SqlQueryInterface {
+getRow(string sqlId, array values, FetchInterface fetch) array|object|null
+getRowList(string sqlId, array values, FetchInterface fetch) array
+exec(string sqlId, array values, FetchInterface fetch) void
+getCount(string sqlId, array values) int
+getPages(string sqlId, array values, int perPage, string queryTemplate, string entity) PagesInterface
<<psalm_taint_escape_sql>> getRow
<<psalm_taint_escape_sql>> getRowList
<<psalm_taint_escape_sql>> exec
<<psalm_taint_escape_sql>> getCount
<<psalm_taint_escape_sql>> getPages
}
class SqlQuery {
+exec(string sqlId, array values, FetchInterface fetch) void
+getRow(string sqlId, array values, FetchInterface fetch) array|object|null
+getRowList(string sqlId, array values, FetchInterface fetch) array
+getCount(string sqlId, array values) int
+getPages(string sqlId, array values, int perPage, string queryTemplate, string entity) PagesInterface
<<psalm_taint_escape_sql>> exec
<<psalm_taint_escape_sql>> getRow
<<psalm_taint_escape_sql>> getRowList
<<psalm_taint_escape_sql>> getCount
<<psalm_taint_escape_sql>> getPages
}
SqlQueryInterface <|.. SqlQuery
class PerformSqlInterface {
+perform(ExtendedPdoInterface pdo, string sqlId, string sql, array values) PDOStatement
<<psalm_taint_escape_sql>> perform
}
class PerformSql {
+perform(ExtendedPdoInterface pdo, string sqlId, string sql, array values) PDOStatement
<<psalm_taint_escape_sql>> perform
}
class PerformTemplatedSql {
+perform(ExtendedPdoInterface pdo, string sqlId, string sql, array values) PDOStatement
<<psalm_taint_escape_sql>> perform
}
PerformSqlInterface <|.. PerformSql
PerformSqlInterface <|.. PerformTemplatedSql
class ExtendedPdoInterface
class PDOStatement
class FetchInterface
class PagesInterface
SqlQuery --> PerformSqlInterface
PerformSqlInterface --> ExtendedPdoInterface
PerformSqlInterface --> PDOStatement
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Double-check whether these methods should be marked as
@psalm-taint-escape sqlon the whole function (i.e. all their outputs) versus more granular annotations on specific parameters/return values, since they are primarily sinks for tainted input rather than sanitizers that produce safe SQL strings. - For
PerformTemplatedSql::perform()in particular, verify that all templating paths enforce bound parameters and never perform string interpolation into the SQL query; if any templating mode allows raw concatenation, this method should not be globally marked as@psalm-taint-escape sql.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check whether these methods should be marked as `@psalm-taint-escape sql` on the whole function (i.e. all their outputs) versus more granular annotations on specific parameters/return values, since they are primarily *sinks* for tainted input rather than sanitizers that produce safe SQL strings.
- For `PerformTemplatedSql::perform()` in particular, verify that all templating paths enforce bound parameters and never perform string interpolation into the SQL query; if any templating mode allows raw concatenation, this method should not be globally marked as `@psalm-taint-escape sql`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Add @psalm-taint-escape sql annotations to indicate that values passed to SQL query methods are safely escaped via prepared statements. Annotated classes: - PerformSqlInterface::perform() - interface declaration - PerformSql::perform() - prepared statement execution - PerformTemplatedSql::perform() - templated SQL execution - SqlQueryInterface - all query methods (getRow, getRowList, exec, getCount, getPages) - SqlQuery - implementation of all query methods These annotations enable Psalm's taint analysis to understand that user input flowing through these methods is properly escaped when used with prepared statements.
koriym
force-pushed
the
add-psalm-taint-annotations
branch
from
cae33b1 to
bde10bf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/PerformTemplatedSql.php (1)
22-30:@psalm-taint-escape sqlannotation is misleading—method concatenates non-tainted values, not escapes taint.The method doesn't escape SQL input via
str_replace(). It concatenates application-controlled values:$sqlIdis an internal identifier (never user input) and$sqlis file content. While this is safe, the annotation suggests active escaping when the real safety comes from these values being non-tainted by design.Recommended fix: Replace the annotation with parameter-level taint sink declarations:
/** * @psalm-taint-escape sql * @psalm-taint-sink sql $sqlId * @psalm-taint-sink sql $sql */This documents that
$sqlIdand$sqlmust never contain tainted user input, which is the actual security requirement.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/PerformSql.phpsrc/PerformSqlInterface.phpsrc/PerformTemplatedSql.phpsrc/SqlQuery.phpsrc/SqlQueryInterface.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (12)
src/SqlQuery.php (5)
66-83: Annotation looks correct assuming trusted SQL files and parameterized values.The
@psalm-taint-escape sqlannotation is appropriate for the same reasons asexec().
85-97: Annotation looks correct assuming trusted SQL files and parameterized values.The
@psalm-taint-escape sqlannotation is appropriate for the same reasons as the other query methods.
99-108: Annotation looks correct assuming trusted SQL files and parameterized values.The
@psalm-taint-escape sqlannotation is appropriate sinceExtendedPdoAdaptershould use parameterized queries for the$valuesarray.
202-217: Annotation looks correct assuming trusted SQL files and parameterized values.The
@psalm-taint-escape sqlannotation is appropriate here as the pager factory and Pages class should use parameterized queries.
55-64: @psalm-taint-escape sql annotation is correct; path traversal risk with $sqlId is mitigated by design.The annotation appropriately marks SQL escaping since PDO prepared statements with parameter binding handle the
$valuessafely. Regarding the path traversal concern:$sqlIdis directly interpolated into the file path at line 117 without validation, making../sequences technically possible (e.g.,$sqlId = '../../../etc/passwd'would attempt to read/etc/passwd.sql). However, this risk is mitigated by the architecture—$sqlIdis hardcoded within application code (seen across tests:'todo_add','todo_item', etc.), not sourced from user input or external configuration. Additionally, only.sqlfiles can be accessed due to the hardcoded.sqlextension in the path format. The design assumes trusted SQL identifiers, which is reasonable in this context.src/PerformSqlInterface.php (1)
12-24: Interface annotation aligns with implementations.The
@psalm-taint-escape sqlannotation on the interface is consistent with the annotations added to implementing classes (PerformSqlandPerformTemplatedSql).Note: The semantic correctness of this annotation depends on all implementations properly escaping SQL. See concerns raised in
PerformTemplatedSql.phpreview.src/PerformSql.php (1)
13-21: Annotation is appropriate for PDO prepared statement execution.The
@psalm-taint-escape sqlannotation is correct here since$pdo->perform()uses PDO prepared statements that safely bind the$valuesarray parameters.Note: This assumes
$sqlcontains only SQL with placeholders, not user-controlled content directly concatenated into the SQL string. Based on the codebase,$sqlcomes from trusted.sqlfiles, which makes this safe.src/SqlQueryInterface.php (5)
9-16: Interface annotation aligns with implementation.The
@psalm-taint-escape sqlannotation is consistent with the implementation inSqlQuery.php.
18-25: Interface annotation aligns with implementation.The
@psalm-taint-escape sqlannotation is consistent with the implementation inSqlQuery.php.
27-32: Interface annotation aligns with implementation.The
@psalm-taint-escape sqlannotation is consistent with the implementation inSqlQuery.php.
34-39: Interface annotation aligns with implementation.The
@psalm-taint-escape sqlannotation is consistent with the implementation inSqlQuery.php.
41-47: Interface annotation aligns with implementation.The
@psalm-taint-escape sqlannotation is consistent with the implementation inSqlQuery.php.
Summary
Add Psalm taint annotations to enable static security analysis for SQL injection detection.
Changes
@psalm-taint-escape sqlon:PerformSqlInterface::perform()- Interface declarationPerformSql::perform()- Prepared statement execution via Aura.SqlPerformTemplatedSql::perform()- Templated SQL executionSqlQueryInterfacemethods (getRow,getRowList,exec,getCount,getPages)SqlQueryimplementation of all query methodsThe escape annotation indicates that values passed to these methods are safely escaped via PDO prepared statements with parameter binding, preventing SQL injection attacks.
Test Plan
./vendor/bin/psalm --taint-analysisto verify annotations workSummary by Sourcery
Annotate SQL execution and query methods with Psalm taint-escape metadata for SQL injection analysis.
New Features:
Enhancements:
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.