readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
559 stars 186 forks source link

*JavaScript* no longer working in latest builds of Chrome App #615

Open rkwright opened 7 years ago

rkwright commented 7 years ago

This issue is a Bug

Related issue(s) and/or pull request(s)

None

Expected Behaviour

When clicked, the links in the document should pop up a little note. The user maintains that this worked in previous versions (not yet verified) Issue was reported on the forum here

Observed behaviour

Instead, nothing happens.

Steps to reproduce

  1. Open the test document here
  2. Go to page 3 and click on the link (e.g. "Danaid")
  3. Nothing happens

Test file(s)

See above

Product

Additional information

Sort of works in iBooks but the note is clipped, user reports it works fine gitden on Android

rkwright commented 7 years ago

I dug into this a bit. A couple of comments:

  1. The EPUB is strictly speaking, not valid since it includes scripts yet labels itself version 2. Scripts are not allowed in EPUB 2 files - they have to be version 3. If you try it in EPUBCheck, you will see a large number of errors. You should always run EPUBCheck on your EPUBs. I do that as I build them using this script here.
  2. The metadata looks wonky. You didn't really build it with Microsoft FrontPage, did you? :-) Note that version 3 requires that you have a "last modified" metadata entry too. There is a jar and a script for it that does this automatically here and here
  3. Finally, the reason the popups don't work is they appear to be violating security rules in Chrome:

    jquery-1.4.2.js:898 Refused to execute inline script because it violates the following Content Security Policy directive: "default-src 'self' blob: filesystem: chrome-extension-resource:". Either the 'unsafe-inline' keyword, a hash ('sha256-gRJoL0r6uzxUbnC2o4Ju4qIZMwuSXuAraMfwyBXC/IE='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'script-src' was not explicitly set, so 'default-src' is used as a fallback.

    01.htm:27 Refused to execute inline script because it violates the following Content Security Policy directive: "default-src 'self' blob: filesystem: chrome-extension-resource:". Either the 'unsafe-inline' keyword, a hash ('sha256-gRJoL0r6uzxUbnC2o4Ju4qIZMwuSXuAraMfwyBXC/IE='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'script-src' was not explicitly set, so 'default-src' is used as a fallback.

    01.htm:37 Refused to execute inline script because it violates the following Content Security Policy directive: "default-src 'self' blob: filesystem: chrome-extension-resource:". Either the 'unsafe-inline' keyword, a hash ('sha256-g4Je1aClPSkwLumspps6Mfa+nr9eoK3fcYEHFhEJLkQ='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'script-src' was not explicitly set, so 'default-src' is used as a fallback.

We did tighten up some security items recently and I suspect that may be the cause and/or Chrome did so.

@danielweck Any comments?

rkwright commented 7 years ago

I downloaded the new version of the user's book, which though it now passes EPUBCheck, still raises the same exceptions in Chrome. The new version is here

@danielweck Any comments?

rkwright commented 7 years ago

Interesting thread on Stackoverflow here

rkwright commented 7 years ago

@danielweck I tried inserting a meta element as referenced in the SO thread, but made no difference. However, I also went back and loaded 0.24 then 0.23. Both failed with the same message. And Chrome won't even load CRXes from before 0.23.

rkwright commented 7 years ago

I am now suspecting this is some Chrome issue/restriction for packaged apps. If I run the HTML in the Chrome browser directly, the popups work fine. But I cannot find any executable (CRX) that doesn't cause the exception to be thrown. But all of the CRXes are executing with my current Chrome engine, 57.0.2987.133

rkwright commented 7 years ago

IMPORTANT @danielweck @JCCR @ryanackley I have now discovered that ANY JavaScript in ANY EPUB now throws these exceptions. None of my WebGL, SVG or any other EPUB with JavaScript will execute. This includes EPUB Test Suite 102, which has some simple tests for scripting. Have bumped the bug up to blocking. I am surprised that we only have one user complaining about this. But it must have been introduced in a very recent build of Chrome, probably 57 as my WebGL and SVG EPUBs (both have JavaScript) were working last week.

rkwright commented 7 years ago

Tested with the CloudReader here and the EPUB Test Suite 102 passes, so it appears that the problem is restricted to the Chrome app. The Chrome dev blog says they fixes "several" security problems, one critical and 9 high, but they don't say what.

rkwright commented 7 years ago

I found this page: It includes a list of security bugs fixed, including:

[$1000][669086] Medium CVE-2017-5033: Bypass of Content Security Policy in Blink. Credit to Nicolai Grødum

But if I try to follow the link to get more details I am told I do not have the permission to view the page.

rkwright commented 7 years ago

Andrew Gribben commented: I haven't looked at the codebase, but the issue rung a bell with me. Is this helpful?

Chrome contentSecurityPolicy

The way I read it Readium would need to white list the origin (blob or filesystem in this case?) in order for the in-line script to execute, but when a script is loaded from an external file using an EventListener then it should work as planned (I think).

rkwright commented 7 years ago

I have now created two simple EPUB test-files, Tiny-Bad-JS and Tiny-Good-EPUB, following the guidelines in the CSP document Andrew referenced above. As expected, the "Bad" EPUB generated three exceptions, while the "Good" EPUB worked as expected with no exceptions. The problem with this is that we can't realistically expect our users to modify all of their existing EPUBs. Some of them have hundreds of EPUBs with snippets of inline JS or event handlers. I have one SVG file that has ONE click handler in it and all the JS in an external file. That file no longer works either.

danielweck commented 7 years ago

Well, inline scripts were always disabled due to Chrome app Content Security Policy ... but it is possible that a more recent build of Chrome plugged a loophole in their iframe implementation.

By the way, this CSP restriction is the reason why we had to inject the RequireJS config script directly into the single-file AMD bundle (which contains the Almond loader). See:

https://github.com/readium/readium-js-viewer/blob/master/src/chrome-app/index.html#L27

<head>
...
    <script type="text/javascript" src="scripts/readium-js-viewer_all_CHROMEAPP.js"> </script>

    <!-- script type="text/javascript" src="scripts/requirejs-config.js"> </script -->

    </head>

https://github.com/readium/readium-js-viewer/blob/master/build-config/RequireJS_config_single-bundle_CHROMEAPP.js#L42

    paths:
    {
...
        "readium_js_viewer_RJS-CONFIG":
            process._RJS_rootDir(3) + '/src/chrome-app/requirejs-config',
...
    },
rkwright commented 7 years ago

@danielweck Hm. I see in the article referenced above:

On the web, such a policy is defined via an HTTP header or meta element. Inside Chrome's 
extension system, neither is an appropriate mechanism. Instead, an extension's policy is 
defined via the extension's manifest.json file as follows:

{
  ...,
  "content_security_policy": "[POLICY STRING GOES HERE]"
  ...
}

Seems like this suggests that we can somehow "white list" or "bless" inline content? Or is this what @ryanackley meant when he said "Readium is a packaged app. Unlike extensions, you can't change the inline script CSP"

rkwright commented 7 years ago

Useful info for this discussion, perhaps: https://developer.chrome.com/webstore/apps_vs_extensions

danielweck commented 7 years ago

Exactly.

rkwright commented 7 years ago

But it's not clear to me that even if we reverted to a "pure" extension that it would solve our problem. Or is it that we could, as an extension, modify the CSP behaviour so that inline scripts would be allowed? However, if we did "revert" back to an extension, it might have the following benefits:

But that's a lot of ifs...

attilavago commented 7 years ago

@rkwright , @danielweck , am planning to look into this during this Easter break and possibly come up with a more radical fix. Took a swing at it last Sunday, but failed building it with Node 7, therefore I now need to set up a Docker just for this running Node 6.

attilavago commented 7 years ago

Ended up using NVM, builds fine with Node 6.10.2, which brings me to a list of issues:

The way I see going forward is a sans HTTP implementation of Readium. Working directly from the file system would have benefits such as:

danielweck commented 7 years ago

Thanks @attilavago Regarding NodeJS v7+: I believe the problem actually stems from NPM v4+, see https://github.com/readium/readium-js-viewer/issues/600

danielweck commented 7 years ago

There is an existing Electron implementation of ReadiumJS in this feature branch: https://github.com/readium/readium-js-viewer/tree/feature/electron (I have not tried NW.js though)

rkwright commented 7 years ago

@attilavago Thanks! I would agree with the benefits you outline. This direction is, I am afraid, probably our only viable course. The problems include having to migrate hundreds of thousands of users from their currently library to a new one having to test all that and then shepherd the users to a new solution. Far from trivial. I've done this before (Adobe Digital Editions when I ran the eBook group at Adobe). It can be a nightmare, even with a dedicated QE team - which Readium does not have.

danielweck commented 7 years ago

The Electron feature branch ( https://github.com/readium/readium-js-viewer/tree/feature/electron ) is a naive (but working) port of the Chrome app, in the sense that the HTML5 FileSystem storage backend is unchanged (as it works perfectly fine in the open-source Chromium engine, as it does in Google's Chrome browser).

BigBlueHat commented 7 years ago

Thankfully (for all) Chrome Apps are on their way out--in favor of the Web: https://blog.chromium.org/2016/08/from-chrome-apps-to-web.html

On Windows, Mac, and Linux, we encourage developers to migrate their Chrome apps to the web. Developers who can’t fully move their apps to the web can help us prioritize new APIs to fill the gaps left by Chrome apps. In the short term, they can also consider using a Chrome extension or platforms such as Electron or NW.js.

So it seems this is heading the right direction with a move to Electron (or similar).

Sadly @rkwright is likely right about the migration cost/process: "The problems include having to migrate hundreds of thousands of users from their currently library to a new one having to test all that and then shepherd the users to a new solution."

I'm still new enough here to not know what all that entails, but wanted to stop by an 👍 the move to something webbier than Chrome Apps (for many reasons). 😃

danielweck commented 6 years ago

For your information: https://github.com/exelearning/iteexe/issues/258 Also: https://github.com/exelearning/iteexe/issues/319 (same problem)