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

Core: ORTB video params validation (work on dupe) #11970

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
26 changes: 2 additions & 24 deletions libraries/ortbConverter/processors/video.js
Original file line number Diff line number Diff line change
@@ -1,37 +1,15 @@
import {deepAccess, isEmpty, logWarn, mergeDeep, sizesToSizeTuples, sizeTupleToRtbSize} from '../../../src/utils.js';
import {VIDEO} from '../../../src/mediaTypes.js';

// parameters that share the same name & semantics between pbjs adUnits and imp.video
const ORTB_VIDEO_PARAMS = new Set([
'pos',
'placement',
'plcmt',
'api',
'mimes',
'protocols',
'playbackmethod',
'minduration',
'maxduration',
'w',
'h',
'startdelay',
'placement',
'linearity',
'skip',
'skipmin',
'skipafter',
'minbitrate',
'maxbitrate',
'delivery',
'playbackend'
]);
import {ORTB_VIDEO_PARAMS} from '../../../src/video.js';

export function fillVideoImp(imp, bidRequest, context) {
if (context.mediaType && context.mediaType !== VIDEO) return;

const videoParams = deepAccess(bidRequest, 'mediaTypes.video');
if (!isEmpty(videoParams)) {
const video = Object.fromEntries(
// Parameters that share the same name & semantics between pbjs adUnits and imp.video
Object.entries(videoParams)
.filter(([name]) => ORTB_VIDEO_PARAMS.has(name))
);
Expand Down
60 changes: 10 additions & 50 deletions modules/adagioBidAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,15 @@ import {
getDNT,
getWindowSelf,
isArray,
isArrayOfNums,
isFn,
isInteger,
isNumber,
isStr,
logError,
logInfo,
logWarn,
} from '../src/utils.js';
import { getRefererInfo, parseDomain } from '../src/refererDetection.js';
import { OUTSTREAM } from '../src/video.js';
import { OUTSTREAM, validateOrtbVideoFields } from '../src/video.js';
import { Renderer } from '../src/Renderer.js';
import { _ADAGIO } from '../libraries/adagioUtils/adagioUtils.js';
import { config } from '../src/config.js';
Expand All @@ -40,39 +38,6 @@ export const BB_RENDERER_URL = `https://${BB_PUBLICATION}.bbvms.com/r/$RENDERER.

const CURRENCY = 'USD';

// This provide a whitelist and a basic validation of OpenRTB 2.5 options used by the Adagio SSP.
// Accept all options but 'protocol', 'companionad', 'companiontype', 'ext'
// https://www.iab.com/wp-content/uploads/2016/03/OpenRTB-API-Specification-Version-2-5-FINAL.pdf
export const ORTB_VIDEO_PARAMS = {
'mimes': (value) => Array.isArray(value) && value.length > 0 && value.every(v => typeof v === 'string'),
'minduration': (value) => isInteger(value),
'maxduration': (value) => isInteger(value),
'protocols': (value) => isArrayOfNums(value),
'w': (value) => isInteger(value),
'h': (value) => isInteger(value),
'startdelay': (value) => isInteger(value),
'placement': (value) => {
logWarn(LOG_PREFIX, 'The OpenRTB video param `placement` is deprecated and should not be used anymore.');
return isInteger(value)
},
'plcmt': (value) => isInteger(value),
'linearity': (value) => isInteger(value),
'skip': (value) => [1, 0].includes(value),
'skipmin': (value) => isInteger(value),
'skipafter': (value) => isInteger(value),
'sequence': (value) => isInteger(value),
'battr': (value) => isArrayOfNums(value),
'maxextended': (value) => isInteger(value),
'minbitrate': (value) => isInteger(value),
'maxbitrate': (value) => isInteger(value),
'boxingallowed': (value) => isInteger(value),
'playbackmethod': (value) => isArrayOfNums(value),
'playbackend': (value) => isInteger(value),
'delivery': (value) => isArrayOfNums(value),
'pos': (value) => isInteger(value),
'api': (value) => isArrayOfNums(value)
};

/**
* Returns the window.ADAGIO global object used to store Adagio data.
* This object is created in window.top if possible, otherwise in window.self.
Expand Down Expand Up @@ -186,6 +151,12 @@ function _getEids(bidRequest) {
}
}

/**
* Merge and compute video params set at mediaTypes and bidder params level
*
* @param {object} bidRequest - copy of the original bidRequest object.
* @returns {void}
*/
function _buildVideoBidRequest(bidRequest) {
const videoAdUnitParams = deepAccess(bidRequest, 'mediaTypes.video', {});
const videoBidderParams = deepAccess(bidRequest, 'params.video', {});
Expand All @@ -206,22 +177,11 @@ function _buildVideoBidRequest(bidRequest) {
};

if (videoParams.context && videoParams.context === OUTSTREAM) {
bidRequest.mediaTypes.video.playerName = getPlayerName(bidRequest);
videoParams.playerName = getPlayerName(bidRequest);
}

// Only whitelisted OpenRTB options need to be validated.
// Other options will still remain in the `mediaTypes.video` object
// sent in the ad-request, but will be ignored by the SSP.
Object.keys(ORTB_VIDEO_PARAMS).forEach(paramName => {
if (videoParams.hasOwnProperty(paramName)) {
if (ORTB_VIDEO_PARAMS[paramName](videoParams[paramName])) {
bidRequest.mediaTypes.video[paramName] = videoParams[paramName];
} else {
delete bidRequest.mediaTypes.video[paramName];
logWarn(`${LOG_PREFIX} The OpenRTB video param ${paramName} has been skipped due to misformating. Please refer to OpenRTB 2.5 spec.`);
}
}
});
validateOrtbVideoFields(videoParams)
bidRequest.mediaTypes.video = videoParams;
}

function _parseNativeBidResponse(bid) {
Expand Down
3 changes: 2 additions & 1 deletion src/prebid.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {enrichFPD} from './fpd/enrichment.js';
import {allConsent} from './consentHandler.js';
import {insertLocatorFrame, renderAdDirect} from './adRendering.js';
import {getHighestCpm} from './utils/reducers.js';
import {fillVideoDefaults} from './video.js';
import {fillVideoDefaults, validateOrtbVideoFields} from './video.js';

const pbjsInstance = getGlobal();
const { triggerUserSyncs } = userSync;
Expand Down Expand Up @@ -134,6 +134,7 @@ function validateVideoMediaType(adUnit) {
delete validatedAdUnit.mediaTypes.video.playerSize;
}
}
validateOrtbVideoFields(video);
return validatedAdUnit;
}

Expand Down
65 changes: 64 additions & 1 deletion src/video.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,52 @@
import {deepAccess, logError} from './utils.js';
import {deepAccess, isArrayOfNums, isInteger, isNumber, isStr, logError, logWarn} from './utils.js';
import {config} from '../src/config.js';
import {hook} from './hook.js';
import {auctionManager} from './auctionManager.js';

export const OUTSTREAM = 'outstream';
export const INSTREAM = 'instream';

/**
* Basic validation of OpenRTB 2.x video object properties.
* Not included: `companionad`, `durfloors`, `ext`
* reference: https://github.com/InteractiveAdvertisingBureau/openrtb2.x/blob/main/2.6.md
*/
export const ORTB_VIDEO_PARAMS = new Map([
[ 'mimes', { validate: (value) => Array.isArray(value) && value.length > 0 && value.every(v => typeof v === 'string') } ],
Copy link
Collaborator

@patmmccann patmmccann Jul 23, 2024

Choose a reason for hiding this comment

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

While this feels like a lot to add to core, so many prominent bid adapters do exactly this that it would likely be a net code reduction, very exciting

[ 'minduration', { validate: (value) => isInteger(value) } ],
[ 'maxduration', { validate: (value) => isInteger(value) } ],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be more compact ('minduration' -> isInteger, 'podid' -> isStr, etc). Are you envisioning it to contain more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to make a map that was not a strict "validation map", it is easier to plug-in something else later, but honestly I don't know how to extend it right now. Tell me if you prefer I use the "compact" way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think compact is better because in terms of bundle size, N maps are still smaller than a map where each node has N things in it.

[ 'startdelay', { validate: (value) => isInteger(value) } ],
[ 'maxseq', { validate: (value) => isInteger(value) } ],
[ 'poddur', { validate: (value) => isInteger(value) } ],
[ 'protocols', { validate: (value) => isArrayOfNums(value) } ],
[ 'w', { validate: (value) => isInteger(value) } ],
[ 'h', { validate: (value) => isInteger(value) } ],
[ 'podid', { validate: (value) => isStr(value) } ],
[ 'podseq', { validate: (value) => isInteger(value) } ],
[ 'rqddurs', { validate: (value) => isArrayOfNums(value) } ],
[ 'placement', { validate: (value) => isInteger(value) } ], // deprecated, see plcmt
[ 'plcmt', { validate: (value) => isInteger(value) } ],
[ 'linearity', { validate: (value) => isInteger(value) } ],
[ 'skip', { validate: (value) => [1, 0].includes(value) } ],
[ 'skipmin', { validate: (value) => isInteger(value) } ],
[ 'skipafter', { validate: (value) => isInteger(value) } ],
[ 'sequence', { validate: (value) => isInteger(value) } ], // deprecated
[ 'slotinpod', { validate: (value) => isInteger(value) } ],
[ 'mincpmpersec', { validate: (value) => isNumber(value) } ],
[ 'battr', { validate: (value) => isArrayOfNums(value) } ],
[ 'maxextended', { validate: (value) => isInteger(value) } ],
[ 'minbitrate', { validate: (value) => isInteger(value) } ],
[ 'maxbitrate', { validate: (value) => isInteger(value) } ],
[ 'boxingallowed', { validate: (value) => isInteger(value) } ],
[ 'playbackmethod', { validate: (value) => isArrayOfNums(value) } ],
[ 'playbackend', { validate: (value) => isInteger(value) } ],
[ 'delivery', { validate: (value) => isArrayOfNums(value) } ],
[ 'pos', { validate: (value) => isInteger(value) } ],
[ 'api', { validate: (value) => isArrayOfNums(value) } ],
[ 'companiontype', { validate: (value) => isArrayOfNums(value) } ],
[ 'poddedupe', { validate: (value) => isArrayOfNums(value) } ],
]);

export function fillVideoDefaults(adUnit) {
const video = adUnit?.mediaTypes?.video;
if (video != null && video.plcmt == null) {
Expand All @@ -17,6 +58,28 @@ export function fillVideoDefaults(adUnit) {
}
}

/**
* validateOrtbVideoFields mutates the `videoParams` object by removing invalid ortb properties.
* Other properties are ignored and kept as is.
*
* @param {object} videoParams
* @returns {void}
*/
export function validateOrtbVideoFields(videoParams) {
if (videoParams != null) {
Object.entries(videoParams)
.forEach(([key, value]) => {
if (ORTB_VIDEO_PARAMS.has(key)) {
const valid = ORTB_VIDEO_PARAMS.get(key).validate(value);
if (!valid) {
delete videoParams[key];
logWarn(`Invalid value for mediaTypes.video.${key} ORTB property. The property has been removed.`);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this enough info to track down the issue - should we log the offending adUnit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This way, I should update the function to validateOrtbVideoFields(adUnit); would it sound better?

In 1st attempt I would like to keep the function scoped on the videoParams object, but perhaps it makes no sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to agree that the scope should be as small as possible, but I'm not sure code aesthetics will convince someone with a complicated setup and a bunch of cryptic warning messages :)

You could also do something like validateOrtbVideoFields(videoParams, onInvalidParam) to keep the scope limited, but let something else that has the adUnit do the logging.

}
});
}
}

/**
* @typedef {object} VideoBid
* @property {string} adId id of the bid
Expand Down
1 change: 0 additions & 1 deletion test/spec/modules/adagioBidAdapter_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,6 @@ describe('Adagio bid adapter', () => {
const requests = spec.buildRequests([bid01], bidderRequest);
expect(requests).to.have.lengthOf(1);
expect(requests[0].data.adUnits[0].mediaTypes.video).to.deep.equal(expected);
sinon.assert.calledTwice(utils.logWarn.withArgs(sinon.match(new RegExp(/^Adagio: The OpenRTB/))));
});
});

Expand Down
60 changes: 59 additions & 1 deletion test/spec/video_spec.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {fillVideoDefaults, isValidVideoBid} from 'src/video.js';
import {fillVideoDefaults, isValidVideoBid, validateOrtbVideoFields} from 'src/video.js';
import {hook} from '../../src/hook.js';
import {stubAuctionIndex} from '../helpers/indexStub.js';

Expand Down Expand Up @@ -77,6 +77,64 @@ describe('video.js', function () {
})
})

describe('validateOrtbVideoFields', () => {
function validate(videoMediaType = {}) {
const adUnit = {
mediaTypes: { video: videoMediaType }
};
const video = adUnit.mediaTypes.video;
validateOrtbVideoFields(video);
return adUnit.mediaTypes.video;
}

it('remove incorrect ortb properties, and keep non ortb ones', () => {
const mt = {
content: 'outstream',

mimes: ['video/mp4'],
minduration: 5,
maxduration: 15,
startdelay: 0,
maxseq: 0,
poddur: 0,
protocols: [7],
w: 600,
h: 480,
podid: 'an-id',
podseq: 0,
rqddurs: [5],
placement: 1,
plcmt: 1,
linearity: 1,
skip: 0,
skipmin: 3,
skipafter: 3,
sequence: 0,
slotinpod: 0,
mincpmpersec: 2.5,
battr: [6, 7],
maxextended: 0,
minbitrate: 800,
maxbitrate: 1000,
boxingallowed: 1,
playbackmethod: [1],
playbackend: 1,
delivery: [2],
pos: 0,
api: 6, // -- INVALID
companiontype: [1, 2, 3],
poddedupe: [1],

otherOne: 'test',
};

const expected = {...mt};
delete expected.api;

expect(validate(mt)).to.eql(expected)
});
})

describe('isValidVideoBid', () => {
it('validates valid instream bids', function () {
const bid = {
Expand Down
Loading