Added safe env wrapper while loading and processing YAML files#5
Added safe env wrapper while loading and processing YAML files#5prabodh-panda wants to merge 1 commit intomainfrom
Conversation
prabodh-panda
commented
Nov 2, 2025
prabodh-panda
commented
- Fixes Invalid environment variable causes entire object to be null #4
![neeto-bugwatch[bot]](https://avatars.githubusercontent.com/in/1747066?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 NeetoBugWatch Review
I've analyzed your pull request and found some potential improvements that might be worth considering. Please review the specific line comments below for helpful suggestions.
💡 To trigger a re-review of this PR, add a comment with the keyword neeto-bugwatch-run anywhere in the comment body.
| 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) |
There was a problem hiding this comment.
Monkey-patching a global object like ENV is 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 the ENV object in a corrupted state. A safer, thread-local approach would be to create a custom binding with a proxy object for ENV and pass it to ERB.new(source).result(binding). This would isolate the change without modifying global state.
| def with_safe_env | ||
| original_env_method = ENV.method(:[]) | ||
|
|
||
| ENV.define_singleton_method(:[]) do |key| |
There was a problem hiding this comment.
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.to_json or YAML.dump(value).strip, which will handle all necessary escaping correctly.
|
@prabodh-panda Can we either close this PR or make it ready for review? |
|
@yedhink I have a call scheduled with Unni today at 11 to discuss this issue. We can take action after that. |
|
This issue occurs only in Rails 7.1. Secvault works as expected in Rails 7.2, so we can close this PR. |
2 participants
