fix(plugins): stop driver error text reaching the system log - #3081
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
#2960 stopped the app publishing error text to the system log and added the
public_error_text_in_logSwiftLint rule to keep it that way. SwiftLint'sincluded:namesTableProandPackagesonly, so the rule never readPlugins/, and 15 plugin log lines kept writingerror.localizedDescriptionatprivacy: .public. The test #2960 added scannedTablePro/alone, and said why: a plugin could not reachpublicLogShape, which is internal to the app.A driver error's text is where a server puts the data it is complaining about. PostgreSQL's unique-violation message carries the offending value (
Key (email)=(a@b.com)), MySQL's duplicate-key message carries the row, and a file error carries the person's paths. At.publicthat text reaches every process that can read the log and every sysdiagnose a user attaches to a bug report. The 15 sites were in BigQuery, CSV Inspector, SQL Server, MongoDB, MySQL, PostgreSQL and Redshift, on paths such as the PostgreSQL create-database sheet readingtemplate1defaults andpg_collation, the Redshift external schema reads, and the MongoDB connect.The same leak was spelled other ways the rule does not match, and those are fixed here too:
String(describing: error)at.publicin the Oracle plugin, the app'sServerOutputCapture, and four places inTableProOracleCore(connect failure, TLS failure, statement failure, transport reset), one of which also published a column name.RedisClusterChannelalready did.mysql_error()text when a server refuses a UTF-8 session, a PostgreSQL session setup failure'sPQresultErrorMessage, a MongoDB script exception, a Snowflake HTTP error body, and up to 300 characters of a raw Elasticsearch response.error.localizedDescriptionthrough amessagevariable (the sync lines also published record names that embed table names and SQLite paths), a failed AppleScript command published the driver error througherrorDescription ?? "", a document open failure publishederror?.localizedDescription ?? "nil", a failed privileged command published its stderr, a plugin bundle load failure published theNSErrordebug description, and the MCP bridge logger published every message, error text included.Fix
LogRedactionandPubliclyLoggableErrormove out of the app into a new dependency-free SwiftPM target,TableProLogRedaction, besideTableProNumberFormattingandTableProDocumentPath, so the app and the plugins share one implementation. A plugin publishesLogRedaction.publicDescription(of: error). The app keepsError.publicLogShapeas a one-line forwarder, so the 113 app files that use it need no new import. The target is linked into the app, themcp-serverhelper (which compilesLogRedaction.swifttoo), the test bundle, and the seven plugins that call it.TableProPluginKit: every public symbol there is permanent resilient ABI, for a helper that is not part of the plugin contract. Why not a publicError.publicLogShapein the new module: measured withswiftc, an app-internal extension member shadows a same-named public one inside the app, but a third module importing both (the test target) fails withambiguous use of 'publicLogShape'..private, so a developer with a logging profile keeps the detail.NSErrordomain is published only when it is identifier-shaped (ASCII letters, digits,.,_,-, at most 64 bytes), as every declared domain is; the old code published any domain that did not end in the type name, soNSError(domain: serverText, code: 1)put the server text in the public line. And a payload-free enum case whose type definesdescriptionordebugDescriptionpublishes the type name, because"\(error)"would print that text..private(mask: .hash), the way fix(diagnostics): stop publishing user data to the system log #2960 treated them in the app.String(describing: <any error name>), a bare error interpolation,.message,.description,.debugDescription,errorDescriptionandfailureReason, and alocalizedDescriptionwith.prefix(n)or a?? "default", at.public.hasErrorandisErrorare not matched.Closing the gap
Measured on
mainwith SwiftLint 0.65.1:swiftlint lint --strictover the 536 Swift files underPlugins/outsideTableProPluginKit, passed as file paths, reports 175 violations across 23 plugins, 15 of them this rule. The rest are 53identifier_name, 51number_separator, 13inclusive_language(Redis protocol terms) and 43 others. That is too much unrelated churn for this PR, andswiftlint lint --strictgates the release job, so addingPluginstoincluded:has to land with every one of them fixed. SwiftLint also runs in CI only on a release tag, so the rule was not checked on any pull request, for the app either.So this adds
scripts/ci/check-log-privacy.pyto the Repo Hygiene job, which runs on every pull request that touches Swift. It reads the rule's regex out of.swiftlint.yml, so the rule and the check cannot drift, and applies it to every Swift file underTablePro/,Packages/andPlugins/, across line breaks the way SwiftLint matches. Onmainit reports 19 sites; on this branch none.scripts/ci/test_check_log_privacy.pypins 11 spellings the rule must reject and 9 it must accept, and runs in the same job. The job now also runs when.swiftlint.ymlchanges. The app suite's old literal-substring scan is removed, since it had drifted from the rule and the check replaces it.A regex cannot follow text through a variable, which is why the
message,refusal,detail,bodyTextand bridge-logger sites above were found by reading, not by the check.Not in this PR
TableProMobile/is outside SwiftLint'sincluded:too, and has 29 log lines that publisherror.localizedDescription, including a failed database switch and failed table, database and schema loads inConnectionCoordinator. The same fix applies, but it needs an iOS build to verify, and the check leaves the directory out until then.fetchTriggersandfetchForeignKeys, SnowflakefetchColumns, Elasticsearch browse queries). fix(diagnostics): stop publishing user data to the system log #2960 moved names to.private(mask: .hash)in the app; the plugins need the same pass.sorted_imports,closure_parameter_position,number_separator,vertical_whitespace_closing_braces,inclusive_language), all present onmainat the same lines.Verification
Rebased on
mainat 5cff78b.verify.sh testoverLogRedactionTests,ServerOutputCaptureTests,PluginBundleLoaderDescribeLoadFailureTestsandCSVDocumentUndoTests, PASS, 16 cases executed, 0 failed. This build includes themcp-serverhelper, which now linksTableProLogRedaction.swift test --package-path Packages/TableProCore --filter LogRedactionShapeTests, 8 tests (one parameterized over 6 domains), all passed. The opt-in case returns text no fallback could produce, so removing thePubliclyLoggableErrorbranch turns it red.verify.sh plugins(AllPlugins), PASS.check-log-privacy.pyexits 0 here,test_check_log_privacy.pypasses its 4 tests,actionlintis clean. The widened rule was also run through SwiftLint itself on a fixture file and overTableProandPackages: it flags every rejected spelling and nothing in the tree.swiftlint lint --strictover the 32 changed Swift files as file paths, with this branch's.swiftlint.yml: only the 6 pre-existing findings above.reviewandadversarial-reviewread an earlier version that putLogRedactioninTableProPluginKit. Their findings are addressed: the gate missederror.messageand multi-line interpolation,mcp-servercompilesLogRedaction.swiftand needed a declared dependency, dynamicNSErrordomains were published, and more sites leaked. Codex then ran out of quota, so twoSkill(code-review)passes (Claude) read the final design. Fixed from them: record names in sync logs, the Elasticsearch filter and body logs, the AppleScript, document-open and MCP bridge leaks, private descriptions beside every shape, a name-agnostic regex with fixture tests, a tautological test and the drifted app scan removed, comments removed from the new file.Error.publicLogShapein the module (ambiguous in the test target, measured); callingLogRedactionfromTableProOracleCore(a separate package with no dependency onTableProCore, so it publishes the type or the TLS failure kind instead); hiding every domain outside a fixed list (identifier-shaped domains such asCKErrorDomainstay useful and cannot carry server text).Found while investigating #3048 (see #3060).