diff --git a/CHANGELOG.md b/CHANGELOG.md index 165d710f..07a185a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A malformed `DATABASE_URL` is refused without printing the password + +`DATABASE_URL` is taken apart before it reaches Bun, and the string most likely to fail that parse is +one with a stray character in the password. The refusal for an unparseable value quoted the whole +string back to name the fault, which wrote the database password into the log line that reported it. +It now names the variable and the shape it expects, the way the other refusals beside it already do, +and never echoes the value. + ### Wiping a computer is recorded even if clearing its stored state fails Resetting a computer destroys the profile first and wrote the audit row last, after two Postgres diff --git a/server/src/db/client.ts b/server/src/db/client.ts index a98687a9..74c31768 100644 --- a/server/src/db/client.ts +++ b/server/src/db/client.ts @@ -40,8 +40,12 @@ function addressOf(databaseUrl: string) { try { url = new URL(databaseUrl); } catch { + // The value is not echoed back. DATABASE_URL holds the database password, and the one string + // most likely to fail `new URL` is one with a stray character in that password, so printing it + // to name the fault would put the credential in the log line that reports it. Name the variable + // and the shape it expects, the way every other refusal in this function does. throw new TypeError( - `DATABASE_URL is not a URL: ${JSON.stringify(databaseUrl)}`, + "DATABASE_URL is not a valid URL. Expected postgres://user:password@host:port/database.", ); } if (url.hostname === "") { diff --git a/server/tests/db-client-address.test.ts b/server/tests/db-client-address.test.ts index d4efca2b..13c71ac4 100644 --- a/server/tests/db-client-address.test.ts +++ b/server/tests/db-client-address.test.ts @@ -27,12 +27,28 @@ describe("the database address", () => { expect(process.env.DATABASE_URL).toBeUndefined(); }); - test("refuses a connection string that is not a URL, naming what it got", () => { + test("refuses a connection string that is not a URL", () => { expect(() => createDatabase("://openbot@/openbot")).toThrow( - /DATABASE_URL is not a URL/, + /DATABASE_URL is not a valid URL/, ); }); + test("does not put the password in the message when the URL will not parse", () => { + // A stray character in a generated password is the likeliest reason `new URL` throws here, so + // the refusal must not echo the string it was given: DATABASE_URL carries the credential, and a + // message quoting it would write the password into the log line that reports the fault. The + // invalid port makes `new URL` throw with the secret still present in the input. + const secret = "s3cr3t-p4ssw0rd"; + let message = ""; + try { + createDatabase(`postgres://openbot:${secret}@127.0.0.1:notaport/openbot`); + } catch (error) { + message = error instanceof Error ? error.message : String(error); + } + expect(message).toMatch(/DATABASE_URL is not a valid URL/); + expect(message).not.toContain(secret); + }); + test("refuses a URL with no host, which would otherwise parse and connect nowhere", () => { // `new URL` accepts this: the scheme is "openbot:" and there is no host at all. expect(() => createDatabase("openbot:openbot@localhost/openbot")).toThrow(