Repository navigation
NCSA log filter: escape client-controlled fields - #567
Merged
Merged
Conversation
michaelJustin
force-pushed
the
563-ncsa-log-escaping
branch
from
October 14, 2026 17:45
4bf550d to
3ffeac3
Compare
TdjNCSALogFilter concatenated the user name (base64-decoded from an Authorization: Basic header), method, URI and version into the access-log line unescaped. A CR/LF in the user name forged extra log entries, a '"' shifted the fields, and an ESC could start a terminal escape sequence. - NCSAEscape: Apache-style escaping -- '"' -> \", '\' -> \, control characters (#0..#31, #127) -> \xHH; other characters unchanged. - FormatNCSALogLine builds the line from plain values (escaping the client-controlled ones), so DoFilter only gathers them and the format is testable without a server. - New djNCSALogFilterTests suite (10 tests), registered in TestHelper and added to the FPC and Delphi test projects. - SECURITY.md: the optional-units note now describes the escaping. Co-Authored-By: Claude Opus 5.5 <[email protected]>
michaelJustin
force-pushed
the
563-ncsa-log-escaping
branch
from
October 14, 2026 17:48
3ffeac3 to
6a62241
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #563.
TdjNCSALogFilterconcatenated client-controlled values into its access-log line unescaped. Before:After (the same request):
Changes
NCSAEscapeescapes the way Apache httpd does for its access log:"→\",\→\, control characters (#0..#31,#127, which covers CR, LF and ESC) →\xHH. Other characters are unchanged.FormatNCSALogLinebuilds the line from plain values and escapes the user name, method, URI and version.DoFilternow only gathers the values, so the format can be tested without a server. The output is unchanged for ordinary requests.djNCSALogFilterTestssuite (10 tests): the escaping, the line format, a forged user name staying on one line, and a"in the URI not ending the request field. It's registered inTestHelperand added toUnittests.lpr/.lpi/.dpr/.dproj.SECURITY.md: the optional-units note now describes the escaping instead of warning about it.Not escaped: characters outside ASCII, which the issue suggested writing as
\xHH. In the FPC buildstringholds UTF-8 bytes, but in Delphi it holds UTF-16 code units, so a byte-level rule would either break valid UTF-8 or behave differently on the two compilers. Line breaks, quotes and ESC are covered on both.Verification
run-fpc.cmd): 208 tests, 0 errors, 0 failures; heap trace at the clean baseline (6 blocks / 744 bytes).run-delphi.cmd): OK, 208 tests.🤖 Generated with Claude Code