Przeglądaj źródła

fix: enforce role allowlist on UI policy proxies (SEC_REVIEW F42)

`PoliciesController::previewProxy` and `scoreDistributionProxy` only
checked `getUser() === null` before forwarding to the api with the
session user's id as `X-Acting-User-Id`. The api was the single gate
on role: it admits Viewer-or-higher to the underlying preview /
score-distribution endpoints. If a future api change ever narrows
that gate (e.g. Viewer-no-longer-allowed) or a session somehow ended
up with a `none` role inside the `/app/*` route group, the UI would
silently forward to the api and let the api's 403 surface as a
generic 502 from the proxy — no clean local denial, and the api
served as a probe target.

Add a `PROXY_ALLOWED_ROLES = ['viewer', 'operator', 'admin']`
constant and reject any session role outside that set with a JSON
403 *before* the api call. The api is not contacted in the rejection
branch (test asserts `apiHistory` stays empty), so an unmapped
session can't use the proxy to enumerate or probe. The role list
mirrors the api's current expectation; if either side changes the
two will need to be aligned, and the local test suite will surface
that as a 403 unexpectedly applied to a previously-allowed role.

`AuthRequiredMiddleware` still intercepts truly-anonymous requests
earlier in the chain (302 → /login), so the controller's own 401
branch is the defence-in-depth fallback for any future route
reshuffle that pulls the proxy out of `/app/*`. Both branches stay
in source so the fail-closed posture is explicit at every layer.

Regression tests in
`ui/tests/Integration/Auth/PoliciesProxyTest.php` cover:

  - anonymous → AuthRequired's 302 (the actual outer gate),
  - role=none → 403 + zero api calls (the F42 hole),
  - score-distribution-proxy mirror,
  - viewer / operator / admin → 200 (api forwarded),
  - unknown role string → 403,
  - empty role → 403.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 dni temu
rodzic
commit
cc77749fca

+ 22 - 0
ui/src/Controllers/PoliciesController.php

@@ -50,6 +50,18 @@ final class PoliciesController
         return $this->sessionManager;
     }
 
