Adding SameSite=None cookie attribute to uids cookie

[PubMatic-OpenWrap]

This pull request contains changes to set SameSite=None attribute to the uids cookie for Chrome browser. Here is a summary of the changes:

1. A call to /setuid would check if browser version is Chrome 67+. If yes, we would add a "Set-Cookie" header for "uids" in the response with SameSite=None attribute. Along with this, we would add another "Set-Cookie" header which is "SSCookie=1" (also with SameSite=None attribute).

2. When a call is made to /cookie_sync, we would check if the browser version is Chrome 67+. If yes, we would also check if "SSCookie" cookie is present. If this cookie is not present, we would return sync-up URLs of all requested partners. If it present, we would return sync-up URLs of partners according to current logic.

3. To find the browser version, we check the user agent in the request: if it contains either "Chrome" or "CriOS" string, we check for the version. For Chrome browsers version 67 onwards, we apply the changes mentioned in fix user sync macro for index #1 and Fix pubmatic user sync #2 above.

[PubMatic-OpenWrap]

#900

[guscarreon]

I believe we can avoid the overhead of passing the http.Request pointer two extra levels in the stack and the expensive operation that comes with de-referencing it, as well as avoid over complicating the logic of SetCookieOnResponse if we just pass it a boolean value. Please let me knwo if you see this scenario feasible or if I'm missing something here.

[guscarreon]

There's a chance we go into both the if statements in lines 182 and 194. Do we want that? Or do we mean to make their true branches mutually exclusive via an else if statement in line 194?

[PubMatic-OpenWrap]

I've updated the code and used an else if now to keep them mutually exclusive.

[guscarreon]

Isn't it better to perform the browser name and browser version "stripping" operation back in the endpoint itself? We can probably avoid passing the http.Request pointer to SetCookieOnResponse and the expensive dereference operation we have to do on it in favor of a single boolean value that signals the difference in the cookie we set. Summarizing the possible scenarios that the logic added into SetCookieOnResponse gives us:

   | strings.Index(ua,"Chrome/") | strings.Index(ua,"CriOS/") | version>=chromeMinVer ||               result
--------------------------------------------------------------------------------------------------------------------------------------
 1 |        FALSE                |          FALSE             |         N/A           || w.Header().Add("Set-Cookie", cookieStr) //"cookieStr" without the "SameSiteAttribute" part
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 2 |        FALSE                |          FALSE             |         N/A           || w.Header().Add("Set-Cookie", cookieStr) //"cookieStr" without the "SameSiteAttribute" part
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 3 |        FALSE                |          TRUE              |       FALSE           || w.Header().Add("Set-Cookie", cookieStr) //"cookieStr" without the "SameSiteAttribute" part
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 4 |        FALSE                |          TRUE              |       TRUE            || w.Header().Add("Set-Cookie", cookieStr+SameSiteAttribute)
   |                             |                            |                       || w.Header().Add("Set-Cookie", sameSiteCookieStr)
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 5 |        TRUE                 |          FALSE             |       FALSE           || w.Header().Add("Set-Cookie", cookieStr) //"cookieStr" without the "SameSiteAttribute" part
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 6 |        TRUE                 |          FALSE             |       TRUE            || w.Header().Add("Set-Cookie", cookieStr+SameSiteAttribute)
   |                             |                            |                       || w.Header().Add("Set-Cookie", sameSiteCookieStr)
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 7 |        TRUE                 |          TRUE              |       FALSE           || w.Header().Add("Set-Cookie", cookieStr) //"cookieStr" without the "SameSiteAttribute" part
---|-----------------------------|----------------------------------------------------||----------------------------------------------
 8 |        TRUE                 |          TRUE              |       TRUE            || w.Header().Add("Set-Cookie", cookieStr+SameSiteAttribute)
   |                             |                            |                       || w.Header().Add("Set-Cookie", sameSiteCookieStr)
--------------------------------------------------------------------------------------------------------------------------------------

