clusev/docs/superpowers/specs/2026-06-15-confirm-action-t...

16 KiB
Raw Blame History

Confirm-Action token hardening — design

Date: 2026-06-15 Scope: app/Livewire/Modals/ConfirmAction.php + its 14 call sites/handlers across 8 components. Driver: Codex v0.9.0 review (P2) on the generic R5 confirm modal.


1. Problem

ConfirmAction is the shared R5 confirm modal, reused by every confirm flow. It carries client-mutable public properties — event, params, auditAction, auditTarget, serverId — that it dispatches and audits on confirm(). An authenticated user (all accounts are equal admins — no RBAC) can:

  • (a) Bypass the modal — dispatch a #[On(...)] apply handler directly (e.g. keyRemoved) from the browser, skipping the human confirmation step entirely.
  • (b) Tamper with routing — substitute event / params / auditTarget to retarget the destructive action or forge a misleading audit row.

This is not a privilege-boundary crossing: the apply handlers already re-validate the real invariants (can't delete self/last user, channel/tls/domain are allowlisted and re-checked). The fix is therefore scoped to integrity / confirmation, not authorization tiers:

  1. Replace arbitrary client-supplied event dispatch with a server-side allowlist.
  2. Mark the modal's routing inputs #[Locked].
  3. Bind each confirm to a signed, single-use token issued server-side when the modal opens, so a direct apply-handler call without a valid token is rejected and the audit descriptor cannot be forged.

Out of scope (flagged as follow-up): the sibling modals with their own client-mutable routing — HardeningAction (mutable serverId/action/enable) and Fail2banBan (mutable serverId). Same class of exposure, lower severity (each does its own work in apply()/save() and re-validates the server). Tracked separately.


2. The 14 ConfirmAction flows (inventory)

Event Opener Handler (component) Params Audit written in
serviceConfirmed Services/Index applyService (Services/Index) op, name confirm()
userRemoved Settings/Users remove (Settings/Users) userId handler (deferred)
userLoggedOut Settings/Users logout (Settings/Users) userId handler (deferred)
domainChanged System/Index applyDomain (System/Index) domain confirm()
channelChanged System/Index applyChannel (System/Index) channel confirm()
tlsModeChanged System/Index applyTlsMode (System/Index) mode handler (own audit)
fileConfirmed Files/Index deleteEntry (Files/Index) name confirm()
keyRemoved Servers/Show removeKey (Servers/Show) fingerprint confirm()
credentialDeleted Servers/Show deleteCredential (Servers/Show) confirm()
firewallRuleDelete Servers/Show deleteFirewallRule (Servers/Show) spec, label handler (deferred)
webauthnKeyRemoved Settings/WebauthnKeys remove (Settings/WebauthnKeys) id handler (own audit)
sessionsLogoutOthers Settings/Sessions logoutOthers (Settings/Sessions) confirm()
sessionsLogoutAll Settings/Sessions logoutAll (Settings/Sessions) confirm()
twoFactorDisabled Settings/Security disableTwoFactor (Settings/Security) confirm()

The split (audit in confirm() vs. in the handler) is the existing architecture and is preservedconfirm() writes audit only when the opener supplied auditAction; deferred flows audit in the handler after the remote op succeeds.


3. New unit: App\Support\Confirm\ConfirmToken

A small stateless service. Lives at app/Support/Confirm/ConfirmToken.php; companion exception app/Support/Confirm/InvalidConfirmToken.php (extends \RuntimeException).

Responsibilities

  • Allowlist of permitted confirm events (the 14 above) as a public const ACTIONS.
  • issue — server-side token minting at opener time.
  • verify — decode + validate without consuming (modal display + confirm()).
  • consume — verify + single-use burn + event match (apply handlers — the security gate).
  • peek — signature-only decode for display, never throws.

Token shape

Payload (JSON) → Crypt::encryptString(...):

{ nonce, event, params, auditAction, auditTarget, serverId, uid }
  • Crypt (APP_KEY, AES-256 + HMAC) gives tamper-evidence — a forged/edited token fails to decrypt or fails the MAC.
  • Single-use via cache: issue plants confirm-token:{nonce} (Redis, TTL 600s); consume does Cache::pull (atomic get+delete). A replay finds nothing → rejected.
  • uid is bound to Auth::id() and re-checked on decode → no cross-user replay.

API (signatures)

public const ACTIONS = [ /* the 14 event names */ ];

public static function issue(
    string $event, array $params = [], string $auditAction = '',
    ?string $auditTarget = null, ?int $serverId = null,
): string;                                   // throws \InvalidArgumentException if event ∉ ACTIONS

public static function verify(string $token): array;            // throws InvalidConfirmToken
public static function consume(string $token, string $event): array; // throws InvalidConfirmToken
public static function peek(string $token): ?array;             // null on any failure

Validation rules (decode)

  • decrypts cleanly (catch DecryptException) and is valid JSON (catch JsonException);
  • eventACTIONS;
  • uid === Auth::id().

verify adds: nonce still present in cache. consume adds: event arg matches payload event, and Cache::pull returns truthy (single-use). On any failure → InvalidConfirmToken.


4. Modal rewrite: ConfirmAction

  • Remove public props event, params, auditAction, auditTarget, serverId.
  • Add #[Locked] public string $token = ''.
  • Keep display props heading, body, confirmLabel, danger, icon, notify.
  • Replace boot() with mount(): assign $token + display copy, apply the localized fallbacks (default_heading, common.confirm, default_notify) that boot() did. Setting $token in mount() is the initial mount → allowed by #[Locked]; subsequent client round-trips (the confirm click) cannot re-tamper it.
  • Audit-target display line → #[Computed] public function targetLabel(): ?string using ConfirmToken::peek($this->token)['auditTarget'] ?? null. Not client state → no tamper surface.
public function confirm(): void
{
    try {
        $payload = ConfirmToken::verify($this->token);   // non-consuming
    } catch (InvalidConfirmToken) {
        $this->dispatch('notify', message: __('modals.confirm_action.rejected'));
        $this->closeModal();
        return;
    }

    if (($payload['auditAction'] ?? '') !== '') {
        AuditEvent::create([
            'user_id'   => Auth::id(),
            'server_id' => $payload['serverId'] ?? null,
            'actor'     => Auth::user()?->name ?? 'system',
            'action'    => $payload['auditAction'],
            'target'    => $payload['auditTarget'] ?? null,
            'ip'        => request()->ip(),
        ]);
    }

    $this->dispatch($payload['event'], confirmToken: $this->token);

    if ($this->notify !== '') {
        $this->dispatch('notify', message: $this->notify);
    }

    $this->closeModal();
}

Blade change (resources/views/livewire/modals/confirm-action.blade.php): the audit-target line switches from @if ($auditTarget) ... {{ $auditTarget }} to @if ($this->targetLabel) ... {{ $this->targetLabel }}. No other markup changes.

Implementation risk to verify first: confirm wire-elements/modal passes the arguments through to mount() (it does — same path the other modals use). If a #[Locked] public prop set from arguments ever threw, the mount() assignment avoids it; both paths covered.


5. Openers (14, across 8 files)

Replace the routing arguments (event, params, auditAction, auditTarget, serverId) with a single 'token'. Display copy is unchanged. Example (Servers/Show::confirmRemoveKey):

$this->dispatch('openModal',
    component: 'modals.confirm-action',
    arguments: [
        'heading'      => __('servers.confirm_remove_key_heading'),
        'body'         => __('servers.confirm_remove_key_body', ['name' => $comment, 'server' => $this->server->name]),
        'confirmLabel' => __('common.remove'),
        'danger'       => true,
        'icon'         => 'trash',
        'notify'       => __('servers.notify_key_removed', ['name' => $comment]),
        'token'        => ConfirmToken::issue(
            'keyRemoved',
            ['fingerprint' => $fingerprint],
            'ssh_key.remove',
            "{$comment} · {$this->server->name}",
            $this->server->id,
        ),
    ],
);

Openers that passed no auditAction (deferred flows) pass ''; no serverIdnull.


6. Apply handlers (14)

Replace domain params with string $confirmToken; consume + read params from the verified payload. Injected services (by type) coexist with the token param (by name). Example:

#[On('keyRemoved')]
public function removeKey(string $confirmToken, FleetService $fleet): void
{
    try {
        $payload = ConfirmToken::consume($confirmToken, 'keyRemoved');
    } catch (InvalidConfirmToken) {
        return;   // forged / replayed / direct-bypass attempt — silent no-op
    }
    $fingerprint = $payload['params']['fingerprint'];
    // ... unchanged ...
}

Rules:

  • Handlers read only $payload['params'] — any client-passed value is ignored.
  • Handlers that already re-validate (applyChannel, applyTlsMode, applyDomain, remove, logout) keep those checks — defense in depth.
  • Handlers stay silent on a rejected token (no toast) — no oracle for a prober. The user-facing rejection notice is shown by the modal's confirm() only.

7. Security model

  • Single-use consume at the handler — the destructive op is the gate. A direct #[On] call without a valid, unburned token is a no-op. → Closes vector (a).
  • event / params / auditTarget come only from the signed token — they can't be retargeted and the audit descriptor can't be forged. → Closes vector (b).
  • confirm() verifies non-consuming and writes audit from the verified payload; the handler burns the token single-use.
  • Residual (documented, accepted): a scripted confirm() replay before the handler round-trip consumes the nonce could write duplicate audit rows — but they are correctly attributed to the actor for an action they authorized. Negligible under the equal-admins model. After the handler burns the nonce, all replays fail verify.

8. Tests

Unit — tests/Unit/Support/Confirm/ConfirmTokenTest.php (actingAs a user, array cache):

  • issueverify/consume round-trips and returns the payload.
  • issue with an event ∉ ACTIONS throws \InvalidArgumentException.
  • consume twice → second throws InvalidConfirmToken (single-use).
  • consume($token, 'otherEvent') → throws (cross-action / event mismatch).
  • garbage / edited token → verify & consume throw; peek returns null.
  • token issued as user A, Auth::id() = user B → throws (uid binding).

Feature — tests/Feature/Modals/ConfirmActionTest.php (RefreshDatabase):

  • happy path: mount with a valid token → call('confirm') → asserts the event was dispatched with confirmToken and (for an auditAction flow) exactly one AuditEvent row written from the payload.
  • forged token: call('confirm') → no AuditEvent, event not dispatched, notify = modals.confirm_action.rejected.
  • ->set('token', '...') on the mounted modal throws (Locked).

Feature — handler bypass (representative, no SSH): tests/Feature/Settings/SecurityConfirmTest.php:

  • dispatch twoFactorDisabled with no / invalid confirmToken → 2FA stays enabled, no audit.
  • with a valid issued token → 2FA disabled; a second dispatch with the same token → no-op (single-use).

All tests use RefreshDatabase + factories — never seed the dev DB (per project rule).


9. Localization (R16)

Add to lang/de/modals.php and lang/en/modals.php under confirm_action:

  • rejected — de: Bestätigung ungültig oder abgelaufen. · en: Confirmation invalid or expired.

(Keys identical in both files.)


10. Verification (R8 / R12 / R15)

  1. Run the suite in-container: docker compose exec app php artisan test --filter='ConfirmToken|ConfirmAction|SecurityConfirm' then the full suite.
  2. R12 browser probe every touched route — HTTP 200, zero console/network errors, loaded state (not skeleton), at 375 / 768 / 1280: servers/show, services, files, system, settings/security, settings/sessions, settings/users, settings/webauthn-keys. Open a confirm modal on each and confirm one action end-to-end.
  3. R15 /codex:review over the diff → fix + re-run until 0 errors / 0 security findings.
  4. git status for stray secrets → commit in sensible steps.

11. File touch list

New: app/Support/Confirm/ConfirmToken.php, app/Support/Confirm/InvalidConfirmToken.php, 3 test files. Edit: app/Livewire/Modals/ConfirmAction.php, resources/views/livewire/modals/confirm-action.blade.php, app/Livewire/Services/Index.php, app/Livewire/Settings/Users.php, app/Livewire/Settings/Security.php, app/Livewire/Settings/Sessions.php, app/Livewire/Settings/WebauthnKeys.php, app/Livewire/System/Index.php, app/Livewire/Files/Index.php, app/Livewire/Servers/Show.php, lang/de/modals.php, lang/en/modals.php.


12. Post-review revisions (Codex R15)

The first Codex pass REJECTED the initial design (§3§7). Three classes of finding were valid and are now folded into the implementation:

A. Two-phase lifecycle (was: single nonce checked at confirm + consume). The original token was consumable the instant issue() returned, so an attacker who invoked an opener could grab the token and call the apply handler directly — still skipping the modal (and, for confirm-audited flows, skipping the audit). The nonce now carries state: issue()pending; ConfirmToken::confirm($token) (called only by the modal's confirm()) flips pendingconfirmed; consume($token, $event) requires confirmed, then deletes. A never-confirmed token is rejected by the handler — the modal must run. This also removes the §7 "duplicate-audit on replay" residual: confirm() transitions at most once.

B. Atomic transitions. Cache::pull is get-then-forget (not atomic), so concurrent handler requests could double-apply. Both confirm() and consume() now run their check-and-mutate inside Cache::lock($key.':lock', 10)->get(...) (array + Redis both provide locks).

C. Server-scope binding. Server-scoped actions seal the server id and the handler verifies it against live state before acting — closing token retargeting to another host/path:

  • Servers/Show removeKey/deleteCredential/deleteFirewallRule: reject unless $payload['serverId'] === $this->server->id (firewall opener now seals serverId).
  • Files/Index deleteEntry: opener seals the full path + active server id; handler rejects on server mismatch and deletes the sealed path (never a client-recombined one).
  • Services/Index applyService: opener seals the active server id; handler rejects on mismatch.

Method shape became: issue(), confirm() (modal), consume() (handler), peek() (display). verify() was removed. Added tests: ConfirmServerScopeTest, plus two-phase / unconfirmed-token cases in the unit + Security/Modal feature tests.