diff --git a/packages/auth/src/Authentication/AuthenticatorInitializer.php b/packages/auth/src/Authentication/AuthenticatorInitializer.php index e87555096b..92a86d549c 100644 --- a/packages/auth/src/Authentication/AuthenticatorInitializer.php +++ b/packages/auth/src/Authentication/AuthenticatorInitializer.php @@ -8,7 +8,7 @@ use Tempest\Container\Initializer; use Tempest\Container\Singleton; use Tempest\Http\Session\Session; -use Tempest\Http\Session\SessionManager; +use Tempest\Http\Session\SessionRegenerator; final readonly class AuthenticatorInitializer implements Initializer { @@ -16,9 +16,9 @@ public function initialize(Container $container): Authenticator { return new SessionAuthenticator( - sessionManager: $container->get(SessionManager::class), session: $container->get(Session::class), authenticatableResolver: $container->get(AuthenticatableResolver::class), + sessionRegenerator: $container->get(SessionRegenerator::class), ); } } diff --git a/packages/auth/src/Authentication/SessionAuthenticator.php b/packages/auth/src/Authentication/SessionAuthenticator.php index 572b4fcfef..cc09a6cc39 100644 --- a/packages/auth/src/Authentication/SessionAuthenticator.php +++ b/packages/auth/src/Authentication/SessionAuthenticator.php @@ -5,7 +5,7 @@ namespace Tempest\Auth\Authentication; use Tempest\Http\Session\Session; -use Tempest\Http\Session\SessionManager; +use Tempest\Http\Session\SessionRegenerator; final class SessionAuthenticator implements Authenticator { @@ -20,9 +20,9 @@ final class SessionAuthenticator implements Authenticator private ?Authenticatable $current = null; public function __construct( - private readonly SessionManager $sessionManager, private readonly Session $session, private readonly AuthenticatableResolver $authenticatableResolver, + private readonly SessionRegenerator $sessionRegenerator, ) {} public function authenticate(Authenticatable $authenticatable): void @@ -43,15 +43,19 @@ public function authenticate(Authenticatable $authenticatable): void $this->currentId = $id; $this->currentClass = $class; $this->current = $authenticatable; + + // The session identifier must not survive a change in privilege level, or one + // known to an attacker before authentication stays valid afterwards. + $this->sessionRegenerator->regenerate(); } public function deauthenticate(): void { - $this->session->remove(self::AUTHENTICATABLE_KEY); - $this->session->remove(self::AUTHENTICATABLE_CLASS); $this->clearCurrent(); - $this->sessionManager->save($this->session); + // Regenerate session without preserving data to prevent session fixation + // and purge all authenticated user data. + $this->sessionRegenerator->invalidate(); } public function current(): ?Authenticatable diff --git a/packages/auth/tests/SessionAuthenticatorTest.php b/packages/auth/tests/SessionAuthenticatorTest.php index 6e791e796d..45dd4e459a 100644 --- a/packages/auth/tests/SessionAuthenticatorTest.php +++ b/packages/auth/tests/SessionAuthenticatorTest.php @@ -13,7 +13,9 @@ use Tempest\DateTime\DateTime; use Tempest\Http\Session\Session; use Tempest\Http\Session\SessionId; +use Tempest\Http\Session\SessionIdResolver; use Tempest\Http\Session\SessionManager; +use Tempest\Http\Session\SessionRegenerator; final class SessionAuthenticatorTest extends TestCase { @@ -27,9 +29,9 @@ public function current_memoizes_the_resolved_authenticatable_for_the_current_se $session->set(SessionAuthenticator::AUTHENTICATABLE_CLASS, MemoizedAuthenticatable::class); $authenticator = new SessionAuthenticator( - sessionManager: new TestingSessionManager(), session: $session, authenticatableResolver: $resolver, + sessionRegenerator: $this->createRegenerator($session), ); $this->assertSame($authenticatable, $authenticator->current()); @@ -46,9 +48,9 @@ public function current_memoizes_a_missing_authenticatable_for_the_current_sessi $session->set(SessionAuthenticator::AUTHENTICATABLE_CLASS, MemoizedAuthenticatable::class); $authenticator = new SessionAuthenticator( - sessionManager: new TestingSessionManager(), session: $session, authenticatableResolver: $resolver, + sessionRegenerator: $this->createRegenerator($session), ); $this->assertNull($authenticator->current()); @@ -68,9 +70,9 @@ public function current_re_resolves_when_the_session_identity_changes(): void $session->set(SessionAuthenticator::AUTHENTICATABLE_CLASS, MemoizedAuthenticatable::class); $authenticator = new SessionAuthenticator( - sessionManager: new TestingSessionManager(), session: $session, authenticatableResolver: $resolver, + sessionRegenerator: $this->createRegenerator($session), ); $current = $authenticator->current(); @@ -95,9 +97,9 @@ public function reset_clears_the_cached_current_authenticatable(): void $session->set(SessionAuthenticator::AUTHENTICATABLE_CLASS, MemoizedAuthenticatable::class); $authenticator = new SessionAuthenticator( - sessionManager: new TestingSessionManager(), session: $session, authenticatableResolver: $resolver, + sessionRegenerator: $this->createRegenerator($session), ); $this->assertSame($authenticatable, $authenticator->current()); @@ -120,9 +122,9 @@ public function authenticate_replaces_a_cached_current_authenticatable(): void $session->set(SessionAuthenticator::AUTHENTICATABLE_CLASS, MemoizedAuthenticatable::class); $authenticator = new SessionAuthenticator( - sessionManager: new TestingSessionManager(), session: $session, authenticatableResolver: $resolver, + sessionRegenerator: $this->createRegenerator($session), ); $current = $authenticator->current(); @@ -136,6 +138,58 @@ public function authenticate_replaces_a_cached_current_authenticatable(): void $this->assertSame(2, $current->id); } + #[Test] + public function authenticate_regenerates_the_session_identifier(): void + { + $session = $this->createSession(); + $sessionManager = new TestingSessionManager(); + + $authenticator = new SessionAuthenticator( + session: $session, + authenticatableResolver: new CountingAuthenticatableResolver(), + sessionRegenerator: $this->createRegenerator($session, $sessionManager), + ); + + $authenticator->authenticate(new MemoizedAuthenticatable(id: 1)); + + $this->assertNotSame('test-session', (string) $session->id); + $this->assertSame(1, $sessionManager->deletedSessions); + $this->assertSame(1, $session->get(SessionAuthenticator::AUTHENTICATABLE_KEY)); + } + + #[Test] + public function deauthenticate_regenerates_the_session_identifier_and_discards_the_data(): void + { + $session = $this->createSession(); + $session->set(SessionAuthenticator::AUTHENTICATABLE_KEY, 1); + $session->set(SessionAuthenticator::AUTHENTICATABLE_CLASS, MemoizedAuthenticatable::class); + $session->set('key', 'value'); + $sessionManager = new TestingSessionManager(); + + $authenticator = new SessionAuthenticator( + session: $session, + authenticatableResolver: new CountingAuthenticatableResolver(), + sessionRegenerator: $this->createRegenerator($session, $sessionManager), + ); + + $authenticator->deauthenticate(); + + $this->assertNotSame('test-session', (string) $session->id); + $this->assertSame(1, $sessionManager->deletedSessions); + $this->assertNull($session->get(SessionAuthenticator::AUTHENTICATABLE_KEY)); + $this->assertNull($session->get(SessionAuthenticator::AUTHENTICATABLE_CLASS)); + $this->assertNull($session->get('key')); + } + + private function createRegenerator(Session $session, ?SessionManager $sessionManager = null): SessionRegenerator + { + return new SessionRegenerator( + sessionManager: $sessionManager ?? new TestingSessionManager(), + session: $session, + sessionIdResolver: new TestingSessionIdResolver(), + ); + } + private function createSession(): Session { $now = DateTime::now(); @@ -186,10 +240,25 @@ public function resolveId(Authenticatable $authenticatable): int } } +final class TestingSessionIdResolver implements SessionIdResolver +{ + public function resolve(): SessionId + { + return new SessionId('test-session'); + } + + public function issueNewId(): SessionId + { + return new SessionId('regenerated-session-' . uniqid()); + } +} + final class TestingSessionManager implements SessionManager { public int $savedSessions = 0; + public int $deletedSessions = 0; + public function getOrCreate(SessionId $id): Session { $now = DateTime::now(); @@ -202,7 +271,10 @@ public function save(Session $session): void $this->savedSessions++; } - public function delete(Session $session): void {} + public function delete(Session $session): void + { + $this->deletedSessions++; + } public function isValid(Session $session): bool { diff --git a/packages/http/src/Session/Resolvers/CookieSessionIdResolver.php b/packages/http/src/Session/Resolvers/CookieSessionIdResolver.php index 4c5077ac76..25b8831ff8 100644 --- a/packages/http/src/Session/Resolvers/CookieSessionIdResolver.php +++ b/packages/http/src/Session/Resolvers/CookieSessionIdResolver.php @@ -30,27 +30,37 @@ public function __construct( public function resolve(): SessionId { - $sessionKey = str($this->appConfig->name ?? 'tempest') - ->snake() - ->append('_session_id') - ->toString(); - - $id = $this->request->getCookie($sessionKey)?->value; + $id = $this->request->getCookie($this->getSessionKey())?->value; if (! $id) { - $id = (string) Uuid::v4(); - - $this->cookies->add(new Cookie( - key: $sessionKey, - value: $id, - expiresAt: $this->clock->now()->plus($this->sessionConfig->expiration), - path: '/', - secure: Str\starts_with($this->appConfig->baseUri, needles: 'https'), - httpOnly: true, - sameSite: SameSite::LAX, - )); + return $this->issueNewId(); } return new SessionId($id); } + + public function issueNewId(): SessionId + { + $id = (string) Uuid::v4(); + + $this->cookies->add(new Cookie( + key: $this->getSessionKey(), + value: $id, + expiresAt: $this->clock->now()->plus($this->sessionConfig->expiration), + path: '/', + secure: Str\starts_with($this->appConfig->baseUri, needles: 'https'), + httpOnly: true, + sameSite: SameSite::LAX, + )); + + return new SessionId($id); + } + + private function getSessionKey(): string + { + return str($this->appConfig->name ?? 'tempest') + ->snake() + ->append('_session_id') + ->toString(); + } } diff --git a/packages/http/src/Session/Resolvers/HeaderSessionIdResolver.php b/packages/http/src/Session/Resolvers/HeaderSessionIdResolver.php index ecc26b7245..765c3513c9 100644 --- a/packages/http/src/Session/Resolvers/HeaderSessionIdResolver.php +++ b/packages/http/src/Session/Resolvers/HeaderSessionIdResolver.php @@ -30,4 +30,9 @@ public function resolve(): SessionId id: $this->request->headers[$sessionKey] ?? Uuid::v4()->toString(), ); } + + public function issueNewId(): SessionId + { + return new SessionId(id: Uuid::v4()->toString()); + } } diff --git a/packages/http/src/Session/Session.php b/packages/http/src/Session/Session.php index 78846c4a92..677485a66f 100644 --- a/packages/http/src/Session/Session.php +++ b/packages/http/src/Session/Session.php @@ -132,6 +132,15 @@ public function cleanup(): void } } + /** + * @internal Prefer {@see SessionRegenerator}, which also destroys the session that is being + * replaced and sends the new identifier to the client. + */ + public function replaceId(SessionId $id): void + { + $this->id = $id; + } + /** * Clears all values from the session. */ diff --git a/packages/http/src/Session/SessionIdResolver.php b/packages/http/src/Session/SessionIdResolver.php index 6b49d69ddb..aa52293593 100644 --- a/packages/http/src/Session/SessionIdResolver.php +++ b/packages/http/src/Session/SessionIdResolver.php @@ -6,5 +6,15 @@ interface SessionIdResolver { + /** + * Resolves the identifier sent by the client, creating a new one if there is none. + */ public function resolve(): SessionId; + + /** + * Creates a new identifier and sends it to the client, replacing the one it was using. + * + * @see SessionRegenerator + */ + public function issueNewId(): SessionId; } diff --git a/packages/http/src/Session/SessionRegenerator.php b/packages/http/src/Session/SessionRegenerator.php new file mode 100644 index 0000000000..9ee2afa123 --- /dev/null +++ b/packages/http/src/Session/SessionRegenerator.php @@ -0,0 +1,45 @@ +sessionManager->delete($this->session); + + $this->session->replaceId($this->sessionIdResolver->issueNewId()); + + $this->sessionManager->save($this->session); + } + + /** + * Assigns a new ID to the current session, discarding all data. + */ + public function invalidate(): void + { + $this->sessionManager->delete($this->session); + + $this->session->replaceId($this->sessionIdResolver->issueNewId()); + $this->session->clear(); + + $this->sessionManager->save($this->session); + } +} diff --git a/tests/Integration/Http/SessionRegeneratorTest.php b/tests/Integration/Http/SessionRegeneratorTest.php new file mode 100644 index 0000000000..9518b6e099 --- /dev/null +++ b/tests/Integration/Http/SessionRegeneratorTest.php @@ -0,0 +1,76 @@ + $this->container->get(Session::class); + } + + private SessionRegenerator $regenerator { + get => $this->container->get(SessionRegenerator::class); + } + + #[Test] + public function assigns_a_new_identifier_and_keeps_data(): void + { + $this->session->set('key', 'value'); + $previousId = (string) $this->session->id; + + $this->regenerator->regenerate(); + + $this->assertNotSame($previousId, (string) $this->session->id); + $this->assertSame('value', $this->session->get('key')); + } + + #[Test] + public function invalidate_discards_data(): void + { + $this->session->set('key', 'value'); + + $this->regenerator->invalidate(); + + $this->assertNull($this->session->get('key')); + } + + #[Test] + public function destroys_the_session_it_replaces(): void + { + $sessionManager = $this->container->get(SessionManager::class); + + $this->session->set('key', 'value'); + $previousId = $this->session->id; + + $this->regenerator->regenerate(); + + $previousSession = $sessionManager->getOrCreate($previousId); + + $this->assertNull($previousSession->get('key')); + } + + #[Test] + public function sends_the_new_identifier_to_the_client(): void + { + $cookies = $this->container->get(CookieManager::class); + + $this->regenerator->regenerate(); + + $this->assertSame( + (string) $this->session->id, + $cookies->get('tempest_session_id')?->value, + ); + } +} diff --git a/tests/Integration/Http/SessionTest.php b/tests/Integration/Http/SessionTest.php index e6d22a9107..7642f2a783 100644 --- a/tests/Integration/Http/SessionTest.php +++ b/tests/Integration/Http/SessionTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\Attributes\Test; use Tempest\Http\Session\Session; +use Tempest\Http\Session\SessionId; use Tests\Tempest\Integration\FrameworkIntegrationTestCase; /** @@ -139,6 +140,17 @@ public function clear(): void $this->assertEmpty($this->session->all()); } + #[Test] + public function replace_id_preserves_data(): void + { + $this->session->set('key', 'value'); + + $this->session->replaceId(new SessionId('new_session')); + + $this->assertSame('new_session', (string) $this->session->id); + $this->assertSame('value', $this->session->get('key')); + } + #[Test] public function session_is_reset(): void {