From abf01da6c0f3a6c070e2d21974cac157a12a7054 Mon Sep 17 00:00:00 2001 From: Julien BIANCHI Date: Wed, 28 Oct 2026 00:13:36 +0100 Subject: [PATCH] fix(links): open only web pages in the in-app browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Conversation::openLink() passed a link preview's URL from devnull straight to Browser::inApp(), and so did the GitHub profile button and View issue beside an opened issue. A URL with another scheme either threw from Browser::inApp() (javascript:, tel:), which broke the tap, or reached SFSafariViewController, which only accepts http and https (devnull://, other apps' schemes). SiteLink::isWebUrl() now holds the one check: http or https, any case, with a host and no user or password before it. Every link the app opens from devnull's answers goes through it, replacing the two copies of a scheme-only check in ShowsCommits and SyncsOutbox, and SiteLink::screen() builds on it. 🤖 Généré avec Claude Code --- app/Devnull/SiteLink.php | 29 +++++++++++++++---- .../Concerns/DraftsIssues.php | 3 +- .../Concerns/ShowsCommits.php | 15 ++-------- app/NativeComponents/Concerns/SyncsOutbox.php | 3 +- app/NativeComponents/Conversation.php | 2 +- app/NativeComponents/Profile.php | 7 +++-- tests/Feature/Devnull/SiteLinkTest.php | 22 ++++++++++++++ tests/Feature/Screens/ConversationTest.php | 11 +++++++ tests/Feature/Screens/IssueCommandsTest.php | 8 +++++ tests/Feature/Screens/ProfileTest.php | 11 +++++++ 10 files changed, 88 insertions(+), 23 deletions(-) diff --git a/app/Devnull/SiteLink.php b/app/Devnull/SiteLink.php index 2ea818dc..6aa1acd3 100644 --- a/app/Devnull/SiteLink.php +++ b/app/Devnull/SiteLink.php @@ -70,6 +70,26 @@ public static function screen(?string $url): ?string return null; } + /** + * Whether a URL from devnull is a web page the browser may open: http or + * https, with a host, and no user or password before it. Nothing else a + * link might carry, like javascript:, file: or another app's scheme, can + * take the phone somewhere unexpected, and the in-app browser only opens + * web pages anyway. + * + * @phpstan-assert-if-true string $url + */ + public static function isWebUrl(?string $url): bool + { + $parts = $url === null ? false : parse_url($url); + + if ($parts === false || isset($parts['user']) || isset($parts['pass'])) { + return false; + } + + return in_array(strtolower($parts['scheme'] ?? ''), ['http', 'https'], true) && ($parts['host'] ?? '') !== ''; + } + /** * The path of an http or https URL on one of dev/null's hosts. A URL with * a user or password before its host is never one, as browsers and PHP @@ -77,17 +97,14 @@ public static function screen(?string $url): ?string */ private static function pathOnDevnull(?string $url): ?string { - $parts = $url === null ? false : parse_url($url); - - if ($parts === false || isset($parts['user']) || isset($parts['pass'])) { + if (! self::isWebUrl($url)) { return null; } - $scheme = strtolower($parts['scheme'] ?? ''); - $host = strtolower($parts['host'] ?? ''); + $parts = (array) parse_url((string) $url); $hosts = [...self::Hosts, strtolower((string) parse_url((string) config('devnull.url'), PHP_URL_HOST))]; - if (! in_array($scheme, ['http', 'https'], true) || $host === '' || ! in_array($host, $hosts, true)) { + if (! in_array(strtolower($parts['host'] ?? ''), $hosts, true)) { return null; } diff --git a/app/NativeComponents/Concerns/DraftsIssues.php b/app/NativeComponents/Concerns/DraftsIssues.php index a4650f73..66e02a98 100644 --- a/app/NativeComponents/Concerns/DraftsIssues.php +++ b/app/NativeComponents/Concerns/DraftsIssues.php @@ -7,6 +7,7 @@ use App\Devnull\DraftImages; use App\Devnull\Exceptions\ApiException; use App\Devnull\IssueCommand; +use App\Devnull\SiteLink; use App\Devnull\Stash; use App\Devnull\Toasts; use Illuminate\Support\Facades\Cache; @@ -169,7 +170,7 @@ public function openIssueFromSheet(): void */ public function viewOpenedIssue(): void { - if ($this->openedIssueUrl !== null) { + if (SiteLink::isWebUrl($this->openedIssueUrl)) { Browser::inApp($this->openedIssueUrl); } } diff --git a/app/NativeComponents/Concerns/ShowsCommits.php b/app/NativeComponents/Concerns/ShowsCommits.php index 12e88225..e6af916c 100644 --- a/app/NativeComponents/Concerns/ShowsCommits.php +++ b/app/NativeComponents/Concerns/ShowsCommits.php @@ -272,7 +272,7 @@ public function openImage(string $id, int $index = 0): void $images = $this->shownCommit($id)?->images ?? []; $image = $images[$index] ?? null; - if ($image === null || ! $this->isWebUrl($image->url)) { + if ($image === null || ! SiteLink::isWebUrl($image->url)) { return; } @@ -588,20 +588,11 @@ private function openUrl(?string $url): void private function openInBrowser(?string $url): void { - if ($this->isWebUrl($url)) { + if (SiteLink::isWebUrl($url)) { Browser::inApp($url); } } - /** - * Only an http or https address is opened, in the browser or the viewer, so - * nothing else a row might carry can take the phone somewhere unexpected. - */ - private function isWebUrl(?string $url): bool - { - return $url !== null && in_array(parse_url($url, PHP_URL_SCHEME), ['http', 'https'], true); - } - /** * Puts the viewer's image back at rest: fresh shared values, which the * phone seeds at one and none, so what the last image's gestures left @@ -623,7 +614,7 @@ private function showInViewedImage(int $index): void { $image = $this->viewerImages[$index] ?? null; - if (! $this->viewerOpen || $image === null || ! $this->isWebUrl($image->url)) { + if (! $this->viewerOpen || $image === null || ! SiteLink::isWebUrl($image->url)) { return; } diff --git a/app/NativeComponents/Concerns/SyncsOutbox.php b/app/NativeComponents/Concerns/SyncsOutbox.php index 03100fe6..1933ce86 100644 --- a/app/NativeComponents/Concerns/SyncsOutbox.php +++ b/app/NativeComponents/Concerns/SyncsOutbox.php @@ -12,6 +12,7 @@ use App\Devnull\Outbox; use App\Devnull\ResponseCache; use App\Devnull\SignedRequest; +use App\Devnull\SiteLink; use App\Devnull\Stash; use App\Devnull\Toasts; use App\Models\PendingWrite; @@ -375,7 +376,7 @@ private function viewIssueFromToast(array $data): void { $url = $data['url'] ?? null; - if (is_string($url) && in_array(parse_url($url, PHP_URL_SCHEME), ['http', 'https'], true)) { + if (is_string($url) && SiteLink::isWebUrl($url)) { Browser::inApp($url); } } diff --git a/app/NativeComponents/Conversation.php b/app/NativeComponents/Conversation.php index 284a684d..1bfa0fb3 100644 --- a/app/NativeComponents/Conversation.php +++ b/app/NativeComponents/Conversation.php @@ -514,7 +514,7 @@ public function openLink(int $id): void return; } - if ($url !== null) { + if (SiteLink::isWebUrl($url)) { Browser::inApp($url); } } diff --git a/app/NativeComponents/Profile.php b/app/NativeComponents/Profile.php index a202c00a..a342bfb8 100644 --- a/app/NativeComponents/Profile.php +++ b/app/NativeComponents/Profile.php @@ -12,6 +12,7 @@ use App\Devnull\Exceptions\ApiException; use App\Devnull\RowCache; use App\Devnull\ShownTabs; +use App\Devnull\SiteLink; use App\Devnull\Stash; use App\Devnull\Timeline; use App\Icons\Lucide; @@ -564,8 +565,10 @@ public function openGitHubMention(string $username): void public function openGitHubProfile(): void { - if ($this->profile !== null) { - Browser::inApp($this->profile->user->gitHubProfileUrl()); + $url = $this->profile?->user->gitHubProfileUrl(); + + if (SiteLink::isWebUrl($url)) { + Browser::inApp($url); } } diff --git a/tests/Feature/Devnull/SiteLinkTest.php b/tests/Feature/Devnull/SiteLinkTest.php index 988dabf8..b50d528c 100644 --- a/tests/Feature/Devnull/SiteLinkTest.php +++ b/tests/Feature/Devnull/SiteLinkTest.php @@ -55,3 +55,25 @@ 'a username with two hyphens in a row' => ['https://dev-null.sh/@a--da'], 'a username longer than GitHub allows' => ['https://dev-null.sh/@'.str_repeat('a', 40)], ]); + +it('takes an http or https address with a host for a web page', function (string $url) { + expect(SiteLink::isWebUrl($url))->toBeTrue(); +})->with([ + 'https' => ['https://nativephp.com/docs?page=1#top'], + 'http' => ['http://example.com'], + 'a scheme in capitals' => ['HTTPS://NATIVEPHP.COM'], +]); + +it('takes nothing else for a web page', function (?string $url) { + expect(SiteLink::isWebUrl($url))->toBeFalse(); +})->with([ + 'nothing' => [null], + 'javascript' => ['javascript:alert(1)'], + 'a file on the phone' => ['file:///etc/passwd'], + "an app's own scheme" => ['devnull://auth/callback'], + 'a phone number' => ['tel:+33100000000'], + 'an email address' => ['mailto:ada@example.com'], + 'no host' => ['https://'], + 'no scheme' => ['nativephp.com'], + 'a user and password before the host' => ['https://nativephp.com@evil.com/'], +]); diff --git a/tests/Feature/Screens/ConversationTest.php b/tests/Feature/Screens/ConversationTest.php index 5f8cd4b2..4e7eef56 100644 --- a/tests/Feature/Screens/ConversationTest.php +++ b/tests/Feature/Screens/ConversationTest.php @@ -460,6 +460,17 @@ function serveRemoteThread(array $thread, ?array $poll = null, int $pushStatus = ->assertNativeCalledTimes('Browser.OpenInApp', 1); }); +it('opens no link preview that is not a web page', function () { + serveRemoteThread(threadJson([ + remoteMessageJson(['id' => 1, 'body' => 'Tap me', 'link_preview' => ['url' => 'javascript:alert(1)', 'title' => 'Tap me']]), + ], ['read_through_id' => 1])); + + finishRefreshes(Native::visit('/remotes/conversations/ada')) + ->tap('link-1') + ->assertNoNavigation() + ->assertNativeNotCalled('Browser.OpenInApp'); +}); + it('pads the end of the thread for the composer bar alone, and for the reply bar while replying', function () { serveRemoteThread(threadJson([remoteMessageJson(['id' => 1])], ['read_through_id' => 1])); diff --git a/tests/Feature/Screens/IssueCommandsTest.php b/tests/Feature/Screens/IssueCommandsTest.php index dc4a69ec..c0299aa4 100644 --- a/tests/Feature/Screens/IssueCommandsTest.php +++ b/tests/Feature/Screens/IssueCommandsTest.php @@ -206,6 +206,14 @@ function hasRef(TestableComponent $screen, string $ref): bool ->and(hasRef($screen, 'sheet-commit'))->toBeTrue(); }); +it('opens no issue link that is not a web page', function () { + $screen = issueSheetOnLog([['number' => 123, 'url' => 'javascript:alert(1)'], 201])->input('sheet-body', '/bug Broken.')->tap('sheet-open-issue'); + + $screen->tap('view-issue') + ->emitNative(ToastActionPressed::class, ['toastId' => 'toast-1', 'actionId' => 'view-issue', 'data' => ['url' => 'file:///etc/passwd']]) + ->assertNativeNotCalled('Browser.OpenInApp'); +}); + it('says so in a toast with View issue when the sheet was swiped away before the issue opened', function () { holdRefreshes(); $screen = issueSheetOnLog()->input('sheet-body', '/feature Dark mode')->tap('sheet-open-issue')->dismissSheet('sheet'); diff --git a/tests/Feature/Screens/ProfileTest.php b/tests/Feature/Screens/ProfileTest.php index 19874f3c..63478cda 100644 --- a/tests/Feature/Screens/ProfileTest.php +++ b/tests/Feature/Screens/ProfileTest.php @@ -111,6 +111,17 @@ function adaCommitJson(array $overrides = []): array ->assertNativeCalled('Browser.OpenInApp', fn (array $params): bool => $params['url'] === 'https://ada.example'); }); +it('opens no GitHub profile link that is not a web page', function () { + fakeApi([ + 'GET /api/v1/users/ada' => [profileJson(user: ['github_url' => 'javascript:alert(1)'])], + 'GET /api/v1/users/ada/commits' => [pageJson([])], + ]); + + Native::visit('/log/users/ada') + ->tap('profile-github') + ->assertNativeNotCalled('Browser.OpenInApp'); +}); + it('opens a website on dev/null here rather than in the browser', function () { fakeApi([ 'GET /api/v1/users/ada' => [profileJson(user: ['website' => 'https://dev-null.sh/@grace'])],