Skip to content

Commit 316b8a9

Browse files
Goosterhofclaude
andcommitted
fix(EnforceCurrentUserAttributeRule): namespace gate replaces no-op ancestry gate
The controller-detection gate used isSubclassOf(Illuminate\Routing\Controller), an ancestry check. Every consumer territory (kendo, ublgenie, entreezuil) ships base-less `final` controllers with no `extends Controller`, so the ancestry walk matched zero controllers and the rule enforced nothing — a silent no-op caught by the agent-review sweep on PR #26 (review pullrequestreview-4401182606). Replace it with the App\Http\Controllers namespace prefix gate used by the sibling ForbidEloquentMutationInControllersRule ($scope->getNamespace() + str_starts_with). Matches the canonical "controllers are identified by the App\Http\Controllers namespace" convention. Regression proof: a base-less `final` controller fixture (RequestUserInBaselessController) that the rule MUST flag, plus a compliant #[CurrentUser] base-less controller (CurrentUserAttributeInBaselessController) that MUST pass. Before/after error count on the flagging fixture: namespace gate -> 1 error (line 18); revert to ancestry -> 0 errors (the bug). False-negative closure; no new identifier, no consumer ignoreErrors shape change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e122c00 commit 316b8a9

5 files changed

Lines changed: 105 additions & 22 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and
88

99
### Added
1010

11-
- `EnforceCurrentUserAttributeRule` — flags calls to `Request::user()` / `Auth::user()` / `auth()->user()` inside classes extending `Illuminate\Routing\Controller`. The canonical fix is Laravel's `#[\Illuminate\Container\Attributes\CurrentUser]` container attribute on the method parameter — eliminates the implicit-nullable-then-assert dance (`$user = $request->user(); assert($user instanceof User);`) introduced by emmie PR #263 (EMMIE-0197) and recurring across the war-room fleet. Doctrine: war-room §Architectural Principles — Explicit over implicit. Identifier: `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser`. Detection branches on three call shapes via `CallLike` registration (mirrors `LogRule` v0.3.0 shape): `MethodCall` on `Illuminate\Http\Request` subtype receiver (type-based via `ObjectType::isSuperTypeOf()`); `MethodCall` whose receiver is a `FuncCall('auth')` (AST-shape match — the helper's return type is unloaded in stub-only environments); `StaticCall` resolving to `Illuminate\Support\Facades\Auth` (FQCN comparison via `$scope->resolveName()`). Scoped to Controller descendants via `ClassReflection::isSubclassOf(Illuminate\Routing\Controller)` — FormRequest descendants (where `$this->user()` is canonical because container-attribute injection does not apply to `FormRequest::rules()` / `toDto()` / `authorize()` invocations), middleware, services, Actions, jobs, and console commands are silent. Origin: war-room cross-territory recon 2026-05-22 (50+ violations across codebook, ublgenie, entreezuil, emmie; kendo already clean with 30 adopted sites). **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has un-migrated controllers). The release PR will determine whether this collapses into the existing v0.3.0 [Unreleased] block (already a Major) or cuts as a separate Major (v0.4.0) after v0.3.0 ships.** **Pre-cascade audit required before tagging** — consuming territories will need either Medic dispatches to migrate (ublgenie 6 sites, entreezuil 3 sites, emmie 2 sites) or PHPStan-baseline staging (codebook ~40+ sites — safer than mass-edit on lightly-staffed AVG/NEN-7510-downstream territory). Kendo gets a constraint bump only (zero violations). Multi-guard ergonomics (`#[CurrentUser] Client $client` resolves via client guard, `#[CurrentUser] User $user` via user guard — verified in emmie's `ClientController::me`) work as expected: Laravel dispatches by typed parameter. Out of scope v1: `Auth::guard('name')->user()` and other guard-specific resolution paths — rare, substitution is more nuanced (`#[CurrentUser(guard: 'name')]`), do not appear in the recon yield.
11+
- `EnforceCurrentUserAttributeRule` — flags calls to `Request::user()` / `Auth::user()` / `auth()->user()` inside classes extending `Illuminate\Routing\Controller`. The canonical fix is Laravel's `#[\Illuminate\Container\Attributes\CurrentUser]` container attribute on the method parameter — eliminates the implicit-nullable-then-assert dance (`$user = $request->user(); assert($user instanceof User);`) introduced by emmie PR #263 (EMMIE-0197) and recurring across the war-room fleet. Doctrine: war-room §Architectural Principles — Explicit over implicit. Identifier: `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser`. Detection branches on three call shapes via `CallLike` registration (mirrors `LogRule` v0.3.0 shape): `MethodCall` on `Illuminate\Http\Request` subtype receiver (type-based via `ObjectType::isSuperTypeOf()`); `MethodCall` whose receiver is a `FuncCall('auth')` (AST-shape match — the helper's return type is unloaded in stub-only environments); `StaticCall` resolving to `Illuminate\Support\Facades\Auth` (FQCN comparison via `$scope->resolveName()`). Scoped to controllers via the `App\Http\Controllers` namespace prefix (`$scope->getNamespace()` + `str_starts_with`) — mirrors `ForbidEloquentMutationInControllersRule` and the canonical "controllers are identified by the `App\Http\Controllers` namespace" convention. FormRequest (`App\Http\Requests`, where `$this->user()` is canonical because container-attribute injection does not apply to `FormRequest::rules()` / `toDto()` / `authorize()` invocations), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix. Origin: war-room cross-territory recon 2026-05-22 (50+ violations across codebook, ublgenie, entreezuil, emmie; kendo already clean with 30 adopted sites). **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has un-migrated controllers). The release PR will determine whether this collapses into the existing v0.3.0 [Unreleased] block (already a Major) or cuts as a separate Major (v0.4.0) after v0.3.0 ships.** **Pre-cascade audit required before tagging** — consuming territories will need either Medic dispatches to migrate (ublgenie 6 sites, entreezuil 3 sites, emmie 2 sites) or PHPStan-baseline staging (codebook ~40+ sites — safer than mass-edit on lightly-staffed AVG/NEN-7510-downstream territory). Kendo gets a constraint bump only (zero violations). Multi-guard ergonomics (`#[CurrentUser] Client $client` resolves via client guard, `#[CurrentUser] User $user` via user guard — verified in emmie's `ClientController::me`) work as expected: Laravel dispatches by typed parameter. Out of scope v1: `Auth::guard('name')->user()` and other guard-specific resolution paths — rare, substitution is more nuanced (`#[CurrentUser(guard: 'name')]`), do not appear in the recon yield.
1212

1313
- `ForbidEloquentMutationInControllersRule` — bans Eloquent persistence APIs (`save`, `update`, `delete`, `create`, `destroy`, `forceDelete`, `forceFill`, `push`, `restore`, `touch`, and their `*OrFail` / `*Quietly` / `*OrCreate` variants — 24-method blocklist) on `Illuminate\Database\Eloquent\Model` subclasses and `Illuminate\Database\Eloquent\Builder` chains when the call site is inside an `App\Http\Controllers\*` class (including sub-namespaces like kendo's `App\Http\Controllers\Central\*`, matched via `str_starts_with`). Reads (`find`, `where`, `get`, `first`, `paginate`, `pluck`, `count`, `exists`, `query`) are deliberately permitted — route-model binding, ResourceData hydration, and policy checks need controller-level Model access; the doctrine line is "Controllers may READ Models, but MUST NOT mutate them." Identifier: `forbidEloquentMutationInControllers.eloquentMutationInController`. Doctrine: ADR-0011 (Action Class Architecture) — Actions are the chokepoint for mutations — combined with ADR-0019 (Explicit Model Hydration) — `Model::create()` / `fill()` / `forceFill()` / `update()` banned application-wide; this rule enforces the controller surface where the violations have been historically common. Algorithm: namespace gate (`App\Http\Controllers`) → recursively walk every `ClassMethod` body collecting `MethodCall` + `StaticCall` nodes → for `MethodCall`, fire if `ObjectType::isSuperTypeOf()` against `Model` OR `Builder` matches the receiver type and the method name is in the blocklist; for `StaticCall`, fire if `Scope::resolveName()` resolves to a Model subclass FQCN and the method name is in the blocklist. Builder coverage is type-aware (`User::query()->where(...)->update([...])` fires) — the generic parameter is not unwrapped because `ObjectType` matches `Builder<User>` as a subtype of the unparameterized `Builder` cleanly, no brittle generic introspection needed. Supersedes the consumer-side string-match Pest arch tests in kendo (`backend/tests/Arch/ControllersTest.php` `controllers must not call Eloquent write methods directly`), ublgenie + entreezuil (`tests/Arch/ControllersTest.php` of the same shape), and the bridge subset in ISMS (`backend/tests/Architecture/ControllerCurrentUserTest.php` from PR #10, 2026-05-28). The string-match shape catches `->save(`, `->update([`, `->delete(`, `->forceDelete(` but cannot discriminate `Model::create()` from `Response::create()`, `Collection::push()` from `Model::push()`, or `->update($vars)` without an inline array literal — the type-aware AST inspection here closes those gaps. Cross-territory cascade post-merge: consumer Pest tests deleted; emmie + brick-inventory-orchestrator pick up coverage automatically on next composer update. Out of scope: non-`App\Http\Controllers\*` namespaces (Actions/Services/Jobs/Middleware are allowed to call persistence APIs), non-Eloquent receivers, dynamic method names (`$model->{$var}()` — value-flow analysis), variable class names in static calls (`$class::create(...)`). Closes war-room enforcement queue #87. **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has a controller calling Eloquent persistence APIs directly — the three territories currently running the string-match Pest test caught the bulk of these, but the type-aware shape will surface additional violations the string-match shape missed: `Model::create()`, `Model::destroy()`, chained Builder mutations, `*Quietly` variants, etc.). Pre-cascade audit required across ISMS, kendo, emmie, entreezuil, ublgenie, brick-inventory before tagging — three territories' Pest tests already closed the string-match-visible violators; the type-aware additional surface (Builder chains, `Model::create()`, `*Quietly` variants) may carry undetected violators.**
1414
- `EnforceAuditTransactionScopeRule` — enforces ADR-0029 (Audit Row Durability Contract) §Decision rule 3. Flags non-transactional state mutations (`StatefulGuard` / `Session` / `Cache` / `Bus` / `Queue` / `Mailer` / `Notification` / `Broadcaster` / `Filesystem` and their `Illuminate\Support\Facades\*` counterparts, mutation methods only) inside `transaction(...)` closures in `App\Actions\*` classes. Identifier: `enforceAuditTransactionScope.nonTransactionalMutationInClosure`. Doctrine: ADR-0029 §Decision rule 3. Seed: ISMS-0003 PR #7 commit `f1d357b` (2026-05-28) — three Auth Actions (`AuthenticateWorkerAction`, `VerifyTwoFactorChallengeAction`, `LogoutWorkerAction`) mutated `StatefulGuard` + `Session` state inside the transaction closure before the audit row write; an audit-write failure would have rolled back the audit row while leaving the session/guard mutation intact (A.8.15 / A.5.33 violation). Reads (`Auth::user()`, `Session::get()`, `Cache::get()`, etc.) are deliberately permitted — only mutations carry the rollback-vs-side-effect asymmetry. Instance-call detection matches the constructor-property's declared FQCN against the blocklist keys; static-call detection resolves the facade name via `Scope::resolveName()`. Nested `transaction(...)` calls inside an outer closure are walked transitively — a nested mutation is still inside the outermost transaction's rollback scope; top-level transaction discovery deduplicates so each violation reports exactly once. Out of scope: manual transaction management (`DB::beginTransaction()` / `commit()`); non-`App\Actions\*` namespaces; the failure-side discipline (sentinel-return; throw-inside-closure detection) which lives as per-territory Pest arch tests under enforcement queue #85. **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has an `App\Actions\*` class mutating non-transactional state inside a `transaction(...)` closure). Pre-cascade audit required across ISMS, kendo, emmie, entreezuil, ublgenie, brick-inventory before tagging — ISMS-0003 PR #7 commit `f1d357b` already closed ISMS's known violators; other consumer territories may carry undetected violators.**
1515

16+
### Fixed
17+
18+
- `EnforceCurrentUserAttributeRule` — corrected the controller-detection gate from an ancestry check (`ClassReflection::isSubclassOf(Illuminate\Routing\Controller)`) to a namespace prefix check (`$scope->getNamespace()` starts with `App\Http\Controllers`). The ancestry gate was a **silent no-op**: every consumer territory (kendo, ublgenie, entreezuil) ships base-less `final` controllers with no `extends Controller`, so the ancestry walk matched **zero** controllers and the rule enforced nothing. The namespace gate mirrors the sibling `ForbidEloquentMutationInControllersRule` and the canonical "controllers are identified by the `App\Http\Controllers` namespace" convention. Caught by the agent-review sweep on PR #26 (review `pullrequestreview-4401182606`). Regression-proofed by a base-less `final` controller fixture (`RequestUserInBaselessController` — flagged; `CurrentUserAttributeInBaselessController` — clean) that reproduces the exact real-world shape the rule missed. **Versioning:** false-negative closure — the rule now actually fires where it always intended to; this is part of the same `[Unreleased]` Major bump the rule's addition already carries (consumers with un-migrated controllers will now see the errors). No new identifier; no consumer `ignoreErrors` migration shape change.
19+
1620
### Changed
1721

1822
- **CI:** pinned `symfony/console` to `^7.2` in `require-dev`. `symfony/console` 8.x (v8.1.0, released 2026-05-29) breaks Infection 0.33.x's mutation runner — its DI container references `Symfony\Component\Console\Helper\QuestionHelper` as a service Symfony Console 8 no longer registers that way, so `composer mutation:ci` aborts with `Unknown service` and exits 1. Because the package's `composer.lock` is gitignored, CI resolves dependencies fresh on every run; `illuminate/*` v13 permits Symfony 8, so the resolver began pulling v8.1.0 and the mutation gate went red fleet-wide (PRs green on 2026-05-28 turned red on 2026-05-29 with no source change). The pin holds the dev toolchain at `symfony/console` v7.4.x — verified mutation gate green (Covered Code MSI 81% ≥ 75) — until Infection ships Symfony Console 8 support, at which point this constraint should be widened or removed. **Versioning:** none (dev-only test-infra; no consumer-facing surface).

src/Rules/EnforceCurrentUserAttributeRule.php

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,26 @@
1414
use PhpParser\Node\Identifier;
1515
use PhpParser\Node\Name;
1616
use PHPStan\Analyser\Scope;
17-
use PHPStan\Reflection\ClassReflection;
1817
use PHPStan\Rules\IdentifierRuleError;
1918
use PHPStan\Rules\Rule;
2019
use PHPStan\Rules\RuleErrorBuilder;
2120
use PHPStan\Type\ObjectType;
2221

2322
use function sprintf;
23+
use function str_starts_with;
2424

2525
/**
2626
* Flags Request::user() / Auth::user() / auth()->user() calls inside controller methods.
2727
* Use the #[\Illuminate\Container\Attributes\CurrentUser] container attribute on the
2828
* method parameter instead — eliminates the nullable-then-assert dance.
2929
*
30-
* Scoped to classes extending Illuminate\Routing\Controller. FormRequest descendants
31-
* are excluded by design — container-attribute injection does not apply to
32-
* FormRequest::rules() / toDto() / authorize() invocations. Middleware, services,
33-
* Actions, jobs, and console commands are likewise out of scope: each context has
34-
* its own canonical resolution path (constructor DI for Actions, authenticated
35-
* payload threading for jobs, etc.).
30+
* Scoped to classes in the `App\Http\Controllers` namespace. FormRequest
31+
* (`App\Http\Requests`) is excluded by design — container-attribute injection
32+
* does not apply to FormRequest::rules() / toDto() / authorize() invocations.
33+
* Middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs,
34+
* and console commands are likewise out of scope: each context has its own
35+
* canonical resolution path (constructor DI for Actions, authenticated payload
36+
* threading for jobs, etc.).
3637
*
3738
* Detection (three call shapes branch in `processNode`):
3839
* 1. `MethodCall` — receiver type is a (subtype of) `Illuminate\Http\Request`
@@ -45,9 +46,15 @@
4546
* and method name is `user`. FQCN comparison via `$scope->resolveName()`
4647
* handles aliased imports.
4748
*
48-
* Containing-class gate (applied to all three branches): walk
49-
* `$scope->getClassReflection()` ancestry and fire only if the class extends
50-
* `Illuminate\Routing\Controller`. Out-of-scope classes return silently.
49+
* Containing-class gate (applied to all three branches): the rule fires only
50+
* when `$scope->getNamespace()` starts with `App\Http\Controllers`. This
51+
* mirrors `ForbidEloquentMutationInControllersRule` and the canonical
52+
* "controllers are identified by the `App\Http\Controllers` namespace"
53+
* convention — consumer controllers are base-less `final` classes with no
54+
* `extends Controller`, so an ancestry walk catches nothing. Out-of-scope
55+
* namespaces (FormRequest in `App\Http\Requests`, middleware in
56+
* `App\Http\Middleware`, Actions in `App\Actions`, jobs, services) return
57+
* silently because their namespaces do not start with the controller prefix.
5158
*
5259
* Implementation note: `getNodeType()` returns `CallLike::class` so the rule
5360
* sees both `MethodCall` and `StaticCall` in a single registration — mirrors
@@ -61,7 +68,7 @@ final class EnforceCurrentUserAttributeRule implements Rule
6168
{
6269
private const string TARGET_METHOD = 'user';
6370

64-
private const string CONTROLLER_BASE_FQCN = 'Illuminate\Routing\Controller';
71+
private const string CONTROLLER_NAMESPACE_PREFIX = 'App\Http\Controllers';
6572

6673
private const string REQUEST_FQCN = Request::class;
6774

@@ -91,23 +98,23 @@ public function processNode(Node $node, Scope $scope): array
9198

9299
/**
93100
* Containing-class gate: the rule fires only inside methods of a class
94-
* that extends `Illuminate\Routing\Controller`. FormRequest descendants,
95-
* middleware, services, Actions, jobs, and console commands are silent
96-
* because their class hierarchies do not include Controller.
101+
* whose namespace starts with `App\Http\Controllers`. FormRequest
102+
* (`App\Http\Requests`), middleware (`App\Http\Middleware`), services,
103+
* Actions (`App\Actions`), jobs, and console commands are silent because
104+
* their namespaces do not start with the controller prefix. Mirrors
105+
* `ForbidEloquentMutationInControllersRule` — consumer controllers are
106+
* base-less `final` classes, so an ancestry walk to a base Controller
107+
* catches nothing.
97108
*/
98109
private function insideControllerMethod(Scope $scope): bool
99110
{
100-
$classReflection = $scope->getClassReflection();
111+
$namespace = $scope->getNamespace();
101112

102-
if (!$classReflection instanceof ClassReflection) {
113+
if ($namespace === null) {
103114
return false;
104115
}
105116

106-
if ($classReflection->getName() === self::CONTROLLER_BASE_FQCN) {
107-
return false;
108-
}
109-
110-
return $classReflection->isSubclassOf(self::CONTROLLER_BASE_FQCN);
117+
return str_starts_with($namespace, self::CONTROLLER_NAMESPACE_PREFIX);
111118
}
112119

113120
/**
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
declare(strict_types = 1);
4+
5+
namespace App\Http\Controllers;
6+
7+
use App\Models\User;
8+
use Illuminate\Container\Attributes\CurrentUser;
9+
10+
// Compliant base-less `final` controller (no `extends Controller`) — the
11+
// canonical replacement shape. Container-attribute injection resolves the
12+
// authenticated user, so the rule must NOT fire even though the namespace
13+
// gate now matches this class.
14+
final class CurrentUserAttributeInBaselessController
15+
{
16+
public function store(#[CurrentUser] User $user): User
17+
{
18+
return $user;
19+
}
20+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
declare(strict_types = 1);
4+
5+
namespace App\Http\Controllers;
6+
7+
use Illuminate\Http\Request;
8+
9+
// Regression proof for the namespace-gate fix: a base-less `final` controller
10+
// with NO `extends Controller`. This is the exact real-world shape kendo /
11+
// ublgenie / entreezuil ship — the prior `isSubclassOf(Controller)` ancestry
12+
// gate matched zero such classes, so the rule was a silent no-op. The
13+
// namespace gate (`App\Http\Controllers` prefix) must flag this.
14+
final class RequestUserInBaselessController
15+
{
16+
public function store(Request $request): ?object
17+
{
18+
return $request->user();
19+
}
20+
}

0 commit comments

Comments
 (0)