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

Add an app.id blacklist to PBS and reject requests from it #969

Merged
merged 19 commits into from
Aug 14, 2019
Merged
Show file tree
Hide file tree
Changes from 17 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
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"`

// PBS-418 array of blacklisted apps and its correpondent hash table to 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 @@ -635,6 +646,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{""})
Copy link
Contributor

Choose a reason for hiding this comment

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

@guscarreon Do we need a slice with empty string as default? I mean why can't it be just an empty slice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the flow keyword was added, the BlacklistedApps field in line 55 below expects an array of strings.

 21 type Configuration struct {
 22     ExternalURL string     `mapstructure:"external_url"`
 23 +-- 28 lines: Host        string     `mapstructure:"host"`---------------------------------------
 51
 52     VideoStoredRequestRequired bool `mapstructure:"video_stored_request_required"`
 53
 54     // PBS-418 array of blacklisted apps and its correpondent hash table to be instantly accessed.
 55     BlacklistedApps   []string `mapstructure:"blacklisted_apps,flow"`
 56     BlacklistedAppMap map[string]bool
 57 }
 58

Therefore, I figured it was better to add a slice with an empty string because other string fields of the Configuration struct add an empty string instead of a nil string as default. Take the Host field for example, in config/config.go its default is set to an empty string as well as StatusResponse and RecaptchaSecret to name a few:

 21 type Configuration struct {
 22     ExternalURL string     `mapstructure:"external_url"`
 23     Host        string     `mapstructure:"host"`
 24     Port        int        `mapstructure:"port"`
 25     Client      HTTPClient `mapstructure:"http_client"`
 26     AdminPort   int        `mapstructure:"admin_port"`
 27     EnableGzip  bool       `mapstructure:"enable_gzip"`
 28     // StatusResponse is the string which will be returned by the /status endpoint when things are OK.
 29     // If empty, it will return a 204 with no content.
 30     StatusResponse  string             `mapstructure:"status_response"`
 31     AuctionTimeouts AuctionTimeouts    `mapstructure:"auction_timeouts_ms"`
 32     CacheURL        Cache              `mapstructure:"cache"`
 33     RecaptchaSecret string             `mapstructure:"recaptcha_secret"`
 34     HostCookie      HostCookie         `mapstructure:"host_cookie"`
 35     Metrics         Metrics            `mapstructure:"metrics"`
 36 +--444 lines: DataCache       DataCache          `mapstructure:"datacache"`---------------------
480 func SetupViper(v *viper.Viper, filename string) {
481     if filename != "" {
482         v.SetConfigName(filename)
483         v.AddConfigPath(".")
484         v.AddConfigPath("/etc/config")
485     }
486     // Fixes #475: Some defaults will be set just so they are accessible via environment variables
487     // (basically so viper knows they exist)
488     v.SetDefault("external_url", "http://localhost:8000")
489     v.SetDefault("host", "")
490     v.SetDefault("port", 8000)
491     v.SetDefault("admin_port", 6060)
492     v.SetDefault("enable_gzip", false)
493     v.SetDefault("status_response", "")
494     v.SetDefault("auction_timeouts_ms.default", 0)
495     v.SetDefault("auction_timeouts_ms.max", 0)
496     v.SetDefault("cache.scheme", "")
497     v.SetDefault("cache.host", "")
498     v.SetDefault("cache.query", "")
499     v.SetDefault("cache.expected_millis", 10)
500     v.SetDefault("cache.default_ttl_seconds.banner", 0)
501     v.SetDefault("cache.default_ttl_seconds.video", 0)
502     v.SetDefault("cache.default_ttl_seconds.native", 0)
503     v.SetDefault("cache.default_ttl_seconds.audio", 0)
504     v.SetDefault("recaptcha_secret", "")
505     v.SetDefault("host_cookie.domain", "")
506     v.SetDefault("host_cookie.family", "")


// 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