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

fix: Use a null prototype object for this.files #766

Merged
merged 3 commits into from Jun 29, 2021

Conversation

@MichaelAquilina
Copy link
Contributor

@MichaelAquilina MichaelAquilina commented Jun 14, 2021

This approach is taken to prevent overriding object methods that would exist on a normal object Object.create({})

This approach is taken to prevent overriding object methods that would
exist on a normal object Object.create({})
@@ -179,16 +179,16 @@ var out = {
*/
forEach: function(cb) {
var filename, relativePath, file;
/* jshint ignore:start */
// ignore warning about unwanted properties because this.files is a null prototype object
for (filename in this.files) {
Copy link
Contributor Author

@MichaelAquilina MichaelAquilina Jun 14, 2021

Choose a reason for hiding this comment

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

@Stuk we could use for(filename of Object.keys(this.files)) { here instead, but it seems jszip is targetting es3 in jshintrc.

We could set this to es6 but I assume this would break for a number of users and you would want to avoid that?

This is the error that comes up if I change it:

   ./lib/object.js
    182 |        for (filename of Object.keys(this.files)) {
                               ^ 'for of' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).

Copy link
Owner

@Stuk Stuk Jun 15, 2021

Choose a reason for hiding this comment

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

Yeah, there's a bigger task for upgrading the tooling and browser compatibility for this library. The approach here looks good.

Copy link
Owner

@Stuk Stuk left a comment

Thanks for the fix!

@@ -179,16 +179,16 @@ var out = {
*/
forEach: function(cb) {
var filename, relativePath, file;
/* jshint ignore:start */
// ignore warning about unwanted properties because this.files is a null prototype object
for (filename in this.files) {
Copy link
Owner

@Stuk Stuk Jun 15, 2021

Choose a reason for hiding this comment

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

Yeah, there's a bigger task for upgrading the tooling and browser compatibility for this library. The approach here looks good.

assert.equal(result, "hello\n", "the zip was correctly read.");
done();
})['catch'](JSZipTestUtils.assertNoError);
});
Copy link
Owner

@Stuk Stuk Jun 15, 2021

Choose a reason for hiding this comment

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

I tried removing the Object.create(null) fix, to see if this test failed and it didn't, so I don't think this is actually catching the issue?

Copy link
Contributor Author

@MichaelAquilina MichaelAquilina Jun 16, 2021

Choose a reason for hiding this comment

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

That's a good point - but I'm not entirely sure how to test this change because essentially this change is about making sure toString / __proto__ / __constructor__ etc are not available in the first place.

I guess I could add a test that shows that toString raises an error - but I'm not sure how useful that would be in the long run?

The reasoning for this test specifically was to show that files with names that shadow standard Object method names can still be accessed correctly

@MichaelAquilina
Copy link
Contributor Author

@MichaelAquilina MichaelAquilina commented Jun 16, 2021

@Stuk it seems like the build pipeline is failing. But from what I can tell this is an existing failure that is unrelated to this change?

@MichaelAquilina
Copy link
Contributor Author

@MichaelAquilina MichaelAquilina commented Jun 23, 2021

@Stuk polite reminder about this :) There's no real rush though so take your time

@Stuk
Copy link
Owner

@Stuk Stuk commented Jun 23, 2021

@MichaelAquilina I've updated the test to confirm the prototype isn't modified. Mind double checking that change? Then I think this is good to go!

Copy link
Contributor Author

@MichaelAquilina MichaelAquilina left a comment

@Stuk looks good :) thanks for helping out

@Stuk Stuk merged commit 6d029b4 into Stuk:master Jun 29, 2021
@MichaelAquilina MichaelAquilina deleted the fix/files-null-prototype branch Jun 29, 2021
igandrews
Copy link

igandrews commented on 2235749 Aug 10, 2021

Choose a reason for hiding this comment

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

Out of curiosity was there actually a report of such an issue as a file named toString that prompted this change? I ask because this actually introduced a bug in my code that uses jszip because i used hasOwnProperty on the files just like your own code above but now that throws because there is no such member.

igandrews
Copy link

igandrews commented on 2235749 Aug 10, 2021

Choose a reason for hiding this comment

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

Nevermind. I see there was.

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

Successfully merging this pull request may close these issues.

None yet

3 participants