ActiveRecord: Encryption silently fails when a db connection isn't already open #217
Comments
|
@anicholson Thank you for you very well written bug report! I think swallowing all the errors is bad. I'm sorry I let that code slip in. I think if anything we should at the bare minimum remove the rescue. I think it's reasonable to expect the DB to be connected. ActiveRecord does a lot of magic based on the DB. However, we definitely should be bubbling up issues. Would you like to open a PR or shall I? |
|
@saghaulor I'm happy to, but it'll be next couple days :) |
|
I've got no beef with AE expecting the db to be connected, btw, just that it should be called out more :) |
|
@saghaulor I'm not sure how to proceed without re-introducing an error for @DouweM (and anyone else who wants to precompile assets without a db present). Suggestions welcomed :) |
|
ping @saghaulor :) |
|
@anicholson Maybe by using |
|
Hey this is happening to me too, in a Sinatra app that uses the Sinatra ActiveRecord extensions. My records store without throwing any errors/exceptions, the saved model has all its attributes set correctly, but all the |
|
I've just done some more digging. When I copypaste @anicholson's spec, with variable names changed around (weird coincidence that my model is also called a # https://github.com/attr-encrypted/attr_encrypted/issues/217
context 'attr_encrypted issue #217', :focus do
before :each do
@credential = Credential.create!(
account_name: 'An Account Name',
username: 'A Username',
password: 'A Password',
)
end
it 'encrypts credentials' do
expect(@credential.encrypted_username).to_not be_nil
expect(@credential.username).to_not be_nil
expect(@credential.username).to_not eq @credential.encrypted_username
end
it 'saves encrypted credentials' do
# This part passes just fine for me
encrypted_username = Credential.find(@credential.id).encrypted_username
expect(encrypted_username).to eq @credential.encrypted_username
expect(@credential.reload.username).to eq 'A Username' # This passes
expect(Credential.find(@credential.id).username).to eq 'A Username' # This fails with "bad decrypt" error
end
end |
|
We hit this issue in production. A brief DNS issue when our node started up caused DB connection errors when initializing the attr_encrypted columns for our ActiveRecord models. These errors were silently ignored due to #160 and attr_encrypted created regular Ruby instance variables for our encrypted attributes (rather than delegating to ActiveRecord for reading/write the attribute values). The end result was a server that returned nil for any encrypted columns (because it was reading from regular instance variables rather than ActiveRecord managed attributes) and wrote nil to the database for any encrypted columns (because it was writing regular instance variable rather than ActiveRecord managed attributes). Perhaps we could avoid using the DB connection altogether during startup if we get rid of the attr_encrypted falling back to regular Ruby instance variables for encrypted attributes on ActiveRecord models? It looks like the methods generated in |
|
We have the same issue which took a long time to track down. This was manifesting as a failed NULL constraint in a column for us so at least it wasn't silently dropping it, but was very confusing for us. |
|
@anicholson you mentioned "When we jiggled our environment.rb to connect to the db before the models loaded, the problem went away". What did you change in your (I'm running into this issue, and haven't been able to find a workaround.) |
|
I believe that #294 should fix this issue. As such, I'm closing this issue. If it does not resolve your issue please feel free to reopen the issue. |
We recently encountered a nasty bug where encryption silently failed:
Rails: No
AR Version: 4.2.1
attr_encryptedversion: > 372ff14Consider the following spec:
describing the following model:
Spec 1 would consistently pass, but Spec 2 would consistently fail, because (surprisingly!)
credentials == nil, not@credentials.encrypted_credentials.After hours of head-scratching & digging around in the console, in desperation we traversed back up the
attr_encryptedcommit history looking for regressions.We hit pay-dirt when we reverted past #160, when we got this error!
Wha?!
And so we realised that
attr_encryptedrequires the db connection to be present before a model that uses it gets loaded. When we jiggled ourenvironment.rbto connect to the db before the models loaded, the problem went away.If #160 hadn't swallowed all the errors, we would have realised this much, much sooner. Is there a nicer way to achieve the same result?
The text was updated successfully, but these errors were encountered: