Address comments, tweak adm logic

prebid · Jan 9, 2023 · 812e532 · 812e532
1 parent 197ee65
commit 812e532
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 21 deletions.
diff --git a/config/config.go b/config/config.go
@@ -646,7 +646,7 @@ const (
 	ValidationSkip    string = "skip"
 )
 
-func (host *Validations) SetBidValidationStatus(account Validations) {
+func (host *Validations) SetBannerCreativeMaxSize(account Validations) {
 	if len(account.BannerCreativeMaxSize) > 0 {
 		host.BannerCreativeMaxSize = account.BannerCreativeMaxSize
 	}

diff --git a/exchange/exchange.go b/exchange/exchange.go
@@ -406,7 +406,7 @@ 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(r.Account.Validations)
+	e.bidValidationEnforcement.SetBannerCreativeMaxSize(r.Account.Validations)
 
 	// Build the response
 	bidResponse, err := e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequestWrapper.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, r.ImpExtInfoMap, r.PubID, errs)
@@ -1298,7 +1298,7 @@ func (e exchange) validateBidAdM(bid *entities.PbsOrtbBid, bidResponseExt *openr
 	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])) {
+	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
 		errorMessage := setErrorMessageSecureMarkup(validationType)
 		bidSecureMarkupError := openrtb_ext.ExtBidderMessage{

diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go
@@ -4238,7 +4238,7 @@ func TestCallSignHeader(t *testing.T) {
 
 }
 
-func TestValidateBidForBidResponse(t *testing.T) {
+func TestValidateBannerCreativeSize(t *testing.T) {
 	exchange := exchange{bidValidationEnforcement: config.Validations{MaxCreativeWidth: 100, MaxCreativeHeight: 100},
 		me: metricsConf.NewMetricsEngine(&config.Configuration{}, openrtb_ext.CoreBidderNames(), nil, nil),
 	}
@@ -4249,49 +4249,92 @@ func TestValidateBidForBidResponse(t *testing.T) {
 		givenBidderName             string
 		givenPubID                  string
 		expectedBannerCreativeValid bool
-		expectedBidAdMValid         bool
 	}{
 		{
-			description:                 "The dimensions and adm of the bid are invalid, expect false",
-			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 200, H: 200, AdM: "http://domain.com/invalid"}},
+			description:                 "The dimensions are invalid, both values bigger than the max",
+			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 200, H: 200}},
 			givenBidResponseExt:         &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
 			givenBidderName:             "bidder",
 			givenPubID:                  "1",
 			expectedBannerCreativeValid: false,
-			expectedBidAdMValid:         false,
 		},
 		{
-			description:                 "The dimensions and adm of the bid are valid, expect true",
-			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 50, H: 50, AdM: "https://domain.com/valid"}},
+			description:                 "The width is invalid, height is valid, the dimensions as a whole are invalid",
+			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 200, H: 50}},
 			givenBidResponseExt:         &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
 			givenBidderName:             "bidder",
 			givenPubID:                  "1",
-			expectedBannerCreativeValid: true,
-			expectedBidAdMValid:         true,
+			expectedBannerCreativeValid: false,
 		},
 		{
-			description:                 "Dimensions are valid. The adm is invalid. Expect true/false",
-			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 50, H: 50, AdM: "http://domain.com/invalid"}},
+			description:                 "The width is valid, height is invalid, the dimensions as a whole are invalid",
+			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 50, H: 200}},
 			givenBidResponseExt:         &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
 			givenBidderName:             "bidder",
 			givenPubID:                  "1",
-			expectedBannerCreativeValid: true,
-			expectedBidAdMValid:         false,
+			expectedBannerCreativeValid: false,
 		},
 		{
-			description:                 "Dimensions are invalid. The adm is valid. Expect false/true",
-			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 200, H: 200, AdM: "https://domain.com/valid"}},
+			description:                 "Both width and height are valid, the dimensions are valid",
+			givenBid:                    &entities.PbsOrtbBid{Bid: &openrtb2.Bid{W: 50, H: 50}},
 			givenBidResponseExt:         &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
 			givenBidderName:             "bidder",
 			givenPubID:                  "1",
