Skip to content

fix: connection form data loss, helper binary integrity, and missing license notices - #2240

Merged
datlechin merged 10 commits into
mainfrom
fix/p0-connection-and-binary-integrity
Aug 19, 2026
Merged

fix: connection form data loss, helper binary integrity, and missing license notices#2240
datlechin merged 10 commits into
mainfrom
fix/p0-connection-and-binary-integrity

Conversation

@datlechin

@datlechin datlechin commented Aug 19, 2026

Copy link
Copy Markdown
Member

Four confirmed defects and one unmet legal obligation. Each is its own commit with its own tests.

What is fixed

Editing a connection reset four of its stored properties (9b85b49bd)

saveConnection rebuilt DatabaseConnection from a 26-argument initializer, so every property added to the model since was silently defaulted. isFavorite, isSample, sortOrder and aiAlwaysAllowedTools were dropped, and savedConnections[index] = connectionToSave wrote the whole object back. Starring a connection and then renaming it lost the star. sortOrder drives the welcome list order and the favorites sidebar; aiAlwaysAllowedTools drives AI tool approval.

Adding the four missing arguments would break again on the next property. The form now expresses what it owns as ConnectionFormEdits and applies it onto the connection being edited, so anything it does not own is carried by construction. The test-connection path uses the same value, so a test now connects with exactly what a save would write.

The same rule covers additionalFields, which had a second instance of the bug: the three panes rebuild their values from the plugin-declared field list only, so an app-invented key was dropped on load and absent on save. ownedAdditionalFieldIDs covers the declared set for the current and previous database type plus the three app-managed keys, so a key nobody declares survives while a type change still clears the old type's fields.

ConnectionFormEditsCoverageTests is the guard against recurrence. It reflects over DatabaseConnection and fails when a stored property is neither classified as form-written nor as carried over, so adding a property forces the decision rather than silently resetting it.

A supervised process dropped its last line, and could strand a waiter (771669af8)

terminationContinuation was a single optional, overwritten on each await, so a second concurrent waiter never resumed. Latent today, since only startDeathWatch awaits it.

Writing the test for that found a live one: finish() cleared the pipe's readability handler before the last chunk arrived, so the final stderr line was lost whenever a helper exited quickly. That line is usually the reason it failed, so a Cloud SQL Auth Proxy or Cloudflare tunnel that failed to start could report an error with nothing in it. finish() now drains with readToEnd() and is idempotent.

A pinned helper binary was never re-checked (cadc76b8c)

ensureBinary() returned as soon as an executable file existed at the path, without comparing installedVersion() right above it and without re-verifying the checksum. Two consequences: raising pinnedVersion downloaded nothing on a Mac that already had the old binary, and the SHA-256 was verified once at download and never again, while the file sits in a user-writable directory and is Signature=adhoc, TeamIdentifier=not set, rejected by spctl -a -t exec. The pinned digest is the only thing attesting to what runs.

CloudSQLProxyBinaryManager.installedBinaryIsCurrent() now requires all three, on every call and again after a download. CopilotBinaryManager has no pin to check against, since npm serves whatever latest resolves to, so it records the digest at install and re-verifies against that. The streaming digest and the quarantine strip moved into DownloadedBinary, which also absorbed the third verbatim copy of the xattr helper in PluginInstaller.

The plugin signature check claimed something it cannot do (b714190ed)

The doc comment said SecStaticCodeCheckValidity performs a notarization check by default. It does not, and no in-process API can: SecRequirementCreateWithString("notarized") returns errSecCSReqFailed for a never-notarized bundle and a revoked one alike, and SecAssessmentTicketLookup is not public SDK. The requirement in use contains no notarization term.

The same claim was in the [Unreleased] CHANGELOG entry and in docs/features/plugins.mdx, promising users a review that does not happen. Both corrected. No behaviour change; the check itself was already right.

Third-party license notices were missing entirely (c023b61a7)

TablePro statically links and redistributes copyleft libraries with no notice surface anywhere. MariaDB Connector/C 3.4.4 is LGPL 2.1 and FreeTDS 1.4.22 is LGPL 2.0 and modified by scripts/patches/freetds/, both linked into shipped plugin bundles. Section 6 of each requires the distributed work to say the library is used, that its use is covered by that license, and to include a copy of it. MIT, BSD, ISC and the PostgreSQL license all require their body to travel with the binary too.

Help › Acknowledgements now lists 38 components with version, SPDX id, copyright lines and full license text (36 texts, 448K). The obligation is per component, not per license: Apache 2.0 section 4(d) requires nine packages' NOTICE attribution lines, and three C libraries carry a second required file, mongo-c-driver's THIRD_PARTY_NOTICES among them, which is what discharges the projects vendored inside it. Each component's text file carries everything that applies to it.

ThirdPartyLicenseInventoryTests is the drift gate. It reads the pins out of scripts/build-*.sh and out of the three Package.resolved files and fails when an entry disagrees, when a built library has no entry at all, or when the patched flag disagrees with scripts/patches/. It does not auto-correct the version, because a bump can change a license (OpenSSL at 3.0, Redis at 7.4) and that needs a person to re-read it.

Two libraries have no license, and this PR does not solve that

CodeEditLanguages (vendored under LocalPackages/) and CodeEditSymbols 0.2.3 (an SPM pin) both ship inside the app and neither project publishes a license file. This was checked against the GitHub API repeatedly; it returns license: null for both. They cannot be attributed, so they are listed under "License Not Yet Confirmed" rather than given a guessed SPDX id.

Listing them is the honest state, not a fix. Resolving it means asking CodeEdit to add a LICENSE, getting a written grant, or replacing the packages. Worth deciding separately.

Verification

  • verify.sh build: PASS
  • verify.sh test across all touched suites: 62 executed, 62 passed, ** TEST SUCCEEDED **
  • Bundled resources confirmed in the built app: Contents/Resources/ThirdPartyLicenses with 36 texts
  • verify.sh lint TablePro TableProTests: two failures, both pre-existing on origin/main and outside this diff (ExecutionAuditLog.swift:40 storage-environment violation; 5 stale doc references in .claude/skills/fix-issue/SKILL.md). Not fixed here to keep the diff to its stated scope.

project.yml gains a folder reference for the license resources. A plain source glob flattens directories into Resources/, which would leave the inventory unable to find its texts/ directory.

@mintlify

mintlify Bot commented Aug 19, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 19, 2026, 11:03 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin

Copy link
Copy Markdown
Member Author

Security review follow-up

Ran a security review over the branch. Two findings, both fixed in d058ff0c9.

1. The checksum check was not on the path that launches the proxy. The new installedBinaryIsCurrent() was wired into ensureBinary(), but the only caller of ensureBinary() was the form's manual Download button. The connect path went createTunnelresolveBinaryPathcachedBinaryPath, which was nothing but isExecutableFile. So the pinned digest was never read before exec, and the CHANGELOG line claiming otherwise was false for the proxy.

That matters because of what the proxy is handed on argv: --credentials-file pointing at a file TablePro just wrote containing the user's GCP service account private key. Anything running as the user could replace the binary in Application Support and receive that key on the next connect.

Root cause was that two ways existed to obtain the managed path, one verified and one not, and the launch path picked the unverified one. cachedBinaryPath is gone. isInstalled replaces it and answers only the form's "Downloaded" question, with a comment saying it must never be used to obtain a path to execute. resolveBinaryPath now goes through ensureBinary(), so a stale or altered copy is reinstalled there instead of run.

CloudSQLProxyManager gained injection for the binary manager and the PATH lookup, so this is covered by two tests rather than by inspection: a tampered binary whose refetch also fails throws and never reaches runnerFactory, and a binary matching its pin launches.

2. The Copilot digest check overclaimed. sha256.txt sits in the same user-writable directory as the binary it attests to, so anything able to swap the binary can rewrite the digest in the same operation. The check is real but it catches truncated downloads and failed extractions, not a deliberate swap. npm serves that package from a floating tag, so there is no version to pin and no stronger attestation available without a network call per launch.

Rather than ship a control that reads stronger than it is, the comment and the CHANGELOG now say what it actually does, and the comment points at CloudSQLProxyBinaryManager as the case where the pin does carry weight. Fixing an overclaiming comment in this same PR and then adding a new one would have been the wrong trade.

Verification after the change: build PASS, 70 executed / 70 passed, ** TEST SUCCEEDED **, exit 0.

Findings the review raised and dismissed with reasons: Yams decoding into a fixed Codable struct from the signed bundle, the unsanitised textFile path (source is the signed bundle, sink is a Text view), the secure-field merge under the new ownedAdditionalFieldIDs semantics (traced; no plaintext path), passwordSource in the test path (byte-identical expression), and carried-over aiAlwaysAllowedTools (destructive tools return .pending before the set is consulted).

Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
@datlechin
datlechin merged commit 3519818 into main Aug 19, 2026
5 checks passed
@datlechin
datlechin deleted the fix/p0-connection-and-binary-integrity branch August 19, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant