fix(security): implement P2-052 through P2-057 fixes
- P2-052: Database SQL injection hardening - block stacked queries - P2-053: Add strict_types to DevController - P2-054: Fix temp file race condition in Servers component - P2-055: Add config validation to CopyDeviceFrames command - P2-056: Create developer config file - P2-057: Apply RequireHades middleware to Livewire routes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
bcb25a40d6
commit
6bf384a489
8 changed files with 302 additions and 14 deletions
174
TODO.md
Normal file
174
TODO.md
Normal file
|
|
@ -0,0 +1,174 @@
|
||||||
|
# TODO.md - core-developer
|
||||||
|
|
||||||
|
Last reviewed: 2026-01-29
|
||||||
|
|
||||||
|
## P1 - Critical / Security
|
||||||
|
|
||||||
|
### Completed
|
||||||
|
- [x] **Server model has no migration** - FIXED: Migration created at `src/Migrations/0001_01_01_000001_create_developer_tables.php`
|
||||||
|
- [x] **Inconsistent Hades authorization in DevController** - FIXED: Now uses `$user->isHades()` method
|
||||||
|
- [x] **SetHadesCookie uses env() directly** - FIXED: Now uses `config('developer.hades_token')`
|
||||||
|
- [x] **HorizonServiceProvider gate is empty** - FIXED: `viewHorizon` gate now checks `$user->isHades()`
|
||||||
|
- [x] **TelescopeServiceProvider gate emails empty** - FIXED: Telescope gate now checks `$user->isHades()`
|
||||||
|
|
||||||
|
### Open
|
||||||
|
|
||||||
|
_No open P1 items._
|
||||||
|
|
||||||
|
### Recently Fixed (Jan 2026)
|
||||||
|
|
||||||
|
- [x] **Database component SQL injection hardening** - FIXED: `isReadOnlyQuery()` now blocks stacked queries by checking for semicolons followed by non-whitespace content using regex `/;\s*\S/`.
|
||||||
|
- File: `src/View/Modal/Admin/Database.php`
|
||||||
|
|
||||||
|
- [x] **DevController missing strict types declaration** - FIXED: Added `declare(strict_types=1);` at top of file.
|
||||||
|
- File: `src/Controllers/DevController.php`
|
||||||
|
|
||||||
|
- [x] **Servers component writes private key to temp file** - FIXED: Now uses `tempnam()` for atomic file creation and sets 0600 permissions before writing sensitive data.
|
||||||
|
- File: `src/View/Modal/Admin/Servers.php`
|
||||||
|
|
||||||
|
- [x] **CopyDeviceFrames command lacks config validation** - FIXED: Added validation for config existence and required keys (source_path, public_path, devices) with proper error messages.
|
||||||
|
- File: `src/Console/Commands/CopyDeviceFrames.php`
|
||||||
|
|
||||||
|
## P2 - High Priority
|
||||||
|
|
||||||
|
### Completed
|
||||||
|
- [x] **Unify authorization pattern** - Created `RequireHades` middleware for consistent authorization
|
||||||
|
- [x] **Add rate limiting to API routes** - Added rate limiters in `Boot.php` for all endpoints
|
||||||
|
- [x] **Log clear action should be audited** - `clearLogs()` now logs with user_id, email, previous_size, IP
|
||||||
|
- [x] **Remove duplicate log reading logic** - Created `LogReaderService` used by both DevController and Logs component
|
||||||
|
- [x] **RemoteServerManager timeout is hardcoded** - Added configurable timeouts via config
|
||||||
|
|
||||||
|
### Open
|
||||||
|
|
||||||
|
- [ ] **Tests pass without Hades tier set** - Tests in `DevToolsBasic.php` create a user without Hades tier but tests pass, suggesting authorization may not be enforced correctly in test environment
|
||||||
|
- File: `src/Tests/UseCase/DevToolsBasic.php`
|
||||||
|
- Acceptance: Tests should fail when user is not Hades; add trait/helper to set Hades status in tests
|
||||||
|
|
||||||
|
- [ ] **Clear logs button has no confirmation** - Unlike cache management, the logs clear button executes immediately without confirmation
|
||||||
|
- File: `src/View/Modal/Admin/Logs.php` and corresponding blade
|
||||||
|
- Acceptance: Add confirmation modal similar to Cache component
|
||||||
|
|
||||||
|
- [ ] **Activity log component shows all workspaces** - ActivityLog queries all Activity records without workspace scoping
|
||||||
|
- File: `src/View/Modal/Admin/ActivityLog.php`
|
||||||
|
- Acceptance: Either scope to current workspace or document that this is intentional for Hades users
|
||||||
|
|
||||||
|
### Recently Fixed (Jan 2026)
|
||||||
|
|
||||||
|
- [x] **Missing developer config file** - FIXED: Config file exists at `src/config.php` with hades_token, ssh.connection_timeout, ssh.command_timeout, and horizon notification settings. Published via `mergeConfigFrom()` in Boot.php.
|
||||||
|
|
||||||
|
- [x] **Livewire pages have no route middleware** - FIXED: RequireHades middleware applied to the `/hub/dev/*` route group in `src/Routes/admin.php` line 15. Authorization now enforced at route level.
|
||||||
|
|
||||||
|
## P3 - Medium Priority
|
||||||
|
|
||||||
|
### Completed
|
||||||
|
- [x] **Multi-log file support** - Added `getAvailableLogFiles()` and `getCurrentLogPath()` to LogReaderService
|
||||||
|
- [x] **Command registration** - CopyDeviceFrames now registered via `onConsole()` handler
|
||||||
|
|
||||||
|
### Open
|
||||||
|
|
||||||
|
- [ ] **Server model missing table name specification** - Relies on Laravel convention; should explicitly set `$table = 'servers'`
|
||||||
|
- File: `src/Models/Server.php`
|
||||||
|
- Acceptance: Add protected `$table` property
|
||||||
|
|
||||||
|
- [ ] **LogReaderService redaction patterns need review** - IP redaction pattern may miss IPv6 addresses
|
||||||
|
- File: `src/Services/LogReaderService.php` line 42
|
||||||
|
- Acceptance: Add IPv6 support or document limitation
|
||||||
|
|
||||||
|
- [ ] **RouteTestService environment check is permissive** - `isTestingAllowed()` returns true for 'testing' environment which could be used in CI
|
||||||
|
- File: `src/Services/RouteTestService.php` line 47
|
||||||
|
- Acceptance: Consider adding config flag to explicitly enable route testing
|
||||||
|
|
||||||
|
- [ ] **Database query tool lacks export functionality** - Users can view results but cannot download/export them
|
||||||
|
- Acceptance: Add CSV/JSON export button for query results
|
||||||
|
|
||||||
|
- [ ] **Route inspector history not persisted** - History is lost on page refresh
|
||||||
|
- File: `src/View/Modal/Admin/RouteInspector.php`
|
||||||
|
- Acceptance: Consider storing history in session or localStorage
|
||||||
|
|
||||||
|
- [ ] **Missing translations for some UI elements** - Servers, Database, and ActivityLog pages have hardcoded English strings instead of using translation keys
|
||||||
|
- Files: Multiple blade files and components
|
||||||
|
- Acceptance: Add translation keys to `src/Lang/en_GB/developer.php` and use them consistently
|
||||||
|
|
||||||
|
## P4 - Low Priority / Improvements
|
||||||
|
|
||||||
|
### Open
|
||||||
|
|
||||||
|
- [ ] **DevController has redundant authorize() calls** - The `routes()` and `session()` methods call `$this->authorize()` but API routes already have `RequireHades` middleware
|
||||||
|
- File: `src/Controllers/DevController.php`
|
||||||
|
- Acceptance: Remove redundant authorization checks or document the defence-in-depth approach
|
||||||
|
|
||||||
|
- [ ] **LogReaderService could use generators** - For very large log files, using generators instead of arrays would reduce memory usage
|
||||||
|
- File: `src/Services/LogReaderService.php`
|
||||||
|
- Acceptance: Refactor `readLogEntries()` to optionally yield entries
|
||||||
|
|
||||||
|
- [ ] **RouteTestResult getFormattedResponseTime has edge case** - Times under 1ms are converted to microseconds incorrectly (multiplied by 1000 instead of keeping as sub-millisecond)
|
||||||
|
- File: `src/Data/RouteTestResult.php` line 93
|
||||||
|
- Acceptance: Fix calculation or clarify the intended behaviour
|
||||||
|
|
||||||
|
- [ ] **Server status enum should be a proper PHP enum** - Currently uses string values ('pending', 'connected', 'failed')
|
||||||
|
- File: `src/Models/Server.php`
|
||||||
|
- Acceptance: Create `ServerStatus` backed enum and use it consistently
|
||||||
|
|
||||||
|
- [ ] **ApplyIconSettings middleware has hardcoded defaults** - Default values should come from config
|
||||||
|
- File: `src/Middleware/ApplyIconSettings.php`
|
||||||
|
- Acceptance: Move defaults to config file
|
||||||
|
|
||||||
|
- [ ] **Pulse dashboard override lacks documentation** - The custom Pulse view is registered but not documented
|
||||||
|
- File: `src/View/Blade/vendor/pulse/dashboard.blade.php`
|
||||||
|
- Acceptance: Add comment in Boot.php explaining the override purpose
|
||||||
|
|
||||||
|
## P5 - Nice to Have / Future
|
||||||
|
|
||||||
|
### From code-review.md (documented features)
|
||||||
|
- [ ] **Server CRUD UI improvements** - Add bulk actions, SSH key validation, connection health checks
|
||||||
|
- [ ] **Log download/export** - FIXED: `downloadLogs()` added, but could add format options (JSON, filtered export)
|
||||||
|
- [ ] **Event log viewer** - Activity logs exist on Server model but no dedicated UI to view activity per model
|
||||||
|
|
||||||
|
### New Ideas
|
||||||
|
- [ ] **Add log search functionality** - Currently only filter by level, add full-text search within log messages
|
||||||
|
- [ ] **Database saved queries** - Allow saving frequently used queries with names
|
||||||
|
- [ ] **Route documentation viewer** - Parse DocBlocks from controllers and display in route inspector
|
||||||
|
- [ ] **SSH terminal emulator** - Interactive terminal for connected servers (complex, security considerations)
|
||||||
|
- [ ] **Cache statistics dashboard** - Show cache hit/miss rates, memory usage, key counts
|
||||||
|
- [ ] **Config diff viewer** - Compare current config values against defaults
|
||||||
|
|
||||||
|
## P6+ - Backlog / Someday
|
||||||
|
|
||||||
|
- [ ] **Real-time log streaming** - WebSocket/SSE for live log tailing
|
||||||
|
- [ ] **Query explain plan visualisation** - Parse and display EXPLAIN output graphically
|
||||||
|
- [ ] **Route performance profiling** - Track response times over time, identify slow routes
|
||||||
|
- [ ] **Deployment integration** - Trigger deployments from server management UI
|
||||||
|
- [ ] **Multi-database support** - Query tool for multiple database connections
|
||||||
|
- [ ] **Scheduled task monitoring** - View and manage Laravel scheduled tasks
|
||||||
|
|
||||||
|
## Test Coverage Gaps
|
||||||
|
|
||||||
|
### Currently Tested
|
||||||
|
- Logs page renders with correct sections and translations
|
||||||
|
- Routes page renders with table headers
|
||||||
|
- Cache page renders with all cache action cards
|
||||||
|
|
||||||
|
### Missing Tests
|
||||||
|
- [ ] DevController API endpoints (logs, routes, session, clear)
|
||||||
|
- [ ] Cache clearing actually executes and clears caches
|
||||||
|
- [ ] Log filtering by level
|
||||||
|
- [ ] Route filtering/searching
|
||||||
|
- [ ] Hades authorization enforcement (both allow and deny cases)
|
||||||
|
- [ ] RemoteServerManager SSH operations (mock phpseclib)
|
||||||
|
- [ ] Server model scopes and methods
|
||||||
|
- [ ] SetHadesCookie listener
|
||||||
|
- [ ] ApplyIconSettings middleware
|
||||||
|
- [ ] CopyDeviceFrames command
|
||||||
|
- [ ] LogReaderService redaction patterns
|
||||||
|
- [ ] RouteTestService route testing logic
|
||||||
|
- [ ] Database component query execution and validation
|
||||||
|
- [ ] ActivityLog filtering and pagination
|
||||||
|
- [ ] RouteInspector request building and execution
|
||||||
|
|
||||||
|
## Notes
|
||||||
|
|
||||||
|
- All Hades-only features require the user's `isHades()` method to return true
|
||||||
|
- The module depends on `host-uk/core` and `host-uk/core-admin`
|
||||||
|
- UK English spellings must be used (colour, organisation, centre)
|
||||||
|
- All PHP files should have `declare(strict_types=1);`
|
||||||
|
- Testing uses Pest syntax, not PHPUnit
|
||||||
|
|
@ -31,6 +31,7 @@ class Boot extends ServiceProvider implements AdminMenuProvider
|
||||||
public function boot(): void
|
public function boot(): void
|
||||||
{
|
{
|
||||||
$this->loadTranslationsFrom(__DIR__.'/Lang', 'developer');
|
$this->loadTranslationsFrom(__DIR__.'/Lang', 'developer');
|
||||||
|
$this->mergeConfigFrom(__DIR__.'/config.php', 'developer');
|
||||||
|
|
||||||
app(AdminMenuRegistry::class)->register($this);
|
app(AdminMenuRegistry::class)->register($this);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,7 @@
|
||||||
<?php
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
namespace Core\Developer\Console\Commands;
|
namespace Core\Developer\Console\Commands;
|
||||||
|
|
||||||
use Illuminate\Console\Command;
|
use Illuminate\Console\Command;
|
||||||
|
|
@ -14,6 +16,28 @@ class CopyDeviceFrames extends Command
|
||||||
public function handle(): int
|
public function handle(): int
|
||||||
{
|
{
|
||||||
$config = config('device-frames');
|
$config = config('device-frames');
|
||||||
|
|
||||||
|
// Validate config exists and has required keys
|
||||||
|
if (! is_array($config)) {
|
||||||
|
$this->error('The device-frames configuration is not defined. Please publish the config file.');
|
||||||
|
|
||||||
|
return Command::FAILURE;
|
||||||
|
}
|
||||||
|
|
||||||
|
$requiredKeys = ['source_path', 'public_path', 'devices'];
|
||||||
|
$missingKeys = array_diff($requiredKeys, array_keys($config));
|
||||||
|
if (! empty($missingKeys)) {
|
||||||
|
$this->error('Missing required config keys: '.implode(', ', $missingKeys));
|
||||||
|
|
||||||
|
return Command::FAILURE;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (! is_array($config['devices']) || empty($config['devices'])) {
|
||||||
|
$this->error('The device-frames.devices configuration must be a non-empty array.');
|
||||||
|
|
||||||
|
return Command::FAILURE;
|
||||||
|
}
|
||||||
|
|
||||||
$sourcePath = $config['source_path'];
|
$sourcePath = $config['source_path'];
|
||||||
$publicPath = public_path($config['public_path']);
|
$publicPath = public_path($config['public_path']);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,7 @@
|
||||||
<?php
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
namespace Core\Developer\Controllers;
|
namespace Core\Developer\Controllers;
|
||||||
|
|
||||||
use Core\Front\Controller;
|
use Core\Front\Controller;
|
||||||
|
|
|
||||||
|
|
@ -9,16 +9,19 @@ use Illuminate\Support\Facades\Route;
|
||||||
*/
|
*/
|
||||||
|
|
||||||
Route::prefix('hub')->name('hub.')->group(function () {
|
Route::prefix('hub')->name('hub.')->group(function () {
|
||||||
// Developer tools (Hades only) - authorization checked in components
|
// Developer tools (Hades only) - authorization enforced via middleware
|
||||||
Route::prefix('dev')->name('dev.')->group(function () {
|
Route::prefix('dev')
|
||||||
Route::get('/logs', \Core\Developer\View\Modal\Admin\Logs::class)->name('logs');
|
->name('dev.')
|
||||||
Route::get('/routes', \Core\Developer\View\Modal\Admin\Routes::class)->name('routes');
|
->middleware(\Core\Developer\Middleware\RequireHades::class)
|
||||||
Route::get('/cache', \Core\Developer\View\Modal\Admin\Cache::class)->name('cache');
|
->group(function () {
|
||||||
Route::get('/activity', \Core\Developer\View\Modal\Admin\ActivityLog::class)->name('activity');
|
Route::get('/logs', \Core\Developer\View\Modal\Admin\Logs::class)->name('logs');
|
||||||
Route::get('/servers', \Core\Developer\View\Modal\Admin\Servers::class)->name('servers');
|
Route::get('/routes', \Core\Developer\View\Modal\Admin\Routes::class)->name('routes');
|
||||||
Route::get('/database', \Core\Developer\View\Modal\Admin\Database::class)->name('database');
|
Route::get('/cache', \Core\Developer\View\Modal\Admin\Cache::class)->name('cache');
|
||||||
Route::get('/route-inspector', \Core\Developer\View\Modal\Admin\RouteInspector::class)->name('route-inspector');
|
Route::get('/activity', \Core\Developer\View\Modal\Admin\ActivityLog::class)->name('activity');
|
||||||
});
|
Route::get('/servers', \Core\Developer\View\Modal\Admin\Servers::class)->name('servers');
|
||||||
|
Route::get('/database', \Core\Developer\View\Modal\Admin\Database::class)->name('database');
|
||||||
|
Route::get('/route-inspector', \Core\Developer\View\Modal\Admin\RouteInspector::class)->name('route-inspector');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
/*
|
/*
|
||||||
|
|
|
||||||
|
|
@ -136,7 +136,17 @@ class Database extends Component
|
||||||
// Get first word of query
|
// Get first word of query
|
||||||
$firstWord = strtoupper(strtok($query, ' '));
|
$firstWord = strtoupper(strtok($query, ' '));
|
||||||
|
|
||||||
return in_array($firstWord, self::ALLOWED_STATEMENTS, true);
|
if (! in_array($firstWord, self::ALLOWED_STATEMENTS, true)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Block stacked queries (e.g., "SELECT 1; DROP TABLE users")
|
||||||
|
// Check for semicolon followed by any non-whitespace content
|
||||||
|
if (preg_match('/;\s*\S/', $query)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
private function checkHadesAccess(): void
|
private function checkHadesAccess(): void
|
||||||
|
|
|
||||||
|
|
@ -165,10 +165,16 @@ class Servers extends Component
|
||||||
}
|
}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
// Create a temporary key file
|
// Create a unique temporary key file with secure permissions
|
||||||
$tempKeyPath = sys_get_temp_dir().'/ssh_test_'.uniqid();
|
// Using tempnam() for atomic file creation to prevent race conditions
|
||||||
file_put_contents($tempKeyPath, $server->getDecryptedPrivateKey());
|
$tempKeyPath = tempnam(sys_get_temp_dir(), 'ssh_key_');
|
||||||
|
if ($tempKeyPath === false) {
|
||||||
|
throw new \RuntimeException('Failed to create temporary key file');
|
||||||
|
}
|
||||||
|
|
||||||
|
// Set restrictive permissions before writing sensitive data
|
||||||
chmod($tempKeyPath, 0600);
|
chmod($tempKeyPath, 0600);
|
||||||
|
file_put_contents($tempKeyPath, $server->getDecryptedPrivateKey());
|
||||||
|
|
||||||
// Test SSH connection with a simple echo command
|
// Test SSH connection with a simple echo command
|
||||||
$result = Process::timeout(15)->run([
|
$result = Process::timeout(15)->run([
|
||||||
|
|
|
||||||
68
src/config.php
Normal file
68
src/config.php
Normal file
|
|
@ -0,0 +1,68 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Developer Tools Module Configuration
|
||||||
|
*
|
||||||
|
* Settings for the developer tools, SSH connections, and Horizon notifications.
|
||||||
|
*/
|
||||||
|
|
||||||
|
return [
|
||||||
|
|
||||||
|
/*
|
||||||
|
|--------------------------------------------------------------------------
|
||||||
|
| Hades (God-Mode) Token
|
||||||
|
|--------------------------------------------------------------------------
|
||||||
|
|
|
||||||
|
| The token used to enable Hades (god-mode) access for developers.
|
||||||
|
| This token is stored in an encrypted cookie when a Hades user logs in.
|
||||||
|
| Changing this token will revoke access for all existing Hades sessions.
|
||||||
|
|
|
||||||
|
*/
|
||||||
|
|
||||||
|
'hades_token' => env('HADES_TOKEN'),
|
||||||
|
|
||||||
|
/*
|
||||||
|
|--------------------------------------------------------------------------
|
||||||
|
| SSH Configuration
|
||||||
|
|--------------------------------------------------------------------------
|
||||||
|
|
|
||||||
|
| Settings for SSH connections to remote servers via the RemoteServerManager
|
||||||
|
| trait. These timeouts help prevent hanging connections and runaway commands.
|
||||||
|
|
|
||||||
|
*/
|
||||||
|
|
||||||
|
'ssh' => [
|
||||||
|
// Connection timeout in seconds (time to establish SSH connection)
|
||||||
|
'connection_timeout' => env('DEVELOPER_SSH_CONNECTION_TIMEOUT', 30),
|
||||||
|
|
||||||
|
// Command timeout in seconds (max time for individual commands)
|
||||||
|
'command_timeout' => env('DEVELOPER_SSH_COMMAND_TIMEOUT', 60),
|
||||||
|
],
|
||||||
|
|
||||||
|
/*
|
||||||
|
|--------------------------------------------------------------------------
|
||||||
|
| Horizon Notifications
|
||||||
|
|--------------------------------------------------------------------------
|
||||||
|
|
|
||||||
|
| Configure notification channels for Laravel Horizon alerts.
|
||||||
|
| These notifications are sent when long wait times or job failures occur.
|
||||||
|
|
|
||||||
|
*/
|
||||||
|
|
||||||
|
'horizon' => [
|
||||||
|
// SMS notification recipient (phone number)
|
||||||
|
'sms_to' => env('HORIZON_SMS_TO'),
|
||||||
|
|
||||||
|
// Email notification recipient
|
||||||
|
'mail_to' => env('HORIZON_MAIL_TO'),
|
||||||
|
|
||||||
|
// Slack webhook URL for notifications
|
||||||
|
'slack_webhook' => env('HORIZON_SLACK_WEBHOOK'),
|
||||||
|
|
||||||
|
// Slack channel for notifications (default: #alerts)
|
||||||
|
'slack_channel' => env('HORIZON_SLACK_CHANNEL', '#alerts'),
|
||||||
|
],
|
||||||
|
|
||||||
|
];
|
||||||
Loading…
Add table
Reference in a new issue