Files
7d13d8bd39 Fix the toApiArray auditor crashing on try/catch blocks (#4599)
* Fix the toApiArray auditor crashing on try/catch blocks

The TryCatch branch read $node->try, which php-parser does not have (the
try body is $node->stmts), so the first toApiArray() containing a
try/catch passed null to collectAssignments() and the audit test died
with a TypeError. The catch and finally nodes were passed in whole, and
collectAssignments() has no branch for them, so their bodies were never
examined either.

Walk the try, catch and finally bodies instead. A new test audits a
temporary module whose toApiArray() sets a field conditionally in each
of the three; it fails with the TypeError before this change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Name the audit test's temporary directory in plain words

cspell rejects "toapiarray".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Adam Daley <admdly@users.noreply.github.com>
2026-10-28 11:04:46 +00:00

246 lines
7.9 KiB
PHP

<?php
/**
* Copyright 2022-2026 FOSSBilling
* SPDX-License-Identifier: Apache-2.0.
*
* @copyright FOSSBilling (https://www.fossbilling.org)
* @license http://www.apache.org/licenses/LICENSE-2.0 Apache License, Version 2.0
*/
declare(strict_types=1);
namespace Tests\Support;
use PhpParser\Node;
use PhpParser\Node\Expr\Array_;
use PhpParser\Node\Expr\ArrayDimFetch;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\NodeFinder;
use PhpParser\ParserFactory;
use Symfony\Component\Finder\Finder;
/**
* Walks every `function toApiArray` under `src/modules` and reports fields
* that are assigned to `$data['xxx']` only inside conditional blocks. The
* toApiArray method is the canonical bridge between a service-layer model
* and the JSON returned to the API; if a template reads a key that is only
* set inside an `if ($identity instanceof \Box\Mod\Staff\Entity\Admin)` branch, the
* template raises `Key "xxx" does not exist` under strict_variables, or
* returns a silent empty value under permissive rendering, when the
* response is consumed by a non-admin caller.
*
* Only fields that are never assigned at the top level are reported. These
* fields need an explicit review because their absence is part of the API
* contract, rather than an accidental omission.
*/
final readonly class ToApiArrayAuditor
{
private \PhpParser\Parser $parser;
private NodeFinder $nodeFinder;
public function __construct(private string $srcDir)
{
// Parenthesise the `new` so the chained `->createForHostVersion()`
// call parses on PHP 8.3 as well as 8.4+. Without the parentheses,
// PHP 8.3 sees `new ParserFactory()->...` as a syntax error: the
// ability to omit them is a PHP 8.4 language change
// (https://wiki.php.net/rfc/new_without_parentheses).
$factory = new ParserFactory();
$this->parser = $factory->createForHostVersion();
$this->nodeFinder = new NodeFinder();
}
/**
* @return array<string, array<string, list<string>>>
*/
public function audit(): array
{
$findings = [];
$finder = new Finder();
$finder->files()->in($this->srcDir . '/modules')->name('*.php');
foreach ($finder as $file) {
$code = $file->getContents();
try {
$ast = $this->parser->parse($code);
} catch (\Throwable) {
continue;
}
if ($ast === null) {
continue;
}
$methods = $this->nodeFinder->findInstanceOf($ast, ClassMethod::class);
foreach ($methods as $method) {
if ($method->name->toString() !== 'toApiArray') {
continue;
}
if ($method->stmts === null) {
continue;
}
$relPath = str_replace($this->srcDir . '/', '', $file->getPathName());
$fields = $this->auditMethodBody($method->stmts);
$conditional = [];
foreach ($fields as $name => $locations) {
if (!in_array('top', $locations, true)) {
$conditional[$name] = array_values(array_unique($locations));
}
}
if ($conditional !== []) {
$findings[$relPath] = $conditional;
}
}
}
ksort($findings);
return $findings;
}
/**
* Walk a list of statements and return a map of `$data['xxx']` field
* names to the set of places they are assigned.
*
* @param Node\Stmt[] $stmts
*
* @return array<string, list<string>>
*/
private function auditMethodBody(array $stmts): array
{
$result = [];
foreach ($stmts as $stmt) {
$this->collectAssignments($stmt, $result, 'top');
}
return $result;
}
/**
* @param array<string, list<string>> $result
*/
private function collectAssignments(Node $node, array &$result, string $where): void
{
if ($node instanceof Node\Stmt\If_) {
$childWhere = 'if';
if ($node->elseifs !== null && count($node->elseifs) > 0) {
$childWhere = 'if (with elseifs)';
}
if ($node->else !== null) {
$childWhere = 'if/else';
}
foreach ($node->stmts as $child) {
$this->collectAssignments($child, $result, $childWhere);
}
foreach ($node->elseifs as $elseif) {
foreach ($elseif->stmts as $child) {
$this->collectAssignments($child, $result, $childWhere);
}
}
if ($node->else !== null) {
foreach ($node->else->stmts as $child) {
$this->collectAssignments($child, $result, $childWhere);
}
}
return;
}
if ($node instanceof Node\Stmt\For_ || $node instanceof Node\Stmt\Foreach_ || $node instanceof Node\Stmt\While_ || $node instanceof Node\Stmt\Do_) {
$childWhere = 'loop';
foreach ($node->stmts as $child) {
$this->collectAssignments($child, $result, $childWhere);
}
return;
}
if ($node instanceof Node\Stmt\Switch_) {
foreach ($node->cases as $case) {
foreach ($case->stmts as $child) {
$this->collectAssignments($child, $result, 'switch');
}
}
return;
}
if ($node instanceof Node\Stmt\TryCatch) {
// The try body is `stmts`; catch and finally nodes have no branch
// of their own here, so walk their bodies directly.
$blocks = [$node->stmts];
foreach ($node->catches as $catch) {
$blocks[] = $catch->stmts;
}
if ($node->finally !== null) {
$blocks[] = $node->finally->stmts;
}
foreach ($blocks as $stmts) {
foreach ($stmts as $child) {
$this->collectAssignments($child, $result, $where);
}
}
return;
}
if ($node instanceof Node\Stmt\Block) {
foreach ($node->stmts as $child) {
$this->collectAssignments($child, $result, $where);
}
return;
}
if ($node instanceof Node\Stmt\Expression) {
$this->collectAssignments($node->expr, $result, $where);
return;
}
if ($node instanceof Assign) {
if ($node->var instanceof Variable && $node->var->name === 'data' && $node->expr instanceof Array_) {
foreach ($node->expr->items as $item) {
if ($item?->key instanceof Node\Scalar\String_) {
$result[$item->key->value][] = $where;
}
}
}
$field = $this->extractDataFieldName($node->var);
if ($field !== null) {
$result[$field][] = $where;
}
}
}
/**
* Extract the field name from a `$data['xxx']` or `$data['xxx'][]` access
* chain, or null if the chain doesn't terminate at a string-keyed
* `$data` access.
*/
private function extractDataFieldName(Node $node): ?string
{
if ($node instanceof ArrayDimFetch) {
if ($node->dim instanceof Node\Scalar\String_) {
$inner = $node->var;
if ($inner instanceof Variable && $inner->name === 'data') {
return $node->dim->value;
}
}
// Recurse in case of chained access like $data['x']['y'].
return $this->extractDataFieldName($node->var);
}
return null;
}
}