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

[CLOUDSTACK-9261] Upgrate jQuery-UI to 1.11 (JQuery UI 1.8.4 prone to XSS) #2524

Merged
merged 5 commits into from Jul 16, 2018

Conversation

@rafaelweingartner
Member

rafaelweingartner commented Apr 3, 2018

Description

This PR addresses an old security issue regarding jQuery-UI. We are updating jQuery-UI to version 1.11, instead of 1.12, because the 1.12 version requires an update of jQuery.js as well. Therefore, to reduce the surface of changes, we are first only updating the jQuery-UI library.

Moreover, unnecessary CSS and image files from the jQuery-UI library were deleted.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

I tested the changes introduced here by generating the RMP packages, updating ACS in a test environment and then using ACS UI to see if I something is not working. So far I have tracked and fixed all of the problems I encountered. I would like to get some help on testing this as well. Therefore, if you have some minutes/hours to spare, please do help.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    Testing
  • I have added tests to cover my changes.
  • All relevant new and existing integration tests have passed.
  • A full integration testsuite with all test that can run on my environment has passed.

@blueorangutan package

@blueorangutan

This comment has been minimized.

blueorangutan commented Apr 3, 2018

@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

This comment has been minimized.

blueorangutan commented Apr 3, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1870

@rafaelweingartner rafaelweingartner requested a review from nvazquez Apr 3, 2018

@khos2ow

This comment has been minimized.

Contributor

khos2ow commented Apr 3, 2018

Thanks Rafael for the PR, but keep in mind that there are lots of changes in these two versions (and considering some functionalities won't work with older JQuery itself, so that needs to be upgraded) and most of the time these are not backward-compatible. Going forward with this PR requires a lot of manual -in browser- testing.

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Apr 3, 2018

Yes, we need more manual testing. I already tested the most common features in ACS UI.

jQuery-UI 1.11 is compatible with our jQuery version. So, there should be no problems there. On the other hand, jQuery-UI changed quite considerably from 1.8 to 1.11. I believe I caught most of the problems, but there might be something I have not tested yet.

@khos2ow

This comment has been minimized.

Contributor

khos2ow commented Apr 3, 2018

Cool, I didn't expect them to be compatible!!

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Apr 3, 2018

The 1.12 (the latest version) is not. However, the 1.11 is. The fix for the XSS vulnerability was first introduced in jQuery-UI 1.10.

@resmo

This comment has been minimized.

Member

resmo commented Apr 3, 2018

Any reason not targeting 4.11.1 (4.11 branch) ?

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Apr 3, 2018

@resmo yes there is a reason. The possibility of getting conflicts and having to solve them when merging forward to 4.12.

It took me over 16 hours to track all of the problems after I updated the library. The Javascript part of ACS and the ad-hoc framework that we have to generate pages is pretty annoying to debug and track problems; especially when there is no error or stack trace. Sometimes information was not appearing, or windows were rendered in an odd way, and I needed to track what was going on under the hood debugging from the most basic method that I knew until the "framework" that is generating HTML elements at runtime.

@resmo

This comment has been minimized.

Member

resmo commented Apr 4, 2018

@rafaelweingartner thanks for explanation.

@rhtyd

There are several changes that make break the UI, also if this is security related 4.11 can benefit from the change. Can you remove the new jquery lib with minified versions, as the added library is 16k+ loc.

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Apr 5, 2018

@rhtyd Changes break or may break? I have tested this. At least all of the functions I used are working just fine.

As I said here before, I know that this update can break some code. I executed a lot of testing so far, and I believe I caught and fixed most of the problems caused by this upgrade. That is why I asked for help, to get different eyes testing this.

Deploy VM, create zone, create VPC, create ACLs, create and edit ACL rules, create and edit LB rules, create and edit roles, create affinity groups, storage (create volume, edit, upload), networks (add l2, add guest, add isolated) and so on.

I can use the minified version of jQuery-UI, but bear in mind that our jquery.js is not minified. It looks like we have many other libraries that are not using minified version either. I only used the non-minified version to help during debugging.

I will not open a PR against 4.11, as I explained before to @resmo. This is a long-standing issue that I decided to address, and I intend to target master only.

I will wait for the community feedback on both.

  • should I use minified version or non-minified ones?
  • is the community ok with me opening this against master only?
@GabrielBrascher

This comment has been minimized.

Member

GabrielBrascher commented Apr 16, 2018

I spent some time testing it in a test environment and it seems to be all good. However, two buttons need some attention:
(i) 'About' button: here the user cannot get out of the info screen unless clicking on some button as the 'add instance' (or a respective button in another view, as the 'add network offering', 'add' a template, ...).

image

(ii) 'Add network offering' button on Home > Service Offerings - Network Offerings: in this case, the add network offering view requires a scroll down from the user.

image

It should be something like the 'Add compute offering':
image

Thanks for the PR @rafaelweingartner . So far these are the points that I could raise in my test environment.

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Apr 17, 2018

Thanks @GabrielBrascher! I fixed both of the issues you raised.
What do you guys think? Should I change the non-minified version of Jquery-Ui to the minified one?

Others (@khos2ow, @rhtyd, @DaanHoogland and so on), could you also help here?

@rhtyd

rhtyd approved these changes May 1, 2018

LGTM, subject to testing. This requires a lot of manual testing of UI to be merged.

@rhtyd

This comment has been minimized.

Member

rhtyd commented May 1, 2018

@rafaelweingartner we'll need to manually test this, I've approved this in theory. I will still prefer a minified jquery library instead of additing another 17k loc to the repo of dependency.

To allow this to be merged, we'll need someone other than the author to manually test the UI and its several views.

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented May 1, 2018

@rhtyd I totally agree. That is why I asked for help.

Even though I tested all of the features we use here, on a second batch of test @GabrielBrascher caught some other problems.

I tested with an advanced setup. Whereas, I think, @GabrielBrascher tested with a basic setup.

@GabrielBrascher

This comment has been minimized.

Member

GabrielBrascher commented May 16, 2018

@rafaelweingartner @rhtyd I tested with a basic network. All points that I raised were fixed by @rafaelweingartner; however, I might have missed some use case when manually testing (e.g. an action such as configure IP range, start VM ...).

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Jul 13, 2018

@GabrielBrascher did you approve the PR?
I think everything is fine to merge then....

@GabrielBrascher

LGTM based on building from the source and testing it.

@rafaelweingartner

This comment has been minimized.

Member

rafaelweingartner commented Jul 16, 2018

I will proceed and merge this one then. Thank you all for the help here.

@rafaelweingartner rafaelweingartner merged commit d0c6cac into apache:master Jul 16, 2018

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@blueorangutan

This comment has been minimized.

blueorangutan commented Jul 16, 2018

@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

This comment has been minimized.

blueorangutan commented Jul 16, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2179

borisstoyanov added a commit to shapeblue/cloudstack that referenced this pull request Jul 23, 2018

[CLOUDSTACK-9261] Upgrate jQuery-UI to 1.11 (JQuery UI 1.8.4 prone to…
… XSS) (apache#2524)

* [CLOUDSTACK-9261] Upgrate jQuery-UI to 1.11 (JQuery UI 1.8.4 prone to XSS)

* fix problems in the UI for lbCertificatePolicy and StaticNAT

* force jenkins build

* Fix about dialog

* Fix position of network service offering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment