ReviewOS

stacks/bun-query-builder

Docs advertise DB_* environment variables that are never read

#1070
Closed glennmichael123 opened this 2 months ago ·
2 months ago

The only environment variables src/ reads are:

DB_CONNECTION  DB_DIALECT  DB_DATABASE  DB_NAME  DB_SSL

Documentation tells users to set several that are read nowhere:

  • DB_USER, DB_TIMEOUT, DB_RETRY_ATTEMPTS, DB_HEALTH_CHECK_INTERVAL, DB_WAIT_READY_ATTEMPTS — never referenced in src/.
  • DB_HOST, DB_PORT, DB_USERNAME, DB_PASSWORD — also never read. Host, port, username and password can only come from config (setConfig, query-builder.config.ts, or database.url), not from the environment.

docs/usage.md:466-482 presents a block of these as a working "Environment-Specific Configurations" recipe, and docs/features/cli.md:874-878 presents another under "Security Considerations" — recommending environment variables specifically so credentials stay out of config files, which is exactly the case that does not work.

This is very likely what #1001's reporter hit before they found config.database.*: they set what the docs described, nothing took effect, and there was no error.

Two ways to close it, and the choice is a product decision:

  1. Document reality — say plainly that credentials come from config or database.url, and delete the variables that do nothing.
  2. Make the docs true — read DB_HOST/DB_PORT/DB_USERNAME/DB_PASSWORD in createConnectionString(). This is what most users expect from a database library, and it makes the CLI's security advice correct.

I'd suggest 2 for the four connection variables and 1 for the rest. Either way #1069's env-var check keeps it honest.

Found while auditing after #1001.

9 days ago

Correcting this issue: the premise is wrong, and it's my error.

I filed it from a grep for process.env.DB_*. createConnectionString does const e = process.env and then reads e.DB_HOST, e.DB_PORT, e.DB_USERNAME, e.DB_PASSWORD (src/db.ts:557-562), so that search missed every one of them. Those four are read.

The real problem is narrower, and more interesting: they only apply when DB_CONNECTION names the dialect (src/db.ts:556). Setting DB_HOST on its own is silently ignored. Verified both directions:

DB_HOST=env-host                        -> postgres://u:p@config-host:5432/d
DB_CONNECTION=postgres DB_HOST=env-host -> postgres://u:p@env-host:5432/d

That gating is undocumented, and it is the likely reason following the docs looked like it did nothing.

Still true from the original report: DB_USER, DB_TIMEOUT, DB_RETRY_ATTEMPTS, DB_HEALTH_CHECK_INTERVAL and DB_WAIT_READY_ATTEMPTS are read nowhere.

Being fixed in #1073 by documenting reality rather than changing behaviour — I withdraw the recommendation to make the variables apply ungated. A stray DB_HOST in an unrelated environment silently redirecting an app's database is a worse failure than the one being fixed.

Sign in to comment on this issue.