[cve] Migrate jquery.cookie to js.cookie for CVE-2022-23395 #4054
Conversation
|
|
Backend Code Coverage Report •
Pytest Report
|
Harshg999
requested review from
tabraiz12,
JohanAhlen,
bjornalm and
ramprasadagarwal
jsj1001
changed the title
| @@ -168,10 +168,10 @@ $(document).ready(function () { | |||
| }); | |||
| }); | |||
|
|
|||
| $("#updateSkipWizard").prop('checked', $.cookie("hueLandingPage", { path: "/" }) == "home"); | |||
| $("#updateSkipWizard").prop('checked', Cookies.set("hueLandingPage", { path: "/" }) == "home"); | |||
There was a problem hiding this comment.
This looks incorrect, but so was the old code even before the inline js migration. Lets leave it for this PR.
jsj1001
force-pushed
the
jquery-cookie-migration-to-js-cookie
branch
from
6e3b0ae to
468d0d9
Compare
|
Fixed failed js lint |
There was a problem hiding this comment.
This PR installs the js-cookie using the package.json file but also includes it as a static source and load that one as a global variable in hue.mako. I assume the static resource was added since the mako files js are not under webpack control. But I don't think we should be adding two different dependencies for the same library.
Can you try to drop the static import and only use the webpack version if you also import that in hue.js and add it to the global window object? You can look at how we handle legacy usage of lodash (_) there. Then in the mako file you can use window.Cookies and in the js files you can import as normal to avoid the linter errors.
| @@ -72,7 +72,8 @@ class DownloadResultModal { | |||
|
|
|||
| params.executable.toContext(id).then(jsonContext => { | |||
| this.cookieId = 'download-' + id; | |||
| $.cookie(this.cookieId, null, { expires: -1, path: '/' }); | |||
| // eslint-disable-next-line no-undef | |||
| Cookies.set(this.cookieId, null, { expires: -1, path: '/' }); | |||
There was a problem hiding this comment.
Missing import statement (import Cookies from 'js-cookie'). Once the import is in place you can remove the eslint-disable lines
| @@ -0,0 +1,34 @@ | |||
| import $ from 'jquery'; | |||
| import 'jquery.cookie'; | |||
There was a problem hiding this comment.
Isn't the goal to remove jquery.cookie?
| /*! | ||
| * Javascript Cookie v1.5.1 | ||
| * https://github.com/js-cookie/js-cookie | ||
| * | ||
| * Copyright 2006, 2014 Klaus Hartl | ||
| * Released under the MIT license | ||
| */ | ||
| (function (factory) { |
There was a problem hiding this comment.
There's no need for the static file,
If Cookies needs to be in the global js scope you can import it and set it on the window object in hue.js.
| @@ -98,6 +98,7 @@ | |||
|
|
|||
| % if not conf.DEV.get(): | |||
| <script src="${ static('desktop/js/hue.errorcatcher.js') }"></script> | |||
| <script src="${ static('desktop/js/js.cookie.js') }"></script> | |||
package.json
Outdated
| @@ -162,7 +161,9 @@ | |||
| "webpack": "5.94.0", | |||
| "webpack-bundle-analyzer": "4.10.2", | |||
| "webpack-bundle-tracker": "3.1.0", | |||
| "webpack-cli": "5.1.4" | |||
| "webpack-cli": "5.1.4", | |||
| "jquery.cookie": "1.4.1", | |||
package.json
Outdated
| "webpack-cli": "5.1.4" | ||
| "webpack-cli": "5.1.4", | ||
| "jquery.cookie": "1.4.1", | ||
| "js-cookie": "1.5.1" |
There was a problem hiding this comment.
Why not use the latest? (3.0.5)
jsj1001
force-pushed
the
jquery-cookie-migration-to-js-cookie
branch
from
468d0d9 to
76edd33
Compare
|
|
|
@bjornalm @JohanAhlen Thank you for reveiw:) |
What changes were proposed in this pull request?
How was this patch tested?
Please Remove #4053 PR as it duplicates this PR.
Please review Hue Contributing Guide before opening a pull request.