Bid Response Validation

[AlexBVolcy]

This PR addresses this issue #1288

The goal of this PR is to validate bids that will be apart of the bid response, and remove them if they are invalid.

The criteria for the bids are based on Creative Size Max's for Banner and Secure Markup for Banner and Video

Creative Size Max's refer to a maxWidth and maxHeight that can be set by the host, and we can check a bid.W and bid.H to ensure they are less than these max's

Secure Markup refers to the bid.AdM value, and ensuring it doesn't have certain strings present in the AdM

Bid Response Validation can be toggled by both the host and account, through the Validations struct, that has a BannerCreativeMaxSize and SecureMarkup variables, that can be set to the string values enforce, warn, and skip.

If either of these variables are set to enforce or warn, then we run a validation for that specific criteria, and if the bid is invalid, we add a message to the debug array, we log metrics, and if the setting was enforce we also then remove the bid from the response.

If the value is set to skip, then we skip the validation altogether

The bulk of the validation logic is found in makeBid() in exchange.go

This Validations struct is also where the host can define the maxWidth and maxHeight allowed.

An account can also utilize the Validations struct to set their own values for BannerCreativeMaxSize and SecureMarkup, and the values the account set take precedence over the host.

Here are the new metrics that were added for this Validation for both Prometheus and Go Metrics (they have slightly different names)

Adapter Metrics: BidValidationCreativeSizeErrorMeter, BidValidationCreativeSizeWarnMeter, BidValidationSecureMarkupErrorMeter, BidValidationSecureMarkupWarnMeter

Account Metrics which are disabled by default through AccountAdapterDetails: bidValidationCreativeSizeMeter, bidValidationCreativeSizeWarnMeter, bidValidationSecureMarkupMeter, bidValidationSecureMarkupWarnMeter

Tests were provided for all of this work

[VeronikaSolovei9]

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"

[VeronikaSolovei9]

Can you think about better names for these variables?

[guscarreon]

The variable names are really descriptive but a bit long. Can we rename? For instance, BannerCreativeMaxSize to BannerMaxSize maybe? (up to you)

[guscarreon]

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

[guscarreon]

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

[AlexBVolcy]

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.

[guscarreon]

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

[AlexBVolcy]

Thanks for the feedback @guscarreon, I will be pushing a commit soon to address them. And in reference to re-naming variables, I want to keep the names consistent with Java, so I think they are fine to remain the same.

[VeronikaSolovei9]

LGTM

[guscarreon]

Pretty solid unit tests in TestValidateBidForBidResponse, TestMakeBidWithValidation and TestSetBidValidationStatus. I think it'll be helpful to assert whether or not the errors array got populated on end-to-end tests when both bid response validation is set to "warn" or "enforce". In the endpoints/openrtb2/ JSON tests we assert return code and the error message, do you think it'd be desirable to incorporate something similar to the exchangetests/? If not, maybe we can write an end-to-end test on the endpoints/openrtb2/ side. Let me know your thoughts.

21     ]
22   },
23   "expectedReturnCode": 400,
24   "expectedErrorMessage": "Invalid request"
25 }
endpoints/openrtb2/sample-requests/invalid-native/empty.json

[guscarreon]

Written as is, SetBidValidationStatus(account Validations) does not modify the method's object but the return value finalEnforcement. Therefore, we are not setting a different value to the host (aka e.bidValidationEnforcement) unless we either:

Option 1. Modify SetBidValidationStatus to be a "setter":

646 - func (host Validations) SetBidValidationStatus(account Validations) Validations {
    + func (host Validations) SetBidValidationStatus(account Validations) {
647 -     finalEnforcement := host
648       if len(account.BannerCreativeMaxSize) > 0 {
649 -         finalEnforcement.BannerCreativeMaxSize = account.BannerCreativeMaxSize
    +         host.BannerCreativeMaxSize = account.BannerCreativeMaxSize
650       }
651 -     return finalEnforcement
652   }
config/config.go

Option 2. Assign the return value to e.bidValidationEnforcement:

391       for _, warning := range r.Warnings {
392           generalWarning := openrtb_ext.ExtBidderMessage{
393               Code:    errortypes.ReadCode(warning),
394               Message: warning.Error(),
395           }
396           bidResponseExt.Warnings[openrtb_ext.BidderReservedGeneral] = append(bidResponseExt.Warnings[openrtb_ext.BidderReservedGeneral], generalWarning)
397       }
398
399       e.bidValidationEnforcement.SetBidValidationStatus(r.Account.Validations)
    +     e.bidValidationEnforcement = 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.return
403   }
exchange/exchange.go

Whichever pattern do you preffer is ok with me. The reason this bug was not getting reflected in the exchangetests/ JSON tests is because we are setting the bid response size restrictions in both sides. Here's were it gets set in PBS config:

2391     bidValidation := config.Validations{}
2392     if bidValidationEnforcement == config.ValidationEnforce || bidValidationEnforcement == config.ValidationWarn {
2393         bidValidation = config.Validations{
2394             BannerCreativeMaxSize: bidValidationEnforcement,
2395             MaxCreativeWidth:      100,
2396             MaxCreativeHeight:     100,
2397         }
2398     }
2399
2400     return &exchange{
2401         adapterMap:               bidderAdapters,
2402         me:                       metricsConf.NewMetricsEngine(&config.Configuration{}, openrtb_ext.CoreBidderNames(), nil),
2403 *-- 13 lines: cache:                    &wellBehavedCache{},------------------------------------------------------------------
2416         server:                   config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "Datacenter"},
2417         bidValidationEnforcement: bidValidation,
2418     }
2419 }

exchange/exchange_test.go

And then again at the account level:

2187     // Account Bid Validation Enforcement
2188     accountBidValidation := config.Validations{}
2189     if spec.BidValidationEnforcement == config.ValidationEnforce {
2190         accountBidValidation.BannerCreativeMaxSize = config.ValidationEnforce
2191     } else if spec.BidValidationEnforcement == config.ValidationWarn {
2192         accountBidValidation.BannerCreativeMaxSize = config.ValidationWarn
2193     }
2194
2195     auctionRequest := AuctionRequest{
2196         BidRequestWrapper: &openrtb_ext.RequestWrapper{BidRequest: &spec.IncomingRequest.OrtbRequest},
2197         Account: config.Account{
2198             ID:            "testaccount",
2199             EventsEnabled: spec.EventsEnabled,
2200             DebugAllow:    true,
2201             Validations:   accountBidValidation,
2202         },
2203         UserSyncs:     mockIdFetcher(spec.IncomingRequest.Usersyncs),
2204         ImpExtInfoMap: impExtInfoMap,
2205     }
2206
2207 *--  7 lines: if spec.StartTime > 0 {---------------------------------------------------------------------
2214
2215     bid, err := ex.HoldAuction(ctx, auctionRequest, debugLog)
exchange/exchange_test.go

Therefore, we weren't able to notice how the account validation settings where not getting copied to the validation enforcement object in line 399 of exchange.go. In order to correct, if you agree, I was thinking we could probably change the BidValidationEnforcement field from being a string, to a full config.Validations object along with the test framework modifications necessary.

4594   type exchangeSpec struct {
4595       GDPREnabled                bool                   `json:"gdpr_enabled"`
4596       IncomingRequest            exchangeRequest        `json:"incomingRequest"`
4597       OutgoingRequests           map[string]*bidderSpec `json:"outgoingRequests"`
4598       Response                   exchangeResponse       `json:"response,omitempty"`
4599       EnforceCCPA                bool                   `json:"enforceCcpa"`
4600       EnforceLMT                 bool                   `json:"enforceLmt"`
4601       AssumeGDPRApplies          bool                   `json:"assume_gdpr_applies"`
4602       DebugLog                   *DebugLog              `json:"debuglog,omitempty"`
4603       EventsEnabled              bool                   `json:"events_enabled,omitempty"`
4604       StartTime                  int64                  `json:"start_time_ms,omitempty"`
4605       BidIDGenerator             *mockBidIDGenerator    `json:"bidIDGenerator,omitempty"`
4606       RequestType                *metrics.RequestType   `json:"requestType,omitempty"`
4607       PassthroughFlag            bool                   `json:"passthrough_flag,omitempty"`
4608       HostSChainFlag             bool                   `json:"host_schain_flag,omitempty"`
4609 -     BidValidationEnforcement   string                 `json:"bid_validation_flag,omitempty"`
     +     pbsConfigBidValidation     config.Validations     `json:"config_bid_validations"`
     +     accountConfigBidValidation config.Validations     `json:"account_bid_validations"`
4610   }
exchange/exchange_test.go

Under this logic, we could probably have JSON test where the account bid validation config overrides that of PBS. An extra, is the ability to set the max values in the JSON test itself. For instance:

1   {
2 -     "bid_validation_flag": "enforce",
  +     "description": "Bid request validation enforcement at both system and account level. Expect PBS values being overriden by account-level values"
  +     "config_bid_validations": {
  +         "banner_creative_max_size": "warn",
  +         "secure_markup": "skip",
  +         "max_creative_width": 600,
  +         "max_creative_height": 480
  +     },
  +     "account_bid_validations": "enforce",
  +         "banner_creative_max_size": "enforce",
  +     },
3       "incomingRequest": {
4           "ortbRequest": {
5               "id": "some-request-id",
6               "site": {
exchange/exchangetest/account_validation_override_one_bid_rejected.json

[AlexBVolcy]

This is a great catch Gus! In my latest commit I'll have an update for these end-to-end tests to better check for this along with updating the SetBidValidationStatus to properly set the host config.

[AlexBVolcy]

I also updated the end-to-end tests in general to support checking that the error messages are being written properly.

[guscarreon]

Hi @AlexBVolcy there are conflicts that must be resolved

[VeronikaSolovei9]

Looks good.

[guscarreon]

It's looking pretty good. A couple more questions before approving

[guscarreon]

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?

[AlexBVolcy]

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

[guscarreon]

What do you think about naming this function SetBannerCreativeMaxSize instead? Do you think it'd be more descriptive?

[guscarreon]

Can we separate TestValidateBidForBidResponse into two tests? One that asserts validateBannerCreativeSize and the other one validateBidAdM separately?

[guscarreon]

Before approving I'd like to double check the expected values of the validateBidAdM unit tests: In issue #1288, @SyntaxNode commented in regards of the validateBidAdM function's behavior something like: "...For the other formats, we may want the validation to fail if there is any http:' or 'http%3A' (as proposed) but also that there must be a match on https:' or 'https%3A'. This will ensure urls without protocols defined are not considered secure...".

Does this mean we should expect every URL in the "AdM" field to be written at least once with a secure protocol? Or do we want at least one secure protocol URL in the entiriety of the "AdM" field? In other words, should we expect the test below to pass or fail?

Pass (at least one secure protocol in the entiriety of the AdM field)

  {
      desc: "AdM comes www.foo.com under 'http:' and www.bar.com under 'https:' because there's at least one URL with a secure protocol ('https://www.bar.com') test should pass",
      givenBid: &entities.PbsOrtbBid{
          Bid: &openrtb2.Bid{
              AdM: `<div id=\"container\"><a href=\"http:\/\/www.foo.com\/"><img src="some-source"\></a><a href=\"https:\/\/www.bar.com\/">buy now!</a></div>`,
          }
      },
      expectedToPass: true, //<-- pass validation
  },

Or fail? (every URL to be written at least once with a secure protocol)

  {
      desc: "AdM comes www.foo.com under 'http:' and www.bar.com under 'https:' expect validation to fail because of non-secure http://www.foo.com",
      givenBid: &entities.PbsOrtbBid{
          Bid: &openrtb2.Bid{
              AdM: `<div id=\"container\"><a href=\"http:\/\/www.foo.com\/"><img src="some-source"\></a><a href=\"https:\/\/www.bar.com\/">buy now!</a></div>`,
          }
      },
      expectedToPass: false, //<-- fail validation
  },

To my understanding of issue #1288, it is agreed upon that the following test should pass:

  {
      desc: "AdM comes with www.foo.com preceeded by  'http:' in one instance and by 'https:' in another instance. Expect validation to pass",
      givenBid: &entities.PbsOrtbBid{
          Bid: &openrtb2.Bid{
              AdM: `<div id=\"container\"><a href=\"http:\/\/www.foo.com\/"><img src="some-source"\></a><a href=\"https:\/\/www.foo.com\/">buy now!</a></div>`,
          }
      },
      expectedToPass: true,
  },

Thoughts?

[AlexBVolcy]

Addressed in a meeting but the validation works as follows: Validation fails if it contains 'http:' or 'http%3A' and does not contain either 'https:' or 'https%3A'

This is how the Java version handles this part, and the Go version will handle it the same way

[guscarreon]

"Oh no" 😄

[guscarreon]

Newly created CounterVecs need to be preloaded, inside metrics/prometheus/preload.go. For instance storedAccountErrors:

38       storedAccountErrors          *prometheus.CounterVec
metrics/prometheus/prometheus.go

Gets preloaded like:

84     preloadLabelValuesForCounter(m.storedAccountErrors, map[string][]string{
85         storedDataErrorLabel: storedDataErrorValues,
86     })
metrics/prometheus/preload.go

We need to do the same for adapterBidResponseValidationSizeError, adapterBidResponseValidationSizeWarn, adapterBidResponseSecureMarkupError, and adapterBidResponseSecureMarkupWarn

[guscarreon]

LGTM

[laurb9]

spec.BidValidationEnforcement does not appear to be set anywhere so this section does not run. I would expect it to be set either in the json file or maybe in line 2201.

There are actually some differences between the ext in json and the one output, that were uncovered when comparing spec.Response.Ext when present in a PR I am working on.

[AlexBVolcy]

Thank you for catching this! I used to have an old way of setting the validation enforcement, and then I updated how I set the object but forgot to update this line. I am going to push a tiny change to a new branch to address this, and open a new PR!