Skip to content
This repository has been archived by the owner on Apr 7, 2021. It is now read-only.

Encryption broken after Laravel code change #30

Closed
axelitus opened this issue Sep 17, 2018 · 11 comments
Closed

Encryption broken after Laravel code change #30

axelitus opened this issue Sep 17, 2018 · 11 comments
Assignees
Labels
Milestone

Comments

@axelitus
Copy link

axelitus commented Sep 17, 2018

Due to a code change in Laravel code identified in #28, the package no longer works because no encrypted values are stored for the models.

This code change introduced 21 days ago affects all Laravel Versions (5.5, 5.6 and 5.7).

I've set up a gist with a sample use case. I want to save email drivers config so that the users can send emails from different sources. As they contain sensitive information (server address, password, etc.) it is better to store them encrypted.

For the sample use case, include the source code from the gist, call the seeder in the DatabaseSeeder file with $this->call(SendersTableSeeder::class); and then just run php artisan migrate:fresh --seed.

In the database there should be a row in the senders table with the mail_config column set with the encrypted value. Due to the code not working it shows the unencrypted value: {"driver":"log","host":null,"port":null,"username":null,"password":null,"encryption":null,"from":{"address":"admin@localhost","name":"Local Admin"}}

@axelitus
Copy link
Author

axelitus commented Sep 17, 2018

As proposed in #28 I've made a test by changing the name of the AustinHeap\Database\Encryption\Traits::getAttributes() method to getUnencryptedAttributes() and letting Laravel handle the getAttributes() call (which just returns the $this->attributes property that has the encrypted value as before the change).

The encrypted value is being stored correctly in the database:

��__LARAVEL-DATABASE-ENCRYPTED-VERSION-00-01-02__��version�VERSION-00-01-02��type�string[native]��eyJpdiI6IlpXdjlXWkNTYVZvXC9ENVhsS3paaUp3PT0iLCJ2YWx1ZSI6IlhIekh4TDJDUzBVa0o4SGpVMVVCdmN4UXVldmNEdThNdWZUY1JJUkR5QkVRSkNpMStEZ2pVUUp2WnpDWUtVdXprZ1ViMlwvQ3Vpa2NWWEZ6aGNXbXNSVUdIXC8rTnppMytFUTJFeGFnQ1RlZldxWmUxY2V2QlFITXJ2S3hQeEZzOVdOTGpRVUEzUHNid3JFcDFqRDNFRGd5OWxqYnNcL2hOWE82QUNGbnVFaGtOTWkxU2NXTFwvVTkrQ0FxSDVqbUZpb3hQT21VaGdcL0REWUNLNTVVUVI5MUsrdz09IiwibWFjIjoiYWZlNTQ4NjZiYmY2MjIwZWE0ZmMyNDRkZGRkNzRkMjc1YjUxZGQxMmFhMjkzNzkzMDAxNmEyNmJjNzVjOTk4NyJ9

However, this change may potentially break other things in your code. I don't know (haven't had the time to inspect everything) if you are relying on the getAttributes() method to return the unencrypted values elsewhere.

@austinheap austinheap self-assigned this Sep 20, 2018
@austinheap austinheap added the bug label Sep 20, 2018
@austinheap austinheap added this to the 0.2.0 milestone Sep 20, 2018
@TechTailor
Copy link

Hey, can you guys confirm if this issue is fixed?

@axelitus
Copy link
Author

axelitus commented Oct 8, 2018

At the moment there hasn't been any new commits since reporting this issue, so I think that it still does not work out-of-the-box. I've been using the code with my workaround applied and it has been working (though I haven't tested it thoroughly). I'll try to do a pull-request with the changes I've made later on the day.

@axelitus
Copy link
Author

axelitus commented Oct 8, 2018

Hey @TechTailor see if the changes from the pull-request #32 help you out and fix your issue #31.

@austinheap
Copy link
Owner

@axelitus @TechTailor any feedback? v0.2.0-rc3 ready for testing/feedback. Should be good to go!

@axelitus
Copy link
Author

Sorry @austinheap been busy with other things. Let me try the new code this week.

@katzsimon
Copy link

It looks like it is working now for me

@austinheap
Copy link
Owner

Sorry @austinheap been busy with other things. Let me try the new code this week.

@axelitus No worries, just want to make sure it's working for you now 😄

It looks like it is working now for me

@katzsimon Great to hear!

@axelitus
Copy link
Author

Alright @austinheap, I tried the v0.2.0-rc3 in my project and it seems to be working fine. The data is now being encrypted into the database record.

@austinheap
Copy link
Owner

@axelitus Good deal! I'll release v0.2.0 in a week to give time for regressions to occur.

@austinheap
Copy link
Owner

v0.2.0 (Packagist) released, thanks for the help @axelitus, @TechTailor, & @katzsimon!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants