From 4507884f74d42e2cf5ad6b9b62020a9d8b8fce4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 31 Aug 2026 10:58:47 +0200 Subject: [PATCH] Only accept a positive integer as the partner id The affiliate query parameter was cast to int without validation, so an empty or mangled value (a tracker rewriting the link) set the cookie to partner id 0, overwriting a legitimate attribution, and an array value (?paid[]=x) made InputBag::get() throw, turning any shop URL into a 400. The cookie value had the same leniency. A raw value is now only a partner id if it is a positive integer; the cookie handler treats anything else as absent, and the subscriber reads the raw value so arrays cannot throw. Fixes #51 Claude-Session: https://claude.ai/code/session_01Mt12J8vdGwwWg4V23uoEf9 --- src/CookieHandler/CookieHandler.php | 19 +++++++--- src/CookieHandler/CookieHandlerInterface.php | 6 ++- .../CreateConversionSubscriber.php | 5 +-- src/EventListener/SetCookieSubscriber.php | 8 +++- src/Parser/PartnerIdParser.php | 24 ++++++++++++ .../Unit/CookieHandler/CookieHandlerTest.php | 29 ++++++++++++++- .../CreateConversionSubscriberTest.php | 13 ------- .../EventListener/SetCookieSubscriberTest.php | 29 +++++++++++++++ tests/Unit/Parser/PartnerIdParserTest.php | 37 +++++++++++++++++++ 9 files changed, 142 insertions(+), 28 deletions(-) create mode 100644 src/Parser/PartnerIdParser.php create mode 100644 tests/Unit/Parser/PartnerIdParserTest.php 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]; + } +}