fix: strip C0/C1 control chars from admin string fields (SEC_REVIEW F52)

Free-form admin string inputs — reporter / consumer / category
`name` and `description`, manual_block / allowlist `reason` — were
accepted with NULs, newlines, ANSI ESC bytes and DEL intact. They
land in `audit_log.target_label` and `details_json`. Twig auto-
escapes for browser display, but a log-pipeline reader sees:

  - `name = "innocent\n[CRIT] fake event from system"` — newline-
    based log injection forging an entry.
  - `reason = "abuse\u{001b}[31malert"` — terminal-escape attack
    that recolours / repositions the cursor when an oncall reads
    the log on a TTY.
  - NUL bytes that truncate strings unexpectedly in C-tooled
    pipelines.

Two new helpers on `AdminControllerSupport`:

  - `stripControlChars(string, $allowNewlines=false): string` —
    `preg_replace`s C0 (0x00..0x1f), DEL (0x7f), and C1
    (0x80..0x9f) out of the string. Removing the ESC lead-in
    neutralises terminal-interpretation attacks even if the
    trailing `[31m` text remains as inert visible bytes.
  - `cleanString(mixed, …): ?string` — convenience wrapper around
    `trim() + stripControlChars()` for the common "single-line
    free-form name/reason" pattern; returns null for non-string
    input so the caller can branch on presence.

Applied at every relevant call site on both create and update paths
in the 5 affected controllers. Slugs are already regex-validated to
a tight charset and need no separate strip. NFC normalisation was
deliberately skipped — the api doesn't require `ext-intl` and
adding the dependency for a defence-in-depth nice-to-have isn't
worth the install-footprint cost. The `details_json` audit blob
inherits the scrub because the controllers feed the cleaned values
into the audit emit, not the raw request body.

Regression tests in
`api/tests/Integration/Admin/InputControlCharStrippingTest.php`:
one POST per controller plus a PATCH on the reporter update path.
Each assertion uses a `preg_match('/[\x00-\x1f\x7f-\x9f]/u', …)`
helper to prove no control byte survives the round-trip. The
reporter case also drills into `audit_log.target_label` and
`details_json` to prove the audit row never sees the raw bytes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

api/src/Application/Admin/AdminControllerSupport.php

@@ -138,4 +138,41 @@ trait AdminControllerSupport
 
         return false;
     }
+
+    /**
+     * SEC_REVIEW F52: strip C0/C1 control characters from a free-form
+     * string field before it's stored or fed into the audit log. NULs,
+     * newlines, ANSI escape sequences and DEL otherwise land in
+     * `audit_log.target_label` and `details_json` — Twig auto-escapes
+     * for browser display, but log-injection (`\n[CRIT] fake event`)
+     * and terminal-escape attacks on log viewers remain.
+     *
+     * Strips 0x00..0x1F (C0) and 0x7F..0x9F (DEL + C1) by default.
+     * Reserved for future multi-line `description` fields: callers
+     * can pass `$allowNewlines=true` to keep CR/LF/tab.
+     */
+    private static function stripControlChars(string $value, bool $allowNewlines = false): string
+    {
+        $pattern = $allowNewlines
+            ? '/[\x00-\x08\x0b-\x1f\x7f-\x9f]/u'
+            : '/[\x00-\x1f\x7f-\x9f]/u';
+
+        return (string) preg_replace($pattern, '', $value);
+    }
+
+    /**
+     * Convenience wrapper for the very common
+     * `trim($body['x']) + strip` pattern on free-form string inputs
+     * (`name`, `reason`, `slug`-shaped fields). Returns `null` for
+     * non-string / null inputs so the caller can branch on
+     * presence; returns the cleaned string otherwise.
+     */
+    private static function cleanString(mixed $value, bool $allowNewlines = false): ?string
+    {
+        if (!is_string($value)) {
+            return null;
+        }
+
+        return self::stripControlChars(trim($value), $allowNewlines);
+    }
 }

api/src/Application/Admin/AllowlistController.php

@@ -90,7 +90,13 @@ final class AllowlistController
             if ($body['reason'] !== null && !is_string($body['reason'])) {
                 $errors['reason'] = 'must be string or null';
             } else {
-                $reason = $body['reason'];
+                // SEC_REVIEW F52: strip C0/C1 control characters before
+                // the reason lands in `audit_log.target_label` /
+                // `details_json` to defeat log-injection and terminal-
+                // escape attacks on log viewers.
+                $reason = is_string($body['reason'])
+                    ? self::stripControlChars($body['reason'])
+                    : null;
             }
         }
 

api/src/Application/Admin/CategoriesController.php

@@ -77,7 +77,10 @@ final class CategoriesController
             $errors['slug'] = 'already exists';
         }
 
-        $name = isset($body['name']) && is_string($body['name']) ? trim($body['name']) : '';
+        // SEC_REVIEW F52: strip C0/C1 control characters from free-form
+        // input before it lands in `audit_log.target_label` /
+        // `details_json`. Slug is already regex-validated.
+        $name = self::cleanString($body['name'] ?? null) ?? '';
         if ($name === '' || strlen($name) > 128) {
             $errors['name'] = 'required, 1–128 chars';
         }
@@ -87,7 +90,9 @@ final class CategoriesController
             if ($body['description'] !== null && !is_string($body['description'])) {
                 $errors['description'] = 'must be string or null';
             } else {
-                $description = $body['description'];
+                $description = is_string($body['description'])
+                    ? self::stripControlChars($body['description'])
+                    : null;
             }
         }
 
@@ -181,10 +186,12 @@ final class CategoriesController
         }
 
         if (array_key_exists('name', $body)) {
-            if (!is_string($body['name']) || trim($body['name']) === '' || strlen(trim($body['name'])) > 128) {
+            // SEC_REVIEW F52: see create() — same strip applied on update.
+            $cleaned = self::cleanString($body['name']);
+            if ($cleaned === null || $cleaned === '' || strlen($cleaned) > 128) {
                 $errors['name'] = 'required, 1–128 chars';
             } else {
-                $fields['name'] = trim($body['name']);
+                $fields['name'] = $cleaned;
             }
         }
 
@@ -192,7 +199,9 @@ final class CategoriesController
             if ($body['description'] !== null && !is_string($body['description'])) {
                 $errors['description'] = 'must be string or null';
             } else {
-                $fields['description'] = $body['description'];
+                $fields['description'] = is_string($body['description'])
+                    ? self::stripControlChars($body['description'])
+                    : null;
             }
         }
 

api/src/Application/Admin/ConsumersController.php

@@ -66,7 +66,10 @@ final class ConsumersController
         $body = self::jsonBody($request);
         $errors = [];
 
-        $name = isset($body['name']) && is_string($body['name']) ? trim($body['name']) : '';
+        // SEC_REVIEW F52: strip C0/C1 control characters from
+        // free-form input before it lands in `audit_log.target_label`
+        // / `details_json`.
+        $name = self::cleanString($body['name'] ?? null) ?? '';
         if ($name === '' || strlen($name) > 128) {
             $errors['name'] = 'required, 1–128 chars';
         }
@@ -76,7 +79,9 @@ final class ConsumersController
             if ($body['description'] !== null && !is_string($body['description'])) {
                 $errors['description'] = 'must be string or null';
             } else {
-                $description = $body['description'];
+                $description = is_string($body['description'])
+                    ? self::stripControlChars($body['description'])
+                    : null;
             }
         }
 
@@ -148,15 +153,16 @@ final class ConsumersController
         $fields = [];
 
         if (array_key_exists('name', $body)) {
-            if (!is_string($body['name']) || trim($body['name']) === '' || strlen(trim($body['name'])) > 128) {
+            // SEC_REVIEW F52: see create() — same strip applied on update.
+            $cleaned = self::cleanString($body['name']);
+            if ($cleaned === null || $cleaned === '' || strlen($cleaned) > 128) {
                 $errors['name'] = 'required, 1–128 chars';
             } else {
-                $name = trim($body['name']);
-                $other = $this->consumers->findByName($name);
+                $other = $this->consumers->findByName($cleaned);
                 if ($other !== null && $other->id !== $id) {
                     $errors['name'] = 'already exists';
                 } else {
-                    $fields['name'] = $name;
+                    $fields['name'] = $cleaned;
                 }
             }
         }
@@ -164,7 +170,9 @@ final class ConsumersController
             if ($body['description'] !== null && !is_string($body['description'])) {
                 $errors['description'] = 'must be string or null';
             } else {
-                $fields['description'] = $body['description'];
+                $fields['description'] = is_string($body['description'])
+                    ? self::stripControlChars($body['description'])
+                    : $body['description'];
             }
         }
         if (array_key_exists('policy_id', $body)) {

api/src/Application/Admin/ManualBlocksController.php

@@ -100,7 +100,13 @@ final class ManualBlocksController
             if ($body['reason'] !== null && !is_string($body['reason'])) {
                 $errors['reason'] = 'must be string or null';
             } else {
-                $reason = $body['reason'];
+                // SEC_REVIEW F52: strip C0/C1 control characters before
+                // the reason lands in `audit_log.target_label` /
+                // `details_json` to defeat log-injection and terminal-
+                // escape attacks on log viewers.
+                $reason = is_string($body['reason'])
+                    ? self::stripControlChars($body['reason'])
+                    : null;
             }
         }
 

api/src/Application/Admin/ReportersController.php

@@ -68,7 +68,11 @@ final class ReportersController
         $body = self::jsonBody($request);
         $errors = [];
 
-        $name = isset($body['name']) && is_string($body['name']) ? trim($body['name']) : '';
+        // SEC_REVIEW F52: strip C0/C1 control characters from
+        // free-form input before it lands in `audit_log.target_label`
+        // / `details_json` to defeat log-injection and terminal-
+        // escape attacks on log viewers.
+        $name = self::cleanString($body['name'] ?? null) ?? '';
         if ($name === '' || strlen($name) > 128) {
             $errors['name'] = 'required, 1–128 chars';
         }
@@ -78,7 +82,9 @@ final class ReportersController
             if ($body['description'] !== null && !is_string($body['description'])) {
                 $errors['description'] = 'must be string or null';
             } else {
-                $description = $body['description'];
+                $description = is_string($body['description'])
+                    ? self::stripControlChars($body['description'])
+                    : null;
             }
         }
 
@@ -145,15 +151,16 @@ final class ReportersController
         $fields = [];
 
         if (array_key_exists('name', $body)) {
-            if (!is_string($body['name']) || trim($body['name']) === '' || strlen(trim($body['name'])) > 128) {
+            // SEC_REVIEW F52: see create() — same strip applied on update.
+            $cleaned = self::cleanString($body['name']);
+            if ($cleaned === null || $cleaned === '' || strlen($cleaned) > 128) {
                 $errors['name'] = 'required, 1–128 chars';
             } else {
-                $name = trim($body['name']);
-                $other = $this->reporters->findByName($name);
+                $other = $this->reporters->findByName($cleaned);
                 if ($other !== null && $other->id !== $id) {
                     $errors['name'] = 'already exists';
                 } else {
-                    $fields['name'] = $name;
+                    $fields['name'] = $cleaned;
                 }
             }
         }
@@ -161,7 +168,9 @@ final class ReportersController
             if ($body['description'] !== null && !is_string($body['description'])) {
                 $errors['description'] = 'must be string or null';
             } else {
-                $fields['description'] = $body['description'];
+                $fields['description'] = is_string($body['description'])
+                    ? self::stripControlChars($body['description'])
+                    : null;
             }
         }
         if (array_key_exists('trust_weight', $body)) {

api/tests/Integration/Admin/InputControlCharStrippingTest.php

@@ -0,0 +1,181 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Integration\Admin;
+
+use App\Domain\Auth\Role;
+use App\Domain\Auth\TokenKind;
+use App\Tests\Integration\Support\AppTestCase;
+
+/**
+ * SEC_REVIEW F52 — admin CRUD endpoints must strip C0/C1 control
+ * characters from free-form string fields (`name`, `description`,
+ * `reason`) before they land in `audit_log.target_label` /
+ * `details_json`. NULs, newlines and ANSI escapes otherwise enable
+ * log-injection (`\n[CRIT] fake event`) and terminal-escape attacks
+ * on log viewers.
+ *
+ * One test per controller is enough — the helper is shared in
+ * `AdminControllerSupport` and applied at every relevant call site.
+ */
+final class InputControlCharStrippingTest extends AppTestCase
+{
+    public function testReporterNameAndDescriptionAreStripped(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Admin);
+
+        $resp = $this->request(
+            'POST',
+            '/api/v1/admin/reporters',
+            ['Authorization' => 'Bearer ' . $token, 'Content-Type' => 'application/json'],
+            (string) json_encode([
+                'name' => "web\x00prod\n01\u{007f}",
+                'description' => "first\nwebserver\x07with\x00control\u{001b}chars",
+                'trust_weight' => 1.0,
+            ]),
+        );
+        self::assertSame(201, $resp->getStatusCode());
+        $body = $this->decode($resp);
+        self::assertControlBytesGone($body['name']);
+        self::assertControlBytesGone($body['description']);
+        // Visible payload (post-scrub) round-trips byte-for-byte.
+        self::assertSame('webprod01', $body['name']);
+        self::assertSame('firstwebserverwithcontrolchars', $body['description']);
+
+        // Audit log target_label / details_json must also be clean.
+        $audit = $this->db->fetchAssociative(
+            "SELECT target_label, details_json FROM audit_log WHERE action = 'reporter.created' AND target_id = ?",
+            [(string) $body['id']],
+        );
+        self::assertIsArray($audit);
+        self::assertSame('webprod01', $audit['target_label']);
+        self::assertControlBytesGone((string) $audit['details_json']);
+    }
+
+    public function testConsumerNameIsStripped(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Admin);
+        $policyId = (int) $this->db->fetchOne(
+            'SELECT id FROM policies WHERE name = :name',
+            ['name' => 'moderate'],
+        );
+
+        $resp = $this->request(
+            'POST',
+            '/api/v1/admin/consumers',
+            ['Authorization' => 'Bearer ' . $token, 'Content-Type' => 'application/json'],
+            (string) json_encode([
+                'name' => "fw\x00edge\nok\u{007f}",
+                'description' => "edge\nrouter\x00",
+                'policy_id' => $policyId,
+            ]),
+        );
+        self::assertSame(201, $resp->getStatusCode());
+        $body = $this->decode($resp);
+        self::assertControlBytesGone($body['name']);
+        self::assertControlBytesGone($body['description']);
+        self::assertSame('fwedgeok', $body['name']);
+        self::assertSame('edgerouter', $body['description']);
+    }
+
+    public function testCategoryNameIsStripped(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Admin);
+
+        $resp = $this->request(
+            'POST',
+            '/api/v1/admin/categories',
+            ['Authorization' => 'Bearer ' . $token, 'Content-Type' => 'application/json'],
+            (string) json_encode([
+                'slug' => 'sec_review_f52',
+                'name' => "Brute\x00 force\n attempts",
+                'description' => "Repeated\nlogin\x00fails",
+                'decay_function' => 'linear',
+                'decay_param' => 14,
+            ]),
+        );
+        self::assertSame(201, $resp->getStatusCode());
+        $body = $this->decode($resp);
+        self::assertControlBytesGone($body['name']);
+        self::assertControlBytesGone($body['description']);
+        self::assertSame('Brute force attempts', $body['name']);
+    }
+
+    public function testManualBlockReasonIsStripped(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Operator);
+
+        $resp = $this->request(
+            'POST',
+            '/api/v1/admin/manual-blocks',
+            ['Authorization' => 'Bearer ' . $token, 'Content-Type' => 'application/json'],
+            (string) json_encode([
+                'kind' => 'ip',
+                'ip' => '198.51.100.42',
+                'reason' => "abuse\n\x00alert\x07more\u{007f}",
+            ]),
+        );
+        self::assertSame(201, $resp->getStatusCode());
+        $body = $this->decode($resp);
+        self::assertControlBytesGone($body['reason']);
+        self::assertSame('abusealertmore', $body['reason']);
+    }
+
+    public function testAllowlistReasonIsStripped(): void
+    {
+        $token = $this->createToken(TokenKind::Admin, role: Role::Operator);
+
+        $resp = $this->request(
+            'POST',
+            '/api/v1/admin/allowlist',
+            ['Authorization' => 'Bearer ' . $token, 'Content-Type' => 'application/json'],
+            (string) json_encode([
+                'kind' => 'ip',
+                'ip' => '203.0.113.99',
+                'reason' => "trusted\nsource\x00ok",
+            ]),
+        );
+        self::assertSame(201, $resp->getStatusCode());
+        $body = $this->decode($resp);
+        self::assertControlBytesGone($body['reason']);
+        self::assertSame('trustedsourceok', $body['reason']);
+    }
+
+    public function testReporterUpdateAlsoStrips(): void
+    {
+        // Defence-in-depth: update path must apply the same scrub.
+        $token = $this->createToken(TokenKind::Admin, role: Role::Admin);
+        $reporterId = $this->createReporter('web-update-strip');
+
+        $resp = $this->request(
+            'PATCH',
+            "/api/v1/admin/reporters/{$reporterId}",
+            ['Authorization' => 'Bearer ' . $token, 'Content-Type' => 'application/json'],
+            (string) json_encode([
+                'name' => "renamed\x00\n",
+                'description' => "later\u{001b}",
+            ]),
+        );
+        self::assertSame(200, $resp->getStatusCode());
+        $body = $this->decode($resp);
+        self::assertControlBytesGone($body['name']);
+        self::assertControlBytesGone($body['description']);
+        self::assertSame('renamed', $body['name']);
+        self::assertSame('later', $body['description']);
+    }
+
+    private static function assertControlBytesGone(string $value): void
+    {
+        // Empty regex match → no C0/C1/DEL bytes in the value. The
+        // ESC byte (0x1B) is the lead-in for ANSI escape sequences;
+        // its absence neutralises terminal-interpretation attacks
+        // even if the trailing `[31m`-style payload remains as
+        // visible text.
+        self::assertSame(
+            0,
+            preg_match('/[\x00-\x1f\x7f-\x9f]/u', $value),
+            sprintf('control byte found in %s', json_encode($value)),
+        );
+    }
+}