Failing test: updating document with array of subdocuments that have encrypted fields

[maximilianschmitt]

Hi!

I've noticed an issue when mongoose-field-encryption is used in a subdocument that is inside an array of its parent.

Something like this:

const nestedAuthorSchema = new Schema({
    name: String,
    password: String,
});
nestedAuthorSchema.plugin(mongooseFieldEncryption, { fields: ["password"], secret: "some secret key" });
const postSchema = new Schema({
    title: String,
    message: String,
    authors: [nestedAuthorSchema],
});
const Post = mongoose.model("Post3", postSchema);

If you attempt to update the parent and save it using Document.prototype.save(), you get an error similar to the following:

const post = await Post.findOne({})
post.title = "Something"
await post.save()

// 💥
// MongoError: Cannot create field '-1' in element {authors: [ { _id: ObjectId('5fbac6eb2ae2b1cd93ef2ac4'), name: "some name", password: "26518a99edf664ea97c38108214f4d14:b86ecce381e3c8dc7930f6ae7f139437", __enc_password: true } ]}

I've included a failing test in the PR: https://github.com/wheresvic/mongoose-field-encryption/pull/49/files#diff-755667fe9b6408db7a3d8206cd5830c4c912027bbf2290f90adea9713bc5324cR121-R167

This problem occurs because post-init, fields are first encrypted and then pre-save they are encrypted again. Because mongoose has issues tracking changes that occur in arrays without explicitly setting document.array.set(index, changedObject), the error above occurs:

  // Every subdocument that has mongoose-field-encryption as a plugin,
  // will get decrypted after it's fetched from the db:
  schema.post("init", function (_next, _data) {
    const next = getCompatitibleNextFunc(_next);
    const data = getCompatibleData(_next, _data);
    try {
      decryptFields(data, fieldsToEncrypt, secret());
      next();
    } catch (err) {
      next(err);
    }
  });

  schema.pre("save", function (_next) {
    const next = getCompatitibleNextFunc(_next);

    try {
      // ⚠️ After encrypting here, we need to tell mongoose which indexes were affected
      // in case `this` is a sub-document inside an array.
      // 🚧 Problem: We can get the parent document with `this.parent()` but how do we know
      // which path and index this sub-document belongs to?
      encryptFields(this, fieldsToEncrypt, secret());
      next();
    } catch (err) {
      next(err);
    }
  });

So, after encrypting pre-save, we need to tell mongoose which sub-documents in the array changed. What I'm not sure about: How, in a pre-save hook of a subdocument, can we tell which path and index the subdocument belongs to?

I'm thankful for input and ideas! Thanks!

[maximilianschmitt]

Here is a modified pre-save hook that seems to fix the issue for 1 level of nesting:

  schema.pre("save", function (_next) {
    const next = getCompatitibleNextFunc(_next);

    try {
      encryptFields(this, fieldsToEncrypt, secret());

      function getModifiedPath(doc, parentDoc) {
        for (const [key, value] of Object.entries(parentDoc._doc)) {
          if (Array.isArray(value)) {
            const index = value.findIndex((v) => v === doc);
            if (index !== -1) {
              return `${key}.${index}`;
            }
          } else if (value === doc) {
            return key;
          }
        }
      }

      const parentDoc = this.parent();
      if (parentDoc) {
        const modifiedPath = getModifiedPath(this, parentDoc);
        parentDoc.set(modifiedPath, this.toObject());
      }

      next();
    } catch (err) {
      next(err);
    }
  });

It's pretty brute-force. We're looking for a parent of the document. If we find one, we iterate over all the keys until we find the one that contains the subdocument. In case of an array, we also find the index.

Then, we mark the paths as modified on the parent document:

      const parentDoc = this.parent();
      if (parentDoc) {
        const modifiedPath = getModifiedPath(this, parentDoc);
        parentDoc.set(modifiedPath, this.toObject());
      }

[wheresvic]

Hi @maximilianschmitt ,

Thanks a lot for your contribution and I apologize for the delay in getting back to you on this!

The concept of nested schemas is indeed tricky and I really believe that this should not be handled by the plugin for children. It would be far simpler to create another schema for the child and simply add the plugin to the child schma. What do you think?

Also, I do not have a problem merging a test into the code, however, I see that the build failed. If you could investigate and fix the build that would be great :).

Finally, if you really do have a custom setup and are intent on using the plugin to handle say 1 level of nesting you can just fork the plugin and use it directly from github. I have decided to stabilize the API here so there will likely be no more breaking changes apart from dropping support for no longer supported mongoose/node versions.

Cheers,

[wheresvic]

@maximilianschmitt see https://github.com/wheresvic/mongoose-field-encryption/issues/34#issuecomment-577383776 for an example :)

[maximilianschmitt]

Hi @wheresvic, thanks for getting back to me!

It would be far simpler to create another schema for the child and simply add the plugin to the child schma. What do you think?

Yes that's what I did and that's how the error occurs 😬 :

// ChildSchema with encrypted fields:
const nestedAuthorSchema = new Schema({ /* ... */ });
nestedAuthorSchema.plugin(mongooseFieldEncryption, { fields: ["password"], secret: "some secret key" });

// ChildSchema is used by parent inside an array:
const postSchema = new Schema({ authors: [nestedAuthorSchema] });

Also, I do not have a problem merging a test into the code, however, I see that the build failed. If you could investigate and fix the build that would be great :).

Yes, the failing test case basically documents the issue I'm facing.

I've managed to fix the issue for arbitrary nesting in my own application by adding an additional save-hook.

I just pushed the fix to this PR. It's pretty brute force. I'm not sure if there's a more straight-forward way of dealing with the issue.

I would gladly contribute it back to this project if you think that makes sense. I'm not the most-experienced mongoose user so I also wonder if maybe there is room in mongoose itself to do something about this issue. 🤔

[wheresvic]

Hey @maximilianschmitt ,

Thanks for the update and I understand the issue now! Also, you had pretty much explained it properly the first time but somehow I did not read carefully enough and I am sorry for that.

Anyways, so yes this is really a mongoose level issue because of the way it handles subdocument array updates. Regarding your fix, I do not think that it is a wise idea to rely on mongoose internals to fix this. I will not merge this and I also recommend that you do not try something like this either. It will get difficult for you later on if mongoose changes their internals.

However, I do have a small solution to offer you - I don't know if the example you posted is anything similar to the real code but I think that the essential issue that you have is that you have a Post that can have multiple authors where an author has private data. What you could consider is to just have a list of authorId instead of the documents themselves and then keep the authors separate as a top-level document. You lose a bit of document object storage flexibility and you need to write a bit more code but this could be one approach that might be easier to maintain in the future as well.

I'm happy to discuss further - feel free to email me vic@smalldata.tech and we can even do a quick zoom or whatever if you'd like. I will close this issue for the moment!

Cheers,