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

Add connection property to limit server error detail in exception exceptions #1579

Merged
merged 3 commits into from Nov 14, 2019

Conversation

@sehrope
Copy link
Contributor

sehrope commented Oct 11, 2019

This patch keeps the old behavior but adds a new connection property to disable error detail. It defaults to "true" (the old behavior).

RFC for:

  • A better name for the property?
  • Whether we really need the single-argument constructor for PSQLException(ServerErrorMessage)? (i.e. is the constructor part of our public API, if not remove it to ensure we don't accidentally create it without the option)
  • Whether the non-sensitive message should include more than just the SEVERITY + MESSAGE?
  • Should we go to the next level and instead add a class-name style property that can customize the error, i.e. instantiate it as an object and call .getMessage(ServerErrorMessage) to get the text? (Seems like overkill but would give users option to format things however they'd like)

Consider this patch a WIP / proof-of-concept until those are addressed.

Also would need some tests to actually try out enabling and disabling things and verifying they're not included.

Also I did a quick compile check and ran a handful of tests but not the entire suite yet. I doubt the error messages break anything but if so we'll need to address those too...

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Oct 11, 2019

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Oct 12, 2019

This one looks better. Seems that the datasource also needs a getter/setter.

Thanks for working on this.

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Oct 12, 2019

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Nov 12, 2019

@sehrope is this ready ?

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Nov 13, 2019

Never got any feedback to change the naming etc so I suppose it's fine. I'll try to add at least one test for it today and then it should be good.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Nov 13, 2019

Excellent, thanks

sehrope added 2 commits Oct 11, 2019
…on messages

Adds a boolean connection property, LOG_SERVER_MESSAGE_DETAIL ("logServerMessage") that
controls whether the full error detail, as reported back by the server, is included in
the exception messages. This property can be used to limit potentially sensitive content
in server messages from bubbling up in exception messages and stack traces.

If set to "false" then only the top level error will be included in the exception message.
If set to "true" then all details in the server error will be included.

Default value for the new property is "true" to reflect the prior behavior of logging the
full error detail.

Closes #1577
@sehrope sehrope force-pushed the sehrope:feat-hide-error-detail branch from 817f661 to 421eed0 Nov 13, 2019
@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Nov 13, 2019

Rebased and added a test for the new property. Works on later versions of PG, hopefully it passes on all the older versions too. Tried to keep the error message checking generic enough that it should. Let's see what Travis says...

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 13, 2019

@sehrope sehrope force-pushed the sehrope:feat-hide-error-detail branch from 421eed0 to b3d2295 Nov 13, 2019
@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Nov 13, 2019

Looks like it was good on everything except 8.4 as the older server versions do not include the full details in the error response. I force pushed an update to skip test unless it's 9.1+. Once it passes should be good to merge.

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 13, 2019

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Nov 14, 2019

@sehrope I can't seem to find any code that actually looks for the version?

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Nov 14, 2019

@davecramer Because it's not... I added the check on line 38 but didn't wrap it in an assumeTrue(...) to actually skip the test. I'll fix it in a min.

@sehrope sehrope force-pushed the sehrope:feat-hide-error-detail branch from b3d2295 to c5b7fdd Nov 14, 2019
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 14, 2019

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Nov 14, 2019

So Travis passes and it correctly skips 8.4, but looks like it failed on some unrelated race condition on AppVeyor: https://ci.appveyor.com/project/davecramer/pgjdbc/builds/28856322/job/cyyur72crn2t7ohl#L306

I say this is good to go and will take a peek at that unrelated race condition some time soon.

@davecramer davecramer merged commit cd0b555 into pgjdbc:master Nov 14, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.