pkp/pkp-lib#9753 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports #10167

blesildaramirez · 2024-07-05T12:12:28Z

For v3.3.0, we need to:

Upgrade jQuery from v3.5.1 to v.3.7.1
Upgrade jQuery validation from v1.11.1 to v1.19.5
Upgrade jQuery UI from v1.12.1 to v1.13.3
Update submodule
- OJS: ojs:stable-3_3_0 PR
- OMP: omp:stable-3_3_0 PR
- OPS: ops:stable-3_3_0 PR

Notes:

My investigation indicates that there are no breaking changes in the upgrades for jQuery, jQuery UI, and jQuery Validation.
In Composer, we updated the version of components/jquery and removed components/jqueryui since the newer version is not available through Composer. We then added jquery-ui and jquery-validate as custom repositories in Composer, sourcing them from npm registry tar files.
We copied these Composer repository installs to the existing paths for the mentioned dependencies, so changing the paths when adding these scripts in the frontend is NOT necessary for pkp:v3.3.0
Usage statistics that use Chart JS is not yet implemented in v3.3, so there's no need to do a version upgrade of this on the frontend.

CLAassistant · 2024-07-05T12:12:33Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

jonasraoni · 2024-08-13T13:29:15Z

classes/dev/ComposerScript.php

+		function copyDir($src, $dst) {
+			$dir = opendir($src);
+			@mkdir($dst, 0755, true);
+			while (false !== ($file = readdir($dir))) {
+				if ($file != '.' && $file != '..') {
+					if (is_dir($src . '/' . $file)) {
+						copyDir($src . '/' . $file, $dst . '/' . $file);
+					} else {
+						copy($src . '/' . $file, $dst . '/' . $file);
+					}
+				}
+			}
+			closedir($dir);
+		}


I think you might use existing code to copy the directory, it will also have better permission defaults:

import('lib.pkp.classes.file.FileManager'); $fileManager = new FileManager(); $fileManager->copyDir($source, $destiny);

Hi @jonasraoni , thanks for checking this. I tried to implement your suggestion, but I'm running to lots of issues with importing the FileManager. When I run the composer install, I'm getting lots of "not found" errors so I did something like this when testing the use of FileManager:

require_once './includes/bootstrap.inc.php'; require_once './tools/core/Core.inc.php'; require_once './classes/core/Core.inc.php'; require_once './classes/config/Config.inc.php'; require_once './classes/file/FileManager.inc.php'; $fileManager = new \FileManager();

Finally, I run to an issue where the constant "INDEX_FILE_LOCATION" is undefined. As I check, this is defined on the app level of OJS/OMP/OPS.

I noticed a comment from this file ComposerScript.php from the main branch, that we did not use "Core::getBaseDir()" because the script is being called directly by composer and that "INDEX_FILE_LOCATION" is expected to be undefined. Would you be able to provide further guidance on this? Thanks!

This should probably work:

require_once './tools/bootstrap.inc.php'; import('lib.pkp.classes.file.FileManager'); $fileManager = new FileManager(); $fileManager->copyDir('source', 'destiny');

This worked, thank you! I updated the script to use FileManager, please check. Thanks!

Btw, I needed to update the FileManager file for 3.3 - specifically on the mkdir function:

I added this because when I re-run the composer install (with already copied files), I get an error from php saying:

Weirdly, I don't get this error on 3.4 so I didn't change the FileManager file for 3.4.

To not cause side-effects, the return true; should be probably modified to return false;.
Or modified to return $this->setMode($dirPath, DIRECTORY_MODE_MASK); to honor the expectations of the caller.

I actually think it would be better not to use FileManager, since this is code that's run upon Composer dependency installation, long before there's a config.inc.php configuration file. FileManager expects the OJS bootstrapping process to be present, and it won't be.

Thanks both, I updated the script to not use FileManager.

jonasraoni · 2024-08-13T13:49:12Z

classes/dev/ComposerScript.php

+		} catch (Exception $e) {
+			error_log($e->getMessage());
+		}


I think it's better to let the code fail, it's important that the user get this working properly and a simple warning might be ignored.

composer.json

jonasraoni · 2024-08-13T13:54:59Z

classes/dev/ComposerScript.php

+			// jQuery UI
+			if (!file_exists($vendorComponents . '/jqueryui')) {
+				mkdir($vendorComponents . '/jqueryui', 0755, true);
+			}


Can be removed if you use the FileManager.

jonasraoni · 2024-08-13T13:55:15Z

classes/dev/ComposerScript.php

+			if (!file_exists($jqueryPluginsDir . '/validate')) {
+				mkdir($jqueryPluginsDir . '/validate', 0755, true);
+			}


Can be removed if you use the FileManager.

jonasraoni · 2024-08-13T13:58:22Z

composer.lock

@@ -7746,5 +7686,5 @@
    "platform-overrides": {
        "php": "7.3.0"
    },
-    "plugin-api-version": "2.3.0"
+    "plugin-api-version": "2.6.0"


Not sure if this might cause compatibility issues, it sounds safer to leave the old value.

jonasraoni · 2024-08-13T14:07:46Z

classes/dev/ComposerScript.php

+			copy($jqueryUiDist . '/jquery-ui.js', $vendorComponents . '/jqueryui/jquery-ui.js');
+			copy($jqueryUiDist . '/jquery-ui.min.js', $vendorComponents . '/jqueryui/jquery-ui.min.js');


There's a FileManager::copyFile() too, which will also setup permissions.

…ndency

…ugins

…n to composer dependencies

…validation paths

…itignore

…unning composer install

…dation

… exists

jonasraoni · 2024-08-14T19:50:38Z

classes/dev/ComposerScript.php

