Skip to content

Do not attempt to convert nil into PII attributes #6124

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

Merged
mitchellhenke merged 1 commit into from
Mar 30, 2022
Merged

Do not attempt to convert nil into PII attributes #6124

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions app/models/profile.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def decrypt_pii(password)
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?
Copy link
Contributor

@aduth aduth Mar 30, 2022

Choose a reason for hiding this comment

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

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)
new_from_hash(pii) 

      return new if pii_json.blank?
      pii = JSON.parse(pii_json, symbolize_names: true)
      return new if pii.blank?
      new_from_hash(pii)

Copy link
Contributor Author

@mitchellhenke mitchellhenke Mar 30, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@zachmargolis zachmargolis Mar 30, 2022

Choose a reason for hiding this comment

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

should we have done a truthiness check? in case the value was a literal false ?

Copy link
Contributor Author

@mitchellhenke mitchellhenke Mar 30, 2022

Choose a reason for hiding this comment

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

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)

Pii::Attributes.new_from_json(decrypted_recovery_json)
end

Expand Down