diff --git a/src/CookieHandler/CookieHandler.php b/src/CookieHandler/CookieHandler.php index 6888da5..3e75863 100644 --- a/src/CookieHandler/CookieHandler.php +++ b/src/CookieHandler/CookieHandler.php @@ -4,6 +4,7 @@ namespace Setono\SyliusPartnerAdsPlugin\CookieHandler; +use Setono\SyliusPartnerAdsPlugin\Parser\PartnerIdParser; use Symfony\Component\HttpFoundation\Cookie; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -28,17 +29,23 @@ public function remove(Response $response): void public function get(Request $request): int { - $value = $request->cookies->get($this->cookieName); + $partnerId = $this->parse($request); - // Fail loudly on misuse: callers must check has() first. Without this guard a missing - // cookie would silently be cast to partner id 0 and reported to Partner Ads. - Assert::notNull($value, sprintf('No "%s" cookie found on the request', $this->cookieName)); + // Fail loudly on misuse: callers must check has() first. Without this guard a missing or + // tampered cookie would silently become partner id 0 and be reported to Partner Ads. + Assert::notNull($partnerId, sprintf('No "%s" cookie holding a valid partner id found on the request', $this->cookieName)); - return (int) $value; + return $partnerId; } public function has(Request $request): bool { - return $request->cookies->has($this->cookieName); + return null !== $this->parse($request); + } + + private function parse(Request $request): ?int + { + // all() instead of get(): get() throws when the cookie is an array, see PartnerIdParser + return PartnerIdParser::parse($request->cookies->all()[$this->cookieName] ?? null); } } diff --git a/src/CookieHandler/CookieHandlerInterface.php b/src/CookieHandler/CookieHandlerInterface.php index 3213e82..430b842 100644 --- a/src/CookieHandler/CookieHandlerInterface.php +++ b/src/CookieHandler/CookieHandlerInterface.php @@ -20,12 +20,14 @@ public function set(Response $response, int $partnerId): void; public function remove(Response $response): void; /** - * Returns the cookie value which is a Partner Ads partner id + * Returns the Partner Ads partner id held by the cookie. Callers must check has() first: this method throws + * if the cookie is missing or does not hold a valid partner id. */ public function get(Request $request): int; /** - * Returns true if the request has the cookie set + * Returns true if the request has the cookie and it holds a valid (positive integer) partner id. + * A missing, empty, or tampered cookie is treated as absent. */ public function has(Request $request): bool; } diff --git a/src/EventListener/CreateConversionSubscriber.php b/src/EventListener/CreateConversionSubscriber.php index 63a0d49..21c0e4d 100644 --- a/src/EventListener/CreateConversionSubscriber.php +++ b/src/EventListener/CreateConversionSubscriber.php @@ -65,11 +65,8 @@ public function createConversion(GenericEvent $event): void return; } - // a tampered or mangled cookie casts to a non-positive integer - do not attribute the order in that case + // has() is only true for a valid partner id, so a tampered or mangled cookie never attributes the order $partnerId = $this->cookieHandler->get($request); - if ($partnerId <= 0) { - return; - } // best effort only: this prevents duplicates when the event is dispatched more than once sequentially, // but cannot see a concurrent request's uncommitted insert (see the class docblock) diff --git a/src/EventListener/SetCookieSubscriber.php b/src/EventListener/SetCookieSubscriber.php index 43a07d1..6189361 100644 --- a/src/EventListener/SetCookieSubscriber.php +++ b/src/EventListener/SetCookieSubscriber.php @@ -5,6 +5,7 @@ namespace Setono\SyliusPartnerAdsPlugin\EventListener; use Setono\SyliusPartnerAdsPlugin\CookieHandler\CookieHandlerInterface; +use Setono\SyliusPartnerAdsPlugin\Parser\PartnerIdParser; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpKernel\Event\ResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; @@ -37,10 +38,13 @@ public function setCookie(ResponseEvent $event): void return; } - if (!$request->query->has($this->queryParameter)) { + // all() instead of get(): get() throws a BadRequestException (a 400 for the whole page) when the + // parameter is an array (?paid[]=x), and a malformed affiliate link must never break a shop page + $partnerId = PartnerIdParser::parse($request->query->all()[$this->queryParameter] ?? null); + if (null === $partnerId) { return; } - $this->cookieHandler->set($event->getResponse(), (int) $request->query->get($this->queryParameter)); + $this->cookieHandler->set($event->getResponse(), $partnerId); } } diff --git a/src/Parser/PartnerIdParser.php b/src/Parser/PartnerIdParser.php new file mode 100644 index 0000000..309057c --- /dev/null +++ b/src/Parser/PartnerIdParser.php @@ -0,0 +1,24 @@ + ['min_range' => 1]]); + + return false === $partnerId ? null : $partnerId; + } +} diff --git a/tests/Unit/CookieHandler/CookieHandlerTest.php b/tests/Unit/CookieHandler/CookieHandlerTest.php index f622aed..11e61f4 100644 --- a/tests/Unit/CookieHandler/CookieHandlerTest.php +++ b/tests/Unit/CookieHandler/CookieHandlerTest.php @@ -4,6 +4,7 @@ namespace Setono\SyliusPartnerAdsPlugin\Tests\Unit\CookieHandler; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Setono\SyliusPartnerAdsPlugin\CookieHandler\CookieHandler; @@ -95,6 +96,32 @@ public function it_returns_false_if_cookie_is_not_set(): void self::assertFalse($cookieHandler->has($request)); } + #[Test] + #[DataProvider('invalidCookieValues')] + public function it_treats_a_cookie_without_a_valid_partner_id_as_absent(mixed $value): void + { + $cookieHandler = new CookieHandler($this->name, $this->expire); + $request = new Request([], [], [], [$this->name => $value]); + + self::assertFalse($cookieHandler->has($request)); + + $this->expectException(\InvalidArgumentException::class); + + $cookieHandler->get($request); + } + + /** + * @return iterable + */ + public static function invalidCookieValues(): iterable + { + yield 'empty' => ['']; + yield 'garbage' => ['junk']; + yield 'zero' => ['0']; + yield 'negative' => ['-1']; + yield 'array' => [['1']]; + } + private function createCookieHandler(Response $response): CookieHandler { $cookieHandler = new CookieHandler($this->name, $this->expire); @@ -106,7 +133,7 @@ private function createCookieHandler(Response $response): CookieHandler private function createRequest(?string $name = null): Request { // Cookies arrive as strings over HTTP, so the value is a string here on purpose. This also - // verifies the int cast in CookieHandler::get() is actually exercised. + // verifies that CookieHandler::get() parses the string into an integer. return new Request([], [], [], [ $name ?? $this->name => (string) $this->partnerId, ]); diff --git a/tests/Unit/EventListener/CreateConversionSubscriberTest.php b/tests/Unit/EventListener/CreateConversionSubscriberTest.php index f1e069a..5c16c05 100644 --- a/tests/Unit/EventListener/CreateConversionSubscriberTest.php +++ b/tests/Unit/EventListener/CreateConversionSubscriberTest.php @@ -115,19 +115,6 @@ public function it_does_nothing_when_the_cookie_is_not_set(): void $this->getSubscriber()->createConversion(new GenericEvent($order->reveal())); } - #[Test] - public function it_does_nothing_when_the_cookie_does_not_hold_a_valid_partner_id(): void - { - $order = $this->prophesize(OrderInterface::class); - - $this->cookieHandler->has($this->request)->willReturn(true); - $this->cookieHandler->get($this->request)->willReturn(0); - - $this->conversionRepository->add(Argument::any())->shouldNotBeCalled(); - - $this->getSubscriber()->createConversion(new GenericEvent($order->reveal())); - } - #[Test] public function it_does_nothing_when_the_order_already_has_a_conversion(): void { diff --git a/tests/Unit/EventListener/SetCookieSubscriberTest.php b/tests/Unit/EventListener/SetCookieSubscriberTest.php index 97bb8ed..2de9400 100644 --- a/tests/Unit/EventListener/SetCookieSubscriberTest.php +++ b/tests/Unit/EventListener/SetCookieSubscriberTest.php @@ -4,6 +4,7 @@ namespace Setono\SyliusPartnerAdsPlugin\Tests\Unit\EventListener; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Prophecy\Argument; @@ -71,6 +72,34 @@ public function it_does_nothing_when_the_query_parameter_is_not_set(): void $this->createSubscriber($cookieHandler->reveal())->setCookie($event); } + #[Test] + #[DataProvider('invalidPartnerIds')] + public function it_does_not_set_the_cookie_when_the_query_parameter_is_not_a_valid_partner_id(mixed $value): void + { + $cookieHandler = $this->prophesize(CookieHandlerInterface::class); + + $request = new Request([self::PARAM => $value]); + $event = $this->createEvent($request, HttpKernelInterface::MAIN_REQUEST); + + $cookieHandler->set(Argument::cetera())->shouldNotBeCalled(); + + // must not throw either - a malformed affiliate link must never break a shop page + $this->createSubscriber($cookieHandler->reveal())->setCookie($event); + } + + /** + * @return iterable + */ + public static function invalidPartnerIds(): iterable + { + yield 'empty' => ['']; + yield 'garbage' => ['junk']; + yield 'zero' => ['0']; + yield 'negative' => ['-5']; + yield 'decimal' => ['1.5']; + yield 'array' => [['1']]; + } + #[Test] public function it_sets_the_cookie(): void { diff --git a/tests/Unit/Parser/PartnerIdParserTest.php b/tests/Unit/Parser/PartnerIdParserTest.php new file mode 100644 index 0000000..65efa2e --- /dev/null +++ b/tests/Unit/Parser/PartnerIdParserTest.php @@ -0,0 +1,37 @@ + + */ + public static function values(): iterable + { + yield 'valid' => ['123', 123]; + yield 'smallest valid' => ['1', 1]; + yield 'integer' => [123, 123]; + yield 'zero' => ['0', null]; + yield 'negative' => ['-1', null]; + yield 'garbage' => ['junk', null]; + yield 'empty' => ['', null]; + yield 'decimal' => ['1.5', null]; + yield 'null' => [null, null]; + yield 'array' => [['1'], null]; + } +}