Only accept a positive integer as the partner id

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);
     }
 }

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;
 }

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)

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);
     }
 }

src/Parser/PartnerIdParser.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Setono\SyliusPartnerAdsPlugin\Parser;
+
+final class PartnerIdParser
+{
+    /**
+     * Returns the Partner Ads partner id held by a raw request value - the affiliate query parameter or the
+     * cookie - or null if the value is not a positive integer.
+     *
+     * Anything else must never be treated as a partner id: an empty or mangled value (a tracker rewriting the
+     * affiliate link, a hand-edited cookie) would otherwise be cast to partner id 0, overwriting a legitimate
+     * attribution and later being reported to Partner Ads as partner 0. Arrays (?paid[]=x) are rejected here too,
+     * so callers can read the raw value without InputBag::get() throwing a 400 for them.
+     */
+    public static function parse(mixed $value): ?int
+    {
+        $partnerId = filter_var($value, \FILTER_VALIDATE_INT, ['options' => ['min_range' => 1]]);
+
+        return false === $partnerId ? null : $partnerId;
+    }
+}

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<string, array{mixed}>
+     */
+    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,
         ]);

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
     {

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<string, array{mixed}>
+     */
+    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
     {

tests/Unit/Parser/PartnerIdParserTest.php

@@ -0,0 +1,37 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Setono\SyliusPartnerAdsPlugin\Tests\Unit\Parser;
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\TestCase;
+use Setono\SyliusPartnerAdsPlugin\Parser\PartnerIdParser;
+
+final class PartnerIdParserTest extends TestCase
+{
+    #[Test]
+    #[DataProvider('values')]
+    public function it_parses(mixed $value, ?int $expected): void
+    {
+        self::assertSame($expected, PartnerIdParser::parse($value));
+    }
+
+    /**
+     * @return iterable<string, array{mixed, int|null}>
+     */
+    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];
+    }
+}