-
-
Notifications
You must be signed in to change notification settings - Fork 1
Parse the _fbc cookie the way Meta writes it #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,31 @@ | |
|
|
||
| namespace Setono\MetaConversionsApiBundle\Context\Fbc; | ||
|
|
||
| use Psr\Log\LoggerInterface; | ||
| use Psr\Log\NullLogger; | ||
| use Setono\MetaConversionsApi\ValueObject\Fbc; | ||
| use Symfony\Component\HttpFoundation\RequestStack; | ||
|
|
||
| final class CookieBasedFbcContext implements FbcContextInterface | ||
| { | ||
| public function __construct(private readonly RequestStack $requestStack) | ||
| { | ||
| /** | ||
| * Matches the _fbc cookie as Meta writes it today: fb.<subdomain index>.<creation time>.<click id> with an | ||
| * optional trailing appendix segment. | ||
| * | ||
| * This is deliberately more lenient than Fbc::fromString() in the SDK, which only accepts alphanumeric click ids | ||
| * and exactly four segments. Real click ids are base64url and contain - and _, and Meta's own parameter builder | ||
| * (facebook/capi-param-builder-php) appends a 2 or 8 character appendix, so the strict pattern rejects cookies | ||
| * that the browser pixel writes | ||
| */ | ||
| private const COOKIE_PATTERN = '/^fb\.([012])\.(\d{13})\.([A-Za-z0-9_-]+)(?:\.([A-Za-z0-9_-]{2,8}))?$/'; | ||
|
|
||
| private readonly LoggerInterface $logger; | ||
|
|
||
| public function __construct( | ||
| private readonly RequestStack $requestStack, | ||
| ?LoggerInterface $logger = null, | ||
| ) { | ||
| $this->logger = $logger ?? new NullLogger(); | ||
| } | ||
|
|
||
| public function getFbc(): ?Fbc | ||
|
|
@@ -20,14 +38,30 @@ public function getFbc(): ?Fbc | |
| return null; | ||
| } | ||
|
|
||
| $fbc = $request->cookies->get('_fbc'); | ||
| if (is_string($fbc)) { | ||
| try { | ||
| return Fbc::fromString($fbc); | ||
| } catch (\InvalidArgumentException) { | ||
| } | ||
| $cookie = $request->cookies->get('_fbc'); | ||
| if (!is_string($cookie) || '' === $cookie) { | ||
| return null; | ||
| } | ||
|
|
||
| return null; | ||
| if (1 !== preg_match(self::COOKIE_PATTERN, $cookie, $matches)) { | ||
| $this->logger->debug('The _fbc cookie value "{value}" could not be parsed and is ignored', ['value' => $cookie]); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return (new Fbc($matches[3])) | ||
| ->withSubdomainIndex((int) $matches[1]) | ||
| ->withCreationTime((int) $matches[2]) | ||
| ; | ||
| } catch (\InvalidArgumentException $e) { | ||
| // The creation time is in the future or predates Facebook | ||
| $this->logger->debug('The _fbc cookie value "{value}" was rejected: {message}', [ | ||
| 'value' => $cookie, | ||
| 'message' => $e->getMessage(), | ||
| ]); | ||
|
|
||
| return null; | ||
|
Comment on lines
+46
to
+64
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why put the code here instead of rewriting
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, it belongs there. I moved it: Setono/meta-conversions-api-php-sdk#13. My reason for putting it here was that the SDK is a separate release cycle, which is a weak argument when you maintain both, and it left the bundle carrying a second parser next to the SDK's. Chasing it into the SDK also turned up something my review missed: Fbp::fromString('fb.1.1657051589577.1088522659.AQEAAQMB'); // throwsThat is the shape Meta's parameter builder writes. The SDK PR relaxes both patterns and preserves the appendix, so a value read from a cookie is written back byte for byte rather than being rewritten into a shape the pixel does not expect. Coverage stays at 100% and Infection stays above the threshold. Suggested order. This PR is blocked until that one is released, since CI here would still resolve Say the word if you would rather I park this one and reopen it after the tag. |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Setono\MetaConversionsApiBundle\Tests\Unit\Context\Fbc; | ||
|
|
||
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use Setono\MetaConversionsApi\ValueObject\Fbc; | ||
| use Setono\MetaConversionsApiBundle\Context\Fbc\CookieBasedFbcContext; | ||
| use Symfony\Component\HttpFoundation\Request; | ||
| use Symfony\Component\HttpFoundation\RequestStack; | ||
|
|
||
| #[CoversClass(CookieBasedFbcContext::class)] | ||
| final class CookieBasedFbcContextTest extends TestCase | ||
| { | ||
| #[Test] | ||
| public function it_returns_null_without_a_request(): void | ||
| { | ||
| self::assertNull((new CookieBasedFbcContext(new RequestStack()))->getFbc()); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function it_returns_null_without_a_cookie(): void | ||
| { | ||
| self::assertNull((new CookieBasedFbcContext(self::requestStack(null)))->getFbc()); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('cookies')] | ||
| public function it_parses_the_cookie(string $cookie, string $expectedClickId, int $expectedSubdomainIndex, int $expectedCreationTime): void | ||
| { | ||
| $fbc = (new CookieBasedFbcContext(self::requestStack($cookie)))->getFbc(); | ||
|
|
||
| self::assertInstanceOf(Fbc::class, $fbc); | ||
| self::assertSame($expectedClickId, $fbc->getClickId()); | ||
| self::assertSame($expectedSubdomainIndex, $fbc->getSubdomainIndex()); | ||
| self::assertSame($expectedCreationTime, $fbc->getCreationTime()); | ||
| } | ||
|
|
||
| /** | ||
| * @return iterable<string, array{string, string, int, int}> | ||
| */ | ||
| public static function cookies(): iterable | ||
| { | ||
| yield 'alphanumeric click id' => [ | ||
| 'fb.1.1657051589577.IwAR0rmfgHgxjdKoEopat9y2SPzyjGgfHm9AhdqygToWvarP59nPq15T07MiA', | ||
| 'IwAR0rmfgHgxjdKoEopat9y2SPzyjGgfHm9AhdqygToWvarP59nPq15T07MiA', | ||
| 1, | ||
| 1657051589577, | ||
| ]; | ||
|
|
||
| // Real click ids are base64url, so they contain - and _ | ||
| yield 'base64url click id' => [ | ||
| 'fb.1.1657051589577.IwZXh0bgNhZW0CMTAAAR-uK_5w', | ||
| 'IwZXh0bgNhZW0CMTAAAR-uK_5w', | ||
| 1, | ||
| 1657051589577, | ||
| ]; | ||
|
|
||
| // Meta's own parameter builder appends a 2 or 8 character appendix | ||
| yield 'appendix v1' => [ | ||
| 'fb.2.1657051589577.IwAR0rmfgHgx.AQ', | ||
| 'IwAR0rmfgHgx', | ||
| 2, | ||
| 1657051589577, | ||
| ]; | ||
|
|
||
| yield 'appendix v2' => [ | ||
| 'fb.0.1657051589577.IwAR0rmfgHgx.AbCdEfGh', | ||
| 'IwAR0rmfgHgx', | ||
| 0, | ||
| 1657051589577, | ||
| ]; | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('invalidCookies')] | ||
| public function it_returns_null_for_an_unparseable_cookie(string $cookie): void | ||
| { | ||
| self::assertNull((new CookieBasedFbcContext(self::requestStack($cookie)))->getFbc()); | ||
| } | ||
|
|
||
| /** | ||
| * @return iterable<string, array{string}> | ||
| */ | ||
| public static function invalidCookies(): iterable | ||
| { | ||
| yield 'empty' => ['']; | ||
| yield 'garbage' => ['not-an-fbc-cookie']; | ||
| yield 'wrong prefix' => ['xx.1.1657051589577.abc']; | ||
| yield 'invalid subdomain index' => ['fb.3.1657051589577.abc']; | ||
| yield 'short creation time' => ['fb.1.165705158.abc']; | ||
| yield 'missing click id' => ['fb.1.1657051589577.']; | ||
| yield 'creation time in the future' => ['fb.1.9999999999999.abc']; | ||
| } | ||
|
|
||
| private static function requestStack(?string $cookie): RequestStack | ||
| { | ||
| $requestStack = new RequestStack(); | ||
| $requestStack->push(new Request([], [], [], null === $cookie ? [] : ['_fbc' => $cookie])); | ||
|
|
||
| return $requestStack; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the facebook/capi-param-builder-php if possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth doing, but I would not fold it into this PR. I ran it against the current vendored copy (1.3.1, it arrives transitively through
facebook/php-business-sdk) to get real numbers:That confirms two things the review found: Meta writes a five segment value with an eight character appendix, and it sets the cookie on the registrable domain rather than host-only.
What speaks for it
getClientIpAddress()andgetEventSourceUrl().processRequest($host, $queries, $cookies, $referer, $xForwardedFor, $remoteAddress)takes explicit arguments, so no$_GET/$_SERVERaccess. It autoloads cleanly.What speaks against doing it here
require. Today we only get it becausephp-business-sdkpulls it in.Contextclasses are small decorators a user can swap one at a time. Adopting it means redesigningContext/Fbc,Context/Fbp,StoreFbcSubscriberandStoreFbpSubscriber, and deciding whatFbcContextInterface::getFbc(): ?Fbcreturns, since the builder deals in strings.Fbc/Fbpvalue objects.User::$fbcacceptsstring|Fbc|nullso it would work, but our own interfaces would change.So: a good idea, and a design change rather than a bug fix. I opened #45 for it. This PR stays the minimal fix so the cookies Meta writes today are actually read, whichever way that decision goes.