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

Bump "rollup-plugin-terser" dependency to v7.0.0 to fix "serialize-javascript" high security vulnerability #2601

Closed
fvpDev opened this issue Aug 12, 2020 · 13 comments · Fixed by #2604

Comments

@fvpDev
Copy link

fvpDev commented Aug 12, 2020

Library Affected:
workbox-build

Browser & Platform:
All projects using workbox-build

Issue or Feature Request Description:
Bump "rollup-plugin-terser" dependency to v7.0.0 to fix "serialize-javascript" high security vulnerability

@fvpDev fvpDev changed the title Bump "rollup-plugin-terser" dependency to v7.0.0 to fix serialize-javascript high security vulnerability Bump "rollup-plugin-terser" dependency to v7.0.0 to fix "serialize-javascript" high security vulnerability Aug 12, 2020
@slorber
Copy link

slorber commented Aug 14, 2020

Hey, we also need this to fix Docusaurus:
facebook/docusaurus#3268

Vulnerability: https://www.npmjs.com/advisories/1548

If that helps: here's the 7.0.0 release link: https://github.com/TrySound/rollup-plugin-terser/releases/tag/v7.0.0

@jeffposnick
Copy link
Contributor

I'll push out a new v6 alpha release of Workbox that includes this bumped dependency.

Similar to #2319, we're stuck using older versions of rollup-plugin-terser in the Workbox v5 branch, as we need to maintain Node v8.x compatibility. Recent versions of rollup-plugin-terser explicitly require Node v10.x or higher.

@slorber
Copy link

slorber commented Aug 14, 2020

Thanks.

That's unfortunate, isn't rollup publishing a backport with the security fix?

If we need to adopt workbox alpha to solve this security issue, how stable is it currently?

@jeffposnick
Copy link
Contributor

These dependency updates are now out as part of https://github.com/GoogleChrome/workbox/releases/tag/v6.0.0-alpha.2

I don't see anything about a backport for the security update to older versions of rollup-plugin-terser.

Our alpha releases pass our full test suite, and there are relatively minor breaking changes between v5 and v6 in the current alphas.

That being said, @philipwalton has some plans to change the PrecacheController interface as part of an additional set of breaking changes that we're going to introduce before going from alpha to beta. Not very many folks use PrecacheController directly (as opposed to using a wrapper for it, like precacheAndRoute()), so that probably wouldn't end up affecting you, but it is the one major change left that we're likely to get in before considering the v6 interface stable.

I would selfishly like more folks to try out our alpha releases, just so that we get good usage prior to the final release, but I obviously don't want to pressure folks into doing that in order to get that security fix. If your project that uses Workbox v5 doesn't need to support Node v8.x, then you could consider using npm-shrinkwrap (or the yarn equivalent...?) to force workbox-build to use rollup-plugin-terser v7. I don't think there are any breaking changes in that plugin other than the lack of Node v8.x support, so theoretically that would work.

@slorber
Copy link

slorber commented Aug 17, 2020

Thanks,

We use a bit the PrecacheController directly, do you think it's likely to have breaking change?

  const precacheManifest = self.__WB_MANIFEST;
  const controller = new PrecacheController();

  controller.addToCacheList(precacheManifest);

  self.addEventListener('install', (event) => {
    event.waitUntil(controller.install());
  });

  self.addEventListener('activate', (event) => {
    event.waitUntil(controller.activate());
  });

  self.addEventListener('fetch', async (event) => {
    if (params.offlineMode) {
      const requestURL = event.request.url;
      const possibleURLs = getPossibleURLs(requestURL);
      for (let i = 0; i < possibleURLs.length; i += 1) {
        const possibleURL = possibleURLs[i];
        const cacheKey = controller.getCacheKeyForURL(possibleURL);
        if (cacheKey) {
          if (params.debug) {
            console.log('[Docusaurus-PWA][SW]: serving cached asset', {
              requestURL,
              possibleURL,
              possibleURLs,
              cacheKey,
            });
          }
          event.respondWith(caches.match(cacheKey));
          break;
        }
      }
    }
  });

@philipwalton
Copy link
Member

philipwalton commented Aug 17, 2020

@slorber In v6 we will likely require events to be passed to all strategies/handlers, and since PrecacheController will now use a strategy under the hood to do the precaching, controller.install() will likely need to become controller.install({event}).

That being said, given your usage above, I think you could simply your code since the precache() method will call addToCacheList(...) and also add the install/activate listeners for you. I.e. your code could become this:

const precacheManifest = self.__WB_MANIFEST;
const controller = new PrecacheController();

controller.precache(precacheManifest);

self.addEventListener('fetch', async (event) => {
  if (params.offlineMode) {
    const requestURL = event.request.url;
    const possibleURLs = getPossibleURLs(requestURL);
    for (let i = 0; i < possibleURLs.length; i += 1) {
      const possibleURL = possibleURLs[i];
      const cacheKey = controller.getCacheKeyForURL(possibleURL);
      if (cacheKey) {
        if (params.debug) {
          console.log('[Docusaurus-PWA][SW]: serving cached asset', {
            requestURL,
            possibleURL,
            possibleURLs,
            cacheKey,
          });
        }
        event.respondWith(caches.match(cacheKey));
        break;
      }
    }
  }
});

@jeffposnick
Copy link
Contributor

(Re-opening this issue for visibility.)

@slorber
Copy link

slorber commented Aug 21, 2020

Thanks, will try to upgrade to v6 but don't have time right now 🤪

@jeffposnick
Copy link
Contributor

To repeat something buried at the end of one of the other comments: for folks who don't need to maintain Node v8.x compatibility and would prefer not to try out the alpha releases of Workbox v6, it should be possible to use npm-shrinkwrap to continue using Workbox v5.x but force an upgrade to rollup-plugin-terser v7.x.

@raphaelbs
Copy link

Hey @jeffposnick,
rollup-plugin-terser released version 5.1.2 fixing this.

Is it possible to make a new patch release on v5 to bump this package? I made it on this branch and it seems ok:

$ npm ls serialize-javascript                                      
workbox@0.0.0 /home/raphaelbrandao/workbox
├─┬ copy-webpack-plugin@5.1.2
│ └── serialize-javascript@4.0.0 
├─┬ rollup-plugin-terser@5.3.1
│ └── serialize-javascript@4.0.0  deduped
└─┬ webpack@4.44.1
  └─┬ terser-webpack-plugin@1.4.5
    └── serialize-javascript@4.0.0  deduped

@jeffposnick
Copy link
Contributor

Ah, cool.

Yes, I should be able to get a Workbox v5.1.4 out this week that includes (just) that updated rollup-plugin-terser dependency.

@jeffposnick
Copy link
Contributor

Workbox v5.1.4 was just released, and should include the updated rollup-plugin-terser dependency, while maintaining Node v8.x compatibility. Please let me know if you run into any issues.

@slorber
Copy link

slorber commented Sep 11, 2020

thank you @jeffposnick :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants