瀏覽代碼

fix: strip args from logged stack traces (SEC_REVIEW F21)

PHP's getTraceAsString() inlines scalar arguments into each frame, so
an exception thrown from password_verify, an OIDC token validator, or
any other call invoked with a plaintext credential persisted that
secret into stdout logs. SecretScrubbingProcessor only catches known
token shapes and would not scrub arbitrary passwords or generic
client_secret values.

Route both production trace-logging call sites (JsonErrorHandler,
JobRunner) through a new SafeTrace::format() helper that walks
Throwable::getTrace() (plus the getPrevious() chain) and renders
'#N file(line): Class::method()' per frame, dropping the args entry
entirely. Regression test pins the no-args invariant and the
"Caused by" wrapping behaviour.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 天之前
父節點
當前提交
0da01a83d0

+ 2 - 1
api/src/Infrastructure/Http/JsonErrorHandler.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace App\Infrastructure\Http;
 
+use App\Infrastructure\Logging\SafeTrace;
 use Psr\Http\Message\ResponseFactoryInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
@@ -45,7 +46,7 @@ final class JsonErrorHandler
                 'exception' => $exception::class,
                 'method' => $request->getMethod(),
                 'path' => $request->getUri()->getPath(),
-                'trace' => $logErrorDetails ? $exception->getTraceAsString() : null,
+                'trace' => $logErrorDetails ? SafeTrace::format($exception) : null,
             ]);
         }
 

+ 2 - 1
api/src/Infrastructure/Jobs/JobRunner.php

@@ -9,6 +9,7 @@ use App\Domain\Jobs\JobContext;
 use App\Domain\Jobs\JobOutcome;
 use App\Domain\Jobs\JobStatus;
 use App\Domain\Time\Clock;
+use App\Infrastructure\Logging\SafeTrace;
 use Psr\Log\LoggerInterface;
 use Throwable;
 
@@ -88,7 +89,7 @@ final class JobRunner
                 'job' => $job->name(),
                 'run_id' => $runId,
                 'error' => $e->getMessage(),
-                'trace' => $e->getTraceAsString(),
+                'trace' => SafeTrace::format($e),
             ]);
             $this->runs->finishRun($runId, JobStatus::Failure, 0, $finish, $e->getMessage());
 

+ 55 - 0
api/src/Infrastructure/Logging/SafeTrace.php

@@ -0,0 +1,55 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Infrastructure\Logging;
+
+use Throwable;
+
+/**
+ * Produces a stringified backtrace that, unlike Throwable::getTraceAsString(),
+ * never inlines call-arguments into frames (SEC_REVIEW F21).
+ *
+ * PHP's built-in stringifier renders scalar args directly, so an exception
+ * thrown from `password_verify($plaintext, $hash)` writes the plaintext
+ * password into the trace; SecretScrubbingProcessor only matches known token
+ * shapes (irdb_*, $argon2$, bcrypt) and won't catch arbitrary plaintext
+ * passwords or generic OIDC client_secret values. This formatter keeps
+ * file/line/class/function so the trace stays useful for debugging, but
+ * drops args entirely.
+ *
+ * Walks `$previous` chains so wrapped exceptions don't smuggle args back in
+ * via "Caused by" sections.
+ */
+final class SafeTrace
+{
+    public static function format(Throwable $e): string
+    {
+        $out = self::formatOne($e);
+        $prev = $e->getPrevious();
+        while ($prev !== null) {
+            $out .= "\nCaused by: " . $prev::class . ': ' . $prev->getMessage()
+                . "\n" . self::formatOne($prev);
+            $prev = $prev->getPrevious();
+        }
+
+        return $out;
+    }
+
+    private static function formatOne(Throwable $e): string
+    {
+        $lines = [];
+        $i = 0;
+        foreach ($e->getTrace() as $frame) {
+            $location = isset($frame['file'])
+                ? $frame['file'] . '(' . ($frame['line'] ?? '?') . ')'
+                : '[internal function]';
+            $call = ($frame['class'] ?? '') . ($frame['type'] ?? '') . $frame['function'];
+            $lines[] = '#' . $i . ' ' . $location . ': ' . $call . '()';
+            $i++;
+        }
+        $lines[] = '#' . $i . ' {main}';
+
+        return implode("\n", $lines);
+    }
+}

+ 80 - 0
api/tests/Unit/Logging/SafeTraceTest.php

@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Unit\Logging;
+
+use App\Infrastructure\Logging\SafeTrace;
+use PHPUnit\Framework\TestCase;
+use RuntimeException;
+use Throwable;
+
+/**
+ * Regression test for SEC_REVIEW F21: PHP's getTraceAsString() inlines
+ * scalar args into each frame, which leaks plaintext secrets when an
+ * exception is thrown from inside `password_verify`, an OIDC token
+ * validation call, etc. SafeTrace must drop args entirely.
+ */
+final class SafeTraceTest extends TestCase
+{
+    public function testFormattedTraceDoesNotContainArgs(): void
+    {
+        $secret = 'super-plaintext-password-please-do-not-leak';
+        $caught = $this->capture(fn () => $this->throwWithSecret($secret));
+
+        $trace = SafeTrace::format($caught);
+
+        self::assertStringNotContainsString($secret, $trace);
+        self::assertStringContainsString('throwWithSecret', $trace);
+    }
+
+    public function testFormattedTraceWalksPreviousChain(): void
+    {
+        $innerSecret = 'inner-secret-123';
+        $outerSecret = 'outer-secret-456';
+
+        $caught = $this->capture(function () use ($innerSecret, $outerSecret): void {
+            try {
+                $this->throwWithSecret($innerSecret);
+            } catch (Throwable $inner) {
+                $this->wrap($outerSecret, $inner);
+            }
+        });
+
+        $trace = SafeTrace::format($caught);
+        self::assertStringNotContainsString($innerSecret, $trace);
+        self::assertStringNotContainsString($outerSecret, $trace);
+        self::assertStringContainsString('Caused by', $trace);
+    }
+
+    public function testFrameLayoutResemblesPhpNativeFormat(): void
+    {
+        $caught = $this->capture(fn () => $this->throwWithSecret('x'));
+
+        $trace = SafeTrace::format($caught);
+
+        // Numbered frames, file(line) suffix, class::method() call, then {main}.
+        self::assertMatchesRegularExpression('/^#0 .+\(\d+\): .+\(\)$/m', $trace);
+        self::assertStringEndsWith('{main}', $trace);
+    }
+
+    private function capture(callable $fn): Throwable
+    {
+        try {
+            $fn();
+        } catch (Throwable $e) {
+            return $e;
+        }
+        self::fail('expected callable to throw');
+    }
+
+    private function throwWithSecret(string $secret): never
+    {
+        throw new RuntimeException('boom');
+    }
+
+    private function wrap(string $secret, Throwable $inner): never
+    {
+        throw new RuntimeException('outer', 0, $inner);
+    }
+}