+		];
+
+		// jQuery UI
+		$fileManager->copyFile($source['jquery-ui.js'], $dest['jquery-ui.js']) || throw new Exception('Failed to copy jquery-ui.js to destination folder');


Just tagging @asmecher to confirm if it's ok this breaking change, regarding the required permissions (https://docs.pkp.sfu.ca/admin-guide/en/troubleshooting#configuring-file-permissions).

This script is run by Composer during dependency installation, not when OJS is installed and in production. For the most part, it'll be run from the package build script when I build a release, and for folks that host OJS from git it'll be run early in the installation process from the same terminal that's used to e.g. clone the code into place. I don't think the production file permission document is relevant. (This is also why I recommended not using FileManager -- it's intended for execution in the bootstrapped PKP environment, not elsewhere.)

…reating a dir

blesildaramirez force-pushed the i9753-3_3_0 branch from 7d25292 to 9dbfc07 Compare July 10, 2024 10:24

blesildaramirez marked this pull request as ready for review July 10, 2024 12:09

blesildaramirez force-pushed the i9753-3_3_0 branch 2 times, most recently from 9952819 to 454635a Compare July 18, 2024 16:10

blesildaramirez force-pushed the i9753-3_3_0 branch from 454635a to 07913a6 Compare August 12, 2024 15:45

blesildaramirez changed the title ~~pkp/pkp-lib#9754 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports~~ pkp/pkp-lib#9753 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports Aug 13, 2024

jonasraoni requested changes Aug 13, 2024

View reviewed changes

blesildaramirez added 18 commits August 15, 2024 01:28

pkp#9753 [stable-3_3_0] Update jquery version from 3.5.1 to 3.7.1

61ec3bd

pkp#9753 [stable-3_3_0] Update jquery validation from v1.11.1 to v1.19.5

f8534d6

pkp#9753 [stable-3_3_0] Remove components/jqueryui as a composer depe…

18ec6b9

…ndency

pkp#9753 [stable-3_3_0] Manually add jquery-ui v1.13.3 to jquery plugins

3558cc6

pkp#9753 [stable-3_3_0] Move import of jqueryui from vendors to js pl…

037c3f4

…ugins

pkp#9753 [stable-3_3_0] Remove jquery-ui local files

5bebaf5

pkp#9753 [stable-3_3_0] Add jquery-ui dependency to composer from npm

22c2a69

pkp#9753 [stable-3_3_0] Remove jquery-validation local files

11f1508

pkp#9753 [stable-3_3_0] Add jquery-validation to composer from npm

281ba42

pkp#9753 [stable-3_3_0] Remove jquery, jquery-ui and jquery-validatio…

db32b3b

…n to composer dependencies

pkp#9753 [stable-3_3_0] Undo updates to jquery, jquery-ui and jquery-…

5e1c3d5

…validation paths

pkp#9753 [stable-3_3_0] Add js/lib/jquery/plugins/validate/ path to g…

adf20a8

…itignore

pkp#9753 [stable-3_3_0] Add script to copy jquery and jqueryui when r…

6586b70

…unning composer install

pkp#9753 Add composer custom repositories for jquery-ui, jquery-validate

e056252

pkp#9753 Update composer script when copying jqueryui and jquery vali…

a7e43ff

…dation

pkp#9753 Update FileManager mkdir function to check if folder already…

b162b82

… exists

pkp#9753 Update ComposerScript to use FileManager to copy files and dir

09f3a5b

pkp#9753 Undo unintended version bump for plugin-api-version

d1d12c0

blesildaramirez force-pushed the i9753-3_3_0 branch from 07913a6 to d1d12c0 Compare August 14, 2024 17:33

jonasraoni reviewed Aug 14, 2024

View reviewed changes

blesildaramirez added 6 commits August 16, 2024 00:03

pkp#9753 Update returned value when directory already exists before c…

a3a0cb7

…reating a dir

pkp#9753 Resolve unexpected throw statement on ComposerScript file

70ed240

pkp#9753 Update path when requiring tools/bootstrap.inc.php

3624d79

pkp#9753 Update ComposerScript to use native functions to copy files

6270fa6

pkp#9753 Undo check of is_dir on mkdir function in FileManager

1e227f9

pkp#9753 Update message log if creating dir fails in ComposerScript

bf243e1

asmecher approved these changes Sep 5, 2024

View reviewed changes

asmecher merged commit b12d2ef into pkp:stable-3_3_0 Sep 5, 2024
1 check was pending

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#9753 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports #10167

pkp/pkp-lib#9753 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports #10167

blesildaramirez commented Jul 5, 2024 •

edited

Loading

CLAassistant commented Jul 5, 2024

jonasraoni Aug 13, 2024

blesildaramirez Aug 14, 2024

jonasraoni Aug 14, 2024

blesildaramirez Aug 14, 2024

blesildaramirez Aug 14, 2024

jonasraoni Aug 14, 2024

asmecher Aug 19, 2024

blesildaramirez Aug 21, 2024

jonasraoni Aug 13, 2024

jonasraoni Aug 13, 2024

jonasraoni Aug 13, 2024

jonasraoni Aug 13, 2024

jonasraoni Aug 13, 2024

jonasraoni Aug 14, 2024

asmecher Aug 23, 2024

		copy($jqueryUiDist . '/jquery-ui.js', $vendorComponents . '/jqueryui/jquery-ui.js');
		copy($jqueryUiDist . '/jquery-ui.min.js', $vendorComponents . '/jqueryui/jquery-ui.min.js');

pkp/pkp-lib#9753 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports #10167

pkp/pkp-lib#9753 [stable-3_3_0] Update jquery, jquery-ui and chart.js to address security vulnerability reports #10167

Conversation

blesildaramirez commented Jul 5, 2024 • edited Loading

CLAassistant commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blesildaramirez commented Jul 5, 2024 •

edited

Loading