diff --git a/docs/config-app.md b/docs/config-app.md index 5628341a812..8c7510d9ff8 100644 --- a/docs/config-app.md +++ b/docs/config-app.md @@ -99,6 +99,7 @@ Removes and downloads file again if depending service cant process probably corr - `auction.enforce-random-bid-id` - whether to enforce generating a robust random seatbid[].bid[].id in the OpenRTB response if the initial value is less than 17 characters. - `auction.validations.banner-creative-max-size` - enables creative max size validation for banners. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. - `auction.validations.secure-markup` - enables secure markup validation. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. +- `auction.validations.secure-markup-allowed-paths` - enables secure markup validation. Possible values: `skip`, `enforce`, `warn`. Default is `skip`. - `auction.host-schain-node` - defines global schain node that will be appended to `request.source.ext.schain.nodes` passed to bidders - `auction.category-mapping-enabled` - if equals to `true` the category mapping feature will be active while auction. - `auction.strict-app-site-dooh` - if set to `true`, it will reject requests that contain more than one of app/site/dooh. Defaults to `false`. diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index 1e496ea7e9a..a7d9475b7d2 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -117,6 +117,7 @@ import org.prebid.server.validation.BidderParamValidator; import org.prebid.server.validation.ImpValidator; import org.prebid.server.validation.RequestValidator; +import org.prebid.server.validation.ResponseBidAdmValidator; import org.prebid.server.validation.ResponseBidValidator; import org.prebid.server.validation.VideoRequestValidator; import org.prebid.server.vast.VastModifier; @@ -1072,17 +1073,26 @@ BidderParamValidator bidderParamValidator(BidderCatalog bidderCatalog, JacksonMa @Bean ResponseBidValidator responseValidator( + ResponseBidAdmValidator responseBidAdmValidator, @Value("${auction.validations.banner-creative-max-size}") BidValidationEnforcement bannerMaxSizeEnforcement, @Value("${auction.validations.secure-markup}") BidValidationEnforcement secureMarkupEnforcement, Metrics metrics) { return new ResponseBidValidator( + responseBidAdmValidator, bannerMaxSizeEnforcement, secureMarkupEnforcement, metrics, logSamplingRate); } + @Bean + ResponseBidAdmValidator responseBidAdmValidator( + @Value("${auction.validations.secure-markup-allowed-paths:null}") Set allowedPaths) { + + return new ResponseBidAdmValidator(allowedPaths); + } + @Bean CriteriaLogManager criteriaLogManager(JacksonMapper mapper) { return new CriteriaLogManager(mapper); diff --git a/src/main/java/org/prebid/server/validation/ResponseBidAdmValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidAdmValidator.java new file mode 100644 index 00000000000..bf44fefb7ab --- /dev/null +++ b/src/main/java/org/prebid/server/validation/ResponseBidAdmValidator.java @@ -0,0 +1,75 @@ +package org.prebid.server.validation; + +import org.apache.commons.collections4.CollectionUtils; +import org.apache.commons.lang3.StringUtils; + +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Collection; +import java.util.Set; +import java.util.stream.Collectors; + +public class ResponseBidAdmValidator { + + private static final String[] SECURE_MARKUP_MARKERS = {"https:", "https%3A"}; + + private final Collection allowedPaths; + + public ResponseBidAdmValidator(Collection allowedPaths) { + this.allowedPaths = CollectionUtils.emptyIfNull(allowedPaths); + } + + public boolean isSecure(String adm) { + if (!StringUtils.containsAny(adm, SECURE_MARKUP_MARKERS)) { + return false; + } + + final Set encodedAllowedPaths = allowedPaths.stream() + .map(pattern -> URLEncoder.encode(pattern, StandardCharsets.UTF_8)) + .collect(Collectors.toSet()); + + return allUrlsAllowed(adm, allowedPaths, "http:", "//") + && allUrlsAllowed(adm, encodedAllowedPaths, "http%3A", "%2F%2F"); + } + + private static boolean allUrlsAllowed(String adm, + Collection allowedPaths, + String protocol, + String doubleSlash) { + + int searchStartIndex = 0; + while (searchStartIndex < adm.length()) { + final int httpIndex = adm.indexOf(protocol, searchStartIndex); + + if (httpIndex == -1) { + return true; + } + + final int afterHttpPrefixIndex = httpIndex + protocol.length(); + if (afterHttpPrefixIndex + 1 > adm.length()) { + return true; + } + + if (!adm.startsWith(doubleSlash, afterHttpPrefixIndex)) { + searchStartIndex = httpIndex + 1; + continue; + } + + final int afterHttpDoubleSlashIndex = afterHttpPrefixIndex + doubleSlash.length(); + if (afterHttpDoubleSlashIndex + 1 > adm.length()) { + return true; + } + + final boolean isAllowedExactPathMatch = allowedPaths.stream() + .anyMatch(allowedPattern -> adm.startsWith(allowedPattern, afterHttpDoubleSlashIndex)); + + if (!isAllowedExactPathMatch) { + return false; + } + + searchStartIndex = httpIndex + 1; + } + + return false; + } +} diff --git a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java index cca8f88faf7..d09b591556a 100644 --- a/src/main/java/org/prebid/server/validation/ResponseBidValidator.java +++ b/src/main/java/org/prebid/server/validation/ResponseBidValidator.java @@ -48,20 +48,20 @@ public class ResponseBidValidator { private static final ConditionalLogger alternateBidderCodeLogger = new ConditionalLogger("alternate_bidder_code_validation", logger); - private static final String[] INSECURE_MARKUP_MARKERS = {"http:", "http%3A"}; - private static final String[] SECURE_MARKUP_MARKERS = {"https:", "https%3A"}; - + private final ResponseBidAdmValidator admValidator; private final BidValidationEnforcement bannerMaxSizeEnforcement; private final BidValidationEnforcement secureMarkupEnforcement; private final Metrics metrics; private final double logSamplingRate; - public ResponseBidValidator(BidValidationEnforcement bannerMaxSizeEnforcement, + public ResponseBidValidator(ResponseBidAdmValidator admValidator, + BidValidationEnforcement bannerMaxSizeEnforcement, BidValidationEnforcement secureMarkupEnforcement, Metrics metrics, double logSamplingRate) { + this.admValidator = admValidator; this.bannerMaxSizeEnforcement = Objects.requireNonNull(bannerMaxSizeEnforcement); this.secureMarkupEnforcement = Objects.requireNonNull(secureMarkupEnforcement); this.metrics = Objects.requireNonNull(metrics); @@ -277,7 +277,7 @@ private List validateSecureMarkup(BidderBid bidderBid, final Bid bid = bidderBid.getBid(); final String adm = bid.getAdm(); - if (isImpSecure(correspondingImp) && markupIsNotSecure(adm)) { + if (isImpSecure(correspondingImp) && !admValidator.isSecure(adm)) { final String message = """ BidResponse validation `%s`: bidder `%s` response triggers secure \ creative validation for bid %s, account=%s, referrer=%s, adm=%s""" @@ -301,11 +301,6 @@ private static boolean isImpSecure(Imp imp) { return Objects.equals(imp.getSecure(), 1); } - private static boolean markupIsNotSecure(String adm) { - return StringUtils.containsAny(adm, INSECURE_MARKUP_MARKERS) - || !StringUtils.containsAny(adm, SECURE_MARKUP_MARKERS); - } - private List singleWarningOrValidationException( BidValidationEnforcement enforcement, Consumer metricsRecorder, diff --git a/src/test/java/org/prebid/server/validation/ResponseBidAdmValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidAdmValidatorTest.java new file mode 100644 index 00000000000..1f3c20b56ab --- /dev/null +++ b/src/test/java/org/prebid/server/validation/ResponseBidAdmValidatorTest.java @@ -0,0 +1,77 @@ +package org.prebid.server.validation; + +import org.junit.jupiter.api.Test; + +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +class ResponseBidAdmValidatorTest { + + //todo: add separate test for unique cases + private final ResponseBidAdmValidator target = new ResponseBidAdmValidator(Set.of("www.w3.org", "www.url.ua/page")); + + @Test + public void shouldValidateAdm() { + assertThat(target.isSecure("http:")).isFalse(); + assertThat(target.isSecure("http:http:")).isFalse(); + // treats 'https' as a domain, which is not allowed + assertThat(target.isSecure("http://https:")).isFalse(); + // 'http:///' is actually empty + assertThat(target.isSecure("https:http:///")).isFalse(); + assertThat(target.isSecure("http://www.url.uahttps:")).isFalse(); + + assertThat(target.isSecure("https:")).isTrue(); + assertThat(target.isSecure("http:https:")).isTrue(); + assertThat(target.isSecure("http:/https:")).isTrue(); + assertThat(target.isSecure("http:https://")).isTrue(); + // 'http://' is treated as empty only in the end + assertThat(target.isSecure("https:http://")).isTrue(); + assertThat(target.isSecure("https:http:")).isTrue(); + assertThat(target.isSecure("https:https:")).isTrue(); + assertThat(target.isSecure("http://www.w3.orghttps:")).isTrue(); + assertThat(target.isSecure("http://www.url.ua/pagehttps:")).isTrue(); + // empty domain because no slashes + assertThat(target.isSecure("http:www.w3.orghttps:")).isTrue(); + } + + @Test + public void shouldValidateUrlEncodedAdm() { + // case insensitive + assertThat(target.isSecure("https%3a")).isFalse(); + assertThat(target.isSecure("http%3A")).isFalse(); + assertThat(target.isSecure("http%3Ahttp%3A")).isFalse(); + //treats 'https' as a domain, which is not allowed + assertThat(target.isSecure("http%3A%2F%2Fhttps%3A")).isFalse(); + assertThat(target.isSecure("https%3Ahttp%3A%2F%2F%2F")).isFalse(); + assertThat(target.isSecure("http%3A%2F%2Fwww.url.uahttps%3A")).isFalse(); + + assertThat(target.isSecure("https%3A")).isTrue(); + assertThat(target.isSecure("http%3Ahttps%3A")).isTrue(); + assertThat(target.isSecure("http%3A%2Fhttps%3A")).isTrue(); + assertThat(target.isSecure("http%3Ahttps%3A%2F%2F")).isTrue(); + // http:// is treated as empty only in the end + assertThat(target.isSecure("https%3Ahttp%3A%2F%2F")).isTrue(); + assertThat(target.isSecure("https%3Ahttps%3A")).isTrue(); + assertThat(target.isSecure("http%3A%2F%2Fwww.w3.orghttps%3A")).isTrue(); + assertThat(target.isSecure("http%3A%2F%2Fwww.url.ua%2Fpagehttps%3A")).isTrue(); + // empty domain because no slashes + assertThat(target.isSecure("http%3Awww.w3.orghttps%3A")).isTrue(); + } + + @Test + public void shouldValidatePlainAndUrlEncodedPaths() { + // combined plain and encoded is not allowed + assertThat(target.isSecure("http://www.url.ua%2Fpagehttps%3A")).isFalse(); + + assertThat(target.isSecure("http%3Ahttps:")).isTrue(); + assertThat(target.isSecure("http:https%3A")).isTrue(); + assertThat(target.isSecure("https%3Ahttp%3A//")).isTrue(); + // 'http%3A//' is treated like an empty 'http:' and '//' separately + assertThat(target.isSecure("http%3A//any.domainhttps%3A")).isTrue(); + assertThat(target.isSecure("http://www.url.ua/pagehttps%3A")).isTrue(); + // empty domain because no slashes + assertThat(target.isSecure("http:www.w3.orghttps%3A")).isTrue(); + assertThat(target.isSecure("http://www.w3.orghttps://any.domain,http%3Aanything")).isTrue(); + } +} diff --git a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java index 91969151268..85c732b2abb 100644 --- a/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ResponseBidValidatorTest.java @@ -55,6 +55,9 @@ public class ResponseBidValidatorTest extends VertxTest { @Mock private BidRejectionTracker bidRejectionTracker; + @Mock(strictness = LENIENT) + private ResponseBidAdmValidator admValidator; + private ResponseBidValidator target; @Mock(strictness = LENIENT) @@ -62,10 +65,11 @@ public class ResponseBidValidatorTest extends VertxTest { @BeforeEach public void setUp() { - target = new ResponseBidValidator(enforce, enforce, metrics, 0.01); + target = new ResponseBidValidator(admValidator, enforce, enforce, metrics, 0.01); given(bidderAliases.resolveBidder(anyString())).willReturn(BIDDER_NAME); given(bidderAliases.isAllowedAlternateBidderCode(anyString(), anyString())).willReturn(true); + given(admValidator.isSecure(anyString())).willReturn(true); } @Test @@ -259,49 +263,12 @@ public void validateShouldFailIfBidHasNoCorrespondingImp() { } @Test - public void validateShouldFailIfBidHasInsecureMarkerInCreativeInSecureContext() { - // when - final BidderBid givenBid = givenBid(builder -> builder.adm("http://site.com/creative.jpg")); - final ValidationResult result = target.validate( - givenBid, - BIDDER_NAME, - givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), - bidderAliases); - - // then - assertThat(result.getErrors()) - .containsOnly(""" - BidResponse validation `enforce`: bidder `bidder` response triggers \ - secure creative validation for bid bidId1, account=account, referrer=unknown, \ - adm=http://site.com/creative.jpg"""); - verify(bidRejectionTracker) - .rejectBid(givenBid, BidRejectionReason.RESPONSE_REJECTED_INVALID_CREATIVE_NOT_SECURE); - } - - @Test - public void validateShouldFailIfBidHasInsecureEncodedMarkerInCreativeInSecureContext() { - // when - final BidderBid givenBid = givenBid(builder -> builder.adm("http%3A//site.com/creative.jpg")); - final ValidationResult result = target.validate( - givenBid, - BIDDER_NAME, - givenAuctionContext(givenBidRequest(builder -> builder.secure(1))), - bidderAliases); - - // then - assertThat(result.getErrors()) - .containsOnly(""" - BidResponse validation `enforce`: bidder `bidder` response triggers \ - secure creative validation for bid bidId1, account=account, referrer=unknown, \ - adm=http%3A//site.com/creative.jpg"""); - verify(bidRejectionTracker) - .rejectBid(givenBid, BidRejectionReason.RESPONSE_REJECTED_INVALID_CREATIVE_NOT_SECURE); - } + public void validateShouldFailIfBidHasNoSecureAdm() { + // given + given(admValidator.isSecure("adm")).willReturn(false); + final BidderBid givenBid = givenBid(builder -> builder.adm("adm")); - @Test - public void validateShouldFailIfBidHasNoSecureMarkersInCreativeInSecureContext() { // when - final BidderBid givenBid = givenBid(builder -> builder.adm("//site.com/creative.jpg")); final ValidationResult result = target.validate( givenBid, BIDDER_NAME, @@ -313,7 +280,7 @@ public void validateShouldFailIfBidHasNoSecureMarkersInCreativeInSecureContext() .containsOnly(""" BidResponse validation `enforce`: bidder `bidder` response triggers \ secure creative validation for bid bidId1, account=account, referrer=unknown, \ - adm=//site.com/creative.jpg"""); + adm=adm"""); verify(bidRejectionTracker) .rejectBid(givenBid, BidRejectionReason.RESPONSE_REJECTED_INVALID_CREATIVE_NOT_SECURE); } @@ -393,7 +360,7 @@ public void validateShouldReturnSuccessfulResultForValidBid() { @Test public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { // given - target = new ResponseBidValidator(skip, enforce, metrics, 0.01); + target = new ResponseBidValidator(admValidator, skip, enforce, metrics, 0.01); // when final ValidationResult result = target.validate( @@ -410,7 +377,7 @@ public void validateShouldReturnSuccessIfBannerSizeValidationNotEnabled() { @Test public void validateShouldReturnSuccessWithWarningIfBannerSizeEnforcementIsWarn() { // given - target = new ResponseBidValidator(warn, enforce, metrics, 0.01); + target = new ResponseBidValidator(admValidator, warn, enforce, metrics, 0.01); // when final BidderBid givenBid = givenBid(builder -> builder.w(null).h(null)); @@ -433,7 +400,7 @@ public void validateShouldReturnSuccessWithWarningIfBannerSizeEnforcementIsWarn( @Test public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { // given - target = new ResponseBidValidator(enforce, skip, metrics, 0.01); + target = new ResponseBidValidator(admValidator, enforce, skip, metrics, 0.01); // when final ValidationResult result = target.validate( @@ -450,10 +417,11 @@ public void validateShouldReturnSuccessIfSecureMarkupValidationNotEnabled() { @Test public void validateShouldReturnSuccessWithWarningIfSecureMarkupEnforcementIsWarn() { // given - target = new ResponseBidValidator(enforce, warn, metrics, 0.01); + target = new ResponseBidValidator(admValidator, enforce, warn, metrics, 0.01); + given(admValidator.isSecure("adm")).willReturn(false); // when - final BidderBid givenBid = givenBid(builder -> builder.adm("http://site.com/creative.jpg")); + final BidderBid givenBid = givenBid(builder -> builder.adm("adm")); final ValidationResult result = target.validate( givenBid, BIDDER_NAME, @@ -466,7 +434,7 @@ public void validateShouldReturnSuccessWithWarningIfSecureMarkupEnforcementIsWar .containsOnly(""" BidResponse validation `warn`: bidder `bidder` response triggers \ secure creative validation for bid bidId1, account=account, referrer=unknown, \ - adm=http://site.com/creative.jpg"""); + adm=adm"""); verifyNoInteractions(bidRejectionTracker); } @@ -485,7 +453,7 @@ public void validateShouldIncrementSizeValidationErrMetrics() { @Test public void validateShouldIncrementSizeValidationWarnMetrics() { // given - target = new ResponseBidValidator(warn, warn, metrics, 0.01); + target = new ResponseBidValidator(admValidator, warn, warn, metrics, 0.01); // when final BidderBid givenBid = givenBid(builder -> builder.w(150).h(200)); @@ -498,8 +466,11 @@ public void validateShouldIncrementSizeValidationWarnMetrics() { @Test public void validateShouldIncrementSecureValidationErrMetrics() { + // given + given(admValidator.isSecure("adm")).willReturn(false); + final BidderBid givenBid = givenBid(builder -> builder.adm("adm")); + // when - final BidderBid givenBid = givenBid(builder -> builder.adm("http://site.com/creative.jpg")); target.validate( givenBid, BIDDER_NAME, @@ -515,10 +486,11 @@ public void validateShouldIncrementSecureValidationErrMetrics() { @Test public void validateShouldIncrementSecureValidationWarnMetrics() { // given - target = new ResponseBidValidator(warn, warn, metrics, 0.01); + target = new ResponseBidValidator(admValidator, warn, warn, metrics, 0.01); + given(admValidator.isSecure("adm")).willReturn(false); // when - final BidderBid givenBid = givenBid(builder -> builder.adm("http://site.com/creative.jpg")); + final BidderBid givenBid = givenBid(builder -> builder.adm("adm")); target.validate( givenBid, BIDDER_NAME,