Security hardening (FAZ 1-3): close confirmed authz defects, secure-by-default + multi-DB CI#102
Merged
Merged
Conversation
Add five read-only Claude Code review agents (laravel-architect, security-pentest, test-quality, data-integrity, db-engine-specialist) that review the diff before a PR against AAuth's real failure modes, with zero-data-leak as the top red line, plus a /pre-pr-review orchestrator that fans them out and produces one go/no-go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that a top-level where()/orWhere() on an AAuth-scoped model cannot leak rows outside the active role's org subtree — Laravel groups scope-added wheres separately, so the consumer OR stays AND'ed with the OrBAC/ABAC constraints. Verified with role-5 (node 2) not seeing a node-3 row via orWhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- F1 updateWith/deleteWithAAuthOrganizationNode: bind to real OrganizationService signatures (persist node changes then recompute subtree paths; pass model not id), wrapped in a transaction - F2 Role::permissions(): scope to this role (stop all-roles leak) - F3 getAssignedUserCountAttribute: distinct users, not pivot rows - F4 syncPermissionsOfRole: wrap detach+attach in a transaction - F5 empty accessible-node set now returns ZERO rows (fail closed) via a single empty-guarded builder; organizationNodes()/getAccessibleOrganizationNodes() delegate to it (kills the N+1 and the whole-table over-exposure) - F6 pgsql seed: advance the organization_nodes sequence (not scopes twice) - F7 cache invalidation now uses the configured cache.store in all sites Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes the confirmed HIGH findings directly (documented behaviour changes): - S1 parametric permissions fail CLOSED (positional): no-arg on a parametric perm denies; missing/non-numeric-vs-int/unknown → deny; add string exact-match; keep positional matching (middleware/Blade/Gate pass args positionally) - S2 deactivated (status='passive') roles are rejected in the constructor and hidden from switchable-role listings — deactivateRole is now an effective kill switch - S3 Gate::before defers to a host Policy when one handles the ability for the model (no IDOR via name-collision policy shadowing) - S4 createWith/updateWith/deleteWithAAuthOrganizationNode authorize the target node against the active role's subtree (context-bound; seeders/ console/queue without a context are skipped) - S5 Role type/organization_scope_id are no longer mass-assignable; the service sets them explicitly (no org→system escalation via raw input) - S6 descendant() anchored to the '/' separator (no sibling-prefix match) - S7 ABAC rule 'attribute' validated as a bare column identifier (no SQL/column injection from stored rules) - S8 passOrAbort() gains an additive optional $arguments to reach can() - S9 attachOrganizationRoleToUser authorizes the target node (context-bound) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- docker-compose.yml: add PostgreSQL (:54322) alongside MariaDB (:33062), both healthchecked - composer.json: test:sqlite / test:mariadb / test:pgsql / test:all scripts - GitHub Actions: run the suite against BOTH MariaDB and PostgreSQL in every PHP/Laravel/stability cell - .gitlab-ci.yml: quality (phpstan+pint) + test:mariadb + test:postgres - README-contr.md: contributor + agent guide (setup, multi-DB tests, gates, pre-PR review agents); README.md pointer - add db-engine-specialist review agent; security-pentest gains the where/orWhere no-leak check Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Independent /pre-pr-review audit (5 agents) on the FAZ 1+2 diff surfaced two security issues + hardening, all fixed here: - BLOCKER (security-pentest): updateWithAAuthOrganizationNode authorized only the destination parent, not the node being moved — a role could re-parent a node from another subtree into its own and escalate over its whole subtree. Now authorizes BOTH $nodeId and the parent, and verifies the node belongs to the model. New negative test. - HIGH (security-pentest): assertOrganizationNodeAuthorized() skipped enforcement on ANY unresolvable context, conflating console/seeders with unauthenticated HTTP. Now skips ONLY in console; an HTTP request with no/invalid role is denied. - HIGH (data-integrity): createOrganizationNode two-step path write wrapped in a transaction (no orphan '/?' placeholder path). - Seeder used Role::create() with the now-narrowed $fillable → forceCreate() so the trusted seed keeps setting type/organization_scope_id. - BLOCKER (test-quality): added cross-subtree create/update/delete + role-assignment negative tests (Faz2WriteAuthzTest); string-constraint + switchable-active tests. - Disclosure: CHANGELOG.md + UPGRADE.md enumerate the behaviour changes. - Align code style + CI/docs to the project's PHP-CS-Fixer (not Pint); php-cs-fixer --dry-run is clean. 155 tests green (SQLite), phpstan clean, php-cs-fixer clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Safe FAZ 3 items endorsed by the independent review (breaking FK/UNIQUE migrations, PHPStan un-exclude and the stored depth column remain a documented follow-up): - DRY (laravel-architect): getAccessibleOrganizationNodes() now delegates to the single empty-guarded organizationNodesQuery() builder — removes the third copy of the path-LIKE loop and its own empty-scope guard. - Postgres index (db-engine-specialist): add a driver-conditional varchar_pattern_ops index on organization_nodes(path) so pgsql uses an index for the hot 'path LIKE prefix/%' subtree queries instead of a seq scan. Additive + pgsql-only; MySQL/MariaDB/SQLite untouched (portability preserved). 155 tests green, phpstan clean, php-cs-fixer clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Composer 2.10+ refuses to resolve dependency versions that have published security advisories, which breaks the --prefer-lowest CI cells (they install older, now-advisory-affected Laravel 11.x releases) → 'could not be resolved'. Set config.policy.advisories.block=false so AAuth's OWN CI/dev resolution can install those versions for the matrix. This config lives in AAuth's root composer.json and does NOT propagate to downstream consumers (only the root project's config applies), so their security posture is unchanged. Verified locally with composer 2.10.2: prefer-lowest goes from 'could not be resolved' to a clean 'Lock file operations' after the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Parallel pass adding precise PHPStan types (array<K,V> on validation rules, event/exception/service params; $this generics on Eloquent relations; @Property on the user contract) across 20 src files. Reduces the level-7 error count from 101 to a small residual of Larastan library-trait / facade quirks, toward removing the suppressions. No runtime behaviour change; 155 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every matrix cell now installs pgsql/pdo_pgsql, boots a postgres:16 service, and runs pest a second time with DB_CONNECTION=pgsql — so all tests must pass on both MySQL and PostgreSQL (materialized-path LIKE, depth whereRaw, pgsql seed sequence and JSON columns are driver-sensitive). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e 25->8 Completes the PHPStan cleanup toward zero-suppression WITHOUT breaking changes: - Un-exclude both security scopes (AAuthOrganizationNodeScope, AAuthABACModelScope) — they are now fully analysed. - Remove the blanket `missingType.iterableValue` ignore and all inline @PHPStan-Ignore comments; the real array/relation/generic types are annotated instead (parallel pass across src). - Reduce the baseline from 25 entries to 8 (14 errors), each a genuine irreducible Larastan/library/query quirk documented in the baseline header (qualified-column where() on joins, $id contract access, library trait.unused, contract relation variance). Removing these would require breaking the public contract API, so they stay baselined. - Revert the exploratory `__get()` addition to AAuthUserContract (would have been a breaking interface change). phpstan clean, 155 tests green, php-cs-fixer clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The phpstan job pinned PHP 8.2, but composer.lock resolves laravel/framework 13 (which requires PHP 8.3+). On 8.2 the analysis saw a non-generic Eloquent Scope and rejected the `@implements Scope<...>` docblocks. Running on 8.3 installs the locked Laravel 13, so Scope is generic and phpstan matches local. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 2 were spurious (collect() calls) — removed cleanly. - new AAuth(Auth::user(), ...): annotate the resolved user as AAuthUserContract|null. - Role::$organizationNode: type the @Property nullable (find() may return null) + cast the id to int so the assignment is well-typed. src now contains ZERO inline suppressions; the only remaining suppression is the documented 8-entry baseline of irreducible Larastan/library/query quirks. phpstan clean, 155 tests green, php-cs-fixer clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-architect The pre-PR laravel-architect agent now continuously reviews: - LA11 Cache security: every cache key must encode the FULL security context (user + active role + scope + permission) — flags cross-user data bleed, cross-role privilege escalation, and stale-allow-after-revoke as BLOCKERs; also store/tags/TTL correctness. - LA12 Octane: scoped() (not singleton()) binding, no request-surviving static or per-user mutable state (no A->B bleed on a shared worker). Plus two new red-lines (cache authorization leak; Octane state-bleed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r-instance
Cache was over-engineering for this package (architect C1): the role cache
re-loaded the exact relations it eager-loaded, so it guarded ~1 query/request
behind a whole invalidation apparatus, plus dead forget() keys and a
multi-tenant key-collision risk. Removed entirely:
- getCachedRole/getCachedSwitchableRoles + the aauth-advanced.cache config
- the RoleObserver/RolePermissionObserver cache-forget + RolePermissionService
clearUserRoleCache (observers keep only event dispatch + live-context refresh)
Per-request loading is now the ONLY mechanism and it is correct on both PHP-FPM
and Octane: the scoped('aauth') instance loads role+permissions+ABAC ONCE in the
constructor (loadRole eager-load; loadMissing() drops the redundant re-query) and
every can() reads from that in-memory context — no per-check DB query.
Security: the auth context now lives on the request-scoped INSTANCE, not in the
shared Context store under a fixed 'aauth_context' key. Two AAuth instances in one
request (e.g. impersonation / a job over several users) can no longer overwrite and
read each other's permissions. New regression test proves no cross-user leak; the
Octane scoping test still passes.
153 tests green, phpstan clean, php-cs-fixer clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hitect The pre-PR architect now also enforces: authorization data loaded once per request (no per-check DB query, loadMissing over load, no persistent cache unless it earns its keep), and SOLID in its lean reading (one responsibility, no speculative layers/second source of truth). North Star updated to name SOLID explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The persistent cache was removed in the previous commit; drop the now-false docs that still advertised it (README Performance/Cache sections, UPGRADE cache config + 'Cache Issues' runbook, CHANGELOG fix clause, config header comment). Replace with a short note that authorization data is loaded once per request into the scoped instance — no cross-request cache. Flagged by the laravel-architect + data-integrity review agents (doc drift). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ENTS.md Doc-quality pass (findings from the laravel-architect doc audit, all verified vs code): - Rewrote the Organization (OrBAC) and ABAC concept sections with a tree diagram, the real rule format, and the secure-by-default 'no rule = sees all rows' guarantee. - Fixed stale/incorrect content: features list no longer advertises the removed cache; service-provider example shows scoped() (not singleton); withoutGlobalScopes()->get() (was ->all(), not runnable); UPGRADE support matrix (Laravel 11-13, PHP 8.2-8.4, SQLite/MySQL/MariaDB/Postgres); super_admin config path (aauth-advanced.php); two mislabeled attach/detach headers now match their code. - Documented previously-missing behaviour: the Blade directives (@aauth/@aauth_can/ @aauth_role/@aauth_super_admin) and the constructor 'when AAuth throws' prerequisites. - Removed Laravel Boost: deleted resources/boost/guidelines/core.blade.php and the misplaced .ai/guidelines/core.blade.php (both stale — still described the cache). Boost is app-level AI tooling and its package file drifted; replaced with a lean, tool-agnostic AGENTS.md (Claude Code / Cursor / Copilot) carrying the non-negotiable rules (zero-leak, secure-by-default, no-breaking, lean, Octane-safe) + the quality gates. No code change; phpstan clean, 153 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Secure-by-default fixes for confirmed authorization defects (no config flags), with regression tests, multi-DB CI, and a pre-PR review-agent suite. No breaking API changes.
Security fixes (FAZ 2)
passOrAbortgains an optional$argumentsparampassive) roles are rejected and excluded from the switchable listGate::beforedefers to a host Policy when one handles the ability (no IDOR by name-collision)createWith/updateWith/deleteWithAAuthOrganizationNodeandattachOrganizationRoleToUserauthorize the target node against the active role's subtree —updateWithalso authorizes the node being moved (fixes a cross-org re-parent escalation)Roleprivilege columns (type,organization_scope_id) are no longer mass-assignabledescendant()is/-separator-anchored; ABAC rule attributes are allowlisted; an empty accessible-node set returns zero rows (fail closed)Bug fixes (FAZ 1)
Role::permissions()all-roles leak; assigned-user count; non-atomic permission sync & org-node create; pgsql seed sequence; cache invalidation honours the configured store; runtime-fatal org-node update/delete trait helpers.FAZ 3 (partial, non-breaking, audit-recommended)
getAccessibleOrganizationNodesdelegates to the single empty-guarded builder (DRY); driver-conditional Postgresvarchar_pattern_opsindex for the hotpath LIKE 'prefix/%'subtree queries.Verification
updateWithnode authz; no-context fail-open)docker-compose.ymlis provided for local MariaDB/PostgresNotes / follow-up
run-tests.ymlmulti-DB update was held out of this PR (the pushing token lackedworkflowscope). GitLab CI already covers MariaDB + PostgreSQL; the Actions update can be applied in a small follow-up.UPGRADE.md): FK/UNIQUE integrity migrations (need orphan cleanup); PHPStan un-exclude of the two scope files; a storeddepthcolumn.UPGRADE.md+CHANGELOG.mdfor the observable behaviour changes.🤖 Generated with Claude Code