gmackie/ForgeGraph

Fork 0

fix(traffic): never sync DATABASE_URL onto a Worker #541

Merged

gmackie merged 2 commits from fix/canary-never-push-database-url into main

2026-08-30 23:10:55 +00:00

gmackie commented

2026-08-30 22:40:43 +00:00

Owner

What broke

syncSecretsToCanary pushed every stage secret to the canary Worker. The production store legitimately holds DATABASE_URL for consumers that are not Workers (migrations, systemd apps, local dev), so the canary received it too.

apps/web/src/lib/db.ts resolves the connection with DATABASE_URL taking precedence over the HYPERDRIVE binding — and the comment directly above that code says why it must never be set on a Worker. So the sync silently repointed the Worker at a Tailscale address the Cloudflare edge cannot reach.

Observed, 2026-08-30

A sync put DATABASE_URL on the Worker serving the control-plane apex:

Endpoint	22:26Z	22:33Z
`/api/fg/nodes`	200	500
`/api/fg/apps`	200	500
`/api/fg/hub-token`	200	500

The Hyperdrive binding was present and intact the whole time, just unused. That is why it reads as a database outage instead of a secret-sync bug. Deleting the one secret restored all three to 200 immediately.

syncSecretsToCanary runs on every traffic shift, not only at enable, so it re-broke production each time it ran. This is the same mechanism as #452.

Fix

Withhold the DB-routing keys at the Worker boundary — that is where the constraint lives. Removing them from the store would break the non-Worker consumers that need them.

WORKER_FORBIDDEN_SECRET_KEYS = DATABASE_URL, DATABASE_URL_LOCAL, DATABASE_OWNER_PASSWORD
Reported as skipped, not failed. A correct sync must not look broken, and conflating the two would mask real push errors. Both callers read only .failed, so the added field is backward compatible.
Withheld before decrypt, so a forbidden value never becomes plaintext.
missingCanarySecrets filters the same set: the primary predates the denylist and still carries these, so counting them as missing would report a gap no sync can ever close — and under enforcement that blocks every split forever.

Tests

23 pass. Three new:

never pushes DB-routing secrets to a Worker
withholds without decrypting
does not report a denylisted key as a parity gap

Two existing tests used DATABASE_URL as an arbitrary placeholder, one of them asserting it is pushed — that assertion encoded the bug. Swapped to SENTRY_DSN so they still cover push-all and failure-reporting.

Note: packages/api vitest cannot run locally through the normal entrypoint (PGlite global setup fails, pre-existing and unrelated); these were run with a minimal config scoped to this file.

🤖 Generated with Claude Code

## What broke `syncSecretsToCanary` pushed **every** stage secret to the canary Worker. The production store legitimately holds `DATABASE_URL` for consumers that are *not* Workers (migrations, systemd apps, local dev), so the canary received it too. `apps/web/src/lib/db.ts` resolves the connection with `DATABASE_URL` taking precedence over the `HYPERDRIVE` binding — and the comment directly above that code says why it must never be set on a Worker. So the sync silently repointed the Worker at a Tailscale address the Cloudflare edge cannot reach. ## Observed, 2026-08-30 A sync put `DATABASE_URL` on the Worker serving the control-plane apex: | Endpoint | 22:26Z | 22:33Z | | --- | --- | --- | | `/api/fg/nodes` | 200 | **500** | | `/api/fg/apps` | 200 | **500** | | `/api/fg/hub-token` | 200 | **500** | The Hyperdrive binding was present and intact the whole time, just unused. That is why it reads as a database outage instead of a secret-sync bug. Deleting the one secret restored all three to 200 immediately. `syncSecretsToCanary` runs on **every traffic shift**, not only at enable, so it re-broke production each time it ran. This is the same mechanism as #452. ## Fix Withhold the DB-routing keys at the Worker boundary — that is where the constraint lives. Removing them from the store would break the non-Worker consumers that need them. - `WORKER_FORBIDDEN_SECRET_KEYS` = `DATABASE_URL`, `DATABASE_URL_LOCAL`, `DATABASE_OWNER_PASSWORD` - Reported as `skipped`, **not** `failed`. A correct sync must not look broken, and conflating the two would mask real push errors. Both callers read only `.failed`, so the added field is backward compatible. - Withheld *before* decrypt, so a forbidden value never becomes plaintext. - `missingCanarySecrets` filters the same set: the primary predates the denylist and still carries these, so counting them as missing would report a gap no sync can ever close — and under enforcement that blocks every split forever. ## Tests 23 pass. Three new: - never pushes DB-routing secrets to a Worker - withholds without decrypting - does not report a denylisted key as a parity gap Two existing tests used `DATABASE_URL` as an arbitrary placeholder, one of them asserting it *is* pushed — that assertion encoded the bug. Swapped to `SENTRY_DSN` so they still cover push-all and failure-reporting. Note: `packages/api` vitest cannot run locally through the normal entrypoint (PGlite global setup fails, pre-existing and unrelated); these were run with a minimal config scoped to this file. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gmackie added 1 commit

