Add Psalm taint annotations for SQL injection analysis#89
Conversation
- Add @psalm-taint-sink sql to ExtendedPdoAdapter::__construct() - Add @psalm-taint-sink sql to AuraSqlPagerFactory::newInstance() - Add @psalm-taint-sink sql and @psalm-taint-escape sql to FetchAssoc::__invoke() - Add @psalm-taint-sink sql and @psalm-taint-escape sql to FetchEntity::__invoke() These annotations enable Psalm's taint analysis to detect SQL injection vulnerabilities by tracking tainted data flow from sources to sinks.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds Psalm taint analysis annotations to SQL-related methods to mark SQL query strings as taint sinks and prepared-statement-based fetchers as SQL escape points for better SQL injection detection. Sequence diagram for Psalm SQL taint flow through Pagerfanta adapterssequenceDiagram
actor UserCode
participant AuraSqlPagerFactory
participant ExtendedPdoAdapter
participant FetchAssoc
participant FetchEntity
participant ExtendedPdoInterface
UserCode->>AuraSqlPagerFactory: newInstance(pdo, sql, params, paging, uriTemplate, entity)
Note over AuraSqlPagerFactory: sql parameter is a psalm taint sink (sql)
AuraSqlPagerFactory->>ExtendedPdoAdapter: __construct(pdo, sql, params)
Note over ExtendedPdoAdapter: sql parameter is a psalm taint sink (sql)
alt Fetch associative arrays
ExtendedPdoAdapter->>FetchAssoc: __invoke(sql, params)
Note over FetchAssoc: sql parameter is a psalm taint sink and escape (sql)
FetchAssoc->>ExtendedPdoInterface: executePrepared(sql, params)
ExtendedPdoInterface-->>FetchAssoc: result rows
FetchAssoc-->>ExtendedPdoAdapter: array
else Fetch entities
ExtendedPdoAdapter->>FetchEntity: __invoke(sql, params)
Note over FetchEntity: sql parameter is a psalm taint sink and escape (sql)
FetchEntity->>ExtendedPdoInterface: executePrepared(sql, params)
ExtendedPdoInterface-->>FetchEntity: hydrated entities
FetchEntity-->>ExtendedPdoAdapter: array
end
ExtendedPdoAdapter-->>UserCode: Pagerfanta results
Updated class diagram for Pagerfanta SQL adapter taint-annotated methodsclassDiagram
class AdapterInterface
class FetcherInterface
class AuraSqlPagerInterface
class ExtendedPdoInterface
class ExtendedPdoAdapter {
- ExtendedPdoInterface pdo
- string sql
+ __construct(ExtendedPdoInterface pdo, string sql, array params)
}
class AuraSqlPagerFactory {
- AuraSqlPagerInterface auraSqlPager
+ __construct(AuraSqlPagerInterface auraSqlPager)
+ newInstance(ExtendedPdoInterface pdo, string sql, array params, int paging, string uriTemplate, string entity)
}
class FetchAssoc {
- ExtendedPdoInterface pdo
+ __construct(ExtendedPdoInterface pdo)
+ __invoke(string sql, array params) array
}
class FetchEntity {
- ExtendedPdoInterface pdo
- string entity
+ __construct(ExtendedPdoInterface pdo, string entity)
+ __invoke(string sql, array params) array
}
ExtendedPdoAdapter ..> FetcherInterface : uses
FetchAssoc ..|> FetcherInterface
FetchEntity ..|> FetcherInterface
AuraSqlPagerFactory ..> ExtendedPdoAdapter : creates
FetchAssoc ..> ExtendedPdoInterface : uses
FetchEntity ..> ExtendedPdoInterface : uses
ExtendedPdoAdapter ..> ExtendedPdoInterface : uses
AuraSqlPagerFactory ..> AuraSqlPagerInterface : returns instances
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded Psalm taint-sink docblock annotations ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #89 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 142 142
===========================================
Files 41 41
Lines 505 505
===========================================
Hits 505 505 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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:
- For
FetchAssoc::__invoke()andFetchEntity::__invoke(), consider whether marking$sqlas both@psalm-taint-sink sqland@psalm-taint-escape sqlon the same method matches Psalm’s expectations (sometimes escapes are better modeled on lower-level helper methods or return values rather than on the same signature as the sink). - If
array $paramscan also carry tainted SQL fragments (e.g., when building dynamic queries or passing raw fragments), consider whether additional taint annotations are needed on$paramsto accurately reflect the dataflow model.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For `FetchAssoc::__invoke()` and `FetchEntity::__invoke()`, consider whether marking `$sql` as both `@psalm-taint-sink sql` and `@psalm-taint-escape sql` on the same method matches Psalm’s expectations (sometimes escapes are better modeled on lower-level helper methods or return values rather than on the same signature as the sink).
- If `array $params` can also carry tainted SQL fragments (e.g., when building dynamic queries or passing raw fragments), consider whether additional taint annotations are needed on `$params` to accurately reflect the dataflow model.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Pagerfanta/AuraSqlPagerFactory.phpsrc/Pagerfanta/ExtendedPdoAdapter.phpsrc/Pagerfanta/FetchAssoc.phpsrc/Pagerfanta/FetchEntity.php
🧰 Additional context used
🪛 GitHub Check: cs / Coding Standards
src/Pagerfanta/FetchEntity.php
[failure] 27-27:
Expected 1 line between description and annotations, found 0.
src/Pagerfanta/ExtendedPdoAdapter.php
[failure] 33-33:
Incorrect annotations group.
src/Pagerfanta/FetchAssoc.php
[failure] 19-19:
Expected 1 line between description and annotations, found 0.
src/Pagerfanta/AuraSqlPagerFactory.php
[failure] 18-18:
Expected 1 line between description and annotations, found 0.
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Pagerfanta/FetchEntity.php (1)
29-29: Consider the placement of the escape annotation.The
@psalm-taint-escape sqlannotation suggests this method escapes SQL taint, but the actual sanitization occurs in$this->pdo->perform()(line 34) via prepared statements, not in__invoke()itself. This concern was raised in a previous review but remains unaddressed. While marking the wrapper method as an escape point works for taint analysis purposes, it may be more precise to annotate only the actual escape point.
🧹 Nitpick comments (1)
src/Pagerfanta/FetchAssoc.php (1)
21-21: Consider the placement of the escape annotation.Similar to
FetchEntity, the@psalm-taint-escape sqlannotation suggests this method escapes SQL taint, but the actual sanitization occurs in$this->pdo->perform()(line 26) via prepared statements, not in__invoke()itself. While marking the wrapper method as an escape point works for taint analysis purposes, it may be more precise to annotate only the actual escape point in the call stack.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Pagerfanta/AuraSqlPagerFactory.phpsrc/Pagerfanta/FetchAssoc.phpsrc/Pagerfanta/FetchEntity.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). (3)
- GitHub Check: Sourcery review
- GitHub Check: sa / ComposerRequireChecker
- GitHub Check: PHPUnit - PHP 8.3 (ubuntu-latest, highest)
🔇 Additional comments (3)
src/Pagerfanta/AuraSqlPagerFactory.php (1)
16-20: LGTM! Formatting and annotation are correct.The blank line between
{@inheritDoc}and the Psalm annotation now conforms to the coding standard, and the@psalm-taint-sink sql $sqlannotation appropriately marks the SQL parameter as a potential injection point for static analysis.src/Pagerfanta/FetchEntity.php (1)
25-28: LGTM! Formatting and sink annotation are correct.The blank line between
{@inheritDoc}and the annotations now conforms to the coding standard, and the@psalm-taint-sink sql $sqlannotation appropriately marks the SQL parameter as a potential injection point for static analysis.src/Pagerfanta/FetchAssoc.php (1)
17-20: LGTM! Formatting and sink annotation are correct.The blank line between
{@inheritDoc}and the annotations now conforms to the coding standard, and the@psalm-taint-sink sql $sqlannotation appropriately marks the SQL parameter as a potential injection point for static analysis.
The escape annotation incorrectly implies these methods perform SQL sanitization. The actual escaping happens in ExtendedPdo::perform() via prepared statements, not in these wrapper methods.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai Removed |
|
Tip For best results, initiate chat on the files or code changes.
If ✏️ Learnings added
|
Parameters already have type declarations in the method signature.
1 participant
Summary
@psalm-taint-sink sqlto mark SQL query parameters as SQL injection sinks@psalm-taint-escape sqlto mark fetch methods as SQL escape points (prepared statements)Changed files
ExtendedPdoAdapter::__construct()- marks$sqlparameter as SQL sinkAuraSqlPagerFactory::newInstance()- marks$sqlparameter as SQL sinkFetchAssoc::__invoke()- marks as SQL sink and escape (uses prepared statements)FetchEntity::__invoke()- marks as SQL sink and escape (uses prepared statements)These annotations enable Psalm's taint analysis to detect SQL injection vulnerabilities by tracking tainted data flow from sources to sinks.
Summary by Sourcery
Annotate SQL-related methods and constructors with Psalm taint metadata to improve static detection of SQL injection risks.
Enhancements:
Summary by CodeRabbit