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

Use of eval in "node_modules/pdfjs-dist/build/pdf.js" is strongly discouraged as it poses security risks and may cause issues with minification #16061

Closed
gkiely opened this issue Feb 15, 2023 · 6 comments

Comments

@gkiely
Copy link

gkiely commented Feb 15, 2023

Configuration:

Repo:

Steps to reproduce the problem (with repo):

  • git clone git@github.com:gkiely/pdfjs-issue.git
  • cd pdfjs-issue
  • npm i
  • npm run build

Steps to reproduce the problem (without repo):

  1. npm create vite@latest
  2. Project name: pdfjs-issue
  3. Vanilla
  4. JavaScript
  5. cd pdfjs-issue
  6. npm i pdfjs-dist
  7. Add the following to main.js
    import pdfjs from 'pdfjs-dist';
  8. npm run build

What is the expected behavior? (add screenshot)
image

@Snuffleupagus
Copy link
Collaborator

Please note that "Vite" isn't something we use here, and thus we unfortunately can't really provide help/support for it.

Furthermore, note that the code in question is intended specifically for Node.js environments; please see the following comment for additional context here:

pdf.js/src/display/api.js

Lines 2296 to 2317 in 546902d

if (
PDFJSDev.test("GENERIC") &&
isNodeJS &&
// eslint-disable-next-line no-undef
typeof __non_webpack_require__ === "function"
) {
// Since bundlers, such as Webpack, cannot be told to leave `require`
// statements alone we are thus forced to jump through hoops in order
// to prevent `Critical dependency: ...` warnings in third-party
// deployments of the built `pdf.js`/`pdf.worker.js` files; see
// https://github.com/webpack/webpack/issues/8826
//
// The following hack is based on the assumption that code running in
// Node.js won't ever be affected by e.g. Content Security Policies that
// prevent the use of `eval`. If that ever occurs, we should revert this
// to a normal `__non_webpack_require__` statement and simply document
// the Webpack warnings instead (telling users to ignore them).
//
// eslint-disable-next-line no-eval
const worker = eval("require")(this.workerSrc);
return worker.WorkerMessageHandler;
}

@Snuffleupagus Snuffleupagus closed this as not planned Won't fix, can't repro, duplicate, stale Feb 15, 2023
@gkiely
Copy link
Author

gkiely commented Feb 15, 2023

That's ok thanks for providing me a link to the source. I'll check with the vite team to see if there's a way to remove this error.

@FredrikWallstrom
Copy link

Hey. We have the same issue with the Rollup bundler. Rollup complains about the use of eval when bundling: https://rollupjs.org/troubleshooting/#avoiding-eval

Is there something that can be done to remove eval in pdfjs?
Or Is there a way to for me to get around this? We are using stenciljs in our project which in turn uses Rollup as bundler under the hood.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 3, 2023

Is there something that can be done to remove eval in pdfjs?

Maybe, but that could very easily end up breaking more things than it fixes unfortunately.

If you look at the code/comment linked in #16061 (comment) you'll see that this was done for improved Webpack compatibility (for a while there were loads of issues filed about that). Hence trying to fix this issue could thus risk:

  • Re-introducing the Webpack warning yet again (and having to deal with a bunch of issues/complaints).
  • Potentially breaking general Node.js support, since this code is specifically intended for that environment (however bundlers won't leave it alone despite the run-time checks).
  • Removing the eval unfortunately doesn't guarantee that Rollup starts working correctly, since it's certainly possible that it uncovers other problems (given that we don't use Rollup it's not feasible for us to "officially" support it).
  • Any patch(es) might need be reverted, if there are problems with the first two points above.

Given that this doesn't affect the built-in Firefox PDF Viewer, I cannot really justify spending my spare time trying to fix this (since I'm not getting paid to hack on the PDF.js library).
However, if you'd be willing to compensate me for my time, I could certainly attempt a patch that removes the eval (with all the caveats above).

@FredrikWallstrom
Copy link

Ok, I understand. Thank you for taking your time and for the work you are putting into this, it's appreciated.

@wojtekmaj
Copy link
Contributor

It looks like this could be a way to suppress these warnings:

https://rollupjs.org/troubleshooting/#eval2-eval

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

No branches or pull requests

4 participants