refactor: simplify config file loading with unified path handling#22
refactor: simplify config file loading with unified path handling#22billybonks merged 1 commit intomasterfrom
Conversation
![managerbot-app[bot]](https://avatars.githubusercontent.com/in/853044?s=60&v=4){kind=small}
| - "spec/**/*" | ||
|
|
||
| Security/YAMLLoad: | ||
| Enabled: false |
There was a problem hiding this comment.
Disabling Security/YAMLLoad cop is potentially dangerous. YAML.load is unsafe as it can deserialize arbitrary objects which could lead to remote code execution. Consider using YAML.safe_load instead.
lib/committer/config/accessor.rb
Outdated
| home_config = load_config_from_path(Committer::Config::Constants::CONFIG_FILE) | ||
| git_root_config = load_config_from_git_root | ||
|
|
||
| home_config = YAML.load(read_file_from_home(Committer::Config::Constants::CONFIG_FILE_NAME)) || {} |
There was a problem hiding this comment.
Using YAML.load instead of YAML.safe_load is a security risk that can lead to remote code execution vulnerabilities. This is likely why the Security/YAMLLoad cop was disabled. Consider using YAML.safe_load instead.
lib/committer/config/accessor.rb
Outdated
| git_root_config = load_config_from_git_root | ||
|
|
||
| home_config = YAML.load(read_file_from_home(Committer::Config::Constants::CONFIG_FILE_NAME)) || {} | ||
| git_root_config = YAML.load(read_file_from_git_root(Committer::Config::Constants::CONFIG_FILE_NAME)) || {} |
There was a problem hiding this comment.
Same security concern with YAML.load here. Consider using YAML.safe_load.
spec/committer/config_spec.rb
Outdated
| let(:config_dir) { File.join(temp_home, '.committer') } | ||
| let(:config_file) { File.join(config_dir, 'config.yml') } | ||
| let(:home_formatting_rules) { File.join(config_dir, 'formatting_rules.txt') } | ||
| let(:home_formatting_rules) { File.join(config_dir, 'commit_message_only.propmpt') } |
There was a problem hiding this comment.
There's a typo in the file name 'commit_message_only.propmpt' - should be 'commit_message_only.prompt'
spec/committer/config_spec.rb
Outdated
| let(:git_config_dir) { File.join(git_root, '.committer') } | ||
| let(:git_config_file) { File.join(git_config_dir, 'config.yml') } | ||
| let(:git_formatting_rules) { File.join(git_config_dir, 'formatting_rules.txt') } | ||
| let(:git_formatting_rules) { File.join(git_config_dir, 'commit_message_only.propmpt') } |
There was a problem hiding this comment.
Same typo here in 'commit_message_only.propmpt'
spec/committer/config_spec.rb
Outdated
| # Make sure YAML.load_file raises an exception for the home config | ||
| allow(YAML).to receive(:load_file).with(config_file).and_raise( | ||
| # Make sure YAML.read_path_prioritized_file raises an exception for the home config | ||
| allow(YAML).to receive(:read_path_prioritized_file).with(config_file).and_raise( |
There was a problem hiding this comment.
There seems to be a mismatch between the method name in the comment (read_path_prioritized_file) and the actual mock (YAML.read_path_prioritized_file). The comment should be updated to correctly reflect what's being mocked.
This refactors the configuration loading system to create a consistent path prioritization mechanism (git root > home dir > defaults) for all file types. The changes include: - Replaced specific file loading methods with generic path-prioritized readers - Created a centralized defaults path in constants - Moved default formatting rules to a dedicated defaults directory - Simplified error handling for malformed YAML configs This streamlined approach will make it easier to add new prompt file types in the future while reducing code duplication and maintenance overhead.
billybonks
force-pushed
the
refactor/config-accessor
branch
from
b8ed10c to
03e06a6
Compare
1 participant
This refactors the configuration loading system to create a consistent path prioritization mechanism (git root > home dir > defaults) for all file types. The changes include: