Fix annotating sites with broken Function.bind polyfills by seanh · Pull Request #315 · hypothesis/client

seanh · 2017-03-27T15:17:48Z

Fixes #245

seanh · 2017-03-27T15:18:29Z

src/annotator/annotation-sync.coffee

+    iframe = document.createElement('iframe')
+    iframe.style.display = 'none'
+    document.body.appendChild(iframe)
+    bind = iframe.contentWindow.Function.prototype.bind


Should we move this bind into some standard place that different files can import it from?

I think this would make sense. It seems likely that we might want to do this in other places in future. It should also be much easier to test if you extract it out into a separate module.

seanh · 2017-03-27T15:21:08Z

src/annotator/test/annotation-sync-test.js

-        createAnnotationSync();
-
-        publish('deleteAnnotation', {msg: ann}, function() {});
+  function addTests() {


The problem here is that I want to run all of AnnotationSync's tests in two contexts: with and without a broken Function.bind in the parent page. Maybe not all the tests really need to be run with the broken Function.bind, but a few of them should be, and this avoids duplicating them.

It does add some "cleverness" to the test code though.

An alternative would be to just add one test for the broken Function.bind context: that the constructor doesn't crash. Since the broken Function.bind that the tests are using is one that crashes when called, just this one test would prove that AnnotationSync doesn't use the document's Function.bind

seanh · 2017-03-27T15:23:57Z

Most of the tests diff that you see is just due to code having been indented. I didn't add or change any tests. I just wrapped all the tests in an addTests() function (indenting them all), called addTests() at the bottom of the file, and then added a context() for when the parent page contains a broken Function.bind polyfill (just a context() containing a beforeEach() and an afterEach()) and called addTests() a second time from within that context.

As noted above, I think it might be better just to add a single "it does not crash if the parent page contains a crashing Function.bind polyfill" test, instead of doing this addTests() thing.

seanh · 2017-03-27T16:40:28Z

Ok, I've moved the clean function.Bind into a clean-context util module, and I've replaced the test cleverness discussed above. Ready for review again

robertknight · 2017-03-28T10:40:30Z

src/annotator/util/clean-context.js

@@ -0,0 +1,22 @@
+/**
+ * Export clean versions of standard functions and objects.
+ *


I think we need to be a bit clearer about the motivation here and what "clean" means. Specifically that this module is a workaround for environments that overwrite properties on global objects with broken polyfills or other problematic values.

I've edited the paragraph below to make this more explicit

robertknight · 2017-03-28T10:41:35Z

src/annotator/util/clean-context.js

+ * This module exports "clean" versions of various standard functions that may
+ * have been modified in the parent browsing context that the client is
+ * injected into. Code should always use the clean versions from this module,
+ * and never the potentially modified versions from the parent context.


Need to clarify what "Code" means here. Specifically you mean code in the "annotator/" dir, as this isn't relevant for the sidebar.

Done, although I don't want to mention the annotator dir by name here as that comment seems unlikely to be updated if we rename it

robertknight · 2017-03-28T10:51:21Z

src/annotator/util/clean-context.js

+// browsing context to extract clean objects and functions from.
+var iframe = document.createElement('iframe');
+iframe.style.display = 'none';
+document.body.appendChild(iframe);


On my shiny new Mac I measured the cost of creating an iframe as being ~6-13ms, so probably up ~100ms on a mobile device. It would be nice not to do this for the vast majority of pages that are not going to need it.

I've asked around whether there is a cheaper way, but an approach I'm pretty sure will work fine for feature detection is just to check whether the stringified function (via fn.toString()) contains the text [native code]. This is currently a convention but will most likely be part of the language spec in future.

That looks like it's "NativeFunction" rather than "[native code]"? But is there any guarantee that a page's replacement of a native function won't stringify to "NativeFunction" as well?

@seanh "NativeFunction" in the spec is a reference to another part of the spec. Search for other occurrences of "NativeFunction" in that document :)

Yes it is technically possible for a page to overwrite toString() with something which always returns function () { [native code] } but I think it is unlikely enough that we'll be OK doing that.

Or of someone just modifies Function.prototype.bind.something but doesn't change toString?

Let's see if we run into anyone that does it, I guess. I think this clean context thing is a temporary patch (before moving all of src/annotator into a same-origin iframe) anyway.

If we want to move the whole thing into an iframe by the way, when we do that we will have to eat this 6-100ms delay every time anyway won't we?

Done - I've changed it to not inject an iframe if it doesn't think bind has been modified

robertknight · 2017-03-28T10:52:47Z

I tested this by overwriting Function.prototype.bind with a function that just throws an exception and found a couple more places that blow up _setupSidebarEvents and _setupDocumentEvents.

robertknight

This approach works fine. Three requests:

Creating an iframe is expensive, we should avoid it when not needed
The documentation needs to clarify what "clean" means and which code this applies to
I found a couple more places in the sidebar app that use bind() that will still call the broken version.

seanh · 2017-03-28T13:54:43Z

I tested this by overwriting Function.prototype.bind with a function that just throws an exception and found a couple more places that blow up _setupSidebarEvents and _setupDocumentEvents.

Fixed these two as well

Add failing tests for bugs in annotation-sync.js and sidebar.coffee when the parent document contains a broken Function.bind polyfill. See #245

Add a module that uses an invisible same-origin iframe to return clean copies of global function and objects if the parent browsing context's copies have been modified. So far it contains only a clean copy of Function.bind, but more can be added. Also change other modules to always use cleanContext.bind and never Function.bind directly. Fixes #245

codecov-io · 2017-03-30T13:10:00Z

Codecov Report

Merging #315 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   76.48%   76.73%   +0.24%     
==========================================
  Files         120      121       +1     
  Lines        5886     5918      +32     
  Branches      959      964       +5     
==========================================
+ Hits         4502     4541      +39     
+ Misses       1384     1377       -7

Impacted Files	Coverage Δ
src/annotator/util/clean-context.js	`100% <100%> (ø)`
src/annotator/annotation-sync.coffee	`76.19% <100%> (+0.58%)`	⬆️
src/annotator/sidebar.coffee	`71.42% <100%> (+0.37%)`	⬆️
src/sidebar/components/annotation-share-dialog.js	`100% <0%> (+21.87%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b3c132...ff60dc9. Read the comment docs.

seanh · 2017-03-30T13:21:41Z

src/annotator/test/annotation-sync-test.js

      });
    });
+
+    context('if the parent context has a crashing Function.bind polyfill', function() {


I've left this test (and the one for sidebar below) here as I can't think of a better way. This obviously isn't ideal as every module that uses clean-context would need to have its own "doesn't crash" test.

I looked into replace Function.prototype.bind globally with a crashing function in the tests, but don't see any way to do that for all non-sidebar tests but not do it for the src/sidebar tests.

I looked into writing an eslint rule to warn on uses of bind, but it seemed difficult to warn when bind() or bind.call() is called but not when the calling code is in src/sidebar and not when cleanContext.bind.call() is called. (It may be doable, I've not written eslint rules before, but there wasn't an obvious good way to do it.)

The remaining option would be to delete these tests, but that would simply mean that the actual bug fixes in this pull request would not be covered by tests. I don't think the presence of these tests is doing any particular harm or means that we'd necessarily have to add similar tests for every other piece of code that uses clean-context.

seanh · 2017-03-30T13:39:07Z

@robertknight This can be reviewed again.

My one remaining concern is whether CSP can prevent the iframe from being injected? I tested in Chrome using the Chrome extension on a page with <meta http-equiv="Content-Security-Policy" content="child-src 'none';"> and it works fine. Appending an iframe with no src seems to be allowed. This seems to make sense as the rule is child-src, but I do wish I could find some good written confirmation.

The bookmarklet fails on that page (either Firefox or Chrome) but that's not this branch's fault - app.html runs afoul of the page's content security policy,

If I set an src="http://www.example.com" on the iframe (to simulate what would happen if Chrome did block it) then on the page with the CSP, using the Chrome extension, it gets a CSP error that entirely prevents the sidebar from loading.

So I don't think this branch breaks anything. But what I'd really like to do to be safer is catch this CSP DOMException and just fall back on using the parent context's bind. But a normal try ... catch doesn't seem to catch this exception.

seanh · 2017-03-30T13:45:35Z

(My theory by the way is that since the iframe has no src at all is always allowed. If I set iframe.src to 'http://localhost:8889/' (which is there I'm serving the page with the CSP) that is blocked. And if I change the CSP from none to self it's not blocked anymore. But as long as there no src at all Chrome at least seems to allow it regardless of CSP.

seanh · 2017-03-31T15:55:05Z

Closed in favour of #331

seanh added the Needs Review label Mar 27, 2017

seanh commented Mar 27, 2017

View reviewed changes

seanh force-pushed the fix-annotating-sites-with-broken-Function.bind-polyfills branch from 32c37d4 to b2caed7 Compare March 27, 2017 16:39

seanh assigned robertknight and seanh Mar 28, 2017

robertknight reviewed Mar 28, 2017

View reviewed changes

robertknight removed the Needs Review label Mar 28, 2017

robertknight suggested changes Mar 28, 2017

View reviewed changes

seanh force-pushed the fix-annotating-sites-with-broken-Function.bind-polyfills branch from b2caed7 to 31458f8 Compare March 28, 2017 13:54

seanh added 3 commits March 30, 2017 13:45

Add failing tests for broken Function.bind polyfill

34a51a9

Add failing tests for bugs in annotation-sync.js and sidebar.coffee when the parent document contains a broken Function.bind polyfill. See #245

Fix crash when Function.prototype.bind is not a function

ff60dc9

seanh force-pushed the fix-annotating-sites-with-broken-Function.bind-polyfills branch from 31458f8 to ff60dc9 Compare March 30, 2017 13:09

seanh commented Mar 30, 2017

View reviewed changes

seanh mentioned this pull request Mar 30, 2017

Spike: Investigate ways to isolate annotation layer code from page JS hypothesis/product-backlog#148

Open

seanh closed this Mar 31, 2017

lyzadanger deleted the fix-annotating-sites-with-broken-Function.bind-polyfills branch March 3, 2020 14:12

Conversation

seanh commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanh commented Mar 27, 2017

Uh oh!

seanh commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanh Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertknight commented Mar 28, 2017

Uh oh!

robertknight left a comment

Choose a reason for hiding this comment

Uh oh!

seanh commented Mar 28, 2017

Uh oh!

codecov-io commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanh commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanh commented Mar 30, 2017

Uh oh!

seanh commented Mar 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanh Mar 28, 2017 •

edited

Loading

codecov-io commented Mar 30, 2017 •

edited

Loading

seanh commented Mar 30, 2017 •

edited

Loading