Преглед изворни кода

fix: enforce list<string> shape in RoleMappingRepository (SEC_REVIEW F51)

`resolveRole()` builds the IN clause as `count($groupIds)`
placeholders concatenated with `?, ?, …`, then binds
`array_values($groupIds)`. The PHPDoc declares `list<string>` but
PHP doesn't enforce that at runtime. A future caller that passed a
mixed array (PHPDoc lies, callsite pushed an int / null / bool by
mistake) would break the placeholder count → bind-list invariant —
DBAL would either bind the wrong types into a string column or
mismatch placeholder/bind counts.

Add `array_values(array_filter($groupIds, 'is_string'))` as the
first step. The filter discards non-strings; `array_values`
re-keys to a clean 0..n-1 list so a hash with non-contiguous keys
also normalises into a list before the IN clause is built. After
filtering, the empty case correctly falls back to the default role.

Regression tests in
`api/tests/Integration/Auth/RoleMappingRepositoryTest.php`:

  - happy-path: highest role wins across two matching mappings.
  - default-fallback: unmapped group returns the default.
  - empty list: returns default.
  - mixed array (`['group-admin', 42, null, true, ['nested']]`)
    filters to the single valid string and resolves to Admin.
  - all-non-string: collapses to default.
  - hash with gaps (`[5 => 'group-operator', 12 => 'unmapped']`)
    re-keys cleanly and resolves to Operator.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa пре 3 дана
родитељ
комит
9c0fef58d2

+ 10 - 1
api/src/Infrastructure/Auth/RoleMappingRepository.php

@@ -24,6 +24,15 @@ final class RoleMappingRepository
      */
     public function resolveRole(array $groupIds, Role $default): Role
     {
+        // SEC_REVIEW F51: enforce the declared `list<string>` shape at
+        // runtime before building the IN-clause. The static type is
+        // PHPDoc-only; nothing stops a future caller from passing a
+        // mixed array (or a hash with skipped indexes) and breaking
+        // the placeholder count → bind-list invariant. `array_values`
+        // re-keys to a clean 0..n-1 list; `array_filter` drops any
+        // non-string entry. Matches the declared contract explicitly
+        // so the SQL row count and the bind list always agree.
+        $groupIds = array_values(array_filter($groupIds, 'is_string'));
         if ($groupIds === []) {
             return $default;
         }
@@ -32,7 +41,7 @@ final class RoleMappingRepository
         /** @var list<array<string, mixed>> $rows */
         $rows = $this->connection->fetchAllAssociative(
             sprintf('SELECT role FROM oidc_role_mappings WHERE group_id IN (%s)', $placeholders),
-            array_values($groupIds)
+            $groupIds
         );
 
         if ($rows === []) {

+ 106 - 0
api/tests/Integration/Auth/RoleMappingRepositoryTest.php

@@ -0,0 +1,106 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Integration\Auth;
+
+use App\Domain\Auth\Role;
+use App\Infrastructure\Auth\RoleMappingRepository;
+use App\Tests\Integration\Support\AppTestCase;
+
+/**
+ * SEC_REVIEW F51 — `RoleMappingRepository::resolveRole` builds an IN
+ * clause from `count($groupIds)` placeholders. The PHPDoc declares
+ * `list<string>` but PHP doesn't enforce that at runtime; a future
+ * caller could pass a mixed array and break the placeholder/bind
+ * invariant. The repository now `array_filter`s to strings and
+ * re-keys with `array_values` before generating placeholders.
+ */
+final class RoleMappingRepositoryTest extends AppTestCase
+{
+    public function testResolvesHighestRoleAcrossMatchingGroups(): void
+    {
+        $this->seedMapping('group-viewer', Role::Viewer);
+        $this->seedMapping('group-admin', Role::Admin);
+
+        $repo = new RoleMappingRepository($this->db);
+        self::assertSame(
+            Role::Admin,
+            $repo->resolveRole(['group-viewer', 'group-admin'], Role::Viewer),
+        );
+    }
+
+    public function testFallsBackToDefaultWhenNoGroupMatches(): void
+    {
+        $repo = new RoleMappingRepository($this->db);
+        self::assertSame(
+            Role::Viewer,
+            $repo->resolveRole(['unmapped-group'], Role::Viewer),
+        );
+    }
+
+    public function testEmptyListReturnsDefault(): void
+    {
+        $repo = new RoleMappingRepository($this->db);
+        self::assertSame(Role::Viewer, $repo->resolveRole([], Role::Viewer));
+    }
+
+    public function testNonStringEntriesAreFilteredOut(): void
+    {
+        // SEC_REVIEW F51: a mixed array (e.g. PHPDoc lied, callsite
+        // pushed an int / null / bool by mistake) must not crash the
+        // placeholder math or change the result. The non-string
+        // entries are discarded; the remaining strings drive the
+        // lookup.
+        $this->seedMapping('group-admin', Role::Admin);
+
+        $repo = new RoleMappingRepository($this->db);
+        /** @var list<string> $groups @phpstan-ignore-line — exercising the runtime guard */
+        $groups = ['group-admin', 42, null, true, ['nested']];
+
+        self::assertSame(
+            Role::Admin,
+            $repo->resolveRole($groups, Role::Viewer),
+        );
+    }
+
+    public function testAllNonStringEntriesFallsBackToDefault(): void
+    {
+        // After filtering, the list is empty — caller meant something
+        // but typed nothing usable; treat as "no groups → default".
+        $repo = new RoleMappingRepository($this->db);
+        /** @var list<string> $groups @phpstan-ignore-line — exercising the runtime guard */
+        $groups = [42, null, true, ['nested']];
+
+        self::assertSame(
+            Role::Viewer,
+            $repo->resolveRole($groups, Role::Viewer),
+        );
+    }
+
+    public function testHashWithGapsInIndicesIsAccepted(): void
+    {
+        // Another PHPDoc-lies case: caller passed a hash whose keys
+        // aren't 0..n-1. `array_values(array_filter(...))` re-keys
+        // before the placeholder/bind invariant is built.
+        $this->seedMapping('group-operator', Role::Operator);
+
+        $repo = new RoleMappingRepository($this->db);
+        /** @var list<string> $groups @phpstan-ignore-line — exercising the runtime guard */
+        $groups = [5 => 'group-operator', 12 => 'unmapped'];
+
+        self::assertSame(
+            Role::Operator,
+            $repo->resolveRole($groups, Role::Viewer),
+        );
+    }
+
+    private function seedMapping(string $groupId, Role $role): void
+    {
+        $this->db->insert('oidc_role_mappings', [
+            'group_id' => $groupId,
+            'role' => $role->value,
+            'created_at' => (new \DateTimeImmutable('now', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'),
+        ]);
+    }
+}