diff --git a/core/Support/helpers/action_context.php b/core/Support/helpers/action_context.php index 246550d..4b76ea6 100644 --- a/core/Support/helpers/action_context.php +++ b/core/Support/helpers/action_context.php @@ -238,6 +238,15 @@ function actionFragmentResolveOrStatus( * Two-Level-Authorize (CONTEXT + SUBMIT, used by Roles/Permissions) is NOT * built in — actions that need it call the building blocks directly. * + * Forward-compatibility pattern: this aggregator's signature uses an assoc + * array $args to permit additive Step 2+ extension (e.g. notFoundFlashScopeKey + * override added with the Departments-Edit pilot in Run + * 2026-04-25-action-context-rollout-step2-departments). Building blocks + * (actionResolveModelOrFail et al.) remain frozen by + * ActionContextHelperContractTest::testBuildingBlocksExistWithStableSignatures; + * caller-specific behavior must be expressed via additive aggregator args, not + * by widening building-block signatures. + * * Required keys in $args: * - finder: callable(string): mixed * - rawId: string @@ -251,6 +260,14 @@ function actionFragmentResolveOrStatus( * - tenantScopeFlagKey: string * - tenantScopeListKey: string * - canViewPageFlagKey: string (default 'can_view_page') + * - notFoundFlashScopeKey: string|null — when provided, the aggregator + * inlines Flash::error(t($notFoundFlashKey), $notFoundRedirectPath, + * $notFoundFlashScopeKey) + Router::redirect instead of delegating to + * actionResolveModelOrFail (which hardcodes scope-key 'not_found'). Use + * this to preserve a domain-specific dedup-scope-key (e.g. + * 'department_not_found') without widening the frozen building-block + * signature. The scope-key is a flash-dedup identifier only — it must + * not contain PII (GR-SEC-002). * * @param array $args * @return array{model: mixed, capabilities: array, tenantScope: array{scope: 'all'|'list', ids: list}, viewAuth: array{page: array}} @@ -268,8 +285,24 @@ function actionEditContext(array $args): array $tenantScopeFlagKey = (string) ($args['tenantScopeFlagKey'] ?? 'can_manage_all_tenants'); $tenantScopeListKey = (string) ($args['tenantScopeListKey'] ?? 'allowed_tenant_ids'); $canViewPageFlagKey = (string) ($args['canViewPageFlagKey'] ?? 'can_view_page'); + $notFoundFlashScopeKey = $args['notFoundFlashScopeKey'] ?? null; + $notFoundFlashScopeKey = is_string($notFoundFlashScopeKey) && $notFoundFlashScopeKey !== '' + ? $notFoundFlashScopeKey + : null; - $model = actionResolveModelOrFail($finder, $rawId, $notFoundFlashKey, $notFoundRedirectPath); + if ($notFoundFlashScopeKey !== null) { + $model = is_callable($finder) ? $finder($rawId) : null; + if ($model === null) { + Flash::error(t($notFoundFlashKey), $notFoundRedirectPath, $notFoundFlashScopeKey); + Router::redirect($notFoundRedirectPath); + // Router::redirect() calls die() in production. Guard for tests + // where executeRedirect=false: caller MUST not rely on this being + // reached — return value mirrors the building-block fallthrough. + $model = null; + } + } else { + $model = actionResolveModelOrFail($finder, $rawId, $notFoundFlashKey, $notFoundRedirectPath); + } $capabilities = actionAuthorizeAndExtractCapabilities($abilityKey, $context, $forbiddenStrategy); $tenantScope = actionDeriveTenantScope($capabilities, $tenantScopeFlagKey, $tenantScopeListKey); actionEnforceCanViewPage($capabilities, $canViewPageFlagKey, $forbiddenStrategy); diff --git a/pages/admin/departments/edit($id).php b/pages/admin/departments/edit($id).php index 064633b..4db9fdb 100644 --- a/pages/admin/departments/edit($id).php +++ b/pages/admin/departments/edit($id).php @@ -24,41 +24,37 @@ $directoryScopeGateway = app(\MintyPHP\Service\Tenant\TenantScopeService::class) $uuid = trim((string) ($id ?? '')); $editTarget = requestPathWithReturnTarget("admin/departments/edit/{$uuid}", $returnTarget); + +// Resolve the department BEFORE the aggregator so the authorize-context can +// carry the real target_department_id (matches today's order: lookup → CONTEXT +// authorize → can_view_page). The aggregator receives a trivial finder that +// returns the already-loaded model so its not-found branch fires only when +// findByUuid returned null. $department = $uuid !== '' ? $departmentService->findByUuid($uuid) : null; -if (!$department) { - Flash::error('Department not found', $closeTarget, 'department_not_found'); - Router::redirect($closeTarget); -} - $departmentId = (int) ($department['id'] ?? 0); -$contextDecision = $authorizationService->authorize(DepartmentAuthorizationPolicy::ABILITY_ADMIN_DEPARTMENTS_EDIT_CONTEXT, [ - 'actor_user_id' => $currentUserId, - 'target_department_id' => $departmentId, -]); -if (!$contextDecision->isAllowed()) { - Router::redirect('error/forbidden'); - return; -} -$capabilities = $contextDecision->attribute('capabilities', []); -if (!is_array($capabilities)) { - $capabilities = []; -} -$canViewPage = (bool) ($capabilities['can_view_page'] ?? false); +$context = actionEditContext([ + 'finder' => static fn (string $_id): mixed => $department, + 'rawId' => $uuid, + 'notFoundFlashKey' => 'Department not found', + 'notFoundRedirectPath' => $closeTarget, + 'notFoundFlashScopeKey' => 'department_not_found', + 'abilityKey' => DepartmentAuthorizationPolicy::ABILITY_ADMIN_DEPARTMENTS_EDIT_CONTEXT, + 'context' => [ + 'actor_user_id' => $currentUserId, + 'target_department_id' => $departmentId, + ], + 'viewAuthFlags' => ['can_update_department', 'can_delete_department'], + 'forbiddenStrategy' => 'redirect', +]); +$department = is_array($context['model']) ? $context['model'] : $department; +$capabilities = $context['capabilities']; +$tenantScope = $context['tenantScope']; +$viewAuth = $context['viewAuth']; $canUpdateDepartment = (bool) ($capabilities['can_update_department'] ?? false); $canDeleteDepartment = (bool) ($capabilities['can_delete_department'] ?? false); -$viewAuth['page'] = [ - 'can_update_department' => $canUpdateDepartment, - 'can_delete_department' => $canDeleteDepartment, -]; -$allowedTenantIdsRaw = $capabilities['allowed_tenant_ids'] ?? []; -$allowedTenantIds = is_array($allowedTenantIdsRaw) - ? array_values(array_unique(array_filter(array_map('intval', $allowedTenantIdsRaw), static fn (int $tenantId): bool => $tenantId > 0))) - : []; -if (!$canViewPage) { - Router::redirect('error/forbidden'); - return; -} +$canManageAllTenants = $tenantScope['scope'] === 'all'; +$allowedTenantIds = $tenantScope['ids']; app(\MintyPHP\Service\Audit\AuditMetadataEnricherInterface::class)->enrich($department); @@ -67,7 +63,9 @@ $errors = []; $warnings = []; $form = $department; $tenants = $tenantService->list(); -if ($allowedTenantIds) { +if ($canManageAllTenants) { + // No filtering — actor may pick any tenant. +} elseif ($allowedTenantIds) { $tenants = array_values(array_filter($tenants, static function (array $tenant) use ($allowedTenantIds): bool { $tenantId = (int) ($tenant['id'] ?? 0); return $tenantId > 0 && in_array($tenantId, $allowedTenantIds, true); @@ -110,7 +108,9 @@ if ($request->isMethod('POST')) { } $selectedTenantId = $request->bodyInt('tenant_id'); - if ($allowedTenantIds) { + if ($canManageAllTenants) { + // No restriction — submitted tenant_id passes through. + } elseif ($allowedTenantIds) { $selectedTenantId = in_array($selectedTenantId, $allowedTenantIds, true) ? $selectedTenantId : 0; } elseif ($directoryScopeGateway->isStrict()) { $selectedTenantId = 0; @@ -134,13 +134,13 @@ if ($request->isMethod('POST')) { if ($warnings) { Flash::info(implode(' ', $warnings), $closeTarget, 'department_warning'); } - Flash::success('Department updated', $closeTarget, 'department_updated'); + Flash::success(t('Department updated'), $closeTarget, 'department_updated'); Router::redirect($closeTarget); } else { if ($warnings) { Flash::info(implode(' ', $warnings), $editTarget, 'department_warning'); } - Flash::success('Department updated', $editTarget, 'department_updated'); + Flash::success(t('Department updated'), $editTarget, 'department_updated'); Router::redirect($editTarget); } } diff --git a/tests/Architecture/ActionContextCsrfPairingContractTest.php b/tests/Architecture/ActionContextCsrfPairingContractTest.php new file mode 100644 index 0000000..ed23f2f --- /dev/null +++ b/tests/Architecture/ActionContextCsrfPairingContractTest.php @@ -0,0 +1,257 @@ +method()', + 'hasBody(', + 'bodyAll()', + 'bodyInt(', + 'bodyString(', + ]; + + /** + * Body-access indicators. These must NOT happen before + * actionRequireCsrf(). isMethod('POST')/method() are guards, not body + * accesses, so they're excluded here — guards may legitimately come + * before the CSRF call (in the very same line as actionRequireCsrf). + */ + private const BODY_ACCESS_INDICATORS = [ + 'hasBody(', + 'bodyAll()', + 'bodyInt(', + 'bodyString(', + ]; + + public function testActionsUsingAggregatorsHaveCsrfBeforeAggregator(): void + { + $root = $this->projectRootPath(); + + $missing = []; + $unverifiable = []; + $orderViolations = []; + + foreach (['pages', 'modules'] as $dir) { + $base = $root . '/' . $dir; + if (!is_dir($base)) { + continue; + } + + $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($base, FilesystemIterator::SKIP_DOTS)); + /** @var \SplFileInfo $file */ + foreach ($iterator as $file) { + if (!$file->isFile() || $file->getExtension() !== 'php') { + continue; + } + + $content = (string) file_get_contents($file->getPathname()); + $aggregatorOffsets = $this->locateAggregatorCalls($content); + if ($aggregatorOffsets === []) { + continue; + } + + $relativePath = str_replace($root . '/', '', $file->getPathname()); + + // Non-static-extractable aggregator pattern (heredoc, eval, …). + if ($aggregatorOffsets === false) { + $unverifiable[] = $relativePath; + continue; + } + + $isPostHandler = $this->hasAny($content, self::POST_PRESENCE_INDICATORS) + || preg_match('/->body\(/', $content) === 1; + + // Drawer-fragment allowlist: actionFragmentContext-only callers + // without any POST indicator are GET-only and don't need CSRF. + if (!$isPostHandler) { + continue; + } + + $csrfOffset = $this->locateActionRequireCsrf($content); + if ($csrfOffset === null) { + $missing[] = $relativePath; + continue; + } + + $bodyAccessOffset = $this->locateFirstBodyAccess($content); + if ($bodyAccessOffset !== null && $csrfOffset >= $bodyAccessOffset) { + $orderViolations[] = sprintf( + '%s — actionRequireCsrf() at offset %d but POST body access at offset %d (CSRF must precede body access)', + $relativePath, + $csrfOffset, + $bodyAccessOffset + ); + } + } + } + + sort($missing); + sort($unverifiable); + sort($orderViolations); + + $messages = []; + if ($missing !== []) { + $messages[] = "Aggregator-callers handling POST without actionRequireCsrf():\n " . implode("\n ", $missing); + } + if ($orderViolations !== []) { + $messages[] = "Aggregator-callers with actionRequireCsrf() AFTER body access:\n " . implode("\n ", $orderViolations); + } + if ($unverifiable !== []) { + $messages[] = "Cannot verify CSRF pairing — review manually:\n " . implode("\n ", $unverifiable); + } + + $this->assertSame( + [], + $messages, + "GR-SEC-001 / aggregator pairing violations:\n" . implode("\n", $messages) + ); + } + + /** + * @return list|false list of byte offsets of aggregator calls; + * false when at least one match is wrapped in a + * heredoc/string literal and cannot be statically + * verified. + */ + private function locateAggregatorCalls(string $content): array|false + { + $offsets = []; + foreach (self::AGGREGATORS as $name) { + $needle = $name . '('; + $pos = 0; + while (($found = strpos($content, $needle, $pos)) !== false) { + if ($this->isInsideStringLiteral($content, $found)) { + return false; + } + $offsets[] = $found; + $pos = $found + strlen($needle); + } + } + sort($offsets); + return $offsets; + } + + private function locateFirstBodyAccess(string $content): ?int + { + $earliest = null; + foreach (self::BODY_ACCESS_INDICATORS as $needle) { + $pos = strpos($content, $needle); + if ($pos !== false && ($earliest === null || $pos < $earliest)) { + $earliest = $pos; + } + } + if (preg_match('/->body\(/', $content, $m, PREG_OFFSET_CAPTURE) === 1) { + $pos = (int) $m[0][1]; + if ($earliest === null || $pos < $earliest) { + $earliest = $pos; + } + } + return $earliest; + } + + private function locateActionRequireCsrf(string $content): ?int + { + $pos = strpos($content, 'actionRequireCsrf('); + return $pos === false ? null : $pos; + } + + /** + * @param list $needles + */ + private function hasAny(string $content, array $needles): bool + { + foreach ($needles as $needle) { + if (str_contains($content, $needle)) { + return true; + } + } + return false; + } + + /** + * Heuristic: a match is considered inside a heredoc/string literal when + * the immediately preceding non-whitespace character is a backtick or an + * unescaped quote on the same line. Conservative — favours raising + * "review manually" over silent pass. + */ + private function isInsideStringLiteral(string $content, int $offset): bool + { + if ($offset <= 0) { + return false; + } + // Walk back to start of line. + $lineStart = strrpos(substr($content, 0, $offset), "\n"); + $lineStart = $lineStart === false ? 0 : $lineStart + 1; + $prefix = substr($content, $lineStart, $offset - $lineStart); + + // Count unescaped single/double quotes on the line so far. + $singles = 0; + $doubles = 0; + $len = strlen($prefix); + for ($i = 0; $i < $len; $i++) { + $ch = $prefix[$i]; + if ($ch === '\\') { + $i++; + continue; + } + if ($ch === "'") { + $singles++; + } elseif ($ch === '"') { + $doubles++; + } + } + // Odd counts → currently inside that quote type. + if (($singles % 2) === 1 || ($doubles % 2) === 1) { + return true; + } + // Heredoc detection — simplistic: if the line above ended with <<assertTrue(function_exists('actionFragmentContext')); } - public function testNoProductionCallSitesYet(): void - { - $root = $this->projectRootPath(); - $names = [ - 'actionResolveModelOrFail', - 'actionAuthorizeAndExtractCapabilities', - 'actionDeriveTenantScope', - 'actionEnforceCanViewPage', - 'actionBuildViewAuth', - 'actionFragmentResolveOrStatus', - 'actionEditContext', - 'actionCreateContext', - 'actionFragmentContext', - ]; - - $hits = []; - foreach (['pages', 'modules'] as $dir) { - $base = $root . '/' . $dir; - if (!is_dir($base)) { - continue; - } - $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($base, \FilesystemIterator::SKIP_DOTS)); - /** @var \SplFileInfo $file */ - foreach ($iterator as $file) { - if (!$file->isFile() || $file->getExtension() !== 'php') { - continue; - } - $content = (string) file_get_contents($file->getPathname()); - foreach ($names as $name) { - if (preg_match('/\b' . preg_quote($name, '/') . '\s*\(/', $content)) { - $hits[] = $name . ' in ' . str_replace($root . '/', '', $file->getPathname()); - } - } - } - } - - $this->assertSame( - [], - $hits, - "Step 1 must not introduce production call-sites — defer to Step 2 (Departments-Edit pilot):\n" . implode("\n", $hits) - ); - } - /** * Ensure the `mixed` return type of building blocks is preserved where * documented. Catches accidental tightening that would break Step 2. diff --git a/tests/Support/Helpers/ActionContextHelperTest.php b/tests/Support/Helpers/ActionContextHelperTest.php index 938f95f..87f6f50 100644 --- a/tests/Support/Helpers/ActionContextHelperTest.php +++ b/tests/Support/Helpers/ActionContextHelperTest.php @@ -335,6 +335,83 @@ class ActionContextHelperTest extends TestCase $this->assertStringContainsString('admin/users', $this->capturedRedirect()); } + public function testEditContextWithFlashScopeKeyOverride(): void + { + // Departments-Edit pilot use-case: aggregator must emit a flash with + // the caller-specific scope-key ('department_not_found') instead of + // the building-block default ('not_found') when notFoundFlashScopeKey + // is provided. Required for Step 2 behavior identity (Run + // 2026-04-25-action-context-rollout-step2-departments). + $this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow([ + 'capabilities' => ['can_view_page' => true], + ])); + $_SESSION['flash_messages'] = []; + + actionEditContext([ + 'finder' => static fn (): mixed => null, + 'rawId' => 'missing', + 'notFoundFlashKey' => 'Department not found', + 'notFoundRedirectPath' => 'admin/departments', + 'notFoundFlashScopeKey' => 'department_not_found', + 'abilityKey' => 'ABILITY_DEPARTMENTS_EDIT_CONTEXT', + 'context' => [], + 'viewAuthFlags' => [], + ]); + + $this->assertStringContainsString('admin/departments', $this->capturedRedirect()); + + $entry = $this->findFlashEntryByKey('department_not_found'); + $this->assertNotNull($entry, "Expected flash with key 'department_not_found' to be set by aggregator override."); + $this->assertSame('error', $entry['type'] ?? null); + $this->assertSame('admin/departments', $entry['scope'] ?? null); + } + + public function testEditContextWithoutFlashScopeKeyKeepsBuildingBlockDefault(): void + { + // Without notFoundFlashScopeKey, the aggregator delegates to + // actionResolveModelOrFail which hardcodes scope-key 'not_found'. + // This test pins the default-path behavior so a future addition + // doesn't accidentally widen building-block semantics. + $this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow([ + 'capabilities' => ['can_view_page' => true], + ])); + $_SESSION['flash_messages'] = []; + + actionEditContext([ + 'finder' => static fn (): mixed => null, + 'rawId' => 'missing', + 'notFoundFlashKey' => 'action.context.model_not_found', + 'notFoundRedirectPath' => 'admin/users', + 'abilityKey' => 'ABILITY_USERS_EDIT_CONTEXT', + 'context' => [], + 'viewAuthFlags' => [], + ]); + + $entry = $this->findFlashEntryByKey('not_found'); + $this->assertNotNull($entry, "Expected default building-block scope-key 'not_found' when notFoundFlashScopeKey is absent."); + } + + /** + * @return array|null + */ + private function findFlashEntryByKey(string $key): ?array + { + /** @var mixed $rawMessages */ + $rawMessages = $_SESSION['flash_messages'] ?? []; + if (!is_array($rawMessages)) { + return null; + } + foreach ($rawMessages as $entry) { + if (!is_array($entry)) { + continue; + } + if (($entry['key'] ?? null) === $key) { + return $entry; + } + } + return null; + } + /* ---------------------------------------------------------------- * Aggregator: actionCreateContext * ---------------------------------------------------------------- */