Support redacting sensitive options #133
Conversation
There was a problem hiding this comment.
The approach is good I think. I think #131 would rehaul this because in order to implement that we need to move away from free-form messages and towards more structured messages... but that's a bigger piece of work and we will think about it when it's time. With the current implementation, this approach is good I think.
Co-authored-by: Andrea Leopardi <[email protected]>
|
@whatyouhide thank you for reviewing. I'll keep moving forward with this approach. |
|
@anthonator let me know what this is ready for review 🙃 |
|
@whatyouhide I have all code in place that redacts messages for types. Please feel free to review that. I'm working on implementing the |
|
@whatyouhide this is ready for review. |
| @@ -40,4 +41,31 @@ defmodule NimbleOptions.ValidationError do | |||
|
|
|||
| message <> suffix | |||
There was a problem hiding this comment.
Wouldn't it be really important to also redact when turning the exception into a message? That's what happens when you raise said exception so I think this is what we want, not Inspect (as you generally won't inspect the exception).
There was a problem hiding this comment.
The redaction happens when the struct is built so the message should be good at this point. Unless there's something I'm missing.
I think redacting on Inspect is important. I would be surprised if I was using this feature and some secrets leaked due to inspecting ValidationError. If we're going to redact it should be everywhere.
There was a problem hiding this comment.
Added a test showing message is redacted when using NimbleOptions.validate!/2.
There was a problem hiding this comment.
@whatyouhide is there anything more here that needs done?
|
Thanks @anthonator 💟 |
Wanted to get some initial work in front of you all to make sure my approach is acceptable.
My main concern with this implementation is trying to centralize the redact logic as much as I can. Since this is a security minded feature I want to make sure that future contributors don't need to implement redaction themselves so I'm making it part of the implementation pattern for types.
Let me know if you feel like I'm on the right track or not. Open to suggestions as well.