From 84b28f7371972fd92d860fe5d758dae5ecdf99e6 Mon Sep 17 00:00:00 2001 From: Luan Trindade Date: Wed, 23 Sep 2026 10:18:09 -0300 Subject: [PATCH 1/2] fix: reject expired bearer tokens on public discovery endpoints (F-015) OptionalBearerUserResolver resolved bearer tokens via PersonalAccessToken::findToken() outside the auth:sanctum guard, which never checks expires_at. A token expired in the past kept granting its owner's community scope on the public catalog and search endpoints, defeating token expiry as a revocation mechanism there (ADR-31 gap, tracked as F-015). The resolver now treats a resolved-but-expired token the same as any other bearer that fails to resolve: an empty, unpersisted User (empty scope), never the owner's scope and never the anonymous public scope (ADR-23). A token without expires_at is unaffected and keeps resolving to its owner, since sanctum.expiration stays out of scope here. The characterization test that locked in the old, wrong behavior is inverted in this same commit to assert the new empty-scope outcome, and gains coverage for a token that expired seconds ago, a token with no expires_at, and confirmation that the other 7 header cases are unchanged. An unrelated pre-existing unused import in the same test file is also removed to keep Pint green. Co-Authored-By: Claude Opus 5 --- .../Auth/OptionalBearerUserResolver.php | 17 +++-- .../OptionalBearerUserResolutionTest.php | 72 ++++++++++++++++--- 2 files changed, 75 insertions(+), 14 deletions(-) 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(), + ], + }; + } +}); From eaa2e6306e0381ff503f00dd3ecc48578e7ee84b Mon Sep 17 00:00:00 2001 From: Luan Trindade Date: Wed, 23 Sep 2026 10:27:12 -0300 Subject: [PATCH 2/2] docs: record ADR-32 and close F-015 in the brain Co-Authored-By: Claude Opus 5 --- brain/audits/2026-06-13-system-audit.md | 2 ++ brain/canonico/CURRENT_STATE.md | 2 +- brain/canonico/NEXT_ACTIONS.md | 2 +- .../2026-09-23-expired-bearer-fail-closed.md | 30 +++++++++++++++++++ docs/DECISIONS.md | 19 ++++++++++++ 5 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 brain/handoffs/2026-09-23-expired-bearer-fail-closed.md 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.