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

Request validation upgrade to 2.6 locations #3906

Merged
merged 6 commits into from
Sep 25, 2024
Merged

Request validation upgrade to 2.6 locations #3906

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c4fcd13
Initial request validation upgrade to 2.6 locations
VeronikaSolovei9 Sep 8, 2024
10e1faf
Initial request validation upgrade to 2.6 locations - req.BidRequest.…
VeronikaSolovei9 Sep 10, 2024
a8e5a44
Clean up
VeronikaSolovei9 Sep 10, 2024
590e56b
Clean up and unit tests
VeronikaSolovei9 Sep 11, 2024
3a3c492
Code review fixes
VeronikaSolovei9 Sep 19, 2024
04e460f
Redundant validation revert
VeronikaSolovei9 Sep 24, 2024
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
107 changes: 51 additions & 56 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import (
"github.com/prebid/prebid-server/v2/prebid_cache_client"
"github.com/prebid/prebid-server/v2/privacy/ccpa"
"github.com/prebid/prebid-server/v2/privacy/lmt"
"github.com/prebid/prebid-server/v2/schain"
"github.com/prebid/prebid-server/v2/stored_requests"
"github.com/prebid/prebid-server/v2/stored_requests/backends/empty_fetcher"
"github.com/prebid/prebid-server/v2/stored_responses"
Expand Down Expand Up @@ -784,6 +783,10 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
}
}

if err := deps.validateSourceSChain(req); err != nil {
return []error{err}
}

var requestAliases map[string]string
reqExt, err := req.GetRequestExt()
if err != nil {
Expand All @@ -810,10 +813,6 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
return []error{err}
}

if err := validateSChains(reqPrebid.SChains); err != nil {
return []error{err}
}

if err := deps.validateEidPermissions(reqPrebid.Data, requestAliases); err != nil {
return []error{err}
}
Expand All @@ -823,10 +822,6 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
}
}

if err := mapSChains(req); err != nil {
return []error{err}
}

if err := validateOrFillChannel(req, isAmp); err != nil {
return []error{err}
}
Expand Down Expand Up @@ -930,32 +925,6 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
return errL
}

// mapSChains maps an schain defined in an ORTB 2.4 location (req.ext.schain) to the ORTB 2.5 location
// (req.source.ext.schain) if no ORTB 2.5 schain (req.source.ext.schain, req.ext.prebid.schains) exists.
Comment on lines -933 to -934
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we need to update the upconvert logic for schain to only perform an upgrade from 2.4 to 2.5 if req.ext.prebid.schains does not exist. req.ext.prebid.schains is a 2.5 location so it should take precedence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline. I'll take a look to see if this is a problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The order of precedence from highest to lowest is as follows:
req.ext.prebid.schains
req.source.schain (2.6)
req.source.ext.schain (2.5)
req.ext.schain (2.4)

We're upconverting right after we parse the request which will handle the ORTB standard locations resulting in a value in the 2.6 location if an schain exists in the 2.4, 2.5 or 2.6 location. Downstream we will use the schain writer to potentially overwrite the 2.6 location with a value from the req.ext.prebid.schains.

No additional change is needed to the upconvert logic.

// An ORTB 2.4 schain is always deleted from the 2.4 location regardless of whether an ORTB 2.5 schain exists.
func mapSChains(req *openrtb_ext.RequestWrapper) error {
reqExt, err := req.GetRequestExt()
if err != nil {
return fmt.Errorf("req.ext is invalid: %v", err)
}
sourceExt, err := req.GetSourceExt()
if err != nil {
return fmt.Errorf("source.ext is invalid: %v", err)
}

reqExtSChain := reqExt.GetSChain()
reqExt.SetSChain(nil)

if reqPrebid := reqExt.GetPrebid(); reqPrebid != nil && reqPrebid.SChains != nil {
return nil
} else if sourceExt.GetSChain() != nil {
return nil
} else if reqExtSChain != nil {
sourceExt.SetSChain(reqExtSChain)
}
return nil
}

func validateAndFillSourceTID(req *openrtb_ext.RequestWrapper, generateRequestID bool, hasStoredBidRequest bool, isAmp bool) error {
if req.Source == nil {
req.Source = &openrtb2.Source{}
Expand Down Expand Up @@ -1012,11 +981,6 @@ func (deps *endpointDeps) validateBidAdjustmentFactors(adjustmentFactors map[str
return nil
}

func validateSChains(sChains []*openrtb_ext.ExtRequestPrebidSChain) error {
_, err := schain.BidderToPrebidSChains(sChains)
return err
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this needs to be deleted

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to keep this. I believe the source.ext.schain 2.5 field is what is being migrated to source.schain in 2.6. This logic refers to a different schain feature that has no 2.6 equivalent where bidder-specific schain nodes may be specified at source.ext.prebid.schains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored


func (deps *endpointDeps) validateEidPermissions(prebid *openrtb_ext.ExtRequestPrebidData, requestAliases map[string]string) error {
if prebid == nil {
return nil
Expand Down Expand Up @@ -1294,21 +1258,20 @@ func (deps *endpointDeps) validateUser(req *openrtb_ext.RequestWrapper, aliases
}

// Check Universal User ID
eids := userExt.GetEid()
if eids != nil {
eidsValue := *eids
if req.User.EIDs != nil {
eidsValue := req.User.EIDs
for eidIndex, eid := range eidsValue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: I think you can iterate over req.User.EIDs directly and get rid of eidsValue since we are just reading the eids from the slice.

if eid.Source == "" {
return append(errL, fmt.Errorf("request.user.ext.eids[%d] missing required field: \"source\"", eidIndex))
return append(errL, fmt.Errorf("request.user.eids[%d] missing required field: \"source\"", eidIndex))
}

if len(eid.UIDs) == 0 {
return append(errL, fmt.Errorf("request.user.ext.eids[%d].uids must contain at least one element or be undefined", eidIndex))
return append(errL, fmt.Errorf("request.user.eids[%d].uids must contain at least one element or be undefined", eidIndex))
}

for uidIndex, uid := range eid.UIDs {
if uid.ID == "" {
return append(errL, fmt.Errorf("request.user.ext.eids[%d].uids[%d] missing required field: \"id\"", eidIndex, uidIndex))
return append(errL, fmt.Errorf("request.user.eids[%d].uids[%d] missing required field: \"id\"", eidIndex, uidIndex))
}
}
}
Expand Down Expand Up @@ -1338,16 +1301,12 @@ func validateRegs(req *openrtb_ext.RequestWrapper, gpp gpplib.GppContainer) []er
WarningCode: errortypes.InvalidPrivacyConsentWarningCode})
}
}
regsExt, err := req.GetRegExt()
if err != nil {
return append(errL, fmt.Errorf("request.regs.ext is invalid: %v", err))
}

gdpr := regsExt.GetGDPR()
if gdpr != nil && *gdpr != 0 && *gdpr != 1 {
return append(errL, errors.New("request.regs.ext.gdpr must be either 0 or 1"))
if req.BidRequest.Regs.GDPR != nil {
reqGDPR := req.BidRequest.Regs.GDPR
if reqGDPR != nil && *reqGDPR != 0 && *reqGDPR != 1 {
return append(errL, errors.New("request.regs.gdpr must be either 0 or 1"))
}
}

return errL
}

Expand All @@ -1370,7 +1329,20 @@ func validateDevice(device *openrtb2.Device) error {
if device.Geo != nil && device.Geo.Accuracy < 0 {
return errors.New("request.device.geo.accuracy must be a positive number")
}

if device.SUA != nil {
if len(device.SUA.Browsers) > 0 {
for i, browser := range device.SUA.Browsers {
if len(browser.Brand) == 0 {
return fmt.Errorf("request.device.sua.browsers[%d].brand cannot be empty", i)
}
}
}
if device.SUA.Platform != nil {
if len(device.SUA.Platform.Brand) == 0 {
return errors.New("request.device.sua.platform.brand cannot be empty")
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did this validation logic come from? Is this new 2.6 behavior?

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is from the OpenRTB 2.6 specification, paragraph 3.2.30.
device.SUA.Browser[].Brand is required if device.SUA.Browser[] is present.
device.SUA.Platform.Brand is required if device.SUA.Platform is present.

We discussed offline that we can omit validation for the fields we don't use in PBS core, so I'm open to delete this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this validation. PBS's approach is to just validate the schema (attributes & datatypes) for fields we do not use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

return nil
}

Expand Down Expand Up @@ -1467,6 +1439,29 @@ func fillChannel(reqWrapper *openrtb_ext.RequestWrapper, isAmp bool) error {

}

func (deps *endpointDeps) validateSourceSChain(req *openrtb_ext.RequestWrapper) error {
if req.Source == nil || req.Source.SChain == nil {
return nil
}
sChain := req.Source.SChain

if len(sChain.Ver) == 0 {
return errors.New("request.source.schain.ver cannot be empty")
}
if len(sChain.Nodes) == 0 {
return errors.New("request.source.schain.nodes cannot be empty")
}
for i, node := range sChain.Nodes {
if len(node.ASI) == 0 {
return fmt.Errorf("request.source.schain.nodes[%d].asi cannot be empty", i)
}
if len(node.SID) == 0 {
return fmt.Errorf("request.source.schain.nodes[%d].sid cannot be empty", i)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did these validation rules come from https://github.com/InteractiveAdvertisingBureau/openrtb/blob/main/supplychainobject.md? There are a couple of other fields marked as required in this spec.
I don't think we had these validation rules before so I'm wondering if there is a reason for that. I'll look through the closed issues to see if I can figure it out.

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 found this in OpenRTB 2.6 spec, paragraph 3.2.25 and 3.2.26.

You are probably referring to this required field: Complete int8. This field bus not a pointer and will always present in this struct. Specification also says it can 0 = no, 1 = yes. Do we want to validate this value is 0 or 1 only? I missed this part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this validation. PBS's approach is to just validate the schema (attributes & datatypes) for fields we do not use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

return nil
}

func sanitizeRequest(r *openrtb_ext.RequestWrapper, ipValidator iputil.IPValidator) {
if r.Device != nil {
if ip, ver := iputil.ParseIP(r.Device.IP); ip == nil || ver != iputil.IPv4 || !ipValidator.IsValid(ip, ver) {
Expand Down
177 changes: 0 additions & 177 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2889,183 +2889,6 @@ func TestValidateSourceTID(t *testing.T) {
assert.NotEmpty(t, req.Source.TID, "Expected req.Source.TID to be filled with a randomly generated UID")
}

func TestSChainInvalid(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should restore the prebid specific per-bidder schain logic as mentioned in another comment so this test should be restored as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored

deps := &endpointDeps{
fakeUUIDGenerator{},
&nobidExchange{},
ortb.NewRequestValidator(openrtb_ext.BuildBidderMap(), map[string]string{}, mockBidderParamValidator{}),
&mockStoredReqFetcher{},
empty_fetcher.EmptyFetcher{},
empty_fetcher.EmptyFetcher{},
&config.Configuration{},
&metricsConfig.NilMetricsEngine{},
analyticsBuild.New(&config.Analytics{}),
map[string]string{},
false,
[]byte{},
openrtb_ext.BuildBidderMap(),
nil,
nil,
hardcodedResponseIPValidator{response: true},
empty_fetcher.EmptyFetcher{},
hooks.EmptyPlanBuilder{},
nil,
openrtb_ext.NormalizeBidderName,
}

ui := int64(1)
req := openrtb2.BidRequest{
ID: "anyRequestID",
Imp: []openrtb2.Imp{
{
ID: "anyImpID",
Banner: &openrtb2.Banner{
W: &ui,
H: &ui,
},
Ext: json.RawMessage(`{"appnexus": {"placementId": 5667}}`),
},
},
Site: &openrtb2.Site{
ID: "anySiteID",
},
Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["appnexus"],"schain":{"complete":1,"nodes":[{"asi":"directseller1.com","sid":"00001","rid":"BidRequest1","hp":1}],"ver":"1.0"}}, {"bidders":["appnexus"],"schain":{"complete":1,"nodes":[{"asi":"directseller2.com","sid":"00002","rid":"BidRequest2","hp":1}],"ver":"1.0"}}]}}`),
}

errL := deps.validateRequest(nil, nil, &openrtb_ext.RequestWrapper{BidRequest: &req}, false, false, nil, false)

expectedError := errors.New("request.ext.prebid.schains contains multiple schains for bidder appnexus; it must contain no more than one per bidder.")
assert.ElementsMatch(t, errL, []error{expectedError})
}

func TestMapSChains(t *testing.T) {
const seller1SChain string = `"schain":{"complete":1,"nodes":[{"asi":"directseller1.com","sid":"00001","rid":"BidRequest1","hp":1}],"ver":"1.0"}`
const seller2SChain string = `"schain":{"complete":2,"nodes":[{"asi":"directseller2.com","sid":"00002","rid":"BidRequest2","hp":2}],"ver":"2.0"}`

seller1SChainUnpacked := openrtb2.SupplyChain{
Complete: 1,
Nodes: []openrtb2.SupplyChainNode{{
ASI: "directseller1.com",
SID: "00001",
RID: "BidRequest1",
HP: openrtb2.Int8Ptr(1),
}},
Ver: "1.0",
}

tests := []struct {
description string
bidRequest openrtb2.BidRequest
wantReqExtSChain *openrtb2.SupplyChain
wantSourceExtSChain *openrtb2.SupplyChain
wantError bool
}{
{
description: "invalid req.ext",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{"prebid":{"schains":invalid}}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantError: true,
},
{
description: "invalid source.ext",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{"schain":invalid}}`),
},
},
wantError: true,
},
{
description: "req.ext.prebid.schains, req.source.ext.schain and req.ext.schain are nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: nil,
},
{
description: "req.ext.prebid.schains is not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["appnexus"],` + seller1SChain + `}]}}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: nil,
},
{
description: "req.source.ext is not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{` + seller1SChain + `}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: &seller1SChainUnpacked,
},
{
description: "req.ext.schain is not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{` + seller1SChain + `}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: &seller1SChainUnpacked,
},
{
description: "req.source.ext.schain and req.ext.schain are not nil",
bidRequest: openrtb2.BidRequest{
Ext: json.RawMessage(`{` + seller2SChain + `}`),
Source: &openrtb2.Source{
Ext: json.RawMessage(`{` + seller1SChain + `}`),
},
},
wantReqExtSChain: nil,
wantSourceExtSChain: &seller1SChainUnpacked,
},
}

for _, test := range tests {
reqWrapper := openrtb_ext.RequestWrapper{
BidRequest: &test.bidRequest,
}

err := mapSChains(&reqWrapper)

if test.wantError {
assert.NotNil(t, err, test.description)
} else {
assert.Nil(t, err, test.description)

reqExt, err := reqWrapper.GetRequestExt()
if err != nil {
assert.Fail(t, "Error getting request ext from wrapper", test.description)
}
reqExtSChain := reqExt.GetSChain()
assert.Equal(t, test.wantReqExtSChain, reqExtSChain, test.description)

sourceExt, err := reqWrapper.GetSourceExt()
if err != nil {
assert.Fail(t, "Error getting source ext from wrapper", test.description)
}
sourceExtSChain := sourceExt.GetSChain()
assert.Equal(t, test.wantSourceExtSChain, sourceExtSChain, test.description)
}
}
}

func TestSearchAccountID(t *testing.T) {
// Correctness for lookup within Publisher object left to TestGetAccountID
// This however tests the expected lookup paths in outer site, app and dooh
Expand Down
Loading