From 856155b01a2c3964acfb988d3a6bda6116fbb593 Mon Sep 17 00:00:00 2001 From: Simon Hamp Date: Thu, 5 Nov 2026 23:03:51 +0000 Subject: [PATCH] Keep the sign-in attempt when the phone kills the app mid sign-in A Redmi Note 12 4G always got "that sign-in didn't start on this phone". MIUI kills the app while the auth browser is in front, so the callback started a fresh process that had no attempt in memory to check the state against. The attempt's verifier and state are now also kept in the Keychain (the Keystore on Android) until the callback takes them. A fresh process falls back to that copy. It's deleted once used and ignored after ten minutes, and the state is checked as before. Co-Authored-By: Claude Opus 5.5 --- app/Devnull/SignIn.php | 77 ++++++++++++++++++++++-- app/Devnull/SignInAttempt.php | 15 ++++- tests/Feature/Screens/OnboardingTest.php | 47 +++++++++++++-- 3 files changed, 128 insertions(+), 11 deletions(-) diff --git a/app/Devnull/SignIn.php b/app/Devnull/SignIn.php index c037cab0..40974ffa 100644 --- a/app/Devnull/SignIn.php +++ b/app/Devnull/SignIn.php @@ -2,16 +2,32 @@ namespace App\Devnull; +use App\Devnull\Exceptions\KeychainUnavailable; +use Carbon\CarbonImmutable; + /** - * The sign-in under way, held in memory only. + * The sign-in under way. * * It keeps the attempt the browser is showing, and the browser's answer once * devnull://auth/callback brings it back, until onboarding takes both. A new * attempt replaces the old one, and an answer is taken once, so each attempt - * is good for one answer whatever it says. Nothing here is ever written down. + * is good for one answer whatever it says. + * + * The attempt is also kept in the Keychain until it's answered. Phones short + * on memory, Xiaomi's above all, kill the app while the browser is in front, + * and the callback then lands in a fresh process that would know of no + * attempt. Only the Keychain holds it, it's gone once taken, and one older + * than KeptFor is never used. The browser's answer stays in memory. */ final class SignIn { + public const string StorageKey = 'devnull.sign-in'; + + /** + * How long a kept attempt can still be answered, in seconds. + */ + public const int KeptFor = 600; + private ?SignInAttempt $attempt = null; /** @@ -19,11 +35,15 @@ final class SignIn */ private ?array $callback = null; + public function __construct(private KeyStore $store) {} + public function begin(): SignInAttempt { $this->callback = null; + $this->attempt = SignInAttempt::start(); + $this->keep($this->attempt); - return $this->attempt = SignInAttempt::start(); + return $this->attempt; } /** @@ -41,6 +61,7 @@ public function hasCallback(): bool /** * The browser's answer and the attempt it should answer, forgetting both. + * The attempt comes from the Keychain when the app was started afresh. * * @return array{0: array, 1: SignInAttempt|null}|null */ @@ -50,10 +71,58 @@ public function take(): ?array return null; } - $taken = [$this->callback, $this->attempt]; + $taken = [$this->callback, $this->attempt ?? $this->kept()]; $this->callback = null; $this->attempt = null; + $this->forgetKept(); return $taken; } + + /** + * Keeping it is a fallback: when the Keychain won't take it, the attempt + * still works for as long as the app stays alive. + */ + private function keep(SignInAttempt $attempt): void + { + try { + $this->store->set(self::StorageKey, (string) json_encode([ + 'verifier' => $attempt->verifier(), + 'state' => $attempt->state, + 'started_at' => CarbonImmutable::now()->getTimestamp(), + ])); + } catch (KeychainUnavailable) { + // + } + } + + private function kept(): ?SignInAttempt + { + try { + $stored = $this->store->read(self::StorageKey); + } catch (KeychainUnavailable) { + return null; + } + + $kept = $stored->found() ? json_decode((string) $stored->value, true) : null; + + if (! is_array($kept) || ! is_string($kept['verifier'] ?? null) || ! is_string($kept['state'] ?? null) || ! is_int($kept['started_at'] ?? null)) { + return null; + } + + if (CarbonImmutable::now()->getTimestamp() - $kept['started_at'] > self::KeptFor) { + return null; + } + + return SignInAttempt::resume($kept['verifier'], $kept['state']); + } + + private function forgetKept(): void + { + try { + $this->store->delete(self::StorageKey); + } catch (KeychainUnavailable) { + // + } + } } diff --git a/app/Devnull/SignInAttempt.php b/app/Devnull/SignInAttempt.php index 1b01587f..ccde5fb1 100644 --- a/app/Devnull/SignInAttempt.php +++ b/app/Devnull/SignInAttempt.php @@ -2,15 +2,16 @@ namespace App\Devnull; +use SensitiveParameter; use SensitiveParameterValue; /** * One go at signing in through dev-null.sh: OAuth's authorization code flow * with PKCE, devnull being the provider. * - * The verifier and state are made fresh for each attempt and live only in - * memory. The verifier is wrapped so it never shows up in a dump or a stack - * trace, and can't be serialized to disk. + * The verifier and state are made fresh for each attempt, and kept outside + * memory only in the Keychain (see SignIn). The verifier is wrapped so it + * never shows up in a dump or a stack trace, and can't be serialized to disk. */ final readonly class SignInAttempt { @@ -23,6 +24,14 @@ public static function start(): self return new self(new SensitiveParameterValue(self::base64Url(random_bytes(32))), self::base64Url(random_bytes(16))); } + /** + * The attempt SignIn kept in the Keychain, after the app was started afresh. + */ + public static function resume(#[SensitiveParameter] string $verifier, string $state): self + { + return new self(new SensitiveParameterValue($verifier), $state); + } + public function verifier(): string { return $this->verifier->getValue(); diff --git a/tests/Feature/Screens/OnboardingTest.php b/tests/Feature/Screens/OnboardingTest.php index 05b279cb..9027e23b 100644 --- a/tests/Feature/Screens/OnboardingTest.php +++ b/tests/Feature/Screens/OnboardingTest.php @@ -375,7 +375,7 @@ function sendBack(TestableComponent $screen, array $query): TestableComponent Http::assertSentCount(1); }); -it('never writes the verifier or the state down', function () { +it('keeps the verifier and the state only in the Keychain, and forgets them once answered', function () { Storage::fake('local'); $bridge = signInBridge(testSeed()); fakeApi(['POST /api/v1/devices' => [['data' => userWithCountsJson()]]]); @@ -388,17 +388,56 @@ function sendBack(TestableComponent $screen, array $query): TestableComponent ->and(json_encode($screen->tree()))->not->toContain($verifier)->not->toContain($state) ->and(Storage::disk('local')->allFiles())->toBe([]) ->and(fn () => serialize($attempt))->toThrow(Exception::class) - ->and(print_r($attempt, true))->not->toContain($verifier); - - $bridge->assertNotCalled('SecureStorage.Set'); + ->and(print_r($attempt, true))->not->toContain($verifier) + ->and(array_column(array_column($bridge->callsTo('SecureStorage.Set'), 'params'), 'key'))->toBe([SignIn::StorageKey]); sendBack($screen, ['code' => 'c', 'state' => $state])->assertReplacedWith('/log'); expect(app(SignIn::class)->take())->toBeNull() ->and(pendingAttempt())->toBeNull() ->and(json_encode(app(Account::class)->user()?->toArray()))->not->toContain($verifier)->not->toContain($state); + + $bridge->assertCalled('SecureStorage.Delete', fn (array $params): bool => $params['key'] === SignIn::StorageKey); +}); + +it('finishes a sign-in when the phone killed the app while the browser was open', function () { + $bridge = signInBridge(testSeed()); + fakeApi(['POST /api/v1/devices' => [['data' => userWithCountsJson()]]]); + $state = tapSignIn(Native::visit('/onboarding'), $bridge); + $verifier = pendingAttempt()->verifier(); + + // The callback starts the app afresh, with nothing of the attempt left in memory. + app()->forgetInstance(SignIn::class); + + Native::visit('/auth/callback?'.http_build_query(['code' => 'c', 'state' => $state])) + ->assertReplacedWith('/onboarding') + ->follow() + ->assertReplacedWith('/log'); + + Http::assertSent(fn (Request $request): bool => $request->method() === 'POST' + && $request->body() === json_encode(['code' => 'c', 'code_verifier' => $verifier, 'terms_accepted' => true])); }); +it('rejects a callback after a restart when the state is not the kept one, or the attempt is too old', function (int $minutesLater, string $state) { + $bridge = signInBridge(testSeed()); + fakeApi([]); + $sent = tapSignIn(Native::visit('/onboarding'), $bridge); + + app()->forgetInstance(SignIn::class); + $this->travel($minutesLater)->minutes(); + + Native::visit('/auth/callback?'.http_build_query(['code' => 'c', 'state' => $state === 'sent' ? $sent : $state])) + ->assertReplacedWith('/onboarding') + ->follow() + ->assertSee(Onboarding::NotStartedHere); + + Http::assertNothingSent(); + $bridge->assertCalled('SecureStorage.Delete', fn (array $params): bool => $params['key'] === SignIn::StorageKey); +})->with([ + 'another state' => [1, 'not-the-state'], + 'kept too long' => [11, 'sent'], +]); + it('says so when the auth browser does not open', function () { fakeKeychain(testSeed())->respondTo('Browser.OpenAuth', ['success' => false]);