Skip to content

Commit 07ff075

Browse files
committed
fix: address review issues in SSH profile integration
- Fix testConnection leaking Keychain secrets and overriding profile passwords - Clean up all temporary Keychain entries (password, passphrase, TOTP) after test - Skip inline SSH validation when a profile is selected (isValid) - Move profile list loading from section onAppear to loadConnectionData - Separate onSave/onDelete callbacks in SSHProfileEditorView - Pass inline secrets to Save as Profile flow so TOTP secret is preserved
1 parent 021cc7e commit 07ff075

2 files changed

Lines changed: 51 additions & 37 deletions

File tree

TablePro/Views/Connection/ConnectionFormView.swift

Lines changed: 42 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -507,24 +507,29 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
507507
}
508508
.controlSize(.small)
509509
}
510-
.onAppear {
511-
sshProfiles = SSHProfileStorage.shared.loadProfiles()
512-
}
513510
.sheet(isPresented: $showingCreateProfile) {
514-
SSHProfileEditorView(existingProfile: nil) { _ in
511+
SSHProfileEditorView(existingProfile: nil, onSave: { _ in
515512
reloadProfiles()
516-
}
513+
})
517514
}
518515
.sheet(item: $editingProfile) { profile in
519-
SSHProfileEditorView(existingProfile: profile) { _ in
516+
SSHProfileEditorView(existingProfile: profile, onSave: { _ in
520517
reloadProfiles()
521-
}
518+
}, onDelete: {
519+
reloadProfiles()
520+
})
522521
}
523522
.sheet(isPresented: $showingSaveAsProfile) {
524-
SSHProfileEditorView(existingProfile: buildProfileFromInlineConfig()) { savedProfile in
525-
sshProfileId = savedProfile.id
526-
reloadProfiles()
527-
}
523+
SSHProfileEditorView(
524+
existingProfile: buildProfileFromInlineConfig(),
525+
initialPassword: sshPassword,
526+
initialKeyPassphrase: keyPassphrase,
527+
initialTOTPSecret: totpSecret,
528+
onSave: { savedProfile in
529+
sshProfileId = savedProfile.id
530+
reloadProfiles()
531+
}
532+
)
528533
}
529534
}
530535

