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 1 commit
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
53 changes: 46 additions & 7 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,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,7 +814,7 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
return []error{err}
}

if err := validateSChains(reqPrebid.SChains); err != nil {
if err := validateSChains(reqPrebid.SChains); err != nil { //!!! delete this? should be in req.Source.SChain
return []error{err}
}

Expand All @@ -823,7 +827,7 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
}
}

if err := mapSChains(req); err != nil {
if err := mapSChains(req); err != nil { // do we need this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can get rid of this. It should now be handled in moveSupplyChainFrom24To25 from convert_up.go.

return []error{err}
}

Expand Down Expand Up @@ -1279,7 +1283,7 @@ func (deps *endpointDeps) validateUser(req *openrtb_ext.RequestWrapper, aliases
// Check if the buyeruids are valid
prebid := userExt.GetPrebid()
if prebid != nil {
if len(prebid.BuyerUIDs) < 1 {
if len(prebid.BuyerUIDs) < 1 { // how to replace validation to user.buyeruid(string)
return append(errL, errors.New(`request.user.ext.prebid requires a "buyeruids" property with at least one ID defined. If none exist, then request.user.ext.prebid should not be defined.`))
}
for bidderName := range prebid.BuyerUIDs {
Expand All @@ -1294,9 +1298,8 @@ 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))
Expand Down Expand Up @@ -1370,7 +1373,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 +1483,29 @@ func fillChannel(reqWrapper *openrtb_ext.RequestWrapper, isAmp bool) error {

}

func (deps *endpointDeps) validateSourceSChain(req *openrtb_ext.RequestWrapper) error {
if 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
2 changes: 1 addition & 1 deletion openrtb_ext/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type ExtRequestPrebid struct {
MultiBid []*ExtMultiBid `json:"multibid,omitempty"`
MultiBidMap map[string]ExtMultiBid `json:"-"`
Passthrough json.RawMessage `json:"passthrough,omitempty"`
SChains []*ExtRequestPrebidSChain `json:"schains,omitempty"`
SChains []*ExtRequestPrebidSChain `json:"schains,omitempty"` //!!!! delete from req.ext
Sdk *ExtRequestSdk `json:"sdk,omitempty"`
Server *ExtRequestPrebidServer `json:"server,omitempty"`
StoredRequest *ExtStoredRequest `json:"storedrequest,omitempty"`
Expand Down
4 changes: 2 additions & 2 deletions openrtb_ext/request_wrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,11 +420,11 @@ func (rw *RequestWrapper) Clone() *RequestWrapper {
type UserExt struct {
ext map[string]json.RawMessage
extDirty bool
consent *string
consent *string // delete this? now it's a user.consent
consentDirty bool
prebid *ExtUserPrebid
prebidDirty bool
eids *[]openrtb2.EID
eids *[]openrtb2.EID // delete this? now it's user.eids
eidsDirty bool
consentedProvidersSettingsIn *ConsentedProvidersSettingsIn
consentedProvidersSettingsInDirty bool
Expand Down
2 changes: 1 addition & 1 deletion openrtb_ext/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type ExtUser struct {

// ExtUserPrebid defines the contract for bidrequest.user.ext.prebid
type ExtUserPrebid struct {
BuyerUIDs map[string]string `json:"buyeruids,omitempty"`
BuyerUIDs map[string]string `json:"buyeruids,omitempty"` // delete this? now it's user.buyeruid
}

type ConsentedProvidersSettingsIn struct {
Expand Down