We can probably implement func chromeCheck(ua *UserAgent) (isChrome bool, isChromeOSS bool, isOverMinVersion bool) inside endpoints/setuid.go and evaluate the returning values into an statement like: var setSiteCookie bool = (isChrome && isOverMinVersion) || (isChromeOSS && isOverMinVersion) so we can a call a version of SetCookieOnResponse that takes a boolean value instead of a pointer to the http.Request like:

 17 func NewSetUIDEndpoint(cfg config.HostCookie, perms gdpr.Permissions, pbsanalytics analytics.PBSAnalyticsModule, metrics pbsmetrics.MetricsEngine) httprouter.Handle {
 18     cookieTTL := time.Duration(cfg.TTL) * 24 * time.Hour
 19     return httprouter.Handle(func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
 20         so := analytics.SetUIDObject{
 21             Status: http.StatusOK,
 22             Errors: make([]error, 0),
 23         }
 24 +-- 45 lines: defer pbsanalytics.LogSetUIDObject(&so)-----------------------------------------------------------------------------------------------------------------
 69
 70         if err == nil {
 71             labels := pbsmetrics.UserLabels{
 72                 Action: pbsmetrics.RequestActionSet,
 73                 Bidder: openrtb_ext.BidderName(bidder),
 74             }
 75             metrics.RecordUserIDSet(labels)
 76             so.Success = true
 77         }
 78         
      +     isChrome , isChromeOSS , isOverMinVersion := chromeCheck(ua *UserAgent)
      +     var setSiteCookie bool = (isChrome && isOverMinVersion) || (isChromeOSS && isOverMinVersion)
      +  
      -     pc.SetCookieOnResponse(w, r, cfg.Domain, cookieTTL)
 79   +     pc.SetCookieOnResponse(w, setSiteCookie, cfg.Domain, cookieTTL)
 80
 81     })
 82 }
endpoints/setuid.go

Or even better, just make:

      setSiteCookie := siteCookieCheck(ua *UserAgent)

[PubMatic-OpenWrap]

Yes, it's better to do the browser version check at the endpoint level. I've moved the code that checks for browser version to endpoints/setuid.go.

[PubMatic-OpenWrap]

Hi @guscarreon ,
Thanks for reviewing the changes. We'll start working on the suggestions shortly.

[PubMatic-OpenWrap]

I believe we can avoid the overhead of passing the http.Request pointer two extra levels in the stack and the expensive operation that comes with de-referencing it, as well as avoid over complicating the logic of SetCookieOnResponse if we just pass it a boolean value. Please let me knwo if you see this scenario feasible or if I'm missing something here.

Yes, this makes sense. I've updated the code to check for the browser version in endpoints/setuid.go and now passing only a boolean flag to SetCookieOnResponse()

[bretg]

Can we add the Secure flag to this as well? That will be needed soon enough.

[PubMatic-OpenWrap]

Can we add the Secure flag to this as well? That will be needed soon enough.

This is on our roadmap but we're handling it separately.

[guscarreon]

@PubMatic-OpenWrap can you please correct the merge conflicts?

[PubMatic-OpenWrap]

@PubMatic-OpenWrap can you please correct the merge conflicts?

Merged master into the branch and resolved conflicts.
I've also updated logic in usersync/cookie.go - moved the uids cookie setting code line i.e. w.Header().Add("Set-Cookie", uidsCookieStr) outside the if condition, thereby keeping it the way it was before our changes.

[guscarreon]

LGTM. We are leaving the secure flag implementation for another PR. If there is more than one browser out there that needs these or other special considerations, maybe implementing a map of special browsers with their corresponding versions vs a setting cookie function of their own can probably be appropriate. In the meantime, looks good as is.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[PubMatic-OpenWrap]

Thanks @guscarreon for your feedback.
Hi @hhhjort , can you please review this PR?

[SyntaxNode]

@PubMatic-OpenWrap I'm working a rewrite of the user sync handling code to address multiple feature requests and improvements. I understand the behavior of the SSCookie, but I cannot figure out why it's necessary. Do you remember why that was designed?

[PubMatic-OpenWrap]

@SyntaxNode , we wanted to handle the existing cookies i.e. if an existing uids cookie didn't have the SameSite=None attribute set and if the Chrome browser version was 67+, then we wanted /cookie_sync to return the bidder sync URLs to do the sync-up again. At the time prebid-server was using Go 1.11 in which the cookie's attributes were not accessible. Hence, as a workaround, we decided that /setuid would also set a new cookie "SSCookie=1" for Chrome browser version 67+. /cookie_sync then checks if "SSCookie" is present to decide if bidder sync URLs need to be returned or not.
Hope this helps.

[SyntaxNode]

@PubMatic-OpenWrap Thank you. That explains the existence of the SSCookie flag, to detect when the browser has set SameSite=None to get around limitations in the Go networking libraries.

Why is it necessary to do the sync-up again for Chrome 67+ though? Would the existing syncs no longer be valid for some reason?

[PubMatic-OpenWrap]

@SyntaxNode , We did the sync-ups again with the SameSite attribute for version 67+ because we didn't want to lose existing cookies when the user updates to version 76+. Also, older versions (before 67) were not compatible to the SameSite change.