stape-io / facebook-tag

Facebook tag for Google Tag Manager Server Side
https://stape.io/how-to-set-up-facebook-conversion-api/
Apache License 2.0
37 stars 21 forks source link

httpOnly option doesn't set cookie as HttpOnly #16

Closed LTDanwithlegs closed 2 years ago

LTDanwithlegs commented 2 years ago

Action Change the boolean value of httpOnly to true for the cookies _fbc and _fbp

Expected Result The cookie is set with the HttpOnly flag in the response to the browser.

Actual Result The cookie is not set with the HttpOnly flag in the response to the browser.

Cause In the template code, httpOnly is camelCase. Changing this to PascalCase (i.e. HttpOnly: true) sets the cookies with the correct HttpOnly flag.

Context We're trying to remove all Facebook Javascript from our websites. After the implementation of the option to set the _fbp cookie (thanks!), the last step was to have all cookies be readable only by our SSGTM container. The goal is make what we're reporting to Facebook more accurate, especially in browsers with strict privacy restrictions (by removing the browser from the equation as much as we can).

I tested the above cause and fix with Simo Ahava's Cookie Monster tag template. That change in case was the only difference present. After making the change in stape-io/facebook-tag and retesting without Cookie Monster (and with clearing cookies), the _fbc and _fbp cookies were properly set as HttpOnly. This is because the camelCase option names are only valid options for Google's setCookie() function - httpOnly is not a valid option of that function. However HttpOnly (PascalCase) is a default param of how setCookie() is actually setting the cookie in the response - not the function, itself. This bypasses the lack of httpOnly option in the sandboxed setCookie() function.

Recommend adding a checkbox/boolean flag for setting cookies via HttpOnly with this method.

Resources setCookie() API reference Cookie Monster Tag Template by Simo Ahava

Bukashk0zzz commented 2 years ago

Hi,

Thanks for the detailed explanation. I will add a checkbox for setting HttpOnly cookies however I am not sure that you are right in terms of using the HttpOnly property name instead httpOnly. In documentation mentioned that httpOnly proper way of naming https://developers.google.com/tag-platform/tag-manager/server-side/api#setcookie

Also, Cookie Monster used camelCase naming https://github.com/gtm-templates-simo-ahava/cookie-monster/blob/main/template.tpl#L157

Bukashk0zzz commented 2 years ago

Added HttpOnly option https://github.com/stape-io/facebook-tag/commit/c408e9798474f30c97a3b672b4f8da8100f88f6f

LTDanwithlegs commented 2 years ago

Line 157 of Cookie Monster checks to see if that param is set with camelCase and if so, sets it with PascalCase using the value of the camelCase one..

On Sat, Feb 26, 2022, 08:31 Denis Golubovskiy @.***> wrote:

Hi,

Thanks for the detailed explanation. I will add a checkbox for setting HttpOnly cookies however I am not sure that you are right in terms of using the HttpOnly property name instead httpOnly. In documentation mentioned that httpOnly proper way of naming https://developers.google.com/tag-platform/tag-manager/server-side/api#setcookie

Also, Cookie Monster used camelCase naming https://github.com/gtm-templates-simo-ahava/cookie-monster/blob/main/template.tpl#L157

— Reply to this email directly, view it on GitHub https://github.com/stape-io/facebook-tag/issues/16#issuecomment-1052144521, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFJAX6V2YV5HULC6G7YIXT3U5DP5JANCNFSM5PK3VEKA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

LTDanwithlegs commented 2 years ago

Also, https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie shows that the syntax of the response header is PascalCase. Since Google's set setCookie function passes through params other than the options listed in the docs, as is, PascalCase should be used until Google changes this function to add httpOnly as a documented parameter.

On Sat, Feb 26, 2022, 12:00 Daniel Brooks @.***> wrote:

Line 157 of Cookie Monster checks to see if that param is set with camelCase and if so, sets it with PascalCase using the value of the camelCase one..

On Sat, Feb 26, 2022, 08:31 Denis Golubovskiy @.***> wrote:

Hi,

Thanks for the detailed explanation. I will add a checkbox for setting HttpOnly cookies however I am not sure that you are right in terms of using the HttpOnly property name instead httpOnly. In documentation mentioned that httpOnly proper way of naming https://developers.google.com/tag-platform/tag-manager/server-side/api#setcookie

Also, Cookie Monster used camelCase naming https://github.com/gtm-templates-simo-ahava/cookie-monster/blob/main/template.tpl#L157

— Reply to this email directly, view it on GitHub https://github.com/stape-io/facebook-tag/issues/16#issuecomment-1052144521, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFJAX6V2YV5HULC6G7YIXT3U5DP5JANCNFSM5PK3VEKA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

Bukashk0zzz commented 2 years ago

It looks like you are right. Thanks again. I updated the naming to Pascal Case: https://github.com/stape-io/facebook-tag/commit/484bcb40ed517788ef1962a06baae4c39b179e40

By the way, I made tests with camelCase naming and still have correct results, but because of your arguments, I decided to update the template using PascalCase.

LTDanwithlegs commented 2 years ago

Awesome, thanks so much!

I don't know why our tests would be so different. Maybe there's a difference in how the browsers handle the response headers. I wonder if some browsers will accept both and some are more strict. Just conjecture - I have no proof as to that being the case.

Bukashk0zzz commented 2 years ago

Agree. Thanks again for reporting.