Merge pull request #3823 from wallabag/fix-tag-api-leak

Fix tag API leak
This commit is contained in:
Jérémy Benoist 2019-01-03 09:14:26 +01:00 committed by GitHub
commit 2378fd6347
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 204 additions and 137 deletions

View file

@ -46,12 +46,14 @@ class TagRestController extends WallabagRestController
$this->validateAuthentication(); $this->validateAuthentication();
$label = $request->get('tag', ''); $label = $request->get('tag', '');
$tag = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findOneByLabel($label); $tags = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findByLabelsAndUser([$label], $this->getUser()->getId());
if (empty($tag)) { if (empty($tags)) {
throw $this->createNotFoundException('Tag not found'); throw $this->createNotFoundException('Tag not found');
} }
$tag = $tags[0];
$this->getDoctrine() $this->getDoctrine()
->getRepository('WallabagCoreBundle:Entry') ->getRepository('WallabagCoreBundle:Entry')
->removeTag($this->getUser()->getId(), $tag); ->removeTag($this->getUser()->getId(), $tag);
@ -80,15 +82,7 @@ class TagRestController extends WallabagRestController
$tagsLabels = $request->get('tags', ''); $tagsLabels = $request->get('tags', '');
$tags = []; $tags = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findByLabelsAndUser(explode(',', $tagsLabels), $this->getUser()->getId());
foreach (explode(',', $tagsLabels) as $tagLabel) {
$tagEntity = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findOneByLabel($tagLabel);
if (!empty($tagEntity)) {
$tags[] = $tagEntity;
}
}
if (empty($tags)) { if (empty($tags)) {
throw $this->createNotFoundException('Tags not found'); throw $this->createNotFoundException('Tags not found');
@ -120,6 +114,12 @@ class TagRestController extends WallabagRestController
{ {
$this->validateAuthentication(); $this->validateAuthentication();
$tagFromDb = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findByLabelsAndUser([$tag->getLabel()], $this->getUser()->getId());
if (empty($tagFromDb)) {
throw $this->createNotFoundException('Tag not found');
}
$this->getDoctrine() $this->getDoctrine()
->getRepository('WallabagCoreBundle:Entry') ->getRepository('WallabagCoreBundle:Entry')
->removeTag($this->getUser()->getId(), $tag); ->removeTag($this->getUser()->getId(), $tag);

View file

@ -14,97 +14,112 @@ class LoadEntryData extends AbstractFixture implements OrderedFixtureInterface
*/ */
public function load(ObjectManager $manager) public function load(ObjectManager $manager)
{ {
$entry1 = new Entry($this->getReference('admin-user')); $entries = [
$entry1->setUrl('http://0.0.0.0/entry1'); 'entry1' => [
$entry1->setReadingTime(11); 'user' => 'admin-user',
$entry1->setDomainName('domain.io'); 'url' => 'http://0.0.0.0/entry1',
$entry1->setMimetype('text/html'); 'reading_time' => 11,
$entry1->setTitle('test title entry1'); 'domain' => 'domain.io',
$entry1->setContent('This is my content /o/'); 'mime' => 'text/html',
$entry1->setLanguage('en'); 'title' => 'test title entry1',
'content' => 'This is my content /o/',
'language' => 'en',
'tags' => ['foo-tag', 'baz-tag'],
],
'entry2' => [
'user' => 'admin-user',
'url' => 'http://0.0.0.0/entry2',
'reading_time' => 1,
'domain' => 'domain.io',
'mime' => 'text/html',
'title' => 'test title entry2',
'content' => 'This is my content /o/',
'origin' => 'ftp://oneftp.tld',
'language' => 'fr',
],
'entry3' => [
'user' => 'bob-user',
'url' => 'http://0.0.0.0/entry3',
'reading_time' => 1,
'domain' => 'domain.io',
'mime' => 'text/html',
'title' => 'test title entry3',
'content' => 'This is my content /o/',
'language' => 'en',
'tags' => ['foo-tag', 'bar-tag', 'bob-tag'],
],
'entry4' => [
'user' => 'admin-user',
'url' => 'http://0.0.0.0/entry4',
'reading_time' => 12,
'domain' => 'domain.io',
'mime' => 'text/html',
'title' => 'test title entry4',
'content' => 'This is my content /o/',
'language' => 'en',
'tags' => ['foo-tag', 'bar-tag'],
],
'entry5' => [
'user' => 'admin-user',
'url' => 'http://0.0.0.0/entry5',
'reading_time' => 12,
'domain' => 'domain.io',
'mime' => 'text/html',
'title' => 'test title entry5',
'content' => 'This is my content /o/',
'language' => 'fr',
'starred' => true,
'preview' => 'http://0.0.0.0/image.jpg',
],
'entry6' => [
'user' => 'admin-user',
'url' => 'http://0.0.0.0/entry6',
'reading_time' => 12,
'domain' => 'domain.io',
'mime' => 'text/html',
'title' => 'test title entry6',
'content' => 'This is my content /o/',
'language' => 'de',
'archived' => true,
'tags' => ['bar-tag'],
],
];
$entry1->addTag($this->getReference('foo-tag')); foreach ($entries as $reference => $item) {
$entry1->addTag($this->getReference('baz-tag')); $entry = new Entry($this->getReference($item['user']));
$entry->setUrl($item['url']);
$entry->setReadingTime($item['reading_time']);
$entry->setDomainName($item['domain']);
$entry->setMimetype($item['mime']);
$entry->setTitle($item['title']);
$entry->setContent($item['content']);
$entry->setLanguage($item['language']);
$manager->persist($entry1); if (isset($item['tags'])) {
foreach ($item['tags'] as $tag) {
$entry->addTag($this->getReference($tag));
}
}
$this->addReference('entry1', $entry1); if (isset($item['origin'])) {
$entry->setOriginUrl($item['origin']);
}
$entry2 = new Entry($this->getReference('admin-user')); if (isset($item['starred'])) {
$entry2->setUrl('http://0.0.0.0/entry2'); $entry->setStarred($item['starred']);
$entry2->setReadingTime(1); }
$entry2->setDomainName('domain.io');
$entry2->setMimetype('text/html');
$entry2->setTitle('test title entry2');
$entry2->setContent('This is my content /o/');
$entry2->setOriginUrl('ftp://oneftp.tld');
$entry2->setLanguage('fr');
$manager->persist($entry2); if (isset($item['archived'])) {
$entry->setArchived($item['archived']);
}
$this->addReference('entry2', $entry2); if (isset($item['preview'])) {
$entry->setPreviewPicture($item['preview']);
}
$entry3 = new Entry($this->getReference('bob-user')); $manager->persist($entry);
$entry3->setUrl('http://0.0.0.0/entry3'); $this->addReference($reference, $entry);
$entry3->setReadingTime(1); }
$entry3->setDomainName('domain.io');
$entry3->setMimetype('text/html');
$entry3->setTitle('test title entry3');
$entry3->setContent('This is my content /o/');
$entry3->setLanguage('en');
$entry3->addTag($this->getReference('foo-tag'));
$entry3->addTag($this->getReference('bar-tag'));
$manager->persist($entry3);
$this->addReference('entry3', $entry3);
$entry4 = new Entry($this->getReference('admin-user'));
$entry4->setUrl('http://0.0.0.0/entry4');
$entry4->setReadingTime(12);
$entry4->setDomainName('domain.io');
$entry4->setMimetype('text/html');
$entry4->setTitle('test title entry4');
$entry4->setContent('This is my content /o/');
$entry4->setLanguage('en');
$entry4->addTag($this->getReference('foo-tag'));
$entry4->addTag($this->getReference('bar-tag'));
$manager->persist($entry4);
$this->addReference('entry4', $entry4);
$entry5 = new Entry($this->getReference('admin-user'));
$entry5->setUrl('http://0.0.0.0/entry5');
$entry5->setReadingTime(12);
$entry5->setDomainName('domain.io');
$entry5->setMimetype('text/html');
$entry5->setTitle('test title entry5');
$entry5->setContent('This is my content /o/');
$entry5->setStarred(true);
$entry5->setLanguage('fr');
$entry5->setPreviewPicture('http://0.0.0.0/image.jpg');
$manager->persist($entry5);
$this->addReference('entry5', $entry5);
$entry6 = new Entry($this->getReference('admin-user'));
$entry6->setUrl('http://0.0.0.0/entry6');
$entry6->setReadingTime(12);
$entry6->setDomainName('domain.io');
$entry6->setMimetype('text/html');
$entry6->setTitle('test title entry6');
$entry6->setContent('This is my content /o/');
$entry6->setArchived(true);
$entry6->setLanguage('de');
$entry6->addTag($this->getReference('bar-tag'));
$manager->persist($entry6);
$this->addReference('entry6', $entry6);
$manager->flush(); $manager->flush();
} }

View file

@ -14,33 +14,22 @@ class LoadTagData extends AbstractFixture implements OrderedFixtureInterface
*/ */
public function load(ObjectManager $manager) public function load(ObjectManager $manager)
{ {
$tag1 = new Tag(); $tags = [
$tag1->setLabel('foo bar'); 'foo-bar-tag' => 'foo bar', //tag used for EntryControllerTest
'bar-tag' => 'bar',
'baz-tag' => 'baz', // tag used for ExportControllerTest
'foo-tag' => 'foo',
'bob-tag' => 'bob', // tag used for TagRestControllerTest
];
$manager->persist($tag1); foreach ($tags as $reference => $label) {
$tag = new Tag();
$tag->setLabel($label);
$this->addReference('foo-bar-tag', $tag1); $manager->persist($tag);
$tag2 = new Tag(); $this->addReference($reference, $tag);
$tag2->setLabel('bar'); }
$manager->persist($tag2);
$this->addReference('bar-tag', $tag2);
$tag3 = new Tag();
$tag3->setLabel('baz');
$manager->persist($tag3);
$this->addReference('baz-tag', $tag3);
$tag4 = new Tag();
$tag4->setLabel('foo');
$manager->persist($tag4);
$this->addReference('foo-tag', $tag4);
$manager->flush(); $manager->flush();
} }

View file

@ -3,6 +3,7 @@
namespace Wallabag\CoreBundle\Repository; namespace Wallabag\CoreBundle\Repository;
use Doctrine\ORM\EntityRepository; use Doctrine\ORM\EntityRepository;
use Doctrine\ORM\QueryBuilder;
use Wallabag\CoreBundle\Entity\Tag; use Wallabag\CoreBundle\Entity\Tag;
class TagRepository extends EntityRepository class TagRepository extends EntityRepository
@ -45,12 +46,8 @@ class TagRepository extends EntityRepository
*/ */
public function findAllTags($userId) public function findAllTags($userId)
{ {
$ids = $this->createQueryBuilder('t') $ids = $this->getQueryBuilderByUser($userId)
->select('t.id') ->select('t.id')
->leftJoin('t.entries', 'e')
->where('e.user = :userId')->setParameter('userId', $userId)
->groupBy('t.id')
->orderBy('t.slug')
->getQuery() ->getQuery()
->getArrayResult(); ->getArrayResult();
@ -71,18 +68,30 @@ class TagRepository extends EntityRepository
*/ */
public function findAllFlatTagsWithNbEntries($userId) public function findAllFlatTagsWithNbEntries($userId)
{ {
return $this->createQueryBuilder('t') return $this->getQueryBuilderByUser($userId)
->select('t.id, t.label, t.slug, count(e.id) as nbEntries') ->select('t.id, t.label, t.slug, count(e.id) as nbEntries')
->distinct(true) ->distinct(true)
->leftJoin('t.entries', 'e')
->where('e.user = :userId')
->groupBy('t.id')
->orderBy('t.slug')
->setParameter('userId', $userId)
->getQuery() ->getQuery()
->getArrayResult(); ->getArrayResult();
} }
public function findByLabelsAndUser($labels, $userId)
{
$qb = $this->getQueryBuilderByUser($userId)
->select('t.id');
$ids = $qb->andWhere($qb->expr()->in('t.label', $labels))
->getQuery()
->getArrayResult();
$tags = [];
foreach ($ids as $id) {
$tags[] = $this->find($id);
}
return $tags;
}
/** /**
* Used only in test case to get a tag for our entry. * Used only in test case to get a tag for our entry.
* *
@ -101,13 +110,9 @@ class TagRepository extends EntityRepository
public function findForArchivedArticlesByUser($userId) public function findForArchivedArticlesByUser($userId)
{ {
$ids = $this->createQueryBuilder('t') $ids = $this->getQueryBuilderByUser($userId)
->select('t.id') ->select('t.id')
->leftJoin('t.entries', 'e')
->where('e.user = :userId')->setParameter('userId', $userId)
->andWhere('e.isArchived = true') ->andWhere('e.isArchived = true')
->groupBy('t.id')
->orderBy('t.slug')
->getQuery() ->getQuery()
->getArrayResult(); ->getArrayResult();
@ -118,4 +123,20 @@ class TagRepository extends EntityRepository
return $tags; return $tags;
} }
/**
* Retrieve a sorted list of tags used by a user.
*
* @param int $userId
*
* @return QueryBuilder
*/
private function getQueryBuilderByUser($userId)
{
return $this->createQueryBuilder('t')
->leftJoin('t.entries', 'e')
->where('e.user = :userId')->setParameter('userId', $userId)
->groupBy('t.id')
->orderBy('t.slug');
}
} }

View file

@ -7,6 +7,8 @@ use Wallabag\CoreBundle\Entity\Tag;
class TagRestControllerTest extends WallabagApiTestCase class TagRestControllerTest extends WallabagApiTestCase
{ {
private $otherUserTagLabel = 'bob';
public function testGetUserTags() public function testGetUserTags()
{ {
$this->client->request('GET', '/api/tags.json'); $this->client->request('GET', '/api/tags.json');
@ -19,17 +21,33 @@ class TagRestControllerTest extends WallabagApiTestCase
$this->assertArrayHasKey('id', $content[0]); $this->assertArrayHasKey('id', $content[0]);
$this->assertArrayHasKey('label', $content[0]); $this->assertArrayHasKey('label', $content[0]);
$tagLabels = array_map(function ($i) {
return $i['label'];
}, $content);
$this->assertNotContains($this->otherUserTagLabel, $tagLabels, 'There is a possible tag leak');
return end($content); return end($content);
} }
public function testDeleteUserTag() public function testDeleteUserTag()
{ {
$em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
$entry = $this->client->getContainer()
->get('doctrine.orm.entity_manager')
->getRepository('WallabagCoreBundle:Entry')
->findOneWithTags($this->user->getId());
$entry = $entry[0];
$tagLabel = 'tagtest'; $tagLabel = 'tagtest';
$tag = new Tag(); $tag = new Tag();
$tag->setLabel($tagLabel); $tag->setLabel($tagLabel);
$em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
$em->persist($tag); $em->persist($tag);
$entry->addTag($tag);
$em->persist($entry);
$em->flush(); $em->flush();
$em->clear(); $em->clear();
@ -53,6 +71,16 @@ class TagRestControllerTest extends WallabagApiTestCase
$this->assertNull($tag, $tagLabel . ' was removed because it begun an orphan tag'); $this->assertNull($tag, $tagLabel . ' was removed because it begun an orphan tag');
} }
public function testDeleteOtherUserTag()
{
$em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
$tag = $em->getRepository('WallabagCoreBundle:Tag')->findOneByLabel($this->otherUserTagLabel);
$this->client->request('DELETE', '/api/tags/' . $tag->getId() . '.json');
$this->assertSame(404, $this->client->getResponse()->getStatusCode());
}
public function dataForDeletingTagByLabel() public function dataForDeletingTagByLabel()
{ {
return [ return [
@ -112,6 +140,13 @@ class TagRestControllerTest extends WallabagApiTestCase
$this->assertSame(404, $this->client->getResponse()->getStatusCode()); $this->assertSame(404, $this->client->getResponse()->getStatusCode());
} }
public function testDeleteTagByLabelOtherUser()
{
$this->client->request('DELETE', '/api/tag/label.json', ['tag' => $this->otherUserTagLabel]);
$this->assertSame(404, $this->client->getResponse()->getStatusCode());
}
/** /**
* @dataProvider dataForDeletingTagByLabel * @dataProvider dataForDeletingTagByLabel
*/ */
@ -180,4 +215,11 @@ class TagRestControllerTest extends WallabagApiTestCase
$this->assertSame(404, $this->client->getResponse()->getStatusCode()); $this->assertSame(404, $this->client->getResponse()->getStatusCode());
} }
public function testDeleteTagsByLabelOtherUser()
{
$this->client->request('DELETE', '/api/tags/label.json', ['tags' => $this->otherUserTagLabel]);
$this->assertSame(404, $this->client->getResponse()->getStatusCode());
}
} }