Skip to content

Potential for shared state in this.files after clone() in jszip/lib/index.js #949

@xzyingxiashubro

Description

@xzyingxiashubro

I've been working with JSZip (it seems I'm on a version around 3.10.1 based on the source) and encountered a behavior with the clone() method that I thought might be worth discussing. It appears that the current implementation of clone() could potentially lead to an unexpected sharing of the internal files object between the original and the cloned instance.

Current Behavior: From what I can see in index.js, the clone() method iterates through the properties of the original JSZip instance and assigns them to a new instance. For object properties like this.files, this assignment (newObj[i] = this[i];) results in a shallow copy. This means both the original and cloned JSZip instances' files property seem to point to the same object in memory.

// Snippet from JSZip constructor in lib/index.js
this.clone = function() {
    var newObj = new JSZip();
    for (var i in this) {
        if (typeof this[i] !== "function") {
            newObj[i] = this[i]; // This line would shallow copy 'this.files'
        }
    }
    return newObj;
};

Observed Issue: As a consequence, if a file is added or modified in the cloned instance (for example, using clone.file(...)), this modification also appears to affect the files object of the original instance, presumably because they are the same underlying object.
A simple test case that highlights this might look like:

const original = new JSZip();
original.file("example.txt", "initial content");

const clone = original.clone();
// Modifying a file in the clone
clone.file("example.txt", "modified content in clone");

// Checking the original's file content
original.file("example.txt").async("string").then(function (content) {
    // content here is "modified content in clone",
    // whereas one might expect "initial content"
    console.log("Original content:", content);
});

Expected Behavior: Typically, one might expect a clone() method to create a fully independent copy of the object. For an object managing a collection of files, this would ideally include a deep copy of its core mutable state, such as the files collection. This would ensure that operations on the cloned archive do not inadvertently alter the original archive.
Consideration: It's possible this behavior is by design for specific reasons, or perhaps a deep clone of this.files (which would involve cloning each file entry) was considered too costly or complex. However, if the intent is for clone() to provide a truly isolated copy, the current approach for this.files might not fully achieve that.
I wanted to bring this to your attention for consideration. It might be beneficial if this behavior was further clarified in the documentation, or if the clone() method could be enhanced to ensure deeper independence for the files collection if that aligns with the library's goals.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions