Repository navigation
Never drop a line, log CSRF errors where Rails raises them, and log in forked workers - #25
Merged
Merged
Conversation
- Strings and Hash keys are written as valid UTF-8. Puma passes headers as
binary strings, so a byte like \xFF in a User-Agent stopped the whole line
from being written as JSON, and SemanticLogger dropped it: anyone could hide
their request line, and the IP spoofing and CSRF lines. Bytes that aren't
valid UTF-8 become U+FFFD. NaN and Infinity are written as strings.
- A line that still can't be written as JSON is replaced by one that says so,
with the error's class, and the error is handled like LogStruct's own.
- New config.limits: Hashes write their first 100 keys (and _more_keys),
backtrace-like Arrays their first 100 lines (an Array of "a:0" strings was
written whole), other Arrays 10 items, and strings are cut at 32 KB after
they're scrubbed. One huge value no longer holds up the thread that writes
every line.
- logger.error(exception) wrote an empty line, and the message logged beside a
Hash (logger.info("Saved", id: 1)) was dropped. Exceptions are now error
lines with their class, message and backtrace; a message logged with one
goes in msg and the exception's in the new error_message field. The log
methods take SemanticLogger's exception argument (error(msg, payload, e)),
which raised an ArgumentError.
- A SystemStackError from an object's as_json is summarized like other
errors instead of escaping the formatter.
Sidekiq.logger turned a logged Hash into a String before LogStruct saw it,
so its keys weren't filtered ({password: "hunter2"} was logged as it was),
and dropped the Hash logged beside a message. Hashes, LogStruct entries and
exceptions now take the structured path, as with GoodJob's logger; a Hash
beside a message adds its keys to the Sidekiq line, and a message with an
exception is an error line with both.
- CSRF lines never appeared: the middleware sat outside ShowExceptions, which turns InvalidAuthenticityToken into a 422 before any middleware outside it sees the error. LogStruct now registers a DebugExceptions interceptor, which Rails calls with every error from the app in every environment, and logs CSRF and IP spoofing errors there. They're handled in the security_errors mode (reported by default; the IP spoofing line was never reported), and Rails responds as it would without LogStruct. - The IP spoofing check ran on every request and answered 403 app-wide, ignoring the proxies set on RemoteIp. Rails only raises where remote_ip is read, and that's where the error is logged now. - The middleware still logs errors that get past ShowExceptions (or come from the middleware outside it), each once, and raises them again. - Error lines have typed request fields (path, method, source_ip, user_agent, referer), and ActiveJob errors are a typed Log::ActiveJob::Error, instead of known fields in additional_data. - Removed integrations/rack.rb, which nothing loaded and which required a file that doesn't exist.
Production added a log/production.log file appender unless RAILS_LOG_TO_STDOUT was set, overriding an app's config.logger = STDOUT. Nothing rotated the file, and the README said LogStruct adds no file appender. Environments outside config.local_environments now log to STDOUT only; development keeps STDOUT and the log file, and test the log file.
…Puma's signal handlers - With "server" in ARGV and no Puma, requiring LogStruct raised LoadError and the app didn't boot. - The Puma patches, the boot banner replacement and the puma_lifecycle initializer now check LogStruct.enabled?. - The TERM/INT traps log the shutdown and then run the handler they replaced (or the signal's default action).
type_checking_errors now defaults to ReportProduction: raised in development and test, logged and reported in production. With LogProduction they were only logged, so type errors in an app and its gems went unreported. The docs said "log and report"; they now describe each error group's default.
- Ahoy: the tracking wrapper takes Ahoy's arguments as given (so Ahoy's default properties still apply), logs Symbol event names and non-Hash properties (from the browser) without raising, and reports its own failures as LogStruct errors. Before, a String of properties raised a type error and, in production, failed the request. - ActiveModelSerializers, Dotenv and Puma: a failure building their log line is a LogStruct error (logstruct_errors: logged in production, raised in development and test), not one of the app's (standard_errors, raised everywhere), so it can't fail a render, stop the app booting or break Puma in production.
SemanticLogger writes from a background thread, which a forked child doesn't get, so lines logged in Unicorn, Passenger and Pitchfork workers, Resque jobs and other forked children were never written. A Process._fork hook reopens the appenders in each child. It replaces the Puma cluster worker patch, which covered only Puma.
The host authorization integration replaced an app's response_app with its own. It now logs the blocked host and then calls the app's response_app if there is one (LogStruct's 403 page otherwise), and a failure building the line doesn't stop the response.
|
Warning Review limit reached
This review includes 59 billable files and costs up to $14.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 40 minutes for your next included review. View limit details
Comment |
This was referenced Oct 17, 2026
Merged
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.
Tenth of twelve stacked PRs for 0.2.0.
Lines that were dropped or never written
\xFFin aUser-Agent stopped the line from being written as JSON, and SemanticLogger dropped it. Anyone could hide their
request line, and the IP spoofing and CSRF lines. Strings and Hash keys are now written as valid UTF-8 (other bytes
become U+FFFD), and NaN and Infinity as strings. A line that still can't be written is replaced by one that says so,
with the error's class.
config.limits. Hashes write their first 100 keys (and_more_keys), backtraces their first 100 lines,other Arrays 10 items, and strings are cut at 32 KB after they're scrubbed, so one huge value can't hold up the
thread that writes every line.
logger.error(exception)wrote an empty line, and the message logged beside a Hash (logger.info("Saved", id: 1))was dropped. Exceptions are error lines with their class, message and backtrace; a message logged with one goes in
msgand the exception's in the newerror_messagefield. SemanticLogger'serror(msg, payload, e)form no longerraises
ArgumentError.so lines from Unicorn, Passenger and Pitchfork workers, Resque jobs and other forked children were lost. A
Process._forkhook reopens the appenders in each child. It replaces the Puma cluster worker patch, which coveredonly Puma.
Sidekiq.loggerturned a logged Hash into a String before LogStruct saw it, so its keys weren't filtered. Hashes,LogStruct entries and exceptions now take the structured path.
CSRF and IP spoofing
ShowExceptions, which turnsInvalidAuthenticityTokeninto a 422 first. LogStruct now registers aDebugExceptionsinterceptor and logs CSRFand IP spoofing errors there, in the
security_errorsmode (reported by default), and Rails responds as it wouldwithout LogStruct.
RemoteIp.Rails raises only where
remote_ipis read, and that's where the error is logged now.path,method,source_ip,user_agent,referer), and ActiveJob errorsare a typed
Log::ActiveJob::Error.integrations/rack.rb, which nothing loaded and which required a file that doesn't exist.Behaviour changes
log/production.logappender that nothing rotated, unlessRAILS_LOG_TO_STDOUTwas set, overriding an app'sconfig.logger = STDOUT. Development keeps STDOUT and its logfile, and test its log file.
type_checking_errorsdefaults toReportProduction: raised in development and test, logged andreported in production. Type errors were only logged, so they went unreported.
response_appif it set one (LogStruct's 403 page otherwise).Integrations that could fail the app
serverin ARGV and no Puma installed raisedLoadError, and the app didn't boot. The Pumapatches now check that Puma is loaded and that LogStruct is enabled, and Puma's
TERM/INThandlers run afterLogStruct logs the shutdown.
wrapper now takes Ahoy's arguments as given and logs what it gets.
not one of the app's, so it can't fail a render, stop the app booting or break Puma.
Testing
New tests for each change. Sorbet and the unit suite (367 tests) pass at this branch's tip.
Stack for 0.2.0, merged in order: #16 → #17 → #18 → #19 → #20 → #21 → #22 → #23 → #24 → #25 → #26 → #27