Issue2501 Postgres returns inserted data in exception if insert fails#3008
Issue2501 Postgres returns inserted data in exception if insert fails#3008dwat001 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
as this is a suppression, not an inclusion, this description isn't quite accurate
Suggest swapping included with suppressed
There was a problem hiding this comment.
why is this commented?
There was a problem hiding this comment.
😞 Good catch thank you.
There was a problem hiding this comment.
| [Description("When enabled prevents excludes the Postgres DETAIL response from raised exceptions. This DETAIL message often contains user data that may be sensative.")] | |
| [Description("When enabled, removes the Postgres DETAIL property from raised exceptions. This DETAIL message can contain user data that should not be included in logs or displayed to users.")] |
There was a problem hiding this comment.
could place constant text in here to explain why it's missing?
"Detail suppressed as SuppressDetailInPostgressError is enabled"
Based on a new ConnectionString property SuppressDetailInPostgressError drop the DETAILS field sent by postgres as this often contains user data which may be sensitive.
Based on a new connection string property, suppress the logging of Command parameters.
dwat001
force-pushed
the
Issue2501_ExceptionDetailHasProductionData
branch
from
c076842 to
19c36ee
Compare
| public PostgresException() {} | ||
|
|
||
| internal PostgresException(ReadBuffer buf) | ||
| internal PostgresException(ReadBuffer buf, NpgsqlConnectionStringBuilder settings) |
There was a problem hiding this comment.
There is no need to pass all settings, just add a bool flag instead.
| [Description("When enabled prevents excludes the Postgres DETAIL response from raised exceptions. This DETAIL message often contains user data that may be sensative.")] | ||
| [DisplayName("Suppress Postgres Error Detail")] | ||
| [NpgsqlConnectionStringProperty] | ||
| public bool SuppressDetailInPostgressError |
There was a problem hiding this comment.
Probably it would be better to call it SuppressDetailedExceptions or something like this. It should be short enough but meaningful.
|
By the way, this PR should target the |
…#2501) Address review comments: - Fix wording in Documentation - Rename SuppressDetailInPostgressError to SuppressDetailedExceptions - Pass bool flag not compleate settings object - Replace Detail with message when removing
As per the description of this PR I am happy to land this in master, but I would also like this in the 3.X series. |
|
I have started a second PR to take the same change into master #3011 |
|
Closing this in favour of #3011. |
|
Hey @YohDeadfall isn’t this pr required to get this change into the 3.x release line? |
|
We accept only bug fixes into hot fix branches which works only for the specific versions. For all general things like features and fixes for bugs existing in the current version pull requests must target the master branch. After merging we could backport them. |
|
@dwat001 @williamdenton Why do you stuck with so old version of Npgsql when there are 4.1 and 4.0? Any reason no to use them? |
|
@YohDeadfall you're totally right. I/We misspoke about v3.x. We are currently upgrading our services from dotnet 2.1 to 3.1, so updating EFCore too. I got confused with the microsoft pattern of tying version numbers to framework versions (which in itself is confusing given netstandard). We currently use v4.0.8 We should be able to jump to 4.1. Release notes look like plain sailing except for snake case converters, we use our own code to do this currently, inspired from here npgsql/efcore.pg#21 but I've upgraded personal projects from that so should be doable for us here too. |
3 participants
As per comments in the Issue, this PR deals with both
PostgresException.Detailsand the logging of command parameters.If this approach gains approval I will apply the same change to the
devbranch. Given the major version bump in thedevbranch we could consider removing or deprecatingNpgsqlLogManager.IsParameterLoggingEnabledI would not do that on the 3.X branch.