Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Commit

Permalink
Refactor logging to avoid leaking sensitive data (#906)
Browse files Browse the repository at this point in the history
Fixes #905.

Refactors logging to make it easier to avoid logging sensitive data.

Changes:

- Remove `CustomLogStringConvertible`. This was too fancy, and had
fallbacks which made it difficult to reason about where and how to
redact.
- Replace all calls to `String.loggable()` with `String(describing:)`.
    - Use default string representation for actions in more places.
- Relatedly, give actions their own loggers. Actions will be chattier.
Giving them a separate logger makes for easier filtering.
- Introduce `@Redacted` property decorator. Apply to any property to
implement `CustomStringConvertible` and `CustomDebugStringConvertible`
to return the literal string "--redacted"
- Refactor `RecoveryPhrase` to use `@Redacted`
- Previously this was redacting `debugDescription`, but not
`description`. 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.
- Update `FormField` to 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.
- Update Noosphere and NoosphereService to receive mnemonic as String
(this matches other parts of the API)
  • Loading branch information
gordonbrander authored Sep 28, 2023
1 parent b7489d9 commit ae98fdc
Show file tree
Hide file tree
Showing 21 changed files with 211 additions and 212 deletions.
31 changes: 10 additions & 21 deletions xcode/Subconscious/Shared/Components/AppView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ struct AppView: View {
store.send(.appear)
}
.onReceive(store.actions) { action in
let message = String.loggable(action)
AppModel.logger.debug("[action] \(message)")
AppAction.logger.debug("\(String(describing: action))")
}
// Track changes to scene phase so we know when app gets
// foregrounded/backgrounded.
Expand All @@ -97,7 +96,13 @@ typealias NicknameFormField = FormField<String, Petname.Name>
typealias GatewayUrlFormField = FormField<String, GatewayURL>

// MARK: Action
enum AppAction: CustomLogStringConvertible {
enum AppAction {
// Logger for actions
static let logger = Logger(
subsystem: Config.default.rdns,
category: "AppAction"
)

/// Sent immediately upon store creation
case start

Expand Down Expand Up @@ -291,7 +296,7 @@ enum AppAction: CustomLogStringConvertible {
case succeedRecoverOurSphere

/// Set recovery phrase on recovery phrase component
static func setRecoveryPhrase(_ phrase: String) -> AppAction {
static func setRecoveryPhrase(_ phrase: RecoveryPhrase?) -> AppAction {
.recoveryPhrase(.setPhrase(phrase))
}

Expand All @@ -303,22 +308,6 @@ enum AppAction: CustomLogStringConvertible {
static func setAppUpgradeComplete(_ isComplete: Bool) -> AppAction {
.appUpgrade(.setComplete(isComplete))
}

var logDescription: String {
switch self {
case let .succeedMigrateDatabase(version):
return "succeedMigrateDatabase(\(version))"
case let .succeedSyncLocalFilesWithDatabase(fingerprints):
return "succeedSyncLocalFilesWithDatabase(...) \(fingerprints.count) items"
case let .succeedCreateSphere(receipt):
// !!!: Do not log mnemonic
// The user's sphere mnemonic is carried with this sphere receipt.
// It is a secret and should never be logged.
return "succeedCreateSphere(\(receipt.identity))"
default:
return String(describing: self)
}
}
}

extension AppAction {
Expand Down Expand Up @@ -1371,7 +1360,7 @@ struct AppModel: ModelProtocol {
state: state,
actions: [
.setSphereIdentity(receipt.identity),
.setRecoveryPhrase(receipt.mnemonic),
.setRecoveryPhrase(RecoveryPhrase(receipt.mnemonic)),
.followDefaultGeist
],
environment: environment
Expand Down
11 changes: 1 addition & 10 deletions xcode/Subconscious/Shared/Components/Common/Forms/Search.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import ObservableStore
import os

// MARK: Action
enum SearchAction: Hashable, CustomLogStringConvertible {
enum SearchAction: Hashable {
/// Set search presented state
case requestPresent(Bool)
/// Set search presented to false, and clear query
Expand All @@ -40,15 +40,6 @@ enum SearchAction: Hashable, CustomLogStringConvertible {

/// Handle notification of entry delete from somewhere
case entryDeleted(Slug)

var logDescription: String {
switch self {
case .setSuggestions(let suggestions):
return "setSuggestions(\(suggestions.count) items)"
default:
return String(describing: self)
}
}
}

// MARK: Model
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ struct RecoveryModeModel: ModelProtocol {
guard try await environment.noosphere.recover(
identity: did,
gatewayUrl: gatewayUrl,
mnemonic: recoveryPhrase
mnemonic: recoveryPhrase.mnemonic
) else {
return .failRecovery("Failed to recover identity")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import Combine
import ObservableStore

// MARK: Action
enum SubtextTextAction: Hashable, CustomLogStringConvertible {
enum SubtextTextAction: Hashable {
case requestFocus(Bool)
case scheduleFocusChange
case focusChange(Bool)
Expand All @@ -44,15 +44,6 @@ enum SubtextTextAction: Hashable, CustomLogStringConvertible {
/// Set selection at the end of the text
case setSelectionAtEnd
case setEditable(Bool)

var logDescription: String {
switch self {
case .setText(_):
return "setText(...)"
default:
return String(describing: self)
}
}
}

// MARK: Model
Expand Down
38 changes: 9 additions & 29 deletions xcode/Subconscious/Shared/Components/Detail/MemoEditorDetail.swift
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,9 @@ struct MemoEditorDetailView: View {
return .handled
})
.onReceive(store.actions) { action in
let message = String.loggable(action)
MemoEditorDetailModel.logger.debug("[action] \(message)")
MemoEditorDetailAction.logger.debug(
"\(String(describing: action))"
)
}
// Filtermap actions to outer actions, and forward them to parent
.onReceive(
Expand Down Expand Up @@ -284,7 +285,12 @@ extension MemoEditorDetailNotification {
}

/// Actions handled by detail's private store.
enum MemoEditorDetailAction: Hashable, CustomLogStringConvertible {
enum MemoEditorDetailAction: Hashable {
static let logger = Logger(
subsystem: Config.default.rdns,
category: "MemoEditorDetailAction"
)

/// Tagging action for detail meta bottom sheet
case metaSheet(MemoEditorDetailMetaSheetAction)

Expand Down Expand Up @@ -449,32 +455,6 @@ enum MemoEditorDetailAction: Hashable, CustomLogStringConvertible {
static func setMetaSheetDefaultAudience(_ audience: Audience) -> Self {
.metaSheet(.setDefaultAudience(audience))
}

// MARK: logDescription
var logDescription: String {
switch self {
case .setLinkSuggestions(let suggestions):
return "setLinkSuggestions(\(suggestions.count) items)"
case .editor(let action):
return "editor(\(String.loggable(action)))"
case let .setDetailLastWriteWins(detail):
return "setDetailLastWriteWins(\(String.loggable(detail)))"
case let .forceSetDetail(detail):
return "forceSetDetail(\(String.loggable(detail)))"
case .save(let entry):
return "save(\(entry?.address.description ?? "nil"))"
case .succeedSave(let entry):
return "succeedSave(\(entry.address))"
case .setDetail(let detail, _):
return "setDetail(\(String.loggable(detail)))"
case let .setEditor(_, saveState, modified):
return "setEditor(text: ..., saveState: \(String(describing: saveState)), modified: \(modified))"
case let .setEditorSelection(range, _):
return "setEditorSelection(range: \(range), text: ...)"
default:
return String(describing: self)
}
}
}

extension MemoEditorDetailAction {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ struct MemoViewerDetailView: View {
store.send(.appear(description))
}
.onReceive(store.actions) { action in
let message = String.loggable(action)
MemoViewerDetailModel.logger.debug("[action] \(message)")
MemoViewerDetailAction.logger.debug(
"\(String(describing: action))"
)
}
.onReceive(
app.actions.compactMap(MemoViewerDetailAction.fromAppAction),
Expand Down Expand Up @@ -235,6 +236,11 @@ struct MemoViewerDetailDescription: Hashable {

// MARK: Actions
enum MemoViewerDetailAction: Hashable {
static let logger = Logger(
subsystem: Config.default.rdns,
category: "MemoViewerDetailAction"
)

case metaSheet(MemoViewerDetailMetaSheetAction)
case appear(_ description: MemoViewerDetailDescription)
case setDetail(_ entry: MemoEntry?)
Expand Down Expand Up @@ -262,17 +268,6 @@ enum MemoViewerDetailAction: Hashable {
}
}

extension MemoViewerDetailAction: CustomLogStringConvertible {
var logDescription: String {
switch self {
case .setDetail:
return "setDetail(...)"
default:
return String(describing: self)
}
}
}

/// React to actions from the root app store
extension MemoViewerDetailAction {
static func fromAppAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ struct UserProfileDetailView: View {
perform: app.send
)
.onReceive(store.actions) { action in
let message = String.loggable(action)
Self.logger.debug("[action] \(message)")
UserProfileDetailAction.logger.debug(
"\(String(describing: action))"
)
}
}
}
Expand Down Expand Up @@ -111,7 +112,12 @@ struct UserProfileDetailDescription: Hashable {
var initialTabIndex: Int = UserProfileDetailModel.recentEntriesTabIndex
}

enum UserProfileDetailAction: CustomLogStringConvertible {
enum UserProfileDetailAction {
static let logger = Logger(
subsystem: Config.default.rdns,
category: "UserProfileDetailAction"
)

case appear(Slashlink, Int, UserProfile?)
case refresh(forceSync: Bool)
case populate(UserProfileContentResponse)
Expand Down Expand Up @@ -160,15 +166,6 @@ enum UserProfileDetailAction: CustomLogStringConvertible {
case succeedEditProfile

case succeedIndexPeer(_ peer: PeerRecord)

var logDescription: String {
switch self {
case .populate(_):
return "populate(...)"
default:
return String(describing: self)
}
}
}

/// React to actions from the root app store
Expand Down
13 changes: 0 additions & 13 deletions xcode/Subconscious/Shared/Components/Feed.swift
Original file line number Diff line number Diff line change
Expand Up @@ -166,19 +166,6 @@ enum FeedAction {
case requestFeedRoot
}

extension FeedAction: CustomLogStringConvertible {
var logDescription: String {
switch self {
case .search(let action):
return "search(\(String.loggable(action)))"
case .setFeed(let items):
return "setFeed(\(items.count) items)"
default:
return String(describing: self)
}
}
}

struct FeedSearchCursor: CursorProtocol {
typealias Model = FeedModel
typealias ViewModel = SearchModel
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ struct RecoveryPhraseView: View {
var body: some View {
VStack(spacing: 0) {
HStack {
Text(state.phrase)
Text(verbatim: state.phrase?.mnemonic ?? "")
.monospaced()
.textSelection(.enabled)
Spacer()
}
.padding()
.background(.background)

ShareLink(item: state.phrase) {
ShareLink(item: state.phrase?.mnemonic ?? "") {
Label(
"Share",
systemImage: "square.and.arrow.up"
Expand All @@ -43,11 +43,11 @@ struct RecoveryPhraseView: View {
}

enum RecoveryPhraseAction: Hashable {
case setPhrase(_ phrase: String)
case setPhrase(_ phrase: RecoveryPhrase?)
}

struct RecoveryPhraseModel: ModelProtocol {
var phrase: String = ""
var phrase: RecoveryPhrase?

static let logger = Logger(
subsystem: Config.default.rdns,
Expand All @@ -74,7 +74,7 @@ struct RecoveryPhraseView_Previews: PreviewProvider {
struct TestView: View {
@StateObject private var store = Store(
state: RecoveryPhraseModel(
phrase: "foo bar baz bing bong boo biz boz bonk bink boop bop beep bleep bloop blorp blonk blink blip blop boom"
phrase: RecoveryPhrase("foo bar baz bing bong boo biz boz bonk bink boop bop beep bleep bloop blorp blonk blink blip blop boom bim blap blap")
),
environment: RecoveryPhraseEnvironment()
)
Expand Down
21 changes: 6 additions & 15 deletions xcode/Subconscious/Shared/Components/Notebook/Notebook.swift
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ struct NotebookView: View {
perform: app.send
)
.onReceive(store.actions) { action in
let message = String.loggable(action)
NotebookModel.logger.debug("[action] \(message)")
NotebookAction.logger.debug("\(String(describing: action))")
}
}
}
Expand All @@ -88,6 +87,11 @@ struct NotebookView: View {
/// For action naming convention, see
/// https://github.com/gordonbrander/subconscious/wiki/action-naming-convention
enum NotebookAction {
static let logger = Logger(
subsystem: Config.default.rdns,
category: "NotebookAction"
)

/// Tagged action for search HUD
case search(SearchAction)
/// Tagged action for detail stack
Expand Down Expand Up @@ -191,19 +195,6 @@ enum NotebookAction {
}
}

extension NotebookAction: CustomLogStringConvertible {
var logDescription: String {
switch self {
case .search(let action):
return "search(\(String.loggable(action)))"
case .setRecent(let items):
return "setRecent(\(items.count) items)"
default:
return String(describing: self)
}
}
}

// MARK: Cursors and tagging functions

struct NotebookDetailStackCursor: CursorProtocol {
Expand Down
Loading

0 comments on commit ae98fdc

Please sign in to comment.