Skip to content

Commit 6f63b38

Browse files
SeldaekSeldaek
committed
Fixes
1 parent 4be1ce8 commit 6f63b38

9 files changed

Lines changed: 261 additions & 7 deletions

File tree

‎src/Controller/PackageController.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -672,15 +672,21 @@ public function viewPackageAction(Request $req, string $name, CsrfTokenManagerIn
672672
// so a package that fails this check can never need the counter again.
673673
if (
674674
!$package->isSuspect()
675-
&& $data['downloads']['total'] <= 10
676-
&& $package->getCreatedAt()->getTimestamp() >= strtotime('2019-05-01')
675+
&& $data['downloads']['total'] <= PackageRepository::SUSPECT_VIEWS_MAX_DOWNLOADS
676+
&& $package->getCreatedAt()->getTimestamp() >= strtotime(PackageRepository::SUSPECT_VIEWS_MIN_CREATED_AT)
677677
&& $this->downloadManager->incrementViews($package) >= 100
678678
) {
679679
$vendorRepo = $this->getEM()->getRepository(Vendor::class);
680680
if (!$vendorRepo->isVerified($package->getVendor())) {
681681
$package->setSuspect('Too many views');
682682
$repo->markPackageSuspect($package);
683683
}
684+
685+
// The counter has served its purpose either way, so drop it: a package we just
686+
// marked suspect stops counting above, and for a verified vendor nothing can ever
687+
// come of it. Restarting from zero also spaces the isVerified() lookup back out to
688+
// once per 100 views instead of once per view from here on.
689+
$this->downloadManager->deleteViews($package->getId());
684690
}
685691

686692
if ($user) {

‎src/Entity/PackageRepository.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,14 @@
2828
*/
2929
class PackageRepository extends ServiceEntityRepository
3030
{
31+
/**
32+
* Bounds of the "lots of views, no installs" spam heuristic implemented in
33+
* PackageController::viewPackageAction(), shared so packagist:clean-view-counters can tell which
34+
* view counters are still live by exactly the same rules.
35+
*/
36+
public const SUSPECT_VIEWS_MIN_CREATED_AT = '2019-05-01';
37+
public const SUSPECT_VIEWS_MAX_DOWNLOADS = 10;
38+
3139
private const LISTING_FIELDS = 'id, name, description, type, gitHubStars, frozen, language, abandoned, replacementPackage';
3240
// @phpstan-ignore classConstant.unused
3341
private const LISTING_WITH_AUTO_UPDATE_WARNINGS_FIELDS = 'id, name, description, type, gitHubStars, frozen, language, abandoned, replacementPackage, autoUpdated, repository';
@@ -564,6 +572,41 @@ public function getAllSuspectPackages(): array
564572
return $this->getEntityManager()->getConnection()->fetchAllAssociative($sql);
565573
}
566574

575+
/**
576+
* Narrows a list of package ids down to those the "too many views" heuristic could still flag:
577+
* still present, not already suspect, still publicly viewable, new enough, and not owned by a
578+
* vendor a moderator has verified (which blocks the flagging for good). The download half of
579+
* the check lives in Redis, so callers have to apply SUSPECT_VIEWS_MAX_DOWNLOADS themselves.
580+
*
581+
* @param list<int> $ids
582+
*
583+
* @return list<int>
584+
*/
585+
public function getPackageIdsFlaggableByViews(array $ids): array
586+
{
587+
if (\count($ids) === 0) {
588+
return [];
589+
}
590+
591+
// a suppressing freeze 404s the package page for everyone, so no views can come in anymore,
592+
// while a gentle freeze keeps serving it and thus keeps the counter live
593+
$sql = 'SELECT p.id FROM package p
594+
LEFT JOIN vendor v ON v.name = p.vendor
595+
WHERE p.id IN (:ids)
596+
AND p.suspect IS NULL
597+
AND (p.frozen IS NULL OR p.frozen NOT IN (:suppressed))
598+
AND p.createdAt >= :minCreatedAt
599+
AND COALESCE(v.verified, 0) = 0';
600+
601+
$rows = $this->getEntityManager()->getConnection()->fetchFirstColumn(
602+
$sql,
603+
['ids' => $ids, 'suppressed' => PackageFreezeReason::suppressingValues(), 'minCreatedAt' => self::SUSPECT_VIEWS_MIN_CREATED_AT],
604+
['ids' => ArrayParameterType::INTEGER, 'suppressed' => ArrayParameterType::STRING]
605+
);
606+
607+
return array_map('intval', $rows);
608+
}
609+
567610
/**
568611
* @param list<int> $ids
569612
*

‎src/Entity/VendorRepository.php‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
namespace App\Entity;
1414

15+
use App\Model\DownloadManager;
1516
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
1617
use Doctrine\Persistence\ManagerRegistry;
1718

@@ -22,7 +23,7 @@
2223
*/
2324
class VendorRepository extends ServiceEntityRepository
2425
{
25-
public function __construct(ManagerRegistry $registry)
26+
public function __construct(ManagerRegistry $registry, private readonly DownloadManager $downloadManager)
2627
{
2728
parent::__construct($registry, Vendor::class);
2829
}
@@ -50,5 +51,13 @@ public function verify(string $vendor): void
5051
'UPDATE package SET suspect = NULL WHERE vendor = :vendor',
5152
['vendor' => $vendor]
5253
);
54+
55+
// A verified vendor's packages can never be flagged again, so their view counters have
56+
// lost their only reader - drop them instead of leaving them in Redis forever.
57+
$packageIds = $this->getEntityManager()->getConnection()->fetchFirstColumn(
58+
'SELECT id FROM package WHERE vendor = :vendor',
59+
['vendor' => $vendor]
60+
);
61+
$this->downloadManager->deleteViews(...array_map('intval', $packageIds));
5362
}
5463
}

‎src/Model/DownloadManager.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use Doctrine\DBAL\ArrayParameterType;
2121
use Doctrine\Persistence\ManagerRegistry;
2222
use Predis\Client;
23+
use Predis\PredisException;
2324

2425
/**
2526
* Manages the download counts for packages.
@@ -111,6 +112,26 @@ public function incrementViews(Package|int $package): int
111112
return $this->redis->incr('views:'.$package);
112113
}
113114

115+
/**
116+
* Drops the view counters of the given packages.
117+
*
118+
* Safe to call as soon as the spam heuristic can no longer fire for a package: nothing else
119+
* reads the counter, so from that point on the key is only taking up space in Redis. Losing a
120+
* counter costs nothing either, hence the swallowed Redis failure - callers are mid-way through
121+
* more important work (verifying a vendor, deleting a package) and must not fail over this.
122+
*/
123+
public function deleteViews(int ...$packageIds): void
124+
{
125+
if (\count($packageIds) === 0) {
126+
return;
127+
}
128+
129+
try {
130+
$this->redis->del(array_map(static fn (int $id) => 'views:'.$id, $packageIds));
131+
} catch (PredisException) {
132+
}
133+
}
134+
114135
/**
115136
* Gets the total download count for a package.
116137
*/

‎src/Model/PackageManager.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ public function __construct(
6161
private readonly CdnClient $cdnClient,
6262
private readonly UrlGeneratorInterface $urlGenerator,
6363
private readonly Scheduler $scheduler,
64+
private readonly DownloadManager $downloadManager,
6465
) {
6566
}
6667

@@ -80,6 +81,9 @@ public function freeze(Package $package, PackageFreezeReason $reason, ?int $acto
8081

8182
if ($reason->suppressesPackage()) {
8283
$this->scheduler->schedulePackagePurge($package, $actorId);
84+
// the page 404s from here on, so the spam heuristic's view counter has no traffic left
85+
// to read - and unfreezing legitimately starts the count over
86+
$this->downloadManager->deleteViews($package->getId());
8387
}
8488
}
8589

@@ -156,10 +160,7 @@ public function deletePackage(Package $package, ?string $reason = null, ?string
156160
$this->deletePackageMetadata($packageName);
157161

158162
// delete redis stats
159-
try {
160-
$this->redis->del('views:'.$packageId);
161-
} catch (\Predis\Connection\ConnectionException $e) {
162-
}
163+
$this->downloadManager->deleteViews($packageId);
163164

164165
// attempt search index cleanup
165166
$this->deletePackageSearchIndex($packageName);

‎tests/Controller/PackageControllerTest.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use App\Entity\PackageFreezeReason;
2121
use App\Entity\PackageReadme;
2222
use App\Entity\User;
23+
use App\Entity\Vendor;
2324
use App\Entity\Version;
2425
use App\Service\Spam\FeatureExtractor;
2526
use App\Service\Spam\SpamClassifier;
@@ -73,6 +74,52 @@ public function testPackagePageOnlyCountsViewsWhileTheSpamHeuristicCanUseThem():
7374
$redis->del(['views:'.$fresh->getId(), 'dl:'.$established->getId()]);
7475
}
7576

