CSHARP-5065: Deserialize wrappedBytes directly instead of using RawBsonDocument in ExplicitEncryptionLibMongoCryptController.cs #1321
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adelinowona
requested review from
rstam,
BorisDog and
sanych-sun
and removed request for
a team
BorisDog
reviewed
May 8, 2024
| @@ -568,8 +568,8 @@ private Guid UnwrapKeyId(RawBsonDocument wrappedKeyDocument) | |||
|
|
|||
| private BsonValue UnwrapValue(byte[] encryptedWrappedBytes) | |||
| { | |||
| var rawDocument = new RawBsonDocument(encryptedWrappedBytes); | |||
| return rawDocument["v"]; | |||
| var bsonDocument = BsonSerializer.Deserialize<BsonDocument>(encryptedWrappedBytes); | |||
There was a problem hiding this comment.
Looks safe to me. I don't recall what was the motivation to use RawBsonDocument as in most cases the v value will being read completely but consumer.
Maybe @rstam has some ideas.
There was a problem hiding this comment.
I don't know why a RawBsonDocument is being used here. If the goal is to deserialize the key bytes we should just do that directly.
Not sure that is the goal though.
rstam
requested changes
May 24, 2024
| @@ -568,8 +568,8 @@ private Guid UnwrapKeyId(RawBsonDocument wrappedKeyDocument) | |||
|
|
|||
| private BsonValue UnwrapValue(byte[] encryptedWrappedBytes) | |||
| { | |||
| var rawDocument = new RawBsonDocument(encryptedWrappedBytes); | |||
| return rawDocument["v"]; | |||
| var bsonDocument = BsonSerializer.Deserialize<BsonDocument>(encryptedWrappedBytes); | |||
There was a problem hiding this comment.
I don't know why a RawBsonDocument is being used here. If the goal is to deserialize the key bytes we should just do that directly.
Not sure that is the goal though.
adelinowona
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
wrap uses of
RawBsonDocumentappropriately in using statements.Switch to using
BsonSerializer.Deserializein theUnwrapValuefunction instead of usingRawBsonDocument. This helps us since we couldn't dispose ofRawBsonDocumentin this case as the returned value would be disposed as well. Also avoids us having to do cloning as a remedy if we were to still useRawBsonDocument.Just want input on whether I'm missing something here by not using RawBsonDocument