@@ -1002,7 +1007,7 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
10021007
.allSatisfy { !(additionalFieldValues[$0.id] ?? "").isEmpty }
10031008
basicValid = basicValid && hasRequiredFields && !password.isEmpty
10041009
}
1005-
if sshEnabled {
1010+
if sshEnabled && sshProfileId == nil {
10061011
let sshPortValid = sshPort.isEmpty || (Int(sshPort).map { (1...65_535).contains($0) } ?? false)
10071012
let sshValid = !sshHost.isEmpty && !sshUsername.isEmpty && sshPortValid
10081013
let authValid =
@@ -1028,6 +1033,7 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
10281033
}
10291034

10301035
private func loadConnectionData() {
1036+
sshProfiles = SSHProfileStorage.shared.loadProfiles()
10311037
// If editing, load from storage
10321038
if let id = connectionId,
10331039
let existing = storage.loadConnections().first(where: { $0.id == id })
@@ -1345,22 +1351,25 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
13451351
if !password.isEmpty {
13461352
ConnectionStorage.shared.savePassword(password, for: testConn.id)
13471353
}
1348-
if sshEnabled
1349-
&& (sshAuthMethod == .password || sshAuthMethod == .keyboardInteractive)
1350-
&& !sshPassword.isEmpty
1351-
{
1352-
ConnectionStorage.shared.saveSSHPassword(sshPassword, for: testConn.id)
1353-
}
1354-
if sshEnabled && sshAuthMethod == .privateKey && !keyPassphrase.isEmpty {
1355-
ConnectionStorage.shared.saveKeyPassphrase(keyPassphrase, for: testConn.id)
1356-
}
1357-
if sshEnabled && totpMode == .autoGenerate && !totpSecret.isEmpty {
1358-
ConnectionStorage.shared.saveTOTPSecret(totpSecret, for: testConn.id)
1354+
// Only write inline SSH secrets when not using a profile
1355+
if sshEnabled && sshProfileId == nil {
1356+
if (sshAuthMethod == .password || sshAuthMethod == .keyboardInteractive)
1357+
&& !sshPassword.isEmpty
1358+
{
1359+
ConnectionStorage.shared.saveSSHPassword(sshPassword, for: testConn.id)
1360+
}
1361+
if sshAuthMethod == .privateKey && !keyPassphrase.isEmpty {
1362+
ConnectionStorage.shared.saveKeyPassphrase(keyPassphrase, for: testConn.id)
1363+
}
1364+
if totpMode == .autoGenerate && !totpSecret.isEmpty {
1365+
ConnectionStorage.shared.saveTOTPSecret(totpSecret, for: testConn.id)
1366+
}
13591367
}
13601368

1369+
let sshPasswordForTest = sshProfileId == nil ? sshPassword : nil
13611370
let success = try await DatabaseManager.shared.testConnection(
1362-
testConn, sshPassword: sshPassword)
1363-
ConnectionStorage.shared.deleteTOTPSecret(for: testConn.id)
1371+
testConn, sshPassword: sshPasswordForTest)
1372+
cleanupTestSecrets(for: testConn.id)
13641373
await MainActor.run {
13651374
isTesting = false
13661375
if success {
@@ -1374,7 +1383,7 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
13741383
}
13751384
}
13761385
} catch {
1377-
ConnectionStorage.shared.deleteTOTPSecret(for: testConn.id)
1386+
cleanupTestSecrets(for: testConn.id)
13781387
await MainActor.run {
13791388
isTesting = false
13801389
testSucceeded = false
@@ -1405,6 +1414,13 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
14051414
}
14061415
}
14071416

1417+
private func cleanupTestSecrets(for testId: UUID) {
1418+
ConnectionStorage.shared.deletePassword(for: testId)
1419+
ConnectionStorage.shared.deleteSSHPassword(for: testId)
1420+
ConnectionStorage.shared.deleteKeyPassphrase(for: testId)
1421+
ConnectionStorage.shared.deleteTOTPSecret(for: testId)
1422+
}
1423+
14081424
private func browseForPrivateKey() {
14091425
let panel = NSOpenPanel()
14101426
panel.allowsMultipleSelection = false

TablePro/Views/Connection/SSHProfileEditorView.swift

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,11 @@ struct SSHProfileEditorView: View {
99
@Environment(\.dismiss) private var dismiss
1010

1111
let existingProfile: SSHProfile?
12+
var initialPassword: String?
13+
var initialKeyPassphrase: String?
14+
var initialTOTPSecret: String?
1215
var onSave: ((SSHProfile) -> Void)?
16+
var onDelete: (() -> Void)?
1317

1418
// Profile identity
1519
@State private var profileName: String = ""
@@ -319,16 +323,10 @@ struct SSHProfileEditorView: View {
319323
customAgentSocketPath = profile.agentSocketPath
320324
}
321325

322-
// Load secrets from Keychain
323-
if let pwd = SSHProfileStorage.shared.loadSSHPassword(for: profile.id) {
324-
sshPassword = pwd
325-
}
326-
if let phrase = SSHProfileStorage.shared.loadKeyPassphrase(for: profile.id) {
327-
keyPassphrase = phrase
328-
}
329-
if let secret = SSHProfileStorage.shared.loadTOTPSecret(for: profile.id) {
330-
totpSecret = secret
331-
}
326+
// Load secrets from Keychain, falling back to initial values (e.g. from "Save as Profile")
327+
sshPassword = SSHProfileStorage.shared.loadSSHPassword(for: profile.id) ?? initialPassword ?? ""
328+
keyPassphrase = SSHProfileStorage.shared.loadKeyPassphrase(for: profile.id) ?? initialKeyPassphrase ?? ""
329+
totpSecret = SSHProfileStorage.shared.loadTOTPSecret(for: profile.id) ?? initialTOTPSecret ?? ""
332330
}
333331

334332
private func saveProfile() {
@@ -383,7 +381,7 @@ struct SSHProfileEditorView: View {
383381
private func deleteProfile() {
384382
guard let profile = existingProfile else { return }
385383
SSHProfileStorage.shared.deleteProfile(profile)
386-
onSave?(profile)
384+
onDelete?()
387385
dismiss()
388386
}
389387

0 commit comments

Comments
 (0)