search

orta / cocoapods-keys

A key value store for storing per-developer environment and application keys
MIT License
1.55k stars 92 forks source link

Loading keyrings from unvalidated YAML files. #170

Open Skogetroll opened 7 years ago

Skogetroll commented 7 years ago

There's problem with KeyringLiberator.get_keyring_at_path/KeyringLiberator. get_all_keyrings — it works pretty fine when all the files in the keys_dir folder are in exact format library needs, but in case any of them deviates from unwritten scheme all gets broken.

For example, our team had situation when one of YAML files in ~/.cocoapods/keys became empty(for unknown reasons), and when we tried to do pod install we had this:

NoMethodError - undefined method '[]' for false:FalseClass
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/keyring.rb:14:in 'from_hash'
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/keyring_liberator.rb:66:in 'get_keyring_at_path'
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/keyring_liberator.rb:60:in 'block in get_all_keyrings'
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/keyring_liberator.rb:59:in 'each'
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/keyring_liberator.rb:59:in 'get_all_keyrings'
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/keyring_liberator.rb:28:in 'get_current_keyring'
~/.rvm/gems/ruby-2.4.1/gems/cocoapods-keys-2.0.0/lib/preinstaller.rb:23:in 'setup'
… // rest of the callstack is not so important

And it crashes with this because YAML.load(in get_keyring_at_path) for empty file/string returns false, and in Keyring.from_hash there's attempt to perform subscript operator ([]) with false in hash parameter, because there's no checks on this parameter, neither type or keys are validated.

So, it is recommended to add some checks in Keyring.from_hash and similar methods to prevent such nuisance from happening. Now any random invalid .yml file in the folder easily breaks plugin work.

orta commented 7 years ago

Weird - yeah, I'd definitely accept a PR that wraps the YML loader with a nicer error message for cases like this 👍