From 424b14d2b44029b8cc2565058c461611ddd0f14e Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 6 Nov 2020 10:27:30 -0500 Subject: [PATCH] Clean up use of subscriptionFavicon --- lib/Database.php | 4 +- lib/REST/TinyTinyRSS/Icon.php | 11 ++++-- tests/cases/Database/SeriesSubscription.php | 41 +++++++++++---------- tests/cases/REST/TinyTinyRSS/TestIcon.php | 24 ++++++------ 4 files changed, 43 insertions(+), 37 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index db4d125e..b852ecc5 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -926,7 +926,7 @@ class Database { */ public function subscriptionIcon(?string $user, int $id, bool $includeData = true): array { $data = $includeData ? "i.data" : "null as data"; - $q = new Query("SELECT i.id, i.url, i.type, $data from arsse_icons as i join arsse_feeds as f on i.id = f.icon join arsse_subscriptions as s on s.feed = f.id"); + $q = new Query("SELECT i.id, i.url, i.type, $data from arsse_subscriptions as s join arsse_feeds as f on s.feed = f.id left join arsse_icons as i on f.icon = i.id"); $q->setWhere("s.id = ?", "int", $id); if (isset($user)) { if (!Arsse::$user->authorize($user, __FUNCTION__)) { @@ -936,7 +936,7 @@ class Database { } $out = $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues())->getRow(); if (!$out) { - throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "subscription", 'id' => $id]); + throw new Db\ExceptionInput("subjectMissing", ["action" => __FUNCTION__, "field" => "subscription", 'id' => $id]); } return $out; } diff --git a/lib/REST/TinyTinyRSS/Icon.php b/lib/REST/TinyTinyRSS/Icon.php index c5c9030e..b49ae4e4 100644 --- a/lib/REST/TinyTinyRSS/Icon.php +++ b/lib/REST/TinyTinyRSS/Icon.php @@ -7,6 +7,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse\REST\TinyTinyRSS; use JKingWeb\Arsse\Arsse; +use JKingWeb\Arsse\Db\ExceptionInput; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; use Laminas\Diactoros\Response\EmptyResponse as Response; @@ -29,14 +30,16 @@ class Icon extends \JKingWeb\Arsse\REST\AbstractHandler { } elseif (!preg_match("<^(\d+)\.ico$>", $req->getRequestTarget(), $match) || !((int) $match[1])) { return new Response(404); } - $url = Arsse::$db->subscriptionFavicon((int) $match[1], Arsse::$user->id ?? null); - if ($url) { - // strip out anything after literal line-end characters; this is to mitigate a potential header (e.g. cookie) injection from the URL + try { + $url = Arsse::$db->subscriptionIcon(Arsse::$user->id ?? null, (int) $match[1], false)['url']; + if (!$url) { + return new Response(404); + } if (($pos = strpos($url, "\r")) !== false || ($pos = strpos($url, "\n")) !== false) { $url = substr($url, 0, $pos); } return new Response(301, ['Location' => $url]); - } else { + } catch (ExceptionInput $e) { return new Response(404); } } diff --git a/tests/cases/Database/SeriesSubscription.php b/tests/cases/Database/SeriesSubscription.php index 427a9843..0075b992 100644 --- a/tests/cases/Database/SeriesSubscription.php +++ b/tests/cases/Database/SeriesSubscription.php @@ -462,32 +462,35 @@ trait SeriesSubscription { public function testRetrieveTheFaviconOfASubscription(): void { $exp = "http://example.com/favicon.ico"; - $this->assertSame($exp, Arsse::$db->subscriptionFavicon(1)); - $this->assertSame($exp, Arsse::$db->subscriptionFavicon(2)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(3)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(4)); + $this->assertSame($exp, Arsse::$db->subscriptionIcon(null, 1)['url']); + $this->assertSame($exp, Arsse::$db->subscriptionIcon(null, 2)['url']); + $this->assertSame(null, Arsse::$db->subscriptionIcon(null, 3)['url']); // authorization shouldn't have any bearing on this function \Phake::when(Arsse::$user)->authorize->thenReturn(false); - $this->assertSame($exp, Arsse::$db->subscriptionFavicon(1)); - $this->assertSame($exp, Arsse::$db->subscriptionFavicon(2)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(3)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(4)); - // invalid IDs should simply return an empty string - $this->assertSame('', Arsse::$db->subscriptionFavicon(-2112)); + $this->assertSame($exp, Arsse::$db->subscriptionIcon(null, 1)['url']); + $this->assertSame($exp, Arsse::$db->subscriptionIcon(null, 2)['url']); + $this->assertSame(null, Arsse::$db->subscriptionIcon(null, 3)['url']); + } + + public function testRetrieveTheFaviconOfAMissingSubscription(): void { + $this->assertException("subjectMissing", "Db", "ExceptionInput"); + Arsse::$db->subscriptionIcon(null, -2112); } public function testRetrieveTheFaviconOfASubscriptionWithUser(): void { $exp = "http://example.com/favicon.ico"; $user = "john.doe@example.com"; - $this->assertSame($exp, Arsse::$db->subscriptionFavicon(1, $user)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(2, $user)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(3, $user)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(4, $user)); + $this->assertSame($exp, Arsse::$db->subscriptionIcon($user, 1)['url']); + $this->assertSame(null, Arsse::$db->subscriptionIcon($user, 3)['url']); $user = "jane.doe@example.com"; - $this->assertSame('', Arsse::$db->subscriptionFavicon(1, $user)); - $this->assertSame($exp, Arsse::$db->subscriptionFavicon(2, $user)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(3, $user)); - $this->assertSame('', Arsse::$db->subscriptionFavicon(4, $user)); + $this->assertSame($exp, Arsse::$db->subscriptionIcon($user, 2)['url']); + } + + public function testRetrieveTheFaviconOfASubscriptionOfTheWrongUser(): void { + $exp = "http://example.com/favicon.ico"; + $user = "john.doe@example.com"; + $this->assertException("subjectMissing", "Db", "ExceptionInput"); + $this->assertSame(null, Arsse::$db->subscriptionIcon($user, 2)['url']); } public function testRetrieveTheFaviconOfASubscriptionWithUserWithoutAuthority(): void { @@ -495,7 +498,7 @@ trait SeriesSubscription { $user = "john.doe@example.com"; \Phake::when(Arsse::$user)->authorize->thenReturn(false); $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->subscriptionFavicon(-2112, $user); + Arsse::$db->subscriptionIcon($user, -2112); } public function testListTheTagsOfASubscription(): void { diff --git a/tests/cases/REST/TinyTinyRSS/TestIcon.php b/tests/cases/REST/TinyTinyRSS/TestIcon.php index 38dbd8fe..5341238f 100644 --- a/tests/cases/REST/TinyTinyRSS/TestIcon.php +++ b/tests/cases/REST/TinyTinyRSS/TestIcon.php @@ -48,10 +48,10 @@ class TestIcon extends \JKingWeb\Arsse\Test\AbstractTest { } public function testRetrieveFavion(): void { - \Phake::when(Arsse::$db)->subscriptionFavicon->thenReturn(""); - \Phake::when(Arsse::$db)->subscriptionFavicon(42, $this->anything())->thenReturn("http://example.com/favicon.ico"); - \Phake::when(Arsse::$db)->subscriptionFavicon(2112, $this->anything())->thenReturn("http://example.net/logo.png"); - \Phake::when(Arsse::$db)->subscriptionFavicon(1337, $this->anything())->thenReturn("http://example.org/icon.gif\r\nLocation: http://bad.example.com/"); + \Phake::when(Arsse::$db)->subscriptionIcon->thenReturn(['url' => null]); + \Phake::when(Arsse::$db)->subscriptionIcon($this->anything(), 42, false)->thenReturn(['url' => "http://example.com/favicon.ico"]); + \Phake::when(Arsse::$db)->subscriptionIcon($this->anything(), 2112, false)->thenReturn(['url' => "http://example.net/logo.png"]); + \Phake::when(Arsse::$db)->subscriptionIcon($this->anything(), 1337, false)->thenReturn(['url' => "http://example.org/icon.gif\r\nLocation: http://bad.example.com/"]); // these requests should succeed $exp = new Response(301, ['Location' => "http://example.com/favicon.ico"]); $this->assertMessage($exp, $this->req("42.ico")); @@ -71,14 +71,14 @@ class TestIcon extends \JKingWeb\Arsse\Test\AbstractTest { } public function testRetrieveFavionWithHttpAuthentication(): void { - $url = "http://example.org/icon.gif\r\nLocation: http://bad.example.com/"; - \Phake::when(Arsse::$db)->subscriptionFavicon->thenReturn(""); - \Phake::when(Arsse::$db)->subscriptionFavicon(42, $this->user)->thenReturn($url); - \Phake::when(Arsse::$db)->subscriptionFavicon(2112, "jane.doe")->thenReturn($url); - \Phake::when(Arsse::$db)->subscriptionFavicon(1337, $this->user)->thenReturn($url); - \Phake::when(Arsse::$db)->subscriptionFavicon(42, null)->thenReturn($url); - \Phake::when(Arsse::$db)->subscriptionFavicon(2112, null)->thenReturn($url); - \Phake::when(Arsse::$db)->subscriptionFavicon(1337, null)->thenReturn($url); + $url = ['url' => "http://example.org/icon.gif\r\nLocation: http://bad.example.com/"]; + \Phake::when(Arsse::$db)->subscriptionIcon->thenReturn(['url' => null]); + \Phake::when(Arsse::$db)->subscriptionIcon($this->user, 42, false)->thenReturn($url); + \Phake::when(Arsse::$db)->subscriptionIcon("jane.doe", 2112, false)->thenReturn($url); + \Phake::when(Arsse::$db)->subscriptionIcon($this->user, 1337, false)->thenReturn($url); + \Phake::when(Arsse::$db)->subscriptionIcon(null, 42, false)->thenReturn($url); + \Phake::when(Arsse::$db)->subscriptionIcon(null, 2112, false)->thenReturn($url); + \Phake::when(Arsse::$db)->subscriptionIcon(null, 1337, false)->thenReturn($url); // these requests should succeed $exp = new Response(301, ['Location' => "http://example.org/icon.gif"]); $this->assertMessage($exp, $this->req("42.ico"));