Fix path traversal via username in session log file paths
Registration only validated username with NotBlank, so a username like "../../../../public/x" got concatenated directly into a filesystem path for both writing (game activity logs) and reading (admin log viewer) - reachable from the public webroot since public/ is a few directories up from where those logs are stored. Adds a character-set validator (letters, numbers, underscore, hyphen) to registration and admin user editing going forward, and sanitizes at the point of use (Player::getLogFileBasename()) so any already-stored unsafe username can't escape the log directory either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -54,11 +54,10 @@ final class GameAdminController extends AbstractController
|
|||||||
{
|
{
|
||||||
$playersLogs = [];
|
$playersLogs = [];
|
||||||
foreach ($session->getPlayers() as $player) {
|
foreach ($session->getPlayers() as $player) {
|
||||||
$username = $player->getUser()->getUsername();
|
$logFile = $this->projectDir . '/var/log/sessions/' . $session->getId() . '/' . $player->getLogFileBasename() . '.txt';
|
||||||
$logFile = $this->projectDir . '/var/log/sessions/' . $session->getId() . '/' . $username . '.txt';
|
|
||||||
|
|
||||||
$playersLogs[] = [
|
$playersLogs[] = [
|
||||||
'username' => $username,
|
'username' => $player->getUser()->getUsername(),
|
||||||
'logs' => file_exists($logFile) ? file_get_contents($logFile) : '',
|
'logs' => file_exists($logFile) ? file_get_contents($logFile) : '',
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -66,4 +66,19 @@ class Player
|
|||||||
|
|
||||||
return $this;
|
return $this;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A filesystem-safe basename derived from the player's username, for use when
|
||||||
|
* building per-player log file paths. Usernames are validated to only contain
|
||||||
|
* safe characters at registration time, but this sanitizes defensively too, so
|
||||||
|
* a path segment can never traverse outside its intended directory regardless
|
||||||
|
* of what ends up stored on the user.
|
||||||
|
*/
|
||||||
|
public function getLogFileBasename(): string
|
||||||
|
{
|
||||||
|
$username = $this->user?->getUsername() ?? '';
|
||||||
|
$safe = preg_replace('/[^A-Za-z0-9_-]/', '_', $username);
|
||||||
|
|
||||||
|
return $safe !== null && $safe !== '' ? $safe : ('player-' . $this->id);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -107,14 +107,13 @@ class GameResponseService
|
|||||||
private function logSessionActivity(Player $player, string $content): void
|
private function logSessionActivity(Player $player, string $content): void
|
||||||
{
|
{
|
||||||
$sessionId = $player->getSession()->getId();
|
$sessionId = $player->getSession()->getId();
|
||||||
$username = $player->getUser()->getUsername();
|
|
||||||
$logDir = $this->projectDir . '/var/log/sessions/' . $sessionId;
|
$logDir = $this->projectDir . '/var/log/sessions/' . $sessionId;
|
||||||
|
|
||||||
if (!is_dir($logDir)) {
|
if (!is_dir($logDir)) {
|
||||||
mkdir($logDir, 0777, true);
|
mkdir($logDir, 0777, true);
|
||||||
}
|
}
|
||||||
|
|
||||||
$logFile = $logDir . '/' . $username . '.txt';
|
$logFile = $logDir . '/' . $player->getLogFileBasename() . '.txt';
|
||||||
$timestamp = date('Y-m-d H:i:s');
|
$timestamp = date('Y-m-d H:i:s');
|
||||||
$logMessage = sprintf("[%s] %s\n", $timestamp, $content);
|
$logMessage = sprintf("[%s] %s\n", $timestamp, $content);
|
||||||
|
|
||||||
|
|||||||
@@ -16,6 +16,7 @@ use Symfony\Component\OptionsResolver\OptionsResolver;
|
|||||||
use Symfony\Component\Validator\Constraints\Email;
|
use Symfony\Component\Validator\Constraints\Email;
|
||||||
use Symfony\Component\Validator\Constraints\Length;
|
use Symfony\Component\Validator\Constraints\Length;
|
||||||
use Symfony\Component\Validator\Constraints\NotBlank;
|
use Symfony\Component\Validator\Constraints\NotBlank;
|
||||||
|
use Symfony\Component\Validator\Constraints\Regex;
|
||||||
|
|
||||||
class AdminUserType extends AbstractType
|
class AdminUserType extends AbstractType
|
||||||
{
|
{
|
||||||
@@ -26,7 +27,14 @@ class AdminUserType extends AbstractType
|
|||||||
'constraints' => [new NotBlank(), new Email()],
|
'constraints' => [new NotBlank(), new Email()],
|
||||||
])
|
])
|
||||||
->add('username', TextType::class, [
|
->add('username', TextType::class, [
|
||||||
'constraints' => [new NotBlank(), new Length(min: 2, max: 180)],
|
'constraints' => [
|
||||||
|
new NotBlank(),
|
||||||
|
new Length(min: 2, max: 32),
|
||||||
|
new Regex(
|
||||||
|
pattern: '/^[A-Za-z0-9_-]+$/',
|
||||||
|
message: 'Username may only contain letters, numbers, underscores, and hyphens.',
|
||||||
|
),
|
||||||
|
],
|
||||||
])
|
])
|
||||||
->add('plainPassword', PasswordType::class, [
|
->add('plainPassword', PasswordType::class, [
|
||||||
'mapped' => false,
|
'mapped' => false,
|
||||||
|
|||||||
@@ -16,6 +16,7 @@ use Symfony\Component\OptionsResolver\OptionsResolver;
|
|||||||
use Symfony\Component\Validator\Constraints\IsTrue;
|
use Symfony\Component\Validator\Constraints\IsTrue;
|
||||||
use Symfony\Component\Validator\Constraints\Length;
|
use Symfony\Component\Validator\Constraints\Length;
|
||||||
use Symfony\Component\Validator\Constraints\NotBlank;
|
use Symfony\Component\Validator\Constraints\NotBlank;
|
||||||
|
use Symfony\Component\Validator\Constraints\Regex;
|
||||||
|
|
||||||
class RegistrationFormType extends AbstractType
|
class RegistrationFormType extends AbstractType
|
||||||
{
|
{
|
||||||
@@ -26,6 +27,11 @@ class RegistrationFormType extends AbstractType
|
|||||||
->add('username', TextType::class, [
|
->add('username', TextType::class, [
|
||||||
'constraints' => [
|
'constraints' => [
|
||||||
new NotBlank(message: 'Please enter a username'),
|
new NotBlank(message: 'Please enter a username'),
|
||||||
|
new Length(min: 3, max: 32, minMessage: 'Your username should be at least {{ limit }} characters', maxMessage: 'Your username cannot be longer than {{ limit }} characters'),
|
||||||
|
new Regex(
|
||||||
|
pattern: '/^[A-Za-z0-9_-]+$/',
|
||||||
|
message: 'Your username may only contain letters, numbers, underscores, and hyphens.',
|
||||||
|
),
|
||||||
],
|
],
|
||||||
])
|
])
|
||||||
->add('plainPassword', RepeatedType::class, [
|
->add('plainPassword', RepeatedType::class, [
|
||||||
|
|||||||
@@ -74,6 +74,7 @@ class SessionLoggingTest extends TestCase
|
|||||||
$player->method('getUser')->willReturn($user);
|
$player->method('getUser')->willReturn($user);
|
||||||
$player->method('getSession')->willReturn($session);
|
$player->method('getSession')->willReturn($session);
|
||||||
$player->method('getScreen')->willReturn(1);
|
$player->method('getScreen')->willReturn(1);
|
||||||
|
$player->method('getLogFileBasename')->willReturn('player1');
|
||||||
|
|
||||||
$this->security->method('getUser')->willReturn($user);
|
$this->security->method('getUser')->willReturn($user);
|
||||||
$this->playerService->method('GetCurrentlyActiveAsPlayer')->willReturn($player);
|
$this->playerService->method('GetCurrentlyActiveAsPlayer')->willReturn($player);
|
||||||
|
|||||||
Reference in New Issue
Block a user