Skip to content

Commit

Permalink
Add an app.id blacklist to PBS and reject requests from it (#969)
Browse files Browse the repository at this point in the history
  • Loading branch information
guscarreon authored and mansinahar committed Aug 14, 2019
1 parent d0d0cd8 commit 59271c9
Show file tree
Hide file tree
Showing 28 changed files with 256 additions and 44 deletions.
12 changes: 12 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ type Configuration struct {
DefReqConfig DefReqConfig `mapstructure:"default_request"`

VideoStoredRequestRequired bool `mapstructure:"video_stored_request_required"`

// Array of blacklisted apps that is used to create the hash table BlacklistedAppMap so App.ID's can be instantly accessed.
BlacklistedApps []string `mapstructure:"blacklisted_apps,flow"`
BlacklistedAppMap map[string]bool
}

type HTTPClient struct {
Expand Down Expand Up @@ -393,6 +397,13 @@ func New(v *viper.Viper) (*Configuration, error) {
for i := 0; i < len(c.GDPR.NonStandardPublishers); i++ {
c.GDPR.NonStandardPublisherMap[c.GDPR.NonStandardPublishers[i]] = 1
}

// To look for a request's app_id in O(1) time, we fill this hash table located in the
// the BlacklistedApps field of the Configuration struct defined in this file
c.BlacklistedAppMap = make(map[string]bool)
for i := 0; i < len(c.BlacklistedApps); i++ {
c.BlacklistedAppMap[c.BlacklistedApps[i]] = true
}
return &c, nil
}

Expand Down Expand Up @@ -637,6 +648,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("default_request.type", "")
v.SetDefault("default_request.file.name", "")
v.SetDefault("default_request.alias_info", false)
v.SetDefault("blacklisted_apps", []string{""})

// Set environment variable support:
v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
Expand Down
10 changes: 10 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ adapters:
endpoint: http://test-bid.ybp.yahoo.com/bid/appnexuspbs
adkerneladn:
usersync_url: https://tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=
blacklisted_apps: ["spamAppID","sketchy-app-id"]
`)

var invalidAdapterEndpointConfig = []byte(`
Expand Down Expand Up @@ -176,6 +177,15 @@ func TestFullConfig(t *testing.T) {
_, found = cfg.GDPR.NonStandardPublisherMap["appnexus"]
cmpBools(t, "cfg.GDPR.NonStandardPublisherMap", found, false)

//Assert the NonStandardPublishers was correctly unmarshalled
cmpStrings(t, "blacklisted_apps", cfg.BlacklistedApps[0], "spamAppID")
cmpStrings(t, "blacklisted_apps", cfg.BlacklistedApps[1], "sketchy-app-id")

//Assert the BlacklistedAppMap hash table was built correctly
for i := 0; i < len(cfg.BlacklistedApps); i++ {
cmpBools(t, "cfg.BlacklistedAppMap", cfg.BlacklistedAppMap[cfg.BlacklistedApps[i]], true)
}

cmpStrings(t, "currency_converter.fetch_url", cfg.CurrencyConverter.FetchURL, "https://currency.prebid.org")
cmpInts(t, "currency_converter.fetch_interval_seconds", cfg.CurrencyConverter.FetchIntervalSeconds, 1800)
cmpStrings(t, "recaptcha_secret", cfg.RecaptchaSecret, "asdfasdfasdfasdf")
Expand Down
41 changes: 24 additions & 17 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,22 +271,6 @@ func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
}
}

impIDs := make(map[string]int, len(req.Imp))
for index := range req.Imp {
imp := &req.Imp[index]
if firstIndex, ok := impIDs[imp.ID]; ok {
errL = append(errL, fmt.Errorf(`request.imp[%d].id and request.imp[%d].id are both "%s". Imp IDs must be unique.`, firstIndex, index, imp.ID))
}
impIDs[imp.ID] = index
errs := deps.validateImp(imp, aliases, index)
if len(errs) > 0 {
errL = append(errL, errs...)
}
if fatalError(errs) {
return errL
}
}

if (req.Site == nil && req.App == nil) || (req.Site != nil && req.App != nil) {
errL = append(errL, errors.New("request.site or request.app must be defined, but not both."))
return errL
Expand All @@ -312,6 +296,22 @@ func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
return errL
}

impIDs := make(map[string]int, len(req.Imp))
for index := range req.Imp {
imp := &req.Imp[index]
if firstIndex, ok := impIDs[imp.ID]; ok {
errL = append(errL, fmt.Errorf(`request.imp[%d].id and request.imp[%d].id are both "%s". Imp IDs must be unique.`, firstIndex, index, imp.ID))
}
impIDs[imp.ID] = index
errs := deps.validateImp(imp, aliases, index)
if len(errs) > 0 {
errL = append(errL, errs...)
}
if fatalError(errs) {
return errL
}
}

return errL
}

Expand Down Expand Up @@ -789,6 +789,12 @@ func (deps *endpointDeps) validateApp(app *openrtb.App) error {
return nil
}

if app.ID != "" {
if _, found := deps.cfg.BlacklistedAppMap[app.ID]; found {
return &errortypes.BlacklistedApp{Message: fmt.Sprintf("Prebid-server does not process requests from App ID: %s", app.ID)}
}
}

if len(app.Ext) > 0 {
var a openrtb_ext.ExtApp
if err := json.Unmarshal(app.Ext, &a); err != nil {
Expand Down Expand Up @@ -1166,7 +1172,8 @@ func writeError(errs []error, w http.ResponseWriter) bool {
// Checks to see if an error in an error list is a fatal error
func fatalError(errL []error) bool {
for _, err := range errL {
if errortypes.DecodeError(err) != errortypes.BidderTemporarilyDisabledCode {
errCode := errortypes.DecodeError(err)
if errCode != errortypes.BidderTemporarilyDisabledCode || errCode == errortypes.BlacklistedAppCode {
return true
}
}
Expand Down
2 changes: 1 addition & 1 deletion endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func (gr *getResponseFromDirectory) doRequest(t *testing.T, requestData []byte)
// NewMetrics() will create a new go_metrics MetricsEngine, bypassing the need for a crafted configuration set to support it.
// As a side effect this gives us some coverage of the go_metrics piece of the metrics engine.
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
endpoint, _ := NewEndpoint(&nobidExchange{}, newParamsValidator(t), &mockStoredReqFetcher{}, empty_fetcher.EmptyFetcher{}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), disabledBidders, aliasJSON, bidderMap)
endpoint, _ := NewEndpoint(&nobidExchange{}, newParamsValidator(t), &mockStoredReqFetcher{}, empty_fetcher.EmptyFetcher{}, &config.Configuration{MaxRequestSize: maxSize, BlacklistedApps: []string{"spam_app"}, BlacklistedAppMap: map[string]bool{"spam_app": true}}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), disabledBidders, aliasJSON, bidderMap)

request := httptest.NewRequest("POST", "/openrtb2/auction", bytes.NewReader(requestData))
recorder := httptest.NewRecorder()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
{
"description": "This is a perfectly valid request except that it comes from a blacklisted App",
"message": "Invalid request: Prebid-server does not process requests from App ID: spam_app\n",

"requestPayload": {
"id": "some-request-id",
"user": {
"ext": {
"consent": "gdpr-consent-string",
"prebid": {
"buyeruids": {
"appnexus": "override-appnexus-id-in-cookie"
}
}
}
},
"app": {
"id": "spam_app"
},
"regs": {
"ext": {
"gdpr": 1
}
},
"imp": [
{
"id": "some-impression-id",
"banner": {
"format": [
{
"w": 300,
"h": 250
},
{
"w": 300,
"h": 600
}
]
},
"ext": {
"appnexus": {
"placementId": 10433394
},
"districtm": {
"placementId": 105
},
"rubicon": {
"accountId": 1001,
"siteId": 113932,
"zoneId": 535510
}
}
}
],
"tmax": 500,
"ext": {
"prebid": {
"aliases": {
"districtm": "appnexus"
},
"bidadjustmentfactors": {
"appnexus": 1.01,
"districtm": 0.98,
"rubicon": 0.99
},
"cache": {
"bids": {}
},
"targeting": {
"includewinners": false,
"pricegranularity": {
"precision": 2,
"ranges": [
{
"max": 20,
"increment": 0.10
}
]
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"mimes": []
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"hmax":50
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"hmin":50
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
"id": "imp-id",
"banner": null
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"wmax": 50
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"wmin":50
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
]
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
"format": [{}]
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
]
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
]
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
]
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
"id": "req-id",
"imp": [
{ }
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
},
"ext": {}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
"appnexus": "invalidParams"
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
}
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
]
}
}
]
],
"app": {
"id": "app_001"
}
}
}
Loading

0 comments on commit 59271c9

Please sign in to comment.