-
Notifications
You must be signed in to change notification settings - Fork 765
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
Validate native inputs #353
Changes from 9 commits
6b39d35
491c0cf
612ba9a
c891fd3
bc730a5
49de5fe
71978e3
52fa7a9
bf766b8
fee8685
27741b5
0bc34ca
f7b00a4
d7e8b40
47c17cd
7adff99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import ( | |
"github.com/julienschmidt/httprouter" | ||
"github.com/mssola/user_agent" | ||
"github.com/mxmCherry/openrtb" | ||
nativeRequests "github.com/mxmCherry/openrtb/native/request" | ||
"github.com/prebid/prebid-server/config" | ||
"github.com/prebid/prebid-server/exchange" | ||
"github.com/prebid/prebid-server/openrtb_ext" | ||
|
@@ -255,10 +256,8 @@ func (deps *endpointDeps) validateImp(imp *openrtb.Imp, aliases map[string]strin | |
} | ||
} | ||
|
||
if imp.Native != nil { | ||
if imp.Native.Request == "" { | ||
return fmt.Errorf("request.imp[%d].native.request must be a JSON encoded string conforming to the openrtb 1.2 Native spec", index) | ||
} | ||
if err := fillAndValidateNative(imp.Native, index); err != nil { | ||
return err | ||
} | ||
|
||
if err := validatePmp(imp.PMP, index); err != nil { | ||
|
@@ -300,6 +299,183 @@ func validateBanner(banner *openrtb.Banner, impIndex int) error { | |
return nil | ||
} | ||
|
||
// fillAndValidateNative validates the request, and assigns the Asset IDs as recommended by the Native v1.2 spec. | ||
func fillAndValidateNative(n *openrtb.Native, impIndex int) error { | ||
if n == nil { | ||
return nil | ||
} | ||
|
||
var nativePayload nativeRequests.Request | ||
if err := json.Unmarshal(json.RawMessage(n.Request), &nativePayload); err != nil { | ||
return err | ||
} | ||
|
||
if err := validateNativeContext(nativePayload.Context, impIndex); err != nil { | ||
return err | ||
} | ||
if err := validateNativePlacementType(nativePayload.PlcmtType, impIndex); err != nil { | ||
return err | ||
} | ||
if err := fillAndValidateNativeAssets(nativePayload.Assets, impIndex); err != nil { | ||
return err | ||
} | ||
|
||
// TODO #218: Validate eventtrackers once mxmcherry/openrtb has been updated to support Native v1.2 | ||
|
||
serialized, err := json.Marshal(nativePayload) | ||
if err != nil { | ||
return err | ||
} | ||
encoded, err := json.Marshal(string(serialized)) | ||
if err != nil { | ||
return err | ||
} | ||
n.Request = string(encoded) | ||
return nil | ||
} | ||
|
||
func validateNativeContext(c nativeRequests.ContextType, impIndex int) error { | ||
if c < 1 || c > 3 { | ||
return fmt.Errorf("request.imp[%d].native.request.context must be in the range [1, 3]. Got %d", impIndex, c) | ||
} | ||
return nil | ||
} | ||
|
||
func validateNativePlacementType(pt nativeRequests.PlacementType, impIndex int) error { | ||
if pt < 1 || pt > 4 { | ||
return fmt.Errorf("request.imp[%d].native.request.plcmttype must be in the range [1, 4]. Got %d", impIndex, pt) | ||
} | ||
return nil | ||
} | ||
|
||
func fillAndValidateNativeAssets(assets []nativeRequests.Asset, impIndex int) error { | ||
if len(assets) < 1 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets must be an array containing at least one object.", impIndex) | ||
} | ||
|
||
for i := 0; i < len(assets); i++ { | ||
// Per the OpenRTB spec docs, this is a "unique asset ID, assigned by exchange. Typically a counter for the array" | ||
// To avoid conflict with the Request, we'll return a 400 if the Request _did_ define this ID, | ||
// and then populate it as the spec suggests. | ||
if err := validateNativeAsset(assets[i], impIndex, i); err != nil { | ||
return err | ||
} | ||
assets[i].ID = int64(i) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the spec: "Typically a counter for the array." This enforces that it will be a counter. I don't know of any use cases for ID being anything else, so its probably good. But potentially will break something that stretches the spec here. Oh, I see below that we are changing the spec that we don't want anyone else filling asset IDs. So any breaking changes should not be a surprise, but still could potentially block some funky implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah... to be honest I'm not 100% sure how to handle this. I saw the following:
Since Prebid is the exchange, in this context, I don't think we're breaking the spec here. We just decided that the "typical" implementation is the one we'll be using. I decided to return 4xx on IDs sent by the Publisher just to prevent all sorts of nasty logic/confusion. As a general rule, we never overwrite anything which the Publisher gives us. So if we allowed IDs, then we'd have to make sure that the ones we added weren't duplicates, which gets... messy. Decided to just reject those requests to keep things simple. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with that then. |
||
} | ||
return nil | ||
} | ||
|
||
func validateNativeAsset(asset nativeRequests.Asset, impIndex int, assetIndex int) error { | ||
if asset.ID != 0 { | ||
return fmt.Errorf(`request.imp[%d].native.request.assets[%d].id must not be defined. Prebid Server will set this automatically, using the index of the asset in the array as the ID.`, impIndex, assetIndex) | ||
} | ||
|
||
foundType := false | ||
|
||
if asset.Title != nil { | ||
foundType = true | ||
if err := validateNativeAssetTitle(asset.Title, impIndex, assetIndex); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if asset.Img != nil { | ||
if foundType { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d] must define at most one of {title, img, video, data}", impIndex, assetIndex) | ||
} | ||
foundType = true | ||
if err := validateNativeAssetImg(asset.Img, impIndex, assetIndex); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if asset.Video != nil { | ||
if foundType { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d] must define at most one of {title, img, video, data}", impIndex, assetIndex) | ||
} | ||
foundType = true | ||
if err := validateNativeAssetVideo(asset.Video, impIndex, assetIndex); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
if asset.Data != nil { | ||
if foundType { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d] must define at most one of {title, img, video, data}", impIndex, assetIndex) | ||
} | ||
foundType = true | ||
if err := validateNativeAssetData(asset.Data, impIndex, assetIndex); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validateNativeAssetTitle(title *nativeRequests.Title, impIndex int, assetIndex int) error { | ||
if title.Len < 1 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].title.len must be a positive integer", impIndex, assetIndex) | ||
} | ||
return nil | ||
} | ||
|
||
func validateNativeAssetImg(image *nativeRequests.Image, impIndex int, assetIndex int) error { | ||
// Note that w, wmin, h, and hmin cannot be negative because these variables use unsigned ints. | ||
// Those fail during the standard json.Unmarshal() call. | ||
if image.W == 0 && image.WMin == 0 { | ||
return fmt.Errorf(`request.imp[%d].native.request.assets[%d].img must contain at least one of "w" or "wmin"`, impIndex, assetIndex) | ||
} | ||
if image.H == 0 && image.HMin == 0 { | ||
return fmt.Errorf(`request.imp[%d].native.request.assets[%d].img must contain at least one of "h" or "hmin"`, impIndex, assetIndex) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validateNativeAssetVideo(video *nativeRequests.Video, impIndex int, assetIndex int) error { | ||
if len(video.MIMEs) < 1 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].video.mimes must be an array with at least one MIME type.", impIndex, assetIndex) | ||
} | ||
if video.MinDuration < 1 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].video.minduration must be a positive integer.", impIndex, assetIndex) | ||
} | ||
if video.MaxDuration < 1 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].video.maxduration must be a positive integer.", impIndex, assetIndex) | ||
} | ||
if err := validateNativeVideoProtocols(video.Protocols, impIndex, assetIndex); err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validateNativeAssetData(data *nativeRequests.Data, impIndex int, assetIndex int) error { | ||
if data.Type < 1 || data.Type > 11 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 12 is a valid data type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! haha 12 was on a separate page in the spec, so I must have missed it. |
||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].data.type must in the range [1, 11]. Got %d.", impIndex, assetIndex, data.Type) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validateNativeVideoProtocols(protocols []nativeRequests.Protocol, impIndex int, assetIndex int) error { | ||
if len(protocols) < 1 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].video.protocols must be an array with at least one element", impIndex, assetIndex) | ||
} | ||
for i := 0; i < len(protocols); i++ { | ||
if err := validateNativeVideoProtocol(protocols[i], impIndex, assetIndex, i); err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func validateNativeVideoProtocol(protocol nativeRequests.Protocol, impIndex int, assetIndex int, protocolIndex int) error { | ||
if protocol < 0 || protocol > 10 { | ||
return fmt.Errorf("request.imp[%d].native.request.assets[%d].video.protocols[%d] must be in the range [1, 10]. Got %d", impIndex, assetIndex, protocolIndex, protocol) | ||
} | ||
return nil | ||
} | ||
|
||
func validateFormat(format *openrtb.Format, impIndex int, formatIndex int) error { | ||
usesHW := format.W != 0 || format.H != 0 | ||
usesRatios := format.WMin != 0 || format.WRatio != 0 || format.HRatio != 0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that was a bug in the original AMP tests, should have been testing all the invalid requests. Not terribly important though, since this shares code with openrtb, and the invalid requests should be handled there anyway. I had failed to advance the index in
invalidRequests[]
on each line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah... I see. Yeah, I didn't pay much attention to the semantics of these tests, but I'll clean them up here so that they make sense again.