-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Upgrade rexml to 3.3.4 to address CVE-2024-39908, 41123, 41946 #5181
Upgrade rexml to 3.3.4 to address CVE-2024-39908, 41123, 41946 #5181
Conversation
activemerchant.gemspec
Outdated
@@ -26,7 +26,7 @@ Gem::Specification.new do |s| | |||
s.add_dependency('builder', '>= 2.1.2', '< 4.0.0') | |||
s.add_dependency('i18n', '>= 0.6.9') | |||
s.add_dependency('nokogiri', '~> 1.4') | |||
s.add_dependency('rexml', '~> 3.2.5') | |||
s.add_dependency('rexml', '~> 3.3.2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why it has to be so tight? This will still prevent minor version upgrades, which generally shouldn't cause breakages. Would the following be suitable to pin to major version 3 while staging above the version required by the CVE?
s.add_dependency('rexml', '~> 3.3.2') | |
s.add_dependency('rexml', '~> 3.3', '>=3.3.2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That solves the immediate problem, but doesn't prevent this scenario from happening again.
To help avoid this type of problem in the future, and to better align with the other add_dependency
lines in the gemspec, I think we would prefer this:
s.add_dependency('rexml', '>= 3.3.2')
In other words, I don't think we have any reason for a pessimistic version constraint (meaning ~>
) on rexml
, because there doesn't seem to be any reason to believe future versions would break active_merchant
.
For background, the original pessimistic constraint was added in eed45fe simply. There is no documented reason for a pessimistic constraint, so I think the version constraint was overly tight from the beginning. The gemspec declarations for activesupport
, builder
, and i18n
do not have pessimistic constraints, which seems correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dan-jensen Assuming that rexml
is following semantic versioning correctly, wouldn't we want a conscious, verified step to any 4.0 version to check for breaking changes, however remote they may be? As such I think the suggestion from @ozydingo sets a sensible base and ceiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@damienjbyrne you're right, it's definitely sensible. But there is a tradeoff. We can choose to prioritize breaking changes or vulnerabilities. Breaking changes tend to be obvious, and easy to resolve (simply don't change). Vulnerabilities tend to be non-obvious, and harder to resolve (because change is required). I think it makes sense to prioritize vulnerabilities, and flexibility in general, like the majority of this gemspec already does. But I didn't intend to cause a delay here. If there's doubt about flexible specifications, I support merging this PR now.
Merging this would be great, because after upgrading rexml to 3.3.2, activemerchant is currently forced back to 1.126.0 |
@Heavyblade you seem to be a maintainer, would you have time to review & merge this? (Simple change, already with 2 community approvals.) |
And now there's a another DoS vulnerability, CVE-2024-41123, requiring rexml 3.3.3. |
This resolves CVE-2024-39908 : DoS in REXML
c5dc655
to
1efd694
Compare
|
||
assert_failure response | ||
assert response.test? | ||
assert_equal 'Missing parameter: UserId.', response.message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should remove Mercury and TransFirst completely in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! these gateways are still active, you cannot remove them
I'm not a regular maintainer but got merge rights, and this is blocking for many people so I'll take the initiative to merge it. |
@raymzag @casperisfine hello! this PR breaks the mercury gateway and it is still in use. Can you open a PR to fix it or revert this one? |
@aenand Do you have a link to mercury's documentation? |
Unfortunately not. I did put up a PR to try and resolve the issue for their gateway so maybe that will suffice. We're syncing up internally tomorrow to see if it'll work |
This refers to #5206 in case anyone is looking. |
@aenand apologies I didn't fix the test properly. It was hard to tell they were in use. No website/docs, no recent commits in those gateways and remote tests have been failing in master don't know since when. Thanks for putting up the fix. |
…emerchant#5181) * Upgrade rexml to 3.3.2 This resolves CVE-2024-39908 : DoS in REXML * Apply suggestion * Fix tests * Upgrade rexml to 3.3.4
This resolves CVE-2024-39908 : DoS in REXML and #5180
This vulnerability is showing up in bundle-audit.
Tests
bundle exec rake test:local
ruby -Itest test/remote/gateways/remote_paypal_test.rb
This fails, but it seems to be failing on
master
branch anyway. Not sure if still active. https://www.mercurypay.com/ does not exist anymore.ruby -Itest test/remote/gateways/remote_mercury_test.rb