Security hardening across the macOS and iOS apps#1640
Conversation
…words out of the keychain
…nt NoSQL injection
![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: e57c1cbb14
ℹ️ 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".
| guard let connectionId else { | ||
| return decideTokenTier(token: token, tool: tool) | ||
| return .allowed |
There was a problem hiding this comment.
When a bearer token is limited to specific connection IDs, returning .allowed for any tool call without a connection_id lets that token bypass its connectionAccess: list_connections has no arguments and returns all non-blocked saved connections, and search_query_history can omit connection_id to search across all allowed-by-external-access history. In those contexts a scoped token can enumerate other connections and read their query history despite the new per-connection scope check.
Useful? React with 👍 / 👎.
…erialization values
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bd1278cb6
ℹ️ 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 MCPToolName.requiresFullAccess.contains(tool) { | ||
| if externalAccess != .readWrite { | ||
| return String(localized: "Connection is read only for external clients") | ||
| } |
There was a problem hiding this comment.
For confirm_destructive_operation, this branch now returns success as long as the connection's external access is read/write, but it no longer verifies that the principal came from a fullAccess token. Because readWrite tokens map to .toolsWrite and the confirm tool only requires .toolsWrite, any read/write token can execute DROP/TRUNCATE/ALTER...DROP after supplying the phrase; before this change decideTokenTier required .fullAccess for this tool.
Useful? React with 👍 / 👎.
| if !principal.connectionAccess.allows(connectionId) { | ||
| return .denied(reason: String(localized: "Token does not have access to this connection")) |
There was a problem hiding this comment.
This connection allow-list is only checked from ToolsCallHandler; resources/read and resources/list never call MCPAuthPolicy before serving tablepro://connections, .../schema, or .../history via ResourcesReadHandler.fetchPayload. A bearer token limited to one connection but granted resources:read can therefore enumerate other saved/connected connections and read their schema or query history by URI, bypassing the new connectionAccess restriction.
Useful? React with 👍 / 👎.
1 participant
Security audit of the macOS (
TablePro/) and iOS (TableProMobile/) apps, with the fixes it surfaced. One concern per commit.Fixes
Critical / High
preConnectScriptthat runs/bin/bash -con connect, with the import sheet never showing it. Import now strips behaviour-bearing keys via one denylist, applied at both the deeplink and shared-blob boundaries. (593b093)$whereinjection. All five now route through one escaping helper. (8314069)sslConfigurationmode now flows into MySQL, PostgreSQL, and Redis. (b6b3f65)Medium / Low
DEVELOPMENT_TEAMfromSecrets.xcconfiginstead of a hardcoded ID. (c82b81e)download-libs.shverifies downloaded static libs against the git-committed checksum baseline, not the archive's own copy. (217d885)env:to block shell script injection. (9eb5897)Validation
swiftlint --strict: clean on all changed files.Secrets.xcconfigand downloadedLibs/ios/*.xcframework, so it was reviewed statically here and builds on a configured machine.Deferred on purpose
Each of these would break a real path, so they are documented rather than changed:
NSAllowsArbitraryLoads: ClickHouse and Etcd connect to user-typed plain-HTTP hosts over URLSession.disable-library-validation: registry plugins are signed by one team while contributors sign with their own.Tests
Also fixed 7
ConnectionSharingTeststhat fail onmain. They asserted camelCase serialization tokens (privateKey,verifyCa,red), but the app's shipped wire format (storage, iCloud sync, exports, deeplinks) uses the enum display rawValues (Private Key,Verify CA,Red). The source is the real format and changing it would break decoding of existing saved connections, so the test expectations were corrected to match.ConnectionSharingTestsnow passes 54/0.