- 🇮🇹Italy itamair
Thanks a lot @here!
I merged the MR !16 and I am going to deploy a new Geofield Map 3.0.16 release with this, that exactly match the corresponding fix already happened on the the Drupal Leaflet module, some weeks ago ( https://www.drupal.org/project/leaflet/issues/3426106 📌 Polyfill.io is no longer considered safe and should be removed Fixed ).Feel free to open a follow-up Feature Request if we would really better embed the whole polyfill.js library (https://polyfill-fastly.io/v3/polyfill.min.js?features=IntersectionObserver) in the Geofield Map module itself, and make everything more safe & solid.
-
itamair →
committed 1fa66fbd on 3.0.x authored by
aaron.ferris →
Issue #3456704 by aaron.ferris, tinto, greggles, mcdruid, HeikkiY, seanB...
-
itamair →
committed 1fa66fbd on 3.0.x authored by
aaron.ferris →
- First commit to issue fork.
- 🇳🇱Netherlands seanB Netherlands
+1 for moving the removal of the library to a followup!
- 🇩🇰Denmark ressa Copenhagen
I agree, it's a balancing act -- but the wording is very much in favor of using third-parties:
Having third party libraries that are hosted elsewhere on the Internet in the Drupal.org repo is strongly discouraged.
This wording could be relaxed a bit, to something like this:
Third party libraries hosted elsewhere on the Internet can be included in the Drupal.org repo, though it's not recommended.
I also agree with this:
Putting a static copy of the library in the module mitigates supply chain risk, but on the other hand if/when a vulnerability is found and fixed in the (upstream) library... we then need to find all copies of it everywhere and update them (or backport the fix).
It might not be an option, due to licensing, or some other structural blocker I am not aware of -- but the Drupal community could make a handful of Composer packages for the most popular JavaScript libraries, so that a module maintainer could require
drupal/javascript-polyfill
and always get the latest, safest version. NPM is an option, though I do prefer Composer if it's possible, since that's already used by everyone and well known. I am sure this has been discussed at length in other issues :-) - 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Yeah it's a balance, and I don't think there's a clear "right answer" at present.
Putting a static copy of the library in the module mitigates supply chain risk, but on the other hand if/when a vulnerability is found and fixed in the (upstream) library... we then need to find all copies of it everywhere and update them (or backport the fix).
Which problem is worse?
Referencing dependencies with tools like composer (and perhaps npm in the future?) has the advantage that it makes automated auditing easier (dependabot etc..) and potentially sites can update themselves without relying on Drupal projects doing new releases (that's subject to appropriate constraints being in place, and everyone's interpretation of semver agreeing etc.. etc..)
- 🇩🇰Denmark ressa Copenhagen
Thanks Drupal security team for looking into this, and @itamair for removing this third-party library. For now polyfill-fastly.io looks safe, but what happens if Fastly is bought up and closed, goes bankrupt, or something else ... and the domain is squatted in two years time, just like polyfill.io was?
Currently on the "Policy on 3rd party assets on Drupal.org > Third party libraries" page it says:
Having third party libraries that are hosted elsewhere on the Internet in the Drupal.org repo is strongly discouraged. Instead, if the library in question is available under GPLv2+, or has a “GPL-friendly” license (see note below), you may use one of the methods described on the community documentation page titled “ Including 3rd party libraries → ”.
From https://www.drupal.org/node/422996#libraries →
As I see it, it could make it simpler for module maintainers like @itamair, if it wasn't strongly discouraged to include a small library such as the Polyfill library (200 kB) in the Leaflet module ... This would reduce the risk, with little downside.
- 🇺🇸United States Kristi Wachter
THANK YOU, No Sssweat - that is extremely helpful information!
- 🇸🇰Slovakia poker10
@solideogloria according to this: https://www.drupal.org/docs/develop/git/git-for-drupal-project-maintaine... → , each "Security update" release type should be linked to the correspoding SA. There are no SAs for these polyfill.io issues, see: https://www.drupal.org/psa-2024-06-26 → . Therefore I think that we should not use a Security update release type here.
Users are more likely to update the module if the release says "Security". As far as I know, security releases can also include issues fixed in the public issue queue, or issues related to 3rd party libraries, such as for this PSA
To me, it looks like its up to the maintainers. https://www.drupal.org/drupal-security-team/security-release-numbers-and... →
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
I thought that was only for releases which were released in coordination with the security team. This issue has been public for ages.
- 🇨🇦Canada No Sssweat
@lymannx, no. This one is different. You can read about it here
Other known modules affected:
- webform
- leaflet
- sitewide_alert - 🇺🇸United States Luke.Leber Pennsylvania
I think we might need a follow-up to update the Choices library after https://github.com/Choices-js/Choices/pull/1162 lands upstream (Thanks
Jürgen!)Presently, the choices.js library, as pulled in via `composer.libraries.json` will continue to place `./[web-root]/libraries/choices/public/index.html` on disk which loads scripts from polyfill.io. Given sophisticated enough malicious javascript, this file could potentially be used by threat actors to launch phishing attacks.
- 🇬🇧United Kingdom aaron.ferris
Agreed with the above, fix the pressing issue at hand then a task in the backlog to remove said library.
- 🇳🇱Netherlands tinto Amsterdam
I agree with #14. There are valid points in #13, but considering this is a security issue, I think for now it's best to limit the scope (and time needed) and simply replace the malicious/untrustworthy url.
Removing the polyfill library altogether warrants a separate issue IMO.
- 🇮🇳India abhiyanshu_rawat
Hi @greggles,
I installed the theme locally and didn't find any comments related to polyfill.io.
However,polyfill-io
is present only in the.js.map
file, and the changes related to it are not in the base file of the map file.
Please refer to the attached screenshot for more clarity.I believe we can ignore it or If you have any suggestions, please let us know so that we can proceed accordingly. Thanks.
-
Techbot →
committed 3d4daeae on master authored by
abhiyanshu_rawat →
Issue #3456706: polyfill.io library is no longer considered safe to use
-
Techbot →
committed 3d4daeae on master authored by
abhiyanshu_rawat →
- First commit to issue fork.
- First commit to issue fork.
- 🇺🇸United States greggles Denver, Colorado, USA
I like the idea of committing this as-is, making a release, and moving to a removal as a followup task that can get more testing and upgrade notes before it gets released.
- 🇨🇦Canada NickDickinsonWilde Victoria, BC (T'So-uke lands)
I'm marking this as RTBC. It is the most simple fix that resolves the current security issue with no other impacts.
That said, I think a better fix would be to totally remove the external library for a few reasons:
- External code is bad(tm)
- But yes, seriously, code from any third party that is only accessed at runtime is unpredictable, reduces testability and security
- The only use that Geofield Map is using polyfill for is IntersectionObserver. the availability of that is over 96% - https://caniuse.com/?search=IntersectionObserver
So yes, fastly is as far as trusted sources, pretty high. But, will they long term keep that domain? if they don't, will it be just a dead link? or will another hostile actor take control of it? With pretty much negligible benefit I think it would be better to remove but that could be a breaking change so I could see that as a follow-up depending on maintainer decisions.
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
@#23 I think there are only a few mentions of IE in the codebase. They can probably be removed in a single issue.
- 🇨🇦Canada No Sssweat
Providing some guidance on how this library gets loaded since no one has yet.
Check if in /admin/structure/webform/config/libraries, Choices is enabled.
However, this setting alone won't cause the library to get loaded.
In order for it to load, a select element/field needs to have Choices checked in it's config.
The fix is either update the module to the latest version OR make sure Choices is unchecked in /admin/structure/webform/config/libraries
- 🇺🇸United States Luke.Leber Pennsylvania
Hey Liam,
In the coming weeks I'll be doing a screen cast about our experience in theming out a somewhat complete feature set of Webform functionality.
There were a few other probable IE-isms that popped out at us (some datetime stuff, specifically). I wonder if a meta might be in order to group that effort?
- 🇺🇸United States greggles Denver, Colorado, USA
There's a proposed change as an MR here so status should be Needs Review.
- 🇺🇸United States greggles Denver, Colorado, USA
Generally looks good to me. Thanks.
- 🇺🇸United States greggles Denver, Colorado, USA
Adjusting status as the MR needs review.
It generally looks good to me. Thanks.
- 🇺🇸United States greggles Denver, Colorado, USA
That change looks good to me. Thanks.
- 🇺🇸United States greggles Denver, Colorado, USA
Thanks for the quick fix. I agree this instance isn't as critical given it was in the documentation.
FWIW, I noticed that 7.x-1.x doesn't have the fix in it just yet. Maybe that branch hasn't been merged or hasn't been pushed?
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
I would welcome a follow-up issue to remove this library entirely since IE 11 is no longer supported.
- 🇳🇱Netherlands tinto Amsterdam
Reuploading the patch, in case anyone needs it. It seems #3 was deleted by mistake. :)
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Apologies - I didn't mean to hide the patch in #3 - I was just adding some tags to try to link related issues together.
- 🇮🇳India abhiyanshu_rawat
Since it's only one change, but to avoid accidentally using the wrong CDN for the files, I have updated the CDN library to use polyfill-fastly.io. Thanks.
- 🇬🇧United Kingdom aaron.ferris
Sorry didn't see the patch on this, raised an MR anyway.
- @aaronferris opened merge request.
- @abhiyanshu_rawat opened merge request.
- First commit to issue fork.
- 🇬🇧United Kingdom aaron.ferris
aaron.ferris → made their first commit to this issue’s fork.
- 🇮🇳India abhiyanshu_rawat
I have updated all the comments with polyfill-fastly.io to avoid accidentally using the wrong CDN for the files. Thanks.
- 🇮🇳India abhiyanshu_rawat
I have updated all the comments with polyfill-fastly.io to avoid accidentally using the wrong CDN for the files. Thanks.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Sorry, I was just adding some tags and didn't mean to change the status.
- @abhiyanshu_rawat opened merge request.
- 🇮🇳India abhiyanshu_rawat
abhiyanshu_rawat → changed the visibility of the branch 3456710-polyfill.io-library-is to hidden.
- First commit to issue fork.
- @abhiyanshu_rawat opened merge request.
- 🇺🇸United States Trigve Hagen Lodi CA
Not sure whats the problem? I got rid of all references.
- First commit to issue fork.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Thank you @torotil - it's only a Security issue in that the trustworthiness of the original 3rd party service has been questioned, so it's probably best not to provide that specifically as an example any more. Appreciate your swift action.
- 🇳🇱Netherlands tinto Amsterdam
The webform module has the exact same issue 🐛 polyfill.io Library is no longer considered safe to use Fixed . They're solving it by replacing the suspicious url.
Here's a patch.
- 🇦🇹Austria torotil
While it’s technically true that the README mentioned polyfill.io as as an example, it’s a bit strange for me to consider that a security issue.
However I’ve removed it from the README and tagged a new 7.x-1.3 (bugfix) release for this. I also removed the same sentence in the project description.
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
I was about to make an RC, but tests are not passing.
- 🇺🇸United States greggles Denver, Colorado, USA
This was originally "minor" since polyfill is just mentioned in comments, but it would be good to update/remove that even in a comment to avoid accidentally using the wrong cdn for the files.
- 🇺🇸United States greggles Denver, Colorado, USA
This was originally "minor" since polyfill is just mentioned in comments, but it would be good to update/remove that even in a comment to avoid accidentally using the wrong cdn for the files.
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
Comment #3 links to https://security.drupal.org/node/180276 which was a private issue (that will remain private) where there was discussion and it was agreed this could be handled in public.
- 🇺🇸United States jrockowitz Brooklyn, NY
You can make an RC, but I need to tag a stable one this week. We could skip the RC
- 🇺🇸United States jrockowitz Brooklyn, NY
Yep, this can happen in the next day or so. I will review open tickets to see if anything else should go into the release
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
I think this priority and issue tag makes sense.
Since it's about a 3rd party library this can be fixed in public without a security advisory, but should ideally be addressed quickly with a code change and new release(s).
- 🇺🇸United States greggles Denver, Colorado, USA
Thanks for fixing and creating new releases!
- 🇬🇧United Kingdom kewesley
This would be helpful. We have seen some issues reported recently that look like they can be traced back to the polyfill.io service. While the patch/MR works for us, a new release would make it easier to update.
- 🇺🇸United States greggles Denver, Colorado, USA
Thanks for the work here.
I filed 📌 Create new stable release for 6.2.3 to close security issue Active about getting this into a stable release.
- Issue created by @greggles
- 🇺🇸United States mherchel Gainesville, FL, US
Also fixed and released a version for D7 https://www.drupal.org/project/quicklink/releases/7.x-1.1 →
- 🇺🇸United States mherchel Gainesville, FL, US
Added to the new 2.0.3 release. Thank you @HeikkiY!
-
mherchel →
committed 1fa9b1dd on 2.0.x authored by
HeikkiY →
Issue #3456671 by HeikkiY, mherchel: polyfill.io Library is no longer...
-
mherchel →
committed 1fa9b1dd on 2.0.x authored by
HeikkiY →
- last update
6 days ago 3 pass - last update
6 days ago 3 pass - 🇫🇮Finland HeikkiY Oulu
Excellent. It would be good to act fast because it seems like it is already used to distribute malicious code based on https://github.com/polyfillpolyfill/polyfill-service/issues/2873#issueco....
- 🇺🇸United States mherchel Gainesville, FL, US
Thanks for the heads up. I'll remove this. We also probably need to fix some tests for thsi.
- Issue created by @HeikkiY
- Issue created by @HeikkiY
- Issue created by @HeikkiY
- Issue created by @HeikkiY
- Issue created by @HeikkiY
- Issue created by @HeikkiY
- last update
6 days ago Build Successful - @heikkiy opened merge request.
- Issue created by @HeikkiY