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

Remove jQuery from the default kit installation #482

Closed
edwardhorsford opened this issue Jan 23, 2018 · 14 comments
Closed

Remove jQuery from the default kit installation #482

edwardhorsford opened this issue Jan 23, 2018 · 14 comments
Labels
🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) 🕔 Days A few unknowns, but we roughly know what’s involved.
Milestone

Comments

@edwardhorsford
Copy link
Contributor

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

Should we explore upgrading?

@36degrees
Copy link
Contributor

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
Copy link
Contributor

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 joelanman changed the title Should we update our version of jQuery? Update jQuery Oct 17, 2018
@36degrees 36degrees added this to the v9.0.0 milestone Oct 17, 2018
@joelanman joelanman added the Feature Request User requests a new feature label Oct 17, 2018
@timpaul timpaul added 🕔 Days A few unknowns, but we roughly know what’s involved. Priority: low labels May 20, 2019
@36degrees 36degrees removed this from the v9.0.0 milestone Jul 9, 2019
@joelanman joelanman changed the title Update jQuery Update or remove jQuery Sep 7, 2021
@joelanman
Copy link
Contributor

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

@joelanman joelanman added 🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) and removed Feature Request User requests a new feature labels Sep 8, 2021
@domoscargin
Copy link
Contributor

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 (alphagov/govuk_publishing_components#1645).

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

@joelanman
Copy link
Contributor

oh good catch thanks!

@domoscargin domoscargin added this to the v11.0 milestone Nov 2, 2021
@domoscargin domoscargin changed the title Update or remove jQuery Remove jQuery Dec 2, 2021
@joelanman
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@domoscargin
Copy link
Contributor

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
Copy link
Contributor

domoscargin commented Dec 7, 2021

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

  • point out that users can downgrade manually at their own risk
  • point them to jQuery's migration plugin to get their prototypes working with v3+

@domoscargin domoscargin changed the title Remove jQuery Update/Remove jQuery Dec 7, 2021
@Izabela-16
Copy link

Decision is to remove it.

@lfdebrux
Copy link
Member

lfdebrux commented Jun 29, 2022

@lfdebrux lfdebrux changed the title Update/Remove jQuery Remove jQuery from the default kit installation Jul 1, 2022
@lfdebrux
Copy link
Member

This will be fixed in v13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) 🕔 Days A few unknowns, but we roughly know what’s involved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants