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

Fix #3592. Bad Input Error if pbjs s2s config contains alias configuration for a disabled adapter #3650

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions docs/config-app.md
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,10 @@ Preconfigured application settings can be obtained from multiple data sources co

Warning! Application will not start in case of no one data source is defined and you'll get an exception in logs.

For requests validation mode available next options:
- `settings.fail-on-unknown-bidders` - fail with validation error or just make warning for unknown bidders.
- `settings.fail-on-disabled-bidders` - fail with validation error or just make warning for disabled bidders.

For filesystem data source available next options:
- `settings.filesystem.settings-filename` - location of file settings.
- `settings.filesystem.stored-requests-dir` - directory with stored requests.
Expand Down
4 changes: 4 additions & 0 deletions docs/metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ where `[DATASOURCE]` is a data source name, `DEFAULT_DS` by defaul.
- `imps_video` - number of video impressions
- `imps_native` - number of native impressions
- `imps_audio` - number of audio impressions
- `disabled_bidder` - number of disabled bidders received within requests
- `unknown_bidder` - number of unknown bidders received within requests
- `requests.(ok|badinput|err|networkerr|blocklisted_account|blocklisted_app).(openrtb2-web|openrtb-app|amp|legacy)` - number of requests broken down by status and type
- `bidder-cardinality.<cardinality>.requests` - number of requests targeting `<cardinality>` of bidders
- `connection_accept_errors` - number of errors occurred while establishing HTTP connection
Expand Down Expand Up @@ -92,6 +94,8 @@ Following metrics are collected and submitted if account is configured with `det
- `account.<account-id>.requests.type.(openrtb2-web,openrtb-app,amp,legacy)` - number of requests received from account with `<account-id>` broken down by type of incoming request
- `account.<account-id>.debug_requests` - number of requests received from account with `<account-id>` broken down by type of incoming request (when debug mode is enabled)
- `account.<account-id>.requests.rejected` - number of rejected requests caused by incorrect `accountId`
- `account.<account-id>.requests.disabled_bidder` - number of disabled bidders received within requests from account with `<account-id>`
- `account.<account-id>.requests.unknown_bidder` - number of unknown bidder names received within requests from account with `<account-id>`
- `account.<account-id>.adapter.<bidder-name>.request_time` - timer tracking how long did it take to make a request to `<bidder-name>` when incoming request was from `<account-id>`
- `account.<account-id>.adapter.<bidder-name>.bids_received` - number of bids received from `<bidder-name>` when incoming request was from `<account-id>`
- `account.<account-id>.adapter.<bidder-name>.requests.(gotbids|nobid)` - number of requests made to `<bidder-name>` broken down by result status when incoming request was from `<account-id>`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ private Future<BidRequest> updateBidRequest(AuctionContext auctionContext) {
.map(bidRequest -> paramsResolver.resolve(bidRequest, auctionContext, ENDPOINT, true))
.map(bidRequest -> ortb2RequestFactory.removeEmptyEids(bidRequest, auctionContext.getDebugWarnings()))
.compose(resolvedBidRequest -> ortb2RequestFactory.validateRequest(
account,
resolvedBidRequest,
auctionContext.getHttpRequest(),
auctionContext.getDebugContext(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ private Future<BidRequest> updateAndValidateBidRequest(AuctionContext auctionCon
return storedRequestProcessor.processAuctionRequest(account.getId(), auctionContext.getBidRequest())
.compose(auctionStoredResult -> updateBidRequest(auctionStoredResult, auctionContext))
.compose(bidRequest -> ortb2RequestFactory.validateRequest(
bidRequest, httpRequest, auctionContext.getDebugContext(), debugWarnings))
account, bidRequest, httpRequest, auctionContext.getDebugContext(), debugWarnings))
.map(interstitialProcessor::process);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,14 @@ public Future<ActivityInfrastructure> activityInfrastructureFrom(AuctionContext
auctionContext.getDebugContext().getTraceLevel()));
}

public Future<BidRequest> validateRequest(BidRequest bidRequest,
public Future<BidRequest> validateRequest(Account account,
BidRequest bidRequest,
HttpRequestContext httpRequestContext,
DebugContext debugContext,
List<String> warnings) {

final ValidationResult validationResult = requestValidator.validate(
bidRequest, httpRequestContext, debugContext);
account, bidRequest, httpRequestContext, debugContext);

if (validationResult.hasWarnings()) {
warnings.addAll(validationResult.getWarnings());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ public Future<WithPodErrors<AuctionContext>> fromRequest(RoutingContext routingC
.map(auctionContext -> auctionContext.with(debugResolver.debugContextFrom(auctionContext)))

.compose(auctionContext -> ortb2RequestFactory.validateRequest(
auctionContext.getAccount(),
auctionContext.getBidRequest(),
auctionContext.getHttpRequest(),
auctionContext.getDebugContext(),
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/prebid/server/metric/MetricName.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ public enum MetricName {
nobid,
gotbids,
badinput,
disabled_bidder,
unknown_bidder,
blocklisted_account,
blocklisted_app,
badserverresponse,
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/org/prebid/server/metric/Metrics.java
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,22 @@ public void updateAdapterRequestErrorMetric(String bidder, MetricName errorMetri
forAdapter(bidder).request().incCounter(errorMetric);
}

public void updateDisabledBidderMetric(Account account) {
incCounter(MetricName.disabled_bidder);
if (accountMetricsVerbosityResolver.forAccount(account)
.isAtLeast(AccountMetricsVerbosityLevel.detailed)) {
forAccount(account.getId()).requests().incCounter(MetricName.disabled_bidder);
}
}

public void updateUnknownBidderMetric(Account account) {
incCounter(MetricName.unknown_bidder);
if (accountMetricsVerbosityResolver.forAccount(account)
.isAtLeast(AccountMetricsVerbosityLevel.detailed)) {
forAccount(account.getId()).requests().incCounter(MetricName.unknown_bidder);
}
}

public void updateAnalyticEventMetric(String analyticCode, MetricName eventType, MetricName result) {
forAnalyticReporter(analyticCode).forEventType(eventType).incCounter(result);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1033,15 +1033,19 @@ RequestValidator requestValidator(
Metrics metrics,
JacksonMapper mapper,
@Value("${logging.sampling-rate:0.01}") double logSamplingRate,
@Value("${auction.strict-app-site-dooh:false}") boolean enabledStrictAppSiteDoohValidation) {
@Value("${auction.strict-app-site-dooh:false}") boolean enabledStrictAppSiteDoohValidation,
@Value("${settings.fail-on-disabled-bidders:true}") boolean failOnDisabledBidders,
@Value("${settings.fail-on-unknown-bidders:true}") boolean failOnUnknownBidders) {

return new RequestValidator(
bidderCatalog,
impValidator,
metrics,
mapper,
logSamplingRate,
enabledStrictAppSiteDoohValidation);
enabledStrictAppSiteDoohValidation,
failOnDisabledBidders,
failOnUnknownBidders);
}

@Bean
Expand Down
44 changes: 34 additions & 10 deletions src/main/java/org/prebid/server/validation/RequestValidator.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.prebid.server.proto.openrtb.ext.request.ExtUserPrebid;
import org.prebid.server.proto.openrtb.ext.request.ImpMediaType;
import org.prebid.server.proto.openrtb.ext.response.BidType;
import org.prebid.server.settings.model.Account;
import org.prebid.server.util.HttpUtil;
import org.prebid.server.validation.model.ValidationResult;

Expand Down Expand Up @@ -74,6 +75,8 @@ public class RequestValidator {
private final JacksonMapper mapper;
private final double logSamplingRate;
private final boolean enabledStrictAppSiteDoohValidation;
private final boolean failOnDisabledBidders;
private final boolean failOnUnknownBidders;

/**
* Constructs a RequestValidator that will use the BidderParamValidator passed in order to validate all critical
Expand All @@ -83,21 +86,26 @@ public RequestValidator(BidderCatalog bidderCatalog,
ImpValidator impValidator, Metrics metrics,
JacksonMapper mapper,
double logSamplingRate,
boolean enabledStrictAppSiteDoohValidation) {
boolean enabledStrictAppSiteDoohValidation,
boolean failOnDisabledBidders,
boolean failOnUnknownBidders) {

this.bidderCatalog = Objects.requireNonNull(bidderCatalog);
this.impValidator = Objects.requireNonNull(impValidator);
this.metrics = Objects.requireNonNull(metrics);
this.mapper = Objects.requireNonNull(mapper);
this.logSamplingRate = logSamplingRate;
this.enabledStrictAppSiteDoohValidation = enabledStrictAppSiteDoohValidation;
this.failOnDisabledBidders = failOnDisabledBidders;
this.failOnUnknownBidders = failOnUnknownBidders;
}

/**
* Validates the {@link BidRequest} against a list of validation checks, however, reports only one problem
* at a time.
*/
public ValidationResult validate(BidRequest bidRequest,
public ValidationResult validate(Account account,
BidRequest bidRequest,
HttpRequestContext httpRequestContext,
DebugContext debugContext) {

Expand Down Expand Up @@ -126,7 +134,7 @@ public ValidationResult validate(BidRequest bidRequest,
validateTargeting(targeting);
}
aliases = ObjectUtils.defaultIfNull(extRequestPrebid.getAliases(), Collections.emptyMap());
validateAliases(aliases);
validateAliases(aliases, warnings, account);
validateAliasesGvlIds(extRequestPrebid, aliases);
validateBidAdjustmentFactors(extRequestPrebid.getBidadjustmentfactors(), aliases);
validateExtBidPrebidData(extRequestPrebid.getData(), aliases, isDebugEnabled, warnings);
Expand Down Expand Up @@ -505,18 +513,34 @@ private static void validateGranularityRangeIncrement(ExtGranularityRange range)
* Validates aliases. Throws {@link ValidationException} in cases when alias points to invalid bidder or when alias
* is equals to itself.
*/
private void validateAliases(Map<String, String> aliases) throws ValidationException {
private void validateAliases(Map<String, String> aliases, List<String> warnings,
Account account) throws ValidationException {

for (final Map.Entry<String, String> aliasToBidder : aliases.entrySet()) {
final String alias = aliasToBidder.getKey();
final String coreBidder = aliasToBidder.getValue();
if (!bidderCatalog.isValidName(coreBidder)) {
throw new ValidationException(
"request.ext.prebid.aliases.%s refers to unknown bidder: %s".formatted(alias, coreBidder));
}
if (!bidderCatalog.isActive(coreBidder)) {
throw new ValidationException(
"request.ext.prebid.aliases.%s refers to disabled bidder: %s".formatted(alias, coreBidder));
metrics.updateUnknownBidderMetric(account);

final String message = "request.ext.prebid.aliases.%s refers to unknown bidder: %s".formatted(alias,
coreBidder);
if (failOnUnknownBidders) {
throw new ValidationException(message);
} else {
warnings.add(message);
}
} else if (!bidderCatalog.isActive(coreBidder)) {
metrics.updateDisabledBidderMetric(account);

final String message = "request.ext.prebid.aliases.%s refers to disabled bidder: %s".formatted(alias,
coreBidder);
if (failOnDisabledBidders) {
throw new ValidationException(message);
} else {
warnings.add(message);
}
}

if (alias.equals(coreBidder)) {
throw new ValidationException("""
request.ext.prebid.aliases.%s defines a no-op alias. \
Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/application.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ currency-converter:
settings:
generate-storedrequest-bidrequest-id: false
enforce-valid-account: false
fail-on-unknown-bidders: true
fail-on-disabled-bidders: true
database:
pool-size: 20
idle-connection-timeout: 300
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1753,8 +1753,8 @@ private void givenBidRequest(

given(ortb2ImplicitParametersResolver.resolve(any(), any(), any(), anyBoolean())).willAnswer(
answerWithFirstArgument());
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willAnswer(invocation -> Future.succeededFuture((BidRequest) invocation.getArgument(0)));
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any(), any()))
.willAnswer(invocation -> Future.succeededFuture((BidRequest) invocation.getArgument(1)));

given(ortb2RequestFactory.enrichBidRequestWithAccountAndPrivacyData(any()))
.willAnswer(invocation -> Future.succeededFuture(((AuctionContext) invocation.getArgument(0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ public void setUp() {
given(ortb2RequestFactory.executeRawAuctionRequestHooks(any()))
.willAnswer(invocation -> Future.succeededFuture(
((AuctionContext) invocation.getArgument(0)).getBidRequest()));
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willAnswer(invocationOnMock -> Future.succeededFuture((BidRequest) invocationOnMock.getArgument(0)));
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any(), any()))
.willAnswer(invocationOnMock -> Future.succeededFuture((BidRequest) invocationOnMock.getArgument(1)));
given(ortb2RequestFactory.removeEmptyEids(any(), any()))
.willAnswer(invocationOnMock -> invocationOnMock.getArgument(0));
given(ortb2RequestFactory.updateTimeout(any())).willAnswer(invocation -> invocation.getArgument(0));
Expand Down Expand Up @@ -662,7 +662,7 @@ public void shouldReturnFailedFutureIfRequestValidationFailed() {
// given
givenValidBidRequest();

given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any(), any()))
.willReturn(Future.failedFuture(new InvalidRequestException("errors")));

// when
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
public class Ortb2RequestFactoryTest extends VertxTest {

private static final List<String> BLOCKLISTED_ACCOUNTS = singletonList("bad_acc");
private static final String ACCOUNT_ID = "accountId";

@Mock
private UidsCookieService uidsCookieService;
Expand Down Expand Up @@ -658,12 +659,13 @@ public void enrichAuctionContextShouldSetDebugOff() {
@Test
public void validateRequestShouldThrowInvalidRequestExceptionIfRequestIsInvalid() {
// given
given(requestValidator.validate(any(), any(), any())).willReturn(ValidationResult.error("error"));
given(requestValidator.validate(any(), any(), any(), any())).willReturn(ValidationResult.error("error"));

final BidRequest bidRequest = givenBidRequest(identity());

// when
final Future<BidRequest> result = target.validateRequest(
Account.empty(ACCOUNT_ID),
bidRequest,
HttpRequestContext.builder().build(),
DebugContext.empty(),
Expand All @@ -675,25 +677,26 @@ public void validateRequestShouldThrowInvalidRequestExceptionIfRequestIsInvalid(
.isInstanceOf(InvalidRequestException.class)
.hasMessage("error");

verify(requestValidator).validate(eq(bidRequest), any(), any());
verify(requestValidator).validate(any(), eq(bidRequest), any(), any());
}

@Test
public void validateRequestShouldReturnSameBidRequest() {
// given
given(requestValidator.validate(any(), any(), any())).willReturn(ValidationResult.success());
given(requestValidator.validate(any(), any(), any(), any())).willReturn(ValidationResult.success());

final BidRequest bidRequest = givenBidRequest(identity());

// when
final BidRequest result = target.validateRequest(
Account.empty(ACCOUNT_ID),
bidRequest,
HttpRequestContext.builder().build(),
DebugContext.empty(),
new ArrayList<>()).result();

// then
verify(requestValidator).validate(eq(bidRequest), any(), any());
verify(requestValidator).validate(any(), eq(bidRequest), any(), any());

assertThat(result).isSameAs(bidRequest);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ public void shouldReturnExpectedResultAndReturnErrors() throws JsonProcessingExc
verify(ortb2RequestFactory).createAuctionContext(any(), eq(MetricName.video));
verify(ortb2RequestFactory).enrichAuctionContext(any(), any(), eq(bidRequest), eq(0L));
verify(ortb2RequestFactory).fetchAccountWithoutStoredRequestLookup(any());
verify(ortb2RequestFactory).validateRequest(eq(bidRequest), any(), any(), any());
verify(ortb2RequestFactory).validateRequest(any(), eq(bidRequest), any(), any(), any());
verify(paramsResolver)
.resolve(eq(bidRequest), any(), eq(Endpoint.openrtb2_video.value()), eq(false));
verify(ortb2RequestFactory).enrichBidRequestWithAccountAndPrivacyData(
Expand Down Expand Up @@ -404,8 +404,8 @@ private void givenBidRequest(BidRequest bidRequest, List<PodError> podErrors) {
.build());
given(ortb2RequestFactory.fetchAccountWithoutStoredRequestLookup(any())).willReturn(Future.succeededFuture());

given(ortb2RequestFactory.validateRequest(any(), any(), any(), any()))
.willAnswer(invocation -> Future.succeededFuture((BidRequest) invocation.getArgument(0)));
given(ortb2RequestFactory.validateRequest(any(), any(), any(), any(), any()))
.willAnswer(invocation -> Future.succeededFuture((BidRequest) invocation.getArgument(1)));

given(paramsResolver.resolve(any(), any(), any(), anyBoolean()))
.willAnswer(answerWithFirstArgument());
Expand Down
Loading
Loading