node-postgres icon indicating copy to clipboard operation
node-postgres copied to clipboard

Fix parsing of ssl=false connection string parameter

Open sonicdoe opened this issue 4 years ago • 4 comments

Cherry-picks https://github.com/iceddev/pg-connection-string/pull/39 to this monorepo.

sonicdoe avatar Nov 27 '20 12:11 sonicdoe

ssl=false isn’t supported by libpq, only ssl=true. Incompatibilities already exist between pg-connection-string and libpq, but I’m against adding new ones.

(sslmode=disable is the correct version.)

charmander avatar Nov 27 '20 21:11 charmander

ssl=false is documented in pg-connection-string’s readme, though. Would you rather like to remove it there?

Alternatively, we could remove support for all ssl values except true to align with libpq. After all, it might be a bit surprising to support ssl=1, ssl=true, ssl=0 but not ssl=false.

sonicdoe avatar Nov 30 '20 10:11 sonicdoe

I agree that since the other non-libpq modes are supported, ssl=false should be supported.

When ssl=false is set, it actually ends up enabling ssl because it gets set to the string "false", which is truthy.

This is very surprising behavior, especially because the docs actually explicitly state this is allowed.

If feature parity with libpq is more important, it could at least log a warning or throw an exception if the user uses this ssl flag in an unsupported way.

snowbldr avatar Jul 27 '21 01:07 snowbldr

It could possibly throw an uncaught exception without this patch.

Reproduce Steps

  1. Postgres with SSL
  2. Connect with ssl=false
const client = new Client({
    connectionString: "postgres://postgres:postgres@localhost:5432/postgres?ssl=false",
});
client.connect();
// Throw uncaught exception

TypeError: Cannot use 'in' operator to search for 'key' in false

https://github.com/brianc/node-postgres/blob/b1a8947738ce0af004cb926f79829bb2abc64aa6/packages/pg/lib/connection.js#L90C7-L90C7

Since my application allows people to input any connection strings freely, throw an uncaught exception is not nice.

Possible solutions

Method 1: Merge this pull request. Method 2: In case you prefer sslmode=disable, "ssl=false" should completely ignored and muted, rather than parse it as a string "false".

louislam avatar Oct 12 '23 02:10 louislam