Repository navigation
Add logging and log obfuscation (T9d, T9e) - #5
Merged
Merged
Conversation
An injected Logger with the eight-level enum, a stderr default, and a structured HTTP entry carrying method, URL, both bodies, status, duration, attempt number, request id and the server trace id. That entry is the point of the feature: a support ticket needs those as fields, not as prose to be parsed back out. LogHttp is not pure. The base implementation renders a one-line summary through Log(), so an adopter who only wants text does not reimplement the formatting. There is no built-in file sink. It is a dozen lines for an adopter, and building it in means owning rotation, flush policy and a path convention on every platform - three support surfaces for something the interface already makes possible. Config::log_level defaults to Warning, so NoLogging silences the SDK without swapping the logger. That is what a shipped game wants: the logger is still there for a bug-report build and nothing is constructed differently. Obfuscation is a scan and splice, not a parse and reserialise. Three reasons: a round-trip does not preserve key order, whitespace or number formatting, so the log stops being comparable against a report; a non-JSON body - an HTML 502 page - is exactly the artefact a support ticket needs and must reach the log untouched; and a round-trip through a model silently drops unknown keys, which is the opposite of what a log is for. The trade is more code and some string-skipping subtlety, both covered by tests. player_identifier joins the deny-list. It is a stable per-player id that survives across sessions, so it is personal data in the same sense an email is. Two things worth recording about the tests. The short-string guard's boundary is visible_start + visible_end + 4, and the first test asserted only that the value did not appear - which passed with the guard deleted, because a split mask also makes the whole string non-contiguous. It now asserts the exact output at 4, 6, 8, 10 and 11 characters. Disabling the deny-list check fails 19 assertions. Headers were dropped from the HTTP entry. The spec listed them, but a token in a header would need its own obfuscation path and no header the SDK sends carries one. Worth revisiting when T10 lands the real transport. The RequestId half of T9d's verification needs T11d. The entry carries request_id and the test drives it through a recording logger, which is as far as it can be proved without the executor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several unresolved critical issues can leak denied data or crash on malformed input, and the new log-level setting is not applied by the documented configuration loaders.
7 open issues
Unicode-escaped keys bypass the secret deny-list · New Composite secret values retain nested content · New Unterminated strings can underflow the redaction length · New HTTP bodies reach log sinks without central sanitization · New Config loaders ignore the new log_level setting · New Non-JSON text can be incorrectly redacted · New Redaction alters whitespace around the value · New
What changed in this PR
Adds the SDK’s core logging and JSON log-obfuscation infrastructure, including injectable loggers, structured HTTP entries, configurable log levels, stderr output, deny-list masking, and dedicated tests.
Changes:
- Introduces
Logger,LogLevel,HttpEntry,StderrLogger, and HTTP formatting APIs. - Adds scan-and-splice obfuscation for built-in and adopter-defined sensitive fields.
- Exposes logging configuration through
Config. - Integrates the implementation and test suite with CMake.
This pull request requires changes.
| File | Summary | Review notes |
|---|---|---|
tests/test_log.cpp |
Adds logging and obfuscation coverage. | No findings. |
tests/CMakeLists.txt |
Registers the logging test executable. | No findings. |
src/core/log.cpp |
Implements log levels, stderr output, HTTP formatting, deny-list management, and JSON masking. | Critical: Decode escaped key names before deny-list matching (4 votes); handle composite denied values without exposing nested secrets (4 votes); prevent unterminated-string range underflow (1 vote); sanitize bodies through a central dispatch before invoking sinks (1 vote). Moderate: Preserve non-JSON bodies rather than scanning text containing quoted fields (2 votes); avoid truncating long HTTP summaries (1 vote). Nit: Preserve separator bytes when replacing values (4 votes). |
include/lootlocker/log.h |
Defines the public logging, HTTP entry, and obfuscation interfaces. | No findings. |
include/lootlocker/config.h |
Adds configurable logging support to Config. |
Moderate: Parse and validate log_level in both configuration loaders and add tests (2 votes). Append the new member to preserve compatibility with positional aggregate initialization (1 vote). |
CMakeLists.txt |
Adds the logging implementation to the core library. | No findings. |
Review effort: Lite
The serious one: a key written with a JSON unicode escape bypassed the
deny-list entirely. "sess\u0069on_token" is the same key as "session_token" to
every JSON parser, but the decoder only stripped the backslash, so the key
became "sessu0069on_token", matched nothing, and the token was logged in clear.
It is now a real JSON string decoder, including \uXXXX. Verified by reverting
the decoder and watching three assertions fail.
Obfuscation never ran on the HTTP path. LogHttp forwarded the entry straight to
the formatter, so any caller that built an entry from raw transport bodies
leaked, and a custom LogHttp override bypassed the base method anyway. There is
now a non-member log::LogExchange that obfuscates both bodies and then calls the
virtual sink. It is the only way the SDK logs an exchange, and it is not a
member so it cannot be overridden away.
A composite value was masked with the scalar policy, so an array of tokens kept
its first and last three characters - bytes from inside the value, which can be
other secrets. Arrays and objects are now replaced wholesale.
An unterminated string underflowed size_t: SkipString returns body.size(), and
the length expression subtracted 2 for a closing quote that was not there. In a
no-exceptions build that is a termination, not an exception. The unterminated
case now copies the remainder verbatim.
Whitespace after the colon was skipped but never copied, so
{ "session_token" : "x" } became { "session_token" :"***" }. The separator
region is now copied verbatim, which is what the byte-identical contract
promised.
Config::log_level was not read by either loader. The file loader now accepts a
level by name or number and rejects an unknown name rather than silently
defaulting; the CLI accepts --log-level. Both have tests.
Two of the new tests could not fail on the first attempt, and both were caught
by breaking the code and watching them pass. The composite test asserted only
that the secret was absent, which is true even when the array is masked with the
scalar policy - the retained bytes are the bug, not the full string. It now
asserts the exact output.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kirre-bylund
commented
Nov 13, 2026
The same three functions had been written independently in four translation units: LowerAscii twice byte-identical, EqualsIgnoreCase three times in two slightly different forms. That is how the copies drift, and one of them already had - the config loaders folded case inline while the timezone converter used a helper. core::text is a new internal header, following the core::json / core::transport / core::state convention the spec already sets out. It is not public: src/ is not on the adopter's include path, so the test reaches in deliberately rather than the header being promoted. The refactor is mechanical - the bodies moved unchanged and the call sites are qualified. Verified by the existing timezone and logging suites, which exercise all three helpers through real behaviour, plus a new test_text suite for the helpers themselves now that four files depend on them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.



