diff --git a/README.md b/README.md index a4d2be320..abd5aed9e 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ Other errors have no `code`, like a `422` for a commit that fails validation or The app signs in the way an OAuth app does, with the authorization code flow and PKCE ([RFC 7636](https://www.rfc-editor.org/rfc/rfc7636)), and /dev/null as the provider. The code it gets is bound to its key, so only the app that holds the key can redeem it. -1. The app makes an ed25519 key and a random `code_verifier`, and opens this page in a browser session (`ASWebAuthenticationSession` on iOS): +1. The app makes an ed25519 key and a random `code_verifier`, and opens this page in a browser session (`ASWebAuthenticationSession` on iOS, the system browser on a desktop): ```text https://dev-null.sh/mobile/authorize @@ -144,7 +144,7 @@ The app signs in the way an OAuth app does, with the authorization code flow and &redirect_uri=devnull://auth/callback ``` - `code_challenge` is `base64url(sha256(code_verifier))` without padding. `redirect_uri` has to start with `devnull://`. A guest signs in with GitHub first and comes back to the page. A request with a missing or bad parameter gets an error page and is never redirected. + `code_challenge` is `base64url(sha256(code_verifier))` without padding. `redirect_uri` is either the mobile app's own `devnull://` scheme, or, for any other native app such as a desktop app or a CLI, a loopback address it listens on, as [RFC 8252](https://www.rfc-editor.org/rfc/rfc8252#section-7.3) recommends: `http://127.0.0.1:/…` or `http://[::1]:/…`, with a port and no user or fragment (not `localhost`). Either way the code is bound to the key and the `code_verifier`, so another app that gets it can't redeem it. A guest signs in with GitHub first and comes back to the page. A request with a missing or bad parameter gets an error page and is never redirected. 2. The page asks "Approve devnull on iPhone?", with the key's fingerprint and the account it signs in as. **Approve** redirects (`303`) to `devnull://auth/callback?code=…&state=af0ifjsldkj`. **Cancel** goes to `devnull://auth/callback?error=access_denied&state=af0ifjsldkj`. The app checks that `state` is the one it sent. A code is good for 5 minutes and only once, and each user can approve 10 times a minute. 3. The app redeems the code with `POST /api/v1/devices`, signed with the key like any other request, though the key isn't approved yet: diff --git a/app/Http/Requests/CreateMobileAuthorizationRequest.php b/app/Http/Requests/CreateMobileAuthorizationRequest.php index 0e0692530..41b9e8b38 100644 --- a/app/Http/Requests/CreateMobileAuthorizationRequest.php +++ b/app/Http/Requests/CreateMobileAuthorizationRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Dto\MobileAuthorization; +use App\Rules\DeviceRedirectUri; use Illuminate\Contracts\Validation\Validator; use Illuminate\Foundation\Http\FormRequest; @@ -19,7 +20,7 @@ public function authorize(): bool } /** - * @return array> + * @return array> */ public function rules(): array { diff --git a/app/Http/Requests/ShowMobileAuthorizationRequest.php b/app/Http/Requests/ShowMobileAuthorizationRequest.php index 872c7b0fe..d1b7aa666 100644 --- a/app/Http/Requests/ShowMobileAuthorizationRequest.php +++ b/app/Http/Requests/ShowMobileAuthorizationRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Dto\MobileAuthorization; +use App\Rules\DeviceRedirectUri; use Illuminate\Contracts\Validation\Validator; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Http\Exceptions\HttpResponseException; @@ -18,7 +19,7 @@ final class ShowMobileAuthorizationRequest extends FormRequest /** * The rules for what the app sends, on the page and again when the user approves it. * - * @return array> + * @return array> */ public static function parameterRules(): array { @@ -31,7 +32,7 @@ public static function parameterRules(): array 'code_challenge' => ['required', 'string', 'regex:/^[A-Za-z0-9_-]{43}$/'], 'code_challenge_method' => ['required', 'string', 'in:S256'], 'state' => ['required', 'string', 'max:255'], - 'redirect_uri' => ['required', 'string', 'max:255', 'starts_with:'.implode(',', $prefixes), 'not_regex:/#/'], + 'redirect_uri' => ['required', 'string', 'max:255', new DeviceRedirectUri($prefixes), 'not_regex:/#/'], ]; } @@ -51,7 +52,7 @@ public function authorize(): bool } /** - * @return array> + * @return array> */ public function rules(): array { diff --git a/app/Rules/DeviceRedirectUri.php b/app/Rules/DeviceRedirectUri.php new file mode 100644 index 000000000..4f366f2e4 --- /dev/null +++ b/app/Rules/DeviceRedirectUri.php @@ -0,0 +1,46 @@ + $prefixes + */ + public function __construct(private array $prefixes) + { + // + } + + public function validate(string $attribute, mixed $value, Closure $fail): void + { + if (! is_string($value) || (! Str::startsWith($value, $this->prefixes) && ! $this->isLoopback($value))) { + $fail('The :attribute field must start with '.implode(', ', $this->prefixes).', or be a loopback URL like http://127.0.0.1:/.'); + } + } + + private function isLoopback(string $value): bool + { + $url = parse_url($value); + + return is_array($url) + && ($url['scheme'] ?? null) === 'http' + && in_array($url['host'] ?? null, ['127.0.0.1', '[::1]'], true) + && isset($url['port']) + && ! isset($url['user']) + && ! isset($url['pass']) + && ! isset($url['fragment']); + } +} diff --git a/config/mobile.php b/config/mobile.php index c0a953c58..9c22475f8 100644 --- a/config/mobile.php +++ b/config/mobile.php @@ -5,8 +5,9 @@ return [ /* - * Where the authorize page may send the devnull app back to with its code. A redirect_uri has to start with one of - * these, so a code never leaves for anywhere but the app's own URL scheme. + * The URL schemes the authorize page may send an app back to with its code. A redirect_uri has to start with one + * of these, or else be a loopback address the app listens on, so a code never leaves the device it was asked for + * on (see App\Rules\DeviceRedirectUri). */ 'redirect_uri_prefixes' => ['devnull://'], diff --git a/tests/Feature/Controllers/MobileAuthorizationControllerTest.php b/tests/Feature/Controllers/MobileAuthorizationControllerTest.php index 2fc04f408..95928c0a2 100644 --- a/tests/Feature/Controllers/MobileAuthorizationControllerTest.php +++ b/tests/Feature/Controllers/MobileAuthorizationControllerTest.php @@ -89,9 +89,25 @@ 'a plain challenge' => [['code_challenge_method' => 'plain'], 'The selected code challenge method is invalid.'], 'no state' => [['state' => null], 'The state field is required.'], 'no redirect' => [['redirect_uri' => null], 'The redirect uri field is required.'], - 'a redirect to a website' => [['redirect_uri' => 'https://evil.example/callback'], 'The redirect uri field must start with one of the following: devnull://.'], - 'a redirect to another app' => [['redirect_uri' => 'otherapp://auth/callback'], 'The redirect uri field must start with one of the following: devnull://.'], + 'a redirect to a website' => [['redirect_uri' => 'https://evil.example/callback'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], + 'a redirect to another app' => [['redirect_uri' => 'otherapp://auth/callback'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], 'a redirect with a fragment' => [['redirect_uri' => 'devnull://auth/callback#x'], 'The redirect uri field format is invalid.'], + 'a loopback-looking redirect to a website' => [['redirect_uri' => 'http://127.0.0.1:80@evil.example/x'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], + 'a redirect to localhost' => [['redirect_uri' => 'http://localhost:1234/x'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], + 'a loopback redirect over https' => [['redirect_uri' => 'https://127.0.0.1:1/x'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], + 'a redirect to a host under 127.0.0.1' => [['redirect_uri' => 'http://127.0.0.1.evil.example/x'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], + 'a loopback redirect without a port' => [['redirect_uri' => 'http://127.0.0.1/x'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], + 'a loopback redirect with a fragment' => [['redirect_uri' => 'http://127.0.0.1:53682/x#y'], 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.'], +]); + +it('lets an app take the code on a loopback address it listens on', function (string $redirectUri): void { + $this->actingAs(User::factory()->create()) + ->get(route('mobile-authorization.show', [...$this->parameters, 'redirect_uri' => $redirectUri])) + ->assertOk() + ->assertInertia(fn (Assert $page): Assert => $page->where('authorization.redirect_uri', $redirectUri)); +})->with([ + 'ipv4' => 'http://127.0.0.1:53682/callback', + 'ipv6' => 'http://[::1]:53682/callback', ]); it('sends the app back with a one-time code and its state once the user approves it', function (): void { @@ -116,6 +132,22 @@ ]); }); +it('sends an app back to its loopback address with a one-time code and its state', function (): void { + $response = $this->actingAs(User::factory()->create()) + ->post(route('mobile-authorization.store'), [...$this->parameters, 'redirect_uri' => 'http://127.0.0.1:53682/callback']); + + $response->assertStatus(303); + + $location = (string) $response->headers->get('Location'); + parse_str((string) parse_url($location, PHP_URL_QUERY), $query); + + expect($location)->toStartWith('http://127.0.0.1:53682/callback?code=') + ->and($query['state'])->toBe('af0ifjsldkj') + ->and($query['code'])->toBeString(); + + expect($this->codesHandedOut)->toBe(1); +}); + it('keeps the query a redirect already has', function (): void { $response = $this->actingAs(User::factory()->create()) ->post(route('mobile-authorization.store'), [...$this->parameters, 'redirect_uri' => 'devnull://auth/callback?app=1']); @@ -129,7 +161,7 @@ ->post(route('mobile-authorization.store'), [...$this->parameters, 'redirect_uri' => 'https://evil.example/callback']); $response->assertBadRequest() - ->assertInertia(fn (Assert $page): Assert => $page->where('error', 'The redirect uri field must start with one of the following: devnull://.')); + ->assertInertia(fn (Assert $page): Assert => $page->where('error', 'The redirect uri field must start with devnull://, or be a loopback URL like http://127.0.0.1:/.')); expect($this->codesHandedOut)->toBe(0); });