Do not attempt to convert nil into PII attributes#6124
Conversation
changelog: Bug Fixes, Account Management, Request password to decrypt PII before regenerating personal key
changelog: Bug Fixes, Account Management, Request password to decrypt PII before regenerating personal key
| def recover_pii(personal_key) | ||
| encryptor = Encryption::Encryptors::PiiEncryptor.new(personal_key) | ||
| decrypted_recovery_json = encryptor.decrypt(encrypted_pii_recovery, user_uuid: user.uuid) | ||
| return nil if JSON.parse(decrypted_recovery_json).nil? |
There was a problem hiding this comment.
Could this have been implemented in Pii::Attributes.new_from_json to avoid double-parsing, and since we already have at least one check there already?
Here:
identity-idp/app/services/pii/attributes.rb
Lines 22 to 24 in daf7265
return new if pii_json.blank?
pii = JSON.parse(pii_json, symbolize_names: true)
return new if pii.blank?
new_from_hash(pii)There was a problem hiding this comment.
It could, yeah
The competing factor is this is so far only known to affect recovery PII, and Pii::Attributes is used in more places and this is a quicker fix, so I opted towards the smaller scope, though it should definitely be given a fuller consideration in the follow-up work.
There was a problem hiding this comment.
should we have done a truthiness check? in case the value was a literal false ?
There was a problem hiding this comment.
I think for the specific case here the problem value is nil, but we could expand it to be anything that is not a Hash since that's really the only valid type (and we could also validate that it has the expected keys, etc. if we wanted as well)
4 participants
Follow up to #6121