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

Housekeeping: Update 3rd party dependencies #3786

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

JimTharioAmazon
Copy link

@JimTharioAmazon JimTharioAmazon commented Feb 25, 2025

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

  • Updated three dependencies to address several CVEs and improve security posture: geoip2, json-schema-validator, and pinned commons-logging to a safe version.
  • Updated unit tests for geoip2 and json-schema-validator for dependency updates.
    • Updated geoip2 removed several constructors requiring more mocking for the Geo types.
    • Updated json-schema-validator returns an additional message for some failed validations, ""$: should be valid to one and only one schema, but 0 are valid"

🧠 Rationale behind the change

Update dependencies to improve security posture.

🔎 New Bid Adapter Checklist

  • verify email contact works
  • NO fully dynamic hostnames
  • geographic host parameters are NOT required
  • direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
  • if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
  • cover an adapter configuration with an integration test

🧪 Test plan

  • Relied on existing unit tests for verification of changes.
  • Used mvn clean package for build and test after each change.

🏎 Quality check

  • [Y] Are your changes following our code style guidelines?
  • [N] Are there any breaking changes in your code?
  • [-] Does your test coverage exceed 90%?
  • [N] Are there any erroneous console logs, debuggers or leftover code in your changes?

@JimTharioAmazon JimTharioAmazon marked this pull request as ready for review February 25, 2025 17:17
@Net-burst Net-burst requested a review from osulzhenko February 25, 2025 17:19
@osulzhenko osulzhenko added dependencies Pull requests that update a dependency file do not port labels Feb 26, 2025
@Net-burst Net-burst changed the title Update 3rd party dependencies Housekeeping: Update 3rd party dependencies Feb 26, 2025
Copy link
Collaborator

@osulzhenko osulzhenko left a comment

Choose a reason for hiding this comment

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

LGTM, no Maven conflicts or issues. But could you please also update a functional test: PBS should validate request as alias request and emit proper warnings when validation fails for request?
The error message changed due to an update in the json-schema-validator dependency to:

        and: "Bid response should contain warning"
        assert bidResponse.ext?.warnings[PREBID]?.code == [999, 999]
        assert bidResponse.ext?.warnings[PREBID]*.message ==
                ["WARNING: request.imp[0].ext.prebid.bidder.${APPNEXUS.value} was dropped with a reason: request.imp[0].ext.prebid.bidder.${APPNEXUS.value} failed validation.\n" +
                         "\$: must be valid to one and only one schema, but 0 are valid\n" +
                         "\$: required property 'placement_id' not found\n" +
                         "\$: required property 'inv_code' not found\n" +
                         "\$: required property 'placementId' not found\n" +
                         "\$: required property 'member' not found\n" +
                         "\$: required property 'invCode' not found",
                         "WARNING: request.imp[0].ext must contain at least one valid bidder"]

@JimTharioAmazon
Copy link
Author

Hi, updated the PR with the functional test change. Thanks for reviewing.

osulzhenko
osulzhenko previously approved these changes Feb 27, 2025
extra/pom.xml Outdated
Comment on lines 124 to 128
<dependency>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
<version>${commons-logging.version}</version>
</dependency>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you adding this new dependency?

Copy link
Author

Choose a reason for hiding this comment

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

It's getting pulled in at a vulnerable version by another dependency, and this will pin it at a non-vulnerable version.

Copy link
Collaborator

@Net-burst Net-burst Mar 3, 2025

Choose a reason for hiding this comment

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

@JimTharioAmazon I think what @CTMBNara meant is why the dependency was pulled into the project only to define the version. This is usually an extreme measure. There are 3 material project dependencies pulling in the commons-logging library: httpclient, mockserver-client-java and google-cloud-storage by one of the modules. The correct way forward is to look into the actual usage. I did a quick glance, and you should be able to exclude this dependency from all of them. And the google-cloud-storage dependency could also be bumped. One of the newer versions outright removed this dependency.

extra/pom.xml Outdated
@@ -120,6 +121,11 @@
<artifactId>commons-compress</artifactId>
<version>${commons.compress.version}</version>
</dependency>
<dependency>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leaving a separate remark. This looks unnecessary. This dependency can be excluded from other dependencies that rely on it without any visible issues.

Remove pinned commons-logging version. Add exclusions to httpclient and mockserver-client-java.
Add exclusion for commons-logging.
@JimTharioAmazon
Copy link
Author

Hi, thanks for the feedback again. I've updated the PR. I removed the pinned commons-logging, and added exclusions to httpclient, mockserver-client-java in extra/pom.xml and also to google-cloud-storage in extra/modules/greenbids-real-time-data/pom.xml. I'll look at the versions of google-cloud-storage that dropped commons-logging and see what works there.

Net-burst
Net-burst previously approved these changes Mar 4, 2025
Copy link
Collaborator

@Net-burst Net-burst left a comment

Choose a reason for hiding this comment

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

LGTM with a nitpick due to wrong indentations in src/test/java/org/prebid/server/geolocation/MaxMindGeoLocationServiceTest.java
I won't push to fix this as there is an upcoming project-wide update to Checkstyle enforcement, so there will be a blanket style inconsistency fix happening.

Copy link
Collaborator

@Net-burst Net-burst left a comment

Choose a reason for hiding this comment

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

LGTM

@Net-burst
Copy link
Collaborator

A functional test failure looks like flak. @osulzhenko FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file do not port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants