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.

Sign up for GitHub

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

Bid Response Validation #2450

Merged
merged 15 commits into from
Jan 11, 2023
Merged

Bid Response Validation #2450

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5adc1aa
Bid Resposnse Validation Impl
AlexBVolcy Nov 14, 2022
1f314fa
Updated validating logic and tests
AlexBVolcy Nov 15, 2022
cda79ba
Merge branch 'master' into bid_response_validation
AlexBVolcy Nov 15, 2022
f9ce0aa
Update variable names
AlexBVolcy Nov 16, 2022
594cae2
Function refactor, variable name changes
AlexBVolcy Nov 21, 2022
72961e9
Merge branch 'master' into bid_response_validation
AlexBVolcy Nov 21, 2022
97f3541
Address all feedback, update tests, tweak logic
AlexBVolcy Dec 6, 2022
56e4d99
Added more end to end tests
AlexBVolcy Dec 9, 2022
d60533c
Tweak end-to-end test structure, fix bug
AlexBVolcy Dec 9, 2022
308ae18
Merge branch 'master' into bid_response_validation
AlexBVolcy Jan 3, 2023
677626a
Fix failed test
AlexBVolcy Jan 3, 2023
f824a89
Merge branch 'master' into bid_response_validation
AlexBVolcy Jan 5, 2023
197ee65
Merge branch 'master' into bid_response_validation
AlexBVolcy Jan 6, 2023
812e532
Address comments, tweak adm logic
AlexBVolcy Jan 9, 2023
761247f
Preload new adapter metrics
AlexBVolcy Jan 10, 2023
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
1 change: 1 addition & 0 deletions config/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type Account struct {
TruncateTargetAttribute *int `mapstructure:"truncate_target_attr" json:"truncate_target_attr"`
AlternateBidderCodes *openrtb_ext.ExtAlternateBidderCodes `mapstructure:"alternatebiddercodes" json:"alternatebiddercodes"`
Hooks AccountHooks `mapstructure:"hooks" json:"hooks"`
Validations Validations `mapstructure:"validations" json:"validations"`
}

// CookieSync represents the account-level defaults for the cookie sync endpoint.
Expand Down
20 changes: 19 additions & 1 deletion config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ type Configuration struct {
// Refers to main.go `configFileName` constant
BidderInfos BidderInfos `mapstructure:"adapters"`
// Hooks provides a way to specify hook execution plan for specific endpoints and stages
Hooks Hooks `mapstructure:"hooks"`
Hooks Hooks `mapstructure:"hooks"`
Validations Validations `mapstructure:"validations"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that these validations only validate the bid response, unless we plan to include more validations in this field in the future, should we rename to bidResponseValidations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

II think we should keep the name Validations as that is what was specified in the issue along with what’s present in the Java version

}

const MIN_COOKIE_SIZE_BYTES = 500
Expand Down Expand Up @@ -629,6 +630,19 @@ type TimeoutNotification struct {
FailOnly bool `mapstructure:"fail_only"`
}

type Validations struct {
BannerCreativeMaxSize string `mapstructure:"banner_creative_max_size" json:"banner_creative_max_size"`
SecureMarkup string `mapstructure:"secure_markup" json:"secure_markup"`
MaxCreativeWidth int64 `mapstructure:"max_creative_width" json:"max_creative_width"`
MaxCreativeHeight int64 `mapstructure:"max_creative_height" json:"max_creative_height"`
}

const (
ValidationEnforce string = "enforce"
ValidationWarn string = "warn"
ValidationSkip string = "skip"
)

func (cfg *TimeoutNotification) validate(errs []error) []error {
if cfg.SamplingRate < 0.0 || cfg.SamplingRate > 1.0 {
errs = append(errs, fmt.Errorf("debug.timeout_notification.sampling_rate must be positive and not greater than 1.0. Got %f", cfg.SamplingRate))
Expand Down Expand Up @@ -819,6 +833,10 @@ func SetupViper(v *viper.Viper, filename string, bidderInfos BidderInfos) {
v.SetDefault("host_cookie.ttl_days", 90)
v.SetDefault("host_cookie.max_cookie_size_bytes", 0)
v.SetDefault("host_schain_node", nil)
v.SetDefault("validations.banner_creative_max_size", "skip")
v.SetDefault("validations.secure_markup", "skip")
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably use ValidationSkip that was declared above rather than the hardcoded version

v.SetDefault("validations.max_creative_size.height", 0)
v.SetDefault("validations.max_creative_size.width", 0)
v.SetDefault("http_client.max_connections_per_host", 0) // unlimited
v.SetDefault("http_client.max_idle_connections", 400)
v.SetDefault("http_client.max_idle_connections_per_host", 10)
Expand Down
13 changes: 13 additions & 0 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ func TestDefaults(t *testing.T) {
cmpNils(t, "host_schain_node", cfg.HostSChainNode)
cmpStrings(t, "datacenter", cfg.DataCenter, "")
cmpBools(t, "hooks.enabled", cfg.Hooks.Enabled, false)
cmpStrings(t, "validations.banner_creative_max_size", cfg.Validations.BannerCreativeMaxSize, "skip")
cmpStrings(t, "validations.secure_markup", cfg.Validations.SecureMarkup, "skip")
cmpInts(t, "validations.max_creative_width", int(cfg.Validations.MaxCreativeWidth), 0)
cmpInts(t, "validations.max_creative_height", int(cfg.Validations.MaxCreativeHeight), 0)

//Assert purpose VendorExceptionMap hash tables were built correctly
expectedTCF2 := TCF2{
Expand Down Expand Up @@ -405,6 +409,11 @@ host_schain_node:
sid: "00001"
rid: "BidRequest"
hp: 1
validations:
banner_creative_max_size: "skip"
secure_markup: "skip"
max_creative_width: 0
max_creative_height: 0
experiment:
adscert:
mode: inprocess
Expand Down Expand Up @@ -493,6 +502,10 @@ func TestFullConfig(t *testing.T) {
cmpStrings(t, "host_schain_node.rid", cfg.HostSChainNode.RID, "BidRequest")
cmpInt8s(t, "host_schain_node.hp", cfg.HostSChainNode.HP, &int8One)
cmpStrings(t, "datacenter", cfg.DataCenter, "1")
cmpStrings(t, "validations.banner_creative_max_size", cfg.Validations.BannerCreativeMaxSize, "skip")
cmpStrings(t, "validations.secure_markup", cfg.Validations.SecureMarkup, "skip")
cmpInts(t, "validations.max_creative_width", int(cfg.Validations.MaxCreativeWidth), 0)
cmpInts(t, "validations.max_creative_height", int(cfg.Validations.MaxCreativeHeight), 0)

//Assert the NonStandardPublishers was correctly unmarshalled
assert.Equal(t, []string{"pub1", "pub2"}, cfg.GDPR.NonStandardPublishers, "gdpr.non_standard_publishers")
Expand Down
122 changes: 95 additions & 27 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,23 +53,24 @@ type IdFetcher interface {
}

type exchange struct {
adapterMap map[openrtb_ext.BidderName]AdaptedBidder
bidderInfo config.BidderInfos
bidderToSyncerKey map[string]string
me metrics.MetricsEngine
cache prebid_cache_client.Client
cacheTime time.Duration
gdprPermsBuilder gdpr.PermissionsBuilder
tcf2ConfigBuilder gdpr.TCF2ConfigBuilder
currencyConverter *currency.RateConverter
externalURL string
gdprDefaultValue gdpr.Signal
privacyConfig config.Privacy
categoriesFetcher stored_requests.CategoryFetcher
bidIDGenerator BidIDGenerator
hostSChainNode *openrtb2.SupplyChainNode
adsCertSigner adscert.Signer
server config.Server
adapterMap map[openrtb_ext.BidderName]AdaptedBidder
bidderInfo config.BidderInfos
bidderToSyncerKey map[string]string
me metrics.MetricsEngine
cache prebid_cache_client.Client
cacheTime time.Duration
gdprPermsBuilder gdpr.PermissionsBuilder
tcf2ConfigBuilder gdpr.TCF2ConfigBuilder
currencyConverter *currency.RateConverter
externalURL string
gdprDefaultValue gdpr.Signal
privacyConfig config.Privacy
categoriesFetcher stored_requests.CategoryFetcher
bidIDGenerator BidIDGenerator
hostSChainNode *openrtb2.SupplyChainNode
adsCertSigner adscert.Signer
server config.Server
bidValidationEnforcement config.Validations
}

// Container to pass out response ext data from the GetAllBids goroutines back into the main thread
Expand Down Expand Up @@ -145,10 +146,11 @@ func NewExchange(adapters map[openrtb_ext.BidderName]AdaptedBidder, cache prebid
GDPR: cfg.GDPR,
LMT: cfg.LMT,
},
bidIDGenerator: &bidIDGenerator{cfg.GenerateBidID},
hostSChainNode: cfg.HostSChainNode,
adsCertSigner: adsCertSigner,
server: config.Server{ExternalUrl: cfg.ExternalURL, GvlID: cfg.GDPR.HostVendorID, DataCenter: cfg.DataCenter},
bidIDGenerator: &bidIDGenerator{cfg.GenerateBidID},
hostSChainNode: cfg.HostSChainNode,
adsCertSigner: adsCertSigner,
server: config.Server{ExternalUrl: cfg.ExternalURL, GvlID: cfg.GDPR.HostVendorID, DataCenter: cfg.DataCenter},
bidValidationEnforcement: cfg.Validations,
}
}

Expand Down Expand Up @@ -394,8 +396,10 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
bidResponseExt.Warnings[openrtb_ext.BidderReservedGeneral] = append(bidResponseExt.Warnings[openrtb_ext.BidderReservedGeneral], generalWarning)
}

e.bidValidationEnforcement = setBidValidationStatus(e.bidValidationEnforcement, r.Account.Validations)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Maybe we can simplify setBidValidationStatus if it becomes a method of the config.Validations type. Do you agree?

397       }
398
399 -     e.bidValidationEnforcement = setBidValidationStatus(e.bidValidationEnforcement, r.Account.Validations)
    +     e.bidValidationEnforcement.setBidValidationStatus(r.Account.Validations)
400
401       // Build the response
402       return e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequestWrapper.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, r.ImpExtInfoMap, r.PubID, errs)
403   }
exchange/exchange.go


// Build the response
return e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequestWrapper.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, r.ImpExtInfoMap, errs)
return e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequestWrapper.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, r.ImpExtInfoMap, r.PubID, errs)
}

func (e *exchange) parseGDPRDefaultValue(bidRequest *openrtb2.BidRequest) gdpr.Signal {
Expand Down Expand Up @@ -693,7 +697,7 @@ func errsToBidderWarnings(errs []error) []openrtb_ext.ExtBidderMessage {
}

// This piece takes all the bids supplied by the adapters and crafts an openRTB response to send back to the requester
func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_ext.BidderName, adapterSeatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, bidRequest *openrtb2.BidRequest, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, bidResponseExt *openrtb_ext.ExtBidResponse, returnCreative bool, impExtInfoMap map[string]ImpExtInfo, errList []error) (*openrtb2.BidResponse, error) {
func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_ext.BidderName, adapterSeatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, bidRequest *openrtb2.BidRequest, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, bidResponseExt *openrtb_ext.ExtBidResponse, returnCreative bool, impExtInfoMap map[string]ImpExtInfo, pubID string, errList []error) (*openrtb2.BidResponse, error) {
bidResponse := new(openrtb2.BidResponse)
var err error

Expand All @@ -709,7 +713,7 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_
for a, adapterSeatBids := range adapterSeatBids {
//while processing every single bib, do we need to handle categories here?
if adapterSeatBids != nil && len(adapterSeatBids.bids) > 0 {
sb := e.makeSeatBid(adapterSeatBids, a, adapterExtra, auc, returnCreative, impExtInfoMap)
sb := e.makeSeatBid(adapterSeatBids, a, adapterExtra, auc, returnCreative, impExtInfoMap, bidResponseExt, pubID)
seatBids = append(seatBids, *sb)
bidResponse.Cur = adapterSeatBids.currency
}
Expand Down Expand Up @@ -1009,26 +1013,42 @@ func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pb

// Return an openrtb seatBid for a bidder
// BuildBidResponse is responsible for ensuring nil bid seatbids are not included
func (e *exchange) makeSeatBid(adapterBid *pbsOrtbSeatBid, adapter openrtb_ext.BidderName, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, returnCreative bool, impExtInfoMap map[string]ImpExtInfo) *openrtb2.SeatBid {
func (e *exchange) makeSeatBid(adapterBid *pbsOrtbSeatBid, adapter openrtb_ext.BidderName, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, returnCreative bool, impExtInfoMap map[string]ImpExtInfo, bidResponseExt *openrtb_ext.ExtBidResponse, pubID string) *openrtb2.SeatBid {
seatBid := &openrtb2.SeatBid{
Seat: adapter.String(),
Group: 0, // Prebid cannot support roadblocking
}

var errList []error
seatBid.Bid, errList = e.makeBid(adapterBid.bids, auc, returnCreative, impExtInfoMap)
seatBid.Bid, errList = e.makeBid(adapterBid.bids, auc, returnCreative, impExtInfoMap, bidResponseExt, adapter, pubID)
if len(errList) > 0 {
adapterExtra[adapter].Errors = append(adapterExtra[adapter].Errors, errsToBidderErrors(errList)...)
}

return seatBid
}

func (e *exchange) makeBid(bids []*pbsOrtbBid, auc *auction, returnCreative bool, impExtInfoMap map[string]ImpExtInfo) ([]openrtb2.Bid, []error) {
func (e *exchange) makeBid(bids []*pbsOrtbBid, auc *auction, returnCreative bool, impExtInfoMap map[string]ImpExtInfo, bidResponseExt *openrtb_ext.ExtBidResponse, adapter openrtb_ext.BidderName, pubID string) ([]openrtb2.Bid, []error) {
result := make([]openrtb2.Bid, 0, len(bids))
errs := make([]error, 0, 1)

for _, bid := range bids {
if e.bidValidationEnforcement.BannerCreativeMaxSize == config.ValidationEnforce && bid.bidType == openrtb_ext.BidTypeBanner {
if !e.validateBannerCreativeSize(bid, bidResponseExt, adapter, pubID) {
continue // Don't add bid to result
}
} else if e.bidValidationEnforcement.BannerCreativeMaxSize == config.ValidationWarn && bid.bidType == openrtb_ext.BidTypeBanner {
e.validateBannerCreativeSize(bid, bidResponseExt, adapter, pubID)
}

if e.bidValidationEnforcement.SecureMarkup == config.ValidationEnforce && (bid.bidType == openrtb_ext.BidTypeBanner || bid.bidType == openrtb_ext.BidTypeVideo) {
if !e.validateBidAdM(bid, bidResponseExt, adapter, pubID) {
continue // Don't add bid to result
}
} else if e.bidValidationEnforcement.SecureMarkup == config.ValidationWarn && (bid.bidType == openrtb_ext.BidTypeBanner || bid.bidType == openrtb_ext.BidTypeVideo) {
e.validateBidAdM(bid, bidResponseExt, adapter, pubID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Under both config.ValidationEnforce and config.ValidationWarn we end up doing the e.validateBannerCreativeSize(..) and e.validateBidAdM(..) calls. The code below might be an option to simplify:

if e.bidValidationEnforcement.doBidValidation() {
    switch bidType {
        case banner:
            hasValidSize := e.validateBannerCreativeSize(bid, bidResponseExt, adapter, pubID)
            if !hasValidSize && e.bidValidationEnforcement.BannerCreativeMaxSize == config.ValidationEnforce {
                continue
            }
            fallthrough
        case video:
            isAdmValid := e.validateBidAdM(bid, bidResponseExt, adapter, pubID)
            if !isAdmValid && e.bidValidationEnforcement.SecureMarkup == config.ValidationEnforce {
                continue
            }
    }
}
exchange/exchange.go

Under the assumptions above, the doBidValidation() method could probably look like:

func (v Validations) doBidValidation() bool {
    return v.BannerCreativeMaxSize != v.ValidationSkip || v.SecureMarkup != v.ValidationSkip
}

But it's up to you. Let me know your thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate this suggestion, but I think I prefer the current way it's structured. It feels a bit more readable to me, but I can also bring up this suggestion at team time.


bidExtPrebid := &openrtb_ext.ExtBidPrebid{
DealPriority: bid.dealPriority,
DealTierSatisfied: bid.dealTierSatisfied,
Expand Down Expand Up @@ -1242,3 +1262,51 @@ func isAdsCertEnabled(experiment *openrtb_ext.Experiment, info config.BidderInfo
bidderAdsCertEnabled := info.Experiment.AdsCert.Enabled
return requestAdsCertEnabled && bidderAdsCertEnabled
}

func (e exchange) validateBannerCreativeSize(bid *pbsOrtbBid, bidResponseExt *openrtb_ext.ExtBidResponse, adapter openrtb_ext.BidderName, pubID string) bool {
if bid.bid.W > e.bidValidationEnforcement.MaxCreativeWidth || bid.bid.H > e.bidValidationEnforcement.MaxCreativeHeight {
// Add error to debug array
bidCreativeMaxSizeError := openrtb_ext.ExtBidderMessage{
Code: errortypes.BadInputErrorCode,
Message: "bidResponse rejected: size WxH",
}
bidResponseExt.Errors[adapter] = append(bidResponseExt.Errors[adapter], bidCreativeMaxSizeError)

// Log Metrics
e.me.RecordBidValidationCreativeSizeError(adapter, pubID)

return false
}
return true
}

func (e exchange) validateBidAdM(bid *pbsOrtbBid, bidResponseExt *openrtb_ext.ExtBidResponse, adapter openrtb_ext.BidderName, pubID string) bool {
invalidAdM := []string{"http:", "http%3A"}
requiredAdM := []string{"https:", "https%3A"}

if (strings.Contains(bid.bid.AdM, invalidAdM[0]) || strings.Contains(bid.bid.AdM, invalidAdM[1])) && (!strings.Contains(bid.bid.AdM, requiredAdM[0]) || !strings.Contains(bid.bid.AdM, requiredAdM[1])) {
// Add error to debug array
bidSecureMarkupError := openrtb_ext.ExtBidderMessage{
Code: errortypes.BadInputErrorCode,
Message: "bidResponse rejected: insecure creative in secure context",
}
bidResponseExt.Errors[adapter] = append(bidResponseExt.Errors[adapter], bidSecureMarkupError)

// Log Metrics
e.me.RecordBidValidationSecureMarkupError(adapter, pubID)

return false
}
return true
}

func setBidValidationStatus(host config.Validations, account config.Validations) config.Validations {
finalEnforcement := host
if len(account.BannerCreativeMaxSize) > 0 {
finalEnforcement.BannerCreativeMaxSize = account.BannerCreativeMaxSize
}
if len(account.SecureMarkup) > 0 {
finalEnforcement.SecureMarkup = account.SecureMarkup
}
return finalEnforcement
}
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

func setBidValidationStatus(host config.Validations, account config.Validations) config.Validations {
	finalEnforcement := host
	if len(account.BannerCreativeMaxSize) > 0 {
		finalEnforcement.BannerCreativeMaxSize = account.BannerCreativeMaxSize
	}
	if len(account.SecureMarkup) > 0 {
		finalEnforcement.SecureMarkup = account.SecureMarkup
	}
	return finalEnforcement
}

unit tests pass.

Unless you need to check values specifically and they can have other values from "enforce", "warn" and "skip"

Loading