Password not redacted in case of a URL that isn't fully valid

[dominicbarnes]

Within parseConfigError, it attempts to redact any user password. Unfortunately, if url.Parse fails for any reason, it simple proceeds with the DSN parsing logic, which causes a password to be leaked if it is part of the connection string.

I hit this by fuzz-testing some input on connection strings. In my case, I set port as "abc" and then got back the full DSN (including the password) back in the error message.

My hunch is if the string begins with postgres:// or postgresql:// and the URL cannot be parsed, it would be more safe to be cautious and not return the original connection string in the error message. It might be valuable to have redactPW return an error and perhaps adjust the behavior accordingly.

[jackc]

Maybe the parsing logic should be more selective and should check for postgres:// or postgresql:// and use that to determine whether to parse as a URL or DSN. A valid URL must start with it and a valid DSN must not. I think that would solve the error and would also improve the connection string parsing code.

[mikenai]

I have met same problem, but with characters in password which should be escaped in URL.

Example: https://play.golang.org/p/m33KrDRWke6

[jackc]

That's interesting. That seems to indicate that password redacting is impossible to do reliably when the connection string itself is invalid.

That is unfortunate. It seems to mean that if we are to safely redact we cannot return the underlying error as it may contain the conn string. I'm not even sure if trying to remove the entire conn string from the error message would be reliable as the error message may have escaped special characters in the original conn string.

But masking the entire error would be unhelpful to a developer trying to figure out why they can't connect to a database...

[mikenai]

we can try to cut out password from url with regexp if url.Parse error has gotten

[jackc]

we can try to cut out password from url with regexp if url.Parse error has gotten

We could try..., but there are an infinite number of ways for the parse to fail. Some of them are sure to not be handled by the regexp.

[noilpa]

I agree, using the wrong uri format, I can steal the database password from the logs. Example:

Error: database: ping error: parse postgres://user:pass/word@postgres:5432/db_name?sslmode=disable&connect_timeout=5": invalid port ":pass" after host

Probably add an explicit ban on parseConfigError.connString in the error

[jackc]

I've merged in this change. It at least improves the situation.

Though I am still dubious that any redaction short of omitting the entire conn string would be entirely reliable. But that would negatively impact the developer experience.

But perhaps this is now a good middle ground.