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

CSP violations for unsafe-inline in pdfjst-dist@2.2.228 #11036

Closed
loremaps opened this issue Aug 2, 2019 · 25 comments
Closed

CSP violations for unsafe-inline in pdfjst-dist@2.2.228 #11036

loremaps opened this issue Aug 2, 2019 · 25 comments
Labels

Comments

@loremaps
Copy link

loremaps commented Aug 2, 2019

Attach (recommended) or Link to PDF file here:

Configuration:

  • Chrome Version 76.0.3809.87 (Official Build) (64-bit)
  • Ubuntu 18.04.2 LTS (Bionic Beaver)
  • PDF.js version: pdfjs-dist v2.2.228
  • Is a browser extension: No

Steps to reproduce the problem:
We have a content security policy that prevents unsafe-inline.
The policy is violated by this line in v2.2.228 Function("r", "regeneratorRuntime = r")(runtime);

Additional info:
Similar issue #10229

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Aug 2, 2019

The policy is violated by this line in v2.2.228 Function("r", "regeneratorRuntime = r")(runtime);

That's not actually code originating anywhere within the PDF.js library itself, but it rather comes from a Babel polyfill needed to support async/await in older browser. As such, it's unfortunately not at all clear what (if anything) could be done about it here.

@timvandermeij
Copy link
Contributor

Yes, I think this should be reported upstream instead.

@lennybacon
Copy link

@timvandermeij Code in pdf.js could be written so that the polyfill is not needed.
@Snuffleupagus Any hints at whta file to look at?

@lennybacon
Copy link

Looked deeper into it. No chance :-(

@lennybacon
Copy link

Here is the issue regenerator-runtime/runtime.js.

@jkroepke
Copy link

jkroepke commented Sep 4, 2019

Same issue here.

@Snuffleupagus it's possible to distribute 2 separate files one for the older browser a 2nd for the evergreen browsers?

@maxime1992
Copy link

Is there any issue upstream to track? Facing the same issue currently

@timvandermeij
Copy link
Contributor

You can report the issue to https://github.com/facebook/regenerator. It looks like they fixed some CSP violations there before, so perhaps they are willing to fix this one too. If they fix it upstream, we can update the version we use to fix it on our side too.

@jkroepke
Copy link

jkroepke commented Oct 4, 2019

It looks like the authors of pdf.js fixed this issue here

facebook/regenerator#353

but due issues on babel they had to rollback it

facebook/regenerator#369

It looks like we have a deadlock here. I see three solutions here:

  • Wait until facebook/regenerator fixes the issues.
    It might be the clearest but a lot of users will stick to an old version of pdf.js
  • Downgrade facebook/regenerator (maybe babel has to be downgraded, too)
  • Provide in addition to the current ES5 an evergreen (ES2016+) version of pdf.js which doesn't need facebook/regenerator. (A lot of current web application didn't need to be ES5 compliant).

Anyway the CSP violation the well documented here
https://github.com/facebook/regenerator/blob/6e9e8d7747c2ab49927bdd9dd6261753181faec1/packages/regenerator-runtime/runtime.js#L716-L725

  // This module should not be running in strict mode, so the above
  // assignment should always work unless something is misconfigured. Just
  // in case runtime.js accidentally runs in strict mode, we can escape
  // strict mode using a global Function call. This could conceivably fail
  // if a Content Security Policy forbids using Function, but in that case
  // the proper solution is to fix the accidental strict mode problem. If
  // you've misconfigured your bundler to force strict mode and applied a
  // CSP to forbid Function, and you're not willing to fix either of those
  // problems, please detail your unique predicament in a GitHub issue.
  Function("r", "regeneratorRuntime = r")(runtime);

It means, if you have an CSP violation you should not run this in strict mode. Since this is a known behavior in regenerator-runtime, pdf.js need should turn off the strict-mode.

@timvandermeij Can you re-read the comment please if there is a task todo for pdf.js or not?

@timvandermeij
Copy link
Contributor

I don't really see what PDF.js could do differently here. Even though the comment is clear, we intentionally run PDF.js with strict mode to prevent errors and allow for optimizations. Given that this didn't happen before and we don't even use facebook/regenerator directly (but only as a dependency of another package) I would say that those should be patched, unless there is a trivial change we can/need to do on our side, but I don't know what that would be then...

@jkroepke
Copy link

jkroepke commented Oct 6, 2019

@timvandermeij

Thanks for figure it out.

What about a babel free version of pdf.js ? Modern browsern should not need regenerator-runtime polyfill.

@timvandermeij
Copy link
Contributor

timvandermeij commented Oct 6, 2019

@jkroepke Ideally we wouldn't use Babel at all, but unfortunately it's required for now for PDF.js to work in all browsers that we support, so getting rid of it is not something we can consider right now. Of course you can build PDF.js yourself and disable Babel transpiling.

@sonhle
Copy link

sonhle commented Nov 9, 2019

I am experiencing the same issue with 2.3.200. Any workaround or fixes? Thanks.

@jkroepke
Copy link

@timvandermeij

At the end, I see that pdf.js is doing it wrong since it use the strict mode which is not supported when using regenerator-runtime/babel. So it's not an upstream problem anymore because the limitations are well documented.

I see 4 alternatives to resolve this issues:

  • Using an older version of babel. The 2.1.266 version of pdf.js does not have this issue. pinning the versions should be resolve it. This should be is quickest.
  • Do not use strict mode since the dependencies of babel doesn't support it.
  • Turn off the transpile plugins with using regenerator-runtime. (https://caniuse.com/#feat=es6-generators)
  • Provide two version of pdfjs-dist. One for old browsers (transpiled to ES5) and one for the current browsers. (transpile to ES6)

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 11, 2019

Using an older version of babel. The 2.1.266 version of pdf.js does not have this issue. pinning the versions should be resolve it. This should be is quickest.

Pinning a dependency to an old version is never a good idea, since it for one may cause issues if there's any security bugs in older Babel versions. Furthermore, using older versions of one dependency may prevent, delay, and/or complicate updating of other dependencies thus causing other problems.

[...] (https://caniuse.com/#feat=es6-generators)

Just to clarify: Please note that the PDF.js library isn't (currently) using any generator functions, however async/await is being used and that's why this particular dependency exists here.

Provide two version of pdfjs-dist. One for old browsers (transpiled to ES5) and one for the current browsers. (transpile to ES6)

This seems like the most realistic option out of the suggestions above, but how/when that will happen isn't clear at this point in time (note the discussion/problems in PR #11241).
However, note that only the following (general) kinds of builds would then be provided:

  • Builds only compatible with the latest ES-{version}, whatever that might be at the time, i.e. the built files are not run through Babel and no polyfills are included.
  • Builds which are ES5-compatible, i.e. the built files are parsed by Babel and with polyfills included.

@jkroepke
Copy link

Builds only compatible with the latest ES-{version}

Would you respect to use only functions/technologies supported by the lastest 2 major browser versions?

A version with super bleeding edge language proposals functions which not supported by newer browser would not help us.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 12, 2019

Would you respect to use only functions/technologies supported by the lastest 2 major browser versions?

That would require producing three different types of builds, which doesn't seem like a great idea. For one, users would probably be even more confused as to which build to use (since I imagine users being unsure even with just two types of builds). Furthermore, attempting to provide multiple builds will also put more of a strain, e.g. related to both support and maintenance, on the few regular PDF.js contributors.

Basically, the desirable situation in this case as far as I'm concerned would be that a library user either:

  • Picks the ES-latest build, and provides polyfills as necessary themselves based on which platforms/browsers they need to support.
  • Picks the ES5 build, and gets all the necessary polyfills included and code compatible with "all" modern browsers.

A version with super bleeding edge language proposals functions which not supported by newer browser would not help us.

Historically speaking I don't think that we've ever started to use functionality immediately when it became available in e.g. Firefox Nightly, hence I cannot foresee this actually being much of a problem in practice.

@makidelille
Copy link

Hi,

I ran into the same issue, the solution i used was to load regenerator-runtime thought my polyfill directly.

This way i didn't change pdfjs-dist. It allowed me to resolve my CSP issues without having to recompile pdfjs :)

@jkroepke
Copy link

jkroepke commented Feb 4, 2020

@makidelille Do you use angular?

@makidelille
Copy link

makidelille commented Feb 4, 2020

@makidelille Do you use angular?

we use angularjs (still).

To be exact, we have build a custom viewer on top of pdf.js that is used throught a angularjs directive

edit: the custom viewer is build with typescript angularjs, that's why we have polyfills.

@Dreamsorcerer
Copy link

I am getting another unsafe-inline error for the CSS, from this line of code:
https://github.com/mozilla/pdf.js/blob/master/web/ui_utils.js#L893

Refused to apply inline style because it violates the following Content Security Policy directive: "style-src ...". Either the 'unsafe-inline' keyword, a hash ('sha256-MW4Iy/yu3WipUpTMM/T6OjvUu9R6vUwutodlmYNo6qM='), or a nonce ('nonce-...') is required to enable inline execution.

@Spiral1401
Copy link

Spiral1401 commented Feb 5, 2020

Hi,

I ran into the same issue, the solution i used was to load regenerator-runtime thought my polyfill directly.

This way i didn't change pdfjs-dist. It allowed me to resolve my CSP issues without having to recompile pdfjs :)

Do you (or anyone else) know how this solution would translate to angular 2+ ? Is it possible to fix this?

I am using this library and running into the same CSP problem (if you'd like you can see my ticket in that projects issues list), but these sorts of problems with packages/npm/dependency type stuff is not something I grasp all that well.

Unfortunately the old version of pdfjs that did not have this problem (2.1.266, as mentioned above) does not seem to be compatible with this angular 2+ wrapper library, and it does not appear to have a version that used that version of pdfjs internally.

edit: For anyone in a similar situation as me, there is another angular pdfjs wrapper library that worked for me with no CSP issues. See here.

@Snuffleupagus
Copy link
Collaborator

I believe that this issue can be closed now, since the upcoming release will feature two kinds of builds:

  • A modern build (for up-to-date browsers), which is not transpiled with Babel and without any included polyfills.
  • A ES5-compatible build (can be used e.g. with IE11), which is transpiled with Babel and includes all necessary polyfills.

@loremaps
Copy link
Author

Sounds good! Thanks @Snuffleupagus and everyone worked on this 🥇

@Illyism
Copy link

Illyism commented Jun 1, 2021

For those wondering how to disable strict mode, this is an attempt if you're building yourself: 226106d

You can then simply run gulp minified-legacy to get a build without strict mode.

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

No branches or pull requests