Add Psalm taint annotations for session/cookie security #15
Conversation
Mark session and cookie providers as taint sources since $_COOKIE contains user-controlled data that could be manipulated. - SessionProvider::get() - returns Session initialized with $_COOKIE - CookieProvider::get() - returns $_COOKIE directly Also apply code style fixes (phpcbf).
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds Psalm taint-source annotations for session and cookie providers and applies small type-safety and coding-style refinements across the session module. Class diagram for updated session and cookie providersclassDiagram
direction LR
class ProviderInterface {
}
class SessionFactory {
+newInstance(array cookieData) Session
}
class Session {
}
class SessionProvider {
+get() Session
}
class CookieProvider {
+get() array
}
class AuraSessionInject {
-Session session
+setSession(Session session) void
}
class Cookie {
<<attribute>>
}
class DeleteCookie {
<<attribute>>
}
class AuraSessionModule {
+configure() void
}
class DeleteCookieInvoker {
+__invoke(string name, array params) void
}
ProviderInterface <|.. SessionProvider
ProviderInterface <|.. CookieProvider
SessionFactory --> Session : creates
SessionProvider --> SessionFactory : uses
AuraSessionInject --> Session
Cookie <|-- DeleteCookieInvoker
DeleteCookie <|-- DeleteCookieInvoker
Flow diagram for taint-source cookie and session dataflowchart LR
subgraph Browser
C["$_COOKIE"]
end
subgraph Server
CP["CookieProvider.get()"]
SP["SessionProvider.get()"]
SF["SessionFactory.newInstance(cookieData)"]
S["Session"]
APP["Application code using cookies and session"]
end
C --> CP
CP --> APP
C --> SP
SP --> SF
SF --> S
S --> APP
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis PR makes several focused improvements across annotation and provider classes: separating combined PHP attributes into discrete attributes, adding explicit return types, standardizing docblock tag formatting, incorporating Psalm taint annotations, and minor syntax adjustments. No behavioral changes or control-flow modifications are introduced. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
🧰 Additional context used🧬 Code graph analysis (3)src/Annotation/Cookie.php (1)
src/SessionProvider.php (2)
src/Annotation/DeleteCookie.php (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). (1)
🔇 Additional comments (9)
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1.x #15 +/- ##
=========================================
Coverage 54.16% 54.16%
Complexity 5 5
=========================================
Files 5 5
Lines 24 24
=========================================
Hits 13 13
Misses 11 11 ☔ 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:
- Consider splitting the Psalm taint annotations and the unrelated coding-style/typing tweaks (e.g.,
declare(strict_types=1), addedvoidreturn type, attribute formatting) into separate PRs to make the behavior change easier to review and reason about. - Adding
declare(strict_types=1);toSessionProvideronly may introduce subtle type behavior differences compared to the rest of the package; consider either applying it consistently or omitting it here to avoid inconsistent runtime behavior. - The new
: voidreturn type onAuraSessionInject::setSessioncould be a breaking change for consumers that override this method without a return type, so it may be safer to omit the return type or introduce it in a dedicated major/breaking-change PR.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider splitting the Psalm taint annotations and the unrelated coding-style/typing tweaks (e.g., `declare(strict_types=1)`, added `void` return type, attribute formatting) into separate PRs to make the behavior change easier to review and reason about.
- Adding `declare(strict_types=1);` to `SessionProvider` only may introduce subtle type behavior differences compared to the rest of the package; consider either applying it consistently or omitting it here to avoid inconsistent runtime behavior.
- The new `: void` return type on `AuraSessionInject::setSession` could be a breaking change for consumers that override this method without a return type, so it may be safer to omit the return type or introduce it in a dedicated major/breaking-change PR.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
Add Psalm taint annotations to mark session and cookie data as taint sources.
Changes
@psalm-taint-source inputon:SessionProvider::get()- returns Session initialized with$_COOKIECookieProvider::get()- returns$_COOKIEdirectlyAlso includes code style fixes (phpcbf).
Test Plan
./vendor/bin/psalm --taint-analysisto verify annotations workSummary by Sourcery
Mark session and cookie providers as taint sources for static analysis and apply minor coding style improvements.
Enhancements:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.