Implements T9d (logging) and T9e (log obfuscation). They are one change because the obfuscator exists to protect the logger, and splitting them would ship a logger that can leak a token for a commit.
Logging
An injected
Loggerwith the eight-level enum, a stderr default, and a structuredHttpEntrycarrying method, URL, both bodies, status, duration, attempt number, request id and the server trace id. That entry is the point of the feature: a support ticket needs those as fields, not as prose to be parsed back out.Three shapes differ from the sketch:
LogHttpis not pure. The base implementation renders a one-line summary throughLog(), so an adopter who only wants text does not reimplement the formatting.Config::log_level, defaulting toWarning.NoLoggingsilences the SDK without swapping the logger, which is what a shipped game wants.Obfuscation
A scan and splice, not a parse and reserialise. Three reasons, and the first is the one that decided it:
The trade is more code and some string-skipping subtlety, both covered by tests.
player_identifierjoins the deny-list: it is a stable per-player id that survives sessions, so it is personal data in the same sense an email is.Two testing notes worth reading
The first short-string test could not fail. It asserted only that the value did not appear in the output — which passes with the guard deleted, because a split mask also makes the whole string non-contiguous. I found it by deleting the guard and watching the test still pass. It now asserts the exact output at 4, 6, 8, 10 and 11 characters, and deleting the guard fails 4 checks.
Disabling the deny-list check entirely fails 19 assertions.
Verification
-DLOOTLOCKER_MODULE_ONLYstill configures forcore,authandauth;broadcasts.Not done, deliberately
Headers are not in the
HttpEntry. The spec listed them, but a token in a header would need its own obfuscation path, and no header the SDK sends carries one. It is a one-field addition when T10 lands the real transport, and I would rather add it with the transport than guess at the shape now.The
RequestIdhalf of T9d's verification needs T11d. The entry carriesrequest_idtoday and the test drives it through a recording logger, which is as far as it can be proved without the executor existing.Spec
lootlocker/index#1881 records the three shape changes, the scan-and-splice decision, the guard boundary, and the
player_identifieraddition. It also closes out T9a, which was still open from last time.