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.

Sign up for GitHub

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 parsing unknown fields breaks record deserialisation #666

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Fix parsing unknown fields breaks record deserialisation #666

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1cfa3d
- fix records parsing failure on unknown fields (now skips)
borrow-checker Sep 25, 2024
e5e2e3d
Merge remote-tracking branch 'origin/release/sdk/java/core/v16.6.6' i…
borrow-checker Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 68 additions & 54 deletions sdk/java/core/src/main/kotlin/com/keepersecurity/secretsManager/core/SecretsManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,31 @@ data class KeeperFile(
val data: KeeperFileData,
val url: String,
val thumbnailUrl: String?
)
) {
override fun equals(other: Any?): Boolean {
if (this === other) return true
if (javaClass != other?.javaClass) return false

other as KeeperFile

if (!fileKey.contentEquals(other.fileKey)) return false
if (fileUid != other.fileUid) return false
if (data != other.data) return false
if (url != other.url) return false
if (thumbnailUrl != other.thumbnailUrl) return false

return true
}

override fun hashCode(): Int {
var result = fileKey.contentHashCode()
result = 31 * result + fileUid.hashCode()
result = 31 * result + data.hashCode()
result = 31 * result + url.hashCode()
result = 31 * result + (thumbnailUrl?.hashCode() ?: 0)
return result
}
}
Comment on lines +304 to +328
Copy link
Author

Choose a reason for hiding this comment

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

We have a bytearray in this data class which means we need to implement our own equals and hashcode

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we don't do equality checks on this class

Copy link
Author

Choose a reason for hiding this comment

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

That hashcode is more important here - if this ever goes in a hashed data structure without it it won't work


data class KeeperFileUpload(
val name: String,
Expand Down Expand Up @@ -744,22 +768,18 @@ private fun fetchAndDecryptSecrets(
if (response.records != null) {
response.records.forEach {
val recordKey = decrypt(it.recordKey, appKey)
val decryptedRecord = decryptRecord(it, recordKey)
if (decryptedRecord != null) {
records.add(decryptedRecord)
}
decryptRecord(it, recordKey).onSuccess { keeperRecord -> records.add(keeperRecord) }
Copy link
Author

Choose a reason for hiding this comment

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

this is the magic part where if there's a failure it skips gracefully (i.e. onSuccess only gets called if we ge a successful Result)

}
}
if (response.folders != null) {
response.folders.forEach { folder ->
val folderKey = decrypt(folder.folderKey, appKey)
folder.records!!.forEach { record ->
val recordKey = decrypt(record.recordKey, folderKey)
val decryptedRecord = decryptRecord(record, recordKey)
if (decryptedRecord != null) {
decryptedRecord.folderUid = folder.folderUid
decryptedRecord.folderKey = folderKey
records.add(decryptedRecord)
decryptRecord(record, recordKey).onSuccess { keeperRecord ->
keeperRecord.folderUid = folder.folderUid
keeperRecord.folderKey = folderKey
records.add(keeperRecord)
}
}
}
Expand All @@ -777,51 +797,45 @@ private fun fetchAndDecryptSecrets(
}

@ExperimentalSerializationApi
private fun decryptRecord(record: SecretsManagerResponseRecord, recordKey: ByteArray): KeeperRecord? {
val decryptedRecord = decrypt(record.data, recordKey)

val files: MutableList<KeeperFile> = mutableListOf()

if (record.files != null) {
record.files.forEach {
val fileKey = decrypt(it.fileKey, recordKey)
val decryptedFile = decrypt(it.data, fileKey)
files.add(
KeeperFile(
fileKey,
it.fileUid,
Json.decodeFromString(bytesToString(decryptedFile)),
it.url,
it.thumbnailUrl
)
)
}
}

// When SDK is behind/ahead of record/field type definitions then
// strict mapping between JSON attributes and object properties
// will fail on any unknown field/key - currently just log the error
// and continue without the field - nb! field will be lost on save
var recordData: KeeperRecordData? = null
try {
recordData = Json.decodeFromString<KeeperRecordData>(bytesToString(decryptedRecord))
} catch (e: Exception) {
// New/missing field: Polymorphic serializer was not found for class discriminator 'UNKNOWN'...
// New/missing field property (field def updated): Encountered unknown key 'UNKNOWN'.
// Avoid 'ignoreUnknownKeys = true' to prevent erasing new properties on save/update
println("Record ${record.recordUid} has unexpected data properties (ignored).\n"+
" Error parsing record type - KSM SDK is behind/ahead of record/field type definitions." +
" Please upgrade to latest version. If you need assistance please email [email protected]")
//println(e.message)
try {
// Attempt to parse the record data with unknown fields
recordData = nonStrictJson.decodeFromString<KeeperRecordData>(bytesToString(decryptedRecord))
} catch (e: Exception) {
println("Error parsing record data with using non-strict JSON parser. Record ${record.recordUid} will be skipped.")
}
private fun decryptRecord(encryptedRecord: SecretsManagerResponseRecord, recordKey: ByteArray): Result<KeeperRecord> {

val decryptedRecord = decrypt(encryptedRecord.data, recordKey)

val decryptedRecordFiles = encryptedRecord.files?.map { recordFile ->
val fileKey = decrypt(recordFile.fileKey, recordKey)
val decryptedFile = decrypt(recordFile.data, fileKey)
KeeperFile(
fileKey,
recordFile.fileUid,
Json.decodeFromString(bytesToString(decryptedFile)),
recordFile.url,
recordFile.thumbnailUrl
)
} ?: emptyList()
Comment on lines +804 to +814
Copy link
Author

Choose a reason for hiding this comment

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

functional > imperative (plus why allocate when you don't need to?)


val recordData: KeeperRecordData = try {
Copy link
Author

Choose a reason for hiding this comment

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

Restructuring this ensures that there are only two non exception pathways out - either you get the record data you want, or you don't, and both are ok.

nonStrictJson.decodeFromString(bytesToString(decryptedRecord))
} catch (e: SerializationException) {
System.err.println(
"""
Record ${encryptedRecord.recordUid} has unexpected data properties (ignored).
Error parsing record type - KSM SDK is behind/ahead of record/field type definitions.
Please upgrade to latest version. If you need assistance please email
""".trimIndent()
)
return Result.failure(e)
}

return if (recordData != null) KeeperRecord(recordKey, record.recordUid, null, null, record.innerFolderUid, recordData, record.revision, files) else null
val keeperRecord = KeeperRecord(
recordKey = recordKey,
recordUid = encryptedRecord.recordUid,
folderUid = null,
folderKey = null,
innerFolderUid = encryptedRecord.innerFolderUid,
data = recordData,
revision = encryptedRecord.revision,
files = decryptedRecordFiles
)
return Result.success(keeperRecord)
}

@ExperimentalSerializationApi
Expand Down