-
Notifications
You must be signed in to change notification settings - Fork 0
Added safe env wrapper while loading and processing YAML files #5
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,9 @@ | |
|
|
||
| module Secvault | ||
| class Secrets | ||
| # Regexp to match unsafe environment variable values (comma, dash, and colon) | ||
| UNSAFE_ENV_VALUE_REGEXP = /^\s*[,:-]/ | ||
|
|
||
| # Define permitted classes for YAML.safe_load - commonly used in Rails secrets | ||
| PERMITTED_YAML_CLASSES = [ | ||
| Symbol, | ||
|
|
@@ -93,11 +96,8 @@ | |
|
|
||
| # Read and process the plain YAML file content | ||
| source = path.read | ||
|
|
||
| # Process ERB and parse YAML - using same method as Rails | ||
| erb_result = ERB.new(source).result | ||
| secrets = YAML.respond_to?(:unsafe_load) ? YAML.unsafe_load(erb_result) : YAML.safe_load(erb_result, aliases: true, permitted_classes: PERMITTED_YAML_CLASSES) | ||
|
|
||
|
|
||
| secrets = load_secrets_from_yaml(source) | ||
| secrets ||= {} | ||
|
|
||
| # Only load environment-specific section (YAML anchors handle sharing) | ||
|
|
@@ -108,15 +108,46 @@ | |
| def read_secrets(secrets_path, env) | ||
| if secrets_path.exist? | ||
| # Handle plain YAML secrets.yml only - using same method as Rails | ||
| erb_result = ERB.new(secrets_path.read).result | ||
| all_secrets = YAML.respond_to?(:unsafe_load) ? YAML.unsafe_load(erb_result) : YAML.safe_load(erb_result, aliases: true, permitted_classes: PERMITTED_YAML_CLASSES) | ||
| source = secrets_path.read | ||
| all_secrets = load_secrets_from_yaml(source) | ||
|
|
||
| env_secrets = all_secrets[env.to_s] | ||
| return env_secrets.deep_symbolize_keys if env_secrets | ||
| end | ||
|
|
||
| {} | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def load_secrets_from_yaml(source) | ||
|
Check failure on line 123 in lib/secvault/secrets.rb
|
||
| # Process ERB and parse YAML - using same method as Rails but with safe env wrapper | ||
|
|
||
| erb_result = nil | ||
| with_safe_env do | ||
| erb_result = ERB.new(source).result | ||
| end | ||
|
|
||
| YAML.respond_to?(:unsafe_load) ? YAML.unsafe_load(erb_result) : YAML.safe_load(erb_result, aliases: true, permitted_classes: PERMITTED_YAML_CLASSES) | ||
| end | ||
|
|
||
| def with_safe_env | ||
|
Check failure on line 134 in lib/secvault/secrets.rb
|
||
| original_env_method = ENV.method(:[]) | ||
|
|
||
| ENV.define_singleton_method(:[]) do |key| | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current approach of simply wrapping the value in double quotes might not correctly handle environment variables that already contain special characters like quotes or backslashes, potentially leading to invalid YAML. For a more robust solution, consider using a standard library method for serialization, such as |
||
| value = original_env_method.call(key) | ||
|
|
||
| return value if value.nil? || value.strip == '' | ||
|
|
||
| return "\"#{value}\"" if value =~ UNSAFE_ENV_VALUE_REGEXP | ||
|
|
||
| value | ||
| end | ||
|
|
||
| yield | ||
| ensure | ||
| ENV.define_singleton_method(:[], original_env_method) | ||
| end | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Monkey-patching a global object like
ENVis not thread-safe. If multiple threads execute this code concurrently (e.g., in a Puma server), it could lead to race conditions where one thread interferes with another's execution or leaves theENVobject in a corrupted state. A safer, thread-local approach would be to create a custom binding with a proxy object forENVand pass it toERB.new(source).result(binding). This would isolate the change without modifying global state.