Add authentication system with user management, migrations, and CLI commands #1
Conversation
WalkthroughComprehensive authentication system implementation with user management, session handling, CSRF protection, password reset workflows, and maintenance mode. Adds blog management (posts, categories, tags) with repositories and controllers. Includes database migrations, CLI commands for installation and user operations, email service, and extensive test coverage across new subsystems. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant Filter as AuthenticationFilter
participant LoginCtrl as LoginController
participant AuthMgr as AuthManager
participant Repo as DatabaseUserRepository
participant Session as SessionManager
participant Route as Route Handler
User->>Filter: HTTP Request
alt Not Authenticated
Filter->>Filter: check(): false
Filter->>User: Redirect to /login?redirect=/intended
else Authenticated
Filter->>AuthMgr: check()
AuthMgr->>Session: get('user_id')
AuthMgr-->>Filter: true
Filter->>Route: Continue
end
User->>LoginCtrl: POST /login (username, password, remember)
LoginCtrl->>Session: start()
LoginCtrl->>AuthMgr: attempt(username, password, remember)
AuthMgr->>Repo: findByUsername(username)
Repo-->>AuthMgr: User
alt User Inactive/Locked
AuthMgr-->>LoginCtrl: false
LoginCtrl->>Session: flash('error', message)
LoginCtrl->>User: Redirect /login
else Valid Password
AuthMgr->>Session: regenerate()
AuthMgr->>Session: set('user_id', id)
alt Remember Me Checked
AuthMgr->>AuthMgr: setRememberToken(User)
AuthMgr->>User: Set remember_me Cookie
end
AuthMgr-->>LoginCtrl: true
LoginCtrl->>Session: flash('success', message)
LoginCtrl->>User: Redirect to redirect_url
end
sequenceDiagram
participant Client as Client/Request
participant MaintFilter as MaintenanceFilter
participant MaintMgr as MaintenanceManager
participant FileSystem as .maintenance.json
Client->>MaintFilter: HTTP Request
MaintFilter->>MaintMgr: isEnabled()
MaintMgr->>FileSystem: Read .maintenance.json
alt Maintenance Disabled
FileSystem-->>MaintMgr: File not found
MaintMgr-->>MaintFilter: false
MaintFilter->>Client: Continue normal request
else Maintenance Enabled
FileSystem-->>MaintMgr: JSON content
MaintMgr-->>MaintFilter: true
MaintFilter->>MaintFilter: getClientIp()
MaintMgr->>MaintFilter: isIpAllowed(clientIp)
alt IP Whitelisted
MaintMgr-->>MaintFilter: true
MaintFilter->>Client: Continue normal request
else IP Not Allowed
MaintFilter->>MaintFilter: Render 503 page
MaintFilter->>Client: 503 Service Unavailable
end
end
sequenceDiagram
participant User as User
participant ResetCtrl as PasswordResetController
participant AuthMgr as PasswordResetManager
participant TokenRepo as TokenRepository
participant UserRepo as UserRepository
participant Email as EmailService
User->>ResetCtrl: GET /forgot-password
ResetCtrl->>User: Show form
User->>ResetCtrl: POST /forgot-password (email)
ResetCtrl->>AuthMgr: requestReset(email)
AuthMgr->>UserRepo: findByEmail(email)
alt User Found
UserRepo-->>AuthMgr: User
AuthMgr->>AuthMgr: Generate plainToken
AuthMgr->>TokenRepo: create(hashedToken, email)
AuthMgr->>Email: sendResetEmail(email, plainToken)
Email->>User: Reset Link Email
else User Not Found
AuthMgr->>ResetCtrl: true (silent success)
end
ResetCtrl->>User: Redirect with success message
User->>ResetCtrl: GET /reset-password?token=plainToken
ResetCtrl->>AuthMgr: validateToken(plainToken)
AuthMgr->>TokenRepo: findByToken(hashedToken)
alt Token Valid & Not Expired
TokenRepo-->>AuthMgr: PasswordResetToken
AuthMgr-->>ResetCtrl: PasswordResetToken
ResetCtrl->>User: Show reset form
else Token Invalid/Expired
ResetCtrl->>User: Redirect with error
end
User->>ResetCtrl: POST /reset-password (token, password)
ResetCtrl->>AuthMgr: resetPassword(plainToken, newPassword)
AuthMgr->>UserRepo: update(User with new hash)
AuthMgr->>TokenRepo: deleteByToken(hashedToken)
ResetCtrl->>User: Redirect to login
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds three major features to the Neuron CMS component: maintenance mode functionality, a comprehensive authentication layer, and database migration support. The PR includes extensive test coverage for all new features and introduces CLI commands for management operations.
- Adds maintenance mode with IP whitelisting and CIDR support for site-wide access control
- Implements authentication system with session management, CSRF protection, password hashing, and remember-me functionality
- Integrates Phinx for database migrations with CLI commands for creating and running migrations
- Includes comprehensive test suites achieving good coverage across all new components
- Adds CLI command structure with user management and maintenance commands
Reviewed Changes
Copilot reviewed 55 out of 57 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| versionlog.md | Added changelog entries for new features |
| composer.json | Added Phinx dependency and CLI provider configuration |
| tests/Cms/Repositories/DatabaseUserRepositoryTest.php | Comprehensive tests for database user repository |
| tests/Cms/Maintenance/*.php | Test coverage for maintenance mode functionality |
| tests/Cms/Cli/Commands//.php | Tests for CLI commands |
| tests/Cms/Auth/*.php | Tests for authentication components |
| src/Cms/Repositories/*.php | User repository interface and database implementation |
| src/Cms/Models/User.php | User entity model with role-based access |
| src/Cms/Maintenance/*.php | Maintenance mode manager, filter, and config |
| src/Cms/Database/MigrationManager.php | Phinx integration for migrations |
| src/Cms/Auth/*.php | Authentication, session, password, and CSRF managers |
| src/Cms/Controllers/*.php | Login and dashboard controllers |
| src/Cms/Cli/*.php | CLI command implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (15)
versionlog.md (1)
1-4: Add a version header for the new changelog entries.The new changelog entries lack a version header (e.g.,
## 0.9.0or## Unreleased) to group them, which is inconsistent with the existing changelog format where entries are organized under version sections like## 0.8.1and## 0.2.3 2025-11-04.Apply this diff to add a version header:
+## 0.9.0 (Unreleased) + * Added the maintenance mode command. * Added the authentication layer. * Added database migrations.MIGRATIONS.md (2)
49-58: Add language specifier to the fenced code block.The directory structure code block is missing a language specifier, which affects syntax highlighting and accessibility.
Apply this diff to add the language specifier:
-``` +```text your-project/ ├── config/ │ └── config.yaml # Database configuration
165-171: Add language specifier to the fenced code block.The migration status output example is missing a language specifier.
Apply this diff to add the language specifier:
-``` +```text Status Migration ID Migration Name -------------------------------------- up 20241105120000 CreateUsersTableAUTHENTICATION.md (1)
162-202: Add language specifier to the fenced code block.The file structure code block is missing a language specifier.
Apply this diff to add the language specifier:
-``` +```text cms/ ├── src/Cms/ │ ├── Auth/src/Cms/Cli/Commands/User/ListCommand.php (3)
68-104: Consider dynamic column width calculation.The fixed column widths work for typical data, but might truncate important information or waste space. For a better user experience, consider calculating column widths dynamically based on actual content length (with reasonable min/max bounds).
145-145: Consistent config path discovery recommended.The hardcoded config path
getcwd() . '/config/config.yaml'differs from the pattern used in SeedCommand (which has afindConfigPath()helper that searches multiple locations). For consistency across CLI commands, consider:
- Extracting config path discovery to a shared helper/trait
- Using the same multi-location search pattern
This would improve resilience when the command is run from different working directories.
42-42: Remove unused$Parametersparameter.The
$Parametersarray is declared but never used in theexecute()method body. Other similar Command implementations (StatusCommand, EnableCommand, Migrate commands) don't include this parameter, and DeleteCommand demonstrates proper usage when the parameter is actually needed. Remove$Parameters = []to align with consistent patterns and eliminate dead code.specs/authentication-implementation-plan.md (1)
1-1835: Comprehensive authentication implementation plan.This specification document provides excellent architectural guidance for the authentication implementation. The static analysis hints flag minor markdown formatting issues (bare URLs, missing code block languages) that can optionally be addressed for documentation polish, but don't affect the plan's utility.
src/Cms/Controllers/Auth/LoginController.php (1)
25-45: Consider dependency injection for better testability.All authentication components are constructed directly in the constructor, coupling the controller to concrete implementations. Injecting these dependencies (or using a factory/service locator pattern) would improve testability and flexibility.
Example refactor:
- public function __construct( ?Application $app = null ) + public function __construct( + ?Application $app = null, + ?AuthManager $authManager = null, + ?CsrfTokenManager $csrfManager = null + ) { parent::__construct( $app ); - - // Initialize auth components - $StoragePath = $this->getApp()->getBasePath() . '/storage/users'; - $UserRepository = new \Neuron\Cms\Repositories\UserRepository( $StoragePath ); - - $this->_SessionManager = new SessionManager(); - $this->_SessionManager->start(); - - $PasswordHasher = new \Neuron\Cms\Auth\PasswordHasher(); - - $this->_AuthManager = new AuthManager( - $UserRepository, - $this->_SessionManager, - $PasswordHasher - ); - - $this->_CsrfManager = new CsrfTokenManager( $this->_SessionManager ); + + // Use injected or create default instances + if (!$authManager) { + $StoragePath = $this->getApp()->getBasePath() . '/storage/users'; + $UserRepository = new \Neuron\Cms\Repositories\UserRepository( $StoragePath ); + $this->_SessionManager = new SessionManager(); + $this->_SessionManager->start(); + $PasswordHasher = new \Neuron\Cms\Auth\PasswordHasher(); + $authManager = new AuthManager( $UserRepository, $this->_SessionManager, $PasswordHasher ); + $csrfManager = new CsrfTokenManager( $this->_SessionManager ); + } + + $this->_AuthManager = $authManager; + $this->_CsrfManager = $csrfManager ?? new CsrfTokenManager( $this->_SessionManager ); }src/Cms/Auth/PasswordHasher.php (2)
53-121: Consider refactoring to eliminate validation duplication.The validation logic is duplicated between
meetsRequirements()andgetValidationErrors(). Refactoring to share the validation checks would improve maintainability.Example refactor:
+ private function validateLength( string $Password ): ?string + { + return strlen( $Password ) < $this->_MinLength + ? "Password must be at least {$this->_MinLength} characters long" + : null; + } + + private function validateUppercase( string $Password ): ?string + { + return $this->_RequireUppercase && !preg_match( '/[A-Z]/', $Password ) + ? 'Password must contain at least one uppercase letter' + : null; + } + + // ... similar methods for other validations + public function meetsRequirements( string $Password ): bool { - // Check minimum length - if( strlen( $Password ) < $this->_MinLength ) - { - return false; - } - // ... rest of checks + return empty( $this->getValidationErrors( $Password ) ); } public function getValidationErrors( string $Password ): array { - $Errors = []; - - if( strlen( $Password ) < $this->_MinLength ) - { - $Errors[] = "Password must be at least {$this->_MinLength} characters long"; - } - // ... rest of checks - - return $Errors; + return array_filter([ + $this->validateLength( $Password ), + $this->validateUppercase( $Password ), + $this->validateLowercase( $Password ), + $this->validateNumbers( $Password ), + $this->validateSpecialChars( $Password ), + ]); }
171-199: Add input validation to configure() method.The
configure()method doesn't validate input values. For example,min_lengthcould be set to a negative number or zero. Consider adding validation to ensure sensible values.Example validation:
public function configure( array $Settings ): self { if( isset( $Settings['min_length'] ) ) { + if( $Settings['min_length'] < 1 ) + { + throw new \InvalidArgumentException( 'min_length must be at least 1' ); + } $this->setMinLength( $Settings['min_length'] ); } if( isset( $Settings['require_uppercase'] ) ) { + if( !is_bool( $Settings['require_uppercase'] ) ) + { + throw new \InvalidArgumentException( 'require_uppercase must be a boolean' ); + } $this->setRequireUppercase( $Settings['require_uppercase'] ); } // ... similar validation for other settingssrc/Cms/Cli/Commands/Maintenance/DisableCommand.php (1)
109-141: Consider extracting common CLI helpers.The
findConfigPath()andconfirm()methods are duplicated across multiple CLI commands (DisableCommand, StatusCommand, RollbackCommand). Extracting these to a base command class or trait would reduce duplication.Example:
// src/Cms/Cli/Commands/BaseCommand.php abstract class BaseCommand extends Command { protected function findConfigPath(): ?string { $locations = [ getcwd() . '/config', dirname( getcwd() ) . '/config', dirname( getcwd(), 2 ) . '/config', dirname( __DIR__, 6 ) . '/config', dirname( __DIR__, 7 ) . '/config', ]; foreach( $locations as $location ) { if( is_dir( $location ) ) { return $location; } } return null; } protected function confirm( string $question ): bool { $this->output->write( $question . ' [y/N] ' ); $answer = trim( fgets( STDIN ) ); return strtolower( $answer ) === 'y' || strtolower( $answer ) === 'yes'; } }src/Cms/Auth/AuthManager.php (3)
212-220: Cookie domain parameter should be explicit.The empty string for the
domainparameter (line 217) may not behave as expected across different environments. Consider usingnull(omit the parameter) or making it configurable.Additionally, consider making the cookie expiry configurable:
+ private int $_RememberTokenDuration = 30; // days + private function setRememberToken( User $User ): void { // Generate secure random token $Token = bin2hex( random_bytes( 32 ) ); // Hash the token before storing $HashedToken = hash( 'sha256', $Token ); // Store hashed token in user record $User->setRememberToken( $HashedToken ); $this->_UserRepository->update( $User ); - // Set cookie with plain token (30 days) + // Set cookie with plain token setcookie( 'remember_token', $Token, - time() + (30 * 24 * 60 * 60), + time() + ($this->_RememberTokenDuration * 24 * 60 * 60), '/', - '', + null, // Use default domain true, // Secure true // HTTPOnly ); }
244-260: Consider adding input validation to configuration setters.The setter methods don't validate that the values are positive, which could lead to unexpected behavior.
public function setMaxLoginAttempts( int $MaxLoginAttempts ): self { + if( $MaxLoginAttempts < 1 ) + { + throw new \InvalidArgumentException( 'Max login attempts must be at least 1' ); + } $this->_MaxLoginAttempts = $MaxLoginAttempts; return $this; } public function setLockoutDuration( int $LockoutDuration ): self { + if( $LockoutDuration < 1 ) + { + throw new \InvalidArgumentException( 'Lockout duration must be at least 1 minute' ); + } $this->_LockoutDuration = $LockoutDuration; return $this; }
293-305: LGTM! Consistent role hierarchy checking.The implementation is correct and consistent with
isEditorOrHigher().Consider extracting the common pattern to reduce duplication:
private function hasAnyRole( array $Roles ): bool { $User = $this->user(); if( !$User ) { return false; } return in_array( $User->getRole(), $Roles ); } public function isEditorOrHigher(): bool { return $this->hasAnyRole( [User::ROLE_ADMIN, User::ROLE_EDITOR] ); } public function isAuthorOrHigher(): bool { return $this->hasAnyRole( [User::ROLE_ADMIN, User::ROLE_EDITOR, User::ROLE_AUTHOR] ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
AUTHENTICATION.md(1 hunks)MIGRATIONS.md(1 hunks)composer.json(2 hunks)config/auth.yaml(1 hunks)examples/config/routes.yaml(1 hunks)resources/config/database.yaml.example(1 hunks)resources/config/maintenance.yaml.example(1 hunks)resources/views/admin/auth/login.php(1 hunks)resources/views/admin/dashboard/index.php(1 hunks)resources/views/admin/layouts/admin.php(1 hunks)resources/views/admin/layouts/auth.php(1 hunks)specs/authentication-implementation-plan.md(1 hunks)src/Cms/Auth/AuthManager.php(1 hunks)src/Cms/Auth/CsrfTokenManager.php(1 hunks)src/Cms/Auth/Filters/AuthenticationFilter.php(1 hunks)src/Cms/Auth/Filters/CsrfFilter.php(1 hunks)src/Cms/Auth/PasswordHasher.php(1 hunks)src/Cms/Auth/SessionManager.php(1 hunks)src/Cms/Auth/helpers.php(1 hunks)src/Cms/Cli/Commands/Install/InstallCommand.php(1 hunks)src/Cms/Cli/Commands/Maintenance/DisableCommand.php(1 hunks)src/Cms/Cli/Commands/Maintenance/EnableCommand.php(1 hunks)src/Cms/Cli/Commands/Maintenance/StatusCommand.php(1 hunks)src/Cms/Cli/Commands/Migrate/CreateCommand.php(1 hunks)src/Cms/Cli/Commands/Migrate/RollbackCommand.php(1 hunks)src/Cms/Cli/Commands/Migrate/RunCommand.php(1 hunks)src/Cms/Cli/Commands/Migrate/SeedCommand.php(1 hunks)src/Cms/Cli/Commands/Migrate/StatusCommand.php(1 hunks)src/Cms/Cli/Commands/User/CreateCommand.php(1 hunks)src/Cms/Cli/Commands/User/DeleteCommand.php(1 hunks)src/Cms/Cli/Commands/User/ListCommand.php(1 hunks)src/Cms/Cli/Provider.php(1 hunks)src/Cms/Controllers/Admin/DashboardController.php(1 hunks)src/Cms/Controllers/Auth/LoginController.php(1 hunks)src/Cms/Database/MigrationManager.php(1 hunks)src/Cms/Maintenance/MaintenanceConfig.php(1 hunks)src/Cms/Maintenance/MaintenanceFilter.php(1 hunks)src/Cms/Maintenance/MaintenanceManager.php(1 hunks)src/Cms/Maintenance/README.md(1 hunks)src/Cms/Models/User.php(1 hunks)src/Cms/Repositories/DatabaseUserRepository.php(1 hunks)src/Cms/Repositories/IUserRepository.php(1 hunks)tests/Cms/Auth/AuthManagerTest.php(1 hunks)tests/Cms/Auth/CsrfTokenManagerTest.php(1 hunks)tests/Cms/Auth/PasswordHasherTest.php(1 hunks)tests/Cms/Auth/SessionManagerTest.php(1 hunks)tests/Cms/Auth/UserTest.php(1 hunks)tests/Cms/Cli/Commands/Install/InstallCommandTest.php(1 hunks)tests/Cms/Cli/Commands/User/CreateCommandTest.php(1 hunks)tests/Cms/Maintenance/MaintenanceCommandsTest.php(1 hunks)tests/Cms/Maintenance/MaintenanceConfigTest.php(1 hunks)tests/Cms/Maintenance/MaintenanceFilterTest.php(1 hunks)tests/Cms/Maintenance/MaintenanceManagerTest.php(1 hunks)tests/Cms/Repositories/DatabaseUserRepositoryTest.php(1 hunks)versionlog.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/authentication-implementation-plan.md
[grammar] ~17-~17: Use a hyphen to join words.
Context: ...Integrate with existing routing and rate limiting systems 5. Maintain framework s...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~46-~46: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ect for authentication middleware #### Rate Limiting - Location: `routing/src/Routing/Ra...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~49-~49: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Capabilities**: - Per-IP and per-user rate limiting - Multiple storage backends (Redis, f...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~80-~80: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ion (X-API-Key) - Integration with rate limiting - Support for tiered access (free/pa...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~615-~615: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ..., "status": 401 } ``` #### 3.4 API Rate Limiting Integration **Configure Tiered Rate Li...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~832-~832: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...ed as verified 6. Redirect to dashboard with success message #### 4.7 "Remember Me" Functio...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
[uncategorized] ~1167-~1167: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...equests: false # Can be noisy ``` ### Rate Limiting Configuration File: `config/config...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~1676-~1676: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e created/revoked via admin panel - [ ] Rate limiting works per API key tier - [ ] API docume...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
MIGRATIONS.md
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
specs/authentication-implementation-plan.md
85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
205-205: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
235-235: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
237-237: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
261-261: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
291-291: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
312-312: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
314-314: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
331-331: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
352-352: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
371-371: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
394-394: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
396-396: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
426-426: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
428-428: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
436-436: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
438-438: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
450-450: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
452-452: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
466-466: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
468-468: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
489-489: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
491-491: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
501-501: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
503-503: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
525-525: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
527-527: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
548-548: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
550-550: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
564-564: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
566-566: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
592-592: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
594-594: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
617-617: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
650-650: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
652-652: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
678-678: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
680-680: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
692-692: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
707-707: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
709-709: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
726-726: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
728-728: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
746-746: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
748-748: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
766-766: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
768-768: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
801-801: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
803-803: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
816-816: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
818-818: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
836-836: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
856-856: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1381-1381: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1551-1551: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1560-1560: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1565-1565: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1570-1570: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1575-1575: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1589-1589: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1605-1605: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1689-1689: Bare URL used
(MD034, no-bare-urls)
1690-1690: Bare URL used
(MD034, no-bare-urls)
1691-1691: Bare URL used
(MD034, no-bare-urls)
1694-1694: Bare URL used
(MD034, no-bare-urls)
1695-1695: Bare URL used
(MD034, no-bare-urls)
1696-1696: Bare URL used
(MD034, no-bare-urls)
1706-1706: Bare URL used
(MD034, no-bare-urls)
1707-1707: Bare URL used
(MD034, no-bare-urls)
1708-1708: Bare URL used
(MD034, no-bare-urls)
AUTHENTICATION.md
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
src/Cms/Auth/Filters/CsrfFilter.php
36-36: Avoid unused parameters such as '$Route'. (undefined)
(UnusedFormalParameter)
src/Cms/Maintenance/MaintenanceFilter.php
40-40: Avoid unused parameters such as '$Route'. (undefined)
(UnusedFormalParameter)
84-84: Avoid unused local variables such as '$message'. (undefined)
(UnusedLocalVariable)
tests/Cms/Auth/AuthManagerTest.php
101-101: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
111-111: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
226-226: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
239-239: Avoid unused local variables such as '$user'. (undefined)
(UnusedLocalVariable)
301-301: Avoid unused local variables such as '$oldHash'. (undefined)
(UnusedLocalVariable)
src/Cms/Cli/Commands/Install/InstallCommand.php
59-59: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
157-187: Avoid unused private methods such as 'createDirectories'. (undefined)
(UnusedPrivateMethod)
192-205: Avoid unused private methods such as 'publishViews'. (undefined)
(UnusedPrivateMethod)
210-241: Avoid unused private methods such as 'createRouteConfig'. (undefined)
(UnusedPrivateMethod)
246-265: Avoid unused private methods such as 'createAuthConfig'. (undefined)
(UnusedPrivateMethod)
317-359: Avoid unused private methods such as 'setupDatabase'. (undefined)
(UnusedPrivateMethod)
src/Cms/Cli/Commands/User/ListCommand.php
42-42: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Admin/DashboardController.php
25-25: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
src/Cms/Cli/Commands/User/CreateCommand.php
44-44: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
src/Cms/Controllers/Auth/LoginController.php
50-50: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
79-79: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
123-123: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
tests/Cms/Cli/Commands/Install/InstallCommandTest.php
207-213: Avoid unused private methods such as 'setInputStream'. (undefined)
(UnusedPrivateMethod)
🔇 Additional comments (35)
composer.json (1)
37-40: LGTM!The CLI provider registration is properly configured and will enable automatic discovery of CMS commands.
resources/config/database.yaml.example (1)
1-42: LGTM!The database configuration example is well-structured with helpful comments. The use of
utf8mb4charset is the correct choice for full Unicode support in MySQL.resources/config/maintenance.yaml.example (1)
1-28: LGTM!The maintenance mode configuration example is well-documented with sensible defaults. The inclusion of both IPv4 and IPv6 localhost addresses and CIDR notation examples is helpful.
resources/views/admin/layouts/auth.php (1)
1-80: LGTM!The authentication layout template properly escapes all user-provided data using
htmlspecialchars(). The gradient design provides a visually distinct authentication experience.AUTHENTICATION.md (1)
1-454: Excellent comprehensive documentation!This authentication guide is thorough, well-organized, and provides both quick start options and detailed explanations. The inclusion of security features, troubleshooting, and clear phase indicators is particularly valuable.
tests/Cms/Auth/SessionManagerTest.php (1)
1-207: Comprehensive test coverage for SessionManager.The test suite thoroughly exercises SessionManager's functionality including basic operations, flash messaging with single-use semantics, session regeneration with data preservation, and lifecycle management. The use of
@runTestsInSeparateProcessesensures proper test isolation for session state.src/Cms/Cli/Commands/Migrate/CreateCommand.php (1)
44-129: Well-structured command with robust error handling.The
execute()method demonstrates good practices:
- Validates required inputs with clear error messages
- Handles missing configuration gracefully
- Provides helpful usage information
- Includes verbose mode for debugging
- Properly delegates to MigrationManager for Phinx integration
src/Cms/Cli/Commands/User/DeleteCommand.php (1)
42-113: Excellent safeguards for destructive operation.The delete command implements strong safety measures:
- Displays complete user information before deletion (lines 78-85)
- Requires strict "DELETE" confirmation (line 90) to prevent accidental deletions
- Provides clear, formatted output with emoji for visual clarity
- Handles both ID and username lookup gracefully
This is a good pattern for destructive CLI operations.
src/Cms/Auth/CsrfTokenManager.php (1)
1-69: Secure CSRF token implementation following best practices.The implementation correctly uses:
random_bytes(32)for cryptographically secure token generation (line 28)hash_equals()for timing-safe comparison to prevent timing attacks (line 59)- Clean API design with automatic token generation when absent
This provides solid protection against CSRF attacks.
tests/Cms/Maintenance/MaintenanceConfigTest.php (1)
1-169: Comprehensive test coverage for MaintenanceConfig.The test suite effectively validates:
- Default configuration behavior without settings source
- Loading from YAML with all options specified
- Static factory method
fromSettings()- Edge cases: comma-separated IP strings, partial configs with defaults, empty config sections
This provides strong confidence in the MaintenanceConfig implementation.
tests/Cms/Maintenance/MaintenanceFilterTest.php (1)
42-240: Thorough coverage of maintenance scenariosGreat job exercising the filter across disabled mode, whitelisting, proxy headers, custom views, and retry-after rendering. This suite gives strong confidence in the maintenance gate.
tests/Cms/Maintenance/MaintenanceCommandsTest.php (1)
32-232: Command workflow tests look solidCovering status defaults, re-enable updates, persistence across instances, and CIDR checks makes this command suite very robust. Nicely done.
src/Cms/Cli/Commands/Migrate/StatusCommand.php (1)
44-142: Status command implementation reads cleanConfig discovery, guardrails for missing directories, and delegation to
MigrationManagerare tidy. No changes needed.src/Cms/Cli/Commands/Migrate/SeedCommand.php (1)
1-157: LGTM! Well-structured seed command implementation.The command follows consistent patterns with other migration commands in the PR. The error handling is appropriate, config discovery logic is sound, and the integration with MigrationManager is clean.
src/Cms/Maintenance/MaintenanceFilter.php (3)
40-58: Parameter required by callback signature.The static analysis warning about unused
$Routeparameter is a false positive. This parameter is required by theFilterparent class callback signature, even though it's not used in this particular implementation.
84-86: Variable is used in dynamic include.The static analysis warning about unused
$messagevariable is a false positive. The variable is intentionally extracted for use within the included custom view template at line 86.
234-266: IP detection logic is sound.The
getClientIp()method properly handles proxy headers and validates IP addresses. The header priority order (X-Forwarded-For → X-Real-IP → Client-IP → REMOTE_ADDR) is standard and appropriate. The validation usingfilter_var()withFILTER_VALIDATE_IPprevents header injection.tests/Cms/Maintenance/MaintenanceManagerTest.php (1)
1-274: Excellent test coverage!The test suite is comprehensive and well-structured:
- Proper isolation using vfsStream
- Covers default states, enable/disable workflows
- Tests IP allowance including CIDR notation (/, /16, /24)
- Validates JSON file creation and format
- Tests edge cases appropriately
src/Cms/Maintenance/MaintenanceConfig.php (1)
1-146: Clean configuration handler implementation.The class properly:
- Provides sensible defaults for all settings
- Handles both array and comma-separated string formats for
allowed_ips- Type casts values appropriately (e.g.,
retry_afterto int)- Uses a static factory pattern for convenient instantiation
src/Cms/Auth/SessionManager.php (1)
1-185: LGTM! Solid session management implementation.The SessionManager class follows security best practices with secure cookie defaults (httponly, secure, samesite), proper session lifecycle management, and correct flash message implementation. The defensive checks (e.g., line 34 checking both
_Startedandsession_status()) prevent duplicate session starts.src/Cms/Repositories/IUserRepository.php (1)
1-58: LGTM! Clean repository interface.The interface provides a complete contract for user repository implementations with proper type hints and comprehensive CRUD operations plus specialized finders (username, email, remember token).
src/Cms/Controllers/Auth/LoginController.php (1)
123-132: Question: Why restart session after logout?Line 127 restarts the session immediately after calling
logout()(which presumably destroys it) just to set a flash message. This seems unnecessary—consider flashing the message before logout, or passing the message as a query parameter to avoid session overhead.Alternative approach:
public function logout( array $Parameters ): string { + $this->_SessionManager->flash( 'success', 'You have been logged out successfully.' ); $this->_AuthManager->logout(); - - $this->_SessionManager->start(); - $this->_SessionManager->flash( 'success', 'You have been logged out successfully.' ); header( 'Location: /login' ); exit; }src/Cms/Auth/PasswordHasher.php (1)
24-48: LGTM! Proper password hashing implementation.The hash/verify/needsRehash methods correctly use PHP's native password functions with appropriate algorithm preference (Argon2id when available, bcrypt fallback).
src/Cms/Cli/Commands/Maintenance/DisableCommand.php (1)
1-142: LGTM! Clean command implementation.The DisableCommand is well-structured with proper error handling, user confirmation, and clear output messages. The logic flows cleanly from validation through confirmation to execution.
src/Cms/Cli/Commands/Maintenance/StatusCommand.php (1)
1-259: LGTM! Comprehensive status reporting.The StatusCommand provides excellent user experience with both JSON and human-readable output formats, detailed status information, and helpful time formatting. The
getTimeSince()helper (lines 203-232) with proper singular/plural handling is a nice touch.src/Cms/Cli/Commands/Migrate/RollbackCommand.php (1)
1-208: LGTM! Well-structured rollback command.The RollbackCommand provides comprehensive options (target, date, force, dry-run, fake) with proper validation, confirmation, and error handling. The Phinx integration is clean and the output is informative.
src/Cms/Auth/AuthManager.php (9)
1-24: LGTM! Clean class structure.The class properties and default lockout configuration (5 attempts, 15-minute lockout) are well-structured and reasonable.
25-34: LGTM! Proper dependency injection.Constructor follows best practices with explicit dependency injection.
95-112: LGTM! Proper session fixation prevention.Session regeneration on login is a critical security practice that's correctly implemented here. Note that caching
user_rolein the session means role changes won't take effect until the next login, but this is typically an acceptable tradeoff.Verify that role changes are either:
- Expected to require re-login to take effect, or
- Handled by explicitly clearing sessions when roles are modified
188-194: LGTM! Proper credential validation delegation.The method correctly delegates to the
PasswordHasher, which should use timing-safe comparison internally.
226-242: Remember token behavior may surprise multi-device users.Line 239 passes
$Remember = true, which triggerssetRememberToken()(line 110), generating a new token and invalidating the old one. This implements automatic token rotation, which is good for security but means users will be logged out of other devices when auto-logging in from one device.Additionally, there's no expiry check on remember tokens. Consider:
- Documenting the token rotation behavior if it's intentional
- Adding a token expiry check:
public function loginUsingRememberToken( string $Token ): bool { // Hash the token to compare with stored hash $HashedToken = hash( 'sha256', $Token ); $User = $this->_UserRepository->findByRememberToken( $HashedToken ); - if( !$User || !$User->isActive() ) + if( !$User || !$User->isActive() || $this->isRememberTokenExpired( $User ) ) { return false; }
- If token rotation is not desired, consider:
- $this->login( $User, true ); + $this->login( $User, false );
262-269: LGTM! Role checking logic is correct.The role comparison logic is straightforward and appropriate.
271-277: LGTM! Clean helper method.Good use of the role constant and delegation pattern.
279-291: LGTM! Proper hierarchical role checking.The implementation correctly checks for multiple roles using
in_array.
134-137: Cookie deletion parameters should match cookie creation.The
setcookiecall to delete the remember token should use the samepath,domain,secure, andhttponlyparameters that were used when creating the cookie (line 212-220). Currently, only the domain and path are specified, butsecureandhttponlyare omitted.Apply this diff to match the cookie creation parameters:
// Delete remember me cookie if exists if( isset( $_COOKIE['remember_token'] ) ) { - setcookie( 'remember_token', '', time() - 3600, '/', '', true, true ); + setcookie( + 'remember_token', + '', + time() - 3600, + '/', + '', + true, // Secure + true // HTTPOnly + ); }Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ljonesfl
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: ljonesfl <1099983+ljonesfl@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: ljonesfl <1099983+ljonesfl@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: ljonesfl <1099983+ljonesfl@users.noreply.github.com>
Co-authored-by: ljonesfl <1099983+ljonesfl@users.noreply.github.com>
Fix open redirect vulnerability in LoginController
Fix open redirect vulnerability in login controller
Co-authored-by: ljonesfl <1099983+ljonesfl@users.noreply.github.com>
Co-authored-by: ljonesfl <1099983+ljonesfl@users.noreply.github.com>
Add path traversal protection to MaintenanceFilter custom view includes
There was a problem hiding this comment.
Actionable comments posted: 45
♻️ Duplicate comments (4)
src/Cms/Cli/Commands/User/CreateCommand.php (1)
245-249: Hide password input when prompting credentials.The
prompt()method echoes user input directly to the terminal. When collecting the password at Line 94, this leaks administrator credentials to shoulder surfers and terminal scrollback, which is a security vulnerability.Apply the fix from the previous review to disable terminal echo for password input:
- private function prompt( string $Message ): string - { - echo $Message; - return trim( fgets( STDIN ) ); - } + private function prompt( string $Message, bool $Hidden = false ): string + { + echo $Message; + + $restore = null; + if( $Hidden ) + { + if( function_exists( 'sapi_windows_set_echo' ) ) + { + sapi_windows_set_echo( false ); + $restore = static fn() => sapi_windows_set_echo( true ); + } + else + { + shell_exec( 'stty -echo' ); + $restore = static fn() => shell_exec( 'stty echo' ); + } + } + + $input = fgets( STDIN ); + + if( $Hidden && $restore ) + { + $restore(); + echo PHP_EOL; + } + + return $input === false ? '' : rtrim( $input, "\r\n" ); + }Then update Line 94:
- $password = $this->prompt( "Enter password (min 8 characters): " ); + $password = $this->prompt( "Enter password (min 8 characters): ", true );src/Cms/Cli/Commands/Install/InstallCommand.php (1)
572-578: Handle Windows absolute SQLite pathsPaths like
C:\data\cms.sqliteor\\server\share\cms.sqlitestill fall into the “relative” branch and get prefixed with the project directory, breaking installs on Windows. Please treat drive-letter and UNC prefixes as absolute before prepending the project root.- if( !empty( $dbPath ) && $dbPath[0] !== '/' ) + if( + !empty( $dbPath ) + && !preg_match( '#^(?:/|[A-Za-z]:[\\\\/]|\\\\\\\\)#', $dbPath ) + ) { $dbPath = $this->_ProjectPath . '/' . $dbPath; }src/Cms/Cli/Commands/Maintenance/EnableCommand.php (1)
65-115: Still need a non-null default message
$messagestaysnullwhen no option and no config default exist, so the call toMaintenanceManager::enable()throws aTypeErrorin the default path. Please ensure we always pass a real string (e.g., fall back to the manager’s default message) before enabling maintenance mode.$message = $this->input->getOption( 'message' ); - if( !$message ) + if( $message === null || $message === '' ) { $message = $config ? $config->getDefaultMessage() : null; } + if( $message === null || $message === '' ) + { + $message = 'Site is currently under maintenance. Please check back soon.'; + }src/Cms/Auth/AuthManager.php (1)
62-76: Make failed-login counting atomic.
The increment + update sequence is still non-atomic, so parallel failed attempts overwrite each other and let attackers bypass lockouts. Use a transactional update or a repository method that executesfailed_login_attempts = failed_login_attempts + 1atomically (and setslocked_untilin the same statement).
🧹 Nitpick comments (23)
src/Cms/Repositories/IPostRepository.php (1)
1-136: LGTM! Well-designed repository interface.The interface provides a comprehensive contract for Post repository implementations with clear method signatures and good documentation.
Consider using more precise PHPDoc array syntax for better IDE support and static analysis:
/** * Get all posts * * @param string|null $Status Filter by status (published, draft, scheduled) * @param int $Limit Limit number of results (0 = no limit) * @param int $Offset Offset for pagination - * @return Post[] + * @return array<int, Post> */Apply similar changes to other methods returning
Post[]arrays.src/Cms/Maintenance/MaintenanceManager.php (2)
206-224: Replace deprecatedlist()with array destructuring.The
list()construct is deprecated in favor of the short array destructuring syntax.- list( $subnet, $mask ) = explode( '/', $Cidr ); + [ $subnet, $mask ] = explode( '/', $Cidr );
170-182: Add error handling for JSON decode failures.The
json_decode()call can fail silently, returningnullfor malformed JSON. Consider adding explicit error checking.private function readMaintenanceFile(): array { $contents = file_get_contents( $this->_MaintenanceFilePath ); if( $contents === false ) { return []; } $data = json_decode( $contents, true ); + + if( json_last_error() !== JSON_ERROR_NONE ) + { + return []; + } return is_array( $data ) ? $data : []; }src/Cms/Cli/Commands/User/CreateCommand.php (1)
167-240: Extract duplicate database configuration logic to a shared helper.The
getUserRepository()andgetDatabaseConfig()methods (lines 167-240) are identical to those inDeleteCommand.php. This violates the DRY principle and makes maintenance harder.Consider extracting these methods to a shared base class or trait:
// src/Cms/Cli/Commands/User/DatabaseConfigTrait.php trait DatabaseConfigTrait { private function getUserRepository(): ?DatabaseUserRepository { // ... existing implementation } private function getDatabaseConfig( SettingManager $Settings ): ?array { // ... existing implementation } }Then use it in both commands:
class CreateCommand extends Command { + use DatabaseConfigTrait; - private function getUserRepository(): ?DatabaseUserRepository - { - // ... remove duplicate code - }resources/views/http_codes/404.php (1)
7-12: Consider moving styles to a layout or CSS file.Inline styles in views make them harder to maintain and prevent style reuse. Consider moving these styles to a shared CSS file or the layout template.
This is a minor style concern and can be deferred if layouts already provide adequate styling.
resources/views/admin/tags/edit.php (1)
4-4: Consider using a URL helper for route generation.Hard-coded URLs (
/admin/tags) make the application harder to maintain if routing structure changes. If a URL helper or route name resolver is available, prefer using it.Example (assuming a
url()orroute()helper exists):- <a href="/admin/tags" class="btn btn-secondary">Back to Tags</a> + <a href="<?= url('/admin/tags') ?>" class="btn btn-secondary">Back to Tags</a>If no helper exists yet, this can be addressed in a future refactoring pass.
Also applies to: 9-9, 25-25
resources/views/admin/users/edit.php (1)
4-4: Consider using a URL helper for route generation.Hard-coded URLs (
/admin/users) make the application harder to maintain if routing structure changes. If a URL helper or route name resolver is available, prefer using it.Example (assuming a
url()orroute()helper exists):- <a href="/admin/users" class="btn btn-secondary">Back to Users</a> + <a href="<?= url('/admin/users') ?>" class="btn btn-secondary">Back to Users</a>If no helper exists yet, this can be addressed in a future refactoring pass.
Also applies to: 9-9, 42-42
resources/views/admin/tags/create.php (2)
1-28: Consider adding error message display and old input preservation.The form lacks:
- A mechanism to display validation errors returned from the controller
- Preservation of old input values if validation fails
This could lead to poor UX where users lose their input on validation failure.
Consider adding error display at the top of the form:
<?php if( isset( $errors ) && !empty( $errors ) ): ?> <div class="alert alert-danger"> <ul class="mb-0"> <?php foreach( $errors as $error ): ?> <li><?= htmlspecialchars( $error ) ?></li> <?php endforeach; ?> </ul> </div> <?php endif; ?>And preserve old input values:
<input type="text" class="form-control" id="name" name="name" value="<?= htmlspecialchars( $old['name'] ?? '' ) ?>" required> <input type="text" class="form-control" id="slug" name="slug" value="<?= htmlspecialchars( $old['slug'] ?? '' ) ?>" required>
18-19: Consider making slug auto-generation optional.While the slug field is marked required, typical CMS workflows allow automatic slug generation from the name field. The helper text suggests it's a "URL-friendly version of the name" but requires manual entry.
Consider adding JavaScript to auto-generate the slug from the name field:
<script> document.getElementById('name').addEventListener('input', function(e) { const slug = e.target.value .toLowerCase() .replace(/[^a-z0-9]+/g, '-') .replace(/^-+|-+$/g, ''); document.getElementById('slug').value = slug; }); </script>resources/views/admin/categories/create.php (2)
1-33: Code duplication with tags/create.php.This form is nearly identical to
resources/views/admin/tags/create.php(lines 1-28), with only minor differences (entity name and description field). This violates DRY principles.Consider creating a shared form partial or component that can be reused:
Create a new file
resources/views/admin/partials/entity-form.php:<div class="container-fluid"> <div class="d-flex justify-content-between align-items-center mb-4"> <h2><?= htmlspecialchars( $formTitle ) ?></h2> <a href="<?= htmlspecialchars( $backUrl ) ?>" class="btn btn-secondary">Back to <?= htmlspecialchars( $entityName ) ?></a> </div> <div class="card"> <div class="card-body"> <form method="POST" action="<?= htmlspecialchars( $formAction ) ?>"> <?= csrf_field() ?> <?= $formFields ?> <button type="submit" class="btn btn-primary"><?= htmlspecialchars( $submitText ) ?></button> <a href="<?= htmlspecialchars( $backUrl ) ?>" class="btn btn-secondary">Cancel</a> </form> </div> </div> </div>
9-31: Missing error display and old input preservation.Same issues as in
resources/views/admin/tags/create.php: no validation error display or input preservation on validation failure.Apply the same error handling pattern suggested for the tags form (see resources/views/admin/tags/create.php review).
resources/views/admin/users/create.php (2)
1-44: Code duplication with other admin create forms.This form follows the same structure as
resources/views/admin/tags/create.phpandresources/views/admin/categories/create.php, further indicating the need for a shared form component.Consider extracting common form structure into a reusable partial as suggested in the categories/create.php review.
25-27: Password policy hint lacks client-side enforcement.The password requirements are displayed but not enforced client-side, leading to poor UX where users submit invalid passwords and must wait for server validation.
Add HTML5 pattern validation:
<div class="mb-3"> <label for="password" class="form-label">Password</label> - <input type="password" class="form-control" id="password" name="password" required> + <input type="password" class="form-control" id="password" name="password" + pattern="^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$" + title="Password must be at least 8 characters and include uppercase, lowercase, number, and special character" + required> <small class="form-text text-muted"> Password requirements: min 8 chars, uppercase, lowercase, number, special character </small> </div>resources/views/auth/forgot-password.php (2)
8-8: Inconsistent CSRF token implementation.This form uses
$CsrfTokenvariable directly, while admin forms use thecsrf_field()helper (see resources/views/admin/tags/create.php line 10). This inconsistency makes the codebase harder to maintain.Use the
csrf_field()helper for consistency:- <input type="hidden" name="csrf_token" value="<?= htmlspecialchars($CsrfToken) ?>"> + <?= csrf_field() ?>This assumes the controller provides the CSRF token via the helper's mechanism. If
$CsrfTokenis specifically provided for auth forms, document this pattern or unify the approach.
1-30: Missing success/error message display.The form has no area to display feedback messages (e.g., "Reset link sent" success or "Email not found" error), requiring a full page reload or redirect to show feedback.
Add a message display area at the top:
<?php if( isset( $message ) ): ?> <div class="alert alert-<?= htmlspecialchars( $messageType ?? 'info' ) ?>"> <?= htmlspecialchars( $message ) ?> </div> <?php endif; ?>resources/public/index.php (1)
10-16: Missing error handling for bootstrap process.If
boot()ordispatch()throws an exception, the entry point has no error handling, potentially exposing stack traces or causing a blank page.Add error handling:
require '../vendor/autoload.php'; error_reporting( E_ALL ); -$App = boot( '../config' ); +try { + $App = boot( '../config' ); + dispatch( $App ); +} catch ( \Throwable $e ) { + // Log the error + error_log( $e->getMessage() ); + + // Show friendly error page (don't expose details in production) + if( getenv('APP_ENV') !== 'production' ) { + throw $e; + } + + http_response_code( 500 ); + echo 'An error occurred. Please try again later.'; +} - -dispatch( $App );resources/views/layouts/default.php (1)
24-27: Consider adding SRI hashes for external resources.External CDN resources lack Subresource Integrity (SRI) hashes, which could expose users to supply-chain attacks if the CDN is compromised.
resources/views/admin/profile/edit.php (1)
70-70: Hidden email field may be unnecessary.The hidden email field in the password change form passes the user's email to the backend. Since the user is already authenticated (this is an admin profile page), the backend should retrieve the user's email from the session rather than trusting a client-provided value. This prevents potential manipulation.
resources/views/admin/posts/edit.php (1)
48-61: Consider accessibility improvements for multi-select.The categories multi-select relies on users knowing to hold Ctrl/Cmd for multiple selections. Consider enhancing accessibility by:
- Adding
sizeattribute to show multiple options- Using checkboxes instead for better mobile experience
- Implementing a JavaScript-enhanced multi-select component
resources/views/admin/tags/index.php (1)
30-34: Consider unobtrusive JavaScript for delete confirmation.The inline
onclickattribute for delete confirmation works but mixing JavaScript with HTML is not ideal. Consider moving to unobtrusive JavaScript with event delegation for better maintainability and Content Security Policy compatibility.src/Cms/Cli/Provider.php (1)
60-69: Namespace the non-CMS-prefixed commands
All of the user and maintenance commands are namespacedcms:*, butmail:generateandqueue:installare registered globally. That makes them easy to collide with other components on the same CLI (and breaks the mental model that everything coming from this provider begins withcms:). Please align these command names with the existing prefix—e.g.cms:mail:generateandcms:queue:install—and adjust any docs/usages accordingly.resources/views/admin/dashboard/index.php (1)
31-33: Consider escaping the formatted date output for consistency.While
DateTimeImmutable::format()output is generally safe, for consistency with the rest of the template that escapes all dynamic content, consider escaping the formatted date as well.Apply this diff:
<?php if($User->getLastLoginAt()): ?> - <p class="mb-0"><strong>Last Login:</strong> <?= $User->getLastLoginAt()->format('F j, Y g:i A') ?></p> + <p class="mb-0"><strong>Last Login:</strong> <?= htmlspecialchars($User->getLastLoginAt()->format('F j, Y g:i A')) ?></p> <?php endif; ?>resources/app/Initializers/AuthInitializer.php (1)
44-66: Consider validating required database configuration keys.The code checks if
$dbConfigis non-empty before initializing auth components, but doesn't validate that essential keys (likeadapter,name) are present. An incomplete configuration could cause theDatabaseUserRepositoryconstructor to fail with a less clear error message.Consider adding validation after the config loop:
} catch( \Exception $e ) { Log::error( "Failed to load database config: " . $e->getMessage() ); } + // Validate required database configuration keys + $requiredKeys = ['adapter', 'name']; + foreach( $requiredKeys as $key ) + { + if( !isset( $dbConfig[$key] ) || $dbConfig[$key] === null ) + { + Log::error( "Missing required database config key: {$key}" ); + return null; + } + } + // Only register auth filter if database is configured if( !empty( $dbConfig ) )
| <?php | ||
|
|
||
| require_once __DIR__ . '/vendor/autoload.php'; | ||
|
|
||
| use Neuron\Cms\Maintenance\MaintenanceManager; | ||
|
|
||
| $basePath = getcwd(); | ||
| $manager = new MaintenanceManager($basePath); | ||
|
|
||
| echo "Maintenance Status Check\n"; | ||
| echo "========================\n\n"; | ||
|
|
||
| echo "Base Path: $basePath\n"; | ||
| echo "Maintenance File: " . $basePath . "/.maintenance.json\n"; | ||
| echo "File Exists: " . (file_exists($basePath . '/.maintenance.json') ? 'Yes' : 'No') . "\n\n"; | ||
|
|
||
| if (file_exists($basePath . '/.maintenance.json')) { | ||
| echo "File Contents:\n"; | ||
| echo file_get_contents($basePath . '/.maintenance.json') . "\n\n"; | ||
| } | ||
|
|
||
| echo "Is Enabled: " . ($manager->isEnabled() ? 'Yes' : 'No') . "\n"; | ||
| echo "Message: " . $manager->getMessage() . "\n"; | ||
| echo "Retry After: " . $manager->getRetryAfter() . "\n\n"; | ||
|
|
||
| $status = $manager->getStatus(); | ||
| echo "Full Status:\n"; | ||
| print_r($status); |
There was a problem hiding this comment.
Debug script should not be in version control or deployed to production.
This debug script exposes maintenance configuration details and file paths, which could provide attackers with information about the application structure. It has no authentication or authorization checks.
Options:
-
Remove from version control - Add to
.gitignoreand delete:echo "debug-maintenance.php" >> .gitignore git rm debug-maintenance.php -
Move to a development-only location - Place in a
dev/orscripts/directory that's excluded from production deployments -
Add authentication - If this must be accessible, add authentication:
<?php // Only allow in development or with valid auth if( getenv('APP_ENV') === 'production' ) { http_response_code(404); exit('Not found'); }
Recommendation: Remove from version control as it's a temporary debugging tool.
🤖 Prompt for AI Agents
In debug-maintenance.php around lines 1 to 28, this debug script exposes
internal maintenance configuration and file paths without any authentication and
should not be in version control or deployed to production; remove the file from
the repo and add it to .gitignore (or move it to a dev-only scripts/ or dev/
directory that is excluded from production deployments), or if it must remain in
the tree wrap it with strict environment gating so it only runs in
non-production environments (e.g., bail out when APP_ENV=production) and add
authentication/authorization checks before printing any sensitive info.
|
|
||
| require '../vendor/autoload.php'; | ||
|
|
||
| error_reporting( E_ALL ); |
There was a problem hiding this comment.
Error reporting should be environment-aware.
Setting error_reporting(E_ALL) in the public entry point exposes detailed error information in production, which is a security risk that could reveal sensitive system details to attackers.
Make error reporting environment-aware:
-error_reporting( E_ALL );
+// Error reporting should be configured via environment or config
+// For production, errors should be logged, not displayed
+if( getenv('APP_ENV') === 'development' ) {
+ error_reporting( E_ALL );
+ ini_set('display_errors', '1');
+} else {
+ error_reporting( E_ALL );
+ ini_set('display_errors', '0');
+ ini_set('log_errors', '1');
+}Alternatively, move this configuration to the bootstrap/config layer where environment-specific settings are typically managed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error_reporting( E_ALL ); | |
| // Error reporting should be configured via environment or config | |
| // For production, errors should be logged, not displayed | |
| if( getenv('APP_ENV') === 'development' ) { | |
| error_reporting( E_ALL ); | |
| ini_set('display_errors', '1'); | |
| } else { | |
| error_reporting( E_ALL ); | |
| ini_set('display_errors', '0'); | |
| ini_set('log_errors', '1'); | |
| } |
🤖 Prompt for AI Agents
In resources/public/index.php around line 12, error_reporting(E_ALL) is set
unconditionally; change this to be environment-aware by either removing the call
here and delegating environment-specific error configuration to the
bootstrap/config layer, or replace it with a conditional that reads the runtime
environment (e.g., APP_ENV or similar) and sets error_reporting to E_ALL and
display_errors on for development, but to minimal reporting (and display_errors
off) for production; ensure production also suppresses display of errors while
still logging them.
|
|
||
| <div class="card"> | ||
| <div class="card-body"> | ||
| <form method="POST" action="/admin/categories/<?= $category->getId() ?>"> |
There was a problem hiding this comment.
XSS risk: Escape the category ID in the form action.
While category IDs are typically integers, it's best practice to escape all dynamic values in HTML attributes to prevent potential XSS.
Apply this diff:
- <form method="POST" action="/admin/categories/<?= $category->getId() ?>">
+ <form method="POST" action="/admin/categories/<?= htmlspecialchars( $category->getId() ) ?>">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <form method="POST" action="/admin/categories/<?= $category->getId() ?>"> | |
| <form method="POST" action="/admin/categories/<?= htmlspecialchars( $category->getId() ) ?>"> |
🤖 Prompt for AI Agents
In resources/views/admin/categories/edit.php around line 9 the form action uses
the raw dynamic value <?= $category->getId() ?> which risks XSS; update the
template to escape the ID when outputting into the HTML attribute by
casting/sanitizing the value and passing it through htmlspecialchars (e.g.
htmlspecialchars((string) $category->getId(), ENT_QUOTES, 'UTF-8')) so the
action becomes safe for inclusion in the attribute.
| <select class="form-select" id="categories" name="categories[]" multiple> | ||
| <?php if( isset( $categories ) ): ?> | ||
| <?php | ||
| $postCategoryIds = array_map( fn( $c ) => $c->getId(), $post->getCategories() ); |
There was a problem hiding this comment.
🧩 Analysis chain
Arrow functions require PHP 7.4+.
The code uses arrow function syntax (fn()) which requires PHP 7.4 or higher. Ensure this minimum version requirement is documented in the project's dependencies.
Also applies to: 66-66
🏁 Script executed:
#!/bin/bash
# Check composer.json for PHP version requirement
fd -e json composer.json --exec cat {} | jq -r '.require.php // "not specified"'Length of output: 13
I need to verify the code at the specified lines and check for PHP version requirements elsewhere in the project.
🏁 Script executed:
#!/bin/bash
# Check the actual content at lines 53 and 66 of the edit.php file
echo "=== Line 53 ==="
sed -n '53p' resources/views/admin/posts/edit.php
echo "=== Line 66 ==="
sed -n '66p' resources/views/admin/posts/edit.php
# Also check surrounding context for better understanding
echo "=== Context around line 53 (50-56) ==="
sed -n '50,56p' resources/views/admin/posts/edit.php
echo "=== Context around line 66 (63-69) ==="
sed -n '63,69p' resources/views/admin/posts/edit.php
# Search for PHP version specifications in other common files
echo "=== Checking .php-version ==="
[ -f .php-version ] && cat .php-version || echo "Not found"
echo "=== Checking for version in README ==="
grep -i "php.*version\|version.*php" README.md 2>/dev/null | head -5 || echo "Not found in README"
# Check if there are any other configuration files specifying version
echo "=== Checking .nvmrc or other version files ==="
ls -la | grep -E "version|\.php-version|\.nvmrc" || echo "No version files found"Length of output: 1357
Let me check the version files found in the project root:
🏁 Script executed:
#!/bin/bash
# Check .version.json for PHP version info
echo "=== Contents of .version.json ==="
cat .version.json
echo ""
echo "=== Contents of versionlog.md ==="
cat versionlog.md
# Also search through the entire project for any mention of PHP 7.4 or version requirements
echo ""
echo "=== Searching for PHP 7.4 or version requirements in project ==="
rg -i "php.*7\.|version.*requirement|require.*php" --type=php --type=md --type=json -C2 | head -30Length of output: 3279
Document PHP 7.4+ as minimum required version in composer.json.
This codebase uses arrow function syntax (fn()) at lines 53 and 66, which requires PHP 7.4 or higher. Add "php": "^7.4" to the require section in composer.json to enforce and document this dependency.
🤖 Prompt for AI Agents
In resources/views/admin/posts/edit.php around line 53 the code uses PHP 7.4
arrow function syntax (fn()) which requires PHP >=7.4; update composer.json by
adding "php": "^7.4" to the "require" section (and adjust existing php entry if
present), then run composer update or composer validate to ensure the manifest
reflects the minimum PHP version.
| <tr> | ||
| <td><?= $post->getId() ?></td> | ||
| <td><?= htmlspecialchars( $post->getTitle() ) ?></td> | ||
| <td><?= htmlspecialchars( $post->getAuthor() ) ?></td> |
There was a problem hiding this comment.
Critical: Fix author display - getAuthor() returns a User object.
getAuthor() returns a ?User object, not a string. This will cause an error or unexpected output when passed to htmlspecialchars().
Apply this diff to display the author's username:
- <td><?= htmlspecialchars( $post->getAuthor() ) ?></td>
+ <td><?= htmlspecialchars( $post->getAuthor()?->getUsername() ?? 'Unknown' ) ?></td>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <td><?= htmlspecialchars( $post->getAuthor() ) ?></td> | |
| <td><?= htmlspecialchars( $post->getAuthor()?->getUsername() ?? 'Unknown' ) ?></td> |
🤖 Prompt for AI Agents
In resources/views/admin/posts/index.php at line 27, the template calls
htmlspecialchars($post->getAuthor()) but getAuthor() returns a ?User object, not
a string; change the expression to use the author's username (or a safe
fallback) before escaping — e.g. retrieve $post->getAuthor()?->getUsername() (or
null-coalesce to a placeholder like '—') and pass that string into
htmlspecialchars so you never pass an object to htmlspecialchars and handle null
authors safely.
| private ?int $_Id = null; | ||
| private string $_Name; | ||
| private string $_Slug; | ||
| private ?DateTimeImmutable $_CreatedAt = null; | ||
| private ?DateTimeImmutable $_UpdatedAt = null; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->_CreatedAt = new DateTimeImmutable(); | ||
| } |
There was a problem hiding this comment.
Initialize required properties or make them nullable.
Properties $_Name and $_Slug are typed as non-nullable string but are not initialized in the constructor. Calling getName() or getSlug() on a freshly instantiated Tag (via new Tag()) before calling the setters will trigger a PHP error for accessing uninitialized typed properties.
Apply this diff to fix:
- private string $_Name;
- private string $_Slug;
+ private string $_Name = '';
+ private string $_Slug = '';Or alternatively, make them nullable if empty strings are not appropriate:
- private string $_Name;
- private string $_Slug;
+ private ?string $_Name = null;
+ private ?string $_Slug = null;Then update the getter return types accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private ?int $_Id = null; | |
| private string $_Name; | |
| private string $_Slug; | |
| private ?DateTimeImmutable $_CreatedAt = null; | |
| private ?DateTimeImmutable $_UpdatedAt = null; | |
| public function __construct() | |
| { | |
| $this->_CreatedAt = new DateTimeImmutable(); | |
| } | |
| private ?int $_Id = null; | |
| private string $_Name = ''; | |
| private string $_Slug = ''; | |
| private ?DateTimeImmutable $_CreatedAt = null; | |
| private ?DateTimeImmutable $_UpdatedAt = null; | |
| public function __construct() | |
| { | |
| $this->_CreatedAt = new DateTimeImmutable(); | |
| } |
🤖 Prompt for AI Agents
In src/Cms/Models/Tag.php around lines 15 to 24, the non-nullable properties
$_Name and $_Slug are declared but not initialized, causing uninitialized
property errors when a new Tag() is used before setters run; either initialize
them to sensible defaults (e.g., empty strings) in their declarations or in the
constructor, or change their types to ?string and set them to null, and if you
make them nullable update the getName() and getSlug() return types to ?string
(and adjust any callers accordingly).
| $stmt = $this->_PDO->query( | ||
| "SELECT c.*, COUNT(pc.post_id) as post_count | ||
| FROM categories c | ||
| LEFT JOIN post_categories pc ON c.id = pc.category_id | ||
| GROUP BY c.id | ||
| ORDER BY c.name ASC" | ||
| ); | ||
|
|
||
| $rows = $stmt->fetchAll(); | ||
|
|
||
| return array_map( function( $row ) { | ||
| $category = Category::fromArray( $row ); | ||
| return [ | ||
| 'category' => $category, | ||
| 'post_count' => (int)$row['post_count'] | ||
| ]; | ||
| }, $rows ); | ||
| } |
There was a problem hiding this comment.
Fix GROUP BY for PostgreSQL and strict MySQL
SELECT c.* ... GROUP BY c.id works on SQLite/MySQL without ONLY_FULL_GROUP_BY, but it fails on PostgreSQL and strict MySQL because every selected column must appear in the GROUP BY. Since this repository advertises multi-database support, the query will throw and break admin listings on PostgreSQL.
- $stmt = $this->_PDO->query(
- "SELECT c.*, COUNT(pc.post_id) as post_count
+ $stmt = $this->_PDO->query(
+ "SELECT
+ c.id,
+ c.name,
+ c.slug,
+ c.description,
+ c.created_at,
+ c.updated_at,
+ COUNT(pc.post_id) as post_count
FROM categories c
LEFT JOIN post_categories pc ON c.id = pc.category_id
- GROUP BY c.id
+ GROUP BY
+ c.id,
+ c.name,
+ c.slug,
+ c.description,
+ c.created_at,
+ c.updated_at
ORDER BY c.name ASC"
);Committable suggestion skipped: line range outside the PR's diff.
| public function template( string $templatePath, array $data = [] ): self | ||
| { | ||
| try | ||
| { | ||
| $view = new Html(); | ||
| $view->setViewPath( $this->_BasePath . '/resources/views' ); | ||
| $view->setPage( $templatePath ); | ||
|
|
||
| // Render the template | ||
| ob_start(); | ||
| foreach( $data as $key => $value ) | ||
| { | ||
| $$key = $value; | ||
| } | ||
| require $view->getViewPath() . '/' . $templatePath . '.php'; | ||
| $this->_Body = ob_get_clean(); | ||
| $this->_IsHtml = true; | ||
|
|
||
| return $this; | ||
| } | ||
| catch( \Exception $e ) | ||
| { | ||
| Log::error( "Email template error: " . $e->getMessage() ); | ||
| throw new \RuntimeException( "Failed to render email template: {$templatePath}" ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle missing or failing templates without crashing the request
template() calls require on whatever path was passed in, but there’s no existence check and the catch only traps Exception. If the template is missing—or it throws a TypeError/Error—PHP will fatally abort the request before we can log or surface a friendly message. Please guard the file lookup, catch \Throwable, and clean up the output buffer on failure.
@@ public function template( string $templatePath, array $data = [] ): self
- try
- {
- $view = new Html();
- $view->setViewPath( $this->_BasePath . '/resources/views' );
- $view->setPage( $templatePath );
-
- // Render the template
- ob_start();
- foreach( $data as $key => $value )
- {
- $$key = $value;
- }
- require $view->getViewPath() . '/' . $templatePath . '.php';
- $this->_Body = ob_get_clean();
- $this->_IsHtml = true;
-
- return $this;
- }
- catch( \Exception $e )
- {
- Log::error( "Email template error: " . $e->getMessage() );
- throw new \RuntimeException( "Failed to render email template: {$templatePath}" );
- }
+ $view = new Html();
+ $view->setViewPath( $this->_BasePath . '/resources/views' );
+ $view->setPage( $templatePath );
+
+ $templateFile = $view->getViewPath() . '/' . $templatePath . '.php';
+
+ if( !is_file( $templateFile ) )
+ {
+ throw new \RuntimeException( "Email template not found: {$templatePath}" );
+ }
+
+ try
+ {
+ ob_start();
+ foreach( $data as $key => $value )
+ {
+ $$key = $value;
+ }
+ require $templateFile;
+ $this->_Body = ob_get_clean();
+ $this->_IsHtml = true;
+
+ return $this;
+ }
+ catch( \Throwable $e )
+ {
+ if( ob_get_level() > 0 )
+ {
+ ob_end_clean();
+ }
+ Log::error( "Email template error: " . $e->getMessage() );
+ throw new \RuntimeException( "Failed to render email template: {$templatePath}", 0, $e );
+ }🤖 Prompt for AI Agents
In src/Cms/Services/EmailService.php around lines 102 to 127, the template()
method currently directly requires the template file and only catches
\Exception, causing fatal errors for missing files or PHP \Error/\TypeError and
leaving the output buffer uncleared; to fix, first build the full template
filename and check file_exists/is_readable before starting output buffering and
requiring it (return or throw a clear RuntimeException if not found), wrap the
rendering in a try block that catches \Throwable (not just \Exception), ensure
any started output buffer is cleaned (ob_get_level() and
ob_end_clean()/ob_clean()) on failure, log the throwable with its details, and
rethrow a RuntimeException that preserves the original throwable as previous (or
at least includes its message) so the request doesn't crash silently.
| if( $result ) | ||
| { | ||
| Log::info( "Email sent to: " . $this->_To[0]['email'] ); | ||
| } |
There was a problem hiding this comment.
Guard logging when no primary recipient is present
When we send an email with only CC/BCC recipients, $this->_To stays empty. Logging $this->_To[0]['email'] will then emit an “Undefined array key 0” warning. Please fall back gracefully.
@@
- if( $result )
- {
- Log::info( "Email sent to: " . $this->_To[0]['email'] );
- }
+ if( $result )
+ {
+ $primaryRecipient = $this->_To[0]['email'] ?? null;
+
+ if( $primaryRecipient )
+ {
+ Log::info( "Email sent to: " . $primaryRecipient );
+ }
+ else
+ {
+ Log::info( 'Email sent without a primary TO recipient' );
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if( $result ) | |
| { | |
| Log::info( "Email sent to: " . $this->_To[0]['email'] ); | |
| } | |
| if( $result ) | |
| { | |
| $primaryRecipient = $this->_To[0]['email'] ?? null; | |
| if( $primaryRecipient ) | |
| { | |
| Log::info( "Email sent to: " . $primaryRecipient ); | |
| } | |
| else | |
| { | |
| Log::info( 'Email sent without a primary TO recipient' ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Cms/Services/EmailService.php around lines 180 to 183, the log
unconditionally accesses $this->_To[0]['email'] which triggers an "Undefined
array key 0" when there is no primary recipient; update the code to guard that
access by checking if $this->_To is set and non-empty (e.g. isset($this->_To[0])
or count($this->_To) > 0) before reading index 0, and if empty fall back to a
safe alternative such as logging the first available address from CC/BCC arrays
or logging a concatenated list of recipients (or a message like "no primary
recipient; recipients: <list>") so no undefined index warning occurs.
| /** | ||
| * BlogTest - DEPRECATED | ||
| * | ||
| * This test file is deprecated and needs to be rewritten for the current | ||
| * Blog controller implementation which uses DatabasePostRepository, | ||
| * DatabaseCategoryRepository, and DatabaseTagRepository instead of the | ||
| * old Blahg library. | ||
| * | ||
| * The current implementation requires: | ||
| * - Settings in Registry with database configuration | ||
| * - Database repositories instead of Blahg\Repository | ||
| * - Post model instead of Blahg\Article | ||
| * | ||
| * All tests are currently skipped pending refactoring. | ||
| */ | ||
| class BlogTest extends TestCase | ||
| { | ||
| private $Router; | ||
| private $Repository; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
|
|
||
| // Create mock router | ||
| $this->Router = $this->createMock( Router::class ); | ||
|
|
||
| // Mock Repository | ||
| $this->Repository = $this->createMock( Repository::class ); | ||
| $this->markTestSkipped( | ||
| 'BlogTest is deprecated and needs to be rewritten for the current ' . | ||
| 'Blog controller implementation. The Blog controller now uses ' . | ||
| 'DatabasePostRepository, DatabaseCategoryRepository, and DatabaseTagRepository ' . | ||
| 'instead of the old Blahg library.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove deprecated tests or prioritize refactoring.
The entire test suite is marked as deprecated and skipped. Keeping dead test code adds maintenance burden and provides no value. Either:
- Refactor the tests to work with the new repository-based architecture (preferred)
- Remove the file entirely if tests are covered elsewhere
Based on the related file tests/Cms/BlogControllerTest.php (lines 27-72), it appears new tests exist for the current architecture. If BlogControllerTest provides equivalent coverage, this file should be deleted.
Do you want me to verify the test coverage overlap and recommend whether to delete or refactor this file?
| public function store( array $Parameters ): string | ||
| { | ||
| $User = Registry::getInstance()->get( 'Auth.User' ); | ||
| $SessionManager = new SessionManager(); | ||
| $SessionManager->start(); | ||
|
|
||
| if( !$User ) | ||
| { | ||
| throw new \RuntimeException( 'Authenticated user not found' ); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Get form data | ||
| $title = $_POST['title'] ?? ''; | ||
| $slug = $_POST['slug'] ?? ''; | ||
| $content = $_POST['content'] ?? ''; | ||
| $excerpt = $_POST['excerpt'] ?? ''; | ||
| $featuredImage = $_POST['featured_image'] ?? ''; | ||
| $status = $_POST['status'] ?? Post::STATUS_DRAFT; | ||
| $categoryIds = $_POST['categories'] ?? []; | ||
| $tagNames = $_POST['tags'] ?? ''; | ||
|
|
||
| // Create post | ||
| $Post = new Post(); | ||
| $Post->setTitle( $title ); | ||
| $Post->setSlug( $slug ?: $this->generateSlug( $title ) ); | ||
| $Post->setContent( $content ); | ||
| $Post->setExcerpt( $excerpt ); | ||
| $Post->setFeaturedImage( $featuredImage ); | ||
| $Post->setAuthor( $User->getUsername() ); | ||
| $Post->setStatus( $status ); | ||
| $Post->setCreatedAt( new DateTimeImmutable() ); | ||
|
|
||
| // Set published date if status is published | ||
| if( $status === Post::STATUS_PUBLISHED ) | ||
| { | ||
| $Post->setPublishedAt( new DateTimeImmutable() ); | ||
| } | ||
|
|
||
| // Load categories | ||
| $categories = []; | ||
| foreach( $categoryIds as $categoryId ) | ||
| { | ||
| $category = $this->_CategoryRepository->findById( (int)$categoryId ); | ||
| if( $category ) | ||
| { | ||
| $categories[] = $category; | ||
| } | ||
| } | ||
| $Post->setCategories( $categories ); | ||
|
|
||
| // Parse and create/load tags | ||
| $tags = []; | ||
| if( !empty( $tagNames ) ) | ||
| { | ||
| $tagArray = array_map( 'trim', explode( ',', $tagNames ) ); | ||
| foreach( $tagArray as $tagName ) | ||
| { | ||
| if( empty( $tagName ) ) continue; | ||
|
|
||
| $tag = $this->_TagRepository->findByName( $tagName ); | ||
| if( !$tag ) | ||
| { | ||
| $tag = new Tag(); | ||
| $tag->setName( $tagName ); | ||
| $tag->setSlug( $this->generateSlug( $tagName ) ); | ||
| $this->_TagRepository->create( $tag ); | ||
| } | ||
| $tags[] = $tag; | ||
| } | ||
| } | ||
| $Post->setTags( $tags ); | ||
|
|
||
| // Save post | ||
| $this->_PostRepository->create( $Post ); | ||
|
|
||
| $SessionManager->flash( 'success', 'Post created successfully' ); | ||
| header( 'Location: /admin/posts' ); | ||
| exit; | ||
| } | ||
| catch( \Exception $e ) | ||
| { | ||
| $SessionManager->flash( 'error', 'Failed to create post: ' . $e->getMessage() ); | ||
| header( 'Location: /admin/posts/create' ); | ||
| exit; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add CSRF validation before mutating state.
This handler trusts $_POST without checking the submitted CSRF token, so the endpoint is wide open to CSRF. Validate the token (and abort on failure) before reading the payload. Apply the same safeguard to the update/destroy actions.
$SessionManager = new SessionManager();
$SessionManager->start();
+ $CsrfManager = new CsrfTokenManager( $SessionManager );
+
+ if( !$CsrfManager->validate( $_POST['csrf_token'] ?? '' ) )
+ {
+ $SessionManager->flash( 'error', 'Invalid CSRF token' );
+ header( 'Location: /admin/posts/create' );
+ exit;
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 PHPMD (2.15.0)
136-136: Avoid unused parameters such as '$Parameters'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/Cms/Controllers/Admin/PostController.php around lines 136 to 223, add
CSRF validation before reading or using any $_POST values: retrieve the posted
token (e.g. $_POST['csrf_token'] or header), compare it to the expected token
stored in session (or use the app's CsrfTokenManager) and if validation fails
immediately flash an error, redirect and exit without mutating state; perform
this check at the top of this store() method (before extracting title/slug/etc.)
and apply the same guard to the update and destroy actions to abort on invalid
or missing tokens.
| $Post = new Post(); | ||
| $Post->setTitle( $title ); | ||
| $Post->setSlug( $slug ?: $this->generateSlug( $title ) ); | ||
| $Post->setContent( $content ); | ||
| $Post->setExcerpt( $excerpt ); | ||
| $Post->setFeaturedImage( $featuredImage ); | ||
| $Post->setAuthor( $User->getUsername() ); | ||
| $Post->setStatus( $status ); | ||
| $Post->setCreatedAt( new DateTimeImmutable() ); |
There was a problem hiding this comment.
Stop calling undefined setters and pass the correct author value.
Post::setContent() does not exist and this call fatals. Right after, setAuthor() receives a string, but its signature requires a User (or at least an ID via setAuthorId()), which currently throws a TypeError. Use the body setter and assign the author ID.
- $Post->setContent( $content );
+ $Post->setBody( $content );
…
- $Post->setAuthor( $User->getUsername() );
+ $Post->setAuthorId( (int)$User->getId() );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $Post = new Post(); | |
| $Post->setTitle( $title ); | |
| $Post->setSlug( $slug ?: $this->generateSlug( $title ) ); | |
| $Post->setContent( $content ); | |
| $Post->setExcerpt( $excerpt ); | |
| $Post->setFeaturedImage( $featuredImage ); | |
| $Post->setAuthor( $User->getUsername() ); | |
| $Post->setStatus( $status ); | |
| $Post->setCreatedAt( new DateTimeImmutable() ); | |
| $Post = new Post(); | |
| $Post->setTitle( $title ); | |
| $Post->setSlug( $slug ?: $this->generateSlug( $title ) ); | |
| $Post->setBody( $content ); | |
| $Post->setExcerpt( $excerpt ); | |
| $Post->setFeaturedImage( $featuredImage ); | |
| $Post->setAuthorId( (int)$User->getId() ); | |
| $Post->setStatus( $status ); | |
| $Post->setCreatedAt( new DateTimeImmutable() ); |
🤖 Prompt for AI Agents
In src/Cms/Controllers/Admin/PostController.php around lines 160 to 168, the
code calls undefined Post::setContent() and passes a username string into
setAuthor(), which expects a User (or an ID via setAuthorId()). Replace the
invalid call with the proper body setter (e.g. setBody(...) or the actual method
name used on Post for the post body) and assign the author by ID using
setAuthorId($User->getId()) (or pass the User object to setAuthor() if that is
the intended signature) so the correct types are used and the fatal TypeError is
avoided.
| public function update( array $Parameters ): string | ||
| { | ||
| $User = Registry::getInstance()->get( 'Auth.User' ); | ||
| $SessionManager = new SessionManager(); | ||
| $SessionManager->start(); | ||
|
|
||
| if( !$User ) | ||
| { | ||
| throw new \RuntimeException( 'Authenticated user not found' ); | ||
| } | ||
|
|
||
| $postId = (int)$Parameters['id']; | ||
| $Post = $this->_PostRepository->findById( $postId ); | ||
|
|
||
| if( !$Post ) | ||
| { | ||
| $SessionManager->flash( 'error', 'Post not found' ); | ||
| header( 'Location: /admin/posts' ); | ||
| exit; | ||
| } | ||
|
|
||
| // Check permissions | ||
| if( !$User->isAdmin() && !$User->isEditor() && $Post->getAuthor() !== $User->getUsername() ) | ||
| { | ||
| throw new \RuntimeException( 'Unauthorized to edit this post' ); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Get form data | ||
| $title = $_POST['title'] ?? ''; | ||
| $slug = $_POST['slug'] ?? ''; | ||
| $content = $_POST['content'] ?? ''; | ||
| $excerpt = $_POST['excerpt'] ?? ''; | ||
| $featuredImage = $_POST['featured_image'] ?? ''; | ||
| $status = $_POST['status'] ?? Post::STATUS_DRAFT; | ||
| $categoryIds = $_POST['categories'] ?? []; | ||
| $tagNames = $_POST['tags'] ?? ''; | ||
|
|
||
| // Update post | ||
| $Post->setTitle( $title ); | ||
| $Post->setSlug( $slug ?: $this->generateSlug( $title ) ); | ||
| $Post->setContent( $content ); | ||
| $Post->setExcerpt( $excerpt ); | ||
| $Post->setFeaturedImage( $featuredImage ); | ||
| $Post->setStatus( $status ); | ||
|
|
||
| // Set published date if status changed to published | ||
| if( $status === Post::STATUS_PUBLISHED && !$Post->getPublishedAt() ) | ||
| { | ||
| $Post->setPublishedAt( new DateTimeImmutable() ); | ||
| } | ||
|
|
||
| // Load categories | ||
| $categories = []; | ||
| foreach( $categoryIds as $categoryId ) | ||
| { | ||
| $category = $this->_CategoryRepository->findById( (int)$categoryId ); | ||
| if( $category ) | ||
| { | ||
| $categories[] = $category; | ||
| } | ||
| } | ||
| $Post->setCategories( $categories ); | ||
|
|
||
| // Parse and create/load tags | ||
| $tags = []; | ||
| if( !empty( $tagNames ) ) | ||
| { | ||
| $tagArray = array_map( 'trim', explode( ',', $tagNames ) ); | ||
| foreach( $tagArray as $tagName ) | ||
| { | ||
| if( empty( $tagName ) ) continue; | ||
|
|
||
| $tag = $this->_TagRepository->findByName( $tagName ); | ||
| if( !$tag ) | ||
| { | ||
| $tag = new Tag(); | ||
| $tag->setName( $tagName ); | ||
| $tag->setSlug( $this->generateSlug( $tagName ) ); | ||
| $this->_TagRepository->create( $tag ); | ||
| } | ||
| $tags[] = $tag; | ||
| } | ||
| } | ||
| $Post->setTags( $tags ); | ||
|
|
||
| // Save post | ||
| $this->_PostRepository->update( $Post ); | ||
|
|
||
| $SessionManager->flash( 'success', 'Post updated successfully' ); | ||
| header( 'Location: /admin/posts' ); | ||
| exit; | ||
| } | ||
| catch( \Exception $e ) | ||
| { | ||
| $SessionManager->flash( 'error', 'Failed to update post: ' . $e->getMessage() ); | ||
| header( 'Location: /admin/posts/' . $postId . '/edit' ); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Validate the CSRF token on updates (and mirror it for delete).
update() never checks the submitted token, so attackers can forge post edits. Add the same validation pattern as in store(), and ensure the delete action performs the identical check before mutating anything.
$SessionManager = new SessionManager();
$SessionManager->start();
+ $CsrfManager = new CsrfTokenManager( $SessionManager );
+
+ if( !$CsrfManager->validate( $_POST['csrf_token'] ?? '' ) )
+ {
+ $SessionManager->flash( 'error', 'Invalid CSRF token' );
+ header( 'Location: /admin/posts/' . $postId . '/edit' );
+ exit;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Cms/Controllers/Admin/PostController.php around lines 282 to 381, the
update() method does not validate the CSRF token before applying changes (and
delete routes are missing the same check); add the same CSRF validation pattern
used in store() at the start of update() (retrieve token from $_POST, validate
via the session/token service, flash an error and redirect back if invalid) and
implement identical validation for the delete action before any mutation; ensure
both paths abort execution after redirect to prevent the update/delete from
proceeding when the token is missing or invalid.
| // Update post | ||
| $Post->setTitle( $title ); | ||
| $Post->setSlug( $slug ?: $this->generateSlug( $title ) ); | ||
| $Post->setContent( $content ); | ||
| $Post->setExcerpt( $excerpt ); | ||
| $Post->setFeaturedImage( $featuredImage ); | ||
| $Post->setStatus( $status ); |
There was a problem hiding this comment.
Use the correct body setter during updates.
Post::setContent() still doesn't exist here, so editing a post will crash. Swap it for setBody().
- $Post->setContent( $content );
+ $Post->setBody( $content );🤖 Prompt for AI Agents
In src/Cms/Controllers/Admin/PostController.php around lines 321 to 327, the
update routine calls Post::setContent(), which doesn't exist and causes a crash
when editing; replace the setContent($content) call with setBody($content) (or
the appropriate existing setter name) so the post body is updated using the
correct method; ensure the variable passed remains $content and run tests to
confirm no other references to setContent remain.
| public function destroy( array $Parameters ): string | ||
| { | ||
| $User = Registry::getInstance()->get( 'Auth.User' ); | ||
| $SessionManager = new SessionManager(); | ||
| $SessionManager->start(); | ||
|
|
||
| if( !$User ) | ||
| { | ||
| throw new \RuntimeException( 'Authenticated user not found' ); | ||
| } | ||
|
|
||
| $postId = (int)$Parameters['id']; | ||
| $Post = $this->_PostRepository->findById( $postId ); | ||
|
|
||
| if( !$Post ) | ||
| { | ||
| $SessionManager->flash( 'error', 'Post not found' ); | ||
| header( 'Location: /admin/posts' ); | ||
| exit; | ||
| } | ||
|
|
||
| // Check permissions | ||
| if( !$User->isAdmin() && !$User->isEditor() && $Post->getAuthor() !== $User->getUsername() ) | ||
| { | ||
| $SessionManager->flash( 'error', 'Unauthorized to delete this post' ); | ||
| header( 'Location: /admin/posts' ); | ||
| exit; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| $this->_PostRepository->delete( $postId ); | ||
| $SessionManager->flash( 'success', 'Post deleted successfully' ); | ||
| } | ||
| catch( \Exception $e ) | ||
| { | ||
| $SessionManager->flash( 'error', 'Failed to delete post: ' . $e->getMessage() ); | ||
| } | ||
|
|
||
| header( 'Location: /admin/posts' ); | ||
| exit; |
There was a problem hiding this comment.
Protect the destroy action with CSRF checks.
destroy() processes the deletion with no CSRF guard, so a crafted request can wipe posts. Validate the token before calling the repository.
$SessionManager = new SessionManager();
$SessionManager->start();
+ $CsrfManager = new CsrfTokenManager( $SessionManager );
+
+ if( !$CsrfManager->validate( $_POST['csrf_token'] ?? '' ) )
+ {
+ $SessionManager->flash( 'error', 'Invalid CSRF token' );
+ header( 'Location: /admin/posts' );
+ exit;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Cms/Controllers/Admin/PostController.php around lines 387 to 427, the
destroy() action performs deletions without CSRF validation and without
enforcing an HTTP method; before any repository delete call, require that the
request is a POST (or other mutating method) and validate the CSRF token from
the request against the server-side token (e.g. session or Csrf service); if the
token is missing/invalid or method is not allowed, flash an error and redirect
without performing the delete. Ensure the token check occurs immediately after
starting the session and before permission checks or try/catch around the
delete.
Add auth, user management, migrations, maintenance mode and cli commands.
Summary by CodeRabbit
Release Notes
New Features
Improvements