search

ruby / xmlrpc

The Ruby standard library package 'xmlrpc'
Other
37 stars 26 forks source link

Changes regarding XML parser #50

Open herwinw opened 2 months ago

herwinw commented 2 months ago

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 commented 2 months ago

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, https://github.com/ruby/webrick/pull/144 fixes it, but is not released as of now.

kou commented 2 months ago

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.