[MTM] Initial token re-work #117

Merged
jonny merged 5 commits from develop into main 2026-08-14 11:42:18 +02:00
14 changed files with 98 additions and 31 deletions
Showing only changes of commit faf8e5c1ec - Show all commits

Fix CSRF exemption and array-input crash on revocation/introspection

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014625MfkGZ7GVdbqKme4a8L
Jonny Barnes 2026-08-13 17:03:43 +01:00
Signed by: jonny
SSH key fingerprint: SHA256:CTuSlns5U7qlD9jqHvtnVmfYV3Zwl2Z7WnJ4/dqOaL8

View file

@ -188,7 +188,7 @@ class IndieAuthController extends Controller
*/ */
public function processRevocationRequest(Request $request): JsonResponse public function processRevocationRequest(Request $request): JsonResponse
{ {
MicropubToken::findActive($request->get('token', ''))?->revoke(); MicropubToken::findActive($request->get('token'))?->revoke();
return response()->json([], 200); return response()->json([], 200);
} }
@ -205,11 +205,11 @@ class IndieAuthController extends Controller
*/ */
public function processIntrospectionRequest(Request $request): JsonResponse public function processIntrospectionRequest(Request $request): JsonResponse
{ {
if (! MicropubToken::findActive((string) $request->bearerToken())) { if (! MicropubToken::findActive($request->bearerToken())) {
return response()->json([], 401); return response()->json([], 401);
} }
$token = MicropubToken::findActive((string) $request->get('token', '')); $token = MicropubToken::findActive($request->get('token'));
if (! $token) { if (! $token) {
return response()->json(['active' => false]); return response()->json(['active' => false]);

View file

@ -26,9 +26,7 @@ class MicropubMediaController extends Controller
$tokenData = $request->input('token_data'); $tokenData = $request->input('token_data');
$scopes = $tokenData['scope']; $scopes = $tokenData['scope'];
if (is_string($scopes)) { $scopes = explode(' ', $scopes);
$scopes = explode(' ', $scopes);
}
if (! in_array('create', $scopes, true)) { if (! in_array('create', $scopes, true)) {
return (new MicropubResponses)->insufficientScopeResponse(); return (new MicropubResponses)->insufficientScopeResponse();
} }
@ -84,9 +82,7 @@ class MicropubMediaController extends Controller
$tokenData = $request->input('token_data'); $tokenData = $request->input('token_data');
$scopes = $tokenData['scope']; $scopes = $tokenData['scope'];
if (is_string($scopes)) { $scopes = explode(' ', $scopes);
$scopes = explode(' ', $scopes);
}
if (! in_array('create', $scopes, true)) { if (! in_array('create', $scopes, true)) {
return (new MicropubResponses)->insufficientScopeResponse(); return (new MicropubResponses)->insufficientScopeResponse();
} }

View file

@ -18,6 +18,7 @@ class LinkHeadersMiddleware
$response->header('Link', '<'.route('indieauth.start').'>; rel="authorization_endpoint"', false); $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.token').'>; rel="token_endpoint"', false);
$response->header('Link', '<'.route('indieauth.revocation').'>; rel="revocation_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('micropub-endpoint').'>; rel="micropub"', false);
$response->header('Link', '<'.route('webmention-endpoint').'>; rel="webmention"', false); $response->header('Link', '<'.route('webmention-endpoint').'>; rel="webmention"', false);

View file

@ -5,11 +5,9 @@ declare(strict_types=1);
namespace App\Models; namespace App\Models;
use Illuminate\Database\Eloquent\Attributes\Fillable; use Illuminate\Database\Eloquent\Attributes\Fillable;
use Illuminate\Database\Eloquent\Attributes\Table;
use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Casts\Attribute;
use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Model;
#[Table('micropub_tokens')]
#[Fillable(['token_hash', 'client_id', 'me', 'scope'])] #[Fillable(['token_hash', 'client_id', 'me', 'scope'])]
class MicropubToken extends Model 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; return null;
} }

View file

@ -24,9 +24,7 @@ class CardHandler implements MicropubHandlerInterface
assert($data instanceof CardData); assert($data instanceof CardData);
$scopes = $data->tokenData['scope']; $scopes = $data->tokenData['scope'];
if (is_string($scopes)) { $scopes = explode(' ', $scopes);
$scopes = explode(' ', $scopes);
}
if (! in_array('create', $scopes, true)) { if (! in_array('create', $scopes, true)) {
throw new InvalidTokenScopeException; throw new InvalidTokenScopeException;

View file

@ -27,9 +27,7 @@ class EntryHandler implements MicropubHandlerInterface
assert($data instanceof EntryData); assert($data instanceof EntryData);
$scopes = $data->tokenData['scope']; $scopes = $data->tokenData['scope'];
if (is_string($scopes)) { $scopes = explode(' ', $scopes);
$scopes = explode(' ', $scopes);
}
if (! in_array('create', $scopes, true)) { if (! in_array('create', $scopes, true)) {
throw new InvalidTokenScopeException; throw new InvalidTokenScopeException;

View file

@ -30,9 +30,7 @@ class UpdateHandler implements MicropubHandlerInterface
assert($data instanceof UpdateData); assert($data instanceof UpdateData);
$scopes = $data->tokenData['scope']; $scopes = $data->tokenData['scope'];
if (is_string($scopes)) { $scopes = explode(' ', $scopes);
$scopes = explode(' ', $scopes);
}
if (! in_array('update', $scopes, true)) { if (! in_array('update', $scopes, true)) {
throw new InvalidTokenScopeException; throw new InvalidTokenScopeException;

View file

@ -17,8 +17,10 @@ return Application::configure(basePath: dirname(__DIR__))
->append(LinkHeadersMiddleware::class) ->append(LinkHeadersMiddleware::class)
->preventRequestForgery( ->preventRequestForgery(
except: [ except: [
'auth', // This is the IndieAuth auth endpoint 'auth', // This is the IndieAuth auth endpoint
'token', // This is the IndieAuth token 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/post',
'api/media', 'api/media',
'micropub/places', 'micropub/places',

View file

@ -12,7 +12,7 @@ return new class extends Migration
{ {
Schema::create('micropub_tokens', function (Blueprint $table) { Schema::create('micropub_tokens', function (Blueprint $table) {
$table->id(); $table->id();
$table->string('token_hash')->unique(); $table->string('token_hash', 64)->unique();
$table->string('client_id'); $table->string('client_id');
$table->string('me'); $table->string('me');
$table->string('scope'); $table->string('scope');

View file

@ -14,7 +14,7 @@
@if($token->isRevoked) @if($token->isRevoked)
revoked {{ $token->revoked_at->diffForHumans() }} revoked {{ $token->revoked_at->diffForHumans() }}
@else @else
<form action="/admin/tokens/{{ $token->id }}/revoke" method="post" style="display:inline"> <form action="/admin/tokens/{{ $token->id }}/revoke" method="post">
{{ csrf_field() }} {{ csrf_field() }}
{{ method_field('PUT') }} {{ method_field('PUT') }}
<button type="submit" name="revoke">Revoke</button> <button type="submit" name="revoke">Revoke</button>

View file

@ -0,0 +1,29 @@
<?php
declare(strict_types=1);
namespace Tests\Feature;
use Illuminate\Foundation\Http\Middleware\PreventRequestForgery;
use PHPUnit\Framework\Attributes\Test;
use Tests\TestCase;
class CsrfExemptionsTest extends TestCase
{
/**
* CSRF verification is short-circuited entirely while running the test
* suite (see PreventRequestForgery::runningUnitTests()), so a normal
* feature test hitting these routes would pass even if they were never
* added to bootstrap/app.php's except list. Assert against the actual
* configured exemptions instead.
*/
#[Test]
public function external_api_endpoints_are_exempt_from_csrf_verification(): void
{
$exemptions = $this->app->make(PreventRequestForgery::class)->getExcludedPaths();
foreach (['auth', 'token', 'revocation', 'introspect', 'api/post', 'api/media', 'micropub/places', 'webmention'] as $path) {
$this->assertContains($path, $exemptions);
}
}
}

View file

@ -20,7 +20,8 @@ class HeaderLinkTest extends TestCase
$this->assertSame('<'.config('app.url').'/auth>; rel="authorization_endpoint"', $linkHeaders[1]); $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').'/token>; rel="token_endpoint"', $linkHeaders[2]);
$this->assertSame('<'.config('app.url').'/revocation>; rel="revocation_endpoint"', $linkHeaders[3]); $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').'/introspect>; rel="introspection_endpoint"', $linkHeaders[4]);
$this->assertSame('<'.config('app.url').'/webmention>; rel="webmention"', $linkHeaders[5]); $this->assertSame('<'.config('app.url').'/api/post>; rel="micropub"', $linkHeaders[5]);
$this->assertSame('<'.config('app.url').'/webmention>; rel="webmention"', $linkHeaders[6]);
} }
} }

View file

@ -29,9 +29,10 @@ class IndieAuthTest extends TestCase
'issuer' => config('app.url'), 'issuer' => config('app.url'),
'authorization_endpoint' => route('indieauth.start'), 'authorization_endpoint' => route('indieauth.start'),
'token_endpoint' => route('indieauth.token'), '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'], '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->assertStatus(200);
$response->assertExactJson(['active' => false]); $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]);
}
} }

View file

@ -71,4 +71,18 @@ class TokenServiceTest extends TestCase
'error_description' => 'The provided token did not pass validation', '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));
}
} }