From 93e2d6823c459dfd353610c45855b7c661466aba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vi=CC=81ctor=20Falco=CC=81n?= Date: Fri, 3 Jul 2026 13:09:18 +0200 Subject: [PATCH] fix(security): scope job-status cache keys to the owning user The categorization, bulk rule re-evaluation, and automation-rule apply status endpoints looked jobs up by a bare job UUID. Any authenticated user who obtained another user's job id could poll its progress payload. Include the owning user's id in the cache key so a status request keyed by the polling user's id can only resolve that user's own jobs; a mismatched owner now falls through to the existing 404. No ownership store or extra lookup needed. Added cross-user isolation tests for all three endpoints. --- .../Ai/StartCategorizationBackfill.php | 2 +- .../Ai/CategorizationController.php | 5 ++-- .../ReEvaluateTransactionRulesController.php | 4 +-- .../AutomationRuleApplicationController.php | 4 +-- app/Jobs/ApplySingleAutomationRuleJob.php | 6 ++--- ...CategorizeUncategorizedTransactionsJob.php | 8 +++--- app/Jobs/ReEvaluateTransactionRulesJob.php | 6 ++--- ...ategorizeUncategorizedTransactionsTest.php | 22 +++++++++++++--- .../Feature/AutomationRuleApplicationTest.php | 20 +++++++++++++++ .../ReEvaluateTransactionRulesTest.php | 25 ++++++++++++++----- 10 files changed, 75 insertions(+), 27 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..b1052977 100644 --- a/app/Http/Controllers/Settings/AutomationRuleApplicationController.php +++ b/app/Http/Controllers/Settings/AutomationRuleApplicationController.php @@ -121,7 +121,7 @@ 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(), ); @@ -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..9e3a5dea 100644 --- a/app/Jobs/ApplySingleAutomationRuleJob.php +++ b/app/Jobs/ApplySingleAutomationRuleJob.php @@ -69,14 +69,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/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..fadfa981 100644 --- a/tests/Feature/AutomationRuleApplicationTest.php +++ b/tests/Feature/AutomationRuleApplicationTest.php @@ -318,6 +318,26 @@ 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('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..103894d6 100644 --- a/tests/Feature/ReEvaluateTransactionRulesTest.php +++ b/tests/Feature/ReEvaluateTransactionRulesTest.php @@ -213,7 +213,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 +224,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 +250,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,7 +300,7 @@ 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); @@ -314,7 +327,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 +365,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 +425,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);