+    /**
+     * The api gates `GET /api/v1/admin/policies/{id}/preview` and
+     * `…/score-distribution` to Viewer-or-higher (i.e. not `none` /
+     * unmapped). SEC_REVIEW F42: enforce the same expectation locally
+     * so a UI session that drifts (e.g. the api role mapping changes
+     * to deny these endpoints to Viewer) doesn't silently forward the
+     * call until the api returns 403 — and so a `none`-role session
+     * that somehow reached the protected route group cannot use the
+     * proxy as a probe channel.
+     */
+    private const PROXY_ALLOWED_ROLES = ['viewer', 'operator', 'admin'];
+
     /**
      * Browser-side preview proxy: the policy edit page's Alpine
      * component fetches `/app/policies/{id}/preview-proxy`; this
@@ -66,6 +78,11 @@ final class PoliciesController
 
             return $response->withStatus(401)->withHeader('Content-Type', 'application/json');
         }
+        if (!$this->userIs($user, ...self::PROXY_ALLOWED_ROLES)) {
+            $response->getBody()->write((string) json_encode(['error' => 'forbidden']));
+
+            return $response->withStatus(403)->withHeader('Content-Type', 'application/json');
+        }
         $id = $this->parseId($args['id']);
         if ($id === null) {
             $response->getBody()->write((string) json_encode(['error' => 'not_found']));
@@ -99,6 +116,11 @@ final class PoliciesController
 
             return $response->withStatus(401)->withHeader('Content-Type', 'application/json');
         }
+        if (!$this->userIs($user, ...self::PROXY_ALLOWED_ROLES)) {
+            $response->getBody()->write((string) json_encode(['error' => 'forbidden']));
+
+            return $response->withStatus(403)->withHeader('Content-Type', 'application/json');
+        }
         $id = $this->parseId($args['id']);
         if ($id === null) {
             $response->getBody()->write((string) json_encode(['error' => 'not_found']));

+ 127 - 0
ui/tests/Integration/Auth/PoliciesProxyTest.php

@@ -0,0 +1,127 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Integration\Auth;
+
+use App\Auth\UserContext;
+use App\Tests\Integration\Support\AppTestCase;
+
+/**
+ * SEC_REVIEW F42 — `/app/policies/{id}/preview-proxy` and
+ * `/app/policies/{id}/score-distribution-proxy` defence-in-depth role
+ * enforcement. The api gates the underlying endpoints to Viewer-or-
+ * higher; this test class proves the UI proxy enforces the same
+ * expectation locally so a `none`-role session that somehow reached
+ * the protected route group cannot use the proxy as a probe channel.
+ */
+final class PoliciesProxyTest extends AppTestCase
+{
+    protected function setUp(): void
+    {
+        $this->bootApp();
+        // First /app/* request in a process triggers session_start();
+        // hit a public route first so subsequent $_SESSION priming sticks.
+        $this->request('GET', '/healthz');
+    }
+
+    public function testPreviewProxyAnonymousIsCaughtByAuthRequiredMiddleware(): void
+    {
+        // The proxy lives under /app/*, so AuthRequiredMiddleware
+        // intercepts an anonymous request and redirects to /login
+        // before the controller runs. The controller's own 401
+        // branch is a defence-in-depth fallback for any future
+        // route reshuffle that takes the proxy out of the /app/*
+        // group.
+        $response = $this->request('GET', '/app/policies/1/preview-proxy');
+        self::assertSame(302, $response->getStatusCode());
+        self::assertSame('/login', $response->getHeaderLine('Location'));
+    }
+
+    public function testPreviewProxyRejectsNoneRoleWith403(): void
+    {
+        // SEC_REVIEW F42: a session with role='none' (or empty) must NOT
+        // reach the api via the proxy. AuthRequiredMiddleware lets the
+        // request through (a non-null user is present), so the controller
+        // is the gate.
+        $this->seedUser(role: 'none');
+
+        $response = $this->request('GET', '/app/policies/1/preview-proxy');
+
+        self::assertSame(403, $response->getStatusCode());
+        self::assertSame('application/json', $response->getHeaderLine('Content-Type'));
+        $body = json_decode((string) $response->getBody(), true);
+        self::assertSame('forbidden', $body['error'] ?? null);
+        // Critically: the api must NOT have been called.
+        self::assertSame([], $this->apiHistory);
+    }
+
+    public function testScoreDistributionProxyRejectsNoneRoleWith403(): void
+    {
+        $this->seedUser(role: 'none');
+
+        $response = $this->request('GET', '/app/policies/1/score-distribution-proxy');
+
+        self::assertSame(403, $response->getStatusCode());
+        self::assertSame([], $this->apiHistory);
+    }
+
+    public function testPreviewProxyAllowsViewer(): void
+    {
+        $this->seedUser(role: 'viewer');
+        $this->enqueueApiResponse(200, ['blocked' => 12, 'allowed' => 88]);
+
+        $response = $this->request('GET', '/app/policies/1/preview-proxy');
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertCount(1, $this->apiHistory);
+    }
+
+    public function testPreviewProxyAllowsOperatorAndAdmin(): void
+    {
+        // Cover both remaining "allowed" roles in one test by issuing
+        // two requests against fresh sessions.
+        foreach (['operator', 'admin'] as $role) {
+            $_SESSION = [];
+            $this->bootApp();
+            $this->request('GET', '/healthz');
+            $this->seedUser(role: $role);
+            $this->enqueueApiResponse(200, ['ok' => true]);
+
+            $response = $this->request('GET', '/app/policies/1/preview-proxy');
+
+            self::assertSame(200, $response->getStatusCode(), "role {$role} should be allowed");
+        }
+    }
+
+    public function testPreviewProxyRejectsUnknownRoleWith403(): void
+    {
+        // A drifted/poisoned session role string the UI doesn't recognise
+        // must fail closed, not silently forward. Defence-in-depth even
+        // though the api would 403 anyway.
+        $this->seedUser(role: 'definitely-not-a-real-role');
+
+        $response = $this->request('GET', '/app/policies/1/preview-proxy');
+
+        self::assertSame(403, $response->getStatusCode());
+        self::assertSame([], $this->apiHistory);
+    }
+
+    public function testPreviewProxyEmptyRoleIsRejected(): void
+    {
+        $this->seedUser(role: '');
+
+        $response = $this->request('GET', '/app/policies/1/preview-proxy');
+
+        self::assertSame(403, $response->getStatusCode());
+        self::assertSame([], $this->apiHistory);
+    }
+
+    private function seedUser(string $role): void
+    {
+        $_SESSION['_user'] = (new UserContext(1, 'Test', $role, null, UserContext::SOURCE_LOCAL))->toArray();
+        $_SESSION['_last_active'] = time();
+        $_SESSION['_authenticated_at'] = time();
+        $_SESSION['_revalidated_at'] = time();
+    }
+}