pip: Warn when ownership changes#14235
pip: Warn when ownership changes#14235martincostello wants to merge 1 commit intodependabot:mainfrom
Conversation
When updating a Python package from PyPI, generate a warning if the ownership of the package has changed.
There was a problem hiding this comment.
Pull request overview
This pull request implements ownership change warnings for Python packages, similar to the existing npm functionality. When a Python package is updated from PyPI, Dependabot will now warn users if the package maintainership has changed between versions, helping identify potential supply chain security risks.
Changes:
- Added
maintainer_changesmethod toPython::MetadataFinderthat checks for ownership changes between package versions - Implemented helper methods to fetch and compare ownership data from PyPI's new ownership API
- Added comprehensive test coverage for various ownership change scenarios including complete ownership changes, partial changes, organization changes, and edge cases
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
python/lib/dependabot/python/metadata_finder.rb |
Implements maintainer_changes method and supporting helpers to detect and warn about ownership changes |
python/spec/dependabot/python/metadata_finder_spec.rb |
Adds comprehensive test coverage for ownership change detection scenarios |
python/spec/fixtures/pypi/pypi_response_ownership_single.json |
Test fixture for single maintainer scenario |
python/spec/fixtures/pypi/pypi_response_ownership_changed.json |
Test fixture for complete ownership change scenario |
python/spec/fixtures/pypi/pypi_response_ownership_partial_change.json |
Test fixture for partial ownership change (some overlap) |
python/spec/fixtures/pypi/pypi_response_ownership_org_changed.json |
Test fixture for organization change scenario |
python/spec/fixtures/pypi/pypi_response_no_ownership.json |
Test fixture for missing ownership data |
| previous_ownership = ownership_for_version(T.must(dependency.previous_version)) | ||
| current_ownership = ownership_for_version(T.must(dependency.version)) | ||
|
|
||
| return if previous_ownership.nil? || current_ownership.nil? |
There was a problem hiding this comment.
Can we please you log this?
There was a problem hiding this comment.
I'm happy to add any logging if you have examples as to how to do it in the right way for this repo.
There was a problem hiding this comment.
I shared an example in your other PR: #14236 (comment)
| rescue JSON::ParserError | ||
| next | ||
| rescue Excon::Error::Timeout | ||
| next | ||
| end |
There was a problem hiding this comment.
Given that this is the same, can it be consolidated under a single rescue? Can we log it?
There was a problem hiding this comment.
I can combine the conditions if desired. Copilot wrote this - I assumed it copied patterns already in use in the repository (as that's what I told it to do) and this seemed sane to me.
I'm not a Ruby dev.
There was a problem hiding this comment.
From your profile, I saw you work using .Net, my understanding is that the try-catch-rescue pattern is common in there too and you may be familiar with that. But the logic here is that if you catch exceptions individually is because you'll be doing different things with them. Otherwise, just catch and handle them both in a single block
In this example, given that you started with two exception handlers, logging the individual exceptions with context make sense to get more comprehensive view of what's happening/understand each failure
|
|
||
| possible_version_listing_urls(version).each do |url| | ||
| response = fetch_authed_url(url) | ||
| next unless response.status == 200 |
| previous_org = previous_ownership["organization"] | ||
| current_org = current_ownership["organization"] | ||
|
|
||
| if previous_org != current_org |
There was a problem hiding this comment.
Would this fire when a package moved from no organization (nil) to having one i.e. a popular project being adopted by a foundation. This might generated unintended noise for benign changes.
Would something like this be more appropriate?
| if previous_org != current_org | |
| if previous_org != current_org && !(previous_org.nil? && current_org) |
Or update the messaging to say exactly what change happened? gained org vs changed org vs lost org.
| return data["ownership"] | ||
| rescue JSON::ParserError | ||
| next | ||
| rescue Excon::Error::Timeout |
There was a problem hiding this comment.
Probably need to rescue Excon::Error::Socket here as well.
There was a problem hiding this comment.
And possibly OpenSSL::SSL::SSLError
|
|
||
| sig { params(version: String).returns(T.nilable(T::Hash[String, T.untyped])) } | ||
| def ownership_for_version(version) | ||
| return nil if version.include?("+") |
There was a problem hiding this comment.
Could you add a test case to cover this case please?
There was a problem hiding this comment.
And can we log this as well?
What are you trying to accomplish?
PyPI recently implemented support for determining the owners of packages: pypi/warehouse#19525
This pull request updates the pip ecosystem so that when updating a Python package from PyPI, a warning is generated if the ownership of the package has changed, similar to the support for this for npm packages.
Anything you want to highlight for special attention from reviewers?
No.
How will you know you've accomplished your goal?
Warnings are generated in pull requests where ownership has changed.
Checklist