Ensure JSON Key Path is an Actual File or IO Object#134
Conversation
cacheflow
force-pushed
the
prevent-opening-json-key-path-unless-valid-io-object
branch
2 times, most recently
from
dcc792e to
bb13556
Compare
cacheflow
changed the title
cacheflow
changed the title
|
Hey @sabman or @erimicel, do you all think this would be a good addition to the library? On one project, I worked on we encountered an IO error that caused a credential to leak out unintentionally, so I created this PR to ensure the JSON key path is an IO like object. Any feedback would be greatly appreciated. |
|
|
||
| max_path_len = 1024 | ||
| valid_path = path.is_a?(String) && path.length <= max_path_len | ||
| valid_path && File.file?(path) |
There was a problem hiding this comment.
Suggestion: You might be able to simplify this as :
def valid_json_key_path?(path)
return false unless path.is_a?(String) && path.length <= 1024
File.file?(path)
end There was a problem hiding this comment.
Thanks! I'm going to keep it the way it is because I think the use of false and unless can be confusing due to the negative return value and negative check.
| elsif valid_json_key_path?(@json_key_path) | ||
| File.open(@json_key_path) | ||
| else | ||
| credentials_error_msg(@json_key_path) |
There was a problem hiding this comment.
Suggestion: credentials_error_msg is a bit sounds misleading since it raises an exception rather than returning a message
There was a problem hiding this comment.
I agree - we can probably rename this at some point and/or add an ! to the function name too.
Hi @cacheflow, changes looks good and make sense for me, but I can't merge or release new patch for it, you have to wait @sabman |
|
@erimicel, I made the change. Tell me what you think about the new method name. |
|
@sabman is there a way we can merge this into master/main and then add it to the next release? |
|
Will review shortly |
|
Running CI |
Just following up on this. Did the CI pass? If so, I'd love to get this merged and released in the next version so folks don't face the same issue we once did at work. |
|
Hey @sabman was the history erased related to hound bot in this PR? I can't find the comments anymore. 
|
|
merged - cutting release now |
3 participants
Currently, in the json_key method it assumes that json_key_path will be an IO-like object or filename. This can cause an issue where if someone accidentally passes credentials in plaintext or a non-IO object, the Gem will attempt to open the credentials and display the credentials in plain text.
This PR resolves #133.