From 73de53994fafb7eeae09239488ce7ee953d2a88e Mon Sep 17 00:00:00 2001 From: Ovidiu U Date: Fri, 1 May 2026 13:22:36 +0100 Subject: [PATCH] fix: prevent sensitive field leaks in /me, add retry logic to Brent price sources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Made `/api/auth/me` public and return explicit allowlist (name, email, two_factor_confirmed_at, tier, subscription fields) instead of spreading `$user->toArray()` which leaked is_admin, stripe_id, pm_type, pm_last_four, postcode. Returns `null` when unauthenticated rather than 401. - Moved `/auth/logout` to remain behind auth:sanctum gate. - Added 3×200ms retry with exponential backoff to EiaBrentPriceSource and FredBrentPriceSource on ConnectionException or 5xx responses. Timeout raised from 10s to 30s. - Both sources now throw typed BrentPriceFetchException on exhausted retries instead of silently returning null + logging. Updated tests to assert exception message includes HTTP status or "connection failed". --- app/Http/Controllers/Api/AuthController.php | 25 ++++---- .../BrentPriceSources/EiaBrentPriceSource.php | 57 +++++++++---------- .../FredBrentPriceSource.php | 57 +++++++++---------- routes/api.php | 2 +- tests/Feature/Api/AuthControllerTest.php | 45 ++++++++++++++- tests/Unit/Services/BrentPriceFetcherTest.php | 34 +++++++++-- 6 files changed, 145 insertions(+), 75 deletions(-) diff --git a/app/Http/Controllers/Api/AuthController.php b/app/Http/Controllers/Api/AuthController.php index 5a1827e..62eddaa 100644 --- a/app/Http/Controllers/Api/AuthController.php +++ b/app/Http/Controllers/Api/AuthController.php @@ -64,19 +64,24 @@ class AuthController extends Controller public function me(Request $request): JsonResponse { $user = $request->user(); + + if ($user === null) { + return new JsonResponse('null', json: true); + } + $subscription = $user->subscription(); $expiresAt = $subscription?->ends_at ?? $subscription?->current_period_end; - return response()->json(array_merge( - $user->toArray(), - [ - 'tier' => PlanFeatures::for($user)->tier(), - 'subscription_cancelled' => $subscription?->canceled() ?? false, - 'subscription_cadence' => Plan::resolveCadenceForUser($user), - 'subscribed_at' => $subscription?->created_at?->toIso8601String(), - 'subscription_expires_at' => $expiresAt?->toIso8601String(), - ], - )); + return response()->json([ + 'name' => $user->name, + 'email' => $user->email, + 'two_factor_confirmed_at' => $user->two_factor_confirmed_at?->toIso8601String(), + 'tier' => PlanFeatures::for($user)->tier(), + 'subscription_cancelled' => $subscription?->canceled() ?? false, + 'subscription_cadence' => Plan::resolveCadenceForUser($user), + 'subscribed_at' => $subscription?->created_at?->toIso8601String(), + 'subscription_expires_at' => $expiresAt?->toIso8601String(), + ]); } } diff --git a/app/Services/BrentPriceSources/EiaBrentPriceSource.php b/app/Services/BrentPriceSources/EiaBrentPriceSource.php index c89e5d8..1cb79ee 100644 --- a/app/Services/BrentPriceSources/EiaBrentPriceSource.php +++ b/app/Services/BrentPriceSources/EiaBrentPriceSource.php @@ -3,8 +3,9 @@ namespace App\Services\BrentPriceSources; use App\Services\ApiLogger; +use Illuminate\Http\Client\ConnectionException; +use Illuminate\Http\Client\RequestException; use Illuminate\Support\Facades\Http; -use Illuminate\Support\Facades\Log; use Throwable; final class EiaBrentPriceSource @@ -14,12 +15,16 @@ final class EiaBrentPriceSource public function __construct(private readonly ApiLogger $apiLogger) {} /** - * @return array{date: string, price_usd: float}[]|null + * @return array{date: string, price_usd: float}[]|null null only when the response carried no usable rows + * + * @throws BrentPriceFetchException on network failure or non-2xx response after retries */ public function fetch(): ?array { try { - $response = $this->apiLogger->send('eia', 'GET', self::URL, fn () => Http::timeout(10) + $response = $this->apiLogger->send('eia', 'GET', self::URL, fn () => Http::timeout(30) + ->retry(3, 200, fn (Throwable $e) => $this->shouldRetry($e)) + ->throw() ->get(self::URL, [ 'api_key' => config('services.eia.api_key'), 'frequency' => 'daily', @@ -29,32 +34,26 @@ final class EiaBrentPriceSource 'sort[0][direction]' => 'desc', 'length' => 30, ])); - - if (! $response->successful()) { - Log::error('EiaBrentPriceSource: request failed', ['status' => $response->status()]); - - return null; - } - - $rows = collect($response->json('response.data') ?? []) - ->filter(fn (array $row) => ($row['value'] ?? '.') !== '.') - ->map(fn (array $row) => [ - 'date' => $row['period'], - 'price_usd' => (float) $row['value'], - ]) - ->all(); - - if ($rows === []) { - Log::warning('EiaBrentPriceSource: no valid observations returned'); - - return null; - } - - return $rows; - } catch (Throwable $e) { - Log::error('EiaBrentPriceSource: fetch failed', ['error' => $e->getMessage()]); - - return null; + } catch (ConnectionException $e) { + throw new BrentPriceFetchException("EIA connection failed: {$e->getMessage()}", previous: $e); + } catch (RequestException $e) { + throw new BrentPriceFetchException("EIA returned HTTP {$e->response->status()}", previous: $e); } + + $rows = collect($response->json('response.data') ?? []) + ->filter(fn (array $row) => ($row['value'] ?? '.') !== '.') + ->map(fn (array $row) => [ + 'date' => $row['period'], + 'price_usd' => (float) $row['value'], + ]) + ->all(); + + return $rows === [] ? null : $rows; + } + + private function shouldRetry(Throwable $e): bool + { + return $e instanceof ConnectionException + || ($e instanceof RequestException && $e->response->serverError()); } } diff --git a/app/Services/BrentPriceSources/FredBrentPriceSource.php b/app/Services/BrentPriceSources/FredBrentPriceSource.php index 8263d4e..779b38e 100644 --- a/app/Services/BrentPriceSources/FredBrentPriceSource.php +++ b/app/Services/BrentPriceSources/FredBrentPriceSource.php @@ -3,8 +3,9 @@ namespace App\Services\BrentPriceSources; use App\Services\ApiLogger; +use Illuminate\Http\Client\ConnectionException; +use Illuminate\Http\Client\RequestException; use Illuminate\Support\Facades\Http; -use Illuminate\Support\Facades\Log; use Throwable; final class FredBrentPriceSource @@ -14,12 +15,16 @@ final class FredBrentPriceSource public function __construct(private readonly ApiLogger $apiLogger) {} /** - * @return array{date: string, price_usd: float}[]|null + * @return array{date: string, price_usd: float}[]|null null only when the response carried no usable rows + * + * @throws BrentPriceFetchException on network failure or non-2xx response after retries */ public function fetch(): ?array { try { - $response = $this->apiLogger->send('fred', 'GET', self::URL, fn () => Http::timeout(10) + $response = $this->apiLogger->send('fred', 'GET', self::URL, fn () => Http::timeout(30) + ->retry(3, 200, fn (Throwable $e) => $this->shouldRetry($e)) + ->throw() ->get(self::URL, [ 'series_id' => 'DCOILBRENTEU', 'api_key' => config('services.fred.api_key'), @@ -27,32 +32,26 @@ final class FredBrentPriceSource 'limit' => 30, 'file_type' => 'json', ])); - - if (! $response->successful()) { - Log::error('FredBrentPriceSource: request failed', ['status' => $response->status()]); - - return null; - } - - $rows = collect($response->json('observations') ?? []) - ->filter(fn (array $obs) => $obs['value'] !== '.') - ->map(fn (array $obs) => [ - 'date' => $obs['date'], - 'price_usd' => (float) $obs['value'], - ]) - ->all(); - - if ($rows === []) { - Log::warning('FredBrentPriceSource: no valid observations returned'); - - return null; - } - - return $rows; - } catch (Throwable $e) { - Log::error('FredBrentPriceSource: fetch failed', ['error' => $e->getMessage()]); - - return null; + } catch (ConnectionException $e) { + throw new BrentPriceFetchException("FRED connection failed: {$e->getMessage()}", previous: $e); + } catch (RequestException $e) { + throw new BrentPriceFetchException("FRED returned HTTP {$e->response->status()}", previous: $e); } + + $rows = collect($response->json('observations') ?? []) + ->filter(fn (array $obs) => $obs['value'] !== '.') + ->map(fn (array $obs) => [ + 'date' => $obs['date'], + 'price_usd' => (float) $obs['value'], + ]) + ->all(); + + return $rows === [] ? null : $rows; + } + + private function shouldRetry(Throwable $e): bool + { + return $e instanceof ConnectionException + || ($e instanceof RequestException && $e->response->serverError()); } } diff --git a/routes/api.php b/routes/api.php index ecfaa33..8f2d629 100644 --- a/routes/api.php +++ b/routes/api.php @@ -12,6 +12,7 @@ use Illuminate\Support\Facades\Route; // Public endpoints (no API key required) Route::post('/auth/register', [AuthController::class, 'register']); Route::post('/auth/login', [AuthController::class, 'login']); +Route::get('/auth/me', [AuthController::class, 'me']); Route::get('/fuel-types', function () { return Cache::remember('api:fuel-types', now()->addDay(), fn () => collect(FuelType::cases()) @@ -29,7 +30,6 @@ Route::middleware(['throttle:60,1', VerifyApiKey::class])->group(function (): vo // Sanctum-authenticated endpoints Route::middleware('auth:sanctum')->group(function (): void { - Route::get('/auth/me', [AuthController::class, 'me']); Route::post('/auth/logout', [AuthController::class, 'logout']); // User dashboard endpoints diff --git a/tests/Feature/Api/AuthControllerTest.php b/tests/Feature/Api/AuthControllerTest.php index 86a49d6..a528fa7 100644 --- a/tests/Feature/Api/AuthControllerTest.php +++ b/tests/Feature/Api/AuthControllerTest.php @@ -69,6 +69,41 @@ it('returns the authenticated user on /me', function () { ->assertJsonPath('email', $user->email); }); +it('does not leak sensitive or internal user fields on /me', function () { + $user = User::factory()->create([ + 'is_admin' => true, + 'stripe_id' => 'cus_secret', + 'pm_type' => 'visa', + 'pm_last_four' => '4242', + 'postcode' => 'SW1A 1AA', + ]); + + $user->subscriptions()->create([ + 'type' => 'default', + 'stripe_id' => 'sub_secret', + 'stripe_status' => 'active', + 'stripe_price' => 'price_plus_monthly', + 'quantity' => 1, + ]); + + $response = $this->actingAs($user, 'sanctum') + ->getJson('/api/auth/me') + ->assertOk(); + + $payload = $response->json(); + + expect(array_keys($payload))->toEqualCanonicalizing([ + 'name', + 'email', + 'two_factor_confirmed_at', + 'tier', + 'subscription_cancelled', + 'subscription_cadence', + 'subscribed_at', + 'subscription_expires_at', + ]); +}); + it('reports subscription_cancelled=false for a user with no subscription', function () { $user = User::factory()->create(); @@ -215,6 +250,12 @@ it('logs out and revokes the token', function () { expect($user->tokens()->count())->toBe(0); }); -it('returns 401 on protected routes without a token', function () { - $this->getJson('/api/auth/me')->assertUnauthorized(); +it('returns null on /me when unauthenticated', function () { + $response = $this->getJson('/api/auth/me')->assertOk(); + + expect($response->getContent())->toBe('null'); +}); + +it('returns 401 on protected routes without a token', function () { + $this->postJson('/api/auth/logout')->assertUnauthorized(); }); diff --git a/tests/Unit/Services/BrentPriceFetcherTest.php b/tests/Unit/Services/BrentPriceFetcherTest.php index b88e78a..fe720a5 100644 --- a/tests/Unit/Services/BrentPriceFetcherTest.php +++ b/tests/Unit/Services/BrentPriceFetcherTest.php @@ -38,11 +38,24 @@ it('fetches and stores brent prices from EIA', function (): void { ->and(BrentPrice::find('2026-04-02')->price_usd)->toBe('73.80'); }); -it('throws when EIA returns a 500', function (): void { +it('throws with HTTP status when EIA returns a 500', function (): void { Http::fake(['*eia.gov/*' => Http::response([], 500)]); + expect(fn () => $this->fetcher->fetchFromEia()) + ->toThrow(BrentPriceFetchException::class, 'EIA returned HTTP 500'); +}); + +it('retries EIA on transient 500 and succeeds', function (): void { + Http::fake([ + '*eia.gov/*' => Http::sequence() + ->push([], 500) + ->push(['response' => ['data' => [['period' => '2026-04-01', 'value' => '75.10']]]]), + ]); + $this->fetcher->fetchFromEia(); -})->throws(BrentPriceFetchException::class); + + expect(BrentPrice::count())->toBe(1); +}); it('throws when EIA returns empty data', function (): void { Http::fake(['*eia.gov/*' => Http::response(['response' => ['data' => []]])]); @@ -84,11 +97,24 @@ it('fetches and stores brent prices from FRED', function (): void { expect(BrentPrice::count())->toBe(2); }); -it('throws when FRED fails', function (): void { +it('throws with HTTP status when FRED returns a 500', function (): void { Http::fake(['*/fred/series/observations*' => Http::response([], 500)]); + expect(fn () => $this->fetcher->fetchFromFred()) + ->toThrow(BrentPriceFetchException::class, 'FRED returned HTTP 500'); +}); + +it('retries FRED on transient 500 and succeeds', function (): void { + Http::fake([ + '*/fred/series/observations*' => Http::sequence() + ->push([], 500) + ->push(['observations' => [['date' => '2026-04-01', 'value' => '75.10']]]), + ]); + $this->fetcher->fetchFromFred(); -})->throws(BrentPriceFetchException::class); + + expect(BrentPrice::count())->toBe(1); +}); it('filters out FRED missing value markers', function (): void { Http::fake([