Validate native inputs #353

dbemiller · 2018-02-20T16:43:11Z

This fixes most of #218, and some of #295.

OpenRTB Native v1.2 support was added in mxmCherry/openrtb#20, but the 10.0.0 release of that library included some other breaking changes... so in the interest of moving this along, I'll save those changes for another PR.

Most of these changes are just refactoring so that my native test payloads don't add contribute to the tech debt of #295. I pulled many of the existing test payloads out into their own *.json files.

…pass.

…erage.

hhhjort · 2018-02-20T18:52:32Z

endpoints/openrtb2/amp_auction_test.go

+		"11":  json.RawMessage(validRequest(t, "app.json")),
+		"12":  json.RawMessage(validRequest(t, "timeout.json")),
+		"100": json.RawMessage("5"),
+		"101": json.RawMessage("5"),


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.

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.

hhhjort · 2018-02-20T19:22:02Z

endpoints/openrtb2/auction.go

+		if err := validateNativeAsset(assets[i], impIndex, i); err != nil {
+			return err
+		}
+		assets[i].ID = int64(i)


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.

yeah... to be honest I'm not 100% sure how to handle this. I saw the following:

Unique asset ID, assigned by exchange.

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.

I am fine with that then.

hhhjort · 2018-02-20T19:28:45Z

endpoints/openrtb2/auction.go

+}
+
+func validateNativeAssetData(data *nativeRequests.Data, impIndex int, assetIndex int) error {
+	if data.Type < 1 || data.Type > 11 {


12 is a valid data type.

Good catch! haha 12 was on a separate page in the spec, so I must have missed it.

hhhjort

Need to fix the allowed range of native data asset types.

hhhjort

LGTM

hhhjort

More test cases are good.

hhhjort

LGTM ... basically just removing the dupe directory since my last review.

dbemiller · 2018-03-07T16:24:48Z

been way longer than 5 days, so... merging.

* Upgraded the openrtb library to 9.2.0. * Moved invalid request payloads into their own directory. * Added a bad native request. * Added lots of invalid native bid payloads, to make tests fail. * Moved some files around. Implemented all the logic to make the tests pass. * Moved valid requests into directories. * Deleted some duplicate files. Added some new ones to beef up code coverage. * Removed some unnecessary code. * Added an issue number to the TODO statement. * Fixed the range on data asset types. * Fixed the amp auction tests, which were buggy before. * Moved tests from other PR into their own .json files. * Deleted duplicate directory... not sure how that got added.

Co-authored-by: Qili Chen <[email protected]@Qilis-MacBook-Pro.local>

dbemiller added 9 commits February 15, 2018 11:32

Upgraded the openrtb library to 9.2.0.

6b39d35

Moved invalid request payloads into their own directory.

491c0cf

Added a bad native request.

612ba9a

Added lots of invalid native bid payloads, to make tests fail.

c891fd3

Moved some files around. Implemented all the logic to make the tests …

bc730a5

…pass.

Moved valid requests into directories.

49de5fe

Deleted some duplicate files. Added some new ones to beef up code cov…

71978e3

…erage.

Removed some unnecessary code.

52fa7a9

Added an issue number to the TODO statement.

bf766b8

mkendall07 added the in progress label Feb 20, 2018

dbemiller requested a review from hhhjort February 20, 2018 16:43

dbemiller assigned hhhjort and nhedley Feb 20, 2018

hhhjort reviewed Feb 20, 2018

View reviewed changes

hhhjort requested changes Feb 20, 2018

View reviewed changes

dbemiller added 2 commits February 20, 2018 14:35

Fixed the range on data asset types.

fee8685

Fixed the amp auction tests, which were buggy before.

27741b5

dbemiller assigned ndhimehta and unassigned nhedley Feb 20, 2018

hhhjort previously approved these changes Feb 23, 2018

View reviewed changes

dbemiller added 2 commits February 26, 2018 14:48

Merged from master. Resolved conflicts.

0bc34ca

Moved tests from other PR into their own .json files.

f7b00a4

dbemiller dismissed hhhjort’s stale review via f7b00a4 February 26, 2018 19:56

hhhjort previously approved these changes Feb 26, 2018

View reviewed changes

ndhimehta assigned cindyhere33 Feb 27, 2018

dbemiller unassigned ndhimehta Feb 27, 2018

Merged from master. Fixed conflicts.

d7e8b40

dbemiller dismissed hhhjort’s stale review via d7e8b40 February 28, 2018 13:48

dbemiller mentioned this pull request Mar 1, 2018

Adding GDPR extensions #373

Merged

dbemiller added 2 commits March 2, 2018 09:00

Merged from master. Fixed conflict, and added test cases for GDPR stuff.

47c17cd

Deleted duplicate directory... not sure how that got added.

7adff99

hhhjort approved these changes Mar 7, 2018

View reviewed changes

dbemiller merged commit 8272666 into master Mar 7, 2018

dbemiller deleted the validate-native-inputs branch March 7, 2018 16:25

dbemiller mentioned this pull request Mar 9, 2018

OpenRTB Support: Native request validation #218

Closed

StarWindMoonCloud added a commit to ParticleMedia/prebid-server that referenced this pull request Jun 5, 2024

disable creative cache on msp (prebid#353)

b31e8c3

Co-authored-by: Qili Chen <[email protected]@Qilis-MacBook-Pro.local>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate native inputs #353

Validate native inputs #353

dbemiller commented Feb 20, 2018 •

edited

Loading

hhhjort Feb 20, 2018

dbemiller Feb 20, 2018 •

edited

Loading

hhhjort Feb 20, 2018 •

edited

Loading

dbemiller Feb 20, 2018

hhhjort Feb 20, 2018

hhhjort Feb 20, 2018

dbemiller Feb 20, 2018

hhhjort left a comment

hhhjort left a comment

hhhjort left a comment

hhhjort left a comment

dbemiller commented Mar 7, 2018

Validate native inputs #353

Validate native inputs #353

Conversation

dbemiller commented Feb 20, 2018 • edited Loading

hhhjort Feb 20, 2018

Choose a reason for hiding this comment

dbemiller Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

hhhjort Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

dbemiller Feb 20, 2018

Choose a reason for hiding this comment

hhhjort Feb 20, 2018

Choose a reason for hiding this comment

hhhjort Feb 20, 2018

Choose a reason for hiding this comment

dbemiller Feb 20, 2018

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

dbemiller commented Mar 7, 2018

dbemiller commented Feb 20, 2018 •

edited

Loading

dbemiller Feb 20, 2018 •

edited

Loading

hhhjort Feb 20, 2018 •

edited

Loading