feat: Log one access line per request - #19
Merged
Merged
Conversation
The proxy was silent between startup and shutdown, so there was no way to see what happened to a request. It now logs method, path, status, and latency per request through a middleware that wraps the handler, so no route had to change. The status-capturing wrapper still exposes http.Flusher, so SSE streaming keeps flushing. Headers and bodies are never logged, keeping credentials and prompts out of the log. Successful /health probes are skipped, since the container healthcheck hits them every few seconds. LK_LOG=off silences the per-request lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Sanitize the method and path before logging. A decoded request path can carry control characters, so a crafted path could otherwise forge log lines or inject terminal escapes. Values with control characters are now quoted. - Record only the first WriteHeader, matching net/http, so the logged status is the one the client received. - Log from a deferred call, so a panicking handler still leaves an access line. - Extract requestLoggingEnabled so the LK_LOG semantics are unit-tested, and document that only "off" disables logging. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The path already determined the protocol, but reading it off /v1/messages versus /v1/chat/completions took a second look. Each access line now leads with [openai], [gemini], or [anthropic] (or [-] for a non-API path), from a protocolLabel helper that mirrors the routing in ServeHTTP. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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.
Summary
WithRequestLog) that wraps the handler, so none of the ~12 routes had to change. AstatusRecordercaptures the status while preservinghttp.Flusher, so SSE streaming still flushes.LK_LOG=offsilences the per-request lines (startup lines stay). Onlyoffdisables it;0andfalseleave it on, which the README notes.main. Successful/healthprobes are skipped, since the container healthcheck hits them every few seconds; a failing/healthstill logs.Review found a real security issue, now fixed
Three reviews ran. The notable catch (from the Codex pass, missed by the other two): log injection.
r.URL.Pathis the decoded path, so a request to/x%0A...would put a raw newline or ANSI escape into the log line, letting anyone who can reach the port forge log entries or inject terminal escapes (CWE-117). Fixed: any method or path containing a control character is quoted before logging. Also hardened from review: only the firstWriteHeader/implicit-200 is recorded (so the logged status matches what the client got), and the line is emitted from adeferso a panicking handler still logs.Deliberately out of scope
http.Hijacker/io.ReaderFromare not forwarded by the wrapper. localaik is HTTP + SSE only, with no hijack or upgrade path, and all three reviews confirmed zero current or perf impact (theio.Copyfast path is already unreachable in production because the real writer is always aFlusher). Addingnet/bufioplumbing for unused interfaces would be speculative complexity. Noted here as a conscious YAGNI omission, trivial to add if a future feature ever needs it.RedactUpstreamkeeps a path-based credential in the upstream URL (a pre-existing edge in already-merged code, unrelated to logging). Tracked as a separate follow-up.Test Plan
make lintclean, all unit tests pass. 13 middleware tests cover the access line, error-status text, implicit and explicit status capture (first wins), Flusher preservation, health skip (success suppressed, failure logged), no-header-leak, control-char sanitization, and the nil-logger disable path.requestLoggingEnabledis table-tested.docker_integrationProxyImage test passes.%0Apath logs as a single quoted line with the newline escaped (no forgery);/healthspam is suppressed;LK_LOG=offsilences per-request lines while startup lines remain.🤖 Generated with Claude Code