2026-08-30 22:40:43 +00:00

fix(traffic): never sync DATABASE_URL onto a Worker

CI / gitleaks (pull_request) Successful in 7s

Details

CI / storybook (pull_request) Successful in 1m59s

Details

forgegraph/ci CI failed

CI / ci (pull_request) Failing after 5m18s

Details

9d20d18e39

`syncSecretsToCanary` pushed every stage secret to the canary Worker. The
production stage store legitimately holds `DATABASE_URL` for consumers that are
not Workers — migrations, systemd apps, local dev — so the canary got it too.

`apps/web/src/lib/db.ts` resolves the connection with `DATABASE_URL` taking
precedence over the `HYPERDRIVE` binding, and its comment says exactly why that
must not happen. So the sync silently repointed the Worker at a Tailscale
address the Cloudflare edge cannot reach. Every DB-backed route then 500s while
the Hyperdrive binding sits there intact and unused, which reads as a database
outage rather than a secret-sync bug.

This is what happened on 2026-08-30: a sync put `DATABASE_URL` on the Worker
serving the control-plane apex and took `/api/fg/nodes`, `/api/fg/apps` and
`/api/fg/hub-token` from 200 to 500. Because the sync runs on every traffic
shift and not only at enable, it re-broke production each time it ran.

Withhold the DB-routing keys at the Worker boundary, which is where the
constraint actually lives — removing them from the store would break the
non-Worker consumers that need them. They are reported as `skipped`, not
`failed`: a correct sync must not look broken, and conflating the two would
mask real push errors.

`missingCanarySecrets` filters the same set. The primary predates the denylist
and still carries these, so counting them as missing would report a gap no sync
can ever close — and under enforcement that blocks every split forever.

Two existing tests used DATABASE_URL as an arbitrary placeholder, one of them
asserting it gets pushed. Swapped to SENTRY_DSN so they still test push-all and
failure-reporting rather than encoding the bug.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

gmackie added 1 commit

2026-08-30 22:59:54 +00:00

test(traffic): prove the DATABASE_URL withhold through shiftAndBake

CI / gitleaks (pull_request) Successful in 6s

Details

CI / storybook (pull_request) Successful in 1m58s

Details

forgegraph/ci CI passed

CI / ci (pull_request) Successful in 10m12s

Details

d6137e4a60

CI caught what my local run could not: traffic-lifecycle-state.test.ts used
DATABASE_URL as the stage's only fixture secret and asserted it reaches the
canary, so the denylist made `put` a no-op and the ordering assertion had
nothing to order.

The test's actual subject is "the sync happens before the weight moves", and
the secret was incidental. Add SENTRY_DSN to carry that, and keep DATABASE_URL
in the fixture on purpose, now asserted as NOT pushed — so the withhold is
covered end to end through onDeploymentReported rather than only in
syncSecretsToCanary's own unit test.

`packages/api` vitest cannot run through the normal entrypoint locally (the
PGlite global setup fails, pre-existing), which is why one file passed and the
suite did not. Ran the three files that touch the changed surface with a
scoped config instead: 51 pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

gmackie merged commit 499e6ef1a2 into main

2026-08-30 23:10:55 +00:00

gmackie deleted branch fix/canary-never-push-database-url

2026-08-30 23:10:56 +00:00

gmackie referenced this pull request from a commit

2026-08-30 23:10:56 +00:00

Merge pull request 'fix(traffic): never sync DATABASE_URL onto a Worker' (#541) from fix/canary-never-push-database-url into main

gmackie referenced this pull request from a commit

2026-08-30 23:45:19 +00:00

fix(traffic): close the three ways a Worker secret sync misreports itself

gmackie referenced this pull request

2026-08-30 23:45:37 +00:00