[Audit] Error Handling and Recovery #64
Conversation
…dler This commit introduces a custom exception hierarchy and a global exception handler to improve the application's error handling and recovery mechanisms. Key changes include: - A new `ApiException` class and more specific exception types (`AuthorizationException`, `ValidationException`). - A new global exception handler in `src/Core/Exception/Handler.php` that catches `ApiException` types and returns standardized JSON error responses. - The new exception handler is registered in `src/Core/Boot.php`. - The `ManagesTokens` trait has been refactored to use the new `AuthorizationException`. - An `AUDIT-ERRORS.md` file has been created to document the findings of an error-handling audit. Co-authored-by: Snider <631881+Snider@users.noreply.github.com>
|
👋 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. |
📝 WalkthroughWalkthroughThe changes introduce a structured error-handling framework with custom exception hierarchy (ApiException as base, with AuthorizationException and ValidationException subclasses), a global exception handler that returns standardized JSON responses with appropriate HTTP status codes, and update existing code to use these new exception types. A composer manifest is added, and the QA workflow is modified to exclude stderr from Psalm output files. Changes
Sequence DiagramsequenceDiagram
participant Client
participant App as Application
participant Handler as Exception Handler
participant Renderer as Renderable
participant Response
Client->>App: Send Request
App->>App: Process Request
alt Token Not Configured
App->>App: throw AuthorizationException
else Validation Failed
App->>App: throw ValidationException
else Other Error
App->>App: throw ApiException
end
App->>Handler: Caught Exception
Handler->>Handler: Determine HTTP Status Code
Handler->>Renderer: Match Exception Type
alt AuthorizationException
Renderer->>Renderer: HTTP 403
else ValidationException
Renderer->>Renderer: HTTP 422
else ApiException
Renderer->>Renderer: HTTP 400
end
Renderer->>Response: Build JSON Response
Response->>Client: Return JSON with Status Code
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
QA Pipeline Results
Artifacts
Generated by core php qa pipeline |
This commit fixes a CI failure in the Psalm job of the QA workflow. The `2>&1` redirection was causing progress messages from `stderr` to be included in the JSON and SARIF output files, which corrupted them and caused the SARIF parser to fail. By removing the `2>&1` redirection, the output files will now contain only the valid JSON and SARIF data, which should resolve the CI failure. Co-authored-by: Snider <631881+Snider@users.noreply.github.com>
This commit fixes an `InvalidDocblock` error in `src/Core/Exception/Handler.php` that was causing the Psalm CI job to fail. The `@return void` tag has been removed from the `register` method's docblock, and the return type has been added to the function signature to adhere to modern PHP standards. This change should resolve the Psalm error and allow the CI pipeline to pass.
This commit fixes a CI failure by correctly registering the custom exception handler. The previous method of registering the handler in `src/Core/Boot.php` was incorrect for Laravel 11 and caused a Psalm error that resulted in an invalid SARIF file. The exception handler is now registered as a singleton in the `register` method of the `LifecycleEventProvider`, which is the correct approach for this version of Laravel. This should resolve the Psalm error and allow the CI pipeline to pass.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@original_composer.json`:
- Around line 1-86: The file original_composer.json is a redundant duplicate of
composer.json; remove original_composer.json from the repository to avoid
duplicate composer manifests and confusion, ensuring only composer.json remains
and updating any references (scripts, CI configs, or README) that might point to
original_composer.json to reference composer.json instead.
In `@src/Core/Exception/Handler.php`:
- Around line 39-51: The ApiException renderable currently returns only
['message' => ...]; enhance ValidationException to accept and expose a
field-level errors array (add a constructor, a private array property and a
getErrors(): array method) so it can carry per-field messages, then update the
renderable callback in Handler (the $this->renderable(function (ApiException $e,
Request $request) { ... }) block) to detect $e instanceof ValidationException
and include 'errors' => $e->getErrors() in the JSON response alongside
'message'; also review the dontReport list (which currently suppresses
ApiException subclasses) and either remove AuthorizationException from
dontReport or add a specific report() override for AuthorizationException to
ensure auth failures are logged if desired; lastly confirm that returning null
for non-JSON requests remains intentional so Laravel falls back to HTML
rendering.
🧹 Nitpick comments (5)
AUDIT-ERRORS.md (1)
23-23: Use UK English spelling.Per coding guidelines, UK English spelling should be used. "Standardize" should be "standardise".
📝 Suggested fix
-- **Recommendation:** Standardize the creation of user-facing error messages, and ensure they are always clear, concise, and helpful. +- **Recommendation:** Standardise the creation of user-facing error messages, and ensure they are always clear, concise, and helpful.src/Core/Exception/ApiException.php (1)
7-9: Consider adding a class docblock.A brief docblock would clarify the purpose of this base exception class within the API exception hierarchy.
📝 Suggested enhancement
+/** + * Base exception for all API-related errors. + * + * Subclasses include AuthorizationException (403) and ValidationException (422). + * Handled by Core\Exception\Handler to produce standardised JSON responses. + */ class ApiException extends \Exception { }src/Core/Exception/ValidationException.php (1)
7-9: Consider adding validation error support.Laravel's
Illuminate\Validation\ValidationExceptionincludes structured error data via anerrors()method and$validatorproperty. This customValidationExceptionis empty, so validation errors would only be in the message string.If you intend to return structured validation errors (common for API 422 responses), consider adding properties to hold field-level errors:
♻️ Proposed enhancement
+/** + * Exception for validation failures in API requests. + * + * Rendered as HTTP 422 with JSON error response. + */ class ValidationException extends ApiException { + /** + * `@param` array<string, array<string>> $errors Field-level validation errors + */ + public function __construct( + string $message = 'The given data was invalid.', + protected array $errors = [], + ) { + parent::__construct($message); + } + + /** + * `@return` array<string, array<string>> + */ + public function errors(): array + { + return $this->errors; + } }This would also require updating
Handler::register()to include errors in the JSON response forValidationException.src/Core/Exception/Handler.php (2)
19-21: Verify that suppressingAuthorizationExceptionlogs aligns with security requirements.Adding
ApiException::classto$dontReportprevents all subclasses—includingAuthorizationException—from being logged. While suppressing validation noise is reasonable, authorization failures (e.g., missing or invalid tokens) are often security-relevant events worth auditing. Consider whetherAuthorizationExceptionshould be excluded from$dontReportor logged via a separate channel.
54-56: Consider removing the empty reportable callback if unused.The no-op
reportablecallback adds no behaviour. If it's a placeholder for future logging integration, a brief comment noting the intent would help. Otherwise, it can be safely removed.
| { | ||
| "name": "host-uk/core", | ||
| "description": "Modular monolith framework for Laravel - event-driven architecture with lazy module loading", | ||
| "keywords": ["laravel", "modular", "monolith", "framework", "events", "modules"], | ||
| "license": "EUPL-1.2", | ||
| "authors": [ | ||
| { | ||
| "name": "Host UK", | ||
| "email": "support@host.uk.com" | ||
| } | ||
| ], | ||
| "require": { | ||
| "php": "^8.2", | ||
| "laravel/framework": "^11.0|^12.0", | ||
| "laravel/pennant": "^1.0", | ||
| "livewire/livewire": "^3.0|^4.0" | ||
| }, | ||
| "require-dev": { | ||
| "fakerphp/faker": "^1.23", | ||
| "infection/infection": "^0.32.3", | ||
| "larastan/larastan": "^3.9", | ||
| "laravel/pint": "^1.18", | ||
| "mockery/mockery": "^1.6", | ||
| "nunomaduro/collision": "^8.6", | ||
| "orchestra/testbench": "^9.0|^10.0", | ||
| "phpstan/extension-installer": "^1.4", | ||
| "phpstan/phpstan": "^2.1", | ||
| "phpstan/phpstan-deprecation-rules": "^2.0", | ||
| "phpunit/phpunit": "^11.5", | ||
| "psalm/plugin-laravel": "^3.0", | ||
| "rector/rector": "^2.3", | ||
| "roave/security-advisories": "dev-latest", | ||
| "spatie/laravel-activitylog": "^4.8", | ||
| "vimeo/psalm": "^6.14" | ||
| }, | ||
| "suggest": { | ||
| "spatie/laravel-activitylog": "Required for activity logging features (^4.0)" | ||
| }, | ||
| "autoload": { | ||
| "psr-4": { | ||
| "Core\\": "src/Core/", | ||
| "Core\\Website\\": "src/Website/", | ||
| "Core\\Mod\\": "src/Mod/", | ||
| "Core\\Plug\\": "src/Plug/" | ||
| }, | ||
| "files": [ | ||
| "src/Core/Media/Thumbnail/helpers.php" | ||
| ] | ||
| }, | ||
| "autoload-dev": { | ||
| "psr-4": { | ||
| "Core\\Tests\\": "tests/", | ||
| "Core\\TestCore\\": "tests/Fixtures/Core/TestCore/", | ||
| "App\\Custom\\": "tests/Fixtures/Custom/", | ||
| "Mod\\": "tests/Fixtures/Mod/", | ||
| "Plug\\": "tests/Fixtures/Plug/", | ||
| "Website\\": "tests/Fixtures/Website/" | ||
| } | ||
| }, | ||
| "scripts": { | ||
| "test": "vendor/bin/phpunit", | ||
| "pint": "vendor/bin/pint", | ||
| "stan": "vendor/bin/phpstan analyse --memory-limit=512M" | ||
| }, | ||
| "extra": { | ||
| "laravel": { | ||
| "providers": [ | ||
| "Core\\LifecycleEventProvider", | ||
| "Core\\Lang\\LangServiceProvider", | ||
| "Core\\Bouncer\\Gate\\Boot" | ||
| ] | ||
| } | ||
| }, | ||
| "config": { | ||
| "optimize-autoloader": true, | ||
| "preferred-install": "dist", | ||
| "sort-packages": true, | ||
| "allow-plugins": { | ||
| "php-http/discovery": true, | ||
| "phpstan/extension-installer": true, | ||
| "infection/extension-installer": true | ||
| } | ||
| }, | ||
| "minimum-stability": "stable", | ||
| "prefer-stable": true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a composer.json already exists and compare with original_composer.json
if [ -f "composer.json" ]; then
echo "=== composer.json exists ==="
echo "Differences between composer.json and original_composer.json:"
diff composer.json original_composer.json || echo "Files differ"
else
echo "No composer.json found - original_composer.json may need to be renamed"
fi
# Also check for any other composer-related files
echo ""
echo "=== Searching for composer-related files ==="
find . -maxdepth 2 -name "*composer*" -type f 2>/dev/null | head -20Repository: host-uk/core-php
Length of output: 237
🏁 Script executed:
# Check git history and file context
git log --oneline --all -- "*composer*" 2>/dev/null | head -20Repository: host-uk/core-php
Length of output: 1345
🏁 Script executed:
# Look for documentation or comments about this file
git show HEAD:original_composer.json 2>/dev/null | head -10
# Also check if there's a .gitignore or setup instructions
find . -maxdepth 2 \( -name "README*" -o -name "SETUP*" -o -name ".gitignore" -o -name "docker-compose*" \) -type f 2>/dev/nullRepository: host-uk/core-php
Length of output: 471
🏁 Script executed:
# Check README for any mention of composer files or setup instructions
cat README.md | grep -i "composer\|original\|setup" -A 3 -B 3 || echo "No matches in README"
# Also check if there's any documentation in the root about this
ls -la | grep -i "doc\|read\|guide\|setup"Repository: host-uk/core-php
Length of output: 459
🏁 Script executed:
# Check the .gitignore to see if original_composer.json should be ignored
cat .gitignore | head -30Repository: host-uk/core-php
Length of output: 483
Remove the redundant original_composer.json file.
This file is identical to composer.json and serves no documented purpose. Having two identical composer manifests is unnecessary duplication and can cause confusion during maintenance. Delete original_composer.json and keep only composer.json.
🤖 Prompt for AI Agents
In `@original_composer.json` around lines 1 - 86, The file original_composer.json
is a redundant duplicate of composer.json; remove original_composer.json from
the repository to avoid duplicate composer manifests and confusion, ensuring
only composer.json remains and updating any references (scripts, CI configs, or
README) that might point to original_composer.json to reference composer.json
instead.
| $this->renderable(function (ApiException $e, Request $request) { | ||
| if ($request->wantsJson()) { | ||
| $statusCode = match (true) { | ||
| $e instanceof AuthorizationException => 403, | ||
| $e instanceof ValidationException => 422, | ||
| default => 400, | ||
| }; | ||
|
|
||
| return new JsonResponse( | ||
| ['message' => $e->getMessage()], | ||
| $statusCode | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Core/Exception/Handler.phpRepository: host-uk/core-php
Length of output: 1889
🌐 Web query:
Laravel 11 exception handler renderable callback return null behavior
💡 Result:
In Laravel 11, an exception “render” / renderable callback is optional: if it returns null (or returns nothing), Laravel treats that as “not handled” and falls back to the framework’s normal exception rendering for that request (i.e., the default HTML/JSON error response). To override rendering, your callback must return a Response / JsonResponse / etc. [1]
Source: Laravel 11 “Error Handling → Rendering Exceptions” explicitly says: “If the closure given to the render method does not return a value, Laravel's default exception rendering will be utilized.” [1]
References
[1] Laravel 11.x Docs – Error Handling (Rendering Exceptions) https://laravel.com/docs/11.x/errors
🏁 Script executed:
fd -t f "ValidationException" src/ --exec cat -n {} \;Repository: host-uk/core-php
Length of output: 233
🏁 Script executed:
rg -A 10 "class ValidationException" src/Repository: host-uk/core-php
Length of output: 239
🏁 Script executed:
rg -B 5 -A 15 "ValidationException" src/Core/Exception/Repository: host-uk/core-php
Length of output: 1769
🏁 Script executed:
rg -B 3 -A 3 "throw.*ValidationException" src/ --max-count=5Repository: host-uk/core-php
Length of output: 42
🏁 Script executed:
rg -B 3 -A 3 "throw.*AuthorizationException" src/ --max-count=5Repository: host-uk/core-php
Length of output: 799
🏁 Script executed:
rg "new ValidationException" src/ --max-count=5Repository: host-uk/core-php
Length of output: 42
🏁 Script executed:
rg -A 20 "class ApiException" src/Core/Exception/Repository: host-uk/core-php
Length of output: 209
🏁 Script executed:
rg "ApiException\|AuthorizationException" src/ -l | head -10Repository: host-uk/core-php
Length of output: 42
🏁 Script executed:
rg "class.*Exception.*extends" src/Core/Exception/ -A 2Repository: host-uk/core-php
Length of output: 587
Consider enhancing ValidationException to carry field-level validation errors if form validation is the intended use case.
The current response returns ['message' => $e->getMessage()] only. For form/field validation, clients typically expect ['message' => '...', 'errors' => [...]] to display per-field messages. The ValidationException class itself is currently empty with no error structure—if this exception should handle field-level validation failures, it needs to be enhanced to store and expose validation errors.
When $request->wantsJson() returns false, the callback returns null, which causes Laravel to use its default exception rendering (HTML error page). This is correct per Laravel documentation, but confirm this behavior is intentional for API exceptions thrown during non-JSON requests.
Additionally, dontReport suppresses all ApiException subclasses from logging, including AuthorizationException failures. Consider whether authorization failures should be logged separately for security audit purposes.
🤖 Prompt for AI Agents
In `@src/Core/Exception/Handler.php` around lines 39 - 51, The ApiException
renderable currently returns only ['message' => ...]; enhance
ValidationException to accept and expose a field-level errors array (add a
constructor, a private array property and a getErrors(): array method) so it can
carry per-field messages, then update the renderable callback in Handler (the
$this->renderable(function (ApiException $e, Request $request) { ... }) block)
to detect $e instanceof ValidationException and include 'errors' =>
$e->getErrors() in the JSON response alongside 'message'; also review the
dontReport list (which currently suppresses ApiException subclasses) and either
remove AuthorizationException from dontReport or add a specific report()
override for AuthorizationException to ensure auth failures are logged if
desired; lastly confirm that returning null for non-JSON requests remains
intentional so Laravel falls back to HTML rendering.
This submission introduces a custom exception hierarchy and a global exception handler to improve the application's error handling and recovery mechanisms, and it includes an audit report that details the findings and the changes that were made.
Fixes #51
PR created automatically by Jules for task 13970562873216773264 started by @Snider
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.