Feat/structured logging and request limits - #772
Merged
Abd-Standard merged 3 commits intoSep 1, 2026
Merged
Abd-Standard merged 3 commits into
Abd-Standard merged 3 commits into
Conversation
JSON output was tied to NODE_ENV === production, so structured logs could not be enabled anywhere else — reproducing an aggregator problem locally meant pretending to be production. LOG_FORMAT (json | pretty) now selects it explicitly and wins over the environment; with LOG_FORMAT unset the previous behaviour is preserved exactly. Records keep a stable field set in both formats — timestamp, level, message, plus whatever structured context the call site attached — and error information still goes through formatError, so name/message/stack/cause stay machine-readable rather than collapsing to a string. Secret redaction is new. Log metadata is walked before emission and any field whose name looks like a credential (password, secret, token, apiKey, authorization, signature, cookie, privateKey, plus case and underscore variants) has its value replaced with [REDACTED]. The field name is kept so record shape stays stable for aggregator indexing. Redaction runs on the way into the logger rather than being left to callers: a secret only has to be forgotten once to sit permanently in an aggregator, and the call site is the party most likely to forget. Recursion is depth- bounded so a deeply nested or cyclic object cannot hang the logging path — logging must never be what takes the service down. sanitizeUrl strips credential-bearing query parameters from a URL while keeping the path, which is the part that identifies an endpoint. It builds the query string by hand rather than via URLSearchParams.toString(), which would percent-encode the marker into %5BREDACTED%5D — still redacted, but no longer greppable, which defeats the point of a fixed placeholder. parseLogLevel/parseLogFormat are strict counterparts to resolveLogLevel: they return null instead of downgrading, so config validation can reject. resolveLogLevel keeps its non-throwing fallback, since a bad value must not crash a process that is already running. 41 tests.
Seven POST/PUT handlers in the events API accumulate the request body into a
string (body += chunk) with no ceiling, so a single large upload grew the
process heap unbounded. API_MAX_BODY_BYTES now caps it, defaulting to 1 MiB.
The guard runs once at dispatch rather than in each handler, which is what
makes "does not attempt to process rejected payloads" true: by the time a
route attaches its own data listener the request has already been screened,
and an oversized one has been answered with 413 and had its socket
destroyed. Fixing this per-handler would have been seven chances to miss one
and no protection for routes added later.
Two checks, because either alone is insufficient:
* Content-Length, when present and over the limit, is refused before a
single byte of body is read.
* A streaming byte counter, because Content-Length is client-supplied and
absent entirely on chunked transfers. A client can understate it or omit
it; the counter is what actually bounds memory.
The socket is destroyed on rejection — without it the client keeps sending a
body nobody will read and the bytes still transit the process. The 413
carries code PAYLOAD_TOO_LARGE with maxBytes and observedBytes, and echoing
the observed size leaks nothing the client did not itself send.
Chunks are measured with Buffer.byteLength, not string length: a multi-byte
character counted as one would let several times the intended payload
through.
GET/DELETE/HEAD/OPTIONS pay only a set lookup.
27 tests, concentrated on boundaries — exactly at the limit accepted, one
byte over rejected, one under accepted — plus understated Content-Length,
chunked transfers, multi-byte counting, repeated chunks after an overrun
responding only once, and the headers-already-sent case.
LOG_LEVEL, LOG_FORMAT and API_MAX_BODY_BYTES now go through the same loadConfig/validateConfig path as every other setting, so operators change verbosity the same way they change anything else. Invalid values are rejected rather than silently downgraded. A typo in LOG_LEVEL that quietly resolved to "info" hid the debug output an operator explicitly asked for, and gave them no signal the setting had not taken. Errors are collected with the rest, so a deployment with several bad values sees all of them in one message instead of fixing and restarting repeatedly. The message names the accepted values. Documentation correction: ENVIRONMENT_VARIABLES_AND_SECRETS.md listed http, verbose and silly as valid log levels. The service has only ever implemented error | warn | info | debug — the others were silently downgraded to info. Anyone who followed the table got behaviour that did not match it, and would now get a startup failure instead, so the table is corrected and the change called out in a note. Also documents LOG_FORMAT, API_MAX_BODY_BYTES and the redaction behaviour. response-time middleware now logs a sanitized URL. It already measured duration and included method/status/durationMs, but wrote req.url verbatim — and query strings routinely carry ?token= or ?api_key=, so every request was putting credentials into the aggregator. The path is kept, since that is what identifies a slow endpoint; only parameter values are withheld. Sanitization is a string scan on an already-parsed value, so per-request cost is unchanged in any way a timing measurement would show. Production defaults are unchanged: info verbosity, JSON output under NODE_ENV=production, 1 MiB body limit. 17 tests.
|
@Ike-Nathan Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
2 participants
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.
Overview
Related Issue
Closes #684
closes #685
closes #687
closes #688
Changes
Verification
How to Test
Checklist
maincargo fmt --allrun (if Rust changes)npm run lintpasses (if TypeScript changes)