Repository navigation
fix: detect wrong-kind ClickHouse objects, restrict Tinybird stale-deployment cleanup - #35
Merged
Merged
Conversation
…ployment cleanup Addresses two DeepSec HIGH_BUG findings: `other-schema-diff-kind-confusion` — `computeSchemaDiff` previously fell through to the materialized-view presence check after looking up `actual.get(table.name)` without verifying that the actual object's kind matched the desired kind. A customer with a regular table named the same as a Maple MV would be reported as `up_to_date`, the apply path would skip creating the MV, and downstream aggregate / derived tables would never get populated while sync still reported success. Add a new `wrong_kind` status to `TableDiffEntry` (and the matching `ClickHouseTableDiffEntry` schema in the domain HTTP types). Emit it when `actualTable.kind !== table.kind`. The apply path in `OrgClickHouseSettingsService.applySchema` skips these with a clear "resolve manually" reason rather than auto-remediating, since dropping the customer's existing object is destructive. `other-destructive-cleanup` — `cleanupStaleDeployments` filtered with `!d.live && d.status !== "live"`, which matches in-flight states like `deploying` and `data_ready` and any unrecognised status string from a future Tinybird release. Combined with `Effect.ignore` on every DELETE, this could silently delete an active rollout. Restrict the filter to known terminal-failed states (`failed`, `error`) and require `live !== true` as defense in depth. Replace the silent `Effect.ignore` with `Effect.tapError(Effect.logWarning)` so delete failures surface in logs rather than disappearing. Out of scope (deferred to a follow-up): - Two-phase organization deletion. The current `OrganizationService. deleteOrganization` purges local org-scoped tables before calling Clerk and is not transactional; if Clerk fails after the purge, data is gone but the upstream org persists. The fix requires a new `deletion_pending` column on the orgs table, a DB migration, and a retryable cleanup job — significant scope vs the rest of this security batch. Tests: - 2 new cases in `packages/domain/src/clickhouse/diff.test.ts` covering wrong_kind in both directions (MV where table found, and table where MV found). - All 234 apps/api tests continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PR-#6 schema-diff change added a wrong_kind variant to TableDiffEntry but the settings page consumed the union without handling it, causing a typecheck failure (counts indexer + columnDrifts narrowing). Add `wrong_kind: 0` to the counts initializer and a render branch that shows "Wrong kind: expected MV, found table — resolve manually" so an operator sees what the apply path skipped. Found by running `bun turbo typecheck` across the full monorepo as part of post-merge verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This branch was previously deployed
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
1 participant
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.
Summary
Addresses two DeepSec HIGH_BUG findings.
other-schema-diff-kind-confusioncomputeSchemaDiffpreviously fell through to the materialized-view presence check after looking upactual.get(table.name)without verifying that the actual object's kind matched the desired kind. A customer with a regular table named the same as a Maple MV would be reported asup_to_date, the apply path would skip creating the MV, and downstream aggregate / derived tables would never get populated while sync still reported success.Added a new
wrong_kindstatus toTableDiffEntry(and the matchingClickHouseTableDiffEntryschema in the domain HTTP types). Emitted whenactualTable.kind !== table.kind. The apply path skips these with a clear "resolve manually" reason rather than auto-remediating.other-destructive-cleanupcleanupStaleDeploymentsfiltered with!d.live && d.status !== \"live\", which matches in-flight states likedeployinganddata_readyand any unrecognised status string from a future Tinybird release. Combined withEffect.ignoreon every DELETE, this could silently delete an active rollout.Restricted the filter to known terminal-failed states (
failed,error) and requirelive !== trueas defense in depth. Replaced the silentEffect.ignorewithEffect.tapError(Effect.logWarning)so delete failures surface in logs.Out of scope (deferred)
OrganizationService.deleteOrganization). The current path purges local org-scoped tables before calling Clerk and is not transactional; if Clerk fails after the purge, data is gone but the upstream org persists. The fix needs a newdeletion_pendingcolumn + DB migration + retryable cleanup job.Test plan
wrong_kindcases covering both directions: MV-where-table-found and table-where-MV-found).failed/error(withlive !== true) deleted;deploying,data_ready,deleting, unknown future statuses, andlive: truedeployments all preserved.wrong_kindrow ("Wrong kind: expected MV, found table — resolve manually").bun turbo typecheckpasses across all 18 packages.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need.