From faf8e5c1ec6e1875fc27c840a87e5a08e14f31bd Mon Sep 17 00:00:00 2001 From: Jonny Barnes Date: Thu, 13 Aug 2026 17:03:43 +0100 Subject: [PATCH] Fix CSRF exemption and array-input crash on revocation/introspection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An Opus code review of the branch caught two real bugs the test suite structurally couldn't see: - /revocation and /introspect were never added to bootstrap/app.php's CSRF except list, so both were fully broken (403) for any real external client, despite every feature test passing — CSRF verification is short-circuited entirely while running tests. Verified live against the running app before and after the fix, and added a regression test that asserts against the actual configured exemptions rather than relying on request-time behavior that tests can't exercise. - An array-shaped `token` param (e.g. token[]=a&token[]=b) crashed both endpoints with a 500, since this app promotes PHP warnings ("Array to string conversion") to exceptions. Fixed at the shared root, MicropubToken::findActive(), which also closes the same latent hole in VerifyMicropubToken's access_token param that predates this branch. Verified live and covered with regression tests. Also applied the review's lower-severity findings: added the missing introspection_endpoint Link header and metadata test assertions, removed the now-dead is_string($scopes) array branch in the Micropub handlers and media controller (scope is unconditionally a string from the DB now, this guarded against a JWT-array-claim shape that can no longer occur), dropped a redundant #[Table] model attribute, sized token_hash to its actual 64-char length, and removed a one-off inline style in the admin view. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_014625MfkGZ7GVdbqKme4a8L --- app/Http/Controllers/IndieAuthController.php | 6 ++-- .../Controllers/MicropubMediaController.php | 8 ++--- app/Http/Middleware/LinkHeadersMiddleware.php | 1 + app/Models/MicropubToken.php | 12 ++++--- .../Micropub/Handlers/CardHandler.php | 4 +-- .../Micropub/Handlers/EntryHandler.php | 4 +-- .../Micropub/Handlers/UpdateHandler.php | 4 +-- bootstrap/app.php | 6 ++-- ...13_120924_create_micropub_tokens_table.php | 2 +- resources/views/admin/tokens/index.blade.php | 2 +- tests/Feature/CsrfExemptionsTest.php | 29 +++++++++++++++++ tests/Feature/HeaderLinkTest.php | 5 +-- tests/Feature/IndieAuthTest.php | 32 +++++++++++++++++-- tests/Feature/TokenServiceTest.php | 14 ++++++++ 14 files changed, 98 insertions(+), 31 deletions(-) create mode 100644 tests/Feature/CsrfExemptionsTest.php diff --git a/app/Http/Controllers/IndieAuthController.php b/app/Http/Controllers/IndieAuthController.php index bff8ebee..a795bce8 100644 --- a/app/Http/Controllers/IndieAuthController.php +++ b/app/Http/Controllers/IndieAuthController.php @@ -188,7 +188,7 @@ class IndieAuthController extends Controller */ public function processRevocationRequest(Request $request): JsonResponse { - MicropubToken::findActive($request->get('token', ''))?->revoke(); + MicropubToken::findActive($request->get('token'))?->revoke(); return response()->json([], 200); } @@ -205,11 +205,11 @@ class IndieAuthController extends Controller */ public function processIntrospectionRequest(Request $request): JsonResponse { - if (! MicropubToken::findActive((string) $request->bearerToken())) { + if (! MicropubToken::findActive($request->bearerToken())) { return response()->json([], 401); } - $token = MicropubToken::findActive((string) $request->get('token', '')); + $token = MicropubToken::findActive($request->get('token')); if (! $token) { return response()->json(['active' => false]); diff --git a/app/Http/Controllers/MicropubMediaController.php b/app/Http/Controllers/MicropubMediaController.php index da7c7dc2..d9f8ea32 100644 --- a/app/Http/Controllers/MicropubMediaController.php +++ b/app/Http/Controllers/MicropubMediaController.php @@ -26,9 +26,7 @@ class MicropubMediaController extends Controller $tokenData = $request->input('token_data'); $scopes = $tokenData['scope']; - if (is_string($scopes)) { - $scopes = explode(' ', $scopes); - } + $scopes = explode(' ', $scopes); if (! in_array('create', $scopes, true)) { return (new MicropubResponses)->insufficientScopeResponse(); } @@ -84,9 +82,7 @@ class MicropubMediaController extends Controller $tokenData = $request->input('token_data'); $scopes = $tokenData['scope']; - if (is_string($scopes)) { - $scopes = explode(' ', $scopes); - } + $scopes = explode(' ', $scopes); if (! in_array('create', $scopes, true)) { return (new MicropubResponses)->insufficientScopeResponse(); } diff --git a/app/Http/Middleware/LinkHeadersMiddleware.php b/app/Http/Middleware/LinkHeadersMiddleware.php index 0a280d44..e2810f87 100644 --- a/app/Http/Middleware/LinkHeadersMiddleware.php +++ b/app/Http/Middleware/LinkHeadersMiddleware.php @@ -18,6 +18,7 @@ class LinkHeadersMiddleware $response->header('Link', '<'.route('indieauth.start').'>; rel="authorization_endpoint"', false); $response->header('Link', '<'.route('indieauth.token').'>; rel="token_endpoint"', false); $response->header('Link', '<'.route('indieauth.revocation').'>; rel="revocation_endpoint"', false); + $response->header('Link', '<'.route('indieauth.introspection').'>; rel="introspection_endpoint"', false); $response->header('Link', '<'.route('micropub-endpoint').'>; rel="micropub"', false); $response->header('Link', '<'.route('webmention-endpoint').'>; rel="webmention"', false); diff --git a/app/Models/MicropubToken.php b/app/Models/MicropubToken.php index e85cb206..c4df41bc 100644 --- a/app/Models/MicropubToken.php +++ b/app/Models/MicropubToken.php @@ -5,11 +5,9 @@ declare(strict_types=1); namespace App\Models; use Illuminate\Database\Eloquent\Attributes\Fillable; -use Illuminate\Database\Eloquent\Attributes\Table; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Model; -#[Table('micropub_tokens')] #[Fillable(['token_hash', 'client_id', 'me', 'scope'])] class MicropubToken extends Model { @@ -26,11 +24,15 @@ class MicropubToken extends Model } /** - * Find the active (non-revoked) token matching a raw bearer token string. + * Find the active (non-revoked) token matching a raw bearer token value. + * + * Accepts mixed because callers pass request input directly, which PHP + * lets be an array (e.g. a client sending token[]=a) - casting that to + * string would throw, so anything non-string is just treated as absent. */ - public static function findActive(string $rawToken): ?self + public static function findActive(mixed $rawToken): ?self { - if ($rawToken === '') { + if (! is_string($rawToken) || $rawToken === '') { return null; } diff --git a/app/Services/Micropub/Handlers/CardHandler.php b/app/Services/Micropub/Handlers/CardHandler.php index 02e3a066..6b24f21b 100644 --- a/app/Services/Micropub/Handlers/CardHandler.php +++ b/app/Services/Micropub/Handlers/CardHandler.php @@ -24,9 +24,7 @@ class CardHandler implements MicropubHandlerInterface assert($data instanceof CardData); $scopes = $data->tokenData['scope']; - if (is_string($scopes)) { - $scopes = explode(' ', $scopes); - } + $scopes = explode(' ', $scopes); if (! in_array('create', $scopes, true)) { throw new InvalidTokenScopeException; diff --git a/app/Services/Micropub/Handlers/EntryHandler.php b/app/Services/Micropub/Handlers/EntryHandler.php index ef9740f2..48bbb550 100644 --- a/app/Services/Micropub/Handlers/EntryHandler.php +++ b/app/Services/Micropub/Handlers/EntryHandler.php @@ -27,9 +27,7 @@ class EntryHandler implements MicropubHandlerInterface assert($data instanceof EntryData); $scopes = $data->tokenData['scope']; - if (is_string($scopes)) { - $scopes = explode(' ', $scopes); - } + $scopes = explode(' ', $scopes); if (! in_array('create', $scopes, true)) { throw new InvalidTokenScopeException; diff --git a/app/Services/Micropub/Handlers/UpdateHandler.php b/app/Services/Micropub/Handlers/UpdateHandler.php index 49f86063..136a0840 100644 --- a/app/Services/Micropub/Handlers/UpdateHandler.php +++ b/app/Services/Micropub/Handlers/UpdateHandler.php @@ -30,9 +30,7 @@ class UpdateHandler implements MicropubHandlerInterface assert($data instanceof UpdateData); $scopes = $data->tokenData['scope']; - if (is_string($scopes)) { - $scopes = explode(' ', $scopes); - } + $scopes = explode(' ', $scopes); if (! in_array('update', $scopes, true)) { throw new InvalidTokenScopeException; diff --git a/bootstrap/app.php b/bootstrap/app.php index 9c73bdb9..e29a3598 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -17,8 +17,10 @@ return Application::configure(basePath: dirname(__DIR__)) ->append(LinkHeadersMiddleware::class) ->preventRequestForgery( except: [ - 'auth', // This is the IndieAuth auth endpoint - 'token', // This is the IndieAuth token endpoint + 'auth', // This is the IndieAuth auth endpoint + 'token', // This is the IndieAuth token endpoint + 'revocation', // This is the IndieAuth revocation endpoint + 'introspect', // This is the IndieAuth introspection endpoint 'api/post', 'api/media', 'micropub/places', diff --git a/database/migrations/2026_08_13_120924_create_micropub_tokens_table.php b/database/migrations/2026_08_13_120924_create_micropub_tokens_table.php index cb37f28b..e336912f 100644 --- a/database/migrations/2026_08_13_120924_create_micropub_tokens_table.php +++ b/database/migrations/2026_08_13_120924_create_micropub_tokens_table.php @@ -12,7 +12,7 @@ return new class extends Migration { Schema::create('micropub_tokens', function (Blueprint $table) { $table->id(); - $table->string('token_hash')->unique(); + $table->string('token_hash', 64)->unique(); $table->string('client_id'); $table->string('me'); $table->string('scope'); diff --git a/resources/views/admin/tokens/index.blade.php b/resources/views/admin/tokens/index.blade.php index cb7edda9..8836ab07 100644 --- a/resources/views/admin/tokens/index.blade.php +++ b/resources/views/admin/tokens/index.blade.php @@ -14,7 +14,7 @@ @if($token->isRevoked) — revoked {{ $token->revoked_at->diffForHumans() }} @else -
+ {{ csrf_field() }} {{ method_field('PUT') }} diff --git a/tests/Feature/CsrfExemptionsTest.php b/tests/Feature/CsrfExemptionsTest.php new file mode 100644 index 00000000..c03a3c2c --- /dev/null +++ b/tests/Feature/CsrfExemptionsTest.php @@ -0,0 +1,29 @@ +app->make(PreventRequestForgery::class)->getExcludedPaths(); + + foreach (['auth', 'token', 'revocation', 'introspect', 'api/post', 'api/media', 'micropub/places', 'webmention'] as $path) { + $this->assertContains($path, $exemptions); + } + } +} diff --git a/tests/Feature/HeaderLinkTest.php b/tests/Feature/HeaderLinkTest.php index 8a68d88f..3983b02c 100644 --- a/tests/Feature/HeaderLinkTest.php +++ b/tests/Feature/HeaderLinkTest.php @@ -20,7 +20,8 @@ class HeaderLinkTest extends TestCase $this->assertSame('<'.config('app.url').'/auth>; rel="authorization_endpoint"', $linkHeaders[1]); $this->assertSame('<'.config('app.url').'/token>; rel="token_endpoint"', $linkHeaders[2]); $this->assertSame('<'.config('app.url').'/revocation>; rel="revocation_endpoint"', $linkHeaders[3]); - $this->assertSame('<'.config('app.url').'/api/post>; rel="micropub"', $linkHeaders[4]); - $this->assertSame('<'.config('app.url').'/webmention>; rel="webmention"', $linkHeaders[5]); + $this->assertSame('<'.config('app.url').'/introspect>; rel="introspection_endpoint"', $linkHeaders[4]); + $this->assertSame('<'.config('app.url').'/api/post>; rel="micropub"', $linkHeaders[5]); + $this->assertSame('<'.config('app.url').'/webmention>; rel="webmention"', $linkHeaders[6]); } } diff --git a/tests/Feature/IndieAuthTest.php b/tests/Feature/IndieAuthTest.php index ba456748..08282228 100644 --- a/tests/Feature/IndieAuthTest.php +++ b/tests/Feature/IndieAuthTest.php @@ -29,9 +29,10 @@ class IndieAuthTest extends TestCase 'issuer' => config('app.url'), 'authorization_endpoint' => route('indieauth.start'), 'token_endpoint' => route('indieauth.token'), + 'revocation_endpoint' => route('indieauth.revocation'), + 'introspection_endpoint' => route('indieauth.introspection'), + 'introspection_endpoint_auth_methods_supported' => ['Bearer'], 'code_challenge_methods_supported' => ['S256'], - // 'introspection_endpoint' => 'introspection_endpoint', - // 'introspection_endpoint_auth_methods_supported' => ['none'], ]); } @@ -795,4 +796,31 @@ class IndieAuthTest extends TestCase $response->assertStatus(200); $response->assertExactJson(['active' => false]); } + + #[Test] + public function revocation_does_not_error_on_an_array_shaped_token_param(): void + { + $response = $this->post('/revocation', ['token' => ['a', 'b']]); + + $response->assertStatus(200); + } + + #[Test] + public function introspection_does_not_error_on_an_array_shaped_token_param(): void + { + $callerToken = resolve(TokenService::class)->getNewToken([ + 'me' => config('app.url'), + 'client_id' => 'https://app.example.com', + 'scope' => 'create', + ]); + + $response = $this->post( + '/introspect', + ['token' => ['a', 'b']], + ['HTTP_Authorization' => 'Bearer '.$callerToken] + ); + + $response->assertStatus(200); + $response->assertExactJson(['active' => false]); + } } diff --git a/tests/Feature/TokenServiceTest.php b/tests/Feature/TokenServiceTest.php index 6643452d..91b9e81d 100644 --- a/tests/Feature/TokenServiceTest.php +++ b/tests/Feature/TokenServiceTest.php @@ -71,4 +71,18 @@ class TokenServiceTest extends TestCase 'error_description' => 'The provided token did not pass validation', ]); } + + /** + * Request input for a "string" field can be sent as an array + * (e.g. token[]=a&token[]=b). Casting that to string throws in this app + * (warnings are promoted to exceptions), so findActive() must guard + * against it rather than assume its caller already validated the type. + */ + #[Test] + public function find_active_treats_non_string_input_as_absent(): void + { + $this->assertNull(MicropubToken::findActive(['a', 'b'])); + $this->assertNull(MicropubToken::findActive(null)); + $this->assertNull(MicropubToken::findActive(123)); + } }