Refactor logging to avoid leaking sensitive data #906
Conversation
To prevent log leak, we need to reduce the number of code paths things go through. This protocol was too much magic. Logging favors `CustomStringConvertible`, since interpolation knows how to do that automatically for most types. Given this is the case, we will standardize on using String(describing:) when logging. I've piped actions through a separate `Action.logger`. They'll be chattier now that we aren't changing their description. Separate loggers allow for better filtering when needed.
This affords us an extra layer of "the right thing by default". By wrapping a property, you define CustomStringConvertible and CustomDebugStringConvertible values that redact the underlying string.
This prevents leakage via actions.
This matches our use in other parts of the API.
| static let logger = Logger( | ||
| subsystem: Config.default.rdns, | ||
| category: "AppAction" | ||
| ) |
There was a problem hiding this comment.
Give actions their own logger. Standardizing on this new pattern for actions, at least until we address subconsciousnetwork/ObservableStore#38.
| case .focusChange(let focused): | ||
| return "focusChange(\(focused)" | ||
| case .setValue: | ||
| return "setValue(--redacted--)" |
There was a problem hiding this comment.
Manually redact setValue. It's too hard to get away from strings for validated form fields, since most of our custom types return nil for invalid values, and we need the input to support intermediate values.
Validated form fields are more likely than other places to contain sensitive information which could leak through this action, so the easiest thing to do is redact all setValue() actions.
There was a problem hiding this comment.
Good call, I don't think we need to see the value anyway. It's always reflected on the UI in the context that these actions are generated.
| public let verbatim: String | ||
| public var id: String { description } | ||
| // !!!: Mnemonic is secret and should never be logged or persisted | ||
| @Redacted public private(set) var mnemonic: String |
There was a problem hiding this comment.
Create an explicit field for the secret mnemonic and redact it. This ensures you must access the property explicitly.
Calling String(describing: recoveryPhrase) will not accidentally reveal the mnemonic.
bfollington
deleted the
2023-09-27-do-not-leak-recovery-code-through-inputs
branch
Fixes #905.
Refactors logging to make it easier to avoid logging sensitive data.
Changes:
CustomLogStringConvertible. This was too fancy, and had fallbacks which made it difficult to reason about where and how to redact.String.loggable()withString(describing:).@Redactedproperty decorator. Apply to any property to implementCustomStringConvertibleandCustomDebugStringConvertibleto return the literal string "--redacted"RecoveryPhraseto use@RedacteddebugDescription, but notdescription. This change redacts both, so you have to be very specific that you want the string value of the mnemonic by referencing the property directly.FormFieldto redact all strings for value actions. We should not be logging user's input for most fields anyway, and validated form fields are most likely to contain sensitive information.