Repository navigation
Conversation
📝 WalkthroughWalkthroughDeletes testing docs and a debug script; adds a media library, picker UI, Cloudinary list/upload APIs and tests; introduces EditorJS embed support and renderer; moves CsrfToken to session-backed DI; refactors many controllers to dependency injection and a fluent view builder; installer collects email config. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User (Browser)
participant Modal as Media Picker Modal
participant Server as App Server
participant Cloudinary as Cloudinary API
rect rgb(220,235,250)
Note over Browser,Cloudinary: Library Browse & Select Flow
Browser->>Modal: openMediaPicker(targetInput)
Modal->>Server: GET /admin/media?cursor=...
Server->>Cloudinary: CloudinaryUploader.listResources(options)
Cloudinary-->>Server: resources + next_cursor
Server-->>Modal: Rendered media cards HTML
Modal->>Browser: Display grid
Browser->>Modal: Click media card -> select
Browser->>Modal: Click "Select Image"
Modal-->>Browser: Insert URL into target input, dispatch change, close modal
end
sequenceDiagram
participant Browser as User (Browser)
participant Modal as Media Picker Modal
participant Server as App Server
participant Cloudinary as Cloudinary API
rect rgb(250,235,220)
Note over Browser,Cloudinary: Upload Flow
Browser->>Modal: Switch to Upload tab, choose file
Browser->>Modal: Click Upload
Modal->>Server: POST /admin/media/featured-upload (FormData + CSRF)
Server->>Cloudinary: UploadApi.upload(file, options)
Cloudinary-->>Server: uploaded resource metadata
Server-->>Modal: JSON success with URL
Modal->>Browser: Insert URL into target input, show success, optionally reload library
end
sequenceDiagram
participant Editor as EditorJS
participant Renderer as EditorJsRenderer
participant Validator as Domain Validator
rect rgb(230,250,230)
Note over Editor,Validator: Embed Block Rendering
Editor->>Renderer: renderBlock(type='embed', data)
Renderer->>Validator: validate embed URL domain
alt domain trusted & url present
Validator-->>Renderer: OK
Renderer-->>Editor: <figure> with responsive iframe + optional caption
else missing or untrusted
Validator-->>Renderer: Reject
Renderer-->>Editor: HTML comment warning (no iframe)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
resources/views/admin/posts/create.php (1)
222-232: Consider using 'input' event and adding error handling for image preview.The
changeevent on URL inputs only fires on blur. Usinginputwould provide real-time feedback. Additionally, broken image URLs will show a broken image icon.🔎 Proposed improvement
-document.getElementById('featured_image').addEventListener('change', function() { +document.getElementById('featured_image').addEventListener('input', function() { const preview = document.getElementById('featured_image_preview'); const url = this.value.trim(); if (url) { preview.src = url; preview.classList.remove('d-none'); + preview.onerror = function() { + this.classList.add('d-none'); + }; } else { preview.classList.add('d-none'); } });resources/views/partials/media-picker-modal.php (2)
311-316: Add client-side file size validation before upload.The form text mentions "Max size: 5MB" but there's no validation before initiating the upload. Users could attempt to upload large files and only fail after waiting.
🔎 Proposed fix
uploadBtn?.addEventListener('click', function() { if (!imageFile.files.length) { uploadError.textContent = 'Please select a file'; uploadError.classList.remove('d-none'); return; } + + const maxSize = 5 * 1024 * 1024; // 5MB + if (imageFile.files[0].size > maxSize) { + uploadError.textContent = 'File size exceeds 5MB limit'; + uploadError.classList.remove('d-none'); + return; + } const formData = new FormData();
166-172: HTML parsing approach for media library is fragile.Fetching the full page HTML and parsing it with DOMParser couples the modal tightly to the media library page structure. Consider adding a dedicated JSON API endpoint for better maintainability.
This approach will break if the media library page structure changes. A dedicated API endpoint returning JSON would be more robust and performant.
src/Cms/Cli/Commands/Install/InstallCommand.php (1)
695-702: Consider not logging SMTP host and port in messages.Logging
"Email: SMTP ($host:$port)"to_messagesmay expose infrastructure details in the installation summary. While minor, this could be a security consideration.Consider logging just
"Email: SMTP configured"instead of including the host and port details.resources/views/admin/media/index.php (3)
165-176: Delete functionality is incomplete.The delete button shows a confirmation dialog but only displays an alert instead of actually deleting the image. This should either be implemented or the button should be hidden/disabled until the API is ready.
Would you like me to help implement the delete API endpoint, or should the delete button be hidden until the feature is complete?
186-188: Add file validation before initiating upload.Same as in the media picker modal - validate file size (5MB limit mentioned in the form text) and file type before starting the upload to provide immediate feedback.
🔎 Proposed fix
uploadBtn.addEventListener('click', function() { + if (!imageFile.files.length) { + uploadError.textContent = 'Please select a file'; + uploadError.classList.remove('d-none'); + return; + } + + const file = imageFile.files[0]; + const maxSize = 5 * 1024 * 1024; // 5MB + if (file.size > maxSize) { + uploadError.textContent = 'File size exceeds 5MB limit'; + uploadError.classList.remove('d-none'); + return; + } + const formData = new FormData(); formData.append('image', imageFile.files[0]);
150-163: Consider adding clipboard API fallback for older browsers.
navigator.clipboardis not available in all browsers or non-HTTPS contexts. Consider a fallback usingdocument.execCommand('copy')or gracefully degrading.🔎 Proposed fallback
btn.addEventListener('click', function() { const url = this.dataset.url; - navigator.clipboard.writeText(url).then(() => { + const copyToClipboard = (text) => { + if (navigator.clipboard) { + return navigator.clipboard.writeText(text); + } + // Fallback for older browsers + const textarea = document.createElement('textarea'); + textarea.value = text; + document.body.appendChild(textarea); + textarea.select(); + document.execCommand('copy'); + document.body.removeChild(textarea); + return Promise.resolve(); + }; + + copyToClipboard(url).then(() => { const originalHtml = this.innerHTML;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
TESTING.mddebug-maintenance.phpresources/app/Initializers/AuthInitializer.phpresources/config/routes.yamlresources/views/admin/dashboard/index.phpresources/views/admin/media/index.phpresources/views/admin/pages/create.phpresources/views/admin/pages/edit.phpresources/views/admin/posts/create.phpresources/views/admin/posts/edit.phpresources/views/layouts/admin.phpresources/views/partials/media-picker-modal.phpsrc/Cms/Cli/Commands/Install/InstallCommand.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Services/Content/EditorJsRenderer.phpsrc/Cms/Services/Media/CloudinaryUploader.phptests/Unit/Cms/Services/Content/EditorJsRendererTest.phpversionlog.md
💤 Files with no reviewable changes (3)
- TESTING.md
- versionlog.md
- debug-maintenance.php
🧰 Additional context used
🧬 Code graph analysis (8)
src/Cms/Controllers/Admin/Media.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (1)
listResources(149-184)
resources/views/partials/media-picker-modal.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
resources/app/Initializers/AuthInitializer.php (1)
src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)
resources/views/admin/posts/edit.php (1)
src/Cms/Models/Post.php (1)
getFeaturedImage(191-194)
resources/views/admin/media/index.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (2)
src/Cms/Services/Content/EditorJsRenderer.php (1)
render(27-37)tests/Unit/Cms/Services/Widget/WidgetTest.php (1)
render(16-19)
resources/views/layouts/admin.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
src/Cms/Cli/Commands/Maintenance/EnableCommand.php (1)
confirm(222-227)src/Cms/Cli/Commands/Maintenance/DisableCommand.php (1)
confirm(136-141)
🪛 PHPMD (2.15.0)
src/Cms/Services/Content/EditorJsRenderer.php
255-255: Avoid unused local variables such as '$service'. (undefined)
(UnusedLocalVariable)
256-256: Avoid unused local variables such as '$source'. (undefined)
(UnusedLocalVariable)
258-258: Avoid unused local variables such as '$width'. (undefined)
(UnusedLocalVariable)
259-259: Avoid unused local variables such as '$height'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (17)
resources/app/Initializers/AuthInitializer.php (1)
57-67: LGTM! Proper dependency injection implementation.The change correctly injects the
SessionManagerinstance into theCsrfTokenconstructor, aligning with the updated constructor signature. The dependency flow is sound:SessionManageris instantiated first (line 59), then properly passed toCsrfToken(line 62), and both are correctly utilized in the authentication filters.resources/views/layouts/admin.php (1)
32-55: LGTM! Well-structured navigation enhancement.The new Content and Users dropdown menus provide better organization for the admin interface. The implementation uses Bootstrap classes correctly and integrates properly with the routing system.
resources/views/admin/pages/edit.php (1)
135-135: LGTM! Consistent embed tool integration.The EditorJS embed plugin is properly loaded and configured with appropriate embed services. Version pinning ensures stability.
Also applies to: 187-201
resources/views/admin/pages/create.php (1)
116-116: LGTM! Consistent with the edit view.The embed tool integration follows the same pattern as the edit view, maintaining consistency across create and edit workflows.
Also applies to: 155-169
resources/config/routes.yaml (1)
254-260: LGTM! Proper route configuration.The new media management route is correctly configured with authentication. The CSRF protection on the upload route (line 265) is a security improvement.
src/Cms/Services/Media/CloudinaryUploader.php (1)
142-184: LGTM! Well-structured resource listing implementation.The method properly integrates with Cloudinary's Admin API, supports pagination, and reuses the existing
formatResultmethod for consistency. Error handling appropriately wraps exceptions.src/Cms/Services/Content/EditorJsRenderer.php (2)
269-329: Excellent security implementation for embed rendering.The method implements multiple security layers:
- Domain whitelist to prevent malicious embeds
htmlspecialchars()to prevent XSSsandboxattribute on iframes to restrict capabilities- Graceful handling of missing/invalid URLs
304-304: The project explicitly requires PHP 8.4+ (as stated in readme.md), sostr_ends_with()(introduced in PHP 8.0) is fully compatible and requires no polyfill or alternative implementation.Likely an incorrect or invalid review comment.
resources/views/admin/posts/edit.php (1)
47-60: LGTM! Comprehensive media management integration.The changes successfully integrate:
- Media picker with browse functionality for featured images
- Live image preview for better UX
- EditorJS embed tool for rich content
- Consistent implementation across the admin interface
The implementation is user-friendly and maintains consistency with other admin views.
Also applies to: 103-103, 129-136, 195-209, 247-258, 261-261
resources/views/admin/dashboard/index.php (1)
7-73: Well-organized dashboard navigation structure.The Quick Access sections are logically grouped (Content Management, Organization, Users & Settings) with consistent styling and appropriate icons. The responsive grid layout ensures good usability across device sizes.
resources/views/admin/posts/create.php (1)
91-91: Embed tool integration looks correct.The Embed plugin is properly loaded and configured with appropriate services (YouTube, Vimeo, Twitter, Instagram, Facebook, CodePen, GitHub).
Also applies to: 170-183
tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (3)
562-588: Comprehensive embed block test coverage.The YouTube embed test properly validates iframe presence, domain, caption, sandbox attributes, and figure wrapper. Good coverage of the core embed functionality.
654-672: Good security test for untrusted domains.This test ensures that embeds from untrusted domains are blocked and no iframe is rendered, preventing potential XSS or content injection attacks.
715-736: XSS protection test for embed captions is well-designed.Verifies that script tags in captions are properly escaped to
<script>, preventing XSS attacks through malicious captions.resources/views/partials/media-picker-modal.php (1)
125-143: Well-structured IIFE pattern for modal functionality.The use of an IIFE prevents global namespace pollution while still exposing
openMediaPickeron the window object for external callers. State management with module-scoped variables is clean.src/Cms/Cli/Commands/Install/InstallCommand.php (1)
602-631: Well-structured email configuration flow.The email configuration follows the established pattern from Cloudinary configuration, with clear prompts, sensible defaults, and proper test-mode fallback when the user skips configuration.
resources/views/admin/media/index.php (1)
27-77: Clean responsive grid layout for media items.The media card layout with consistent styling, hover effects, and action buttons provides a good user experience. Proper use of
htmlspecialchars()for output escaping andloading="lazy"for performance.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into feature/image-management
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Cms/Controllers/Admin/Media.php (1)
91-112: Exception details still exposed to users despite previous review.The error logging on line 93 addresses part of the previous review comment, but line 102 still passes
$e->getMessage()directly to the view. Cloudinary exceptions may contain sensitive information such as API credentials, internal paths, configuration details, or error specifics that should not be shown to end users.🔎 Recommended fix to use generic error message
catch( \Exception $e ) { Log::error( 'Error fetching media resources: ' . $e->getMessage() ); $viewData = [ 'Title' => 'Media Library | ' . $this->getName(), 'Description' => 'Manage uploaded images', 'User' => $user, 'resources' => [], 'nextCursor' => null, 'totalCount' => 0, - 'error' => $e->getMessage() + 'error' => 'Unable to load media library. Please try again later.' ]; return $this->renderHtml( HttpResponseStatus::OK, $viewData, 'index', 'admin' ); }
🧹 Nitpick comments (3)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
288-298: Consider extracting credential-checking logic.The three new tests duplicate identical credential-checking logic. Consider extracting this into a helper method to improve maintainability:
🔎 Suggested refactor to reduce duplication
Add a helper method to the test class:
private function skipIfNoCredentials(): void { $cloudName = $this->_settings->get( 'cloudinary', 'cloud_name' ); $isTestCredentials = ($cloudName === 'test-cloud'); $hasRealCredentials = getenv( 'CLOUDINARY_URL' ) || (!$isTestCredentials && $cloudName); if( !$hasRealCredentials ) { $this->markTestSkipped( 'Cloudinary credentials not configured. Set CLOUDINARY_URL environment variable or configure real cloudinary settings to run this integration test.' ); } }Then replace the duplicated blocks in each test with:
public function testListResourcesReturnsExpectedStructure(): void { - // Skip if using test credentials (not real Cloudinary account) - $cloudName = $this->_settings->get( 'cloudinary', 'cloud_name' ); - $isTestCredentials = ($cloudName === 'test-cloud'); - $hasRealCredentials = getenv( 'CLOUDINARY_URL' ) || (!$isTestCredentials && $cloudName); - - if( !$hasRealCredentials ) - { - $this->markTestSkipped( - 'Cloudinary credentials not configured. Set CLOUDINARY_URL environment variable or configure real cloudinary settings to run this integration test.' - ); - } + $this->skipIfNoCredentials(); $uploader = new CloudinaryUploader( $this->_settings );Note: You could also apply this refactoring to the existing integration tests (lines 210-222, 241-253, 264-276) for consistency.
Also applies to: 327-337, 356-366
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
623-630: Consider extracting duplicate test-mode config to a helper.The same test-mode fallback configuration array is repeated 4 times. Extracting to a private method or constant would reduce duplication and ensure consistency if the defaults change.
🔎 Proposed refactor
+ /** + * Get default test-mode email configuration + */ + private function getTestModeEmailConfig(): array + { + return [ + 'email' => [ + 'driver' => 'mail', + 'test_mode' => true, + 'from_address' => 'noreply@example.com', + 'from_name' => 'Neuron CMS' + ] + ]; + }Then replace each occurrence with:
return $this->getTestModeEmailConfig();Also applies to: 665-672, 691-698, 707-713
717-717: Consider validating port range.The port is cast to integer but not validated. Invalid ports (e.g., negative or >65535) would be stored without warning.
🔎 Proposed validation
$port = $this->input->ask( "SMTP port", "587" ); + $portInt = (int)$port; + if( $portInt < 1 || $portInt > 65535 ) + { + $this->output->warning( "Invalid port number. Using default 587." ); + $portInt = 587; + } $encryption = $this->input->choice(Then use
$portIntinstead of(int)$porton line 717.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Cms/Cli/Commands/Install/InstallCommand.phpsrc/Cms/Controllers/Admin/Media.phptests/Unit/Cms/Services/Content/EditorJsRendererTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Cms/Services/Content/EditorJsRendererTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (2)
CloudinaryUploader(17-371)listResources(149-184)
src/Cms/Controllers/Admin/Media.php (2)
src/Cms/Controllers/Content.php (1)
Content(61-302)src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(17-371)__construct(28-32)listResources(149-184)
⏰ 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: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (9)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (3)
286-323: LGTM! Comprehensive structure validation.The test properly verifies the response structure from
listResources()and conditionally validates resource properties when resources are present. The approach of checking structural keys is appropriate for an integration test.
325-352: LGTM! Validates max_results option.The test correctly verifies that the
max_resultsoption is respected when listing resources.
354-398: LGTM! Pagination logic validated.The test properly exercises the pagination flow by fetching multiple pages and verifying that different resources are returned. The conditional logic handles both scenarios (with and without pagination) appropriately.
src/Cms/Cli/Commands/Install/InstallCommand.php (3)
482-486: LGTM!Clean integration of email configuration following the existing pattern established for Cloudinary.
662-714: Previous issue addressed: Validation failures now fall back to test mode.The SMTP validation failures (missing host/username/password) now correctly return a test-mode configuration instead of an empty array. This ensures email is always configured in a valid state.
756-756: LGTM!The method signature extension and email config merging follow the established pattern used for Cloudinary configuration. The default empty array preserves backward compatibility.
Also applies to: 796-800
src/Cms/Controllers/Admin/Media.php (3)
5-5: LGTM: Class hierarchy and imports updated correctly.The change to extend
Contentinstead of a baseControlleraligns with the parent class constructor pattern and provides access to rendering methods likerenderHtml(). The new imports forLogandHttpResponseStatussupport the error handling and response status functionality in theindex()method.Also applies to: 9-10, 20-20
27-32: LGTM: Constructor signature updated correctly.The constructor now accepts an optional
Applicationparameter and correctly passes it to the parentContentconstructor, matching the parent class signature shown in the relevant code snippets. The existing uploader and validator initialization logic is preserved.
55-60: LGTM: User authentication check is appropriate.The authentication check ensures a user is present before rendering the admin media library. Throwing a
RuntimeExceptionfor a missing authenticated user in an admin context is appropriate, as this represents an exceptional condition that should be caught by upstream middleware or error handlers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Cms/Controllers/Admin/Media.php (1)
93-113: Exception details still exposed to the view.While error logging was added (good!), line 104 still passes
$e->getMessage()directly to the view. Cloudinary exceptions may contain sensitive information like API configuration details or internal paths. Consider returning a generic user-facing message instead.🔎 Recommended fix
catch( \Exception $e ) { Log::error( 'Error fetching media resources: ' . $e->getMessage() ); $viewData = [ 'Title' => 'Media Library | ' . $this->getName(), 'Description' => 'Manage uploaded images', 'User' => $user, 'resources' => [], 'nextCursor' => null, 'totalCount' => 0, - 'error' => $e->getMessage() + 'error' => 'Unable to load media library. Please try again later.' ];
🧹 Nitpick comments (4)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (2)
6-6: Remove unused import.The
UploadApiclass is imported but never used in this test file.🔎 Proposed fix
-use Cloudinary\Api\Upload\UploadApi;
36-76: Consider extracting mock setup into a helper method.The mock creation and reflection-based injection pattern is repeated across all seven test methods. Extracting this into a private helper method would improve maintainability and reduce duplication.
💡 Example helper method
private function createUploaderWithMock(AdminApi $adminApiMock): CloudinaryUploader { $cloudinaryMock = $this->createMock(Cloudinary::class); $cloudinaryMock->method('adminApi')->willReturn($adminApiMock); $uploader = new CloudinaryUploader($this->_settings); $reflection = new \ReflectionClass($uploader); $cloudinaryProperty = $reflection->getProperty('_cloudinary'); $cloudinaryProperty->setAccessible(true); $cloudinaryProperty->setValue($uploader, $cloudinaryMock); return $uploader; }Then each test could be simplified:
public function testListResourcesWithCustomMaxResults(): void { $adminApiMock = $this->createMock(AdminApi::class); $adminApiMock->expects($this->once()) ->method('assets') ->with($this->callback(function($options) { return $options['max_results'] === 10; })) ->willReturn([ 'resources' => [], 'next_cursor' => null, 'total_count' => 0 ]); $uploader = $this->createUploaderWithMock($adminApiMock); $result = $uploader->listResources(['max_results' => 10]); $this->assertIsArray($result); }Also applies to: 93-114, 123-144, 153-174, 183-212, 232-248, 260-273
src/Cms/Controllers/Admin/Media.php (1)
124-180: Consider unifying the response structure between upload methods.The two upload methods use different response shapes:
uploadImage():success: 0|1,messagefor errorsuploadFeaturedImage():success: true|false,errorfor errorsIf both methods are consumed by different systems (Editor.js vs. custom frontend), this is fine. Otherwise, consider aligning them for consistency.
Also applies to: 190-242
tests/Unit/Cms/Controllers/MediaUploadTest.php (1)
16-16: Missing test coverage forindex()method.The new
index()method in the Media controller is not covered by this test suite. Consider adding tests for:
- Successful resource listing
- Pagination with cursor
- Error handling when Cloudinary fails
- Unauthenticated user case
Would you like me to generate test cases for the
index()method?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Cms/Controllers/Admin/Media.phptests/Unit/Cms/Controllers/MediaUploadTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (2)
CloudinaryUploader(17-371)listResources(149-184)
src/Cms/Controllers/Admin/Media.php (3)
src/Cms/Controllers/Content.php (1)
Content(61-302)src/Cms/Services/Media/CloudinaryUploader.php (4)
CloudinaryUploader(17-371)__construct(28-32)listResources(149-184)upload(68-89)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
⏰ 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: build-test (sqlite)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
🔇 Additional comments (7)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (1)
33-279: Excellent test coverage for the listResources functionality.The test suite comprehensively covers all key scenarios:
- Default parameter handling and correct API invocation
- Custom parameters (max_results, next_cursor, folder)
- Result formatting and structure validation
- Empty results handling
- Error propagation
The use of reflection to inject mocks aligns with existing test patterns, and all assertions appropriately verify both the Cloudinary API interactions and the returned data structure.
src/Cms/Controllers/Admin/Media.php (2)
5-14: LGTM - Class structure and imports are well-organized.The refactor to extend
Contentprovides proper site configuration initialization. The imports align with the framework patterns used in the methods.Also applies to: 22-22
32-45: Constructor pattern is consistent with the framework.The validation of Settings type and initialization of dependencies follows the established patterns in the codebase.
tests/Unit/Cms/Controllers/MediaUploadTest.php (4)
21-55: Good test isolation practices.The setup properly backs up and restores Registry state, and tearDown cleans up global state (
$_FILES). This prevents test pollution.
57-157: Error path tests are comprehensive.Good coverage of both upload methods' error conditions: missing files and validation failures. The reflection-based mock injection works for the current implementation.
159-261: Success path tests verify expected response contracts.The tests properly mock both validator and uploader, then verify the JSON response structure matches what consumers (Editor.js, frontend) expect.
263-345: Exception tests will need updating if error messages are genericized.These tests verify the current behavior where
$e->getMessage()is returned directly. If the security recommendation to use generic error messages is implemented, update these assertions accordingly:- $this->assertEquals( 'Upload failed', $json['message'] ); + $this->assertEquals( 'Image upload failed. Please try again.', $json['message'] );
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Cms/Controllers/Admin/Media.php (3)
93-114: Exception details still exposed to users.The error view on line 104 includes
$e->getMessage(), which can leak sensitive information such as API credentials, internal paths, or Cloudinary configuration details. While logging on line 95 is correct, the raw exception message should not be shown to users.🔎 Recommended fix
catch( \Exception $e ) { Log::error( 'Error fetching media resources: ' . $e->getMessage() ); $viewData = [ 'Title' => 'Media Library | ' . $this->getName(), 'Description' => 'Manage uploaded images', 'User' => $user, 'resources' => [], 'nextCursor' => null, 'totalCount' => 0, - 'error' => $e->getMessage() + 'error' => 'Unable to load media library. Please try again later.' ]; return $this->renderHtml( HttpResponseStatus::OK, $viewData, 'index', 'admin' ); }
170-182: Exception details exposed in API response.Line 178 returns the raw exception message to API consumers, which can leak sensitive internal details such as configuration errors, file system paths, or API credentials.
🔎 Recommended fix
catch( \Exception $e ) { Log::error( 'uploadImage: ' . $e->getMessage() ); return $this->renderJson( HttpResponseStatus::INTERNAL_SERVER_ERROR, [ 'success' => 0, - 'message' => $e->getMessage() + 'message' => 'Image upload failed. Please try again.' ] ); }
234-246: Exception details exposed in API response.Line 242 returns the raw exception message, which can leak sensitive information to API consumers. Additionally, line 236-237 has a stray semicolon after the Log::error call (minor syntax style issue).
🔎 Recommended fix
catch( \Exception $e ) { - Log::error( 'uploadFeaturedImage: ' . $e->getMessage() ) - ; + Log::error( 'uploadFeaturedImage: ' . $e->getMessage() ); + return $this->renderJson( HttpResponseStatus::INTERNAL_SERVER_ERROR, [ 'success' => false, - 'error' => $e->getMessage() + 'error' => 'Featured image upload failed. Please try again.' ] ); }
🧹 Nitpick comments (1)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
60-149: Excellent test coverage for upload error scenarios!These seven test methods provide thorough coverage of PHP's upload error constants, ensuring the
MediaValidatorhandles all error cases correctly. The tests are well-structured and the assertions appropriately verify both the validation result and error messages.Optional: Consider using a data provider to reduce duplication
While the current approach is clear and maintainable, you could reduce duplication using PHPUnit's
@dataProvider:/** * @dataProvider uploadErrorProvider */ public function testValidateReturnsFalseForUploadErrors(int $errorCode, string $expectedSubstring): void { $file = [ 'error' => $errorCode, 'tmp_name' => '' ]; $result = $this->_validator->validate($file); $this->assertFalse($result); $this->assertStringContainsString($expectedSubstring, $this->_validator->getFirstError()); } public static function uploadErrorProvider(): array { return [ 'ini_size' => [UPLOAD_ERR_INI_SIZE, 'upload_max_filesize'], 'form_size' => [UPLOAD_ERR_FORM_SIZE, 'MAX_FILE_SIZE'], 'partial' => [UPLOAD_ERR_PARTIAL, 'partially uploaded'], 'no_tmp_dir' => [UPLOAD_ERR_NO_TMP_DIR, 'temporary folder'], 'cant_write' => [UPLOAD_ERR_CANT_WRITE, 'write file to disk'], 'extension' => [UPLOAD_ERR_EXTENSION, 'extension stopped'], 'unknown' => [999, 'Unknown'], ]; }This is purely optional—the current individual methods have the advantage of clearer failure messages when a specific test breaks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Cms/Controllers/Admin/Media.phptests/Unit/Cms/Controllers/MediaIndexTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.phptests/Unit/Cms/Services/Media/MediaValidatorTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
src/Cms/Services/Media/MediaValidator.php (1)
getFirstError(188-191)
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (2)
CloudinaryUploader(17-371)listResources(149-184)
tests/Unit/Cms/Controllers/MediaIndexTest.php (3)
src/Cms/Controllers/Admin/Media.php (2)
Media(22-247)index(57-114)src/Cms/Services/Media/CloudinaryUploader.php (1)
CloudinaryUploader(17-371)src/Cms/Auth/SessionManager.php (1)
set(99-103)
⏰ 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: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (9)
tests/Unit/Cms/Controllers/MediaIndexTest.php (3)
23-45: LGTM - Proper test isolation with registry state management.The setUp method correctly stores original registry values and configures in-memory settings for Cloudinary. This ensures test isolation without side effects on the global registry.
47-56: LGTM - Proper cleanup of test state.The tearDown method correctly restores the original registry values, ensuring no test pollution.
58-70: LGTM - Test correctly verifies authentication requirement.The test properly verifies that accessing the media index without authentication throws the expected RuntimeException. The use of expectException ensures the security check is enforced.
tests/Unit/Cms/Services/Media/CloudinaryUploaderMockTest.php (3)
33-279: LGTM - Comprehensive test coverage for listResources.The test suite thoroughly covers the listResources functionality including parameter validation, pagination, custom options, result formatting, empty results, and error handling. The use of reflection to inject mocks is appropriate for testing internal dependencies.
281-348: LGTM - Complete test coverage for delete functionality.The delete tests cover all scenarios: successful deletion, resource not found, and API errors. The assertions correctly verify the return values and exception handling.
350-394: LGTM - Thorough upload test coverage with proper cleanup.The upload tests comprehensively cover default options, custom options, and transformations. Temporary file cleanup is properly handled in these tests.
Also applies to: 437-538
src/Cms/Controllers/Admin/Media.php (3)
57-92: LGTM - Proper authentication check and request handling.The index method correctly validates user authentication and uses the framework's Request object to safely retrieve the cursor parameter. The view rendering with pagination support is well-structured.
124-169: LGTM - Proper file validation and upload handling.The uploadImage method correctly validates file presence and format before uploading. The Editor.js-compatible JSON response structure is appropriate for the integration.
192-233: LGTM - Consistent validation and upload logic.The uploadFeaturedImage method follows the same validation and upload pattern as uploadImage, with an appropriately different response structure.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
src/Cms/Controllers/Admin/Posts.php (3)
146-184: Missing CSRF token validation instore()method.The
store()method performs a state-changing operation (creating a post) but does not validate the CSRF token. Compare this withEvents.phpwhich validates CSRF before processing instore(). This inconsistency could leave the endpoint vulnerable to CSRF attacks.🔎 Proposed fix
public function store( Request $request ): never { if( !auth() ) { throw new \RuntimeException( 'Authenticated user not found' ); } + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_posts_create', [], ['error', 'Invalid security token. Please try again.'] ); + } + try {
235-287: Missing CSRF token validation inupdate()method.Similar to
store(), theupdate()method modifies data without CSRF validation. TheEvents.phpcontroller validates CSRF tokens in itsupdate()method.🔎 Proposed fix
// Check permissions if( !is_admin() && !is_editor() && $post->getAuthorId() !== user_id() ) { throw new \RuntimeException( 'Unauthorized to edit this post' ); } + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_posts_edit', ['id' => $postId], ['error', 'Invalid security token. Please try again.'] ); + } + try {
294-324: Missing CSRF token validation indestroy()method.The
destroy()method deletes data without CSRF validation. This is inconsistent withEvents.phpwhich validates CSRF before deletion.🔎 Proposed fix
// Check permissions if( !is_admin() && !is_editor() && $post->getAuthorId() !== user_id() ) { $this->redirect( 'admin_posts', [], ['error', 'Unauthorized to delete this post'] ); } + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_posts', [], ['error', 'Invalid security token. Please try again.'] ); + } + try {src/Cms/Controllers/Admin/Users.php (3)
104-126: Missing CSRF token validation instore()method.The
store()method creates a new user without validating the CSRF token. This is inconsistent with other controllers and leaves the endpoint vulnerable to CSRF attacks.🔎 Proposed fix
public function store( Request $request ): never { + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_users_create', [], ['error', 'Invalid security token. Please try again.'] ); + } + $username = $request->post( 'username','' );
168-201: Missing CSRF token validation inupdate()method.The
update()method modifies user data without CSRF validation.🔎 Proposed fix
public function update( Request $request ): never { $id = (int)$request->getRouteParameter( 'id' ); $user = $this->_repository->findById( $id ); if( !$user ) { $this->redirect( 'admin_users', [], ['error', 'User not found'] ); } + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_users_edit', ['id' => $id], ['error', 'Invalid security token. Please try again.'] ); + } + $usernameInput = $request->post( 'username', null );
210-230: Missing CSRF token validation indestroy()method.The
destroy()method deletes a user without CSRF validation.🔎 Proposed fix
public function destroy( Request $request ): never { $id = (int)$request->getRouteParameter( 'id' ); $currentUser = auth(); // Prevent self-deletion if( $currentUser && user_id() === $id ) { $this->redirect( 'admin_users', [], ['error', 'Cannot delete your own account'] ); } + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_users', [], ['error', 'Invalid security token. Please try again.'] ); + } + try {src/Cms/Controllers/Admin/Categories.php (3)
94-109: Missing authentication and CSRF validation instore().This method performs a state-changing operation but lacks auth checks and CSRF validation, unlike the
Pages::store()method. An unauthenticated or CSRF attack could create categories.🔎 Proposed fix
public function store( Request $request ): never { + if( !auth() ) + { + throw new \RuntimeException( 'Authenticated user not found' ); + } + + // Validate CSRF token + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + $this->redirect( 'admin_categories_create', [], ['error', 'Invalid security token. Please try again.'] ); + } + try {
146-169: Missing authentication and CSRF validation inupdate().Same security gap as
store()- no auth or CSRF protection for this state-changing operation.
177-190: Missing authentication and CSRF validation indestroy().The deletion operation lacks security controls. This is the most critical case as it allows potential unauthorized deletion.
src/Cms/Controllers/Admin/Tags.php (3)
84-106: Missing authentication and CSRF validation instore().Same security gap as Categories controller - state-changing operation without auth or CSRF protection.
145-174: Missing authentication and CSRF validation inupdate().No auth check or CSRF validation for updating tags.
183-196: Missing authentication and CSRF validation indestroy().No protection for tag deletion.
🧹 Nitpick comments (10)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (3)
60-149: Excellent comprehensive coverage of upload error codes.The tests thoroughly cover all PHP upload error constants plus an unknown error case, ensuring robust error handling. Each test correctly validates failure and expected error messages.
Optional: Consider consolidating with a data provider
You could reduce code duplication by using a PHPUnit data provider:
/** * @dataProvider uploadErrorProvider */ public function testValidateReturnsFalseForUploadErrors(int $errorCode, string $expectedErrorSubstring): void { $file = [ 'error' => $errorCode, 'tmp_name' => '' ]; $result = $this->_validator->validate($file); $this->assertFalse($result); $this->assertStringContainsString($expectedErrorSubstring, $this->_validator->getFirstError()); } public static function uploadErrorProvider(): array { return [ 'INI_SIZE' => [UPLOAD_ERR_INI_SIZE, 'upload_max_filesize'], 'FORM_SIZE' => [UPLOAD_ERR_FORM_SIZE, 'MAX_FILE_SIZE'], 'PARTIAL' => [UPLOAD_ERR_PARTIAL, 'partially uploaded'], 'NO_TMP_DIR' => [UPLOAD_ERR_NO_TMP_DIR, 'temporary folder'], 'CANT_WRITE' => [UPLOAD_ERR_CANT_WRITE, 'write file to disk'], 'EXTENSION' => [UPLOAD_ERR_EXTENSION, 'extension stopped'], 'UNKNOWN' => [999, 'Unknown'], ]; }This maintains clarity while reducing duplication.
254-282: Strong validation tests using real image data.Using actual base64-encoded images (JPEG, PNG, GIF) rather than mocks ensures the validator correctly handles real file formats. The tests properly verify both successful validation and empty error arrays.
Optional: Consider adding try-finally for resource cleanup
While
tmpfile()auto-cleans on script termination, you could ensure immediate cleanup even if assertions fail:public function testValidatePassesForValidJpegFile(): void { $jpegData = base64_decode(/* ... */); $tmpFile = tmpfile(); try { $tmpPath = stream_get_meta_data($tmpFile)['uri']; fwrite($tmpFile, $jpegData); $file = [/* ... */]; $result = $this->_validator->validate($file); $this->assertTrue($result); $this->assertEmpty($this->_validator->getErrors()); } finally { fclose($tmpFile); } }This is a minor improvement since PHP handles
tmpfile()cleanup automatically.Also applies to: 284-308, 355-379
310-330: Good coverage of invalid file scenarios.These tests appropriately validate that the validator rejects files with MIME type mismatches and corrupt image data. The approach of creating actual invalid files is sound.
Consider adding specific error assertion to the corrupt image test
In
testValidateFailsForNonImageFile(line 332-353), you could make the test more precise by checking the specific error message:$result = $this->_validator->validate($file); -// Should fail either on MIME or getimagesize check $this->assertFalse($result); +$this->assertNotEmpty($this->_validator->getFirstError());This ensures not only that validation fails, but that an error message was properly set.
Also applies to: 332-353
src/Cms/Controllers/Admin/EventCategories.php (1)
75-96: Consider the unused$requestparameter.The
$requestparameter is declared but not used in the method body. If it's not required by the framework signature, you could remove it for clarity.🔎 Proposed fix
-public function create( Request $request ): string +public function create(): stringsrc/Cms/Controllers/Member/Profile.php (1)
60-74: Optional: Consider caching session manager reference.While
getSessionManager()is internally cached, storing it in a local variable could slightly improve readability:$sessionManager = $this->getSessionManager(); $csrfToken = new CsrfToken( $sessionManager ); // ... later in the fluent chain: ->with( 'success', $sessionManager->getFlash( 'success' ) ) ->with( 'error', $sessionManager->getFlash( 'error' ) )This is purely a style preference and the current code is perfectly fine.
src/Cms/Controllers/Admin/Dashboard.php (1)
47-50: Use inheritedgetSessionManager()instead of creating a new instance.The parent class
ContentprovidesgetSessionManager()(as shown in the relevant snippets), which handles session initialization. Creating a newSessionManagerinstance here is inconsistent with other controllers (e.g.,Login.phpuses$this->getSessionManager()) and may cause issues if the parent's session manager has already been initialized.🔎 Proposed fix
- $sessionManager = new SessionManager(); - $sessionManager->start(); - $csrfToken = new CsrfToken( $sessionManager ); + $csrfToken = new CsrfToken( $this->getSessionManager() );src/Cms/Controllers/Admin/Pages.php (2)
124-129: Consider removing unused$uservariable.The
$uservariable is assigned but never used; all subsequent code usesuser_id()instead. This is a minor inconsistency.🔎 Proposed fix
public function store( Request $request ): never { - $user = auth(); - - if( !$user ) + if( !auth() ) { throw new \RuntimeException( 'Authenticated user not found' ); }
234-239: Same issue: unused$uservariable.Similar to
store(), the$uservariable is assigned but never used.tests/Unit/Cms/Controllers/MediaIndexTest.php (1)
73-126: Consider adding more specific assertions.The test only asserts
assertIsString($result). Consider verifying thatlistResourceswas called or checking specific content in the rendered output.src/Cms/Controllers/Admin/Media.php (1)
138-229: Unused$requestparameter per static analysis, but acceptable.The static analysis flagged
$requestas unused because file data comes from$_FILES. Consider using$request->files()if available in the framework for consistency, but the current approach is functional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Cms/Controllers/Admin/Categories.phpsrc/Cms/Controllers/Admin/Dashboard.phpsrc/Cms/Controllers/Admin/EventCategories.phpsrc/Cms/Controllers/Admin/Events.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Controllers/Admin/Pages.phpsrc/Cms/Controllers/Admin/Posts.phpsrc/Cms/Controllers/Admin/Profile.phpsrc/Cms/Controllers/Admin/Tags.phpsrc/Cms/Controllers/Admin/Users.phpsrc/Cms/Controllers/Auth/Login.phpsrc/Cms/Controllers/Auth/PasswordReset.phpsrc/Cms/Controllers/Member/Dashboard.phpsrc/Cms/Controllers/Member/Profile.phpsrc/Cms/Controllers/Member/Registration.phptests/Unit/Cms/Controllers/MediaIndexTest.phptests/Unit/Cms/Controllers/MediaUploadTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderUrlTest.phptests/Unit/Cms/Services/Media/MediaValidatorTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Cms/Controllers/MediaUploadTest.php
🧰 Additional context used
🧬 Code graph analysis (14)
tests/Unit/Cms/Services/Media/CloudinaryUploaderUrlTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (1)
CloudinaryUploader(17-371)
src/Cms/Controllers/Admin/Events.php (2)
src/Cms/Auth/helpers.php (4)
auth(19-22)is_admin(74-78)is_editor(86-90)user_id(41-44)src/Cms/Models/Event.php (1)
getCreatedBy(379-382)
src/Cms/Controllers/Auth/PasswordReset.php (2)
src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Auth/SessionManager.php (1)
getFlash(144-150)
src/Cms/Controllers/Admin/Tags.php (3)
src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Controllers/Blog.php (1)
tag(171-202)
src/Cms/Controllers/Admin/Pages.php (1)
src/Cms/Auth/helpers.php (4)
auth(19-22)is_admin(74-78)is_editor(86-90)user_id(41-44)
src/Cms/Controllers/Admin/Dashboard.php (2)
src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Services/Widget/Widget.php (1)
view(50-61)
src/Cms/Controllers/Auth/Login.php (3)
src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Controllers/Content.php (2)
getName(123-126)getSessionManager(222-230)src/Cms/Auth/SessionManager.php (1)
getFlash(144-150)
src/Cms/Controllers/Admin/Posts.php (3)
src/Cms/Services/Auth/Authentication.php (1)
user(229-252)src/Cms/Auth/helpers.php (4)
auth(19-22)is_admin(74-78)is_editor(86-90)user_id(41-44)src/Cms/Auth/SessionManager.php (1)
getFlash(144-150)
src/Cms/Controllers/Admin/Categories.php (4)
src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Repositories/DatabaseCategoryRepository.php (1)
allWithPostCount(167-183)src/Cms/Controllers/Blog.php (1)
category(211-242)
src/Cms/Controllers/Member/Dashboard.php (2)
src/Cms/Services/Auth/Authentication.php (1)
user(229-252)src/Cms/Auth/helpers.php (1)
auth(19-22)
src/Cms/Controllers/Admin/Media.php (4)
src/Cms/Controllers/Content.php (2)
Content(61-302)getSessionManager(222-230)src/Cms/Services/Media/CloudinaryUploader.php (4)
CloudinaryUploader(17-371)__construct(28-32)listResources(149-184)upload(68-89)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
src/Cms/Controllers/Admin/Users.php (3)
src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Auth/SessionManager.php (1)
getFlash(144-150)src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)
src/Cms/Controllers/Member/Profile.php (4)
src/Cms/Services/Auth/Authentication.php (1)
user(229-252)src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Controllers/Content.php (1)
getSessionManager(222-230)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
src/Cms/Services/Media/MediaValidator.php (1)
getFirstError(188-191)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/Events.php
89-89: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/EventCategories.php
75-75: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/Media.php
138-138: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
240-240: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ 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: build-test (mysql)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (postgres)
🔇 Additional comments (53)
src/Cms/Controllers/Admin/EventCategories.php (5)
155-180: LGTM!The refactoring to the fluent view builder pattern is clean and consistent. The authentication check, category lookup, and error handling are all correctly implemented.
185-233: LGTM!The method correctly validates authentication, category existence, and CSRF token before performing the update. Error handling with flash messages is consistent with the rest of the controller.
238-274: LGTM!The deletion method properly validates all preconditions (authentication, category existence, CSRF token) before performing the destructive operation. The error handling and user feedback are well-structured.
116-116: No action required. Theuser_id()helper function is properly defined insrc/Cms/Auth/helpers.phpand correctly used in the logging statement.
50-53: Theauth()helper function is properly defined insrc/Cms/Auth/helpers.phpand configured in composer.json's autoload files, making it globally available throughout the codebase. The refactoring fromRegistry::getInstance()->get('User')toauth()is sound.src/Cms/Controllers/Member/Profile.php (3)
52-57: LGTM! Good refactoring from Registry to auth() helper.The transition to using the
auth()helper for retrieving the authenticated user is clean and consistent. The null checks with descriptive RuntimeException messages appropriately handle edge cases where the user might not be available despite being in an authenticated context.Also applies to: 87-92
67-75: LGTM! Excellent refactoring to fluent view builder.The transition to the fluent view builder pattern significantly improves readability and maintainability. The chainable methods (
title(),description(),withCurrentUser(),withCsrfToken(),with(),render()) make the view configuration clear and declarative, aligning well with the broader PR objective of modernizing controller view handling.
60-61: No issues found - manual CSRF token generation is necessary.All controllers in the codebase that call
->withCsrfToken()first manually generate the token and store it in the Registry under'Auth.CsrfToken'. This is the established pattern across Login, Registration, PasswordReset, Dashboard, Profile (both Admin and Member), Posts, Users, Events, EventCategories, Pages, and Media controllers. ThewithCsrfToken()method relies on the token being pre-set in the Registry, making lines 60-61 a required prerequisite, not redundant code.src/Cms/Controllers/Auth/Login.php (1)
73-80: LGTM! Clean migration to fluent view builder.The refactor from manual view data assembly to the fluent view builder pattern is consistent with the broader codebase migration. The flash message retrieval via
getSessionManager()->getFlash()correctly handles session data.src/Cms/Controllers/Admin/Dashboard.php (2)
40-44: LGTM! Auth check refactored to use helper.The migration from direct Registry lookup to
auth()helper is consistent with the broader refactor.
52-57: LGTM! Fluent view builder pattern applied correctly.The view rendering is clean and consistent with the codebase-wide migration.
src/Cms/Controllers/Admin/Profile.php (3)
52-52: LGTM! Auth helper usage is consistent.Correctly migrated to use
auth()helper.
67-77: LGTM! Clean fluent view builder implementation.The
with()method correctly passes grouped data including flash messages and timezone options.
88-88: LGTM!Consistent use of
auth()helper in the update method.src/Cms/Controllers/Member/Dashboard.php (2)
40-45: LGTM! Auth helper migration is correct.The refactor from Registry lookup to
auth()helper is consistent with the broader pattern.
51-56: LGTM! Fluent view builder applied correctly.Clean and consistent with other controllers in the codebase.
src/Cms/Controllers/Auth/PasswordReset.php (2)
61-68: LGTM! Fluent view builder for forgot password form.The migration to the fluent view builder is clean. Note: using
$this->_sessionManagerdirectly works if the property is inherited and initialized from the parent class.
154-163: LGTM! Fluent view builder for reset form.Correctly passes the token and email needed for the password reset form.
src/Cms/Controllers/Admin/Events.php (10)
54-57: LGTM! Auth check refactored to use helper.Consistent migration to
auth()helper.
64-71: LGTM! Role checks refactored to use helpers.The migration from
$user->isAdmin()tois_admin()andis_editor()helpers is consistent with the codebase-wide pattern. Usinguser_id()for the repository query is appropriate.
73-84: LGTM! Fluent view builder applied correctly.Clean implementation with proper flash message handling.
89-106: The$requestparameter is required by the route handler contract.The static analysis hint about unused
$requestis a false positive—the parameter is required to match the expected controller action signature even if not directly used in this method.
126-127: LGTM! Logging usesuser_id()helper.Consistent use of the helper for logging the user ID in CSRF failure scenarios.
147-163: LGTM! Creator call usesuser_id()helper.Correctly passes the user ID from the helper to the creator service.
192-195: LGTM! Permission check uses helpers consistently.The ownership check correctly compares
getCreatedBy()withuser_id().
200-209: LGTM! Fluent view builder for edit form.Clean and consistent implementation.
233-236: LGTM! Permission check in update is consistent.Same pattern as edit method for authorization.
312-315: LGTM! Permission check in destroy is consistent.Note that unlike
edit()andupdate()which throwRuntimeException, this method redirects on unauthorized access. This is intentional—DELETE operations via form submission may prefer a redirect over an exception page.src/Cms/Controllers/Admin/Posts.php (3)
79-84: LGTM! Auth check refactored to use helper.Consistent migration pattern.
92-99: LGTM! Role checks use helper functions.The migration to
is_admin()andis_editor()helpers is consistent. Using$user->getUsername()for non-privileged users is appropriate for author-based filtering.
101-111: LGTM! Fluent view builder applied correctly.Clean implementation with proper flash message handling.
src/Cms/Controllers/Admin/Users.php (4)
62-74: LGTM! Fluent view builder applied correctly.Clean implementation for the user listing page.
89-95: LGTM! Fluent view builder for create form.Correctly passes the available roles to the view.
149-158: LGTM! Fluent view builder for edit form.Correctly passes the user and available roles to the view.
213-216: LGTM! Self-deletion check uses helpers correctly.The migration to
auth()anduser_id()for the self-deletion prevention is correct.src/Cms/Controllers/Admin/Pages.php (4)
56-89: LGTM! Well-structured index method with proper auth and fluent view rendering.The migration to
auth()helper and fluentview()builder is consistent with the broader refactor. The CSRF token is properly generated and stored in the registry, and flash messages are correctly retrieved.
97-114: LGTM!The create method follows the established pattern with proper auth check and CSRF token generation.
191-224: LGTM!The edit method correctly implements authorization checks, ensuring users can only edit their own pages unless they are admins or editors.
315-372: LGTM with same minor note about unused$user.Good practice storing
$pageTitlebefore deletion for accurate logging.src/Cms/Controllers/Member/Registration.php (3)
69-90: LGTM!The registration form properly generates CSRF tokens and uses the fluent view builder with flash message handling.
153-160: LGTM!Clean migration to fluent view rendering for the verification sent page.
169-217: LGTM! Proper HTTP status codes for verification outcomes.Good use of
HttpResponseStatus::BAD_REQUESTfor invalid tokens andHttpResponseStatus::INTERNAL_SERVER_ERRORfor exceptions.tests/Unit/Cms/Controllers/MediaIndexTest.php (3)
59-71: LGTM!Good test for unauthenticated access, verifying the exception type and message.
128-173: Good test for cursor parameter propagation.The test properly verifies that the cursor parameter is passed to
listResourceswith expected options.
175-212: Good exception handling test.Verifies that the controller gracefully handles Cloudinary API errors and returns HTML instead of throwing.
tests/Unit/Cms/Services/Media/CloudinaryUploaderUrlTest.php (4)
62-111: Excellent SSRF protection test coverage.Tests cover loopback (127.0.0.1), private ranges (10.x, 192.168.x, 172.16.x), and link-local/cloud metadata (169.254.169.254). This is critical for security.
113-149: LGTM!Good test for the happy path with proper mocking and assertions on both the API call and the result.
191-212: LGTM!Properly tests that API errors are wrapped with context in the exception message.
32-40: This test is correct as-is; no changes needed.PHPUnit's
expectExceptionMessage()performs a substring match, not an exact match. The test expects'Invalid URL', and the actual exception message"Invalid URL format: {$url}"contains this substring, so the test will pass correctly.Likely an incorrect or invalid review comment.
src/Cms/Controllers/Admin/Media.php (4)
59-127: Good implementation with proper error handling and generic error messages.The index method correctly logs detailed errors internally while showing generic messages to users, addressing the past review concern. CSRF token is properly generated and stored.
140-153: Good security: auth check with logging.Proper authorization check with warning log for unauthorized attempts.
212-228: Past review concern addressed: generic error message.The exception handler now logs detailed errors internally and returns a generic "Upload failed. Please try again." message to users.
240-327: Good implementation with consistent error handling.The
uploadFeaturedImagemethod follows the same pattern asuploadImagewith proper auth checks, logging, and generic error messages.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Cms/Controllers/Admin/Categories.php (2)
60-62: Extract duplicated CSRF token generation.The CSRF token generation pattern is repeated identically in
index(),create(), andedit()methods. Extract this into a protected helper method to improve maintainability.🔎 Proposed refactor
Add this protected method to the class:
/** * Generate and store CSRF token in Registry */ protected function ensureCsrfToken(): void { $csrfToken = new \Neuron\Cms\Services\Auth\CsrfToken( $this->getSessionManager() ); \Neuron\Patterns\Registry::getInstance()->set( 'Auth.CsrfToken', $csrfToken->getToken() ); }Then replace each occurrence with:
- // Generate CSRF token - $csrfToken = new \Neuron\Cms\Services\Auth\CsrfToken( $this->getSessionManager() ); - \Neuron\Patterns\Registry::getInstance()->set( 'Auth.CsrfToken', $csrfToken->getToken() ); + $this->ensureCsrfToken();Also applies to: 86-88, 142-144
53-53: Consider removing unused$requestparameters.The
$requestparameter inindex()andcreate()methods is declared but never used. If these parameters aren't required by a parent class signature or interface contract, consider removing them to reduce noise.Also applies to: 79-79
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Cms/Controllers/Admin/Categories.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Controllers/Admin/Tags.phptests/Unit/Cms/Controllers/MediaIndexTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/Cms/Controllers/MediaIndexTest.php (1)
src/Cms/Controllers/Admin/Media.php (2)
Media(23-345)index(59-144)
src/Cms/Controllers/Admin/Categories.php (3)
src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Controllers/Content.php (1)
getSessionManager(222-230)
src/Cms/Controllers/Admin/Tags.php (4)
src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Controllers/Content.php (1)
getSessionManager(222-230)src/Cms/Auth/SessionManager.php (1)
set(99-103)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/Media.php
155-155: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
257-257: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ 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: build-test (postgres)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (13)
src/Cms/Controllers/Admin/Tags.php (2)
45-48: LGTM: Consistent authentication pattern.The centralized
auth()helper usage is consistent across all view-rendering methods and aligns with the broader refactoring effort described in the AI summary.Also applies to: 71-74, 127-130
54-60: LGTM: Clean fluent view builder pattern.The consistent use of the fluent view builder pattern across
index(),create(), andedit()is a clean improvement over the previous manual approach. Each method appropriately passes the necessary data to its respective view.Also applies to: 80-85, 144-150
tests/Unit/Cms/Controllers/MediaIndexTest.php (5)
24-57: LGTM! Excellent test isolation.The setUp and tearDown methods properly preserve and restore Registry state, ensuring test isolation. Using an in-memory SettingManager avoids external dependencies during testing.
59-71: LGTM! Clear authentication test.The test correctly verifies that the index method throws a RuntimeException when no authenticated user is present, matching the implementation's authentication guard.
73-131: LGTM! Comprehensive success path test.The test properly uses reflection to inject mocks and validates the successful rendering path. The mock setup for SessionManager and CloudinaryUploader is well-structured.
133-183: LGTM! Thorough cursor parameter validation.The test correctly validates that the cursor parameter is passed to listResources with the expected options structure, using a callback assertion to verify both
next_cursorandmax_resultsvalues.
185-227: LGTM! Proper exception handling test.The test verifies that exceptions from listResources are caught and handled gracefully, returning an HTML string with an error message instead of throwing. This matches the defensive error handling in the implementation.
src/Cms/Controllers/Admin/Media.php (4)
23-47: LGTM! Clean constructor implementation.The constructor properly extends Content, accepts an optional Application parameter, and initializes the uploader and validator with settings from the Registry. Error handling for missing settings is appropriate.
59-144: LGTM! Excellent security and error handling.The index method properly addresses previous review concerns:
- Cursor validation (lines 78-97): Validates cursor format with regex before passing to the API, with security logging for invalid attempts.
- Error handling (lines 123-143): Catches exceptions, logs detailed error information, and returns a generic error message to users instead of exposing internal details.
The implementation includes proper authentication checks, CSRF token generation, and graceful degradation on errors.
155-246: LGTM! Robust upload implementation with proper logging.The uploadImage method includes comprehensive authentication checks, validation, and error handling. Previous concerns about exception message exposure have been addressed—generic error messages are returned to users while detailed information is logged for debugging.
Note on static analysis: The "unused parameter" warning for
$requestis a false positive. The parameter is required by the route handler signature and may be used for framework-level processing (e.g., middleware, request context).
257-344: LGTM! Consistent with uploadImage implementation.The uploadFeaturedImage method follows the same robust pattern as uploadImage, with proper authentication, validation, logging, and error handling. Previous concerns about exception message exposure have been addressed.
Note on static analysis: The "unused parameter" warning is a false positive—the
$requestparameter is required by the route handler signature.src/Cms/Controllers/Admin/Categories.php (2)
55-70: CSRF token successfully added per previous review.The previous review concern about missing CSRF protection has been addressed. The implementation correctly generates a CSRF token, stores it in the Registry, and passes it to the view via
withCsrfToken().
104-119: Add CSRF token validation to mutation methods.The
store(),update(), anddestroy()methods generate CSRF tokens in forms but do not validate them before processing mutations. Implement CSRF validation consistent with the pattern used in Events, Pages, and EventCategories controllers (e.g.,$csrfToken->validate($submittedToken)in each mutation method).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Cms/Controllers/Admin/Posts.php (3)
146-179: Missing CSRF validation instore()method.Unlike
Tags.phpandCategories.phpwhich validate CSRF tokens in their mutating actions,Posts.php::store()does not validate the submitted CSRF token before processing the form data. While the route hasauth-csrffilter, the other controllers in this PR implement controller-level validation as defense-in-depth.🔎 Proposed fix to add CSRF validation
public function store( Request $request ): never { + if( !auth() ) + { + throw new \RuntimeException( 'Authenticated user not found' ); + } + + // Validate CSRF token before any state changes + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + \Neuron\Log\Log::warning( 'CSRF validation failed for post creation by user ' . user_id() ); + $this->redirect( 'admin_posts_create', [], ['error', 'Invalid security token. Please try again.'] ); + } + try {
230-278: Missing CSRF validation inupdate()method.The
update()method should validate the CSRF token before processing the update, consistent withTags.phpandCategories.php.🔎 Proposed fix
public function update( Request $request ): never { + if( !auth() ) + { + throw new \RuntimeException( 'Authenticated user not found' ); + } $postId = (int)$request->getRouteParameter( 'id' ); $post = $this->_postRepository->findById( $postId ); if( !$post ) { $this->redirect( 'admin_posts', [], ['error', 'Post not found'] ); } // Check permissions if( !is_admin() && !is_editor() && $post->getAuthorId() !== user_id() ) { throw new \RuntimeException( 'Unauthorized to edit this post' ); } + // Validate CSRF token before any state changes + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + \Neuron\Log\Log::warning( "CSRF validation failed for post update: Post {$postId}, user " . user_id() ); + $this->redirect( 'admin_posts_edit', ['id' => $postId], ['error', 'Invalid security token. Please try again.'] ); + } + try {
285-311: Missing CSRF validation indestroy()method.The
destroy()method should also validate the CSRF token before deleting the post, consistent with the pattern inTags.phpandCategories.php.🔎 Proposed fix
public function destroy( Request $request ): never { + if( !auth() ) + { + throw new \RuntimeException( 'Authenticated user not found' ); + } $postId = (int)$request->getRouteParameter( 'id' ); $post = $this->_postRepository->findById( $postId ); if( !$post ) { $this->redirect( 'admin_posts', [], ['error', 'Post not found'] ); } // Check permissions if( !is_admin() && !is_editor() && $post->getAuthorId() !== user_id() ) { $this->redirect( 'admin_posts', [], ['error', 'Unauthorized to delete this post'] ); } + // Validate CSRF token before any state changes + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + \Neuron\Log\Log::warning( "CSRF validation failed for post deletion: Post {$postId}, user " . user_id() ); + $this->redirect( 'admin_posts', [], ['error', 'Invalid security token. Please try again.'] ); + } + try {
🧹 Nitpick comments (1)
src/Cms/Controllers/Admin/Categories.php (1)
60-62: Consider extracting CSRF initialization to a helper method like Tags.php.The CSRF token generation block is duplicated in
index(),create(), andedit().Tags.phpaddresses this with aninitializeCsrfToken()helper method. For consistency across admin controllers, consider the same extraction here.🔎 Proposed refactor
Add a protected helper method:
/** * Initialize CSRF token and store in Registry * @return void */ protected function initializeCsrfToken(): void { $csrfToken = new \Neuron\Cms\Services\Auth\CsrfToken( $this->getSessionManager() ); \Neuron\Patterns\Registry::getInstance()->set( 'Auth.CsrfToken', $csrfToken->getToken() ); }Then replace each occurrence:
- // Generate CSRF token - $csrfToken = new \Neuron\Cms\Services\Auth\CsrfToken( $this->getSessionManager() ); - \Neuron\Patterns\Registry::getInstance()->set( 'Auth.CsrfToken', $csrfToken->getToken() ); + $this->initializeCsrfToken();Also applies to: 86-88, 157-159
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/config/routes.yamlsrc/Cms/Controllers/Admin/Categories.phpsrc/Cms/Controllers/Admin/Posts.phpsrc/Cms/Controllers/Admin/Tags.phpsrc/Cms/Controllers/Admin/Users.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Cms/Controllers/Admin/Users.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Cms/Controllers/Admin/Categories.php (4)
src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Controllers/Content.php (1)
getSessionManager(222-230)src/Cms/Auth/SessionManager.php (1)
set(99-103)
src/Cms/Controllers/Admin/Posts.php (3)
src/Cms/Auth/helpers.php (4)
auth(19-22)is_admin(74-78)is_editor(86-90)user_id(41-44)src/Cms/Services/Widget/Widget.php (1)
view(50-61)src/Cms/Auth/SessionManager.php (1)
getFlash(144-150)
⏰ 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: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (16)
src/Cms/Controllers/Admin/Posts.php (3)
77-112: Refactoredindex()looks good with fluent view builder.The migration to
auth()helper and fluent view builder pattern is clean and consistent. CSRF token generation and the conditional post retrieval based on user role work correctly.
120-138:create()method follows the new pattern correctly.Good use of
auth()check, CSRF token generation, and fluent view builder.
187-222:edit()method correctly implements authorization checks.The permission check at line 203 properly ensures only admins, editors, or the post author can edit. The fluent view builder usage is consistent.
src/Cms/Controllers/Admin/Tags.php (5)
37-46: Good extraction of CSRF token initialization into a helper method.This addresses the previous review comment about DRY violations. The helper is clean and reusable across
index(),create(), andedit()methods.
101-137: CSRF validation instore()is well-implemented.The validation occurs before any state changes, logs failures with user context, and provides a user-friendly error message with redirect.
180-224: CSRF validation inupdate()follows consistent pattern.Good placement after resource lookup but before any modifications.
233-261: CSRF validation indestroy()is correct.Validates token before deletion with appropriate logging and error handling.
273-287: Slug generation handles edge cases well.The fallback using
uniqid()for non-ASCII names is a pragmatic solution. The docblock clearly documents this behavior.src/Cms/Controllers/Admin/Categories.php (3)
104-134: CSRF validation instore()is properly implemented.Validation occurs before any state changes with appropriate logging and user feedback.
176-214: CSRF validation inupdate()follows the correct pattern.Good placement after resource lookup but before modifications.
222-250: CSRF validation indestroy()is correct.Validates token before deletion with proper error handling.
resources/config/routes.yaml (5)
16-21: Good addition of CSRF protection on logout.The
auth-csrffilter on the logout route prevents CSRF attacks that could log users out maliciously.
254-259: New media management route follows the existing pattern.The
admin_mediaGET route withauthfilter is consistent with other admin listing routes.
337-341: Appropriate use ofcsrffilter withoutauthfor resend verification.Using just
csrf(withoutauth) makes sense here since users resending verification emails may not yet be fully authenticated.
363-367:member-csrffilter correctly applied to member profile update.This ensures members are authenticated and CSRF-protected when updating their profiles.
4-457: Consistent CSRF protection pattern across all state-changing routes.The systematic application of
auth-csrfto all POST, PUT, and DELETE admin routes is a solid security improvement. The pattern is consistent throughout the file.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Cms/Controllers/Admin/Users.php (1)
123-145: Missing CSRF validation in store action.Unlike the
CategoriesandEventCategoriescontrollers in this PR, thestore()action does not validate CSRF tokens before creating users. This leaves user creation vulnerable to CSRF attacks.🔎 Proposed fix
Add CSRF validation before processing the form data:
public function store( Request $request ): never { + // Validate CSRF token before any state changes + $csrfToken = new CsrfToken( $this->getSessionManager() ); + $submittedToken = $request->post( 'csrf_token', '' ); + + if( !$csrfToken->validate( $submittedToken ) ) + { + \Neuron\Log\Log::warning( 'CSRF validation failed for user creation by user ' . user_id() ); + $this->redirect( 'admin_users_create', [], ['error', 'Invalid security token. Please try again.'] ); + } + $username = $request->post( 'username','' ); $email = $request->post( 'email', '' ); $password = $request->post( 'password', '' ); $role = $request->post( 'role', User::ROLE_SUBSCRIBER );
🧹 Nitpick comments (7)
tests/Unit/Cms/Controllers/UsersControllerTest.php (1)
64-230: Consider extracting common test setup logic.The three test methods share substantial setup code (mocking controller with view, creating ViewContext chain, injecting SessionManager via reflection). While the current approach is explicit and readable, extracting a helper method could reduce duplication.
💡 Optional refactor to reduce duplication
Consider adding a helper method:
private function createControllerWithMockedView( DatabaseUserRepository $repository, Creator $creator, Updater $updater, Deleter $deleter ): Users { $controller = $this->getMockBuilder( Users::class ) ->setConstructorArgs([ null, $repository, $creator, $updater, $deleter ]) ->onlyMethods( ['view'] ) ->getMock(); // Mock the view builder chain $viewBuilder = $this->createMock( ViewContext::class ); $viewBuilder->method( 'title' )->willReturnSelf(); $viewBuilder->method( 'description' )->willReturnSelf(); $viewBuilder->method( 'withCurrentUser' )->willReturnSelf(); $viewBuilder->method( 'withCsrfToken' )->willReturnSelf(); $viewBuilder->method( 'with' )->willReturnSelf(); $viewBuilder->method( 'render' )->willReturnArgument( 0 ); $controller->method( 'view' )->willReturn( $viewBuilder ); // Inject session manager $reflection = new \ReflectionClass( get_parent_class( Users::class ) ); $sessionProperty = $reflection->getProperty( '_sessionManager' ); $sessionProperty->setAccessible( true ); $sessionManager = $this->createMock( SessionManager::class ); $sessionManager->method( 'getFlash' )->willReturn( null ); $sessionProperty->setValue( $controller, $sessionManager ); return $controller; }Then simplify each test to focus on the specific scenario being tested.
tests/Unit/Cms/Controllers/CategoriesControllerTest.php (1)
91-223: Consider extracting common test setup logic.Similar to the UsersControllerTest, these test methods share substantial setup code for mocking the controller and view builder chain. Extracting a helper method would reduce duplication.
src/Cms/Controllers/Admin/Media.php (3)
36-60: Consider decoupling validator initialization.The constructor couples
$uploaderand$validatorinitialization: both are created when$uploaderis null, but if a caller provides$uploaderwithout$validator, the validator remains uninitialized (null), which will cause runtime errors on line 202 and 304.🔎 Proposed fix to ensure validator is always initialized
public function __construct( ?Application $app = null, ?CloudinaryUploader $uploader = null, ?MediaValidator $validator = null ) { parent::__construct( $app ); - // Use injected dependencies if provided (for testing), otherwise create them (for production) - if( $uploader === null ) + // Use injected dependencies if provided (for testing), otherwise create them (for production) + if( $uploader === null || $validator === null ) { $settings = Registry::getInstance()->get( 'Settings' ); if( !$settings instanceof SettingManager ) { throw new \Exception( 'Settings not found in Registry' ); } - $uploader = new CloudinaryUploader( $settings ); - $validator = new MediaValidator( $settings ); + $uploader = $uploader ?? new CloudinaryUploader( $settings ); + $validator = $validator ?? new MediaValidator( $settings ); } $this->_uploader = $uploader; $this->_validator = $validator; }
168-259: Use Request object to access uploaded files.The
$requestparameter is accepted but unused—the method directly accesses$_FILES['image']. For consistency and testability, retrieve the file from the Request object instead.🔎 Proposed fix to use Request object
public function uploadImage( Request $request ): string { $user = auth(); if( !$user ) { Log::warning( 'Unauthorized image upload attempt' ); return $this->renderJson( HttpResponseStatus::UNAUTHORIZED, [ 'success' => 0, 'message' => 'Unauthorized' ] ); } try { + $files = $request->getFiles(); + // Check if file was uploaded - if( !isset( $_FILES['image'] ) ) + if( !isset( $files['image'] ) ) { return $this->renderJson( HttpResponseStatus::BAD_REQUEST, [ 'success' => 0, 'message' => 'No file was uploaded' ] ); } - $file = $_FILES['image']; + $file = $files['image']; // ... rest of method } catch( \Exception $e ) { Log::error( 'Image upload failed', [ 'user_id' => user_id(), - 'filename' => $_FILES['image']['name'] ?? 'unknown', + 'filename' => $files['image']['name'] ?? 'unknown', 'exception' => $e, 'message' => $e->getMessage() ] ); // ... rest of error handling } }
270-357: Use Request object to access uploaded files.Same issue as
uploadImage()—the$requestparameter is unused while the method directly accesses$_FILES. Apply the same refactor to use$request->getFiles()for consistency.🔎 Proposed fix
public function uploadFeaturedImage( Request $request ): string { $user = auth(); if( !$user ) { Log::warning( 'Unauthorized featured image upload attempt' ); return $this->renderJson( HttpResponseStatus::UNAUTHORIZED, [ 'success' => false, 'error' => 'Unauthorized' ] ); } try { + $files = $request->getFiles(); + // Check if file was uploaded - if( !isset( $_FILES['image'] ) ) + if( !isset( $files['image'] ) ) { return $this->renderJson( HttpResponseStatus::BAD_REQUEST, [ 'success' => false, 'error' => 'No file was uploaded' ] ); } - $file = $_FILES['image']; + $file = $files['image']; // ... rest of method } catch( \Exception $e ) { Log::error( 'Featured image upload failed', [ 'user_id' => user_id(), - 'filename' => $_FILES['image']['name'] ?? 'unknown', + 'filename' => $files['image']['name'] ?? 'unknown', 'exception' => $e, 'message' => $e->getMessage() ] ); // ... rest of error handling } }tests/Unit/Cms/Controllers/TagsControllerTest.php (2)
82-132: Rendering tests verify view builder integration.The tests for index and create correctly verify that:
- Authenticated requests proceed to rendering
- Repository data flows to the view (in index)
- View builder chains are invoked properly
The reflection-based session manager injection is necessary given the current architecture.
The view builder mocking pattern (lines 108-116, 172-177) is duplicated. Consider extracting it to a private helper method to reduce repetition across test methods.
Also applies to: 154-195
1-303: Consider adding success path tests for CRUD operations.The current test suite focuses on authentication guards and error cases but lacks tests for successful operations:
- Store: creating a new tag with valid data
- Edit: rendering the form with an existing tag
- Update: modifying an existing tag
- Destroy: successfully deleting a tag
These tests would verify that the happy paths work correctly and provide better regression protection.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/Cms/Controllers/Admin/Categories.phpsrc/Cms/Controllers/Admin/EventCategories.phpsrc/Cms/Controllers/Admin/Events.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Controllers/Admin/Pages.phpsrc/Cms/Controllers/Admin/Posts.phpsrc/Cms/Controllers/Admin/Profile.phpsrc/Cms/Controllers/Admin/Tags.phpsrc/Cms/Controllers/Admin/Users.phptests/Unit/Cms/Controllers/CategoriesControllerTest.phptests/Unit/Cms/Controllers/EventCategoriesControllerTest.phptests/Unit/Cms/Controllers/EventsControllerTest.phptests/Unit/Cms/Controllers/PagesControllerTest.phptests/Unit/Cms/Controllers/PostsControllerTest.phptests/Unit/Cms/Controllers/TagsControllerTest.phptests/Unit/Cms/Controllers/UsersControllerTest.php
🧰 Additional context used
🧬 Code graph analysis (12)
src/Cms/Controllers/Admin/EventCategories.php (5)
src/Cms/Controllers/Admin/Categories.php (3)
__construct(37-64)create(98-115)update(195-233)src/Cms/Repositories/DatabaseEventCategoryRepository.php (4)
DatabaseEventCategoryRepository(19-181)all(40-46)create(103-126)update(131-152)src/Cms/Services/EventCategory/Creator.php (2)
Creator(13-75)create(32-52)src/Cms/Services/EventCategory/Updater.php (2)
Updater(13-54)update(33-53)src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)
tests/Unit/Cms/Controllers/UsersControllerTest.php (6)
src/Cms/Controllers/Admin/Users.php (5)
Users(24-259)create(102-115)edit(154-178)update(187-225)destroy(234-258)tests/Unit/Cms/Controllers/CategoriesControllerTest.php (2)
setUp(26-52)tearDown(54-63)tests/Unit/Cms/Controllers/EventsControllerTest.php (2)
setUp(27-53)tearDown(55-64)tests/Unit/Cms/Controllers/PagesControllerTest.php (2)
setUp(26-52)tearDown(54-63)tests/Unit/Cms/Controllers/PostsControllerTest.php (2)
setUp(28-54)tearDown(56-65)tests/Unit/Cms/Controllers/TagsControllerTest.php (2)
setUp(23-49)tearDown(51-60)
src/Cms/Controllers/Admin/Profile.php (3)
src/Cms/Controllers/Admin/Tags.php (1)
__construct(27-45)src/Cms/Controllers/Admin/Users.php (1)
__construct(39-67)src/Cms/Auth/helpers.php (1)
auth(19-22)
src/Cms/Controllers/Admin/Pages.php (4)
src/Cms/Controllers/Admin/Tags.php (1)
__construct(27-45)src/Cms/Repositories/DatabasePageRepository.php (3)
DatabasePageRepository(20-196)all(115-132)getByAuthor(153-163)src/Cms/Auth/helpers.php (4)
auth(19-22)is_admin(74-78)is_editor(86-90)user_id(41-44)src/Cms/Repositories/IPageRepository.php (2)
all(62-62)getByAuthor(87-87)
src/Cms/Controllers/Admin/Events.php (2)
src/Cms/Auth/helpers.php (4)
auth(19-22)is_admin(74-78)is_editor(86-90)user_id(41-44)src/Cms/Repositories/IEventRepository.php (3)
all(20-20)getByCreator(81-81)create(89-89)
src/Cms/Controllers/Admin/Users.php (4)
src/Cms/Controllers/Admin/Categories.php (1)
__construct(37-64)src/Cms/Controllers/Admin/Pages.php (1)
__construct(38-67)src/Cms/Controllers/Admin/Tags.php (1)
__construct(27-45)src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)
src/Cms/Controllers/Admin/Tags.php (6)
src/Cms/Controllers/Admin/Media.php (1)
__construct(36-60)src/Cms/Controllers/Admin/Posts.php (1)
__construct(44-94)src/Cms/Auth/SessionManager.php (2)
get(108-112)set(99-103)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Controllers/Content.php (2)
getSessionManager(222-230)redirect(240-251)src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)
tests/Unit/Cms/Controllers/TagsControllerTest.php (1)
src/Cms/Controllers/Admin/Tags.php (7)
Tags(18-298)index(64-80)create(88-103)store(111-148)edit(157-181)update(190-234)destroy(243-271)
tests/Unit/Cms/Controllers/EventCategoriesControllerTest.php (1)
src/Cms/Controllers/Admin/EventCategories.php (3)
EventCategories(22-294)index(67-89)create(94-115)
src/Cms/Controllers/Admin/Categories.php (4)
src/Cms/Controllers/Admin/Pages.php (1)
__construct(38-67)src/Cms/Controllers/Admin/Tags.php (1)
__construct(27-45)src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)
src/Cms/Controllers/Admin/Media.php (6)
src/Cms/Controllers/Content.php (2)
Content(61-302)getSessionManager(222-230)src/Cms/Services/Media/CloudinaryUploader.php (4)
CloudinaryUploader(17-371)__construct(28-32)listResources(149-184)upload(68-89)src/Cms/Services/Media/MediaValidator.php (3)
MediaValidator(14-192)validate(35-73)getFirstError(188-191)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Auth/helpers.php (2)
auth(19-22)user_id(41-44)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
tests/Unit/Cms/Controllers/EventsControllerTest.php (3)
src/Cms/Controllers/Admin/Events.php (3)
Events(24-359)index(74-106)create(111-128)src/Cms/Controllers/Admin/Categories.php (2)
index(72-90)create(98-115)src/Cms/Controllers/Admin/Tags.php (2)
index(64-80)create(88-103)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/EventCategories.php
67-67: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
94-94: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/Events.php
74-74: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
111-111: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/Media.php
168-168: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
270-270: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ 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: build-test (postgres)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (70)
src/Cms/Controllers/Admin/Profile.php (3)
34-58: LGTM! Dependency injection pattern implemented correctly.The constructor now supports optional dependency injection for testing while maintaining backward compatibility with lazy initialization for production use. This aligns with the DI pattern used across other admin controllers in this PR.
66-94: LGTM! Authentication and view rendering improvements.The changes successfully migrate from direct Registry access to the centralized
auth()helper and adopt the fluent view builder pattern. The CSRF token is correctly generated and stored.
102-151: LGTM! Update method correctly uses centralized authentication.The method properly uses
auth()for authentication checks and maintains the existing password validation and update logic with appropriate error handling.src/Cms/Controllers/Admin/Posts.php (7)
44-94: LGTM! Comprehensive dependency injection setup.The constructor properly implements the DI pattern with conditional lazy initialization for all required repositories and services. This enhances testability while maintaining production functionality.
102-137: LGTM! Authentication and role-based filtering implemented correctly.The method correctly uses
auth()for authentication, leveragesis_admin()/is_editor()helpers for role checks, and appliesuser_id()for author-based filtering. The fluent view builder migration is clean.
145-163: LGTM! Create form rendering streamlined.The authentication check and view rendering follow the established pattern. Category data is properly passed to the view.
171-204: LGTM! Post creation uses centralized user ID.The store method correctly uses
user_id()instead of passing a User object, aligning with the new authentication pattern.
212-247: LGTM! Edit authorization correctly checks ownership.The permission check at line 228 properly validates that non-admin/non-editor users can only edit their own posts using
user_id().
255-303: LGTM! Update authorization consistent with edit.The ownership check at line 267 mirrors the edit method's authorization logic, ensuring consistency.
310-336: LGTM! Delete authorization properly enforced.The destroy method correctly enforces the same ownership rules as edit and update operations.
tests/Unit/Cms/Controllers/PostsControllerTest.php (7)
28-65: Test setup/teardown implemented correctly.The setUp and tearDown methods properly preserve and restore registry state, preventing test pollution. The mocked Settings provides necessary defaults for the parent Content controller.
67-95: LGTM! Authentication guard test is comprehensive.This test correctly verifies that the index method throws a RuntimeException when no user is authenticated, validating the security requirement.
97-164: LGTM! Admin path test validates rendering flow.The test properly mocks the repository, view builder chain, and session manager using reflection. The assertion confirms that a rendered HTML string is returned.
166-231: LGTM! Non-admin filtering test validates authorization.This test ensures that non-admin users see only their own posts via the
getByAuthor()repository method with the correct user ID.
233-289: LGTM! Create form test validates authenticated rendering.The test verifies that authenticated users can access the create form and that categories are fetched from the repository.
291-319: LGTM! Create authentication guard test is correct.This test validates that unauthenticated users cannot access the create form.
321-389: LGTM! Edit authorization tests are comprehensive.Both tests validate authentication requirements and ownership-based authorization for the edit action.
tests/Unit/Cms/Controllers/EventsControllerTest.php (5)
27-64: LGTM! Test lifecycle methods follow established pattern.The setUp and tearDown methods properly manage registry state, consistent with other controller tests in this PR.
66-92: LGTM! Authentication guard test validates security.The test correctly verifies that unauthenticated access to the index method is prevented.
94-157: LGTM! Admin access test validates rendering.The test properly validates that admins see all events and the view is rendered correctly.
159-221: LGTM! Creator filtering test validates authorization.This test ensures non-admin users see only events they created, using the
getByCreator()method with the correct user ID.
223-305: LGTM! Create form tests validate authentication flow.Both tests correctly verify authentication requirements and form rendering for the create action.
tests/Unit/Cms/Controllers/EventCategoriesControllerTest.php (3)
26-63: LGTM! Test setup follows project convention.The setUp and tearDown methods properly manage registry state, consistent with other test files.
65-149: LGTM! Index tests validate authentication and rendering.Both tests correctly verify authentication requirements and that all categories are retrieved and rendered.
151-226: LGTM! Create tests validate form access control.The tests properly verify authentication requirements for accessing the create form.
src/Cms/Controllers/Admin/Events.php (7)
41-69: LGTM! Dependency injection properly implemented.The constructor follows the established DI pattern with conditional lazy initialization for repositories and services.
74-106: LGTM! Index method correctly implements role-based access.The method properly uses
auth()for authentication,is_admin()/is_editor()for role checks, anduser_id()for creator-based filtering. The fluent view builder migration is clean.Note: The static analysis warning about unused
$requestparameter is a false positive - the parameter is required by the controller method signature.
111-128: LGTM! Create form rendering follows pattern.The authentication check and fluent view builder usage are consistent with other controllers.
Note: The static analysis warning about unused
$requestparameter is a false positive.
133-193: LGTM! Store method uses centralized user ID and proper logging.The method correctly uses
user_id()for event creation (line 172) and logging (line 148), maintaining consistency with the new authentication pattern.
198-232: LGTM! Edit authorization correctly enforces ownership.The permission check at line 214 properly validates that non-admin/non-editor users can only edit events they created.
237-311: LGTM! Update method maintains authorization consistency.The ownership check at line 255 and logging at line 266 use
user_id()correctly.
316-358: LGTM! Destroy method enforces proper authorization.The permission check at line 334 and logging at line 345 correctly use
user_id().src/Cms/Controllers/Admin/Pages.php (7)
38-67: LGTM! Dependency injection implementation is correct.The constructor properly supports optional DI with lazy initialization for page repository and services.
75-108: LGTM! Index method implements role-based filtering.The method correctly uses centralized auth helpers and fluent view builder. Admin/editor users see all pages while others see only their authored pages.
116-133: LGTM! Create form rendering is streamlined.The authentication check and fluent view builder usage follow the established pattern.
141-202: LGTM! Store method with comprehensive logging.The method correctly uses
user_id()for page creation (line 176) and includes detailed logging at lines 187, 191, and 196 for success, failure, and exception scenarios.
210-243: LGTM! Edit authorization with security logging.The permission check at line 226 and unauthorized access logging at line 228 properly use
user_id().
251-327: LGTM! Update method with comprehensive logging.The authorization checks and logging consistently use
user_id()at lines 269, 271, 281, 312, 316, and 321.
334-391: LGTM! Destroy method with proper authorization and logging.The permission checks and logging at lines 352, 354, 364, 376, 380, and 385 correctly use
user_id().tests/Unit/Cms/Controllers/PagesControllerTest.php (4)
26-63: LGTM! Test lifecycle methods are consistent.The setUp and tearDown methods properly manage registry state, following the pattern used in other controller tests.
65-153: LGTM! Authentication and admin access tests are correct.The tests properly validate authentication requirements and admin access to all pages.
155-216: LGTM! Author filtering test validates authorization.This test ensures non-admin users see only their authored pages using
getByAuthor()with the correct user ID.
218-318: LGTM! Create and edit authentication tests are comprehensive.All tests correctly verify authentication requirements for accessing create and edit forms.
tests/Unit/Cms/Controllers/UsersControllerTest.php (2)
25-62: LGTM!The setUp/tearDown pattern correctly manages Registry state and aligns with the established testing patterns across other controller tests in this PR.
232-284: LGTM!The authentication guard tests correctly validate that
update()anddestroy()throw appropriate exceptions when no authenticated user is present. The expected exception messages match the actual controller implementation.tests/Unit/Cms/Controllers/CategoriesControllerTest.php (3)
26-63: LGTM!The setUp/tearDown pattern correctly manages Registry state and is consistent with other controller tests in this PR.
65-89: LGTM!The authentication guard tests provide comprehensive coverage across all controller actions. The tests correctly validate that operations throw
RuntimeExceptionwith message "Authenticated user not found" when no authenticated user is present.Also applies to: 150-174, 225-249, 251-276, 309-334, 336-361
278-307: LGTM!The edge case test correctly validates that editing a non-existent category throws the appropriate exception.
src/Cms/Controllers/Admin/EventCategories.php (5)
37-62: LGTM!The constructor implements clean dependency injection, allowing optional injection for testing while maintaining backwards compatibility with production usage. This pattern is consistent with other admin controllers in this PR.
67-89: LGTM!The index method correctly implements authentication checks and uses the fluent view rendering pattern. The static analysis warning about unused
$requestparameter is a false positive—the parameter is required by the routing/controller interface.
94-115: LGTM!The create method correctly implements authentication checks and fluent view rendering. The static analysis warning about unused
$requestis a false positive.
174-199: LGTM!The edit method correctly implements authentication checks, validates the category exists, and uses fluent view rendering.
120-169: LGTM!The CSRF validation implementation is correct across all mutating actions (store, update, destroy). The validation occurs before any state changes, logs appropriate warnings on failure, and uses the
user_id()helper for secure logging.Also applies to: 204-252, 257-293
src/Cms/Controllers/Admin/Categories.php (4)
37-64: LGTM!The constructor correctly implements dependency injection for testability while maintaining production compatibility. This pattern is consistent across all admin controllers in this PR.
72-90: LGTM!The index method correctly implements authentication checks, CSRF token generation, and fluent view rendering. The previous review concern about missing CSRF protection has been addressed.
98-115: LGTM!The create and edit methods correctly implement authentication guards, CSRF token generation, and fluent view rendering.
Also applies to: 161-187
123-153: LGTM!The mutating actions (store, update, destroy) correctly implement authentication guards and CSRF validation before any state changes. The implementation is consistent and secure.
Also applies to: 195-233, 241-269
src/Cms/Controllers/Admin/Users.php (4)
39-67: LGTM!The constructor correctly implements dependency injection with lazy initialization. The PasswordHasher is properly instantiated when creating the service instances.
75-94: LGTM!The view rendering has been successfully migrated to the fluent API pattern. CSRF token generation is properly implemented for all view methods, and the rendering chains are clean and consistent.
Also applies to: 102-115, 154-178
189-192: LGTM!Authentication guards are correctly implemented for update and destroy actions. The exception messages match the test expectations and are consistent with other admin controllers.
Also applies to: 236-239
244-244: LGTM!The self-deletion check is improved by using
user_id()for a cleaner comparison.src/Cms/Controllers/Admin/Media.php (2)
72-157: Excellent implementation of media library index.The method demonstrates solid security practices:
- Proper authentication checks
- CSRF token generation and storage
- Thorough cursor validation with security logging
- Generic error messages to users while detailed logging for debugging
- Clean fluent view builder pattern
168-259: BothuploadImage()anduploadFeaturedImage()are already protected by CSRF middleware via theauth-csrffilter applied to their routes inresources/config/routes.yaml(lines 265 and 271). No additional CSRF validation is required.Likely an incorrect or invalid review comment.
src/Cms/Controllers/Admin/Tags.php (4)
27-45: LGTM! Constructor follows established DI pattern.The constructor correctly implements dependency injection for testability while maintaining backward compatibility for production use. This pattern is consistent with other admin controllers (Media, Posts) in the codebase.
47-56: LGTM! CSRF helper addresses previous DRY concern.The
initializeCsrfToken()helper successfully eliminates duplication in GET actions (index, create, edit). The pattern where POST actions create their ownCsrfTokeninstances for validation is appropriate and consistent with the framework's design.
64-80: LGTM! GET actions follow consistent pattern.The index, create, and edit methods correctly implement:
- Authentication guards with clear error messages
- CSRF token initialization for form rendering
- Fluent view builder pattern consistent with other admin controllers in this PR
- Appropriate data loading from the repository
Also applies to: 88-103, 157-181
111-148: LGTM! POST actions implement proper security controls.The store, update, and destroy methods correctly implement:
- Authentication guards before any processing
- CSRF validation before state changes, with appropriate logging and user-friendly error messages
- Consistent error handling with flash messages
- Security logging includes contextual information (user ID, tag ID)
The CSRF validation pattern is consistent across all three methods and follows security best practices.
Also applies to: 190-234, 243-271
tests/Unit/Cms/Controllers/TagsControllerTest.php (3)
23-60: LGTM! Test lifecycle management is correct.The setUp and tearDown methods properly manage Registry state isolation, ensuring tests don't interfere with each other. The SettingManager mock provides necessary configuration for the Content parent class.
62-80: LGTM! Authentication guard coverage is comprehensive.All six controller actions (index, create, store, edit, update, destroy) have dedicated tests verifying that unauthenticated requests throw appropriate exceptions. This ensures the security requirements are consistently enforced.
Also applies to: 134-152, 197-215, 217-236, 262-281, 283-302
238-260: LGTM! Tag not-found scenario is properly tested.This test correctly verifies that the edit action throws an appropriate exception when the requested tag doesn't exist, ensuring proper error handling for invalid tag IDs.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Cms/Controllers/Member/Profile.php (1)
80-90: Critical: Multiple null pointer dereference risks.Lines 80 and 90 call methods on
auth()without checking for null:
- Line 80:
auth()->getEmail()- Line 90:
auth()->getPasswordHash()Since
auth()returns?User, these will cause fatal errors if the user is not authenticated.🔎 Proposed fix to add null safety
+ $user = auth(); + if (!$user) { + $this->redirect('login'); + } + // Security: Only use email from POST if provided by Account Information form // Password change form doesn't include email field, preventing email hijacking attacks - $email = $request->post( 'email', auth()->getEmail() ); + $email = $request->post( 'email', $user->getEmail() ); $timezone = $request->post( 'timezone', '' ); $currentPassword = $request->post( 'current_password', '' ); $newPassword = $request->post( 'new_password', '' ); $confirmPassword = $request->post( 'confirm_password', '' ); // Validate password change if requested if( !empty( $newPassword ) ) { // Verify current password - if( empty( $currentPassword ) || !$this->_hasher->verify( $currentPassword, auth()->getPasswordHash() ) ) + if( empty( $currentPassword ) || !$this->_hasher->verify( $currentPassword, $user->getPasswordHash() ) ) { $this->redirect( 'member_profile', [], ['error', 'Current password is incorrect'] ); }src/Cms/Controllers/Admin/Posts.php (2)
184-187: Exception message exposed in redirect flash message.The catch block includes
$e->getMessage()in the user-facing flash message, which could leak sensitive internal details (e.g., database errors, connection strings).🔎 Recommended fix
catch( \Exception $e ) { - $this->redirect( 'admin_posts_create', [], ['error', 'Failed to create post: ' . $e->getMessage()] ); + // Log detailed error for debugging + // Log::error('Post creation failed', ['exception' => $e]); + $this->redirect( 'admin_posts_create', [], ['error', 'Failed to create post. Please try again.'] ); }
276-279: Same exception message exposure in update and destroy.Both methods expose
$e->getMessage()in flash messages, similar to the store method.Consider logging the full exception and returning a generic message in both cases for consistency with the Media controller's approach.
Also applies to: 309-312
♻️ Duplicate comments (1)
src/Cms/Controllers/Admin/Categories.php (1)
72-83: CSRF token handling now properly implemented.The
index()method now initializes the CSRF token viainitializeCsrfToken()and passes it to the view viawithCsrfToken(). This addresses the previous review concern about missing CSRF protection for forms in the category listing.
🧹 Nitpick comments (12)
tests/Unit/Cms/Auth/AuthenticationFilterTest.php (2)
48-51: Consider removing trivial constructor test.Constructor tests that only verify instance creation provide minimal value. Consider focusing test effort on behavioral tests instead.
81-111: Consider consolidating redundant tests.
testSetsUserIdCorrectlyandtestSetsUserRoleCorrectlyoverlap significantly withtestSetsUserInRegistryForAuthenticatedUser(lines 59-79), which already verifies both UserId and UserRole. Consolidating these would reduce maintenance burden without losing coverage.src/Cms/Controllers/Content.php (1)
309-313: LGTM - Clean CSRF token initialization helper.The centralized approach to CSRF token initialization aligns well with the broader refactoring across controllers. Consider importing
CsrfTokenat the top of the file rather than using the fully qualified class name inline for consistency with other files in this PR.🔎 Optional: Use import instead of FQCN
Add to imports:
use Neuron\Cms\Services\Auth\CsrfToken;Then simplify the method:
protected function initializeCsrfToken(): void { - $csrfToken = new \Neuron\Cms\Services\Auth\CsrfToken( $this->getSessionManager() ); + $csrfToken = new CsrfToken( $this->getSessionManager() ); Registry::getInstance()->set( 'Auth.CsrfToken', $csrfToken->getToken() ); }src/Cms/Controllers/Member/Dashboard.php (1)
6-6: Unused import.
CsrfTokenis imported but no longer directly used since CSRF handling is now delegated toinitializeCsrfToken().🔎 Remove unused import
-use Neuron\Cms\Services\Auth\CsrfToken;tests/Unit/Cms/Controllers/TagsControllerTest.php (1)
99-104: Consider extracting repeated reflection setup to a helper method.The pattern for injecting
SessionManagervia reflection is duplicated across test methods. This could be extracted to a private helper for DRYer tests.🔎 Example helper extraction
private function injectSessionManager( object $controller, SessionManager $sessionManager ): void { $reflection = new \ReflectionClass( get_parent_class( Tags::class ) ); $sessionProperty = $reflection->getProperty( '_sessionManager' ); $sessionProperty->setAccessible( true ); $sessionProperty->setValue( $controller, $sessionManager ); }Also applies to: 142-147
tests/Unit/Cms/Controllers/MediaUploadTest.php (1)
105-109: Consider extracting repeated reflection injection to a helper.The pattern for injecting
_validatorand_uploadervia reflection is repeated across multiple test methods. A private helper would reduce duplication.🔎 Example helper extraction
private function injectMediaMocks( Media $media, ?MediaValidator $validator, ?CloudinaryUploader $uploader ): void { $reflection = new \ReflectionClass( $media ); if( $validator !== null ) { $validatorProperty = $reflection->getProperty( '_validator' ); $validatorProperty->setAccessible( true ); $validatorProperty->setValue( $media, $validator ); } if( $uploader !== null ) { $uploaderProperty = $reflection->getProperty( '_uploader' ); $uploaderProperty->setAccessible( true ); $uploaderProperty->setValue( $media, $uploader ); } }Also applies to: 167-171, 217-226, 277-286, 326-335, 374-383
tests/Unit/Cms/Controllers/MediaIndexTest.php (1)
59-117: Well-structured success path test.The test properly mocks the uploader, session manager, and view rendering chain. The use of reflection to inject mocks into private properties is appropriate for testing controllers with private dependencies.
Consider adding a test case for invalid cursor format validation, as the controller is described to validate cursor format (alphanumeric with
_,-,=) and log warnings for invalid cursors.tests/Unit/Cms/Controllers/UsersControllerTest.php (1)
175-230: Edit test covers user lookup and form rendering.The test properly mocks
findByIdto return a specific user and verifies the edit form is rendered. Consider adding a test for the "user not found" redirect scenario to improve coverage.src/Cms/Controllers/Admin/Events.php (1)
180-183: RuntimeException for authorization is inconsistent with other permission checks.The edit method throws
RuntimeExceptionfor unauthorized access, butdestroy()at line 275-278 uses a redirect with an error message. Consider using consistent error handling - either throw exceptions or redirect with messages throughout.🔎 Proposed fix for consistency
// Check permissions if( !is_admin() && !is_editor() && $event->getCreatedBy() !== user_id() ) { - throw new \RuntimeException( 'Unauthorized to edit this event' ); + $this->redirect( 'admin_events', [], ['error', 'Unauthorized to edit this event'] ); }src/Cms/Controllers/Admin/Pages.php (1)
77-80: Inconsistent CSRF initialization pattern.The
index()method manually creates aCsrfTokenand sets it in the Registry, whilecreate()andedit()use$this->initializeCsrfToken(). For consistency and maintainability, consider using the centralized method here as well.🔎 Proposed fix
public function index( Request $request ): string { - // Generate CSRF token - $sessionManager = $this->getSessionManager(); - $csrfToken = new CsrfToken( $sessionManager ); - Registry::getInstance()->set( 'Auth.CsrfToken', $csrfToken->getToken() ); + $this->initializeCsrfToken(); + $sessionManager = $this->getSessionManager(); // Get all pages or filter by author if not adminsrc/Cms/Controllers/Admin/Media.php (1)
210-217: Inconsistent response structure between uploadImage and uploadFeaturedImage.
uploadImage()returnssuccess: 1(integer) with afilekey containing structured data, whileuploadFeaturedImage()returnssuccess: true(boolean) with adatakey containing the raw result. This inconsistency could confuse API consumers.The difference may be intentional (Editor.js format vs custom format), but consider documenting this clearly or unifying the response structures if both endpoints could be used interchangeably.
Also applies to: 295-300
src/Cms/Controllers/Admin/Posts.php (1)
299-302: Inconsistent authorization error handling in destroy vs edit/update.In
destroy(), an unauthorized user is redirected with an error flash message, while inedit()andupdate()an exception is thrown. This inconsistency could lead to different user experiences.🔎 Consider using consistent behavior
Either redirect with flash in all cases:
// In edit() and update() - if( !is_admin() && !is_editor() && $post->getAuthorId() !== user_id() ) - { - throw new \RuntimeException( 'Unauthorized to edit this post' ); - } + if( !is_admin() && !is_editor() && $post->getAuthorId() !== user_id() ) + { + $this->redirect( 'admin_posts', [], ['error', 'Unauthorized to edit this post'] ); + }Or throw exceptions consistently (with a global exception handler for proper HTTP 403 responses).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/Cms/Controllers/Admin/Categories.phpsrc/Cms/Controllers/Admin/Dashboard.phpsrc/Cms/Controllers/Admin/EventCategories.phpsrc/Cms/Controllers/Admin/Events.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Controllers/Admin/Pages.phpsrc/Cms/Controllers/Admin/Posts.phpsrc/Cms/Controllers/Admin/Profile.phpsrc/Cms/Controllers/Admin/Tags.phpsrc/Cms/Controllers/Admin/Users.phpsrc/Cms/Controllers/Content.phpsrc/Cms/Controllers/Member/Dashboard.phpsrc/Cms/Controllers/Member/Profile.phptests/Unit/Cms/Auth/AuthenticationFilterTest.phptests/Unit/Cms/Auth/CsrfFilterTest.phptests/Unit/Cms/Controllers/CategoriesControllerTest.phptests/Unit/Cms/Controllers/EventCategoriesControllerTest.phptests/Unit/Cms/Controllers/EventsControllerTest.phptests/Unit/Cms/Controllers/MediaIndexTest.phptests/Unit/Cms/Controllers/MediaUploadTest.phptests/Unit/Cms/Controllers/PagesControllerTest.phptests/Unit/Cms/Controllers/PostsControllerTest.phptests/Unit/Cms/Controllers/TagsControllerTest.phptests/Unit/Cms/Controllers/UsersControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Cms/Controllers/Admin/Tags.php
- tests/Unit/Cms/Controllers/CategoriesControllerTest.php
- tests/Unit/Cms/Controllers/EventCategoriesControllerTest.php
- tests/Unit/Cms/Controllers/PostsControllerTest.php
🧰 Additional context used
🧬 Code graph analysis (12)
src/Cms/Controllers/Member/Dashboard.php (2)
src/Cms/Controllers/Content.php (1)
initializeCsrfToken(309-313)src/Cms/Services/Widget/Widget.php (1)
view(50-61)
src/Cms/Controllers/Admin/Dashboard.php (1)
src/Cms/Services/Widget/Widget.php (1)
view(50-61)
src/Cms/Controllers/Content.php (2)
src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)src/Cms/Auth/SessionManager.php (1)
set(99-103)
tests/Unit/Cms/Controllers/MediaIndexTest.php (3)
src/Cms/Controllers/Admin/Media.php (2)
Media(23-321)index(72-150)src/Cms/Services/Media/CloudinaryUploader.php (1)
CloudinaryUploader(17-371)src/Cms/Services/Media/MediaValidator.php (1)
MediaValidator(14-192)
tests/Unit/Cms/Controllers/TagsControllerTest.php (2)
src/Cms/Controllers/Admin/Tags.php (4)
Tags(18-227)index(53-64)create(72-82)edit(121-140)src/Cms/Repositories/DatabaseTagRepository.php (1)
DatabaseTagRepository(20-185)
src/Cms/Controllers/Admin/Media.php (4)
src/Cms/Controllers/Content.php (2)
Content(61-314)getSessionManager(222-230)src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(17-371)__construct(28-32)upload(68-89)src/Cms/Auth/helpers.php (1)
user_id(41-44)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
tests/Unit/Cms/Auth/CsrfFilterTest.php (2)
src/Cms/Auth/Filters/CsrfFilter.php (1)
CsrfFilter(18-94)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)
src/Cms/Controllers/Admin/Users.php (2)
src/Cms/Repositories/DatabaseUserRepository.php (2)
DatabaseUserRepository(19-243)all(141-144)src/Cms/Auth/helpers.php (1)
user_id(41-44)
tests/Unit/Cms/Controllers/EventsControllerTest.php (5)
src/Cms/Controllers/Admin/Events.php (3)
Events(24-290)index(74-100)create(105-116)src/Cms/Repositories/DatabaseEventRepository.php (1)
DatabaseEventRepository(21-380)src/Cms/Auth/SessionManager.php (2)
SessionManager(13-188)set(99-103)src/Cms/Controllers/Admin/EventCategories.php (2)
index(67-84)create(89-105)src/Cms/Controllers/Admin/Posts.php (2)
index(102-128)create(136-147)
tests/Unit/Cms/Controllers/PagesControllerTest.php (2)
src/Cms/Controllers/Admin/Pages.php (3)
Pages(23-322)index(75-103)create(111-121)src/Cms/Auth/SessionManager.php (2)
SessionManager(13-188)set(99-103)
src/Cms/Controllers/Member/Profile.php (5)
src/Cms/Controllers/Content.php (2)
initializeCsrfToken(309-313)getSessionManager(222-230)src/Cms/View/helpers.php (1)
group_timezones_for_select(229-256)src/Cms/Auth/helpers.php (1)
auth(19-22)src/Cms/Models/User.php (4)
getTimezone(396-399)getPasswordHash(112-115)getUsername(78-81)getRole(129-132)src/Cms/Auth/SessionManager.php (1)
getFlash(144-150)
tests/Unit/Cms/Controllers/UsersControllerTest.php (1)
src/Cms/Controllers/Admin/Users.php (4)
Users(24-243)index(75-92)create(100-111)edit(150-172)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/EventCategories.php
67-67: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
89-89: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/Media.php
161-161: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
248-248: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/Events.php
74-74: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ 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: build-test (sqlite)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
🔇 Additional comments (43)
tests/Unit/Cms/Auth/AuthenticationFilterTest.php (5)
12-18: LGTM: Appropriate test isolation annotations.The
@runTestsInSeparateProcessesand@preserveGlobalState disabledannotations are necessary for testing code that interacts with the Registry singleton, ensuring proper test isolation.
24-40: LGTM: Clean test setup.The setUp method properly initializes mocks and resets the Registry singleton for test isolation.
59-79: LGTM: Comprehensive authenticated user test.This test properly verifies that the filter populates all expected Registry entries when a user is authenticated.
136-163: LGTM: Good coverage of multiple roles.The test properly verifies that the filter handles all user roles correctly. Creating new mocks for each iteration ensures proper test isolation.
Note: Line 158 creates the filter without specifying a login URL (unlike setUp on line 32). This is fine if the constructor parameter is optional with a default value.
165-172: Test coverage gap: Custom login URL usage not verified.While this test confirms that the custom login URL can be set via constructor and
setLoginUrl(), it doesn't verify that the custom URL is actually used when redirecting unauthenticated users. Consider adding integration tests to verify the redirect behavior with custom URLs.src/Cms/Controllers/Member/Profile.php (1)
58-66: Fluent view builder methods are implemented and working correctly.The methods
withCurrentUser()andwithCsrfToken()are confirmed to exist and return a fluent interface (self) for method chaining. This is evidenced by their use across 30+ controllers in the codebase and verified by unit tests that mock these methods withwillReturnSelf(). The ViewContext class from the externalneuron-php/mvcframework implements all required fluent methods:title(),description(),withCurrentUser(),withCsrfToken(),with(), andrender().However, verify that line 56 properly handles the case where
auth()returns null (unauthenticated user):auth()->getTimezone()will throw an error if no user is authenticated.src/Cms/Controllers/Member/Dashboard.php (1)
39-46: LGTM - Clean fluent rendering pattern.The refactored approach using
initializeCsrfToken()followed by the fluent view builder is consistent with the broader controller refactoring in this PR.tests/Unit/Cms/Controllers/TagsControllerTest.php (2)
23-60: LGTM - Well-structured test setup and teardown.The Registry state management pattern ensures test isolation properly. Settings mock provides consistent test data.
157-179: LGTM - Exception handling test correctly validates not-found behavior.The test properly verifies that
edit()throwsRuntimeExceptionwith the expected message when a tag doesn't exist.tests/Unit/Cms/Auth/CsrfFilterTest.php (1)
42-83: LGTM - Valid tests for exempt HTTP methods.These tests correctly verify that CSRF validation is skipped for GET, HEAD, and OPTIONS requests by calling
pre()and asserting thatvalidate()is never invoked.src/Cms/Controllers/Admin/Profile.php (2)
66-84: LGTM - Clean fluent view rendering for profile edit.The refactored
edit()method properly initializes CSRF and uses the fluent view builder consistently with other admin controllers.
103-117: LGTM - Password change validation is correctly implemented.The flow properly verifies the current password before allowing changes and validates that the new password matches confirmation.
tests/Unit/Cms/Controllers/MediaUploadTest.php (4)
23-57: LGTM - Proper test setup and cleanup.Registry state is preserved/restored correctly, and
$_FILEScleanup intearDown()prevents test pollution. Cloudinary settings mock provides consistent test configuration.
59-119: LGTM - Good coverage for upload error scenarios.Tests properly verify JSON error responses for missing files and validation failures, including verifying specific error messages.
183-239: LGTM - Comprehensive successful upload testing.Both
uploadImage(Editor.js format) anduploadFeaturedImageresponse structures are properly validated, including checking nestedfile/datakeys and dimension data.Also applies to: 241-297
299-393: LGTM - Exception handling tests verify graceful degradation.Tests correctly verify that upload exceptions result in user-friendly error messages rather than exposing internal error details.
tests/Unit/Cms/Controllers/MediaIndexTest.php (2)
24-46: Good test isolation with Registry state preservation.The setup properly captures and restores global Registry state, preventing test pollution. The in-memory Settings configuration covers all necessary Cloudinary settings for the tests.
171-213: Exception handling test validates graceful degradation.Good coverage for the error path - verifying that the controller returns HTML (not throws) when
listResourcesfails ensures the user sees an error message rather than a crash.tests/Unit/Cms/Controllers/UsersControllerTest.php (2)
21-62: Consistent test scaffolding pattern.The setup/teardown pattern for Registry state preservation matches other controller tests in this PR, promoting consistency across the test suite.
64-123: Index test validates repository interaction.Good use of
expects($this->once())to verify the repository'sall()method is called exactly once. The view builder chain mock is appropriate for isolating controller logic from view rendering.tests/Unit/Cms/Controllers/EventsControllerTest.php (3)
66-129: Admin index path correctly verified.The test properly sets up an admin user with
isAdmin()->willReturn(true)and verifies that$eventRepository->all()is called rather thangetByCreator(). SettingAuth.UserIdensures the global helper functions work correctly.
131-193: Non-admin filtering test validates authorization logic.Good verification that non-admin users only see their own events via
getByCreator(2). The expectation correctly matches the user ID set in the Registry.
195-249: Create form test validates category loading.The test correctly expects
categoryRepository->all()to be called once, matching the controller's behavior of loading categories for the event creation form.src/Cms/Controllers/Admin/Events.php (3)
41-69: Clean dependency injection pattern.The constructor properly supports both testing (with injected mocks) and production (lazy initialization from Settings). This pattern is consistent across all admin controllers in this PR.
74-100: Unused$requestparameter is framework-required.The static analysis hint about unused
$requestis a false positive - the parameter is required by the routing/controller interface signature even when not directly used in the method body.
174-177: No changes needed. Theredirect()method has aneverreturn type and callsexit, guaranteeing execution termination. No additional return statement is required.Likely an incorrect or invalid review comment.
src/Cms/Controllers/Admin/Categories.php (1)
37-64: Dependency injection pattern enables testability.The constructor properly implements the lazy initialization pattern, creating dependencies from Settings only when not injected. This is consistent with other admin controllers.
src/Cms/Controllers/Admin/Pages.php (2)
38-67: Well-implemented dependency injection.The constructor follows the established pattern for optional dependency injection with lazy initialization fallback. The comment clearly explains the dual-purpose design for testing vs production.
186-189: No action required. Theredirect()method terminates execution.The
redirect()method defined in the Content controller has a return type of: neverand explicitly callsexit;(line 250 in Content.php). This ensures execution terminates immediately and no code after the redirect call at line 188 will execute. The concern about null pointer issues at line 192 is unfounded.Likely an incorrect or invalid review comment.
src/Cms/Controllers/Admin/Users.php (3)
39-67: Constructor properly initializes password hashing dependency.Unlike other controllers, Users requires a
PasswordHasherfor the Creator and Updater services. The lazy initialization correctly creates this when dependencies aren't injected.
225-231: Self-deletion prevention is correctly implemented.Using
user_id() === $idproperly prevents users from deleting their own accounts. The strict equality comparison is appropriate here.
75-92: Consistent fluent view rendering.The index method follows the established pattern with
initializeCsrfToken(),withCurrentUser(),withCsrfToken(), and flash message handling.tests/Unit/Cms/Controllers/PagesControllerTest.php (3)
65-127: Admin index test properly validates repository interaction.The test correctly sets up an admin user with both
isAdmin()->willReturn(true)andAuth.UserIdin the Registry, then verifiesrepository->all()is called. The view builder chain mock isolates the controller logic effectively.
129-190: Non-admin filtering test validates author-based access control.Good verification that regular users only see their own pages via
getByAuthor(2), matching the user ID. The expectation accurately reflects the controller's authorization logic.
192-239: Create form test validates authenticated user flow.The test properly sets up an authenticated user and verifies the create form renders correctly.
Likely an incorrect or invalid review comment.
src/Cms/Controllers/Admin/Media.php (5)
72-127: Well-implemented index method with proper cursor validation and error handling.The cursor validation using regex
/^[a-zA-Z0-9_\-=]+$/appropriately restricts to Cloudinary's base64-like format. The logging of invalid cursors with truncation and the fail-closed approach (setting$validatedCursor = nullon invalid input) are good security practices. The fluent view builder pattern is clean and consistent.
129-149: Error handling correctly logs details and returns generic message.The catch block properly logs the exception with context while returning a user-friendly generic error message. This addresses the previous review concern about exposing sensitive exception details.
161-161: Unused$requestparameter is acceptable for controller method signatures.The static analysis flags
$requestas unused inuploadImage()anduploadFeaturedImage(). However, this is intentional—the methods directly access$_FILESfor file uploads, and theRequestparameter maintains consistent controller method signatures expected by the routing framework. Using$_FILESdirectly is appropriate for PHP file uploads.Also applies to: 248-248
220-236: Error handling improvements properly implemented.Both upload methods now log detailed exception information while returning generic, user-safe error messages. This addresses the previous review concerns about exposing internal details.
Also applies to: 303-319
166-175: No further action needed. The Neuron MVC framework does not provide file access methods on the Request object, making direct$_FILESaccess the appropriate pattern for this codebase.src/Cms/Controllers/Admin/Posts.php (3)
102-128: Clean index method with proper role-based filtering.The logic correctly shows all posts for admins/editors and filters by author for regular users using
user_id(). The fluent view builder pattern is consistently applied.
136-147: Create method looks good.CSRF token initialization and fluent view rendering are properly implemented.
207-210: Permission check correctly implemented.The authorization logic properly checks
is_admin(),is_editor(), or ownership viauser_id(). Good use of short-circuit evaluation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/Unit/Cms/Auth/CsrfFilterTest.php (1)
112-145: Tests still verify a hardcoded array, not the filter's behavior.These tests assert that
'PUT'and'DELETE'are not in a locally-defined$exemptMethodsarray, which is independent of the actualCsrfFilter::$_exemptMethods. The assertions would pass even if the filter exempted all methods.Consider using reflection to verify against the filter's actual exempt methods:
🔎 Proposed fix using reflection
public function testPutRequestsRequireCsrfValidation(): void { $_SERVER['REQUEST_METHOD'] = 'PUT'; - // Verify PUT is not in the exempt methods list - // Exempt methods are: GET, HEAD, OPTIONS - $exemptMethods = ['GET', 'HEAD', 'OPTIONS']; + // Use reflection to get actual exempt methods from filter + $reflection = new \ReflectionClass( $this->_filter ); + $property = $reflection->getProperty( '_exemptMethods' ); + $property->setAccessible( true ); + $exemptMethods = $property->getValue( $this->_filter ); + $this->assertNotContains( 'PUT', $exemptMethods, 'PUT requests should require CSRF validation' ); }src/Cms/Controllers/Admin/EventCategories.php (1)
46-60: The partial injection issue flagged in the previous review remains unresolved.If
$repositoryis not null but$creator,$updater, or$deleterare null, the condition at line 47 evaluates to false, the instantiation block is skipped, and nulls are assigned to the private fields at lines 58-60. This will cause errors when these services are invoked.Note: The same pattern appears in
Categories.php, suggesting this may be a codebase-wide issue.
🧹 Nitpick comments (1)
tests/Unit/Cms/Auth/CsrfFilterTest.php (1)
19-32: Consider addingtearDown()for proper global state cleanup.The tests manipulate
$_SERVERand$_POSTsuperglobals. While some tests have inline cleanup, adding atearDown()method ensures consistent cleanup across all tests, preventing test pollution.🔎 Proposed addition
protected function setUp(): void { parent::setUp(); // Create mock CSRF token service $this->_csrfToken = $this->createMock( CsrfToken::class ); // Create filter $this->_filter = new CsrfFilter( $this->_csrfToken ); // Create mock route $this->_route = $this->createMock( RouteMap::class ); $this->_route->method( 'getPath' )->willReturn( '/admin/users' ); } + + protected function tearDown(): void + { + unset( $_SERVER['REQUEST_METHOD'] ); + unset( $_SERVER['HTTP_X_CSRF_TOKEN'] ); + unset( $_POST['csrf_token'] ); + parent::tearDown(); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Cms/Controllers/Admin/Dashboard.phpsrc/Cms/Controllers/Admin/EventCategories.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Controllers/Admin/Posts.phpsrc/Cms/Controllers/Admin/Profile.phpsrc/Cms/Controllers/Member/Profile.phptests/Unit/Cms/Auth/AuthenticationFilterTest.phptests/Unit/Cms/Auth/CsrfFilterTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Cms/Controllers/Admin/Dashboard.php
- src/Cms/Controllers/Member/Profile.php
- tests/Unit/Cms/Auth/AuthenticationFilterTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/Cms/Auth/CsrfFilterTest.php (2)
src/Cms/Auth/Filters/CsrfFilter.php (1)
CsrfFilter(18-94)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)
src/Cms/Controllers/Admin/Posts.php (4)
src/Cms/Controllers/Admin/Tags.php (1)
__construct(27-45)src/Cms/Repositories/DatabasePostRepository.php (1)
DatabasePostRepository(22-533)src/Cms/Auth/SessionManager.php (1)
get(108-112)src/Cms/Auth/helpers.php (2)
is_admin(74-78)is_editor(86-90)
src/Cms/Controllers/Admin/EventCategories.php (3)
src/Cms/Controllers/Admin/Categories.php (2)
__construct(37-64)create(91-101)src/Cms/Repositories/DatabaseEventCategoryRepository.php (3)
DatabaseEventCategoryRepository(19-181)all(40-46)create(103-126)src/Cms/Controllers/Content.php (2)
initializeCsrfToken(309-313)getSessionManager(222-230)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/Media.php
171-171: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
258-258: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/EventCategories.php
66-66: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
87-87: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ 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: build-test (sqlite)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
🔇 Additional comments (14)
src/Cms/Controllers/Admin/Posts.php (3)
44-116: Constructor now properly handles partial dependency injection.The refactored constructor correctly addresses the previously flagged vulnerability. Each dependency is now individually checked and created if null, preventing null assignments when only some dependencies are injected.
One minor observation:
TagResolver(lines 81-84) is always instantiated even when$postCreatorand$postUpdaterare both provided via injection. This creates an unused object in that scenario but has no functional impact.
124-150: LGTM!The
index()method properly initializes CSRF tokens, uses the helper functions for authorization checks, and follows the fluent view builder pattern consistently.
158-169: LGTM!The CRUD methods consistently:
- Use
is_admin()/is_editor()/user_id()helpers for authorization- Follow the fluent view builder pattern for rendering
- Delegate to service classes for business logic
- Handle errors gracefully with flash message redirects
Also applies to: 177-210, 218-246, 254-302, 309-335
src/Cms/Controllers/Admin/Profile.php (3)
34-65: Constructor now properly handles partial dependency injection.The refactored constructor correctly addresses the previously flagged vulnerability. Each dependency (
$repository,$hasher,$userUpdater) is individually checked and created when null, ensuring all properties are properly initialized regardless of which dependencies are injected.
73-99: LGTM!The
edit()method properly validates the authenticated user, initializes CSRF tokens, and uses the fluent view builder pattern consistently with other admin controllers.
107-156: LGTM!The
update()method has proper security measures:
- Validates current password before allowing password changes
- Confirms new password matches confirmation
- Handles email securely by defaulting to current value if not provided
- Uses try-catch with appropriate error messaging
tests/Unit/Cms/Auth/CsrfFilterTest.php (2)
39-80: LGTM!These tests properly exercise the filter by calling
pre()and verifying via mock expectations thatvalidate()is never called for exempt HTTP methods. This is correct unit testing of the filter's exemption behavior.
235-323: LGTM!The reflection-based tests properly exercise
getTokenFromRequest()and include valuable documentation about thefilter_input()limitation in unit tests. The tests for header fallback and null return cases are correctly implemented.src/Cms/Controllers/Admin/Media.php (4)
36-70: Constructor DI pattern implemented correctly.The constructor properly handles dependency injection with appropriate fallbacks. The logic ensures both
$uploaderand$validatorare instantiated when not provided, addressing the previous concern about potential null validator scenarios.
82-123: Index method implementation looks solid.The cursor validation logic (lines 94-113) properly implements defense-in-depth with regex validation and security logging. The structured error handling and generic user-facing messages maintain security best practices.
171-171: Static analysis warnings are false positives.The PHPMD warnings about unused
$requestparameters can be safely ignored. These are controller action methods that follow a standard signature pattern for routing and dependency injection. The parameter is part of the expected interface even if not currently used in the method bodies.Also applies to: 258-258
230-246: Guard against undefined array index in error logging.Line 234 accesses
$_FILES['image']['name']in the catch block, which may not exist if the exception occurred before the file validation or if the$_FILESarray is malformed. This could cause an undefined index warning during error handling.🔎 Proposed fix to safely access filename
catch( \Exception $e ) { Log::error( 'Image upload failed', [ 'user_id' => user_id(), - 'filename' => $_FILES['image']['name'] ?? 'unknown', + 'filename' => $_FILES['image']['name'] ?? $_FILES['image'] ?? 'unknown', 'exception' => $e, 'message' => $e->getMessage() ] );Or more defensively:
catch( \Exception $e ) { Log::error( 'Image upload failed', [ 'user_id' => user_id(), - 'filename' => $_FILES['image']['name'] ?? 'unknown', + 'filename' => isset($_FILES['image']['name']) ? $_FILES['image']['name'] : 'unknown', 'exception' => $e, 'message' => $e->getMessage() ] );Likely an incorrect or invalid review comment.
src/Cms/Controllers/Admin/EventCategories.php (2)
68-68: CSRF initialization is now consistent.All three methods (
index(),create(), andedit()) now useinitializeCsrfToken(), resolving the inconsistency flagged in the previous review.Also applies to: 89-89, 154-154
107-139: CSRF protection is properly configured via route-level filters.The
store(),update(), anddestroy()methods are protected by theauth-csrffilter at the route level (routes.yaml lines 386, 398, 404). The removal of explicit CSRF validation from controller methods is appropriate—the framework's CsrfFilter and AuthCsrfFilter handle validation for all POST, PUT, and DELETE requests to these routes before the controller methods execute. No action required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Cms/Controllers/Admin/Media.php (2)
261-336: Same unused Request parameter as uploadImage.Like
uploadImage(), this method declares the$requestparameter but accesses$_FILESdirectly (lines 266, 277, 319). The same optional refactor applies—either integrate with the Request object or clarify that the parameter serves only as a placeholder for framework routing.The error handling is exemplary: safe filename access (line 318-319), detailed contextual logging (lines 282-286, 300-305, 321-326), and generic user-facing messages (lines 272, 292, 332).
171-250: Consider using the Request parameter or removing it if unused.The
$requestparameter is declared but never used—file data comes directly from the$_FILESsuperglobal (lines 176, 187, 233). For better consistency with the framework interface, consider either:
- Using a method like
$request->getFiles()if the Request class provides file access, or- Removing the parameter if file access is not yet implemented in the framework.
The current implementation is functionally correct with solid error handling (safe filename access, comprehensive logging, generic user messages). This is an optional refactor for consistency.
resources/public/index.php (1)
29-42: LGTM with optional suggestion: Exception handling is secure and functional.The exception handling is correct with proper XSS protection via
htmlspecialchars()on line 40. The basic HTML response for CSRF errors is functional but could be enhanced.💡 Optional: Consider templated error pages
The inline HTML for CSRF errors (lines 39-40) is functional but basic. Consider using a template or error view for consistency with other error pages in the application.
This is optional and can be deferred if the current approach meets requirements.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
resources/public/index.phpsrc/Cms/Auth/Filters/AuthenticationFilter.phpsrc/Cms/Auth/Filters/CsrfFilter.phpsrc/Cms/Auth/Filters/MemberAuthenticationFilter.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Exceptions/CsrfValidationException.phpsrc/Cms/Exceptions/EmailVerificationRequiredException.phpsrc/Cms/Exceptions/UnauthenticatedException.phptests/Unit/Cms/Auth/AuthenticationFilterTest.phptests/Unit/Cms/Auth/CsrfFilterTest.phptests/Unit/Cms/Auth/MemberAuthenticationFilterTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Unit/Cms/Auth/AuthenticationFilterTest.php
- tests/Unit/Cms/Auth/CsrfFilterTest.php
🧰 Additional context used
🧬 Code graph analysis (6)
tests/Unit/Cms/Auth/MemberAuthenticationFilterTest.php (2)
src/Cms/Exceptions/UnauthenticatedException.php (1)
UnauthenticatedException(15-47)src/Cms/Exceptions/EmailVerificationRequiredException.php (1)
EmailVerificationRequiredException(15-36)
src/Cms/Auth/Filters/CsrfFilter.php (2)
src/Cms/Exceptions/CsrfValidationException.php (1)
CsrfValidationException(15-36)src/Cms/Services/Auth/CsrfToken.php (1)
validate(55-66)
src/Cms/Auth/Filters/AuthenticationFilter.php (1)
src/Cms/Exceptions/UnauthenticatedException.php (1)
UnauthenticatedException(15-47)
src/Cms/Auth/Filters/MemberAuthenticationFilter.php (4)
src/Cms/Exceptions/UnauthenticatedException.php (1)
UnauthenticatedException(15-47)src/Cms/Exceptions/EmailVerificationRequiredException.php (1)
EmailVerificationRequiredException(15-36)src/Cms/Services/Auth/Authentication.php (1)
user(229-252)src/Cms/Models/User.php (1)
isEmailVerified(203-206)
src/Cms/Exceptions/EmailVerificationRequiredException.php (5)
src/Cms/Auth/Filters/AuthenticationFilter.php (1)
__construct(25-34)src/Cms/Auth/Filters/CsrfFilter.php (1)
__construct(24-32)src/Cms/Auth/Filters/MemberAuthenticationFilter.php (1)
__construct(29-43)src/Cms/Exceptions/CsrfValidationException.php (1)
__construct(23-27)src/Cms/Exceptions/UnauthenticatedException.php (1)
__construct(25-30)
src/Cms/Exceptions/CsrfValidationException.php (5)
src/Cms/Auth/Filters/AuthenticationFilter.php (1)
__construct(25-34)src/Cms/Auth/Filters/CsrfFilter.php (1)
__construct(24-32)src/Cms/Auth/Filters/MemberAuthenticationFilter.php (1)
__construct(29-43)src/Cms/Exceptions/EmailVerificationRequiredException.php (1)
__construct(23-27)src/Cms/Exceptions/UnauthenticatedException.php (1)
__construct(25-30)
🪛 PHPMD (2.15.0)
src/Cms/Controllers/Admin/Media.php
171-171: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
261-261: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ 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: build-test (postgres)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (16)
src/Cms/Controllers/Admin/Media.php (2)
36-70: Constructor DI pattern looks solid.The lazy dependency initialization correctly handles all combinations of injected/null parameters. Settings are retrieved once when needed, and each dependency is independently validated and created if not provided.
82-160: Excellent security and error handling in index method.The cursor validation (lines 94-113) properly implements a fail-closed approach with security logging for invalid attempts. Error handling provides generic user messages while logging detailed context. Previous review concerns about cursor validation and error message exposure have been thoroughly addressed.
src/Cms/Auth/Filters/CsrfFilter.php (2)
8-8: LGTM: Clean exception-based refactoring.The import and docblock updates correctly document the new exception-driven CSRF validation flow.
Also applies to: 36-37
55-58: LGTM: Exception handling improves control flow.The refactoring from direct 403 responses to exception-based flow is cleaner and allows centralized error handling at the application level. The technical and user-friendly message separation is well-designed.
Also applies to: 65-68
src/Cms/Exceptions/CsrfValidationException.php (1)
15-36: LGTM: Well-designed exception class.The exception class follows a clean pattern with appropriate separation between technical (for logging) and user-friendly messages. The design is consistent with other exception classes in the codebase.
src/Cms/Exceptions/UnauthenticatedException.php (1)
15-47: LGTM: Well-structured authentication exception.The dual-URL design (redirect URL and intended URL) elegantly supports post-login redirection to the originally requested resource. The 401 status code is semantically appropriate for authentication failures.
resources/public/index.php (2)
9-11: LGTM: Necessary exception imports.All required exception classes are properly imported for the exception handling below.
19-28: LGTM: Clean exception-driven dispatch.The try-catch pattern centralizes exception handling at the application entry point. The redirect handling for unauthenticated access is correct and uses the exception's accessor appropriately.
src/Cms/Auth/Filters/AuthenticationFilter.php (2)
8-8: LGTM: Proper documentation of exception behavior.The import and docblock correctly reflect the exception-based authentication flow.
Also applies to: 38-39
49-58: LGTM: Clean exception-based authentication check.The refactoring maintains the original redirect logic (with query parameter for post-login redirect) while moving to a cleaner exception-based flow. The intended URL capture and redirect URL construction are correct.
src/Cms/Auth/Filters/MemberAuthenticationFilter.php (3)
8-9: LGTM: Complete exception documentation.The imports and docblock properly document both authentication failure scenarios that can occur in this filter.
Also applies to: 47-49
66-70: LGTM: Consistent authentication exception handling.The authentication failure handling is consistent with
AuthenticationFilter.phpand correctly usesUnauthenticatedExceptionfor unauthenticated access.
76-79: LGTM: Proper email verification enforcement.The exception handling for unverified email is correct and provides the necessary context for redirection to the verification flow.
tests/Unit/Cms/Auth/MemberAuthenticationFilterTest.php (2)
90-106: LGTM: Test correctly validates exception-based flow.The test has been properly updated to expect
UnauthenticatedExceptionwith code 401 for unauthenticated users, matching the new exception-driven authentication behavior.
108-133: LGTM: Test validates email verification requirement.The test correctly expects
EmailVerificationRequiredExceptionwith code 403 for unverified users, properly validating the email verification enforcement logic.src/Cms/Exceptions/EmailVerificationRequiredException.php (1)
15-36: LGTM: Well-designed verification exception.The exception class is appropriately designed for email verification requirements. The 403 status code is semantically correct (authenticated but not authorized to proceed without verification), and the design is consistent with the other exception classes in this PR.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Documentation / Chores
✏️ Tip: You can customize this high-level summary in your review settings.