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

Bump dependencies for nokogiri, ruby, and rubyzip. #2

Merged
merged 1 commit into from Oct 2, 2019

Conversation

inopinatus
Copy link
Contributor

nokogiri <1.10.4 and rubyzip < 1.2.3 had CVEs and/or regressions, and Ruby 2.4.7 is the minimum maintenance level.

@noniq
Copy link
Member

noniq commented Sep 26, 2019

I’m not sure if we should drop older Ruby versions as long as the tests pass successfully. @straydogstudio @simi What do you think?

@straydogstudio
Copy link
Contributor

@simi @noniq I think it depends on what we want to support. It can result in failed tests that cause confusion down the road.

@noniq
Copy link
Member

noniq commented Sep 26, 2019

I have a slight preference for a pragmatic approach:

Right now, the tests still pass even on older versions of Ruby. So let’s keep this versions in the build matrix for now. If there is a pull request causing tests to fail on older versions: Check if it is trivial to adapt the PR so that the code still works on these version. If so, adapt the PR. If not, officially declare the affected versions as unsupported and delete them from the build matrix.

But as said, this is a slight preference only. Let’s wait for others to comment.

@straydogstudio
Copy link
Contributor

I am fine with the pragmatic approach. It could break a lot of installs to restrict older ruby versions, so we may as well wait until it is necessary.

Note I had to limit Travis CI for axlsx_rails to Ruby 2.4.7 or newer to get tests to pass for Rails 6. I could adjust the matrix to exclude Rails 6 from previous Ruby versions and get those other tests passing again to match. I will look at that. The gemspec requirements are not so restrictive.

@inopinatus
Copy link
Contributor Author

inopinatus commented Sep 29, 2019

PR bumped for latest rubyzip CVE.

The minimum Nokogiri level has a minimum Ruby of 2.3, so any tests below that will fail.

Note that the latest rubyzip 2.0 has a constraint of >= 2.4 in the gemspec, and the upcoming nokogiri 1.11 milestone has their, er, equivalent. Given which I suggest there's little point maintaining test configurations for Ruby below 2.4 once they start to break.

For consensus I relaxed the Ruby version constraint in the PR back to ~> 2. Not so much for security now, but I still think it's reasonable to signal to contributors that it's okay to use Ruby 2.x idioms. Given that minimum secure Nokogiri forces us to minimum Ruby 2.3, I've followed suit in the revised PR to signal to contributors that it's okay to use Ruby 2.3 syntax & methods.

nokogiri <1.10.4 and rubyzip < 1.3.0 had CVEs and/or regressions,
and whilst we're here, signal that it's okay to use Ruby 2.3 syntax
and methods and later.
@straydogstudio
Copy link
Contributor

straydogstudio commented Oct 1, 2019

@inopinatus @noniq I think we have a few issues that have surfaced here.

First, should tests continue to be used if they support outdated dependencies, e.g. ruby 2.2. I don't think tests should be removed provided they are allowed in the context of runtime requirements. As @noniq described, it lets us know when a version should be officially unsupported.

Second, should caxlsx require users to use up to date dependencies. This has consequences for the end user. Nokogiri, for example, is used all over the place. I can imagine (without any real example) of a situation where the latest caxlsx version would help even when an older unsupported nokogiri is required. Should the caxlsx gem require new versions? Or, should it deprecate them and spit out warnings? As long as the function of caxlsx is not hindered by older dependencies I don't think they should be restricted. In my experience real situations have extended insecure configurations on the way to a secure one.

I am not, however, trying to push this as an answer. It does make things flexible for the end user while making them aware of their old dependencies.

@inopinatus Does this make sense to you? Can you provide an argument for your pull request?

@inopinatus
Copy link
Contributor Author

inopinatus commented Oct 1, 2019

@straydogstudio I think - or more accurately, "I am not a lawyer, and I am not your lawyer, but I have been informed by counsel" - that deliberately deciding to allow a third party to continue with an insecure package amounts to negligence and is possibly actionable in civil or criminal proceedings. To avoid liability I'd never knowingly/willingly do so in any product I work on professionally. A gem is a product that installs dependencies as part of that product, possibly to customers unaware they are doing so. Those dependencies are, to all intents and purposes, distributed with it and as part of it.

Ergo, if someone installs my product, and it pulls in an insecure dependency, then the only way I can see to discharge my duty of care to them is to ensure that an upgrade is issued that resolves the vulnerability i.e. upgrades the dependency.

By "duty of care" I mean simultaneously in the ethical and legal senses, but the point I'm making is mostly legal because a moral/ethical argument would get personal fast. (Nonetheless I do feel it's an ethical duty)

Fun fact: software licenses do not protect against being sued or prosecuted. The warranty disclaimer is a possible defense in civil proceedings, but it doesn't stop those civil proceedings occurring. So you hire lawyers and go to court, which is an horrific, expensive, and embarrassing process even when sanity and licenses and your Professional Indemnity Insurance Cover are on your side. And software licenses are worthless against charges of criminal negligence, so let's hope no-one got hurt or national secrets exfiltrated. And in my country (Australia), disclaimers of liability aren't hard barriers and may be set aside the moment they bump into statutory rights, such as fitness for purpose, freedom from defect etc.

"The more steps which the developer takes to detect and correct defects in their software, the less likely it is they will be liable in negligence for any defects in the software.". It's a quote, not sure where from originally, but it's in my "building a product business" notes.

I don't actually think it's very likely to get sued or prosecuted over an old version of a spreadsheet generator quietly installing an insecure Nokogiri and then failing to upgrade it in later updates, but stupider legal actions have occurred, particularly in the software industry, and after yearsdecades of designing and shipping products, observing every dot of legal advice scrupulously is an ingrained habit for me.

The upside of this legal paranoia is that you can use recent features with confidence, should they prove useful.

TL;DR: if you're publishing software, depend only on maintained products, and drop everything else like a hot potato.

@inopinatus
Copy link
Contributor Author

inopinatus commented Oct 1, 2019

By revealing my position I have infected you with a memetic virus, for which my apologies.*

*Should the worst occur, the only way any of you can even hope to maintain plausible deniability from hereon is by refraining from any followup comment, octosmileys, PR merges, deleting any email notifications, changing your name, growing (or shaving off) a beard/hair, and moving to an off-grid desert bunker. Sorry. Did you read this too? Oh dear

@straydogstudio
Copy link
Contributor

Ha! Yes. I'm afraid I am vulnerable, but I won't be shaving my beard. I've been in legal proceedings and they are not nice.

If we can tag older versions, and make a 2.0.2 release to provide a stop gap, we can go forward with this pull request. I would like to provide usable upgrade paths for users of the existing gem.

@noniq
Copy link
Member

noniq commented Oct 1, 2019

I’ll create a PR for a release of 2.0.2

@straydogstudio
Copy link
Contributor

Thanks @noniq

@inopinatus
Copy link
Contributor Author

inopinatus commented Oct 1, 2019

Note that my argument does not "hold water" for Ruby versioning, for two reasons:

  • A user had to explicitly choose to install Ruby first, to install this gem. Or, Ruby was distributed to them by another party e.g. Ubuntu. Either way it's not part of the distribution of this gem.

  • There's no reasonable way to even induce the correct upgrade. Setting s.required_ruby_version = '>= 2.4.8' doesn't upgrade a vulnerable 2.5.6 to 2.5.7, and s.required_ruby_version = '>= 2.6.5' means our revised gem can't be installed by 2.5.7 users.

As a result I think there is no duty of care, ethical or legal, regarding Ruby version. I included it in the PR because it's an adjacent consideration through the general topic of dependencies and probably best handled simultaneously.

My view on required_ruby_version is that it has more to do with signalling to users about what's tested & supported, to contributors about which syntax and standard library methods can be used, and (for gems that have a native element) setting minimum ABI level.

Once Nokogiri and Rubyzip update, all tests on 2.3 and below may start failing, hence my suggestion of ~> 2.3 for now and probably ~> 2.4 in future.

NB: I know I write quite forcefully, but it is still only one man's opinion about how things should be done.

@straydogstudio
Copy link
Contributor

@inopinatus I would far rather have a clear strong statement than a fuzzy one! No worries on my end.

@straydogstudio straydogstudio merged commit 94a9419 into caxlsx:master Oct 2, 2019
@straydogstudio
Copy link
Contributor

@noniq I have created a 2.0.2 branch from the commit just before the changes we started.

@straydogstudio
Copy link
Contributor

@noniq Is it sufficient as is? Or does it need changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants