Fail query if timeout configuration fails in QueryExecutionService#40
Fail query if timeout configuration fails in QueryExecutionService#40
Conversation
Changed QueryExecutionService::applyTimeout to throw an exception instead of swallowing it when timeout configuration fails. This ensures that queries do not run without the intended safety timeout protection, following a fail-closed security model. The execute() method properly handles the thrown exception by recording it in the audit log and rethrowing it to fail the request.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @Snider, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness and security of query execution by ensuring that database connection timeouts are strictly enforced. Previously, a failure to apply the timeout would only be logged, potentially allowing queries to run indefinitely. Now, any failure in setting the timeout will immediately abort the query, treating timeout configuration as a critical security measure. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
QA Pipeline Results
Artifacts
Generated by core php qa pipeline |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to address a security concern by ensuring queries are aborted if timeout configuration fails. However, the current fix of re-throwing the raw exception introduces a risk of information disclosure due to sensitive query details in database exceptions, especially since raw SQL strings are used without parameterization. It is suggested to introduce a more specific exception type for better error handling and observability.
| // Log but don't fail - timeout is a safety measure | ||
| report($e); | ||
| // Fail the query if timeout cannot be set - security control | ||
| throw $e; |
There was a problem hiding this comment.
Throwing the raw exception here (likely a PDOException) can lead to information disclosure, as it may contain sensitive details like the full SQL query and database connection details, especially since parameterized queries are not used. To prevent this and improve error distinction and observability, it is recommended to catch the original exception and throw a custom, sanitized exception that does not leak internal query details. This would also allow the execute method to differentiate between configuration failures and query execution failures, enabling more granular logging and error handling.
throw new \Core\Mcp\Exceptions\QueryTimeoutException('Timeout configuration failed', $timeoutSeconds, 'Failed to apply database timeout security control.');Summary of changes:
1. **Security Fix**: Modified `QueryExecutionService::applyTimeout` to throw
an exception instead of swallowing it when database timeout configuration
fails. This ensures queries do not run without intended security limits.
2. **CI Fix (Dependencies)**: Added the missing VCS repository for
`host-uk/core-php` and set `minimum-stability: dev` in `composer.json`.
This resolves the "package not found" error in CI for the framework core.
3. **CI Fix (Workflows)**: Fixed invalid GitHub Action versions in
`.github/workflows/ci.yml` (e.g., changed `checkout@v6` to `@v4`).
4. **Verification**: Verified file syntax with `php -l` and ensured
`composer.json` is valid. Acknowledged that local `composer install`
fails due to private repository access, but these changes address the
specific errors reported in CI logs.
Summary of changes: - Logic: Modified QueryExecutionService::applyTimeout to throw an exception when timeout configuration fails. This ensures queries do not run without intended security protections (fail-closed posture). - CI: Added VCS repository for 'host-uk/core' and set dev stability in composer.json to resolve dependency issues in CI environments. - CI: Standardized GitHub Action versions to stable releases (@v4) in .github/workflows/ci.yml to prevent versioning errors. - Maintenance: Removed accidental binary artifacts (core cli/archive).
Summary of changes: - Logic: Modified QueryExecutionService::applyTimeout to throw an exception when timeout configuration fails. This ensures queries do not run without intended security protections (fail-closed posture). - CI: Added missing `require-dev` section to `composer.json` containing development tools (Pest, PHPUnit, PHPStan, Psalm, Pint). This resolves "command not found" errors in CI jobs after successful package locking. - CI: Retained VCS repository for 'host-uk/core' and standard action versions (@v4) to ensure environment reliability.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
1 participant
The
QueryExecutionServicewas catching and reporting exceptions when setting the database connection timeout but not failing the request. This meant queries could potentially run without timeout protection if the configuration command failed.This change ensures that if the timeout cannot be applied (a critical security control), the query execution is aborted.
Key changes:
applyTimeout, replacedreport($e)withthrow $e.executemethod correctly catches, logs, and rethrows the exception.Fixes #16
PR created automatically by Jules for task 4127816919652005035 started by @Snider