Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Issue #1577. It is currently possible to leak information into logs from errors #1578

Closed
wants to merge 1 commit into from

Conversation

davecramer
Copy link
Member

The server will send parameter values in the DETAIL server error message
Adding a log_detail property which if set to false will omit the DETAIL part of the
ServerErrorMessage.
While this won't stop all data from leaking it does provide a first pass.

Note this setting once set will be set for all connections in the JVM as this is a static
variable in the Driver.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?
  3. Have you added your new test classes to an existing test suite?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

…into logs from

errors. The server will send parameter values in the DETAIL server error message
Adding a log_detail property which if set to false will omit the DETAIL part of the
ServerErrorMessage.
While this won't stop all data from leaking it does provide a first pass.

Note this setting once set will be set for all connections in the JVM as this is a static
variable in the Driver.
@davecramer
Copy link
Member Author

I'm not entirely sure I like this as it sets the logging for the Driver for all connections.

Consider this a straw man that we can debate around.

The alternative is to provide connection information in the PSQLException which seemed pretty invasive.

@AppVeyorBot
Copy link

Build pgjdbc 1.0.469 failed (commit cae24fc2a3 by @davecramer)

@sehrope
Copy link
Member

sehrope commented Oct 11, 2019

-1 to changing the internals of ServerErrorMessage and hiding the detail based on driver wide flags.

I think the way to do this is add a non-sensitive message method to ServerMessageMessage and have the PSQLException(...) constructor take a parameter of whether to use the full message (i.e. the old toString()) or the new non-sensitive one.

There's only three spots in the driver itself that create a PSQException from a ServerErrorMessage and all of them are in the context of a connection so it could look at connection properties to determine which message type to show. If we consider the exception class part of the "interface" then we could leave the old signature in place as well.

@sehrope
Copy link
Member

sehrope commented Oct 11, 2019

See #1579 for connection option (wip) version of this.

@davecramer davecramer closed this Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants