diff --git a/backend/app/Controllers/AuthController.php b/backend/app/Controllers/AuthController.php index d69a3ef..c59e759 100644 --- a/backend/app/Controllers/AuthController.php +++ b/backend/app/Controllers/AuthController.php @@ -39,13 +39,13 @@ class AuthController extends BaseController try { // Create Company $companyId = Company::create([ - 'name' => htmlspecialchars(strip_tags($data['company_name'])) + 'name' => $data['company_name'] ]); // Create Admin User for this Company $userId = User::createSecure([ 'company_id' => $companyId, - 'name' => htmlspecialchars(strip_tags($data['user_name'])), + 'name' => $data['user_name'], 'email' => strtolower(trim($data['email'])), 'password' => $data['password'], 'role' => 'admin' @@ -127,7 +127,7 @@ class AuthController extends BaseController { $user = User::find($request->user_id); - if (!$user) { + if (!$user || (int)$user['company_id'] !== (int)$request->company_id) { $response->json(['error' => 'User not found'], 404); return; } diff --git a/backend/app/Core/Request.php b/backend/app/Core/Request.php index 5f4a54e..a04d998 100644 --- a/backend/app/Core/Request.php +++ b/backend/app/Core/Request.php @@ -13,6 +13,11 @@ class Request private array $bodyParams; private array $headers; + // Explicit properties to store authentication details to avoid deprecation warnings in PHP 8.2+ + public ?int $user_id = null; + public ?int $company_id = null; + public ?string $role = null; + public function __construct() { $this->method = strtoupper($_SERVER['REQUEST_METHOD'] ?? 'GET'); diff --git a/backend/app/Core/Response.php b/backend/app/Core/Response.php index ddeac34..af6c41d 100644 --- a/backend/app/Core/Response.php +++ b/backend/app/Core/Response.php @@ -39,10 +39,12 @@ class Response $this->setStatusCode($code); $this->setHeader('Content-Type', 'application/json; charset=utf-8'); - // Setup base CORS headers for our API - $this->setHeader('Access-Control-Allow-Origin', '*'); + // Setup CORS headers — restrict origin to the configured allowed domain + $allowedOrigin = getenv('ALLOWED_ORIGIN') ?: '*'; + $this->setHeader('Access-Control-Allow-Origin', $allowedOrigin); $this->setHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS'); $this->setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-Requested-With'); + $this->setHeader('Vary', 'Origin'); // Required when Access-Control-Allow-Origin is not * $this->sendHeaders(); http_response_code($this->statusCode); diff --git a/backend/app/Core/Router.php b/backend/app/Core/Router.php index 400e182..62e1188 100644 --- a/backend/app/Core/Router.php +++ b/backend/app/Core/Router.php @@ -75,9 +75,11 @@ class Router // Handle CORS Preflight Preemptively if ($method === 'OPTIONS') { - $response->setHeader('Access-Control-Allow-Origin', '*'); + $allowedOrigin = getenv('ALLOWED_ORIGIN') ?: '*'; + $response->setHeader('Access-Control-Allow-Origin', $allowedOrigin); $response->setHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, OPTIONS'); $response->setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization, X-Requested-With'); + $response->setHeader('Vary', 'Origin'); $response->setStatusCode(200); exit; } @@ -116,12 +118,14 @@ class Router return; } - $response->error("Handler error for route: {$path}", 500); + error_log("Handler error for route: [{$method}] {$path}"); + $response->error("Internal Server Error", 500); return; } } // Route not found - $response->error("Route not found: [{$method}] {$path}", 404); + error_log("Route not found: [{$method}] {$path}"); + $response->error("Not Found", 404); } } diff --git a/backend/app/Core/Security.php b/backend/app/Core/Security.php index 7d42521..0cffaa2 100644 --- a/backend/app/Core/Security.php +++ b/backend/app/Core/Security.php @@ -14,7 +14,10 @@ class Security */ private static function getEncryptionKey(): string { - $key = getenv('ENCRYPTION_KEY') ; + $key = getenv('ENCRYPTION_KEY'); + if (!$key || strlen($key) < 16) { + throw new \RuntimeException("ENCRYPTION_KEY environment variable is empty or too short. Cryptographic operations aborted."); + } return substr(hash('sha256', $key, true), 0, 32); } @@ -23,7 +26,11 @@ class Security */ private static function getHmacSalt(): string { - return getenv('HMAC_SALT'); + $salt = getenv('HMAC_SALT'); + if (!$salt) { + throw new \RuntimeException("HMAC_SALT environment variable is empty. Cryptographic operations aborted."); + } + return $salt; } /** @@ -31,7 +38,11 @@ class Security */ private static function getJwtSecret(): string { - return getenv('JWT_SECRET'); + $secret = getenv('JWT_SECRET'); + if (!$secret) { + throw new \RuntimeException("JWT_SECRET environment variable is empty. Cryptographic operations aborted."); + } + return $secret; } /** diff --git a/backend/app/Middlewares/RateLimitMiddleware.php b/backend/app/Middlewares/RateLimitMiddleware.php new file mode 100644 index 0000000..521cdb4 --- /dev/null +++ b/backend/app/Middlewares/RateLimitMiddleware.php @@ -0,0 +1,94 @@ +maxAttempts = $maxAttempts; + $this->decaySeconds = $decaySeconds; + } + + public function handle(Request $request, Response $response): void + { + $ip = $this->getClientIp(); + $key = 'rate_' . md5($ip . '_' . $request->getPath()); + + $storageDir = APP_ROOT . '/storage/rate_limits'; + if (!is_dir($storageDir)) { + mkdir($storageDir, 0750, true); + } + + $filePath = $storageDir . '/' . $key . '.json'; + + $data = ['count' => 0, 'expires_at' => time() + $this->decaySeconds]; + + if (file_exists($filePath)) { + $raw = json_decode(file_get_contents($filePath), true); + if ($raw && isset($raw['expires_at']) && $raw['expires_at'] > time()) { + // Window still active — use existing data + $data = $raw; + } + // If window expired, fall through and reset (overwrite with fresh data below) + } + + $data['count']++; + + if ($data['count'] > $this->maxAttempts) { + $retryAfter = max(0, $data['expires_at'] - time()); + $response->setHeader('Retry-After', (string)$retryAfter); + $response->json([ + 'error' => 'Too Many Requests', + 'message' => "You have exceeded the maximum number of {$this->maxAttempts} attempts. Please try again in {$retryAfter} seconds." + ], 429); + return; + } + + // Persist the updated counter + file_put_contents($filePath, json_encode($data), LOCK_EX); + } + + /** + * Get real client IP, accounting for proxies + */ + private function getClientIp(): string + { + $headers = [ + 'HTTP_CF_CONNECTING_IP', // Cloudflare + 'HTTP_X_FORWARDED_FOR', + 'HTTP_X_REAL_IP', + 'REMOTE_ADDR' + ]; + + foreach ($headers as $header) { + if (!empty($_SERVER[$header])) { + // X-Forwarded-For can be a comma-separated list; take first + $ip = trim(explode(',', $_SERVER[$header])[0]); + if (filter_var($ip, FILTER_VALIDATE_IP)) { + return $ip; + } + } + } + + return '0.0.0.0'; + } +} diff --git a/backend/app/Middlewares/SecurityMiddleware.php b/backend/app/Middlewares/SecurityMiddleware.php index 251e30d..978daa6 100644 --- a/backend/app/Middlewares/SecurityMiddleware.php +++ b/backend/app/Middlewares/SecurityMiddleware.php @@ -33,7 +33,7 @@ class SecurityMiddleware } /** - * Recursively sanitize input arrays + * Recursively trim input arrays */ private function sanitizeArray(array $data): array { @@ -42,8 +42,7 @@ class SecurityMiddleware if (is_array($value)) { $sanitized[$key] = $this->sanitizeArray($value); } elseif (is_string($value)) { - // Strip HTML tags and convert special characters to HTML entities - $sanitized[$key] = htmlspecialchars(strip_tags(trim($value)), ENT_QUOTES, 'UTF-8'); + $sanitized[$key] = trim($value); } else { $sanitized[$key] = $value; } diff --git a/backend/app/Models/BaseModel.php b/backend/app/Models/BaseModel.php index 8f65cb7..bcbdb03 100644 --- a/backend/app/Models/BaseModel.php +++ b/backend/app/Models/BaseModel.php @@ -12,12 +12,42 @@ abstract class BaseModel protected static string $table = ''; protected static string $primaryKey = 'id'; + /** + * Validate table, primary key, and column names to prevent SQL injection. + */ + protected static function getSafeTable(): string + { + $table = static::$table; + if (!preg_match('/^[a-zA-Z0-9_]+$/', $table)) { + throw new \InvalidArgumentException("Invalid table name: {$table}"); + } + return $table; + } + + protected static function getSafePrimaryKey(): string + { + $primaryKey = static::$primaryKey; + if (!preg_match('/^[a-zA-Z0-9_]+$/', $primaryKey)) { + throw new \InvalidArgumentException("Invalid primary key: {$primaryKey}"); + } + return $primaryKey; + } + + protected static function validateColumns(array $columns): void + { + foreach ($columns as $column) { + if (!preg_match('/^[a-zA-Z0-9_]+$/', $column)) { + throw new \InvalidArgumentException("Invalid column name: {$column}"); + } + } + } + /** * Retrieve all records */ public static function all(): array { - $table = static::$table; + $table = static::getSafeTable(); return Database::select("SELECT * FROM {$table}"); } @@ -29,22 +59,27 @@ abstract class BaseModel */ public static function find($id): ?array { - $table = static::$table; - $primaryKey = static::$primaryKey; + // Enforce integer conversion or validate to prevent type misuse + $id = is_numeric($id) ? (int)$id : $id; + $table = static::getSafeTable(); + $primaryKey = static::getSafePrimaryKey(); return Database::selectOne("SELECT * FROM {$table} WHERE {$primaryKey} = :id LIMIT 1", ['id' => $id]); } /** * Insert a new record * - * @param array $data Assocation array of columns and values + * @param array $data Association array of columns and values * @return string Last inserted primary key ID */ public static function create(array $data): string { - $table = static::$table; - $columns = implode(', ', array_keys($data)); - $placeholders = ':' . implode(', :', array_keys($data)); + $table = static::getSafeTable(); + $columnKeys = array_keys($data); + static::validateColumns($columnKeys); + + $columns = implode(', ', $columnKeys); + $placeholders = ':' . implode(', :', $columnKeys); $sql = "INSERT INTO {$table} ({$columns}) VALUES ({$placeholders})"; return Database::insert($sql, $data); @@ -59,11 +94,15 @@ abstract class BaseModel */ public static function update($id, array $data): int { - $table = static::$table; - $primaryKey = static::$primaryKey; + $id = is_numeric($id) ? (int)$id : $id; + $table = static::getSafeTable(); + $primaryKey = static::getSafePrimaryKey(); + + $columnKeys = array_keys($data); + static::validateColumns($columnKeys); $sets = []; - foreach (array_keys($data) as $column) { + foreach ($columnKeys as $column) { $sets[] = "{$column} = :{$column}"; } $setSql = implode(', ', $sets); @@ -81,8 +120,9 @@ abstract class BaseModel */ public static function delete($id): int { - $table = static::$table; - $primaryKey = static::$primaryKey; + $id = is_numeric($id) ? (int)$id : $id; + $table = static::getSafeTable(); + $primaryKey = static::getSafePrimaryKey(); $sql = "DELETE FROM {$table} WHERE {$primaryKey} = :id"; return Database::execute($sql, ['id' => $id]); diff --git a/backend/app/bootstrap.php b/backend/app/bootstrap.php index d3797f7..d47d669 100644 --- a/backend/app/bootstrap.php +++ b/backend/app/bootstrap.php @@ -55,3 +55,32 @@ if ($isDebug) { ini_set('display_errors', '0'); error_reporting(0); } + +// 4. Global Uncaught Exception Handler +// Catches any unhandled exception anywhere in the app and returns a clean JSON error +// instead of leaking PHP stack traces to the browser. +set_exception_handler(function (\Throwable $e) { + $isDebug = filter_var(getenv('APP_DEBUG') ?: true, FILTER_VALIDATE_BOOLEAN); + + error_log('[EXCEPTION] ' . get_class($e) . ': ' . $e->getMessage() . ' in ' . $e->getFile() . ':' . $e->getLine()); + + if (!headers_sent()) { + header('Content-Type: application/json; charset=utf-8'); + http_response_code(500); + } + + $body = ['error' => 'Internal Server Error']; + + // In debug mode, expose details to the developer only + if ($isDebug) { + $body['debug'] = [ + 'exception' => get_class($e), + 'message' => $e->getMessage(), + 'file' => $e->getFile(), + 'line' => $e->getLine(), + ]; + } + + echo json_encode($body, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT); + exit(1); +}); diff --git a/backend/public/index.php b/backend/public/index.php index f496c9f..a259813 100644 --- a/backend/public/index.php +++ b/backend/public/index.php @@ -20,23 +20,20 @@ $router = new Router(); $router->use(\App\Middlewares\SecurityMiddleware::class); // 4. Define API Routes +// Health Check — no php_version or environment in production to avoid info disclosure $router->get('/api/health', function ($request, $response) { $response->json([ - 'status' => 'success', - 'message' => 'Nabeh API is healthy', - 'details' => [ - 'app_name' => getenv('APP_NAME') ?: 'Nabeh', - 'environment' => getenv('APP_ENV') ?: 'development', - 'php_version' => PHP_VERSION, - 'time' => date('Y-m-d H:i:s') - ] + 'status' => 'success', + 'message' => 'Nabeh API is healthy', + 'app_name' => getenv('APP_NAME') ?: 'Nabeh', + 'time' => date('Y-m-d H:i:s') ]); }); -// Authentication Routes -$router->post('/api/auth/register', [\App\Controllers\AuthController::class, 'register']); -$router->post('/api/auth/login', [\App\Controllers\AuthController::class, 'login']); -$router->get('/api/auth/me', [\App\Controllers\AuthController::class, 'me'], [\App\Middlewares\AuthMiddleware::class]); +// Authentication Routes (Rate-limited: 5 attempts per 60 seconds per IP) +$router->post('/api/auth/register', [\App\Controllers\AuthController::class, 'register'], [\App\Middlewares\RateLimitMiddleware::class]); +$router->post('/api/auth/login', [\App\Controllers\AuthController::class, 'login'], [\App\Middlewares\RateLimitMiddleware::class]); +$router->get('/api/auth/me', [\App\Controllers\AuthController::class, 'me'], [\App\Middlewares\AuthMiddleware::class]); // 4. Dispatch the request diff --git a/implementation_plan.md b/implementation_plan.md index 6f530c6..c1f6bc9 100644 --- a/implementation_plan.md +++ b/implementation_plan.md @@ -251,4 +251,48 @@ server { > - هل ترغب بإنشاء مسار `POST /api/auth/register` مفتوح للجميع لإنشاء حسابات شركات جديدة؟ أم نكتفي حالياً بإنشاء مستخدم مدير (Admin) افتراضي يدوياً أو عبر سكربت ليكون النظام مغلقاً للشركات المعتمدة فقط؟ > - في عملية تسجيل الدخول، هل نحتاج لإرجاع بيانات الشركة المرتبطة بالمستخدم ضمن نفس الـ Response، أم نكتفي بإرجاع التوكن (Token) وبيانات المستخدم الأساسية فقط؟ +--- + +## المرحلة الخامسة: معالجة وإصلاح الثغرات الأمنية (Security Audit Remediation) + +بناءً على التقرير الأمني الصادر، سنقوم بتطبيق التعديلات البرمجية لرفع مستوى أمان الخادم وحماية البيانات. + +### Proposed Changes + +#### [MODIFY] [Security.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Core/Security.php) +- إرجاع خطأ (Exception) فوراً في حال عدم وجود متغيرات البيئة (`ENCRYPTION_KEY`, `HMAC_SALT`, `JWT_SECRET`) لمنع تشفير البيانات بمفاتيح فارغة. + +#### [MODIFY] [BaseModel.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Models/BaseModel.php) +- تنظيف وفلترة أسماء الأعمدة ديناميكياً باستخدام مصفوفة بيضاء (Whitelist Regex) لمنع ثغرات الـ SQL Injection عبر أسماء الأعمدة في دالتي `create()` و `update()`. + +#### [MODIFY] [SecurityMiddleware.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Middlewares/SecurityMiddleware.php) +- إلغاء تطبيق `htmlspecialchars` و `strip_tags` على المدخلات الخام القادمة للخلفية لمنع تلف كلمات المرور والرموز الخاصة، وتأجيل التنظيف ليكون حصراً عند الطباعة/العرض (Output Encoding). + +#### [MODIFY] [AuthController.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Controllers/AuthController.php) +- إزالة التنظيف المزدوج (Double Encoding) لأسماء المستخدمين والشركات. + +#### [MODIFY] [Router.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Core/Router.php) +- إخفاء مسار الملفات الحقيقي من رسائل الخطأ 404 و 500 وإظهار رسائل عامة لمنع تسريب بنية المجلدات (Information Disclosure). + +#### [MODIFY] [Response.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Core/Response.php) +- جعل رابط الـ CORS ديناميكياً يعتمد على متغير بيئة `ALLOWED_ORIGIN` بدلاً من فتح النطاق للجميع عبر `*`. + +#### [MODIFY] [bootstrap.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/bootstrap.php) +- تسجيل معالج أخطاء عام (`set_exception_handler` / `set_error_handler`) لتحويل الأخطاء الفادحة غير المعالجة إلى استجابات JSON نظيفة ومخفية في الإنتاج، مع تسجيل التفاصيل التقنية في الـ `error_log` فقط. + +#### [MODIFY] [Request.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Core/Request.php) +- تعريف الخصائص المعرفية (`$user_id`, `$company_id`, `$role`) بشكل صريح لتفادي مشاكل الخصائص الديناميكية (Dynamic Properties Deprecation) في إصدارات PHP 8.2+. + +#### [NEW] [RateLimitMiddleware.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/app/Middlewares/RateLimitMiddleware.php) +- بناء فلتر مخصص للحد من معدل الطلبات (Rate Limiting) على مسارات تسجيل الدخول والتسجيل لحماية التطبيق من محاولات التخمين العنيف (Brute Force). + +#### [MODIFY] [index.php](file:///Users/hamzaaleghwairyeen/development/App/nabeh/backend/public/index.php) +- تفعيل فلتر الحد من معدل الطلبات على مسارات الحماية. + +### User Review Required + +> [!IMPORTANT] +> هل نعتمد نظام تخزين المحاولات للحد من معدل الطلبات (Rate Limiting) في ملفات مؤقتة داخل مجلد `storage/` محلي (سهل الإعداد ومستقل)، أم نستخدم جدولاً مخصصاً في قاعدة البيانات؟ + +