feat(connections): resolve connection passwords from file, env, or command (#1254)#1478
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a1e6c6031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let passwordSource = connection.passwordSource { | ||
| return try await PasswordSourceResolver.resolve(passwordSource) |
There was a problem hiding this comment.
When a connection uses SSH or Cloudflare tunneling, DatabaseManager.tunneledConnection(from:localPort:) rebuilds a DatabaseConnection for 127.0.0.1 before createDriver calls this password lookup, but that rebuilt connection does not carry passwordSource. In that environment this new branch is skipped and the driver falls back to Keychain/empty password, so a connection that works directly with passwordSource fails as soon as a tunnel is enabled; propagate the source onto the tunneled connection before resolving the password.
Useful? React with 👍 / 👎.
| localOnly: Bool = false, | ||
| isSample: Bool = false, | ||
| isFavorite: Bool = false, | ||
| passwordSource: PasswordSource? = nil, |
There was a problem hiding this comment.
Because this new initializer argument defaults to nil, existing call sites that reconstruct a connection from form state can now silently drop a manually provisioned passwordSource. For example, editing and saving an existing connection in ConnectionFormCoordinator.saveConnection replaces savedConnections[index] with a freshly constructed DatabaseConnection and there is no form field for passwordSource, so the next connect falls back to Keychain/empty password even though the source was present in connections.json; preserve originalConnection?.passwordSource when saving existing connections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2013836031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // passwordSource is also NOT synced: its file path, env var, or command | ||
| // is device-local and may not exist or resolve on another Mac. |
There was a problem hiding this comment.
When a Mac already has a connection with a local passwordSource and then receives a CloudKit update for the same connection, this omission means SyncRecordMapper.toConnection creates a remote connection with passwordSource == nil; SyncCoordinator.applyRemoteConnection then replaces the local entry while only copying localOnly. In that iCloud-sync path, any edit from another Mac strips the device-local password source from connections.json, so the next connect falls back to Keychain/empty instead of the configured file/env/command source. Please merge the existing local passwordSource back into the remote record, similar to localOnly, when applying remote updates.
Useful? React with 👍 / 👎.
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f43176c8b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let passwordSource = connection.passwordSource { | ||
| return try await PasswordSourceResolver.resolve(passwordSource) |
There was a problem hiding this comment.
This resolves passwordSource only for driver creation, but the PostgreSQL backup/restore path bypasses DatabaseDriverFactory: PostgresDumpService.start still sets PGPASSWORD from the Keychain or session.cachedPassword, and cachedPassword is only populated for prompted passwords. A PostgreSQL/Redshift connection that successfully connects via a file/env/command password will therefore run pg_dump/pg_restore with --no-password and no password, causing backup/restore to fail. Please resolve the same source there or cache the resolved password for these subprocesses.
Useful? React with 👍 / 👎.
| guard let contents = try? String(contentsOfFile: expandedPath, encoding: .utf8) else { | ||
| throw ResolutionError.fileUnreadable(path: expandedPath) | ||
| } | ||
| return try nonEmpty(contents.trimmingCharacters(in: .whitespacesAndNewlines)) |
There was a problem hiding this comment.
For file-based password sources, trimming .whitespacesAndNewlines changes the actual secret when a valid database password starts or ends with a space; the docs only promise to trim a trailing newline. In that case the app will connect with a different password than the file contains, so this should only remove line terminators (or at most one final newline) rather than all surrounding whitespace.
Useful? React with 👍 / 👎.
…form save, and sync (#1254)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb2de910f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| startupCommands: advanced.startupCommands.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty | ||
| ? nil : advanced.startupCommands, | ||
| localOnly: advanced.localOnly, | ||
| passwordSource: originalConnection?.passwordSource, |
There was a problem hiding this comment.
When editing a connection that already has passwordSource, entering a new password or enabling prompt-for-password still saves the old source here. Since DatabaseDriverFactory.resolvePassword checks connection.passwordSource before falling back to the Keychain, the password saved at lines 295-299 is ignored and prompt mode is bypassed for that connection. Preserve the source only when no replacement password/prompt choice was made, as the test path already does.
Useful? React with 👍 / 👎.
| if process.isRunning { | ||
| didTimeout.set() | ||
| process.terminate() | ||
| } |
There was a problem hiding this comment.
For a password command that traps or ignores SIGTERM, this timeout path sets the flag and calls terminate(), but waitUntilExit() below can still block forever because the process never exits. Since passwordSource.command accepts arbitrary shell snippets and promises a 30-second timeout, a hung or TERM-resistant helper can still hang connection creation indefinitely; kill the process group or escalate to SIGKILL after a grace period.
Useful? React with 👍 / 👎.
1 participant
Closes #1254.
Problem
Connection passwords live only in the Keychain. A script that provisions
connections.json(for example, one Docker database per git worktree) can write every field except the password, so the first connect needs a manual password entry.Solution
A connection can declare a
passwordSourcethat is resolved at connect time, instead of reading the Keychain:{ "passwordSource": { "kind": "file", "path": "~/.config/tablepro/secrets/feature-x.pw" } } { "passwordSource": { "kind": "env", "variable": "STAGING_DB_PASSWORD" } } { "passwordSource": { "kind": "command", "shell": "op read op://vault/feature-x/password" } }file: reads the file, trims a trailing newline, warns (does not refuse) on loose permissions.env: reads the named variable. The error names the macOS gotcha that a Dock-launched app does not inherit shell exports.command: runs through/bin/bash, reads stdout, trims a trailing newline. Non-zero exit fails the connection, stderr is surfaced in the error, 30 second timeout, no stdin. Works withop,vault,pass,sops.When
passwordSourceis set it replaces the Keychain lookup. On failure the connection reports the error rather than silently falling back.Design notes
PasswordSourceenum (model) with custom Codable that produces the exactkind-tagged JSON. Stored as a readable nested object so a script can write it by hand.PasswordSourceResolver, modeled onPreConnectHookRunner: off the main thread, both pipes drained to avoid the 64 KiB deadlock, PATH augmented soop/vaultresolve under launchd.DatabaseDriverFactory.resolvePasswordchain, between the password prompt and~/.pgpass.SyncRecordMapperthe same way the SSH and Cloudflare tunnel modes are.$VARsubstitution and~/.pgpassalready resolve at connect time.connections.jsonfile, not the.tableproshare/import format.commandand arbitrary-pathfilemust be gated off.Tests
PasswordSourceResolverTests: each source plus error paths (missing file, empty value, loose permissions, trailing newline, non-zero exit, stderr capture, NUL rejection, timeout).PasswordSourceCodableTests: the JSON shape contract and round-trip throughDatabaseConnection.Docs
New "Password Sources" section in
docs/features/connection-sharing.mdx.