-			expectedBannerCreativeValid: false,
-			expectedBidAdMValid:         true,
+			expectedBannerCreativeValid: true,
 		},
 	}
 	for _, test := range testCases {
 		acutalBannerCreativeValid := exchange.validateBannerCreativeSize(test.givenBid, test.givenBidResponseExt, openrtb_ext.BidderName(test.givenBidderName), test.givenPubID, "enforce")
 		assert.Equal(t, test.expectedBannerCreativeValid, acutalBannerCreativeValid)
+	}
+}
 
+func TestValidateBidAdM(t *testing.T) {
+	exchange := exchange{bidValidationEnforcement: config.Validations{MaxCreativeWidth: 100, MaxCreativeHeight: 100},
+		me: metricsConf.NewMetricsEngine(&config.Configuration{}, openrtb_ext.CoreBidderNames(), nil, nil),
+	}
+	testCases := []struct {
+		description         string
+		givenBid            *entities.PbsOrtbBid
+		givenBidResponseExt *openrtb_ext.ExtBidResponse
+		givenBidderName     string
+		givenPubID          string
+		expectedBidAdMValid bool
+	}{
+		{
+			description:         "The adm of the bid contains insecure string and no secure string, adm is invalid",
+			givenBid:            &entities.PbsOrtbBid{Bid: &openrtb2.Bid{AdM: "http://domain.com/invalid"}},
+			givenBidResponseExt: &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
+			givenBidderName:     "bidder",
+			givenPubID:          "1",
+			expectedBidAdMValid: false,
+		},
+		{
+			description:         "The adm has both an insecure and secure string defined and therefore the adm is valid",
+			givenBid:            &entities.PbsOrtbBid{Bid: &openrtb2.Bid{AdM: "http://www.foo.com https://www.bar.com"}},
+			givenBidResponseExt: &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
+			givenBidderName:     "bidder",
+			givenPubID:          "1",
+			expectedBidAdMValid: true,
+		},
+		{
+			description:         "The adm has both an insecure and secure string defined and therefore the adm is valid",
+			givenBid:            &entities.PbsOrtbBid{Bid: &openrtb2.Bid{AdM: "http%3A//www.foo.com https%3A//www.bar.com"}},
+			givenBidResponseExt: &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
+			givenBidderName:     "bidder",
+			givenPubID:          "1",
+			expectedBidAdMValid: true,
+		},
+		{
+			description:         "The adm of the bid are valid with a secure string",
+			givenBid:            &entities.PbsOrtbBid{Bid: &openrtb2.Bid{AdM: "https://domain.com/valid"}},
+			givenBidResponseExt: &openrtb_ext.ExtBidResponse{Errors: make(map[openrtb_ext.BidderName][]openrtb_ext.ExtBidderMessage)},
+			givenBidderName:     "bidder",
+			givenPubID:          "1",
+			expectedBidAdMValid: true,
+		},
+	}
+	for _, test := range testCases {
 		actualBidAdMValid := exchange.validateBidAdM(test.givenBid, test.givenBidResponseExt, openrtb_ext.BidderName(test.givenBidderName), test.givenPubID, "enforce")
 		assert.Equal(t, test.expectedBidAdMValid, actualBidAdMValid)
 
@@ -4422,7 +4465,7 @@ func TestSetBidValidationStatus(t *testing.T) {
 		},
 	}
 	for _, test := range testCases {
-		test.givenHost.SetBidValidationStatus(test.givenAccount)
+		test.givenHost.SetBannerCreativeMaxSize(test.givenAccount)
 		assert.Equal(t, test.expected, test.givenHost)
 	}
 }