limit the size of the uids cookie #1004

guscarreon · 2019-08-19T05:27:34Z

We hit an issue where the header (containing the uids cookie) overran the limit that Ingress could handle. This pull request checks the size of the PBSCookie and, if found bigger than 32 KB in its http form, traverses the PBSCookie.uids field deleting the entries closest to expiration date.

usersync/cookie.go

hhhjort · 2019-08-19T17:28:28Z

usersync/cookie.go

@@ -162,9 +215,31 @@ func (cookie *PBSCookie) GetId(bidderName openrtb_ext.BidderName) (id string, ex
 // SetCookieOnResponse is a shortcut for "ToHTTPCookie(); cookie.setDomain(domain); setCookie(w, cookie)"
 func (cookie *PBSCookie) SetCookieOnResponse(w http.ResponseWriter, domain string, ttl time.Duration) {
 	httpCookie := cookie.ToHTTPCookie(ttl)
+	var MAX_COOKIE_SIZE int = 1 << 15 // 32 KB


We will want this as a config parameter rather than hardcoded. Also a check for a 0 "size" to disable enforcement of a maximum size may not be bad.

Do you mean get it from the configmap.yaml file itself? Bring it all the way here straight from the Configuration struct found in config/config.go?

What do you mean check for a 0 size? A 0 size httpCookie.String()?

Yes, bring the config struct, or the relevant value, all the way down to here. It is the configured value for MAX_COOKIE_SIZE you should check for a 0 value, as a method of turning off this feature. If a host limits the number of bidders they support, they can limit the size the cookie can grow to.

hhhjort · 2019-08-19T17:32:04Z

usersync/cookie.go

+		var oldestElem string = ""
+		var oldestDate int64 = math.MaxInt64
+		for key, value := range cookie.uids {
+			var timeSince time.Duration = now.Sub(value.Expires)


You shouldn't need to specify time.Duration as that is already defined as the return type of now.Sub()

Also, since you are taking the current time, and subtracting the expirery time, this will result in a negative number. The most negative number will be the expires time that is furthest from now, so in the end we will remove the entry that has the longest time before it expires.

Gosh, I got the Sub() function backwards, corrected

hhhjort · 2019-08-28T14:58:59Z

config/config.go

+	CacheURL           Cache              `mapstructure:"cache"`
+	RecaptchaSecret    string             `mapstructure:"recaptcha_secret"`
+	HostCookie         HostCookie         `mapstructure:"host_cookie"`
+	MaxCookieSizeBytes int                `mapstructure:"max_cookie_size_bytes"`


Why do we have a Max Size here as well as in HostCookie. I think HostCookie is probably the best place for it out of the two.

Sorry, that's a leftover from another approach I was trying. Good call. Removed

hhhjort · 2019-08-28T15:00:21Z

config/config.go

@@ -500,6 +502,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("host_cookie.optout_cookie.name", "")
 	v.SetDefault("host_cookie.value", "")
 	v.SetDefault("host_cookie.ttl_days", 90)
+	v.SetDefault("host_cookie.max_cookie_size_bytes", 32768)


We probably want to set the default as unlimited, so it won't change the behavior of running installs.

hhhjort

LGTM

mansinahar · 2019-09-03T16:22:23Z

usersync/cookie.go

+		var oldestElem string = ""
+		var oldestDate int64 = math.MaxInt64
+		for key, value := range cookie.uids {
+			timeUntilExpiration := value.Expires.Sub(now)


Just a tip: Time package has an until function that will do exactly this for you. So you can potentially replace this with time.Until(value.Expires)

mansinahar · 2019-09-03T18:08:50Z

usersync/cookie.go

+	uids         map[string]uidWithExpiry
+	optOut       bool
+	birthday     *time.Time
+	maxSizeBytes int


I don't think we need maxSizeBytes as part of the PBSCookie struct. We can just access the MaxCookieSizeBytes in the HostCookie struct and pass it on in the call to SetCookieOnResponse. This should make the code a little simpler.

Spoke with @guscarreon offline about this and he said he will make the suggested change.

Bring 'master' branch latest changes

mansinahar · 2019-09-10T02:36:25Z

config/config.go

+	// If a non-zero-less-than-500-byte "host_cookie.max_cookie_size_bytes" value was specified in the
+	// environment configuration of prebid-server, default to 500 bytes
+	if maxCookieSize != 0 && maxCookieSize < MIN_COOKIE_SIZE_BYTES {
+		return errors.New("Configured cookie size is less than allowed minimum size")


Might be good to incorporate the MIN_COOKIE_SIZE_BYTES value in the error message so that the user knows what it is from the error message itself rather than having to dig into the code to find it.

Makes perfect sense, done

mansinahar · 2019-09-10T03:05:55Z

usersync/cookie_test.go

+}
+
+func newBigCookie() (*PBSCookie, int) {
+	var bigCookie *PBSCookie = &PBSCookie{


How about creating a test folder and adding these different cookies there in .json files and then using those in the tests rather than populating this test file with a giant map?

Spoke with @guscarreon about this and we probably don't need this particular test case to start with and so he's gonna evaluate and remove this if not needed.

…ase in usersync/cookie_test.go

mansinahar · 2019-09-10T17:01:39Z

usersync/cookie_test.go

-			assert.Equal(t, cookieToSendLen, len(processedCookie.uids), "[Test %d] MaxCookieSizeBytes equal to zero or bigger than %d bytes should be snough to set and remain cookie unchanged \n", i+1, len(processedCookie.uids))
-			_, oldestFound := processedCookie.uids["key7"]
+			assert.Equal(t, cookieToSendLen, len(processedCookie.uids), "[Test %d] MaxCookieSizeBytes equal to zero or bigger than %d bytes should be enough to set and remain cookie unchanged \n", i+1, len(processedCookie.uids))
+			_, oldestFound := processedCookie.uids[closestToExpirationDate]
 			assert.Equal(t, oldestFound, true, "[Test %d] Oldest entry in cookie should not have been eliminated", i+1)


You can just do assert.Contains here on the map and skip the previous line.

mansinahar · 2019-09-10T17:01:55Z

usersync/cookie_test.go

 			assert.Equal(t, oldestFound, true, "[Test %d] Oldest entry in cookie should not have been eliminated", i+1)
 		case "trim":
 			assert.Equal(t, cookieToSendLen > len(processedCookie.uids), true, "[Test %d] MaxCookieSizeBytes of %d is smaller than %d bytes and cookie entries should have been removed\n", i+1, testCases[i].maxCookieSize, cookieToSendLen)
-			_, oldestFound := processedCookie.uids["key7"]
+			_, oldestFound := processedCookie.uids[closestToExpirationDate]
 			assert.Equal(t, oldestFound, false, "[Test %d] Oldest entry in cookie was not eliminated", i+1)


Same as above

…st.go

* some research * Modifications and test cases in place * Hans' corrections and random expiration date to cookies in test case * Cookie size gets defined in configmap.yaml now * removed MaxCookieSizeBytes duplicate * removed unnecessary constant declaration * Set unlimited size as default, added unlimited size test case * corrected test case * symplified solution by pulling size from Configuration struct itself * substituted time.Sub() for time.Until() and tuned adapter test cases * removed vim .swp files * MIN_COOKIE_SIZE_BYTES implemented in config.go * correct SetCookieOnResponse() return value * Printed MIN_COOKIE_SIZE_BYTES in error message, added one more test case in usersync/cookie_test.go * Resolved conflict in config/config.go * Resolved another conflict

Gus Carreon added 2 commits August 14, 2019 10:33

some research

bb3a1c8

Modifications and test cases in place

ad41f46

guscarreon requested review from hhhjort and mansinahar August 19, 2019 05:27

guscarreon assigned hhhjort Aug 19, 2019

hhhjort reviewed Aug 19, 2019

View reviewed changes

usersync/cookie.go Outdated Show resolved Hide resolved

hhhjort reviewed Aug 19, 2019

View reviewed changes

Gus Carreon added 2 commits August 20, 2019 17:42

Hans' corrections and random expiration date to cookies in test case

6ce9501

Cookie size gets defined in configmap.yaml now

be06e84

hhhjort reviewed Aug 28, 2019

View reviewed changes

Gus Carreon added 4 commits August 28, 2019 11:02

removed MaxCookieSizeBytes duplicate

bf41c0a

removed unnecessary constant declaration

68d1a23

Set unlimited size as default, added unlimited size test case

a5ce966

corrected test case

348b9f7

hhhjort previously approved these changes Aug 28, 2019

View reviewed changes

hhhjort assigned mansinahar and unassigned hhhjort Aug 28, 2019

mansinahar reviewed Sep 3, 2019

View reviewed changes

Gus Carreon added 4 commits September 3, 2019 19:54

Merge branch 'master' into PBS-455

2edd18e

Bring 'master' branch latest changes

symplified solution by pulling size from Configuration struct itself

74c1744

substituted time.Sub() for time.Until() and tuned adapter test cases

5cd01f8

removed vim .swp files

357b6fc

guscarreon dismissed hhhjort’s stale review via 357b6fc September 4, 2019 02:53

Gus Carreon added 2 commits September 4, 2019 22:35

MIN_COOKIE_SIZE_BYTES implemented in config.go

048273c

correct SetCookieOnResponse() return value

04c85c7

hhhjort previously approved these changes Sep 5, 2019

View reviewed changes

mansinahar reviewed Sep 10, 2019

View reviewed changes

Printed MIN_COOKIE_SIZE_BYTES in error message, added one more test c…

1ffc002

…ase in usersync/cookie_test.go

guscarreon dismissed hhhjort’s stale review via 1ffc002 September 10, 2019 16:53

Resolved conflict in config/config.go

24cf211

mansinahar reviewed Sep 10, 2019

View reviewed changes

Gus Carreon added 2 commits September 10, 2019 13:05

Resolved another conflict

89d76b7

Used assert.Containsf() instead of assert.Equal in usersync/cookie_te…

b1134cd

…st.go

mansinahar approved these changes Sep 10, 2019

View reviewed changes

mansinahar assigned hhhjort Sep 10, 2019

hhhjort approved these changes Sep 10, 2019

View reviewed changes

hhhjort merged commit ddf172c into prebid:master Sep 10, 2019

guscarreon mentioned this pull request Jun 5, 2023

Optimize SetCookieOnResponse() #2819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

limit the size of the uids cookie #1004

limit the size of the uids cookie #1004

guscarreon commented Aug 19, 2019

hhhjort Aug 19, 2019

guscarreon Aug 20, 2019

guscarreon Aug 20, 2019

hhhjort Aug 21, 2019

guscarreon Aug 28, 2019

hhhjort Aug 19, 2019

hhhjort Aug 19, 2019

guscarreon Aug 20, 2019

hhhjort Aug 28, 2019

guscarreon Aug 28, 2019

hhhjort Aug 28, 2019

guscarreon Aug 28, 2019

hhhjort left a comment

mansinahar Sep 3, 2019

guscarreon Sep 4, 2019

mansinahar Sep 3, 2019

guscarreon Sep 4, 2019

mansinahar Sep 10, 2019

guscarreon Sep 10, 2019

mansinahar Sep 10, 2019 •

edited

Loading

mansinahar Sep 10, 2019

mansinahar Sep 10, 2019

guscarreon Sep 10, 2019

guscarreon Sep 10, 2019

mansinahar Sep 10, 2019

guscarreon Sep 10, 2019

limit the size of the uids cookie #1004

limit the size of the uids cookie #1004

Conversation

guscarreon commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Sep 10, 2019 •

edited

Loading