serilog-web / classic

[Discontinued] Serilog web request logging and enrichment for classic ASP.NET applications
Apache License 2.0
79 stars 32 forks source link

Fix exception with null key in form data #63

Closed davidp1978 closed 5 years ago

davidp1978 commented 5 years ago

fix #62 Fix null reference exception when a posted form contains an additional ampersand at the end ("Name=Value&") because ASP.NET will add an additional entry to the forms collection with a null key.

tsimbalar commented 5 years ago

Splitting the "if" should be good enough, yes 👍

Le dim. 24 mars 2019 13:34, davidp1978 notifications@github.com a écrit :

@davidp1978 commented on this pull request.

In src/SerilogWeb.Classic/Classic/SerilogWebClassicConfiguration.cs https://github.com/serilog-web/classic/pull/63#discussion_r268431649:

@@ -89,7 +89,7 @@ internal SerilogWebClassicConfiguration Edit(Func<SerilogWebClassicConfiguration /// Value of the pair internal string FilterPasswords(string key, string value) {

  • if (FilterPasswordsInFormData && FilteredKeywordsInFormData.Any(keyword => key.IndexOf(keyword, StringComparison.OrdinalIgnoreCase) != -1))
  • if (FilterPasswordsInFormData && key != null && FilteredKeywordsInFormData.Any(keyword => key.IndexOf(keyword, StringComparison.OrdinalIgnoreCase) != -1))

Should we just split the if statements? It feels like that will negate the need for extracting the IndexOf into a separate method. In a way, adding another method introduces another type of complexity by making a developer dig further into code to understand what it's doing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/serilog-web/classic/pull/63#discussion_r268431649, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJzIAyC3CJrV-fvz4GqRugJhRKb9Gubks5vZ3DngaJpZM4cFXJ- .

tsimbalar commented 5 years ago

hi @davidp1978 ,

is there anything I can do to help you move forward with this PR ?

cheers !

davidp1978 commented 5 years ago

Hi @tsimbalar , Apologies, been a bit busy with work, I'm hoping to split out the if statements either this week or over the weekend.

tsimbalar commented 5 years ago

No need to apologize , no worries 😉

Le mar. 2 avr. 2019 14:56, davidp1978 notifications@github.com a écrit :

Hi @tsimbalar https://github.com/tsimbalar , Apologies, been a bit busy with work, I'm hoping to split out the if statements either this week or over the weekend.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/serilog-web/classic/pull/63#issuecomment-478982500, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJzIM7mPAoq0b9qOG8RgW3s6ky4O7lNks5vc1NigaJpZM4cFXJ- .

davidp1978 commented 5 years ago

Hi @tsimbalar , I've split the "if" statements out. How's it looking now?

tsimbalar commented 5 years ago

LGTM ! 🚀

tsimbalar commented 5 years ago

Published ! https://www.nuget.org/packages/SerilogWeb.Classic/5.0.52

davidp1978 commented 5 years ago

👍