alphagov / govuk-prototype-kit

Rapidly create HTML prototypes of GOV.UK services
https://prototype-kit.service.gov.uk
MIT License
307 stars 237 forks source link

Remove jQuery from the default kit installation #482

Closed edwardhorsford closed 2 years ago

edwardhorsford commented 6 years ago

I've seen a few messages today about jQuery vulnerabilities < v3. We're currently on 1.11.3.

Should we explore upgrading?

36degrees commented 6 years ago

It's definitely worth exploring updating to 1.12.4 which is the most recent 1.x version. However, this will not fix CVE-2015-9251 which I assume is what you're referring to.

jQuery 1.x is the only version of jQuery that supports IE8.

Unfortunately it is also no longer maintained, which means that the recent jQuery vulnerability is not going to be fixed in 1.x – in fact, by definition, the only way to fix it would require a breaking change, which is not possible without bumping the major version number.

However, as I understand it, CVE-2015-9251 relates only to jQuery's built in AJAX functionality and can be effectively mitigated by ensuring AJAX requests specify a suitable dataType. It's also worth noting that it's been an known issue since 2015, it's just that it's only recently been assigned a CVE number and thus is only now showing up in GitHub's vulnerability reports.

NickColley commented 6 years ago

I think now that we do not have a dependency on jQuery we could consider shipping 3.x for user's who want to use it, or remove it entirely.

joelanman commented 3 years ago

Changing this to bug fix as such an old version of jQuery is a security issue

domoscargin commented 3 years ago

We'll probably have to deal with #1081 first, since the out-of-date step by step pattern requires jQuery.

It has been updated to remove the jQuery dependency (https://github.com/alphagov/govuk_publishing_components/pull/1645).

If we decided to bump our version of jQuery, worth noting that the older step by step has not necessarily been thoroughly tested (https://github.com/alphagov/govuk_publishing_components/pull/927).

joelanman commented 3 years ago

oh good catch thanks!

joelanman commented 2 years ago

I'm leaning to removing it - its not hard for our users to add it themselves, either in /app/assets or by referencing a CDN.

edwardhorsford commented 2 years ago

Is it possible to find out how many users use it?

My guess is that it's probably quite common - so removing it would mean an extra step for users.

joelanman commented 2 years ago

2 thoughts:

  1. The current update process does not touch /app so nothing will change in existing prototypes. This is not great if we make this as a deliberate change to remove old code.

  2. If people need jQuery if we remove it, the steps to add would be:

add this into your prototype, in app/views/includes/scripts.html

<script src="https://code.jquery.com/jquery-3.6.0.min.js" integrity="sha256-/xUj+3OJU5yExlq6GSYGSHk7tPXikynS7ogEvDej/m4=" crossorigin="anonymous"></script>`

However we probably want to point them at https://code.jquery.com/ to get the latest version of this line

joelanman commented 2 years ago

we have had more reports of people using it, and MOJ Frontend relies on it (see above)

domoscargin commented 2 years ago

Would updating jQuery be more or less onerous for users? I'm not entirely familiar with the major changes between v1 and v3 of jQuery, but presumably users would need to migrate - probably beyond our remit to help with this.

Alternatively, if they really needed v1, they could just replace the script src as needed.

domoscargin commented 2 years ago

If we chose to update jQuery, we'd need to:

Izabela-16 commented 2 years ago

Decision is to remove it.

lfdebrux commented 2 years ago

We're removing jquery bit by bit:

lfdebrux commented 2 years ago

This will be fixed in v13.