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

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

Open
rkwright opened this issue Apr 1, 2017 · 23 comments
Open

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

rkwright opened this issue Apr 1, 2017 · 23 comments

Comments

@rkwright
Copy link
Member

rkwright commented Apr 1, 2017

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

  • Readium Chrome extension
    • latest official version available from the Chrome Web Store

Additional information

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

@rkwright
Copy link
Member Author

rkwright commented Apr 3, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

Interesting thread on Stackoverflow here

@rkwright
Copy link
Member Author

rkwright commented Apr 4, 2017

@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 rkwright changed the title JavaScript popups no longer working in latest builds *JavaScript* no longer working in latest builds of Chrome App Apr 4, 2017
@rkwright
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member Author

rkwright commented Apr 4, 2017

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
Copy link
Member

danielweck commented Apr 5, 2017

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
Copy link
Member Author

rkwright commented Apr 5, 2017

@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
Copy link
Member Author

rkwright commented Apr 5, 2017

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

@danielweck
Copy link
Member

Exactly.

@rkwright
Copy link
Member Author

rkwright commented Apr 5, 2017

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:

  • Readium would be a tab again
  • We get search via the Ctrl-F command back
  • We might be able to get around the CSP blockage
  • We might avoid deprecation at the end of 2017
  • If the above is true we may not have to migrate all the users to a new book-storage

But that's a lot of ifs...

@attilavago
Copy link

@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
Copy link

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

  • Readium doesn't support Node 7+
  • The Cloud Reader itself would work fine wrapped in NW.js, except the library doesn't work over HTTP and keeps yapping about the epub_content/epub_library.opds file
  • The ChromeApp packaged inside an NW.js app results in more or less similar 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:

  • quicker loading, as it would not need to copy the book over into Readium
  • library export
  • cross-OS support: MAC, Windows, Linux if done with Electron
  • no browser tab, extension and dependency nonsense

@danielweck
Copy link
Member

Thanks @attilavago
Regarding NodeJS v7+: I believe the problem actually stems from NPM v4+, see #600

@danielweck
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link

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
Copy link
Member

danielweck commented Jun 13, 2018

For your information: exelearning/iteexe#258
Also: exelearning/iteexe#319 (same problem)

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

No branches or pull requests

4 participants