Skip to content

fix!: harden audit routes and unify login gate (BREAKING — release as v0.5.0)#14

Merged
bherila merged 2 commits into
mainfrom
fix/vora-integration-footguns
Jun 14, 2026
Merged

fix!: harden audit routes and unify login gate (BREAKING — release as v0.5.0)#14
bherila merged 2 commits into
mainfrom
fix/vora-integration-footguns

Conversation

@bherila

@bherila bherila commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Summary

Four integration bugs were surfaced in a consuming app (vora). This PR fixes the three findings that the package itself can prevent; the fourth is purely app-side.

Finding 1 (P1) — Access-control bypass on audit routes: FIXED

Root cause: The package's audit routes were protected by auth middleware only. If a consuming app's Gate ability for audit.admin_ability only checked is_admin without verifying approved_at/is_disabled, a pending or disabled admin could reach the cross-user admin endpoint.

Fix:

  • Added AuthUserPolicy::canLogin(Authenticatable $user, Request $request): bool to the contract — the single gate for all account-state checks (active, approved, not disabled, etc.).
  • DefaultAuthUserPolicy::canLogin() duck-types $user->canLogin(), falls back to is_disabled, defaults to true. canPasskeyLogin() now delegates to canLogin() so both share one implementation.
  • New RequireActiveUser middleware aborts 403 when the authenticated user fails canLogin(). Applied automatically on all package audit routes (in addition to auth).
  • Added a warning comment in bherila-auth.php config that admin_ability gate definitions must check account state as well as role.

Finding 2 (P1) — Lockout-after-migration of approved_at: DOCUMENTED

Root cause: Apps adding an approved_at (nullable, null = pending) column to their users table leave all existing rows as null after migration, instantly locking out every current user.

Why package can't fully fix this: The approved_at column is app-owned — the package does not ship a migration for the user table. There is nothing to backfill in this repo.

Fix: Added a prominent README section with a copy-paste backfill snippet explaining the requirement and the risk.

Finding 3 (P2) — Post-verification redirect divergence: DOCUMENTED

Root cause: A consuming app hardcoded /pending in its email-verification callback instead of calling AuthUserPolicy::redirectAfterLogin(), so an already-approved user who verified their email was falsely shown the pending page.

Why package can't fully fix this: The package does not ship an email-verification controller — that is app-owned. The bug was in vora's controller.

Fix: Added README guidance that after email verification the app must call redirectAfterLogin() (not a hardcoded path), and that canLogin() should be checked at every entry point including verification callbacks.

Finding 4 (P3) — Dropdown outside-click bug: NOT APPLICABLE

The package ships no navbar, dropdown, or menu components. The bug was in vora's own React component. No change.

Test plan

  • All 40 existing tests continue to pass (composer test)
  • 7 new assertions in RequireActiveUserTest cover: canLogin() default allow, canLogin() via duck-typed canLogin() method, is_disabled fallback, canPasskeyLogin() delegation to canLogin(), middleware pass-through for active user, middleware 403 for inactive user, middleware pass-through for unauthenticated (lets auth middleware handle it)
  • Squash-merge this PR (single logical change)

🤖 Generated with Claude Code

bherila and others added 2 commits June 13, 2026 17:19
…footguns

Four integration bugs surfaced in a consuming app (vora); this commit
prevents the package-preventable ones:

**Finding 1 (P1 — access-control bypass)**
- Add `AuthUserPolicy::canLogin()` to the contract with a docblock
  explaining it is the single gate for account state (active, approved,
  not disabled). The default implementation duck-types `$user->canLogin()`
  then falls back to `is_disabled`, matching the previous
  `canPasskeyLogin()` logic.
- `DefaultAuthUserPolicy::canPasskeyLogin()` now delegates to
  `canLogin()` so both code paths share one implementation.
- New `RequireActiveUser` middleware calls `policy->canLogin()` and
  aborts 403 when the user is not allowed in. Applied automatically on
  all package audit-log routes (previously only `auth` middleware was
  applied, letting a pending/disabled admin reach the admin endpoint if
  their Gate ability only checked role).
- Config comment on `audit.admin_ability` warns that the gate definition
  must check both role and account state.

**Finding 2 (P1 — lockout-after-migration)**
- Package does not own the `approved_at` column (it is app-side), so
  there is nothing to migrate-fix. Added prominent README guidance with
  a code example showing how to backfill the column in the migration that
  adds it, so integrators do not lock out existing users.

**Finding 3 (P2 — onboarding redirect divergence)**
- Package does not ship an email-verification controller (app-owned), so
  the hardcoded `/pending` redirect was not in this repo.
- Added README guidance: after email verification the app must call
  `AuthUserPolicy::redirectAfterLogin()` (not a hardcoded path), so an
  already-approved user who verifies their email is sent to the correct
  destination instead of the pending page.

**Finding 4 (P3 — dropdown outside-click)**
- The package ships no navbar/dropdown/menu components; this bug is
  entirely app-side. No change.

Tests: 7 new assertions covering `canLogin()`, the `is_disabled` fallback,
the `canPasskeyLogin()` delegation, and the middleware pass/block/unauthenticated
cases. All 40 tests pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adding canLogin() to the AuthUserPolicy contract is a BC break for direct
implementers. Document that this must ship as v0.5.0 (not a 0.4.x patch, which
^0.4 consumers would auto-pull and fatal on) and how integrators migrate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bherila bherila changed the title fix: harden audit routes and unify login gate to prevent integration footguns fix!: harden audit routes and unify login gate (BREAKING — release as v0.5.0) Jun 14, 2026
@bherila

bherila commented Jun 14, 2026

Copy link
Copy Markdown
Owner Author

⚠️ Breaking change — must be tagged v0.5.0, not v0.4.x.

This PR adds a required canLogin(Authenticatable, Request): bool method to the AuthUserPolicy contract. Direct implementers of that interface (e.g. vora's VoraAuthUserPolicy) will fatal until they add the method, so the upgrade must be opt-in.

The package versions via git tags (latest v0.4.3) and consumers pin ^0.4 (>=0.4.0 <0.5.0). Tagging this as v0.4.4 would auto-upgrade those consumers and break them. Tag the merge as v0.5.0. Consumer migration steps are in the new README “Upgrading to v0.5.0” section.

Squash-merge per repo convention.

@bherila bherila merged commit f5707cd into main Jun 14, 2026
4 checks passed
@bherila bherila deleted the fix/vora-integration-footguns branch June 14, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant