Invalid connection strings can cause credentials to leak to console

[pauls-ai2]

Node: 16.14.2
PG: 8.11.3

The connectionString config is parsed by URL in node. When that library has an invalid string, it will throw an error with error.input being the value provided. If you create a client without wrapping it in a try/catch, that error will be logged to the console. That means if the password causes makes the Postgres connection string an invalid URL, the credentials for your DB will leaked to the console.

Here's a quick example:

const pg = require('pg');

const host = 'localhost';
const port = 5432;
const user = 'user';
const password = 'g#4624$@F$#v`';
const database = 'db';

const POSTGRES_URL = `postgres://${user}:${password}@${host}:${port}/${database}`;

const clientWorks = new pg.Client({
  host,
  port,
  user,
  password,
  database,
});

const clientFails = new pg.Client({
  connectionString: POSTGRES_URL,
});

node:internal/url:552
  throw new ERR_INVALID_URL(input);
  ^

TypeError [ERR_INVALID_URL]: Invalid URL
    at new NodeError (node:internal/errors:371:5)
    at onParseError (node:internal/url:552:9)
    at new URL (node:internal/url:628:5)
    at parse (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/node_modules/pg-connection-string/index.js:29:14)
    at new ConnectionParameters (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/node_modules/pg/lib/connection-parameters.js:56:42)
    at new Client (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/node_modules/pg/lib/client.js:18:33)
    at Object.<anonymous> (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/index.js:21:21)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  input: 'postgres://user:g#4624$@F$#v`@localhost:5432/db',
  code: 'ERR_INVALID_URL'
}

[sehrope]

Constructing the URI like that is incorrect. You need to escape the values via `encodeURIComponent(...):

const POSTGRES_URL = `postgresql://${encodeURIComponent(user)}:${encodeURIComponent(password)}@${host}:${port}/${encodeURIComponent(database)}`;

That allows you to handle arbitrary characters in those fields and ensure the URI is always valid.

[pauls-ai2]

I agree that it is incorrect. That doesn't change the fact that many places build URLs this way. I found this because Google's SQL can generate URLs like this that are invalid URLs, but the credentials in them would be leaked if used this way. That's one example, but there are many more.

[MiichaelJose]

Nó: 16.14.2 PG: 8.11.3

A connectionStringconfiguração é analisada URLem node . Quando essa biblioteca tiver uma string inválida, ocorrerá um erro ao error.inputser o valor fornecido. Se você criar um cliente sem envolvê-lo em um try/catch, esse erro será registrado no console. Isso significa que se a senha fizer com que a string de conexão do Postgres seja uma URL inválida, as credenciais do seu banco de dados vazarão para o console.

Aqui está um exemplo rápido:

const pg = require('pg');

const host = 'localhost';
const port = 5432;
const user = 'user';
const password = 'g#4624$@F$#v`';
const database = 'db';

const POSTGRES_URL = `postgres://${user}:${password}@${host}:${port}/${database}`;

const clientWorks = new pg.Client({
  host,
  port,
  user,
  password,
  database,
});

const clientFails = new pg.Client({
  connectionString: POSTGRES_URL,
});

node:internal/url:552
  throw new ERR_INVALID_URL(input);
  ^

TypeError [ERR_INVALID_URL]: Invalid URL
    at new NodeError (node:internal/errors:371:5)
    at onParseError (node:internal/url:552:9)
    at new URL (node:internal/url:628:5)
    at parse (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/node_modules/pg-connection-string/index.js:29:14)
    at new ConnectionParameters (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/node_modules/pg/lib/connection-parameters.js:56:42)
    at new Client (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/node_modules/pg/lib/client.js:18:33)
    at Object.<anonymous> (/Users/<REDACTED>/sandbox/2024-02-11--pg-password-leak/index.js:21:21)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  input: 'postgres://user:g#4624$@F$#v`@localhost:5432/db',
  code: 'ERR_INVALID_URL'
}

no DATABASE_URL coloquei apenas "postgresql://" resolveu meu problema

[leandro-cam]

Constructing the URI like that is incorrect. You need to escape the values via `encodeURIComponent(...):

const POSTGRES_URL = postgresql://${encodeURIComponent(user)}:${encodeURIComponent(password)}@${host}:${port}/${encodeURIComponent(database)};

That allows you to handle arbitrary characters in those fields and ensure the URI is always valid.

This helped me!!!

[hjr3]

That doesn't change the fact that many places build URLs this way.

Do you have a suggestion on how to make this safer? This seems more like a bug for URL in node.js that a bug for pg-connection-string.

[pauls-ai2]

This seems more like a bug for URL in node.js that a bug for pg-connection-string.

Yes, I think you are correct.

I have reached out to the Node Security team and they say their URL parser is working as expected. I disagree, but maybe that's why I'm not on the team. 😉

[fchoquet]

I've just faced the same issue. It caused secrets to leak in logs.

Do you have a suggestion on how to make this safer? This seems more like a bug for URL in node.js that a bug for pg-connection-string.

I disagree. The URL parser does what is expected: It parses a URL an triggers an error if the URL is invalid.
The problem here is that the error message should be sanitized to avoid secrets to leak and this is this library's responsibility (using new URL is an implementation detail of this library after all).
So I guess the error should be caught and thrown again but with obfuscated secrets.

[hjr3]

@fchoquet what do you propose that we do?

[fchoquet]

@fchoquet what do you propose that we do?

Sorry for the late answer. I would suggest to wrap the call to new URL in a try catch, and instead of returning directly the error returned by the URL parser, create a new error that leaks less details.