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

Changes regarding XML parser #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Sep 4, 2024

This is related to the rexml gem and the recent flood of updates of that gem with CVE fixes. It handles a couple of issues.

First, the xmlrpc gem depends on a parser: either rexml or libxml-ruby. If you just create an new Gemfile with just the xmlrpc gem, both of them are unavailable and your request will fail with a LoadError because rexml is not available. To fix this, add rexml to the dependencies instead of just a development dependency.

Second, the current CI tries to run all available parsers, but since its missing the libxml-ruby gem it does not test this parser. Add this gem to the development dependencies (it would be better as a test dependency, but we don't have those yet). I've skipped Windows as platform, since the CI is missing the development headers for those. It might be worth it to rewrite this to nokogiri and use the binary gems provided.

The third commit is the one where I'm a bit unsure. It changes the default parser to libxml-ruby, and falls back to rexml if that fails. The code is a bit hacky and repeating, and it also means that we no longer have a real dependency on rexml, it can be replaced by libxml-ruby. So the actual dependency is something like gem 'rexml' | 'libxml-ruby', but afaik there is no either-style syntax for gemfiles.
It would be possible to remove the runtime dependency on rexml, you would still get a LoadError when you try to load it with missing dependencies, but at least now it happens on require, not once you make the actual RPC call.

@herwinw
Copy link
Member Author

herwinw commented Sep 4, 2024

The failing tests are some issue with Ruby 3.4 and webrick, these can be reproduced on the master branch and are not related to this PR.

It looks like a bug in webrick with recent versions of uri, ruby/webrick#144 fixes it, but is not released as of now.

@kou
Copy link
Member

kou commented Sep 5, 2024

First, the xmlrpc gem depends on a parser: either rexml or libxml-ruby. If you just create an new Gemfile with just the xmlrpc gem, both of them are unavailable and your request will fail with a LoadError because rexml is not available. To fix this, add rexml to the dependencies instead of just a development dependency.

Accept.

Second, the current CI tries to run all available parsers, but since its missing the libxml-ruby gem it does not test this parser. Add this gem to the development dependencies (it would be better as a test dependency, but we don't have those yet). I've skipped Windows as platform, since the CI is missing the development headers for those. It might be worth it to rewrite this to nokogiri and use the binary gems provided.

Accept.

The third commit is the one where I'm a bit unsure. It changes the default parser to libxml-ruby, and falls back to rexml if that fails. The code is a bit hacky and repeating, and it also means that we no longer have a real dependency on rexml, it can be replaced by libxml-ruby. So the actual dependency is something like gem 'rexml' | 'libxml-ruby', but afaik there is no either-style syntax for gemfiles. It would be possible to remove the runtime dependency on rexml, you would still get a LoadError when you try to load it with missing dependencies, but at least now it happens on require, not once you make the actual RPC call.

Reject. We can avoid the LoadError on an actual RPC call by the "First", right?

Could you open separated PRs for "First" and "Second" instead of a combined PR like this? I'll merge them.

This should use libxml2 if available, and fall back to rexml otherwise.
Recent version of rexml have a tag limit to mitigate some DoS attacks,
which can easily be reached with the verbosity of XML-RPC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants