administrate icon indicating copy to clipboard operation
administrate copied to clipboard

Uses vulnerable jQuery versions [1.12.4]

Open alex-benoit opened this issue 4 years ago • 10 comments

https://snyk.io/test/npm/jquery/1.12.4

After fresh rails install: /assets/administrate/application.debug...js included version 1.12.4 of jquery

alex-benoit avatar Nov 17 '19 14:11 alex-benoit

I had a look. We import jQuery 1.12.4 via jquery-rails. It looks like there isn't a newer jQuery 1.x, so we'll have to jump a major version. There seems to be a similar problem with jQuery 2, so we'll have to jump to jQuery 3.

This is bundled with jquery-rails, so initially it should be possible to upgrade by changing the require line in application.js to //= require jquery3. Note that, as I write these lines, jquery-rails bundles jQuery 3.4.1, which is also vulnerable. However there's a jQuery 3.5 (no known vulns), and the jquery-rails team got an issue open to address this just yesterday, so it's reasonable to expect that they'll upgrade.

I had a quick look and tried using jQuery 3.4.1. The datepicker is failing. We are using this one: https://github.com/gracewashere/datetime_picker_rails. It hasn't been updated since 2016, and the repo is archived, so perhaps we should start by using a different one.

Apart from that, there's the question of how much trouble may a major jQuery upgrade cause to our users. The first, most obvious point is browser compatibilty. I feel that jQuery 3 offers reasonable compatibility, including IE9+.

As less obvious matter, and much more difficult to weigh, is whether any and how many users have custom JS that depends on jQuery 1. In any case, we should move on with the times, so perhaps we should just handle this as we handle any other deprecation: informing of this in our changelogs, and waiting for a major version. I want to think that Administrate 1.0 is not too far ahead, and that might be a good moment to do this.

pablobm avatar Apr 23 '20 14:04 pablobm

Another option: remove jQuery altogether.

pablobm avatar Apr 23 '20 14:04 pablobm

It sounds like then that the main reason we include jQuery as a main dependency is for the date picker. I'd be a fan of removing any dependency if we can.

  1. Given it was the right solution back in 2015 (when datetime_picker_rails was forked), is the still the right solution to be thinking about in 2020?
  2. If we plan to remove jQuery for Administrate 1.0, that gives us the next release to deprecate it. How can we highlight that we're doing that? (I'm assuming people won't necessarily read the CHANGELOG!)
  3. For user impact: It's really hard for us to know what that'll be, but it seems a reasonable assumption that if someone has extended their JavaScript, it'll be jQuery specific.

nickcharlton avatar Apr 23 '20 14:04 nickcharlton

I would vote for removing the dependency on jQuery instead of upgrading it, if at all possible.

sedubois avatar Apr 23 '20 14:04 sedubois

An alternative to removing can be using scoping so that jQuery does not go into global scope. I haven't looked how easily the stack accommodates this. This would make jQuery upgrades less painful in the future.

betelgeuse avatar Apr 27 '20 08:04 betelgeuse

@nickcharlton - Yay to removing it. To highlight the change, I think we should have an "upgrade guide" in our readme, listing this and any other stuff we think may break compatibility, such as the change to :searchable_fields. This can suggest that users include their own copy of jQuery if they depend on it, and we can direct them to use jQuery.noConflict or something similar to reduce the possibility of conflicts.

@betelgeuse - I think that leaving jQuery, de-scoped, would not work well here. It opens the door to people using several versions of jQuery at the same time, and we'd still be keeping it for the sake of a single library that is unmaintained. However your idea could help as a suggestion on the upgrade path, as I mention above.

pablobm avatar May 01 '20 07:05 pablobm

I'm currently investigating Administrate to determine whether I can use it for some upcoming projects. I'm liking a lot of what I've seen and I'm about to set it up in a project today 👍

I just wanted to comment that I do find it a bit troubling that there is still a dependency on jquery in any version, but particularly 1, and that the reason for its usage is just one type of input. For my projects, it means I would have to have jquery as a dependency exclusively for this element, which normally I wouldn't use. I'm actually not really sure what the benefit of this datepicker is over using an input with a date or datetime-local type (rails' date_field and datetime_local_field for forms [edit: these do have limited cross-browser support, maybe that's the benefit]) and I can't find a working demo of this one anymore (both datetime_picker_rails and the bootstrap one it's based off of link to apparently broken demo pages).

The first thing I saw when coming to the project repo is the front and center warning about how there may be breaking changes. It seems to me this project is well-positioned to overcome anything involving removing the datepicker & jquery by warning of this possibility in general from the get go. Just my 2 cents as a new user who is looking forward to seeing what can be done with Administrate!

AFlowOfCode avatar Jul 18 '20 21:07 AFlowOfCode

I learned this week that <input type="date"> is in the current Safari Technology Preview and so that's likely going to be out soon. When @AFlowOfCode made their last comment, I didn't quite realise that this was what they'd meant: will we be able to just drop the datetime_picker_rails completely?

It seems like once the new version of Safari is out we could do that. By the time we do our next release it'll be a good time after I'd expect Safari users to have upgraded and that saves a bunch of work.

nickcharlton avatar Nov 06 '20 20:11 nickcharlton

Now that Safari 14.1 was released with support for <input type="date" /> I looked into this and it's fine. I think though, datetime fields won't be supported well as datetime-local isn't supported in current Firefox, with no release coming soon. datetime-local is supported in Safari, but I believe it doesn't allow for time selection.

I recommend not waiting for browser releases to remove a vulnerable gem/library. Perhaps implementing with a different date picker (like flatpickr.js.org) would be better.

Cory-Christensen avatar May 13 '21 19:05 Cory-Christensen

I think that once that https://github.com/thoughtbot/administrate/pull/2116 and https://github.com/thoughtbot/administrate/pull/2136 are merged, we'll be able to take on this one once and for all.

pablobm avatar Feb 10 '22 16:02 pablobm

As of #2397, we're shipping jQuery v3.7.0

https://github.com/thoughtbot/administrate/blob/2a1bf79f3d43cb556232aab4d3d22aaefafa8b73/package.json#L9

So, finally, I can close this. Thanks for opening it!

nickcharlton avatar Feb 02 '24 18:02 nickcharlton