feat(security): erase ClickHouse data as a dedicated user, not the superuser - #66
Merged
Conversation
…peruser The deletion routes ran ALTER TABLE events DELETE through getMutatingClickHouseClient(), which keeps the user from CLICKHOUSE_URL — the unrestricted `default`. So the dashboard held a full-rights ClickHouse credential in its environment for the whole process lifetime, and the first account/workspace/project deletion opened a superuser connection. Any RCE, SSRF-to-localhost or env dump in the Next.js process yielded DDL on the analytics store, up to DROP TABLE events. That was the largest remaining gap from the security review, and the follow-up lib/clickhouse.ts already recorded. CH migration 0012 adds `yavio_eraser` with exactly one capability: GRANT ALTER DELETE ON default.events. No SELECT, no INSERT, no DDL, nothing on any other table — it cannot read the rows it is allowed to remove. The user is created with `IDENTIFIED WITH no_password`, so it fails closed. Migration 0007 created its siblings with the literal 'yavio_dev', published in this public repository, which is precisely the defect this week had to unwind. The password comes from CLICKHOUSE_ERASER_PASSWORD and is applied by migrate-clickhouse.ts alongside the ingest and dashboard users, so it never appears in a migration file or the migration history. The client FALLS BACK to the CLICKHOUSE_URL user when the password is unset, so a deployment that has not yet run 0012 keeps erasing rather than silently failing — which is the exact failure mode this file exists to prevent, and the one that shipped in #58. Also fixes the test-hygiene gap the review flagged: the suite now saves and restores every CLICKHOUSE_* variable it touches, instead of only CLICKHOUSE_URL. A repo .env setting CLICKHOUSE_DASHBOARD_PASSWORD previously turned a real assertion into an environment-dependent flake. Ten dashboard tests, including both new branches. 508 dashboard tests pass; compose validates; setup-env.sh generates the new password; typecheck and biome clean. NOT YET DEPLOYED — see the rollout note in the PR. Migrations must run and the password must be verified BEFORE the dashboard is switched, exactly as the yavio_api and per-user ClickHouse rollouts did.
Coverage Report for ingest
File CoverageNo changed files found. |
Coverage Report for dashboard
File Coverage
|
||||||||||||||||||||||||||||||||||||||
The census asserts the exact set of applied versions, so adding 0012_eraser_user.sql made it fail with 12 vs 11 — which is the check working as intended rather than a flake. An exact-set assertion is deliberately noisier than a count: it catches a migration that silently fails to apply.
Coverage Report for db
File CoverageNo changed files found. |
This was referenced Aug 6, 2026
marselsel
added a commit
that referenced
this pull request
Aug 6, 2026
…hind (#69) Three residual findings from reviewing #42–#66, all the same shape: a credential that stays published or absent when the operator takes a path the scripts do not cover. yavio_app kept the published 'yavio_dev'. 0001 creates the role with that literal under an IF NOT EXISTS guard; migrate.ts repairs it from POSTGRES_APP_PASSWORD, and setup-env.sh generates one — but the compose `migrate` service never passed the variable, and compose-injected environment is not inherited from .env. So a deployment migrating through compose kept a role with a password anyone can read on GitHub and DML on all 14 tables, even for an operator who ran setup-env.sh and followed every documented step. RLS is no barrier: its policies key on current_setting('app.current_user_id'), which the connecting session sets for itself, and verification_tokens, login_attempts and stripe_webhook_events have no RLS at all. The two MOST privileged accounts still fell back to 'yavio_dev'. #57 made POSTGRES_API_PASSWORD fail closed with `:?` but left `:-yavio_dev` on POSTGRES_SERVICE_PASSWORD — the Postgres SUPERUSER, which can COPY ... FROM PROGRAM — and on CLICKHOUSE_PASSWORD, which is unrestricted and carries CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1. Both now use the same `:?` form, and .env.example ships them blank rather than pre-filled. Compose refuses to start instead of substituting; a startup error costs a minute, a published default is a credential in a public repo. setup-env.sh put every secret on a command line. `sed -i "s|^${key}=.*|..."` makes the value part of argv, which is world-readable via ps and /proc/<pid>/cmdline while /proc/<pid>/environ is owner-only — so all seven datastore passwords and four app secrets were exposed to any local user, and the chmod 600 two lines above bought nothing (CWE-214). Now the value goes through the environment into awk. That also drops the BSD/GNU `sed -i` split and the escaping question: awk prints the value literally, so a `|`, `&` or backslash from a future generator cannot break the substitution or inject a line. The temp file is created under umask 077 because mv keeps its mode. Also: docker-compose.test.yml published a password-`test` Postgres and ClickHouse on 0.0.0.0 from every developer's machine — the dev compose file has bound to loopback since #42. And the self-hosting docs still advertised 'yavio_dev' as the default for five variables, omitted POSTGRES_API_PASSWORD and CLICKHOUSE_ERASER_PASSWORD entirely, and told readers to change defaults that no longer exist. Verified rather than assumed. The first attempt at proving the yavio_app rotation connected via 127.0.0.1 from inside the container, where pg_hba says `host all all 127.0.0.1/32 trust` — both the old and new passwords "authenticated" because no password was being checked at all. Re-run over a scram-checked connection from the host: 'yavio_dev' REJECTED (28P01), rotated password AUTHENTICATED. Compose fails closed with no .env and validates with the variables set; setup-env.sh produces a 0600 .env, leaves no temp file, and writes no placeholder into any value.
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.
Closes the largest remaining gap from the security review.
The deletion routes ran
ALTER TABLE events DELETEthroughgetMutatingClickHouseClient(), which keeps theCLICKHOUSE_URLuser — the unrestricteddefault. The dashboard therefore held a full-rights ClickHouse credential inprocess.envfor its whole lifetime, and the first deletion opened a superuser connection. Any RCE, SSRF-to-localhost or env dump yielded DDL on the analytics store, up toDROP TABLE events.The new identity
CH migration 0012 adds
yavio_eraserwith exactly one capability:No SELECT, no INSERT, no DDL, nothing on any other table — it cannot read the rows it is allowed to remove.
Created
IDENTIFIED WITH no_passwordso it fails closed. Migration 0007 created its siblings with the literalyavio_devpublished in this repo — precisely the defect this week had to unwind. The password comes fromCLICKHOUSE_ERASER_PASSWORD, applied bymigrate-clickhouse.tsalongside the other two, so it never lands in a migration file or its history.Falls back to the
CLICKHOUSE_URLuser when unset, so a deployment that hasn't run 0012 keeps erasing rather than silently failing — the exact failure mode that shipped in #58.Also fixes the test-hygiene gap the review flagged: the suite now saves/restores every
CLICKHOUSE_*variable, not justCLICKHOUSE_URL. A repo.envpreviously turned a real assertion into an env-dependent flake.Verification
510 dashboard tests (10 in this suite, covering both new branches), compose validates,
setup-env.shgenerates the password, typecheck and biome clean.Not deployed. Production must be rolled out in this order, mirroring the
yavio_apiand per-user ClickHouse rollouts:CLICKHOUSE_ERASER_PASSWORDto the VM.env(generate on the VM; never print it)docker-compose.ymlfrommain— the VM copy is a deployed copy, not a checkoutyavio_eraserand applies the passwordyavio_erasercan authenticate before switching the dashboardStep 4 is what caught the missing
CONNECTgrant during theyavio_apirollout and aborted with production untouched.