diff --git a/backend/app/Services/Auth/OptionalBearerUserResolver.php b/backend/app/Services/Auth/OptionalBearerUserResolver.php index be46b2f..e33c3ed 100644 --- a/backend/app/Services/Auth/OptionalBearerUserResolver.php +++ b/backend/app/Services/Auth/OptionalBearerUserResolver.php @@ -13,11 +13,12 @@ * Behavior (F-013 characterization, not to be changed without a decision * recorded in docs/DECISIONS.md): * - A user already authenticated by the guard is returned as-is. - * - A resolvable bearer token returns its owner. - * - A bearer token that fails to resolve returns an empty, unpersisted - * `User` instance instead of `null`, so an authenticated-looking but - * invalid request is never widened to the anonymous public scope - * (see ADR-23). + * - A resolvable, non-expired bearer token returns its owner. + * - A bearer token that fails to resolve, or that resolves but has an + * `expires_at` in the past, returns an empty, unpersisted `User` + * instance instead of `null`, so an authenticated-looking but invalid + * or lapsed request is never widened to the anonymous public scope + * (see ADR-23, F-015). A token without `expires_at` never lapses here. * - No bearer token at all (including a header that is not a `Bearer` * scheme, or an empty `Bearer` value) returns `null`, the anonymous * public scope. @@ -40,6 +41,12 @@ public function resolve(Request $request): ?User } $accessToken = PersonalAccessToken::findToken($token); + + if ($accessToken?->expires_at?->isPast()) { + // Do not widen an authenticated-looking, lapsed request to the anonymous public scope. + return new User; + } + $tokenable = $accessToken?->tokenable; if ($tokenable instanceof User) { diff --git a/backend/tests/Feature/OptionalBearerUserResolutionTest.php b/backend/tests/Feature/OptionalBearerUserResolutionTest.php index f6cf1c5..e76a116 100644 --- a/backend/tests/Feature/OptionalBearerUserResolutionTest.php +++ b/backend/tests/Feature/OptionalBearerUserResolutionTest.php @@ -9,7 +9,6 @@ use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Artisan; -use Laravel\Sanctum\PersonalAccessToken; use Meilisearch\Client; use Meilisearch\Exceptions\ApiException; @@ -392,22 +391,77 @@ function bearerCases(array $graph): array } }); -it('accepts an expired bearer token as its owner on public discovery endpoints (pre-existing gap, not fixed here)', function (): void { +it('rejects an expired bearer token, falling back to empty scope on public discovery endpoints (F-015)', function (): void { $graph = bearerResolutionGraph(); - // findToken() from laravel/sanctum only hashes and looks up the token - // row; it does not check `expires_at`. That check lives in - // Laravel\Sanctum\Guard, which only runs behind the `auth:sanctum` - // middleware. CatalogController/SearchController::currentUser() call - // PersonalAccessToken::findToken() directly, bypassing that guard, so - // an expired token is currently still resolved to its owner here. + // OptionalBearerUserResolver now checks expires_at itself: findToken() + // from laravel/sanctum only hashes and looks up the token row, and the + // guard-level expiry check in Laravel\Sanctum\Guard never runs here + // because CatalogController/SearchController::currentUser() resolve + // outside the auth:sanctum middleware. An expired token is treated like + // any other bearer that fails to resolve: empty scope, never the + // token owner's scope and never the anonymous public scope. $expiredToken = $graph['member']->createToken('expired', ['*'], now()->subDay())->plainTextToken; $this->withHeaders(['Authorization' => 'Bearer '.$expiredToken]) ->getJson('/api/v1/plugins/'.$graph['plugin']->slug) - ->assertOk(); + ->assertNotFound(); $this->withHeaders(['Authorization' => 'Bearer '.$expiredToken]) ->getJson('/api/v1/plugins/'.$graph['foreignPlugin']->slug) ->assertNotFound(); }); + +it('rejects a bearer token that expired only seconds ago', function (): void { + $graph = bearerResolutionGraph(); + + $justExpiredToken = $graph['member']->createToken('just-expired', ['*'], now()->subSeconds(5))->plainTextToken; + + $this->withHeaders(['Authorization' => 'Bearer '.$justExpiredToken]) + ->getJson('/api/v1/plugins/'.$graph['plugin']->slug) + ->assertNotFound(); + + $this->withHeaders(['Authorization' => 'Bearer '.$justExpiredToken]) + ->getJson('/api/v1/plugins/'.$graph['foreignPlugin']->slug) + ->assertNotFound(); +}); + +it('still accepts a bearer token with no expires_at as its owner', function (): void { + $graph = bearerResolutionGraph(); + + $neverExpiresToken = $graph['member']->createToken('no-expiry')->plainTextToken; + + $this->withHeaders(['Authorization' => 'Bearer '.$neverExpiresToken]) + ->getJson('/api/v1/plugins/'.$graph['plugin']->slug) + ->assertOk(); + + $this->withHeaders(['Authorization' => 'Bearer '.$neverExpiresToken]) + ->getJson('/api/v1/plugins/'.$graph['foreignPlugin']->slug) + ->assertNotFound(); +}); + +it('leaves the other 7 header cases identical after the expired-token fix', function (): void { + $graph = bearerResolutionGraph(); + + foreach (bearerCases($graph) as $label => $case) { + $ownResponse = $this->withHeaders($case['headers']) + ->getJson('/api/v1/plugins/'.$graph['plugin']->slug); + $foreignResponse = $this->withHeaders($case['headers']) + ->getJson('/api/v1/plugins/'.$graph['foreignPlugin']->slug); + + match ($case['group']) { + 'public' => [ + $ownResponse->assertOk(), + $foreignResponse->assertOk(), + ], + 'member' => [ + $ownResponse->assertOk(), + $foreignResponse->assertNotFound(), + ], + 'empty' => [ + $ownResponse->assertNotFound(), + $foreignResponse->assertNotFound(), + ], + }; + } +}); diff --git a/brain/audits/2026-06-13-system-audit.md b/brain/audits/2026-06-13-system-audit.md index e4b9475..df1fde2 100644 --- a/brain/audits/2026-06-13-system-audit.md +++ b/brain/audits/2026-06-13-system-audit.md @@ -398,6 +398,8 @@ Monitor now; upgrade when compatible. Severity: Medium +Status: Closed by ADR-32 in branch `fix/expired-bearer-fail-closed`. An expired token now returns the empty scope. `sanctum.expiration` stays null by decision, so a token without an explicit `expires_at` still never lapses; that is a product question, not this fix. + Found on 2026-09-23 while characterizing F-013. Pre-existing, not introduced by that refactor. Evidence: diff --git a/brain/canonico/CURRENT_STATE.md b/brain/canonico/CURRENT_STATE.md index 49cea93..0b2fb1c 100644 --- a/brain/canonico/CURRENT_STATE.md +++ b/brain/canonico/CURRENT_STATE.md @@ -49,7 +49,7 @@ Core patterns: - Controllers stay thin and delegate domain rules to services, policies, resources, jobs, and models. - API is versioned under `/api/v1`. -- Public discovery endpoints are readable anonymously, but authenticated-looking requests with invalid tokens are not widened to public scope. The rule lives in a single place, `App\Services\Auth\OptionalBearerUserResolver`, injected into `CatalogController` and `SearchController` and locked by characterization tests (ADR-31). Two properties of it are deliberate and documented rather than fixed: a non-bearer `Authorization` header and an empty `Bearer` value fall into the public scope, not the empty scope, and an expired token is still accepted as its owner because these endpoints resolve the token outside the `auth:sanctum` guard (F-015). +- Public discovery endpoints are readable anonymously, but authenticated-looking requests with invalid tokens are not widened to public scope. The rule lives in a single place, `App\Services\Auth\OptionalBearerUserResolver`, injected into `CatalogController` and `SearchController` and locked by characterization tests (ADR-31). An expired token gets the empty scope too, checked in the resolver because these endpoints resolve the token outside the `auth:sanctum` guard (ADR-32); `sanctum.expiration` is null, so a token without an explicit `expires_at` never lapses. One property is deliberate and documented rather than fixed: a non-bearer `Authorization` header and an empty `Bearer` value fall into the public scope, not the empty scope. - Mutating and operational endpoints require Sanctum auth and permission checks. - Ingestion is asynchronous-capable through jobs and Horizon, but can be exercised through services in tests. - Natural keys protect ingestion idempotency: diff --git a/brain/canonico/NEXT_ACTIONS.md b/brain/canonico/NEXT_ACTIONS.md index 89ea085..e348f35 100644 --- a/brain/canonico/NEXT_ACTIONS.md +++ b/brain/canonico/NEXT_ACTIONS.md @@ -299,7 +299,7 @@ Risks: - Decide whether `X-Request-Id` must appear on API responses for requests that match no route. `AssignCorrelationId` lives in the `api` middleware group, so 404 and 405 responses for unrouted paths carry no correlation ID, while the global `SecurityHeaders` still applies. - On Windows worktrees, Jest loads `frontend/e2e/*.spec.ts` despite `testPathIgnorePatterns: ['/e2e/']` and reports 2 failed suites with 0 failed tests. - Fix the two `runtime-config.spec.ts` Jest failures caused by Docker Compose environment variables leaking into the test `process.env`. -- Decide F-015: public discovery endpoints accept an expired bearer token as its owner, because they resolve it through `PersonalAccessToken::findToken` outside the `auth:sanctum` guard and `sanctum.expiration` is null. Closing it changes endpoint responses, so it needs its own branch, its own decision record, and an update to the characterization test that currently locks the behavior. +- Decide whether Sanctum tokens should expire at all. `sanctum.expiration` is null, so a token issued without an explicit `expires_at` never lapses, on public and private endpoints alike. ADR-32 closed the narrower F-015 (an expired token no longer widens the public scope) but deliberately left this product question open, because setting a lifetime changes login and session behavior everywhere. - Keyboard-first power-user UX for command palette actions. - Saved searches and team-level curated collections. - Export command/document references. diff --git a/brain/handoffs/2026-09-23-expired-bearer-fail-closed.md b/brain/handoffs/2026-09-23-expired-bearer-fail-closed.md new file mode 100644 index 0000000..3fac86b --- /dev/null +++ b/brain/handoffs/2026-09-23-expired-bearer-fail-closed.md @@ -0,0 +1,30 @@ +# Handoff - Expired Bearer Fails Closed + +Date: 2026-09-23 +Branch: `fix/expired-bearer-fail-closed` + +## Scope + +Closes F-015, the gap recorded while characterizing F-013: the public discovery endpoints resolve the optional bearer outside the `auth:sanctum` guard, and `PersonalAccessToken::findToken()` does not check `expires_at`, so an expired token still granted its owner's scope. + +## Implemented + +- `App\Services\Auth\OptionalBearerUserResolver` short-circuits between `findToken()` and the `tokenable` check: a resolved token whose `expires_at` is past returns an unpersisted `User`, the same empty scope as any non-empty bearer that fails to resolve. A token with no `expires_at` is untouched. +- The characterization case that locked the old behavior was inverted in the same commit, not deleted, and renamed to state the new contract. Two cases added: a token expired five seconds ago, and a token with no `expires_at` still resolving to its owner. One case pins the other seven header cases as unchanged. +- Decision recorded in ADR-32. + +## Not changed, deliberately + +`config('sanctum.expiration')` stays null, so a token issued without an explicit `expires_at` still never lapses, on every surface. Setting a lifetime changes login and session behavior through the guard; it is a product decision, queued in `NEXT_ACTIONS.md`. Routes, private middleware, policies, and `DiscoveryAccess` untouched. + +## Validation + +- Independent adversarial audit: APPROVED. It probed the resolver directly inside the container against a copy of main's class, case by case: only the expired row differs (owner to empty scope). The `expires_at` null edge was proven by execution, not by reading `?->`. +- The audit proved the test bites: with main's resolver and the branch's test file, exactly the two expired-token cases fail (`Expected response status code [404] but received 200`) and the other ten pass. So the inverted case is stronger, not looser: 404 on the owner's own plugin separates empty scope from both owner (200) and public (200). +- Assertion inventory main to branch: nothing shrank (`assertNotFound` 11 to 18, `assertOk` 17 to 20, the rest equal). No `skip`, `markTestSkipped`, `->todo(`, or `xit(`. +- Gates on the rebuilt image, hashes confirmed to match the branch: Pest 100 passed, Pint 134 files. Assertion counts drift between runs (564 and 566 observed) because of the Meilisearch `retry()` loop; the test count is the stable figure. +- A dead `use Laravel\Sanctum\PersonalAccessToken;` was removed from the test file. Verified dead on `main`: its only other occurrence there is inside a comment. + +## Open + +- Whether Sanctum tokens should expire at all. Until that is decided, this fix only reaches tokens that were given an explicit `expires_at`. diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 38874be..b6dccf9 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -396,3 +396,22 @@ A regra de escopo dos endpoints públicos passa a ter um único ponto de manuten 1. `Authorization: Basic ...` e `Bearer` vazio caem no escopo público, e não no escopo vazio. O guard clause devolve `null` assim que `bearerToken()` é nulo ou vazio, então o escopo vazio só existe para bearer não vazio que falha ao resolver. 2. `findToken` não verifica `expires_at`, e `config('sanctum.expiration')` é `null`. Um token expirado continua aceito como seu dono nestes endpoints públicos, porque eles resolvem o token fora do guard `auth:sanctum`. Os endpoints privados não são afetados: ali quem valida é o guard. A lacuna é pré-existente, está travada por teste nomeado como tal e documentada no docblock do serviço; fechá-la é tarefa própria, com decisão registrada, porque muda resposta de endpoint. + +## ADR-32 — Token expirado falha fechado nos endpoints públicos de descoberta + +### Contexto +Os endpoints públicos de catálogo e busca resolvem o bearer opcional por `PersonalAccessToken::findToken()`, fora do guard `auth:sanctum` (ADR-31). Esse método faz apenas hash e lookup: a verificação de validade temporal vive em `Laravel\Sanctum\Guard`, que só roda atrás do guard. Como `config('sanctum.expiration')` é nulo, nada mais compensava a ausência dessa checagem, e um token com `expires_at` no passado continuava valendo como seu dono nesses endpoints (F-015). O efeito era anular a expiração como mecanismo de revogação justamente na superfície que não passa pelo guard. A lacuna foi encontrada ao caracterizar a F-013 e deixada registrada de propósito, para não misturar mudança funcional a um refactor. + +### Decisão +Rejeitar o token expirado dentro do próprio resolver, entre o `findToken()` e a checagem do `tokenable`: um token resolvido cujo `expires_at` já passou devolve `new User` não persistido, o mesmo escopo vazio de qualquer bearer não vazio que não resolve. Nunca o escopo público, porque a requisição tem credencial e não pode ser promovida a anônima (ADR-23). + +Um token sem `expires_at` continua válido, sem alteração: a checagem é estritamente sobre uma data já vencida. + +`config('sanctum.expiration')` permanece nulo, por decisão explícita. Defini-lo faria todo token passar a vencer, inclusive nos endpoints privados via guard, o que muda login e sessão — é questão de produto, com tarefa e decisão próprias, e segue na fila. + +### Consequências +A expiração volta a valer como revogação em toda a superfície pública: um token vencido deixa de ver o catálogo do seu dono e passa a não ver nada, em vez de cair no escopo público. Endpoints privados não mudam, porque ali quem valida continua sendo o guard. + +O caso de caracterização que travava o comportamento antigo foi invertido no mesmo commit da correção, com asserção mais discriminante: `404` no plugin do próprio dono distingue escopo vazio tanto de dono (`200`) quanto de público (`200`), o que uma asserção de `200` não faria. A auditoria independente confirmou que os dez casos anteriores continuam passando contra o resolver da `main`, e que os dois casos novos falham contra ele — ou seja, o teste morde a mudança em vez de apenas acompanhá-la. + +Com `sanctum.expiration` nulo, a proteção só alcança tokens que receberam `expires_at` explícito. Enquanto essa decisão de produto não for tomada, a maioria dos tokens emitidos não vence, e este ADR não deve ser lido como se tokens caducassem sozinhos.