Skip to content
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

limit the size of the uids cookie #1004

Merged
merged 18 commits into from
Sep 10, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion adapters/conversant/conversant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ func ParseRequest(req *pbs.PBSRequest) (*pbs.PBSRequest, error) {
// Need to pass the conversant user id thru uid cookie

httpReq := httptest.NewRequest("POST", "/foo", body)
cookie := usersync.NewPBSCookie()
cookie := usersync.NewPBSCookie(0)
_ = cookie.TrySync("conversant", ExpectedBuyerUID)
httpReq.Header.Set("Cookie", cookie.ToHTTPCookie(90*24*time.Hour).String())
httpReq.Header.Add("Referer", "http://example.com")
Expand Down
2 changes: 1 addition & 1 deletion adapters/openrtb_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ func TestOpenRTBEmptyUser(t *testing.T) {
}

func TestOpenRTBUserWithCookie(t *testing.T) {
pbsCookie := usersync.NewPBSCookie()
pbsCookie := usersync.NewPBSCookie(0)
pbsCookie.TrySync("test", "abcde")
pbReq := pbs.PBSRequest{
User: &openrtb.User{},
Expand Down
33 changes: 18 additions & 15 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,16 @@ type Configuration struct {
EnableGzip bool `mapstructure:"enable_gzip"`
// StatusResponse is the string which will be returned by the /status endpoint when things are OK.
// If empty, it will return a 204 with no content.
StatusResponse string `mapstructure:"status_response"`
AuctionTimeouts AuctionTimeouts `mapstructure:"auction_timeouts_ms"`
CacheURL Cache `mapstructure:"cache"`
RecaptchaSecret string `mapstructure:"recaptcha_secret"`
HostCookie HostCookie `mapstructure:"host_cookie"`
Metrics Metrics `mapstructure:"metrics"`
DataCache DataCache `mapstructure:"datacache"`
StoredRequests StoredRequests `mapstructure:"stored_requests"`
CategoryMapping StoredRequestsSlim `mapstructure:"category_mapping"`
StatusResponse string `mapstructure:"status_response"`
AuctionTimeouts AuctionTimeouts `mapstructure:"auction_timeouts_ms"`
CacheURL Cache `mapstructure:"cache"`
RecaptchaSecret string `mapstructure:"recaptcha_secret"`
HostCookie HostCookie `mapstructure:"host_cookie"`
MaxCookieSizeBytes int `mapstructure:"max_cookie_size_bytes"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Metrics Metrics `mapstructure:"metrics"`
DataCache DataCache `mapstructure:"datacache"`
StoredRequests StoredRequests `mapstructure:"stored_requests"`
CategoryMapping StoredRequestsSlim `mapstructure:"category_mapping"`
// Note that StoredVideo refers to stored video requests, and has nothing to do with caching video creatives.
StoredVideo StoredRequestsSlim `mapstructure:"stored_video_req"`

Expand Down Expand Up @@ -168,12 +169,13 @@ type FileLogs struct {
}

type HostCookie struct {
Domain string `mapstructure:"domain"`
Family string `mapstructure:"family"`
CookieName string `mapstructure:"cookie_name"`
OptOutURL string `mapstructure:"opt_out_url"`
OptInURL string `mapstructure:"opt_in_url"`
OptOutCookie Cookie `mapstructure:"optout_cookie"`
Domain string `mapstructure:"domain"`
Family string `mapstructure:"family"`
CookieName string `mapstructure:"cookie_name"`
OptOutURL string `mapstructure:"opt_out_url"`
OptInURL string `mapstructure:"opt_in_url"`
MaxCookieSizeBytes int `mapstructure:"max_cookie_size_bytes"`
OptOutCookie Cookie `mapstructure:"optout_cookie"`
// Cookie timeout in days
TTL int64 `mapstructure:"ttl_days"`
}
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

v.SetDefault("http_client.max_idle_connections", 400)
v.SetDefault("http_client.max_idle_connections_per_host", 10)
v.SetDefault("http_client.idle_connection_timeout_seconds", 60)
Expand Down
2 changes: 2 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func TestDefaults(t *testing.T) {
cmpInts(t, "auction_timeouts_ms.max", int(cfg.AuctionTimeouts.Max), 0)
cmpInts(t, "max_request_size", int(cfg.MaxRequestSize), 1024*256)
cmpInts(t, "host_cookie.ttl_days", int(cfg.HostCookie.TTL), 90)
cmpInts(t, "host_cookie.max_cookie_size_bytes", cfg.HostCookie.MaxCookieSizeBytes, 32768)
cmpStrings(t, "datacache.type", cfg.DataCache.Type, "dummy")
cmpStrings(t, "adapters.pubmatic.endpoint", cfg.Adapters[string(openrtb_ext.BidderPubmatic)].Endpoint, "http://hbopenbid.pubmatic.com/translator?source=prebid-server")
cmpInts(t, "currency_converter.fetch_interval_seconds", cfg.CurrencyConverter.FetchIntervalSeconds, 1800)
Expand All @@ -39,6 +40,7 @@ host_cookie:
domain: cookies.prebid.org
opt_out_url: http://prebid.org/optout
opt_in_url: http://prebid.org/optin
max_cookie_size_bytes: 32768
external_url: http://prebid-server.prebid.org/
host: prebid-server.prebid.org
port: 1234
Expand Down
2 changes: 1 addition & 1 deletion endpoints/cookie_sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func doConfigurablePost(body string, existingSyncs map[string]string, gdprHostCo
router.POST("/cookie_sync", endpoint)
req, _ := http.NewRequest("POST", "/cookie_sync", strings.NewReader(body))
if len(existingSyncs) > 0 {
pcs := usersync.NewPBSCookie()
pcs := usersync.NewPBSCookie(0)
for bidder, uid := range existingSyncs {
pcs.TrySync(bidder, uid)
}
Expand Down
6 changes: 3 additions & 3 deletions endpoints/setuid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func TestBadRequests(t *testing.T) {

func TestOptedOut(t *testing.T) {
request := httptest.NewRequest("GET", "/setuid?bidder=pubmatic&uid=123", nil)
cookie := usersync.NewPBSCookie()
cookie := usersync.NewPBSCookie(0)
cookie.SetPreference(false)
addCookie(request, cookie)
response := doRequest(request, true, false)
Expand Down Expand Up @@ -111,7 +111,7 @@ func assertBadRequest(t *testing.T, uri string, errMsg string) {
func makeRequest(uri string, existingSyncs map[string]string) *http.Request {
request := httptest.NewRequest("GET", uri, nil)
if len(existingSyncs) > 0 {
pbsCookie := usersync.NewPBSCookie()
pbsCookie := usersync.NewPBSCookie(0)
for family, value := range existingSyncs {
pbsCookie.TrySync(family, value)
}
Expand Down Expand Up @@ -146,7 +146,7 @@ func parseCookieString(t *testing.T, response *httptest.ResponseRecorder) *users
Name: "uids",
Value: res[1],
}
return usersync.ParsePBSCookie(&httpCookie)
return usersync.ParsePBSCookie(&httpCookie, 0)
}

func assertIntsMatch(t *testing.T, expected int, actual int) {
Expand Down
2 changes: 1 addition & 1 deletion exchange/legacy.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func (bidder *adaptedAdapter) toLegacyRequest(req *openrtb.BidRequest) (*pbs.PBS
domain = req.Site.Domain
}

cookie := usersync.NewPBSCookie()
cookie := usersync.NewPBSCookie(0)
if req.User != nil {
if req.User.BuyerUID != "" {
cookie.TrySync(bidder.adapter.Name(), req.User.BuyerUID)
Expand Down
47 changes: 36 additions & 11 deletions usersync/cookie.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/base64"
"encoding/json"
"errors"
"math"
"net/http"
"time"

Expand All @@ -14,6 +15,7 @@ import (
// DEFAULT_TTL is the default amount of time which a cookie is considered valid.
const DEFAULT_TTL = 14 * 24 * time.Hour
const UID_COOKIE_NAME = "uids"
const MAX_COOKIE_SIZE int = 1 << 15 // 32 KB

// customBidderTTLs stores rules about how long a particular UID sync is valid for each bidder.
// If a bidder does a cookie sync *without* listing a rule here, then the DEFAULT_TTL will be used.
Expand All @@ -30,9 +32,10 @@ var bidderToFamilyNames = map[openrtb_ext.BidderName]string{
// To get an instance of this from a request, use ParsePBSCookieFromRequest.
// To write an instance onto a response, use SetCookieOnResponse.
type PBSCookie struct {
uids map[string]uidWithExpiry
optOut bool
birthday *time.Time
uids map[string]uidWithExpiry
optOut bool
birthday *time.Time
maxSizeBytes int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

// uidWithExpiry bundles the UID with an Expiration date.
Expand All @@ -49,17 +52,17 @@ func ParsePBSCookieFromRequest(r *http.Request, cookie *config.HostCookie) *PBSC
if cookie.OptOutCookie.Name != "" {
optOutCookie, err1 := r.Cookie(cookie.OptOutCookie.Name)
if err1 == nil && optOutCookie.Value == cookie.OptOutCookie.Value {
pc := NewPBSCookie()
pc := NewPBSCookie(cookie.MaxCookieSizeBytes)
pc.SetPreference(false)
return pc
}
}
var parsed *PBSCookie
uidCookie, err2 := r.Cookie(UID_COOKIE_NAME)
if err2 == nil {
parsed = ParsePBSCookie(uidCookie)
parsed = ParsePBSCookie(uidCookie, cookie.MaxCookieSizeBytes)
} else {
parsed = NewPBSCookie()
parsed = NewPBSCookie(cookie.MaxCookieSizeBytes)
}
// Fixes #582
if uid, _, _ := parsed.GetUID(cookie.Family); uid == "" && cookie.CookieName != "" {
Expand All @@ -71,8 +74,8 @@ func ParsePBSCookieFromRequest(r *http.Request, cookie *config.HostCookie) *PBSC
}

// ParsePBSCookie parses the UserSync cookie from a raw HTTP cookie.
func ParsePBSCookie(uidCookie *http.Cookie) *PBSCookie {
pc := NewPBSCookie()
func ParsePBSCookie(uidCookie *http.Cookie, MaxSizeBytes int) *PBSCookie {
pc := NewPBSCookie(MaxSizeBytes)

j, err := base64.URLEncoding.DecodeString(uidCookie.Value)
if err != nil {
Expand All @@ -87,10 +90,11 @@ func ParsePBSCookie(uidCookie *http.Cookie) *PBSCookie {
}

// NewPBSCookie returns an empty PBSCookie
func NewPBSCookie() *PBSCookie {
func NewPBSCookie(MaxSizeBytes int) *PBSCookie {
return &PBSCookie{
uids: make(map[string]uidWithExpiry),
birthday: timestamp(),
uids: make(map[string]uidWithExpiry),
birthday: timestamp(),
maxSizeBytes: MaxSizeBytes,
}
}

Expand Down Expand Up @@ -162,9 +166,30 @@ 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 now time.Time = time.Now()

if domain != "" {
httpCookie.Domain = domain
}

var currSize int = len([]byte(httpCookie.String()))
for cookie.maxSizeBytes > 0 && currSize > cookie.maxSizeBytes {
var oldestElem string = ""
var oldestDate int64 = math.MaxInt64
for key, value := range cookie.uids {
timeUntilExpiration := value.Expires.Sub(now)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if timeUntilExpiration < time.Duration(oldestDate) {
oldestElem = key
oldestDate = int64(timeUntilExpiration)
}
}
delete(cookie.uids, oldestElem)
httpCookie = cookie.ToHTTPCookie(ttl)
if domain != "" {
httpCookie.Domain = domain
}
currSize = len([]byte(httpCookie.String()))
}
http.SetCookie(w, httpCookie)
}

Expand Down
Loading