Prechádzať zdrojové kódy

fix: validate job name regex before audit emit (SEC_REVIEW F44)

`JobsAdminController::trigger()` accepted whatever string the route's
`{name}` segment captured and let `registry->has($name)` gate the
real work. The Slim default segment regex `[^/]+` only protects
against `/`; uppercase, dots, control characters, brackets, and
url-encoded bytes all flow through. Today the registry's exact-
string match catches them, but a future refactor that turns
`has()` permissive on trim / url-decode / case-folding would let
the audit-emit path fire BEFORE the registry rejected the call —
either creating a forged `job.triggered` row in `audit_log` (the
audit emits before the run) or injecting CR/LF into the audit's
JSON detail blob.

Add `JobsAdminController::JOB_NAME_PATTERN = '/^[a-z0-9_-]+$/'` and
gate `trigger()` on it as the first thing it does, returning the
same 404 `unknown_job` envelope used for the missing-job branch.
The check runs BEFORE `registry->has()` and BEFORE the audit emit,
so any malformed `{name}` 404s with zero side effects.

Regression tests in
`api/tests/Integration/Admin/JobsAdminControllerTest.php`:
`testTriggerRejectsMalformedJobName` data-provider covers uppercase
(`Recompute-Scores`), dotted (`recompute.scores`), space, CR / LF
injection, bracket (`recompute[scores]`), percent (`recompute%20scores`),
and `..`. Each case must 404 AND leave zero `job.triggered` rows
in `audit_log` — the F44 forged-audit concern measured directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 3 dní pred
rodič
commit
1a705f6b64

+ 14 - 0
api/src/Application/Admin/JobsAdminController.php

@@ -84,12 +84,26 @@ final class JobsAdminController
         ]);
     }
 
+    /**
+     * Job names allowed by the controller, regardless of what the
+     * registry would return for `has()`. SEC_REVIEW F44: validate the
+     * `{name}` path segment against this character class *before* it
+     * flows into the registry lookup or the `job.triggered` audit
+     * row, so a future refactor that turns `has()` permissive on
+     * trim / url-decode / case-folding cannot escalate the route
+     * into log injection or forged audit entries.
+     */
+    private const JOB_NAME_PATTERN = '/^[a-z0-9_-]+$/';
+
     /**
      * @param array{name: string} $args
      */
     public function trigger(ServerRequestInterface $request, ResponseInterface $response, array $args): ResponseInterface
     {
         $name = $args['name'];
+        if (preg_match(self::JOB_NAME_PATTERN, $name) !== 1) {
+            return self::error($response, 404, 'unknown_job');
+        }
         if (!$this->registry->has($name)) {
             return self::error($response, 404, 'unknown_job');
         }

+ 37 - 0
api/tests/Integration/Admin/JobsAdminControllerTest.php

@@ -54,6 +54,43 @@ final class JobsAdminControllerTest extends AppTestCase
         self::assertSame(404, $resp->getStatusCode());
     }
 
+    /**
+     * @return iterable<string, array{string}>
+     */
+    public static function malformedJobNames(): iterable
+    {
+        // SEC_REVIEW F44: any name outside `^[a-z0-9_-]+$` must 404
+        // BEFORE flowing into `registry->has()` or the `job.triggered`
+        // audit row. The Slim default segment regex `[^/]+` only
+        // protects against `/`; everything else (uppercase, dots,
+        // spaces, control chars, brackets) needs the controller gate.
+        yield 'uppercase' => ['Recompute-Scores'];
+        yield 'dotted' => ['recompute.scores'];
+        yield 'space' => ['recompute scores'];
+        yield 'newline injection' => ["recompute\nfaked"];
+        yield 'cr injection' => ["recompute\rfaked"];
+        yield 'bracket' => ['recompute[scores]'];
+        yield 'percent' => ['recompute%20scores'];
+        yield 'empty after url decode' => ['..'];
+    }
+
+    #[\PHPUnit\Framework\Attributes\DataProvider('malformedJobNames')]
+    public function testTriggerRejectsMalformedJobName(string $name): void
+    {
+        $token = $this->createToken(TokenKind::Admin, Role::Admin);
+        $resp = $this->request(
+            'POST',
+            '/api/v1/admin/jobs/trigger/' . rawurlencode($name),
+            ['Authorization' => 'Bearer ' . $token],
+        );
+        self::assertSame(404, $resp->getStatusCode(), "expected 404 for malformed name '{$name}'");
+        // Critically: no audit row is emitted for the malformed name.
+        $audit = $this->db->fetchOne(
+            "SELECT COUNT(*) FROM audit_log WHERE action = 'job.triggered'"
+        );
+        self::assertSame(0, (int) $audit, "no audit row for malformed name '{$name}'");
+    }
+
     public function testTriggerRecomputeRunsAndAuditsAsManual(): void
     {
         $token = $this->createToken(TokenKind::Admin, Role::Admin);