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

Add bootstrap package resolution to version 3.4.0 #4757

Closed
wants to merge 1 commit into from

Conversation

manasvinibs
Copy link
Member

Description

Path to dependency file: /node_modules/leaflet-draw/docs/examples/basic.html

Path to vulnerable library: /node_modules/leaflet-draw/docs/examples/basic.html

Currently in main, one of the node modules leaflet-draw pulls version of bootstrap-3.3.7 through CDN link which has vulnerability. Adding package resolution to 3.4.0 to install safer version.

Issues Resolved

#4729
#4728
#4727
#4725
#4723
#4722

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #4757 (9492e2f) into main (454fdfb) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4757      +/-   ##
==========================================
- Coverage   66.14%   66.13%   -0.01%     
==========================================
  Files        3316     3316              
  Lines       63944    63944              
  Branches    10135    10135              
==========================================
- Hits        42294    42292       -2     
+ Misses      19248    19169      -79     
- Partials     2402     2483      +81     
Flag Coverage Δ
Linux_1 34.75% <ø> (ø)
Linux_2 55.10% <ø> (ø)
Linux_3 42.98% <ø> (ø)
Linux_4 35.05% <ø> (ø)
Windows_1 34.77% <ø> (ø)
Windows_2 55.07% <ø> (ø)
Windows_3 42.98% <ø> (-0.01%) ⬇️
Windows_4 35.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 20 files with indirect coverage changes

@ananzh
Copy link
Member

ananzh commented Aug 16, 2023

I think adding a resolution in package.json file won't affect the version of Bootstrap loaded via the CDN in the HTML file. Give a quick a look at this file node_modules/leaflet-draw/docs/examples/basic.html, it seems just an example HTML document for demonstrating the use of the Leaflet.draw library.

Since it's part of a static example and not a part of our actual production code, it should not pose a security risk to our application. My point is that this basic.html file is merely an example or documentation file inside the leaflet-draw package and we do not include or serve it in our application, then the Bootstrap 3.3.7 links in this file will have no impact on our application’s runtime behavior. Therefore, we should just resolve the issue with comments.

What do you think @AMoo-Miki ?

@zhongnansu zhongnansu self-assigned this Aug 21, 2023
@AMoo-Miki AMoo-Miki added cve Security vulnerabilities detected by Dependabot or Mend v2.10.0 backport 2.x labels Aug 22, 2023
@AMoo-Miki
Copy link
Collaborator

I agree with Anan. The problem was not with a dep's dep on bootstrap but rather a URL they use in their examples.

@AMoo-Miki
Copy link
Collaborator

Since leaflet-draw has been abandoned, I propose with switch to https://www.npmjs.com/package/leaflet-euclides.

@manasvinibs
Copy link
Member Author

Since leaflet-draw has been abandoned, I propose with switch to https://www.npmjs.com/package/leaflet-euclides.

I see your point in switching the package from leaftlet-draw to leaflet-euclides but leaflet-euclides package also contain links to Bootstrap 3.3.7 versions in their example docs. As switching the package is not actually resolving this CVE, should we close this PR and have a separate one to switch the package?

@zhongnansu zhongnansu removed their assignment Aug 28, 2023
@ashwin-pc
Copy link
Member

@AMoo-Miki @manasvinibs whats our next steps here?

@manasvinibs
Copy link
Member Author

@AMoo-Miki @manasvinibs whats our next steps here?

I think I can close this PR for now as we do not want to add a package json resolution to the vulnerable bootstrap package that gets loaded through CDN links. In future, if we plan to address security threat caused by URLs pointing to vulnerable packages, we will have a separate PR to address the issue more broadly.

@manasvinibs manasvinibs removed cve Security vulnerabilities detected by Dependabot or Mend backport 2.x labels Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants