Skip to content

Prevent the ANSI appender and %aspnet-request dropping events - #316

Merged
FreeAndNil merged 4 commits into
masterfrom
Feature/316-content-loss
Sep 4, 2026
Merged

Prevent the ANSI appender and %aspnet-request dropping events#316
FreeAndNil merged 4 commits into
masterfrom
Feature/316-content-loss

Conversation

@FreeAndNil

@FreeAndNil FreeAndNil commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Both findings let content the application logged make log4net discard the whole
event. That is worse than log injection: the sender chooses what is not
recorded.

  • f029 AnsiColorTerminalAppender. The branch meant for a one-character
    message read message[0] without checking there was one, so an event that
    rendered to nothing threw and AppenderSkeleton swallowed it. The reset codes
    now go at one computed offset, leaving no short-message branch to get wrong.
  • f019 %aspnet-request. Reading HttpRequest.Params runs ASP.NET request
    validation on first access, so a request carrying <script> threw inside the
    layout and the event was discarded. The converter reads through
    HttpRequest.Unvalidated, which keeps the content instead of degrading the
    field to NOT AVAILABLE. The try now also covers the body parse and
    ServerVariables, two further paths that could throw there.

Two changelog-only commits ride along: every 3.5.0 entry now credits the fixer
per CLAUDE.md, and the Ext.Mail and remote-syslog entries cite the findings
behind them. Those touch entries owned by #313, #314 and #315.

Tests: f029 pins all ten line-break branches. f019 adds five, net462 only, so
they run on the Windows leg alone.

FreeAndNil added a commit that referenced this pull request Sep 3, 2026
@FreeAndNil
stop the aspnet-request converter dropping events with rejected content
60a3c7d 
#316

- Reading HttpRequest.Params validates the query string, form and cookies on
  first access, so a request carrying <script> threw inside the layout and
  AppenderSkeleton discarded the whole event: a sender could suppress the log
  record of their own request.
- The converter now reads through HttpRequest.Unvalidated, so the content is
  kept rather than replaced by the not-available marker.
- The try now also covers the body parse and ServerVariables, which throw on an
  oversized body or a lost client. Verified on Windows: with no worker request
  behind it, ServerVariables is empty rather than throwing.
- Five tests, net462 only, so they run on the Windows leg alone. Reverting the
  fix fails exactly the two that assert the content survives.

audit da18b6f-f019
FreeAndNil added a commit that referenced this pull request Sep 3, 2026
@FreeAndNil
stop the ANSI terminal appender dropping empty messages #316
957cb26 
- Logging an empty message threw IndexOutOfRangeException: the branch meant for
  a one-character message read message[0] without checking there was one.
- AppenderSkeleton caught it, so the event simply disappeared.
- The reset codes now go at one computed offset, leaving no short-message branch
  to get wrong.
- All ten line-break cases are pinned by test. They carry explicit names because
  dotnet test --filter cannot see them otherwise; CLAUDE.md records why.

audit da18b6f-f029
FreeAndNil added a commit that referenced this pull request Sep 3, 2026
@FreeAndNil
stop the aspnet-request converter dropping events with rejected content
ebacb66 
#316

- Reading HttpRequest.Params validates the query string, form and cookies on
  first access, so a request carrying <script> threw inside the layout and
  AppenderSkeleton discarded the whole event: a sender could suppress the log
  record of their own request.
- The converter now reads through HttpRequest.Unvalidated, so the content is
  kept rather than replaced by the not-available marker.
- The try now also covers the body parse and ServerVariables, which throw on an
  oversized body or a lost client. Verified on Windows: with no worker request
  behind it, ServerVariables is empty rather than throwing.
- Five tests, net462 only, so they run on the Windows leg alone. Reverting the
  fix fails exactly the two that assert the content survives.

audit da18b6f-f019
@FreeAndNil
FreeAndNil force-pushed the Feature/316-content-loss branch from 411f7b5 to 5b392b3 Compare September 3, 2026 21:22
FreeAndNil added a commit that referenced this pull request Sep 3, 2026
@FreeAndNil
stop the ANSI terminal appender dropping empty messages #316
da4b4af 
- Logging an empty message threw IndexOutOfRangeException: the branch meant for
  a one-character message read message[0] without checking there was one.
- AppenderSkeleton caught it, so the event simply disappeared.
- The reset codes now go at one computed offset, leaving no short-message branch
  to get wrong.
- All ten line-break cases are pinned by test. They carry explicit names because
  dotnet test --filter cannot see them otherwise; CLAUDE.md records why.

audit da18b6f-f029
FreeAndNil added a commit that referenced this pull request Sep 3, 2026
@FreeAndNil
stop the aspnet-request converter dropping events with rejected content
8b44780 
#316

- Reading HttpRequest.Params validates the query string, form and cookies on
  first access, so a request carrying <script> threw inside the layout and
  AppenderSkeleton discarded the whole event: a sender could suppress the log
  record of their own request.
- The converter now reads through HttpRequest.Unvalidated, so the content is
  kept rather than replaced by the not-available marker.
- The try now also covers the body parse and ServerVariables, which throw on an
  oversized body or a lost client. Verified on Windows: with no worker request
  behind it, ServerVariables is empty rather than throwing.
- Five tests, net462 only, so they run on the Windows leg alone. Reverting the
  fix fails exactly the two that assert the content survives.

audit da18b6f-f019
@FreeAndNil
FreeAndNil force-pushed the Feature/316-content-loss branch from 5b392b3 to 4c1120c Compare September 3, 2026 21:27
@FreeAndNil FreeAndNil added this to the 3.5.0 milestone Sep 3, 2026
@FreeAndNil FreeAndNil changed the title Feature/316 content loss Prevent the ANSI appender and %aspnet-request dropping events (#316) Sep 3, 2026
@FreeAndNil FreeAndNil changed the title Prevent the ANSI appender and %aspnet-request dropping events (#316) Prevent the ANSI appender and %aspnet-request dropping events Sep 3, 2026
@FreeAndNil
stop the ANSI terminal appender dropping empty messages #316
d3b8e9b 
- Logging an empty message threw IndexOutOfRangeException: the branch meant for
  a one-character message read message[0] without checking there was one.
- AppenderSkeleton caught it, so the event simply disappeared.
- The reset codes now go at one computed offset, leaving no short-message branch
  to get wrong.
- All ten line-break cases are pinned by test. They carry explicit names because
  dotnet test --filter cannot see them otherwise; CLAUDE.md records why.

audit da18b6f-f029
@FreeAndNil
stop the aspnet-request converter dropping events with rejected content
1452034 
#316

- Reading HttpRequest.Params validates the query string, form and cookies on
  first access, so a request carrying <script> threw inside the layout and
  AppenderSkeleton discarded the whole event: a sender could suppress the log
  record of their own request.
- The converter now reads through HttpRequest.Unvalidated, so the content is
  kept rather than replaced by the not-available marker.
- The try now also covers the body parse and ServerVariables, which throw on an
  oversized body or a lost client. Verified on Windows: with no worker request
  behind it, ServerVariables is empty rather than throwing.
- Five tests, net462 only, so they run on the Windows leg alone. Reverting the
  fix fails exactly the two that assert the content survives.

audit da18b6f-f019
@FreeAndNil
credit the fixer in every changelog entry
39033a4 
CLAUDE.md wants the description to close with both sides, who raised it and who
did the work. Eleven entries named only the audit finding. Nothing but the
attribution changed.
@FreeAndNil
cite the audit findings behind the Ext.Mail and syslog work #314
7de84b7 
The Ext.Mail send rework answers f004 and the remote syslog queue and pump work
answers f034, but neither pair named the finding. The other five 314 and 315
entries stay unattributed: they are our own, not audit findings.
@FreeAndNil
FreeAndNil force-pushed the Feature/316-content-loss branch from 4c1120c to 7de84b7 Compare September 4, 2026 08:09
@FreeAndNil
FreeAndNil marked this pull request as ready for review September 4, 2026 08:10
@FreeAndNil
FreeAndNil merged commit a208c53 into master Sep 4, 2026
3 checks passed
@FreeAndNil
FreeAndNil deleted the Feature/316-content-loss branch September 4, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fluffynuts fluffynuts fluffynuts approved these changes
@gdziadkiewicz gdziadkiewicz Awaiting requested review from gdziadkiewicz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.5.0

Development

Successfully merging this pull request may close these issues.

2 participants

@FreeAndNil @fluffynuts