TRUNK-6556 : Lower CVSS Score from 6.1#5796
TRUNK-6556 : Lower CVSS Score from 6.1#5796sudhanshu-raj wants to merge 7 commits intoopenmrs:masterfrom
Conversation
|
Hi @dkayiwa , is there anything wrong with nvdApiKey |
yes i also had same doubt mine is also failing! do you figured out something @sudhanshu-raj |
I guess something is wrong with nvdApiKey, may be it is missing or invalid. We can't do much here. |
|
Repository secrets are not passed to workflows triggered by pull requests from forks. This is a deliberate security measure to prevent untrusted code from exfiltrating sensitive information. |
|
@dkayiwa @sudhanshu-raj @RajPrakash681 Are you guys also facing the issue of ObsValidatorTest Failure in [Build with Maven / build (ubuntu-latest, 21, true) because of groupmember. I identified the most likely fix as to change the groupMember to groupMembers in lines 427 and 445 of the file api/src/main/java/org/openmrs/validator/ObsValidator.java. Not sure though. |
|
I have seen that on many other pull requests. Are you able to reproduce it locally? |
|
I can see the @wikumChamith created a ticket for the same exact problem. https://openmrs.atlassian.net/browse/TRUNK-6559 |
|
@dkayiwa Nope it happens only on github actions/github ci. Can't produce it locally. |
|
Hi @dkayiwa , meanwhile can we review the changes, any thoughts ? |
|
What results to you get when you run the dependency check tool? |
|
If i check the workflow for dependency check failure, it directly showing - Missing argument for option: nvdApiKey |
sudhanshu-raj
force-pushed
the
TRUNK-6556
branch
from
4ee2f73 to
ad896a9
Compare
|
I debug the dependency check workflow to verify whether this path |
|
Okay so i removed the nvd api key part and it's passing owasp dependency check, probably working without nvd key which clearly depicts there is something wrong with the key only. Isn't @dkayiwa ? |
yes this is the only reason most probably now |
|
Is your JIRA ticket link correct? |
Thanks for pointing out that, fixed it. |
|
What output do you get when you run the dependency check tool mvn org.owasp:dependency-check-maven:aggregate |
|
So I got the report generated , and on that report found no vulnerabilities , actual result : dependency-check version: 12.2.0 |
| --failOnCVSS 6.2 | ||
| --enableRetired | ||
| --suppression dependency-check-suppressions.xml | ||
| --nvdApiKey ${{ secrets.NVD_API_KEY }} |
There was a problem hiding this comment.
No , actually I did this for testing, reverting it back
sudhanshu-raj
force-pushed
the
TRUNK-6556
branch
from
8c98393 to
c2f75ba
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5796 +/- ##
============================================
- Coverage 57.88% 57.87% -0.02%
+ Complexity 9137 9134 -3
============================================
Files 685 685
Lines 37143 37143
Branches 5432 5432
============================================
- Hits 21500 21495 -5
- Misses 13731 13735 +4
- Partials 1912 1913 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Did you test these changes in the user interface to confirm that all is well? |
|
Yes I have tested the interface specially the setup page, it's working as expected |
|
Can you share some screenshots? |
|
Sure, I also though of adding the screen recording. of user interface. Here it is at 2x : OpenMRS_InStp.mov |
|
Compile the latest snapshot version of the legacyui module and try to run it on the latest snapshot version of openmrs-core with your changes. Take a look at the admin screen of the legacyui module and click all the links to confirm that all is well. |
sudhanshu-raj
force-pushed
the
TRUNK-6556
branch
from
9c9267a to
c98606e
Compare
|
I tested the changes with Legacy UI 2.1.0-SNAPSHOT and Core 2.8.4-SNAPSHOT. Almost all functionality is working as expected. I made a small CSS adjustment due to the newer jQuery version. There are no UI changes, except for a minor update to the Add - Upgrade Module popup close button. The newer jQuery replaced anchor tag with button, so the CSS has adjusted accordingly. I’ve attached a short video for this. Uploading OpenMRS_LegacyUI_Module.mov… |
|
I expect the changes to be on core 3.0.0-SNAPSHOT |
|
Can we run the legacy model with core 3.0.0 snapshot ? |
|
Yes, the latest snapshot version. |
|
I mean core 3.0.0 snapshot requires tomcat 10 I guess, and legacyUI module not works with tomact 10 , as i tried pairing both on tomcat 10 but got error which saying it need a class not sure but that's got removed from core 3.0.0 and it's there on previous version. This is my assumption, from what I tried . |
|
And I see after the commit of d9f6827 , openmrs platform welcome page is not opening as I checked , I found dispatcher servlet didn't find the mapping for index.htm and thus getting 404 error page now and even after fixing this 404 page , still I getting errors for creating beans as it not found the class it referencing too. |
|
Which openmrs core version are you running? |
|
3.0.0-SNAPSHOT |
|
Oh, sorry I was testing the old legacyui module , as I see the changes on this let me build on new changes and then will test |
|
On the Admin Create Patient page, there is some unexpected behavior. The birthdate field is not working correctly. When moving to the second step of the Create Patient flow, the birthdate entered on the first step does not appear. Even when the birthdate is entered again on the second step, it is not converted to a date and a type mismatch error is shown, preventing the patient from being saved. No jQuery changes were implemented here. This issue is most likely related to the shift to OpenMRS 3.x and the Spring version upgrade hence I guess it need some changes on legacyui module. Let me know if this seems to be problem for me only and I using 3.x snapshot for both legacyui and core module in this. patient_pg_is.mp4 |
|
After digging into root cause, found global custom editors not getting loaded on platform 3.x . Created the bug on TRUNK-6579 |
|
There is another bug found , on the admin page , create the bug on LUI-208. And this is that screenshot : |
|
Did we intentionally removed the Default Theme option from Locales And Themes on 3.x ? |
|
|
Updated the legacyUI module jQuery code to compatible with this latest version . Raised the pr :PR |
|
@sudhanshu-raj did you test the legacyui and confirm that all is well? |
Yes after all these changes, all looks good |
|
Do you wanna share some screenshots? |
|
I quickly got some screenshots , for some pages although I guess screen recording will be more efficient , isn't ? Anyway here is the google drive link for those screenshots DriveLink |
|
And another thing do we need to update the legacyui module after the junit migration from 4 > 5 done in core , as i can't run tests |
|
Hi @dkayiwa , just a quick reminder. Please share your thoughts when you get a chance. |
5 participants
Description of what I changed
During the OWASP Dependency Check, we found two vulnerable dependencies i.e, jQuery 1.7.1 & jQuery UI 1.8.2, that's why our CVSS score was medium which is around 6.1. So this aim is to reduce the CVSS score hence reduce vulnerabilities found.
Earlier, we were using jQuery very old versions like jQuery 1.7.1 and jQuery UI 1.8.2 from around 2011, which has few vulnerabilities(around 7) found via the OWASP Dependency Check and this is the only reason for CVSS score 6.1, fixing this would solve the problem.
So, I updated both the jQuery dependency to latest version which is jQuery 1.7.1 > 3.7.1 and jQuery UI 1.8.2 > 1.13.3,
which has no any vulnerabilities and CVSS score reduced to 0 as there is no any vulnerability found. I added the latest jquery.min.js and jquery-ui.custom.min.js along with jquery-migrate.min.js which provides compatibility and ensures the smooth running of deprecated jQuery methods even after shifting to newer version. I did some changes like replaced the deprecated method with new one manually but there could be more such place where we need to replace with newer method that's why jquery-migrate will be helpful here and did few changes for progress bar updates compatible for newer version.
After this I tested the initial setup page for core, and this seems to be working as expected.
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6556
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master