77+
public function testPackagePageDropsTheViewCounterOnceTheHeuristicHasFired(): void
78+
{
79+
$package = self::createPackage('test/spammy', 'https://example.com/test/spammy');
80+
$this->store($package);
81+
82+
$redis = $this->redis();
83+
$redis->set('views:'.$package->getId(), '99');
84+
85+
$this->client->request('GET', '/packages/test/spammy');
86+
self::assertResponseIsSuccessful();
87+
88+
$em = self::getEM();
89+
$em->clear();
90+
$reloaded = $em->find(Package::class, $package->getId());
91+
self::assertNotNull($reloaded);
92+
self::assertSame('Too many views', $reloaded->getSuspect());
93+
self::assertNull(
94+
$redis->get('views:'.$package->getId()),
95+
'the counter has done its job, keeping it would only grow a key nothing reads',
96+
);
97+
}
98+
99+
public function testPackagePageDropsTheViewCounterOfAVerifiedVendor(): void
100+
{
101+
$vendor = new Vendor('verifiedvendor');
102+
$vendor->setVerified(true);
103+
$package = self::createPackage('verifiedvendor/pkg', 'https://example.com/verifiedvendor/pkg');
104+
$this->store($vendor, $package);
105+
106+
$redis = $this->redis();
107+
$redis->set('views:'.$package->getId(), '99');
108+
109+
$this->client->request('GET', '/packages/verifiedvendor/pkg');
110+
self::assertResponseIsSuccessful();
111+
112+
$em = self::getEM();
113+
$em->clear();
114+
$reloaded = $em->find(Package::class, $package->getId());
115+
self::assertNotNull($reloaded);
116+
self::assertFalse($reloaded->isSuspect(), 'a verified vendor is never flagged');
117+
self::assertNull(
118+
$redis->get('views:'.$package->getId()),
119+
'nothing can ever come of this counter, and resetting it stops the isVerified() lookup running on every view',
120+
);
121+
}
122+
76123
private function redis(): Client
77124
{
78125
$client = static::getContainer()->get('snc_redis.default');

‎tests/Entity/PackageRepositoryTest.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use App\Entity\Package;
1616
use App\Entity\PackageFreezeReason;
1717
use App\Entity\PackageRepository;
18+
use App\Entity\Vendor;
1819
use App\Tests\IntegrationTestCase;
1920

2021
class PackageRepositoryTest extends IntegrationTestCase
@@ -197,4 +198,33 @@ public function testGetFilteredQueryBuilderExcludesSuppressedByDefault(): void
197198
self::assertContains('vendor/spam', $withFrozen);
198199
self::assertContains('vendor/malware', $withFrozen);
199200
}
201+
202+
public function testGetPackageIdsFlaggableByViewsKeepsOnlyPackagesTheHeuristicCanStillReach(): void
203+
{
204+
$live = self::createPackage('vendor/live', 'https://example.org/live');
205+
$suspect = self::createPackage('vendor/suspect', 'https://example.org/suspect');
206+
$suspect->setSuspect('Too many views');
207+
$old = self::createPackage('vendor/old', 'https://example.org/old');
208+
$old->setCreatedAt(new \DateTimeImmutable('2018-01-01'));
209+
$spam = self::createPackage('vendor/spam', 'https://example.org/spam');
210+
$spam->freeze(PackageFreezeReason::Spam);
211+
$temporary = self::createPackage('vendor/temporary', 'https://example.org/temporary');
212+
$temporary->freeze(PackageFreezeReason::Temporary);
213+
$verified = self::createPackage('verifiedvendor/pkg', 'https://example.org/verifiedvendor/pkg');
214+
$vendor = new Vendor('verifiedvendor');
215+
$vendor->setVerified(true);
216+
$this->store($live, $suspect, $old, $spam, $temporary, $verified, $vendor);
217+
218+
$ids = $this->packageRepository->getPackageIdsFlaggableByViews([
219+
$live->getId(), $suspect->getId(), $old->getId(), $spam->getId(), $temporary->getId(), $verified->getId(), 999999999,
220+
]);
221+
222+
self::assertContains($live->getId(), $ids);
223+
self::assertContains($temporary->getId(), $ids, 'a gentle freeze keeps serving the page, so views still come in');
224+
self::assertNotContains($suspect->getId(), $ids);
225+
self::assertNotContains($old->getId(), $ids);
226+
self::assertNotContains($spam->getId(), $ids, 'a suppressing freeze 404s the page, so no more views can arrive');
227+
self::assertNotContains($verified->getId(), $ids, 'a verified vendor can never be flagged again');
228+
self::assertNotContains(999999999, $ids, 'a deleted package cannot be flagged either');
229+
}
200230
}

‎tests/Entity/VendorRepositoryTest.php‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
<?php declare(strict_types=1);
2+
3+
/*
4+
* This file is part of Packagist.
5+
*
6+
* (c) Jordi Boggiano <j.boggiano@seld.be>
7+
* Nils Adermann <naderman@naderman.de>
8+
*
9+
* For the full copyright and license information, please view the LICENSE
10+
* file that was distributed with this source code.
11+
*/
12+
13+
namespace App\Tests\Entity;
14+
15+
use App\Entity\Package;
16+
use App\Entity\Vendor;
17+
use App\Entity\VendorRepository;
18+
use App\Tests\IntegrationTestCase;
19+
use Predis\Client;
20+
21+
class VendorRepositoryTest extends IntegrationTestCase
22+
{
23+
public function testVerifyClearsSuspectAndDropsTheVendorsViewCounters(): void
24+
{
25+
$suspect = self::createPackage('spamvendor/one', 'https://example.org/spamvendor/one');
26+
$suspect->setSuspect('Too many views');
27+
$innocent = self::createPackage('spamvendor/two', 'https://example.org/spamvendor/two');
28+
$unrelated = self::createPackage('othervendor/pkg', 'https://example.org/othervendor/pkg');
29+
$this->store($suspect, $innocent, $unrelated);
30+
31+
$redis = $this->redis();
32+
$redis->mset([
33+
'views:'.$suspect->getId() => '120',
34+
'views:'.$innocent->getId() => '7',
35+
'views:'.$unrelated->getId() => '3',
36+
]);
37+
38+
$this->vendorRepo()->verify('spamvendor');
39+
40+
self::assertNull($redis->get('views:'.$suspect->getId()));
41+
self::assertNull(
42+
$redis->get('views:'.$innocent->getId()),
43+
'a verified vendor is whitelisted wholesale, so none of its packages can be flagged again',
44+
);
45+
self::assertSame('3', $redis->get('views:'.$unrelated->getId()), 'other vendors must be left alone');
46+
47+
$em = self::getEM();
48+
$em->clear();
49+
$reloaded = $em->find(Package::class, $suspect->getId());
50+
self::assertNotNull($reloaded);
51+
self::assertFalse($reloaded->isSuspect());
52+
self::assertTrue($this->vendorRepo()->isVerified('spamvendor'));
53+
54+
$redis->del(['views:'.$unrelated->getId()]);
55+
}
56+
57+
private function vendorRepo(): VendorRepository
58+
{
59+
$repo = self::getEM()->getRepository(Vendor::class);
60+
self::assertInstanceOf(VendorRepository::class, $repo);
61+
62+
return $repo;
63+
}
64+
65+
private function redis(): Client
66+
{
67+
$client = static::getContainer()->get('snc_redis.default');
68+
self::assertInstanceOf(Client::class, $client);
69+
70+
return $client;
71+
}
72+
}

‎tests/Model/PackageManagerTest.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,12 @@
1515
use App\Audit\AuditRecordType;
1616
use App\Entity\AuditRecord;
1717
use App\Entity\Package;
18+
use App\Entity\PackageFreezeReason;
1819
use App\Entity\User;
1920
use App\Model\PackageManager;
2021
use App\Tests\IntegrationTestCase;
2122
use PHPUnit\Framework\Attributes\TestWith;
23+
use Predis\Client;
2224

2325
class PackageManagerTest extends IntegrationTestCase
2426
{
@@ -130,4 +132,27 @@ private function assertAuditLogWasCreated(Package $package, array $oldMaintainer
130132
$this->assertEqualsCanonicalizing($oldMaintainers, array_map($callable, $record->attributes['previous_maintainers']));
131133
$this->assertEqualsCanonicalizing($newMaintainers, array_map($callable, $record->attributes['current_maintainers']));
132134
}
135+
136+
#[TestWith([PackageFreezeReason::Spam, true])]
137+
#[TestWith([PackageFreezeReason::Temporary, false])]
138+
public function testFreezeDropsTheViewCounterOnlyWhenThePageStopsBeingServed(PackageFreezeReason $reason, bool $expectDropped): void
139+
{
140+
$package = self::createPackage('test/frozen', 'https://example.org/test/frozen');
141+
$this->store($package);
142+
143+
$redis = self::getContainer()->get('snc_redis.default');
144+
self::assertInstanceOf(Client::class, $redis);
145+
$key = 'views:'.$package->getId();
146+
$redis->set($key, '42');
147+
148+
$this->packageManager->freeze($package, $reason);
149+
150+
if ($expectDropped) {
151+
self::assertNull($redis->get($key), 'a suppressed package 404s, so nothing can read the counter anymore');
152+
} else {
153+
self::assertSame('42', $redis->get($key), 'a gentle freeze keeps serving the page, so the counter stays live');
154+
}
155+
156+
$redis->del([$key]);
157+
}
133158
}

0 commit comments

Comments
 (0)