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

instanceof failing in window / iframe contexts #350

Merged
merged 3 commits into from
Sep 5, 2016

Conversation

ddxdental
Copy link
Contributor

prepareContent fails to properly derive type of data passed in, should the Blob originate from another context (window or iframe).

Described here from MDN (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof)

instanceof and multiple context (e.g. frames or windows)

Different scope have different execution environments. This means that they have different built-ins (different global object, different constructors, etc.). This may result in unexpected results. For instance, [] instanceof window.frames[0].Array will return false, because Array.prototype !== window.frames[0].Array and arrays inherit from the former. This may not make sense at first but when you start dealing with multiple frames or windows in your script and pass objects from one context to another via functions, this will be a valid and strong issue. For instance, you can securely check if a given object is in fact an Array using Array.isArray(myObj)

Proposal is to first check instanceof (much faster), with a backup check against Object.prototype.toString which is safe across contexts (http://perfectionkills.com/instanceof-considered-harmful-or-how-to-write-a-robust-isarray/)

instanceof fails with data passed in from other contexts
instanceof failing in window / iframe contexts
@@ -420,6 +420,9 @@ exports.prepareContent = function(name, inputData, isBinary, isOptimizedBinarySt

// if inputData is already a promise, this flatten it.
var promise = external.Promise.resolve(inputData).then(function(data) {

var isBlob = data instanceof Blob || ['[object File]', '[object Blob]'].indexOf(Object.prototype.toString.call(data)) != -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use !== instead of != ?

@dduponchel
Copy link
Collaborator

I think you forgot to actually use isBlob :-)

@ddxdental
Copy link
Contributor Author

Wow. Well that was dumb ;) Upped

@dduponchel dduponchel merged commit 887dcd8 into Stuk:master Sep 5, 2016
dduponchel added a commit to dduponchel/jszip that referenced this pull request Sep 28, 2016
The pull request Stuk#350 introduced a bug but the automated tests didn't
catch it:

> ReferenceError: Blob is not defined
>     at /home/travis/build/Stuk/jszip/lib/utils.js:425:38
>     at Array.1 (/home/travis/build/Stuk/jszip/node_modules/lie/lib/index.js:88:21)
>     at nextTick (/home/travis/build/Stuk/jszip/node_modules/lie/node_modules/immediate/lib/index.js:61:18)
>     at process._tickCallback (node.js:458:13)
>
> The command "npm run $COMMAND" exited with 0.
>
> Done. Your build exited with 0.

This commit fixes:
- the bug: check if Blobs are supported before actually using them
- the build: if an uncaught error/rejection happens, exit with a non
  zero code.
@dduponchel dduponchel mentioned this pull request Oct 5, 2016
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

Successfully merging this pull request may close these issues.

2 participants