marek-miklewicz/mxautologin
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address security review findings

Browse Source
This commit is contained in:
Marek Miklewicz
2026-06-30 11:41:40 +02:00
parent 3b096b67bb
commit 54b2e620ff
12 changed files with 188 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
exec/handlers/login.php
+4 -2
View File
@@ -28,7 +28,8 @@ return static function (): void {
$clientIp = ClientIp::resolve($_SERVER, $settings);
$auditBase['client_ip'] = $clientIp;
$correlation = KeyName::build($ctx->serverName(), $ctx->username(), time());
$createdAt = time();
$correlation = KeyName::build($ctx->serverName(), $ctx->username(), $createdAt);
$auditBase['correlation'] = $correlation;
$limiter = new RateLimiter(Settings::STATE_DIR . '/rate-limit');
@@ -43,7 +44,8 @@ return static function (): void {
$settings->loginKeyTtl(),
$settings->userIpBound(),
$clientIp,
$redirectPath
$redirectPath,
$createdAt
);
try {
exec/lib/DirectAdminContext.php
+19 -2
View File
@@ -16,11 +16,11 @@ final class DirectAdminContext
*/
public static function fromServer(array $server, Settings $settings): self
{
$username = trim((string)($server['USERNAME'] ?? $server['USER'] ?? ''));
$username = trim((string)($server['USERNAME'] ?? ''));
if (!preg_match('/^[A-Za-z0-9][A-Za-z0-9._-]{0,31}$/', $username)) {
throw new InvalidArgumentException('Invalid DirectAdmin username');
}
$host = Settings::normalizeHost((string)($server['HTTP_HOST'] ?? $server['SERVER_NAME'] ?? ''));
$host = self::trustedSourceHost($server, $settings);
if ($host === '') {
throw new InvalidArgumentException('Missing source host');
}
@@ -34,6 +34,23 @@ final class DirectAdminContext
return new self($username, $host, $serverName, strtoupper((string)($server['REQUEST_METHOD'] ?? 'GET')));
}
/**
* @param array<string,mixed> $server
*/
private static function trustedSourceHost(array $server, Settings $settings): string
{
$configuredName = $settings->sourceServerName();
if ($configuredName !== '') {
foreach ($settings->allowedSourceHosts() as $allowedHost) {
$label = $settings->sanitizeServerName(explode('.', $allowedHost)[0] ?? '');
if ($label === $configuredName) {
return $allowedHost;
}
}
}
return Settings::normalizeHost((string)($server['SERVER_NAME'] ?? ''));
}
public function username(): string
{
return $this->username;
exec/lib/RateLimiter.php
+18 -4
View File
@@ -16,10 +16,18 @@ final class RateLimiter
mkdir($this->dir, 0700, true);
}
$file = $this->dir . '/' . preg_replace('/[^A-Za-z0-9._-]/', '_', $bucket) . '.json';
$handle = fopen($file, 'c+');
if ($handle === false) {
throw new RuntimeException('Cannot open rate limit bucket');
}
if (!flock($handle, LOCK_EX)) {
fclose($handle);
throw new RuntimeException('Cannot lock rate limit bucket');
}
$now = time();
$events = [];
if (is_file($file)) {
$decoded = json_decode((string)file_get_contents($file), true);
$contents = stream_get_contents($handle);
$decoded = json_decode(is_string($contents) ? $contents : '', true);
if (is_array($decoded)) {
foreach ($decoded as $event) {
if (is_int($event) && $event > $now - $windowSeconds) {
@@ -27,12 +35,18 @@ final class RateLimiter
}
}
}
}
if (count($events) >= $limit) {
flock($handle, LOCK_UN);
fclose($handle);
throw new RuntimeException('Rate limit exceeded');
}
$events[] = $now;
file_put_contents($file, json_encode($events), LOCK_EX);
ftruncate($handle, 0);
rewind($handle);
fwrite($handle, json_encode($events) ?: '[]');
fflush($handle);
flock($handle, LOCK_UN);
fclose($handle);
@chmod($file, 0600);
}
}
exec/lib/RemoteLoginUrlClient.php
+3 -1
View File
@@ -10,7 +10,8 @@ final class LoginUrlRequest
public readonly int $ttl,
public readonly bool $userIpBound,
public readonly string $clientIp,
public readonly string $redirectPath
public readonly string $redirectPath,
public readonly int $createdAt
) {
}
}
@@ -66,6 +67,7 @@ final class RemoteLoginUrlClient
$request->userIpBound ? '1' : '0',
$request->clientIp,
$request->redirectPath,
(string)$request->createdAt,
];
}
exec/lib/Settings.php
+31 -1
View File
@@ -153,7 +153,7 @@ final class Settings
*/
public function trustedProxies(): array
{
return $this->csvHosts('MAIL_LOGIN_TRUSTED_PROXIES');
return $this->csvNetworks('MAIL_LOGIN_TRUSTED_PROXIES');
}
public function sshHost(): string
@@ -226,6 +226,36 @@ final class Settings
return array_values(array_unique($out));
}
/**
* @return list<string>
*/
private function csvNetworks(string $key): array
{
$raw = $this->getString($key, '');
if ($raw === '') {
return [];
}
$out = [];
foreach (explode(',', $raw) as $item) {
$item = trim($item);
if ($item === '') {
continue;
}
if (str_contains($item, '/')) {
[$ip, $prefix] = explode('/', $item, 2);
if (filter_var($ip, FILTER_VALIDATE_IP) && preg_match('/^[0-9]+$/', $prefix)) {
$out[] = $ip . '/' . $prefix;
}
} else {
$host = self::normalizeHost($item);
if ($host !== '') {
$out[] = $host;
}
}
}
return array_values(array_unique($out));
}
private function validate(): void
{
$ttlRaw = $this->getString('LOGIN_KEY_TTL', '');
mx-helper/cleanup-expired-login-urls.php
+14 -1
View File
@@ -21,7 +21,7 @@ function mailLoginCleanupMain(): void
exit(2);
}
$base = rtrim($config['base_url'], '/');
$base = mailLoginCleanupValidateBaseUrl($config['base_url']);
$now = time();
$states = mailLoginCleanupLoadStates($config['state_dir']);
$entries = mailLoginCleanupApiRequest('GET', $base . '/api/login-keys/urls', $config);
@@ -76,6 +76,9 @@ function mailLoginCleanupShouldDelete(array $entry, array $states, int $now): bo
return false;
}
foreach ($states as $state) {
if ((string)($state['id'] ?? '') === '' || (string)$entry['id'] !== (string)$state['id']) {
continue;
}
if (($state['status'] ?? '') !== 'created') {
continue;
}
@@ -100,6 +103,16 @@ function mailLoginCleanupShouldDelete(array $entry, array $states, int $now): bo
return false;
}
function mailLoginCleanupValidateBaseUrl(string $baseUrl): string
{
$baseUrl = rtrim($baseUrl, '/');
$parts = parse_url($baseUrl);
if (!is_array($parts) || strtolower((string)($parts['scheme'] ?? '')) !== 'https' || empty($parts['host'])) {
throw new InvalidArgumentException('DA_BASE_URL must be an https URL');
}
return $baseUrl;
}
/**
* @param array<string,string> $config
* @return mixed
mx-helper/mail-login-authorized-command.sh
+8 -5
View File
@@ -16,8 +16,8 @@ audit() {
dir=$(dirname "$AUDIT_LOG")
mkdir -p "$dir"
chmod 700 "$dir"
printf '{"timestamp":"%s","event":"%s","result":"%s","correlation":"%s","username":"%s","source_host":"%s","client_ip":"%s"}\n' \
"$(date -u '+%Y-%m-%dT%H:%M:%SZ')" "$1" "$2" "$3" "$4" "$5" "$6" >> "$AUDIT_LOG"
php -r '$fields=["timestamp"=>gmdate("c"),"event"=>$argv[1],"result"=>$argv[2],"correlation"=>$argv[3],"username"=>$argv[4],"source_host"=>$argv[5],"client_ip"=>$argv[6]]; echo json_encode($fields, JSON_UNESCAPED_SLASHES)."\n";' \
"$1" "$2" "$3" "$4" "$5" "$6" >> "$AUDIT_LOG"
chmod 600 "$AUDIT_LOG"
}
@@ -66,6 +66,7 @@ TTL="${5:-}"
USER_IP_BOUND="${6:-}"
CLIENT_IP="${7:-}"
REDIRECT_PATH="${8:-}"
TIMESTAMP_ARG="${9:-}"
[ "$COMMAND" = "mail-login-create-url" ] || fail
printf '%s' "$USERNAME_ARG" | grep -Eq '^[A-Za-z0-9][A-Za-z0-9._-]{0,31}$' || fail
@@ -87,10 +88,11 @@ case "$REDIRECT_PATH" in
//*|*\\*) fail ;;
esac
printf '%s' "$REDIRECT_PATH" | grep -Eq '^/[A-Za-z0-9._~%/?=+-]*$' || fail
printf '%s' "$TIMESTAMP_ARG" | grep -Eq '^[0-9]+$' || fail
SERVER_CLEAN=$(clean_label "$SERVER_NAME")
USER_CLEAN=$(clean_label "$USERNAME_ARG")
TIMESTAMP=$(date '+%s')
TIMESTAMP="$TIMESTAMP_ARG"
CORRELATION="autologin-$SERVER_CLEAN-$USER_CLEAN-$TIMESTAMP"
if [ ${#CORRELATION} -gt 96 ]; then
@@ -116,9 +118,10 @@ case "$URL" in
https://*/api/login/url?key=*) ;;
*) fail ;;
esac
LOGIN_URL_ID=$(printf '%s\n' "$OUTPUT" | awk 'match($0,/HASHURL[A-Z0-9]+/) {print substr($0,RSTART,RLENGTH); exit}')
printf '{"timestamp":%s,"expires":%s,"correlation":"%s","username":"%s","source_host":"%s","client_ip":"%s","ttl":%s,"ip_bound":%s,"redirect_path":"%s","status":"created"}\n' \
"$TIMESTAMP" "$((TIMESTAMP + TTL))" "$CORRELATION" "$USERNAME_ARG" "$SOURCE_HOST" "$CLIENT_IP" "$TTL" "$USER_IP_BOUND" "$REDIRECT_PATH" > "$STATE_DIR/$CORRELATION.json"
php -r '$fields=["timestamp"=>(int)$argv[1],"expires"=>(int)$argv[2],"correlation"=>$argv[3],"id"=>$argv[4],"username"=>$argv[5],"source_host"=>$argv[6],"client_ip"=>$argv[7],"ttl"=>(int)$argv[8],"ip_bound"=>(int)$argv[9],"redirect_path"=>$argv[10],"status"=>"created"]; echo json_encode($fields, JSON_UNESCAPED_SLASHES)."\n";' \
"$TIMESTAMP" "$((TIMESTAMP + TTL))" "$CORRELATION" "$LOGIN_URL_ID" "$USERNAME_ARG" "$SOURCE_HOST" "$CLIENT_IP" "$TTL" "$USER_IP_BOUND" "$REDIRECT_PATH" > "$STATE_DIR/$CORRELATION.json"
chmod 600 "$STATE_DIR/$CORRELATION.json"
audit "create_login_url" "success" "$CORRELATION" "$USERNAME_ARG" "$SOURCE_HOST" "$CLIENT_IP"
printf 'URL: %s\n' "$URL"
tests/cleanup_test.php
+13
View File
@@ -8,6 +8,7 @@ test('cleanup deletes only expired login urls matching local mail-login state',
'timestamp' => 1782810000,
'expires' => 1782810030,
'correlation' => 'autologin-h4-mxtest-1782810000',
'id' => 'HASHURLMATCH',
'redirect_path' => '/',
'client_ip' => '198.51.100.10',
'ip_bound' => 1,
@@ -31,5 +32,17 @@ test('cleanup deletes only expired login urls matching local mail-login state',
assert_true(mailLoginCleanupShouldDelete($matching, [$state], strtotime('2026-06-30T10:01:00Z')));
assert_false(mailLoginCleanupShouldDelete($wrongRedirect, [$state], strtotime('2026-06-30T10:01:00Z')));
assert_false(mailLoginCleanupShouldDelete($notExpired, [$state], strtotime('2026-06-30T10:01:00Z')));
$stateWithoutId = $state;
unset($stateWithoutId['id']);
assert_false(mailLoginCleanupShouldDelete($matching, [$stateWithoutId], strtotime('2026-06-30T10:01:00Z')));
});
test('cleanup rejects non-https directadmin api base url', function (): void {
try {
mailLoginCleanupValidateBaseUrl('http://127.0.0.1:2222');
throw new RuntimeException('Expected http base URL to fail');
} catch (InvalidArgumentException $e) {
assert_contains('https', strtolower($e->getMessage()));
}
});
tests/context_test.php
+42 -1
View File
@@ -12,6 +12,7 @@ test('directadmin context derives server name from first host label', function (
$ctx = DirectAdminContext::fromServer([
'USERNAME' => 'mxtest',
'HTTP_HOST' => 'https://h4.domena.pl:2222',
'SERVER_NAME' => 'h4.domena.pl',
'REMOTE_ADDR' => '198.51.100.10',
'REQUEST_METHOD' => 'GET',
], $settings);
@@ -21,6 +22,34 @@ test('directadmin context derives server name from first host label', function (
assert_same('h4', $ctx->serverName());
});
test('directadmin context prefers trusted configured source server name over host header', function (): void {
$settings = Settings::fromArray(['MAIL_LOGIN_SOURCE_SERVER_NAME' => 'h4']);
$ctx = DirectAdminContext::fromServer([
'USERNAME' => 'mxtest',
'HTTP_HOST' => 'evil.example.net:2222',
'SERVER_NAME' => 'h4.domena.pl',
'REMOTE_ADDR' => '198.51.100.10',
'REQUEST_METHOD' => 'GET',
], $settings);
assert_same('h4.domena.pl', $ctx->sourceHost());
assert_same('h4', $ctx->serverName());
});
test('directadmin context fails closed without directadmin username', function (): void {
try {
DirectAdminContext::fromServer([
'USER' => 'root',
'HTTP_HOST' => 'h4.domena.pl:2222',
'REMOTE_ADDR' => '198.51.100.10',
'REQUEST_METHOD' => 'GET',
], Settings::fromArray([]));
throw new RuntimeException('Expected missing USERNAME to fail');
} catch (InvalidArgumentException $e) {
assert_contains('username', strtolower($e->getMessage()));
}
});
test('directadmin context rejects invalid usernames', function (): void {
try {
DirectAdminContext::fromServer([
@@ -58,6 +87,19 @@ test('client ip honors forwarded header only from trusted proxy', function (): v
assert_same('203.0.113.20', $ip);
});
test('client ip supports trusted proxy cidr ranges', function (): void {
$settings = Settings::fromArray([
'MAIL_LOGIN_CLIENT_IP_MODE' => 'trusted_proxy',
'MAIL_LOGIN_TRUSTED_PROXIES' => '10.0.0.0/24',
]);
$ip = ClientIp::resolve([
'REMOTE_ADDR' => '10.0.0.25',
'HTTP_X_FORWARDED_FOR' => '203.0.113.20',
], $settings);
assert_same('203.0.113.20', $ip);
});
test('source authorizer rejects hosts outside allowlist', function (): void {
$settings = Settings::fromArray(['MAIL_LOGIN_ALLOWED_SOURCE_HOSTS' => 'h4.domena.pl']);
@@ -79,4 +121,3 @@ test('key name uses requested format and truncates long values deterministically
assert_true(str_starts_with($long, 'autologin-'));
assert_true((bool)preg_match('/-[a-f0-9]{10}-1782810000$/', $long), 'Long key should contain hash suffix');
});
tests/mx_helper_test.php
+6 -5
View File
@@ -18,7 +18,7 @@ test('mx helper conditionally passes user ip binding to da login-url', function
escapeshellarg($state),
escapeshellarg('h4.domena.pl'),
escapeshellarg($script),
'mail-login-create-url mxtest h4.domena.pl h4 30 1 198.51.100.10 /'
'mail-login-create-url mxtest h4.domena.pl h4 30 1 198.51.100.10 / 1782810000'
);
exec($cmd, $out, $code);
assert_same(0, $code, implode("\n", $out));
@@ -33,7 +33,7 @@ test('mx helper conditionally passes user ip binding to da login-url', function
escapeshellarg($state),
escapeshellarg('h4.domena.pl'),
escapeshellarg($script),
'mail-login-create-url mxtest h4.domena.pl h4 30 0 198.51.100.10 /'
'mail-login-create-url mxtest h4.domena.pl h4 30 0 198.51.100.10 / 1782810001'
);
exec($cmdNoIp, $outNoIp, $codeNoIp);
assert_same(0, $codeNoIp, implode("\n", $outNoIp));
@@ -57,7 +57,7 @@ test('mx helper accepts forced command arguments from ssh original command', fun
escapeshellarg($audit),
escapeshellarg($state),
escapeshellarg('h4.domena.pl'),
escapeshellarg('mail-login-create-url mxtest h4.domena.pl evil 30 1 198.51.100.10 /'),
escapeshellarg('mail-login-create-url mxtest h4.domena.pl evil 30 1 198.51.100.10 / 1782810002'),
escapeshellarg($script)
);
exec($cmd, $out, $code);
@@ -83,7 +83,7 @@ test('mx helper rejects disallowed source hosts and invalid client ips', functio
escapeshellarg($state),
escapeshellarg('h4.domena.pl'),
escapeshellarg($script),
'mail-login-create-url mxtest evil.domena.pl evil 30 1 198.51.100.10 /'
'mail-login-create-url mxtest evil.domena.pl evil 30 1 198.51.100.10 / 1782810003'
);
exec($badHost, $outHost, $codeHost);
assert_same(1, $codeHost);
@@ -95,8 +95,9 @@ test('mx helper rejects disallowed source hosts and invalid client ips', functio
escapeshellarg($state),
escapeshellarg('h4.domena.pl'),
escapeshellarg($script),
'mail-login-create-url mxtest h4.domena.pl h4 30 1 999.999.999.999 /'
'mail-login-create-url mxtest h4.domena.pl h4 30 1 999.999.999.999 / 1782810004'
);
exec($badIp, $outIp, $codeIp);
assert_same(1, $codeIp);
assert_not_contains("\nBROKEN", file_get_contents($audit) ?: '');
});
tests/rate_limiter_test.php
+19
View File
@@ -0,0 +1,19 @@
<?php
declare(strict_types=1);
require_once PLUGIN_ROOT . '/exec/lib/RateLimiter.php';
test('rate limiter enforces limit across sequential requests', function (): void {
$limiter = new RateLimiter(TEST_TMP . '/rate-limit');
$limiter->assertAllowed('user-mxtest', 2, 60);
$limiter->assertAllowed('user-mxtest', 2, 60);
try {
$limiter->assertAllowed('user-mxtest', 2, 60);
throw new RuntimeException('Expected third request to be rate limited');
} catch (RuntimeException $e) {
assert_contains('rate limit', strtolower($e->getMessage()));
}
});
tests/remote_login_url_client_test.php
+7 -5
View File
@@ -15,20 +15,22 @@ test('remote login url client builds fixed ssh command with positional arguments
'LOGIN_KEY_TTL' => '30',
'USER_IP_BOUND' => '1',
]);
$request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/');
$request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/', 1782810000);
$command = RemoteLoginUrlClient::buildCommand($settings, $request);
assert_same('ssh', $command[0]);
assert_contains('UserKnownHostsFile=/secure/known_hosts', implode(' ', $command));
assert_contains('root@mx1.domena.pl', implode(' ', $command));
assert_same('mail-login-create-url', $command[count($command) - 8]);
assert_same('mxtest', $command[count($command) - 7]);
assert_same('1', $command[count($command) - 3]);
$pos = array_search('mail-login-create-url', $command, true);
assert_true(is_int($pos), 'Command marker missing');
assert_same('mxtest', $command[$pos + 1]);
assert_same('1', $command[$pos + 5]);
assert_same('1782810000', $command[$pos + 8]);
});
test('remote login url client validates helper output before returning it', function (): void {
$settings = Settings::fromArray(['MAIL_LOGIN_TARGET_BASE_URL' => 'https://mx1.domena.pl:2222']);
$request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/');
$request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/', 1782810000);
$client = new RemoteLoginUrlClient(function (array $command, int $timeout): ProcessResult {
return new ProcessResult(0, "URL: https://mx1.domena.pl:2222/api/login/url?key=SECRET\n", '');