This issue was reported privately, but has been approved for public discussion by the Drupal Security Team.
Problem/Motivation
The update module does not sufficiently check if core_version_requirement is a valid version constraint.
Steps to reproduce
- Install a module.
- Issue an updated version of the module with an invalid version number in
core_version_requirementin the.info.ymlfile. Update this module to the new version.Not needed - the existence of the broken release in the updates.d.o release history feed triggers the bug
Proposed resolution
The following was proposed by @MegaphoneJon.
The update module should be checking its inputs rather than accepting them as-is from d.o. And arguably the release process on d.o should be checking for invalid version constraints.
Remaining tasks
- Add tests
- Reviews / refinements
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
TBD
Issue fork drupal-3451701
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3451701-handle-invalid-core-version-constraints
changes, plain diff MR !8261
Comments
Comment #2
quietone commentedComment #5
dwwComment #7
dwwHere's a quick start. Needs tests, but wanted to get an MR open ASAP.
Elevating to critical since this is potentially very bad. 😬 While there are guardrails for the version strings themselves when creating releases on d.o, there's nothing to prevent people from having typos in the
core_version_requirementthey put into tagged releases. Update manager needs to be resilient to those sorts of mistakes, not hard crash with uncaught exceptions. Especially since we generally prevent anyone from fixing a broken release, so once there's a problem, we've got it "forever".Fixing the summary, both formatting, and some technical details.
Comment #8
dwwAdded Unit test coverage that shows the bug and proves the fix.
We probably want to extend some Functional coverage for this, too, since there might be other spots that blow up in this case, and it'd be nice to have an end-to-end test to verify it.
Comment #9
smustgrave commentedThanks @dww for super clear ticket
Test-only was already ran https://git.drupalcode.org/issue/drupal-3451701/-/jobs/1748750 and shows the coverage.
Code review of using a try/catch seems fine and good idea for an immediate fix.
Comment #14
catchCommitted/pushed to 11.x and cherry-picked back through to 10.2.x, thanks!
Comment #16
drumm(Saving to resolve issue indexing issue, please disregard.)