From cb5f7037b2de7818e1863026df4ebb4c036cd573 Mon Sep 17 00:00:00 2001 From: fs Date: Wed, 25 Mar 2026 10:02:03 +0100 Subject: [PATCH] Harden module runtime: audited listener failures and DI-first resolver --- .../Registrars/ServiceFactoryRegistrar.php | 4 +- lib/App/Module/ModuleClassResolver.php | 50 +++++++++ lib/App/Module/ModuleEventDispatcher.php | 36 +++++- lib/App/registerContainer.php | 4 +- lib/Service/Access/AccessPolicyFactory.php | 26 ++--- .../Auth/AuthSessionTenantContextService.php | 6 +- lib/Service/Auth/LdapConnectionGateway.php | 12 +- lib/Support/SearchConfig.php | 37 ++++++- lib/Support/helpers/app.php | 8 +- .../AccessPolicyFactoryModuleResolverTest.php | 104 ++++++++++++++++++ tests/Service/Auth/LdapAuthServiceTest.php | 42 +------ .../Unit/Module/ModuleEventDispatcherTest.php | 64 +++++++++++ 12 files changed, 314 insertions(+), 79 deletions(-) create mode 100644 lib/App/Module/ModuleClassResolver.php create mode 100644 tests/Service/Access/AccessPolicyFactoryModuleResolverTest.php diff --git a/lib/App/Container/Registrars/ServiceFactoryRegistrar.php b/lib/App/Container/Registrars/ServiceFactoryRegistrar.php index 5e5e799..1ef7a1c 100644 --- a/lib/App/Container/Registrars/ServiceFactoryRegistrar.php +++ b/lib/App/Container/Registrars/ServiceFactoryRegistrar.php @@ -4,6 +4,7 @@ namespace MintyPHP\App\Container\Registrars; use MintyPHP\App\AppContainer; use MintyPHP\App\Container\ContainerRegistrar; +use MintyPHP\App\Module\ModuleClassResolver; use MintyPHP\App\Module\ModuleEventDispatcher; use MintyPHP\App\Module\ModuleRegistry; use MintyPHP\Http\CookieStoreInterface; @@ -116,7 +117,8 @@ final class ServiceFactoryRegistrar implements ContainerRegistrar $container->set(AccessPolicyFactory::class, static fn (AppContainer $c): AccessPolicyFactory => new AccessPolicyFactory( $c->get(PermissionService::class), $c->get(TenantScopeService::class), - $c->has(ModuleRegistry::class) ? $c->get(ModuleRegistry::class) : null + $c->has(ModuleRegistry::class) ? $c->get(ModuleRegistry::class) : null, + static fn (string $class): ?object => ModuleClassResolver::resolve($c, $class) )); $container->set(UserGatewayFactory::class, static fn (AppContainer $c): UserGatewayFactory => new UserGatewayFactory( $c->get(AccessServicesFactory::class), diff --git a/lib/App/Module/ModuleClassResolver.php b/lib/App/Module/ModuleClassResolver.php new file mode 100644 index 0000000..558958b --- /dev/null +++ b/lib/App/Module/ModuleClassResolver.php @@ -0,0 +1,50 @@ +has($class)) { + try { + $resolved = $container->get($class); + return is_object($resolved) ? $resolved : null; + } catch (\Throwable) { + return null; + } + } + + if (!class_exists($class)) { + return null; + } + + try { + $reflection = new \ReflectionClass($class); + if (!$reflection->isInstantiable()) { + return null; + } + + $constructor = $reflection->getConstructor(); + if ($constructor !== null && $constructor->getNumberOfRequiredParameters() > 0) { + return null; + } + + return $reflection->newInstance(); + } catch (\Throwable) { + return null; + } + } +} diff --git a/lib/App/Module/ModuleEventDispatcher.php b/lib/App/Module/ModuleEventDispatcher.php index 717ff75..d99da40 100644 --- a/lib/App/Module/ModuleEventDispatcher.php +++ b/lib/App/Module/ModuleEventDispatcher.php @@ -4,6 +4,8 @@ namespace MintyPHP\App\Module; use MintyPHP\App\AppContainer; use MintyPHP\App\Module\Contracts\EventListener; +use MintyPHP\Http\RequestContext; +use MintyPHP\Service\Audit\SystemAuditService; /** * Fire-and-forget event dispatcher for module lifecycle events. @@ -19,7 +21,8 @@ final class ModuleEventDispatcher */ public function __construct( private readonly array $listenerMap, - private readonly AppContainer $container + private readonly AppContainer $container, + private readonly ?SystemAuditService $systemAuditService = null ) { } @@ -31,19 +34,42 @@ final class ModuleEventDispatcher $listeners = $this->listenerMap[$event] ?? []; foreach ($listeners as $descriptor) { + $class = ''; + $method = ''; try { $class = $descriptor['class']; $method = $descriptor['method']; - $listener = $this->container->get($class); + $listener = ModuleClassResolver::resolve($this->container, $class); if (!$listener instanceof EventListener) { - $listener = new $class(); + continue; } $listener->{$method}($event, $payload); - } catch (\Throwable) { - // Swallow — one failing listener must not break the chain. + } catch (\Throwable $exception) { + $this->recordListenerFailure($event, $class, $method, $exception); } } } + + private function recordListenerFailure( + string $event, + string $listenerClass, + string $listenerMethod, + \Throwable $exception + ): void { + if ($this->systemAuditService === null) { + return; + } + + $this->systemAuditService->record('module.listener.failed', 'failed', [ + 'metadata' => [ + 'module_event' => $event, + 'listener_class' => $listenerClass, + 'listener_method' => $listenerMethod, + 'exception_class' => $exception::class, + 'request_id' => RequestContext::id(), + ], + ]); + } } diff --git a/lib/App/registerContainer.php b/lib/App/registerContainer.php index 2242f94..281e415 100644 --- a/lib/App/registerContainer.php +++ b/lib/App/registerContainer.php @@ -12,6 +12,7 @@ use MintyPHP\App\Container\Registrars\SettingsRegistrar; use MintyPHP\App\Container\Registrars\UserRegistrar; use MintyPHP\App\Module\ModuleEventDispatcher; use MintyPHP\App\Module\ModuleRegistry; +use MintyPHP\Service\Audit\SystemAuditService; $container = new AppContainer(); @@ -46,7 +47,8 @@ $moduleRegistry = ModuleRegistry::boot($modulesDir, $enabledModules); $container->set(ModuleRegistry::class, static fn (): ModuleRegistry => $moduleRegistry); $container->set(ModuleEventDispatcher::class, static fn () => new ModuleEventDispatcher( $moduleRegistry->getEventListeners(), - $container + $container, + $container->has(SystemAuditService::class) ? $container->get(SystemAuditService::class) : null )); // Disallow overriding existing core bindings from module registrars. diff --git a/lib/Service/Access/AccessPolicyFactory.php b/lib/Service/Access/AccessPolicyFactory.php index 3ab0961..ff67305 100644 --- a/lib/Service/Access/AccessPolicyFactory.php +++ b/lib/Service/Access/AccessPolicyFactory.php @@ -15,12 +15,17 @@ class AccessPolicyFactory private ?TenantAuthorizationPolicy $tenantAuthorizationPolicy = null; private ?DepartmentAuthorizationPolicy $departmentAuthorizationPolicy = null; private ?AuthorizationService $authorizationService = null; + private \Closure $moduleClassResolver; public function __construct( private readonly PermissionService $permissionService, private readonly TenantScopeService $tenantScopeService, - private readonly ?ModuleRegistry $moduleRegistry = null + private readonly ?ModuleRegistry $moduleRegistry = null, + ?callable $moduleClassResolver = null ) { + $this->moduleClassResolver = $moduleClassResolver !== null + ? \Closure::fromCallable($moduleClassResolver) + : static fn (string $class): ?object => null; } public function createUserAuthorizationPolicy(): UserAuthorizationPolicy @@ -108,26 +113,11 @@ class AccessPolicyFactory private function instantiateModulePolicy(string $policyClass): ?AuthorizationPolicyInterface { - if (!class_exists($policyClass)) { - return null; - } - try { - $reflection = new \ReflectionClass($policyClass); - $constructor = $reflection->getConstructor(); - if ($constructor === null || $constructor->getNumberOfRequiredParameters() === 0) { - $instance = $reflection->newInstance(); - return $instance instanceof AuthorizationPolicyInterface ? $instance : null; - } - - if ($constructor->getNumberOfRequiredParameters() === 1) { - $instance = $reflection->newInstance($this->permissionService); - return $instance instanceof AuthorizationPolicyInterface ? $instance : null; - } + $instance = ($this->moduleClassResolver)($policyClass); + return $instance instanceof AuthorizationPolicyInterface ? $instance : null; } catch (\Throwable) { return null; } - - return null; } } diff --git a/lib/Service/Auth/AuthSessionTenantContextService.php b/lib/Service/Auth/AuthSessionTenantContextService.php index 1658692..91f3ca4 100644 --- a/lib/Service/Auth/AuthSessionTenantContextService.php +++ b/lib/Service/Auth/AuthSessionTenantContextService.php @@ -4,6 +4,7 @@ namespace MintyPHP\Service\Auth; use MintyPHP\App\AppContainer; use MintyPHP\App\Module\Contracts\SessionProvider; +use MintyPHP\App\Module\ModuleClassResolver; use MintyPHP\App\Module\ModuleRegistry; use MintyPHP\Http\SessionStoreInterface; use MintyPHP\Service\User\UserTenantContextService; @@ -70,10 +71,7 @@ class AuthSessionTenantContextService return []; } foreach ($registry->getSessionProviders() as $providerClass) { - if (!class_exists($providerClass)) { - continue; - } - $provider = new $providerClass(); + $provider = ModuleClassResolver::resolve($this->container, $providerClass); if ($provider instanceof SessionProvider) { $providers[] = $provider; } diff --git a/lib/Service/Auth/LdapConnectionGateway.php b/lib/Service/Auth/LdapConnectionGateway.php index 2e4649d..e9434f2 100644 --- a/lib/Service/Auth/LdapConnectionGateway.php +++ b/lib/Service/Auth/LdapConnectionGateway.php @@ -31,8 +31,10 @@ class LdapConnectionGateway return @ldap_bind($conn, $dn, $password); } - /** @return \LDAP\Result|false */ - public function search(\LDAP\Connection $conn, string $baseDn, string $filter, array $attributes = [], int $scope = 0): \LDAP\Result|false + /** + * @return mixed LDAP\Result on success, false on failure. + */ + public function search(\LDAP\Connection $conn, string $baseDn, string $filter, array $attributes = [], int $scope = 0): mixed { if ($scope === 1) { return @ldap_list($conn, $baseDn, $filter, $attributes); @@ -40,8 +42,12 @@ class LdapConnectionGateway return @ldap_search($conn, $baseDn, $filter, $attributes); } - public function getEntries(\LDAP\Connection $conn, \LDAP\Result $result): array|false + public function getEntries(\LDAP\Connection $conn, mixed $result): array|false { + if (!$result instanceof \LDAP\Result) { + return false; + } + return ldap_get_entries($conn, $result); } diff --git a/lib/Support/SearchConfig.php b/lib/Support/SearchConfig.php index cf9c80b..d7aa8aa 100644 --- a/lib/Support/SearchConfig.php +++ b/lib/Support/SearchConfig.php @@ -20,10 +20,15 @@ class SearchConfig /** @var (callable(): ModuleRegistry)|null */ private static $moduleRegistryResolver = null; + /** @var (callable(string): ?object)|null */ + private static $moduleClassResolver = null; - public static function configure(callable $moduleRegistryResolver): void + public static function configure(callable $moduleRegistryResolver, ?callable $moduleClassResolver = null): void { self::$moduleRegistryResolver = $moduleRegistryResolver; + self::$moduleClassResolver = $moduleClassResolver !== null + ? \Closure::fromCallable($moduleClassResolver) + : null; self::resetModuleProviders(); } @@ -155,10 +160,7 @@ class SearchConfig return self::$moduleProviders; } foreach ($registry->getSearchResources() as $providerClass) { - if (!class_exists($providerClass)) { - continue; - } - $provider = new $providerClass(); + $provider = self::resolveModuleProvider($providerClass); if ($provider instanceof SearchResourceProvider) { self::$moduleProviders[] = $provider; foreach ($provider->resources() as $key => $resource) { @@ -182,6 +184,31 @@ class SearchConfig self::$moduleProviderByKey = []; } + private static function resolveModuleProvider(string $providerClass): ?SearchResourceProvider + { + try { + if (is_callable(self::$moduleClassResolver)) { + $resolved = (self::$moduleClassResolver)($providerClass); + return $resolved instanceof SearchResourceProvider ? $resolved : null; + } + + if (!class_exists($providerClass)) { + return null; + } + + $reflection = new \ReflectionClass($providerClass); + $constructor = $reflection->getConstructor(); + if ($constructor !== null && $constructor->getNumberOfRequiredParameters() > 0) { + return null; + } + + $instance = $reflection->newInstance(); + return $instance instanceof SearchResourceProvider ? $instance : null; + } catch (\Throwable) { + return null; + } + } + public static function hotkeyPreviewResource(string $query): ?array { return SearchSpecialResourceProvider::hotkeyPreviewResource($query); diff --git a/lib/Support/helpers/app.php b/lib/Support/helpers/app.php index 3baebe5..542d22e 100644 --- a/lib/Support/helpers/app.php +++ b/lib/Support/helpers/app.php @@ -60,7 +60,8 @@ function setAppContainer(\MintyPHP\App\AppContainer $container): void ); \MintyPHP\Support\SearchConfig::configure( - static fn (): \MintyPHP\App\Module\ModuleRegistry => $container->get(\MintyPHP\App\Module\ModuleRegistry::class) + static fn (): \MintyPHP\App\Module\ModuleRegistry => $container->get(\MintyPHP\App\Module\ModuleRegistry::class), + static fn (string $class): ?object => \MintyPHP\App\Module\ModuleClassResolver::resolve($container, $class) ); } @@ -691,10 +692,7 @@ function appBuildLayoutNavContext(array $layoutAuth, array $session, array $quer } foreach ($moduleRegistry->getLayoutContextProviders() as $providerClass) { - if (!class_exists($providerClass)) { - continue; - } - $provider = new $providerClass(); + $provider = \MintyPHP\App\Module\ModuleClassResolver::resolve($container, $providerClass); if ($provider instanceof \MintyPHP\App\Module\Contracts\LayoutContextProvider) { $providerData = $provider->provide($session, $container); if (!is_array($providerData)) { diff --git a/tests/Service/Access/AccessPolicyFactoryModuleResolverTest.php b/tests/Service/Access/AccessPolicyFactoryModuleResolverTest.php new file mode 100644 index 0000000..501c682 --- /dev/null +++ b/tests/Service/Access/AccessPolicyFactoryModuleResolverTest.php @@ -0,0 +1,104 @@ + 'mod-policy', + 'authorization_policies' => [TestMultiDependencyPolicy::class], + ], true) . ';' + ); + + try { + $registry = ModuleRegistry::boot($fixturesDir, ['mod-policy']); + + $permissionService = $this->createMock(PermissionService::class); + $tenantScopeService = $this->createMock(TenantScopeService::class); + $dependency = new TestPolicyDependency(); + + $container = new AppContainer(); + $container->set( + TestMultiDependencyPolicy::class, + static fn () => new TestMultiDependencyPolicy($permissionService, $dependency) + ); + + $factory = new AccessPolicyFactory( + $permissionService, + $tenantScopeService, + $registry, + static fn (string $class): ?object => ModuleClassResolver::resolve($container, $class) + ); + + $authorizationService = $factory->createAuthorizationService(); + $decision = $authorizationService->authorize(TestMultiDependencyPolicy::ABILITY, [ + 'actor_user_id' => 7, + ]); + + self::assertTrue($decision->isAllowed()); + self::assertSame('container', $decision->attribute('resolver')); + } finally { + @unlink($moduleDir . '/module.php'); + @rmdir($moduleDir); + @rmdir($fixturesDir); + } + } +} + +final class TestPolicyDependency +{ + public function value(): string + { + return 'container'; + } +} + +final class TestMultiDependencyPolicy implements AuthorizationPolicyInterface +{ + public const ABILITY = 'module.multi.dep'; + + public function __construct( + private readonly PermissionService $permissionService, + private readonly TestPolicyDependency $dependency + ) { + } + + public function supports(string $ability): bool + { + return $ability === self::ABILITY; + } + + public function authorize(string $ability, array $context = []): AuthorizationDecision + { + if ($ability !== self::ABILITY) { + return AuthorizationDecision::deny(500, 'authorization_ability_not_supported'); + } + + if ((int) ($context['actor_user_id'] ?? 0) <= 0) { + return AuthorizationDecision::deny(403, 'forbidden'); + } + + return AuthorizationDecision::allow([ + 'resolver' => $this->dependency->value(), + 'permission_service' => $this->permissionService::class, + ]); + } +} diff --git a/tests/Service/Auth/LdapAuthServiceTest.php b/tests/Service/Auth/LdapAuthServiceTest.php index 6ab69fa..8d35c45 100644 --- a/tests/Service/Auth/LdapAuthServiceTest.php +++ b/tests/Service/Auth/LdapAuthServiceTest.php @@ -299,43 +299,11 @@ class LdapAuthServiceTest extends TestCase return $conn; } - private function createMockSearchResult(): \LDAP\Result + private function createMockSearchResult(): object { - // LDAP\Result can't be mocked directly, but since our gateway wraps - // all ldap_* calls, the actual result object is opaque to LdapAuthService. - // The gateway mock returns it, and we just need a type-compatible value. - // Use a real connection + search if ldap is available; otherwise skip. - if (!extension_loaded('ldap')) { - $this->markTestSkipped('LDAP extension not available'); - } - - // Create a real but unused Result object via reflection workaround. - // Since we mock getEntries(), the actual result content doesn't matter. - // But LDAP\Result has no public constructor, so we use a trick: - // We accept that the gateway returns LDAP\Result|false and our mocks - // already handle this. For this test helper, return a mock-compatible value. - // Since LdapConnectionGateway::search returns \LDAP\Result|false, - // and we mock the gateway, we can use createMock on a stdClass and - // rely on the mock returning it. But PHPUnit mock of LdapConnectionGateway - // allows any return value when configured via willReturn(). - // So we just need any object - the gateway mock will return it to the service, - // and the service passes it back to gateway->getEntries(). - $conn = ldap_connect('ldap://127.0.0.1:1'); - if ($conn === false) { - $this->markTestSkipped('Could not create LDAP connection'); - } - - // We can't get a real LDAP\Result without a real server, but since - // the service only passes it to the mocked gateway, we can create - // a test double. PHPUnit allows returning any value from willReturn(). - // Let's use a simple approach: the gateway mock already handles this. - // For the type system, we use an anonymous class that extends nothing - // but the mock system handles the actual passing. - - // Actually, since we mock LdapConnectionGateway::search() to return - // this value, and LdapConnectionGateway::getEntries() is also mocked, - // the value just needs to be truthy (not false). - // We can use createStub to get a compatible value. - return $this->createStub(\LDAP\Result::class); + // The service treats the search handle as opaque and forwards it to + // gateway->getEntries(); in tests the gateway is mocked, so any object + // is sufficient and avoids doubling final internal LDAP classes. + return new \stdClass(); } } diff --git a/tests/Unit/Module/ModuleEventDispatcherTest.php b/tests/Unit/Module/ModuleEventDispatcherTest.php index d8f102c..fd2351e 100644 --- a/tests/Unit/Module/ModuleEventDispatcherTest.php +++ b/tests/Unit/Module/ModuleEventDispatcherTest.php @@ -5,6 +5,8 @@ namespace MintyPHP\Tests\Unit\Module; use MintyPHP\App\AppContainer; use MintyPHP\App\Module\Contracts\EventListener; use MintyPHP\App\Module\ModuleEventDispatcher; +use MintyPHP\Http\RequestContext; +use MintyPHP\Service\Audit\SystemAuditService; use PHPUnit\Framework\TestCase; final class ModuleEventDispatcherTest extends TestCase @@ -62,6 +64,8 @@ final class ModuleEventDispatcherTest extends TestCase public function testDispatchContinuesAfterListenerException(): void { + RequestContext::resetForTests(); + $secondCalled = false; $failingListener = new class () implements EventListener { @@ -100,4 +104,64 @@ final class ModuleEventDispatcherTest extends TestCase self::assertTrue($secondCalled, 'Second listener should be called even after the first one throws'); } + + public function testDispatchAuditsListenerFailureWithoutPayloadLeakage(): void + { + RequestContext::resetForTests(); + + $failingListener = new class () implements EventListener { + public function handle(string $event, array $payload): void + { + throw new \RuntimeException('Do not leak this message'); + } + }; + + $container = new AppContainer(); + $container->set($failingListener::class, static fn () => $failingListener); + + $systemAuditService = $this->createMock(SystemAuditService::class); + $systemAuditService->expects($this->once()) + ->method('record') + ->with( + 'module.listener.failed', + 'failed', + $this->callback(static function (array $context): bool { + $metadata = is_array($context['metadata'] ?? null) ? $context['metadata'] : []; + $keys = array_keys($metadata); + sort($keys); + + if ($keys !== ['exception_class', 'listener_class', 'listener_method', 'module_event', 'request_id']) { + return false; + } + + if (($metadata['module_event'] ?? '') !== 'user.created') { + return false; + } + + if (($metadata['exception_class'] ?? '') !== \RuntimeException::class) { + return false; + } + + if (($metadata['listener_method'] ?? '') !== 'handle') { + return false; + } + + if (!is_string($metadata['request_id'] ?? null) || trim((string) $metadata['request_id']) === '') { + return false; + } + + return !array_key_exists('payload', $metadata) + && !array_key_exists('exception_message', $metadata) + && !array_key_exists('trace', $metadata); + }) + ); + + $dispatcher = new ModuleEventDispatcher( + ['user.created' => [['class' => $failingListener::class, 'method' => 'handle']]], + $container, + $systemAuditService + ); + + $dispatcher->dispatch('user.created', ['secret' => 'must-not-be-logged']); + } }