From d55c3f41df9705fe533d13f3fa9da0cf7790cb4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Fri, 3 Jul 2026 14:49:32 +0200 Subject: [PATCH] fix(security): scope job-status endpoints to owner + feature-area fixes (#627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Starts from a security finding on the background-job status endpoints and folds in the most important issues surfaced by a follow-up review of the same feature area (categorization backfill, bulk rule re-evaluation, automation-rule apply). Each change is its own commit. ## Changes ### 1. `fix(security)` — scope job-status cache keys to the owning user The categorization, bulk re-evaluation, and apply status endpoints looked jobs up by a bare job UUID with no user scoping. Any authenticated user who obtained another user's job id could poll its progress payload. Cache keys now include the owning user's id, so a status request keyed by the polling user's id resolves only that user's own jobs — a mismatched owner falls through to the existing 404. No ownership store or extra lookup. Cross-user isolation tests added for all three endpoints. ### 2. `fix(automation-rules)` — re-check `only_uncategorized` at apply time The apply flow cached a match snapshot for up to 15 min (keyed only by `rule.updated_at`) and applied the rule's category to every id in it without re-checking eligibility. A transaction categorized *after* the snapshot (by the user, a sibling rule, or a concurrent AI backfill) was silently re-categorized and stamped `category_source=Rule`. Now re-filtered through `shouldSkipForOnlyUncategorized()` at apply time; skipped rows are no longer counted as changed. ### 3. `perf(automation-rules)` — memoize rule set per user in `applyRules()` Bulk re-evaluation calls `applyRules()` once per transaction, and each call re-queried the user's whole rule set + labels — an N+1 scaling with transaction count (the sibling apply job already loaded rules once). Memoized per user for the service instance's lifetime (resolved fresh per job, so rules created mid-run are intentionally not seen). Query-count test asserts one `automation_rules` query regardless of transaction count. ### 4. `fix(transactions)` — stop polling when the job never starts The re-evaluate and apply pollers rescheduled on any non-terminal status, including `pending`. If the queue worker is down the job never runs, `failed()` never fires, and the client polled for the full hour-long TTL with a stuck spinner. Now gives up after 30 consecutive `pending` ticks (~30s), mirroring the guard the AI-categorization poller already has. Long `processing` runs are unaffected. ### 5. `test(automation-rules)` — cover apply job execution and failure branches `ApplySingleAutomationRuleJob` was only asserted to be *pushed*; its `handle()` body and `failed()` branch had no coverage, and `ReEvaluateTransactionRulesJob::failed()` was untested too. Added direct `handle()`/`failed()` tests pinning the progress-cache payloads. ## Deferred follow-ups (surfaced by review, not in this PR) - **Consolidate the three frontend pollers onto `usePollJobStatus`** — they hand-roll `setTimeout` loops and don't tear down on unmount (can `setState`/`onClose` after the dialog closes). The hook already exists and is unmount-safe; routing all three through it would also make the `pending` cap unit-testable. The cap in change #4 currently mirrors the already-shipped AI poller and is not separately unit-tested. - **`noteAlreadyPresent()` uses a substring match** (`AutomationRuleService`) — a rule note that is a substring of an existing note is silently not appended. Should compare note lines exactly. - **No dedup/lock on concurrent apply/re-evaluate jobs** — note appends are a non-atomic read-then-write, so two concurrent jobs can duplicate a note. Category/label writes are already idempotent. - **Transient 404 → false "failed"** — a mid-run cache eviction/TTL-expiry (or the deploy window of change #1, where an in-flight job's pre-deploy key is orphaned) makes the poller surface a false failure. Retry a few times on transient errors before giving up. - **`matches()` pagination** advances `next_offset` by fetched rows, not page-window size — deleted ids can stall infinite scroll before reaching `total`. - **"Apply to N" count / encrypted-skip counts** can be stale or under-report vs what's actually processed. - **Job-trait convention drift** between the three jobs (modern `Queueable` vs legacy trait stack). ## Testing - `vendor/bin/pint --test` — pass - `bun run format` / `bun run lint` — pass (one pre-existing unrelated warning in `chart.tsx`) - Affected Pest suites (apply, re-evaluate, evaluation, rule, categorization, apply-rule-suggestions) — 94 tests pass --- .../Ai/StartCategorizationBackfill.php | 2 +- .../Ai/CategorizationController.php | 5 +- .../ReEvaluateTransactionRulesController.php | 4 +- .../AutomationRuleApplicationController.php | 8 +- app/Jobs/ApplySingleAutomationRuleJob.php | 9 +- ...CategorizeUncategorizedTransactionsJob.php | 8 +- app/Jobs/ReEvaluateTransactionRulesJob.php | 6 +- app/Services/AutomationRuleService.php | 50 ++++++++- .../apply-automation-rule-flow.tsx | 8 ++ .../use-re-evaluate-all-transactions.tsx | 8 ++ resources/js/pages/transactions/index.tsx | 10 +- ...ategorizeUncategorizedTransactionsTest.php | 22 +++- .../Feature/AutomationRuleApplicationTest.php | 104 ++++++++++++++++++ .../ReEvaluateTransactionRulesTest.php | 74 ++++++++++++- 14 files changed, 281 insertions(+), 37 deletions(-) diff --git a/app/Actions/Ai/StartCategorizationBackfill.php b/app/Actions/Ai/StartCategorizationBackfill.php index b8506b66..abb88c6c 100644 --- a/app/Actions/Ai/StartCategorizationBackfill.php +++ b/app/Actions/Ai/StartCategorizationBackfill.php @@ -37,7 +37,7 @@ class StartCategorizationBackfill $jobId = (string) Str::uuid(); Cache::put( - CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId), + CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId), ['status' => 'pending', 'processed' => 0, 'total' => $total, 'applied' => 0], now()->addHour(), ); diff --git a/app/Http/Controllers/Ai/CategorizationController.php b/app/Http/Controllers/Ai/CategorizationController.php index 7c705d3b..158b3136 100644 --- a/app/Http/Controllers/Ai/CategorizationController.php +++ b/app/Http/Controllers/Ai/CategorizationController.php @@ -5,6 +5,7 @@ namespace App\Http\Controllers\Ai; use App\Http\Controllers\Controller; use App\Jobs\CategorizeUncategorizedTransactionsJob; use Illuminate\Http\JsonResponse; +use Illuminate\Http\Request; use Illuminate\Support\Facades\Cache; class CategorizationController extends Controller @@ -12,9 +13,9 @@ class CategorizationController extends Controller /** * Return current progress for a consent-triggered categorization backfill. */ - public function status(string $jobId): JsonResponse + public function status(Request $request, string $jobId): JsonResponse { - $progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($request->user()->id, $jobId)); if ($progress === null) { return response()->json(['message' => 'Job not found.'], 404); diff --git a/app/Http/Controllers/ReEvaluateTransactionRulesController.php b/app/Http/Controllers/ReEvaluateTransactionRulesController.php index 90e0cb13..40ecdddf 100644 --- a/app/Http/Controllers/ReEvaluateTransactionRulesController.php +++ b/app/Http/Controllers/ReEvaluateTransactionRulesController.php @@ -47,7 +47,7 @@ class ReEvaluateTransactionRulesController extends Controller // Set initial pending state so the first poll returns something meaningful Cache::put( - ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId), + ReEvaluateTransactionRulesJob::cacheKeyForJobId($user->id, $jobId), ['status' => 'pending', 'processed' => 0, 'total' => 0, 'updated' => 0], now()->addHour(), ); @@ -64,7 +64,7 @@ class ReEvaluateTransactionRulesController extends Controller */ public function status(Request $request, string $jobId): JsonResponse { - $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId); + $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($request->user()->id, $jobId); $progress = Cache::get($cacheKey); if ($progress === null) { diff --git a/app/Http/Controllers/Settings/AutomationRuleApplicationController.php b/app/Http/Controllers/Settings/AutomationRuleApplicationController.php index 84f16281..a72ec5d0 100644 --- a/app/Http/Controllers/Settings/AutomationRuleApplicationController.php +++ b/app/Http/Controllers/Settings/AutomationRuleApplicationController.php @@ -103,7 +103,7 @@ class AutomationRuleApplicationController extends Controller ->with(['account.bank', 'category', 'labels']) ->get(); - $changed = $service->applyRuleActionsToTransactions($transactions, $automationRule); + $changed = $service->applyRuleActionsToTransactions($transactions, $automationRule, $onlyUncategorized); $applied = $transactions->count(); @@ -121,12 +121,12 @@ class AutomationRuleApplicationController extends Controller $jobId = (string) Str::uuid(); Cache::put( - ApplySingleAutomationRuleJob::cacheKeyForJobId($jobId), + ApplySingleAutomationRuleJob::cacheKeyForJobId($automationRule->user_id, $jobId), ['status' => 'pending', 'processed' => 0, 'total' => $total, 'applied' => 0, 'updated' => 0], now()->addHour(), ); - ApplySingleAutomationRuleJob::dispatch($automationRule, $jobId, $matchingIds); + ApplySingleAutomationRuleJob::dispatch($automationRule, $jobId, $matchingIds, $onlyUncategorized); $this->forgetMatchesCache($automationRule, $onlyUncategorized); @@ -141,7 +141,7 @@ class AutomationRuleApplicationController extends Controller */ public function status(Request $request, string $jobId): JsonResponse { - $progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($request->user()->id, $jobId)); if ($progress === null) { return response()->json(['message' => 'Job not found.'], 404); diff --git a/app/Jobs/ApplySingleAutomationRuleJob.php b/app/Jobs/ApplySingleAutomationRuleJob.php index 688fc8a0..bfb05744 100644 --- a/app/Jobs/ApplySingleAutomationRuleJob.php +++ b/app/Jobs/ApplySingleAutomationRuleJob.php @@ -27,6 +27,7 @@ class ApplySingleAutomationRuleJob implements ShouldQueue public AutomationRule $rule, public string $jobId, public array $transactionIds, + public bool $onlyUncategorized = false, ) {} public function handle(AutomationRuleService $service): void @@ -46,7 +47,7 @@ class ApplySingleAutomationRuleJob implements ShouldQueue ->whereNull('description_iv') ->with(['account.bank', 'category', 'labels']) ->chunkById(100, function ($transactions) use ($service, $rule, $total, &$processed, &$applied, &$changed) { - $changed += $service->applyRuleActionsToTransactions($transactions, $rule); + $changed += $service->applyRuleActionsToTransactions($transactions, $rule, $this->onlyUncategorized); $applied += $transactions->count(); $processed += $transactions->count(); @@ -69,14 +70,14 @@ class ApplySingleAutomationRuleJob implements ShouldQueue ); } - public static function cacheKeyForJobId(string $jobId): string + public static function cacheKeyForJobId(string $userId, string $jobId): string { - return "apply_automation_rule_job_{$jobId}"; + return "apply_automation_rule_job_{$userId}_{$jobId}"; } private function cacheKey(): string { - return self::cacheKeyForJobId($this->jobId); + return self::cacheKeyForJobId($this->rule->user_id, $this->jobId); } /** diff --git a/app/Jobs/CategorizeUncategorizedTransactionsJob.php b/app/Jobs/CategorizeUncategorizedTransactionsJob.php index 07ce36c2..dcea2e7d 100644 --- a/app/Jobs/CategorizeUncategorizedTransactionsJob.php +++ b/app/Jobs/CategorizeUncategorizedTransactionsJob.php @@ -41,9 +41,9 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue return (string) config('ai_categorization.queue'); } - public static function cacheKeyForJobId(string $jobId): string + public static function cacheKeyForJobId(string $userId, string $jobId): string { - return "categorize_transactions_job_{$jobId}"; + return "categorize_transactions_job_{$userId}_{$jobId}"; } public function handle(AiCategorizationGate $gate, AiCategorizer $categorizer): void @@ -68,7 +68,7 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue */ public function failed(?Throwable $exception): void { - $progress = Cache::get(self::cacheKeyForJobId($this->jobId), [ + $progress = Cache::get(self::cacheKeyForJobId($this->user->id, $this->jobId), [ 'processed' => 0, 'total' => 0, 'applied' => 0, @@ -87,7 +87,7 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue */ private function updateProgress(string $status, int $processed, int $total, int $applied): void { - Cache::put(self::cacheKeyForJobId($this->jobId), [ + Cache::put(self::cacheKeyForJobId($this->user->id, $this->jobId), [ 'status' => $status, 'processed' => $processed, 'total' => $total, diff --git a/app/Jobs/ReEvaluateTransactionRulesJob.php b/app/Jobs/ReEvaluateTransactionRulesJob.php index 5c552b48..f16467ec 100644 --- a/app/Jobs/ReEvaluateTransactionRulesJob.php +++ b/app/Jobs/ReEvaluateTransactionRulesJob.php @@ -83,14 +83,14 @@ class ReEvaluateTransactionRulesJob implements ShouldQueue ); } - public static function cacheKeyForJobId(string $jobId): string + public static function cacheKeyForJobId(string $userId, string $jobId): string { - return "re_evaluate_rules_job_{$jobId}"; + return "re_evaluate_rules_job_{$userId}_{$jobId}"; } private function cacheKey(): string { - return self::cacheKeyForJobId($this->jobId); + return self::cacheKeyForJobId($this->user->id, $this->jobId); } /** diff --git a/app/Services/AutomationRuleService.php b/app/Services/AutomationRuleService.php index c25ba330..d7a25785 100644 --- a/app/Services/AutomationRuleService.php +++ b/app/Services/AutomationRuleService.php @@ -13,17 +13,25 @@ use JWadhams\JsonLogic; class AutomationRuleService { + /** + * Per-user rule cache, memoized for the lifetime of this service instance. + * + * Bulk re-evaluation calls applyRules() once per transaction; without this + * every transaction re-queried the user's full rule set (an N+1). A service + * instance is short-lived (resolved fresh per job/request), so rules created + * after it is built are never seen mid-run — which is the intended behaviour. + * + * @var array> + */ + private array $rulesByUser = []; + public function applyRules(Transaction $transaction): void { if ($transaction->description_iv !== null) { return; } - $rules = AutomationRule::query() - ->where('user_id', $transaction->user_id) - ->with('labels') - ->orderBy('priority') - ->get(); + $rules = $this->rulesForUser($transaction->user_id); if ($rules->isEmpty()) { return; @@ -61,9 +69,17 @@ class AutomationRuleService } /** + * Apply a rule's actions to a set of transactions. + * + * When $onlyUncategorized is true the set is re-filtered here, at apply + * time, rather than trusting the caller's (possibly stale) match snapshot. + * A transaction that was categorized after the snapshot was taken — by the + * user, another rule, or the AI backfill — is skipped so the rule never + * silently overwrites a category the user did not ask it to touch. + * * @param EloquentCollection $transactions */ - public function applyRuleActionsToTransactions(EloquentCollection $transactions, AutomationRule $rule): int + public function applyRuleActionsToTransactions(EloquentCollection $transactions, AutomationRule $rule, bool $onlyUncategorized = false): int { if ($transactions->isEmpty()) { return 0; @@ -72,6 +88,16 @@ class AutomationRuleService $rule->loadMissing('labels'); $transactions->loadMissing('labels'); + if ($onlyUncategorized) { + $transactions = $transactions->reject( + fn (Transaction $transaction): bool => $this->shouldSkipForOnlyUncategorized($rule, $transaction), + ); + + if ($transactions->isEmpty()) { + return 0; + } + } + $changedTransactionIds = []; if ($rule->action_category_id !== null) { @@ -221,6 +247,18 @@ class AutomationRuleService ]; } + /** + * @return EloquentCollection + */ + private function rulesForUser(string $userId): EloquentCollection + { + return $this->rulesByUser[$userId] ??= AutomationRule::query() + ->where('user_id', $userId) + ->with('labels') + ->orderBy('priority') + ->get(); + } + /** * @param Collection $rules * @param array $transactionData diff --git a/resources/js/components/automation-rules/apply-automation-rule-flow.tsx b/resources/js/components/automation-rules/apply-automation-rule-flow.tsx index bd5c672c..d1b5da26 100644 --- a/resources/js/components/automation-rules/apply-automation-rule-flow.tsx +++ b/resources/js/components/automation-rules/apply-automation-rule-flow.tsx @@ -178,6 +178,7 @@ export function ApplyAutomationRuleFlow({ const pollStatus = useCallback( async (jobId: string) => { const url = statusRoute(jobId).url; + let pendingTicks = 0; const poll = async (): Promise => { const res = await fetch(url, { headers: { Accept: 'application/json' }, @@ -202,6 +203,13 @@ export function ApplyAutomationRuleFlow({ setApplying(false); return; } + // The job never started (e.g. no queue worker running) — give + // up instead of polling forever. + if (data.status === 'pending' && ++pendingTicks > 30) { + toast.error(__('Failed to apply rule to transactions.')); + setApplying(false); + return; + } setTimeout(() => { void poll(); }, 1000); diff --git a/resources/js/hooks/use-re-evaluate-all-transactions.tsx b/resources/js/hooks/use-re-evaluate-all-transactions.tsx index b0bae253..f6762280 100644 --- a/resources/js/hooks/use-re-evaluate-all-transactions.tsx +++ b/resources/js/hooks/use-re-evaluate-all-transactions.tsx @@ -18,6 +18,7 @@ export function useReEvaluateAllTransactions() { const jobId = bulkResponse.data.job_id; await new Promise((resolve, reject) => { + let pendingTicks = 0; const poll = async () => { try { const statusResponse = await axios.get<{ @@ -47,6 +48,13 @@ export function useReEvaluateAllTransactions() { resolve(); } else if (status === 'failed') { reject(new Error('Job failed')); + } else if ( + status === 'pending' && + ++pendingTicks > 30 + ) { + // The job never started (e.g. no queue worker + // running) — give up instead of polling forever. + reject(new Error('Job did not start')); } else { setTimeout(poll, 1000); } diff --git a/resources/js/pages/transactions/index.tsx b/resources/js/pages/transactions/index.tsx index 9fa09fdd..2fba0680 100644 --- a/resources/js/pages/transactions/index.tsx +++ b/resources/js/pages/transactions/index.tsx @@ -940,6 +940,7 @@ export default function Transactions({ const jobId = bulkResponse.data.job_id; await new Promise((resolve, reject) => { + let pendingTicks = 0; const poll = async () => { try { const statusResponse = await axios.get<{ @@ -969,6 +970,13 @@ export default function Transactions({ resolve(); } else if (status === 'failed') { reject(new Error('Job failed')); + } else if ( + status === 'pending' && + ++pendingTicks > 30 + ) { + // The job never started (e.g. no queue worker + // running) — give up instead of polling forever. + reject(new Error('Job did not start')); } else { setTimeout(poll, 1000); } @@ -1472,7 +1480,7 @@ export default function Transactions({ handleDismissAiConsent } disabled={aiConsentSaving} - className="opacity-20 hover:opacity-100 transition-all duration-300" + className="opacity-20 transition-all duration-300 hover:opacity-100" variant="ghost" size="icon" aria-label={__('Dismiss')} diff --git a/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php b/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php index cb6dbbe3..854025e6 100644 --- a/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php +++ b/tests/Feature/Ai/CategorizeUncategorizedTransactionsTest.php @@ -85,7 +85,7 @@ it('does not dispatch a backfill when AI categorization is disabled', function ( it('returns categorization progress from the status endpoint', function () { $user = User::factory()->create(); Cache::put( - CategorizeUncategorizedTransactionsJob::cacheKeyForJobId('job-123'), + CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, 'job-123'), ['status' => 'processing', 'processed' => 1, 'total' => 4, 'applied' => 1], now()->addHour(), ); @@ -102,6 +102,20 @@ it('returns 404 from the status endpoint for an unknown job', function () { ->assertNotFound(); }); +it('does not leak another user\'s categorization progress', function () { + $owner = User::factory()->create(); + Cache::put( + CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($owner->id, 'job-123'), + ['status' => 'processing', 'processed' => 1, 'total' => 4, 'applied' => 1], + now()->addHour(), + ); + + $otherUser = User::factory()->create(); + + actingAs($otherUser)->getJson(route('ai.categorization.status', 'job-123')) + ->assertNotFound(); +}); + it('records progress while categorizing the uncategorized transactions', function () { $user = User::factory()->create(); $user->recordAiConsent(); @@ -139,7 +153,7 @@ it('records progress while categorizing the uncategorized transactions', functio $jobId = 'job-run-1'; app()->call([new CategorizeUncategorizedTransactionsJob($user, $jobId), 'handle']); - $progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId)); expect($progress['status'])->toBe('done') ->and($progress['total'])->toBe(2) @@ -151,7 +165,7 @@ it('marks the cache as failed and preserves counts when the job fails', function $user = User::factory()->create(); $jobId = 'failed-job'; Cache::put( - CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId), + CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId), ['status' => 'processing', 'processed' => 3, 'total' => 10, 'applied' => 2], now()->addHour(), ); @@ -159,7 +173,7 @@ it('marks the cache as failed and preserves counts when the job fails', function (new CategorizeUncategorizedTransactionsJob($user, $jobId)) ->failed(new RuntimeException('boom')); - $progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId)); expect($progress['status'])->toBe('failed') ->and($progress['processed'])->toBe(3) diff --git a/tests/Feature/AutomationRuleApplicationTest.php b/tests/Feature/AutomationRuleApplicationTest.php index 47c62d94..65fd5f5c 100644 --- a/tests/Feature/AutomationRuleApplicationTest.php +++ b/tests/Feature/AutomationRuleApplicationTest.php @@ -10,6 +10,7 @@ use App\Models\Category; use App\Models\Label; use App\Models\Transaction; use App\Models\User; +use App\Services\AutomationRuleService; use Illuminate\Database\Eloquent\Factories\Sequence; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; @@ -289,6 +290,56 @@ test('apply endpoint queues a job when matches exceed threshold', function () { Queue::assertPushed(ApplySingleAutomationRuleJob::class); }); +test('apply job applies the rule and records done progress', function () { + $t1 = Transaction::factory()->enableBanking()->create([ + 'user_id' => $this->user->id, + 'account_id' => $this->account->id, + 'description' => 'Grocery Store', + 'amount' => -5000, + 'category_id' => null, + ]); + $t2 = Transaction::factory()->enableBanking()->create([ + 'user_id' => $this->user->id, + 'account_id' => $this->account->id, + 'description' => 'Grocery Store', + 'amount' => -3000, + 'category_id' => null, + ]); + + $jobId = 'apply-run-1'; + (new ApplySingleAutomationRuleJob($this->rule, $jobId, [$t1->id, $t2->id])) + ->handle(app(AutomationRuleService::class)); + + expect($t1->fresh()->category_id)->toBe($this->category->id) + ->and($t2->fresh()->category_id)->toBe($this->category->id); + + $progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId)); + expect($progress['status'])->toBe('done') + ->and($progress['total'])->toBe(2) + ->and($progress['processed'])->toBe(2) + ->and($progress['applied'])->toBe(2) + ->and($progress['updated'])->toBe(2); +}); + +test('apply job marks cache as failed and preserves counts', function () { + $jobId = 'apply-failed'; + Cache::put( + ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId), + ['status' => 'processing', 'processed' => 3, 'total' => 10, 'applied' => 3, 'updated' => 2], + now()->addHour(), + ); + + (new ApplySingleAutomationRuleJob($this->rule, $jobId, ['x', 'y'])) + ->failed(new RuntimeException('boom')); + + $progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId)); + expect($progress['status'])->toBe('failed') + ->and($progress['processed'])->toBe(3) + ->and($progress['total'])->toBe(10) + ->and($progress['applied'])->toBe(3) + ->and($progress['updated'])->toBe(2); +}); + test('apply endpoint returns done with zero matches when no transactions match', function () { Transaction::factory()->enableBanking()->create([ 'user_id' => $this->user->id, @@ -318,6 +369,59 @@ test('cannot apply rule belonging to another user', function () { ->assertForbidden(); }); +test('cannot poll apply job status belonging to another user', function () { + $jobId = 'apply-job-1'; + Cache::put( + ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId), + ['status' => 'processing', 'processed' => 1, 'total' => 4], + now()->addHour(), + ); + + $this->actingAs($this->user) + ->getJson(route('automation-rules.apply.status', $jobId)) + ->assertOk() + ->assertJsonPath('status', 'processing'); + + $otherUser = User::factory()->onboarded()->create(); + + $this->actingAs($otherUser) + ->getJson(route('automation-rules.apply.status', $jobId)) + ->assertNotFound(); +}); + +test('apply re-checks only_uncategorized at apply time and skips newly categorized transactions', function () { + $otherCategory = Category::factory()->create(['user_id' => $this->user->id]); + + // Both matched the rule when the snapshot was taken, but this one has since + // been categorized (by the user, another rule, or the AI backfill). + $nowCategorized = Transaction::factory()->enableBanking()->create([ + 'user_id' => $this->user->id, + 'account_id' => $this->account->id, + 'description' => 'Grocery Store', + 'amount' => -5000, + 'category_id' => $otherCategory->id, + ]); + + $stillUncategorized = Transaction::factory()->enableBanking()->create([ + 'user_id' => $this->user->id, + 'account_id' => $this->account->id, + 'description' => 'Grocery Store', + 'amount' => -3000, + 'category_id' => null, + ]); + + $transactions = Transaction::query() + ->whereIn('id', [$nowCategorized->id, $stillUncategorized->id]) + ->get(); + + $changed = app(AutomationRuleService::class) + ->applyRuleActionsToTransactions($transactions, $this->rule, onlyUncategorized: true); + + expect($changed)->toBe(1); + expect($nowCategorized->fresh()->category_id)->toBe($otherCategory->id); + expect($stillUncategorized->fresh()->category_id)->toBe($this->category->id); +}); + test('label-only rule applies when only_uncategorized is true', function () { $labelOnlyRule = AutomationRule::factory()->create([ 'user_id' => $this->user->id, diff --git a/tests/Feature/ReEvaluateTransactionRulesTest.php b/tests/Feature/ReEvaluateTransactionRulesTest.php index 35fc73cb..c4144a7f 100644 --- a/tests/Feature/ReEvaluateTransactionRulesTest.php +++ b/tests/Feature/ReEvaluateTransactionRulesTest.php @@ -10,6 +10,7 @@ use App\Models\Transaction; use App\Models\User; use App\Services\AutomationRuleService; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Queue; beforeEach(function () { @@ -213,7 +214,7 @@ test('bulk endpoint sets initial pending status in cache', function () { ->postJson(route('transactions.re-evaluate-rules.bulk')); $jobId = $response->json('job_id'); - $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId); + $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId); expect(Cache::get($cacheKey))->toMatchArray(['status' => 'pending']); }); @@ -224,7 +225,7 @@ test('bulk endpoint sets initial pending status in cache', function () { test('status endpoint returns progress from cache', function () { $jobId = 'test-job-id'; - $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId); + $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId); Cache::put($cacheKey, [ 'status' => 'processing', @@ -250,6 +251,19 @@ test('status endpoint returns 404 for unknown job', function () { ->assertNotFound(); }); +test('status endpoint does not leak another user\'s job progress', function () { + $jobId = 'owned-by-someone-else'; + $cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId); + + Cache::put($cacheKey, ['status' => 'processing', 'processed' => 5, 'total' => 50, 'updated' => 1], now()->addHour()); + + $otherUser = User::factory()->onboarded()->create(); + + $this->actingAs($otherUser) + ->getJson(route('transactions.re-evaluate-rules.status', $jobId)) + ->assertNotFound(); +}); + // ────────────────────────────────────────────── // Job execution // ────────────────────────────────────────────── @@ -287,12 +301,60 @@ test('job applies rules to non-encrypted transactions and tracks progress', func expect($matchingTransaction->fresh()->category_id)->toBe($this->category->id); expect($nonMatchingTransaction->fresh()->category_id)->toBeNull(); - $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId)); expect($progress['status'])->toBe('done'); expect($progress['processed'])->toBe(2); expect($progress['updated'])->toBe(1); }); +test('job queries automation rules once regardless of transaction count', function () { + // Create transactions BEFORE the rule so the creation listener has no rules to apply + Transaction::factory()->enableBanking()->count(5)->create([ + 'user_id' => $this->user->id, + 'account_id' => $this->account->id, + 'description' => 'Grocery Store', + 'amount' => -5000, + 'category_id' => null, + ]); + + AutomationRule::factory()->create([ + 'user_id' => $this->user->id, + 'priority' => 1, + 'rules_json' => ['in' => ['grocery', ['var' => 'description']]], + 'action_category_id' => $this->category->id, + ]); + + $ruleQueries = 0; + DB::listen(function ($query) use (&$ruleQueries): void { + if (str_contains($query->sql, 'automation_rules')) { + $ruleQueries++; + } + }); + + $jobId = 'test-job-'.uniqid(); + (new ReEvaluateTransactionRulesJob($this->user, $jobId))->handle(app(AutomationRuleService::class)); + + expect($ruleQueries)->toBe(1); +}); + +test('job marks cache as failed and preserves counts', function () { + $jobId = 'reeval-failed'; + Cache::put( + ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId), + ['status' => 'processing', 'processed' => 4, 'total' => 20, 'updated' => 2], + now()->addHour(), + ); + + (new ReEvaluateTransactionRulesJob($this->user, $jobId)) + ->failed(new RuntimeException('boom')); + + $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId)); + expect($progress['status'])->toBe('failed') + ->and($progress['processed'])->toBe(4) + ->and($progress['total'])->toBe(20) + ->and($progress['updated'])->toBe(2); +}); + test('job skips encrypted transactions', function () { AutomationRule::factory()->create([ 'user_id' => $this->user->id, @@ -314,7 +376,7 @@ test('job skips encrypted transactions', function () { $job = new ReEvaluateTransactionRulesJob($this->user, $jobId); $job->handle(app(AutomationRuleService::class)); - $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId)); // 0 processed because the encrypted transaction is excluded from the query expect($progress['processed'])->toBe(0); expect($progress['updated'])->toBe(0); @@ -352,7 +414,7 @@ test('job only processes provided transaction_ids', function () { expect($t1->fresh()->category_id)->toBe($this->category->id); expect($t2->fresh()->category_id)->toBeNull(); - $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId)); expect($progress['processed'])->toBe(1); }); @@ -412,7 +474,7 @@ test('job applies rules to transactions matching filters', function () { expect($matchingTransaction->fresh()->category_id)->toBe($this->category->id); expect($outsideRangeTransaction->fresh()->category_id)->toBeNull(); - $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId)); + $progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId)); expect($progress['status'])->toBe('done'); expect($progress['processed'])->toBe(1); expect($progress['updated'])->toBe(1);