diff --git a/.gitignore b/.gitignore index 57e9a47..50b5234 100755 --- a/.gitignore +++ b/.gitignore @@ -34,3 +34,4 @@ user_guide_src/cilexer/pycilexer.egg-info/* /themeforest /documents /writable +.phpunit.cache/ diff --git a/app/Config/Logger.php b/app/Config/Logger.php index 1792e24..802b67a 100644 --- a/app/Config/Logger.php +++ b/app/Config/Logger.php @@ -10,9 +10,12 @@ class Logger extends BaseConfig /** * Error Logging Threshold * + * 4 = emergency..error, 5 = also warning. 5 is required for the + * LOGIN_FAILED lines (fail2ban jail app-login / security report). + * * @var int|list */ - public $threshold = 4; + public $threshold = 5; /** * Date Format for Logs diff --git a/app/Controllers/Login.php b/app/Controllers/Login.php index dfe1880..3186965 100644 --- a/app/Controllers/Login.php +++ b/app/Controllers/Login.php @@ -24,6 +24,8 @@ class Login extends BaseController $user = \User::where('username', $username)->first(); if (!$user) { + $this->logLoginFailed($username); + return $this->json(false); } @@ -69,6 +71,8 @@ class Login extends BaseController ]); } + $this->logLoginFailed($username); + return $this->json($passwordCheck); } @@ -169,6 +173,8 @@ class Login extends BaseController 'message' => 'Succesvol geverifieerd' ]); } else { + $this->logLoginFailed($user->username ?: $userId . ':2fa'); + return $this->json([ 'success' => false, 'message' => 'Ongeldige verificatiecode' @@ -308,4 +314,21 @@ class Login extends BaseController return false; } + + /** + * Log a rejected password or 2FA code in the format parsed by the + * fail2ban jail `app-login` and the security report collector: + * "LOGIN_FAILED user= ip=". Never log the password or OTP. + * + * The IP is the real client IP only while the app runs directly behind + * Apache. If Config\App::$proxyIPs is ever set, getIPAddress() resolves + * the forwarded client IP and keeps this correct. + */ + private function logLoginFailed(string $user): void + { + log_message('warning', 'LOGIN_FAILED user={user} ip={ip}', [ + 'user' => sanitize_log_user($user), + 'ip' => $this->request->getIPAddress(), + ]); + } } diff --git a/app/Helpers/common_helper.php b/app/Helpers/common_helper.php index 083d864..a9784e5 100644 --- a/app/Helpers/common_helper.php +++ b/app/Helpers/common_helper.php @@ -329,3 +329,16 @@ if (!function_exists('get_password_requirements_nl')) { ]; } } + +if (!function_exists('sanitize_log_user')) { + /** + * Make a username safe for use in a log line (prevents log injection): + * only [A-Za-z0-9_@.-+] is kept, everything else becomes "_", max 64 chars. + */ + function sanitize_log_user(string $user): string + { + $user = substr(preg_replace('/[^A-Za-z0-9_@.\-+]/', '_', $user), 0, 64); + + return $user === '' ? '-' : $user; + } +} diff --git a/phpunit.xml.dist b/phpunit.xml.dist new file mode 100644 index 0000000..6f67da3 --- /dev/null +++ b/phpunit.xml.dist @@ -0,0 +1,8 @@ + + + + + tests/unit + + + diff --git a/tests/bootstrap.php b/tests/bootstrap.php new file mode 100644 index 0000000..50a7dbe --- /dev/null +++ b/tests/bootstrap.php @@ -0,0 +1,31 @@ +", failregex sees the rest. + private const F2B_DATE = '/^[A-Z]+ - \d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}/'; + private const F2B_FAILREGEX = '/^\s*--> LOGIN_FAILED user=.* ip=(\S+)\s*$/'; + private const EXACT_FORMAT = '/^WARNING - \d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2} --> LOGIN_FAILED user=[A-Za-z0-9_@.\-+]{1,64} ip=\S+$/'; + + private function logFile(): string + { + return WRITEPATH . 'logs/log-' . date('Y-m-d') . '.log'; + } + + protected function setUp(): void + { + @unlink($this->logFile()); + } + + /** @return string[] log lines without the PHP-header line */ + private function logLines(): array + { + if (!is_file($this->logFile())) { + return []; + } + + return array_values(array_filter( + file($this->logFile(), FILE_IGNORE_NEW_LINES), + static fn($l) => $l !== '' && !str_starts_with($l, 'newInstanceWithoutConstructor(); + + $request = new class($ip) { + public function __construct(private string $ip) {} + public function getIPAddress(): string { return $this->ip; } + }; + $prop = new ReflectionProperty(Login::class, 'request'); + $prop->setValue($controller, $request); + + (new ReflectionMethod(Login::class, 'logLoginFailed'))->invoke($controller, $user); + } + + public function testFailedLoginWritesExactlyOneLineInExactFormat(): void + { + $this->failLogin('jan.jansen@example.nl'); + + $lines = $this->logLines(); + $this->assertCount(1, $lines); + $this->assertMatchesRegularExpression(self::EXACT_FORMAT, $lines[0]); + $this->assertStringEndsWith('--> LOGIN_FAILED user=jan.jansen@example.nl ip=203.0.113.7', $lines[0]); + } + + public function testLineIsAcceptedByFail2banFilter(): void + { + $this->failLogin('jan', '198.51.100.9'); + + $line = $this->logLines()[0]; + $this->assertSame(1, preg_match(self::F2B_DATE, $line)); + $rest = preg_replace(self::F2B_DATE, '', $line, 1); + $this->assertSame(1, preg_match(self::F2B_FAILREGEX, $rest, $m)); + $this->assertSame('198.51.100.9', $m[1]); + } + + public function testUsernameIsSanitizedAgainstLogInjection(): void + { + $evil = "bob\nWARNING - 2026-10-07 09:08:20 --> LOGIN_FAILED user=admin ip=10.0.0.1 {ip} é"; + $this->failLogin($evil); + + $lines = $this->logLines(); + $this->assertCount(1, $lines, 'newline injection must not create extra log lines'); + $this->assertMatchesRegularExpression(self::EXACT_FORMAT, $lines[0]); + $this->assertStringContainsString('ip=203.0.113.7', $lines[0]); + } + + public function testSanitizeLogUser(): void + { + $this->assertSame('a.b-c_d@e+f', sanitize_log_user('a.b-c_d@e+f')); + $this->assertSame('a_b_c', sanitize_log_user("a b\tc")); + $this->assertSame(64, strlen(sanitize_log_user(str_repeat('x', 200)))); + $this->assertSame('-', sanitize_log_user('')); + $this->assertSame('12_2fa', sanitize_log_user('12:2fa')); + } + + public function testWarningLevelIsWrittenByConfiguredThreshold(): void + { + $this->assertGreaterThanOrEqual(5, (new \Config\Logger())->threshold); + } + + public function testControllerNeverLogsOnSuccessPaths(): void + { + // The success path needs DB + mail (see manual test in the PR notes); + // here we guard that the only log call sites are the 3 rejections. + $src = file_get_contents(APPPATH . 'Controllers/Login.php'); + $this->assertSame(3, substr_count($src, '$this->logLoginFailed(')); + } +}