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
|
WalkthroughPsalm taint annotations were added to docblocks in four Pagerfanta classes to mark SQL sinks and escapes for static analysis. No runtime behavior, method signatures, or control flow were changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (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 |
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
| /** | ||
| * {@inheritDoc} | ||
| * @psalm-taint-sink sql $sql | ||
| * @psalm-taint-escape sql | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "FetchEntity.php" -type fRepository: ray-di/Ray.AuraSqlModule
Length of output: 100
🏁 Script executed:
cat -n src/Pagerfanta/FetchEntity.php | head -40Repository: ray-di/Ray.AuraSqlModule
Length of output: 1247
🌐 Web query:
PHP docblock PSR standard blank line between inheritDoc and annotations
💡 Result:
Short answer: There is no final PSR that specifically mandates a blank line between an inline {@inheritdoc} description and the annotations. PSR‑5 (the PHPDoc draft) exists but never became a final PSR; most projects follow conventions/tools instead.
Recommended/common practice:
- Treat {@inheritdoc} as part of the docblock description and put a blank line before the tag block (i.e., leave one blank line between description/{@inheritdoc} and the annotations). This is the convention used by Symfony/other standards and enforced by php-cs-fixer’s phpdoc_separation rule (group annotations and separate different groups by a single blank line). [1][2][3]
Sources:
- PSR‑5 (PHPDoc) draft / notes. [1]
- php-cs-fixer rule: phpdoc_separation (group annotations; different groups separated by a single blank line). [2]
- Symfony coding standards: advice to avoid one-line /** {@inheritdoc} */ and to separate annotation groups with a single blank line. [3]
🌐 Web query:
Psalm @psalm-taint-escape sql prepared statements documentation
💡 Result:
Short answer: use Psalm's taint annotations — @psalm-taint-escape to mark that a function/assignment removes a taint, @psalm-taint-sink to declare SQL sinks, and prefer parameterised (prepared) queries (so values are not treated as SQL) — see Psalm docs below.
Key docs:
- @psalm-taint-escape (how to mark that code sanitises/escapes a taint). [1]
- @psalm-taint-sink (how to declare SQL sinks like exec() or custom wrappers). [2]
- Security analysis intro (recommendation: use parameterised/prepared queries to avoid SQL injection; examples showing $pdo->exec as a sink). [3]
Practical notes:
- Prefer PDO prepared/parameterised queries — they prevent SQL injection (and Psalm’s docs explicitly recommend parameterised queries). [3][4]
- If you have a sanitizer function that makes a value safe for SQL, annotate it with @psalm-taint-escape sql so Psalm won’t report a taint. Example pattern shown in the docs. [1]
- If you wrap DB calls in your own class, declare the method parameter as a taint sink (e.g. /** @psalm-taint-sink sql $sql */) to let Psalm flag unsafe calls. [2]
Sources:
[1] Psalm — Avoiding false-positives / escaping tainted input. https://psalm.dev/docs/security_analysis/avoiding_false_positives/
[2] Psalm — Custom taint sinks. https://psalm.dev/docs/security_analysis/custom_taint_sinks/
[3] Psalm — Security Analysis / Introduction (parameterised queries recommendation; sql sinks). https://psalm.dev/docs/security_analysis/
[4] PHP manual — PDO::prepare (prepared statements prevent SQL injection). https://www.php.net/manual/en/pdo.prepare.php
Would you like a small annotated code example (PDO prepared + @psalm annotations) for your case?
Add blank line between docblock description and Psalm annotations, and remove @psalm-taint-escape sql.
PSR-5 and standard tooling (php-cs-fixer's phpdoc_separation rule) require a blank line between {@inheritDoc} and annotation tags. Additionally, @psalm-taint-escape sql is misleading here—this method declares $sql as a sink (correct) but does not itself escape the SQL. The actual sanitization happens in Aura's perform() method via prepared statements, not in __invoke(). Remove the escape annotation to avoid suggesting this method is responsible for SQL safety.
/**
* {@inheritDoc}
+ *
* @psalm-taint-sink sql $sql
- * @psalm-taint-escape sql
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * {@inheritDoc} | |
| * @psalm-taint-sink sql $sql | |
| * @psalm-taint-escape sql | |
| */ | |
| /** | |
| * {@inheritDoc} | |
| * | |
| * @psalm-taint-sink sql $sql | |
| */ |
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 27-27:
Expected 1 line between description and annotations, found 0.
🤖 Prompt for AI Agents
In src/Pagerfanta/FetchEntity.php around lines 25 to 29, add a blank line
between the docblock description ({@inheritDoc}) and the Psalm annotation tags,
and remove the `@psalm-taint-escape sql` annotation while keeping
`@psalm-taint-sink sql`; specifically, restructure the docblock so there is an
empty line after the description and delete the escape annotation to avoid
implying this method performs SQL sanitization.
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.
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
✏️ Tip: You can customize this high-level summary in your review settings.