Review fixes (2 Important + 4 folded):
1. GET /admin now serves the HTML shell with no authentication -
a browser navigating directly to the URL cannot attach a custom
header. The page reads an admin key from sessionStorage and sends
it as x-api-key on /admin/api/* fetches, falling back to an inline
key-entry form on a missing key or any 401/403. Both JSON routes
keep their original 401/403/200 gate. Spec ss4 amended in place.
2. GET /admin/api/usage no longer serializes 6 GETs per user/group
scope on the shared Redis connection. respClient gains getBatch
(one pipelined round trip); counterStore gains getMulti; limiter
gains storeGetMulti mirroring storeGet's fail-open/fail-closed
policy. currentUsage now issues one getMulti per scope.
3. Provider baseUrl is stripped of userinfo and query before it is
ever echoed in the overview JSON (sanitizeBaseURL), stronger than
net/url's own Redacted (which leaves a bare username in place).
4. limiter.redisStatus now also returns the last failure's timestamp,
so the dashboard can render "last error (Ns ago)" instead of a
bare message that reads as currently-broken indefinitely.
5. Both JSON admin responses now set X-Content-Type-Options: nosniff,
Cache-Control: no-store, and the same CSP the HTML page uses.
6. Added a test proving a file-sourced (replaceFileUsers) admin user
gets the same access an inline admin user does.
go test ./... -count=1 -race, gofmt, vet, make yaegi-check all pass.
Adds GET /admin (single-page dashboard), GET /admin/api/overview, and
GET /admin/api/usage per spec §4: providers/redis/cache/groups summary
and per-user/group current-window usage+limits. Gated by Config.Admin
(default off, not registered when disabled) and UserConfig.Admin.
Gate order: disabled -> 404 fall-through; unauthenticated -> 401;
authenticated non-admin -> 403; admin -> serve, counted against the
admin's own request limits like any other route.
Adds small, read-only, mutex-guarded hooks: modelRegistry.snapshot(),
limiter.currentUsage()/redisStatus(), authStore.snapshot(). No secret
field (API key, digest, redis password) is ever read into any admin
response type; verified by a canary-literal redaction test.
HTML/CSS/JS ship as a single Go raw-string const (Yaegi forbids
go:embed); confirmed interpretable via make yaegi-check.
C1 (quota bypass): a client sending stream_options.include_usage=false
suppressed upstream usage reporting and escaped every token/cost budget,
since the non-streaming T12 estimate never covers a streaming response.
chatCompletion (provider_openai.go) now always forces include_usage=true
on the outgoing request, merged into a copy of the client's own
stream_options map so the caller's map is never mutated in place.
clientAskedUsage is derived from the client's own value via isTruthy, so
the usage-only chunk is still suppressed for a client that never asked
for it.
I1 (vacuous security tests): the X-Api-Key credential-strip set had zero
effective coverage — existing tests either never sent x-api-key, or
targeted anthropic, whose injectAuth overwrites x-api-key regardless of
whether the strip ran. Added one openai-passthrough and one MCP-proxy
test that authenticate via x-api-key itself and assert it arrives empty
upstream. Mutation-checked: both fail when "X-Api-Key": true is removed
from gatewayCredentialHeaders (verified, then restored).
I3 (auth-event logging): added Gateway.logAuthEvent (logger.go), called
at all six authStore.identify call sites (llmgateway.go's ServeHTTP,
routes_unified.go's handleModels). A failed attempt logs method, path,
and remote address; a success logs the resolved user's name and route.
Neither line ever includes the presented API key.
M2 (usage.estimated): runUnified now reads result.estimated and logs the
estimate explicitly when a non-streaming response's usage falls back to
the body-size heuristic, making the field genuinely read — removes the
stale //nolint:unused on limits.go's usage struct.
M8 (gosec consistency): secrets.go's file: secret path read now carries
the same "#nosec G304 -- operator-supplied file path from middleware
config" annotation as users_file.go's equivalent read.
Doc nit: mcp_a2a.go's validateTargetURLs comment now also names the
dot-only ("."/"..") rule validateConfigName enforces, not just character
set and reserved names.
Gates: go test ./... -count=1 (-race) green, gofmt -l . empty, go vet
clean, make yaegi-check OK, golangci-lint (gosec+staticcheck+unused+
errcheck+govet, global config) 0 issues.
Add tools/yaegi-check, a separate Go module (llmgw-yaegi-check,
never a dependency of the plugin module) that replicates the
Traefik Plugin Catalog analyzer (piceus)'s yaegiMiddlewareCheck:
copy the repo into a temp GOPATH, interp.New + i.Use(stdlib.Symbols),
import the plugin package, eval CreateConfig(), decode the plugin's
real .traefik.yml testData into it via a hand-rolled YAML-subset
parser plus a JSON round-trip, reflect-Call New from outside the
interpreter, then exercise the returned handler with two real HTTP
requests (authenticated GET /v1/models against the testData key, and
the same route unauthenticated). Pins github.com/traefik/yaegi to
v0.16.1, confirmed as Traefik v3.5.0's own vendored version by
downloading its go.mod.
Wire cd tools/yaegi-check && GOWORK=off go run . /Users/nvm/Documents/projects/private/traefik-llmgateway
yaegi-check: OK to run the harness from repo root.
The harness caught a real Yaegi v0.16.1 limitation: calling a struct
FIELD of variadic func type (e.g. modelRegistry.log
func(string, ...any)) with two or more variadic arguments crashes
Yaegi's CFG builder ('index out of range'). The identical call shape
through a method or interface method is unaffected — bisected with a
standalone yaegi repro to confirm the exact trigger before touching
plugin code. Fixed by pre-formatting with fmt.Sprintf at
modelRegistry's three affected call sites (registry.go) so the field
is always called with exactly one variadic argument; documented the
constraint on both modelRegistry.log and limiter.logf so it is not
reintroduced. Verified the gate itself is load-bearing by temporarily
renaming CreateConfig (harness failed non-zero) and restoring it
(harness passed again).
Also fixes two golangci-lint findings in the new harness module
(govet shadow, gosec G301 directory permissions).
Review fixes for Task 12:
1. (CRITICAL) runUnified now calls g.limiter.account BEFORE handling an
adapter error, not after returning on it. All three adapters'
forwardStream deliberately return captured usage alongside a mid-stream
error, so a client that aborts right after the usage frame arrives was
getting tokens delivered but never billed - repeated aborts bypassed
every token/cost budget. A non-streaming error path always carries zero
usage by contract, so accounting it is a no-op (account skips writes
once both total tokens and cost are zero). The zero-usage-stream logf
and the zero-usage-non-stream estimate now only fire when callErr is
nil, so a genuine error no longer gets a fabricated token estimate.
New test: TestHandleChat_MidStreamDropAfterUsageChunk_AccountsPartialUsage
(hijack+close after the usage chunk; asserts the limiter's counter shows
the captured tokens). Verified this test fails against the pre-fix
ordering and the two new alias tests below fail when their guarded
delete() call is removed, before restoring and re-running clean.
2. Added TestHandleChat_OpenAI_AliasKeyStrippedFromUpstreamBody and
TestHandleEmbeddings_OpenAI_AliasKeyStrippedFromUpstreamBody: the
openai-type adapter marshals req verbatim as the upstream wire body, so
it is the one path where a leftover gatewayAliasKey entry actually
reaches a real provider. The only existing alias-leak assertion covered
anthropic, which rebuilds the body field-by-field and was never at risk.
3. Reworded the stale comment on limiter.evaluateScope (limits.go): it
described a nil-limits scope's req counters as a normal, expected case.
Ruling (e), implemented in routes_unified.go's buildLimitScopes, means
the unified route never builds a nil-limits scope in the first place -
the nil check remains a defensive no-op, not a path any current caller
exercises.
Gates: gofmt, go vet, fieldalignment, golangci-lint (incl. gosec) all
clean; go test ./... -race -count=1 passes (full suite, including all
Task 12 tests).
Reject a nested array reply instead of recursing into it. decodeReply
now takes a depth parameter; a '*' type byte is accepted only at
depth 0, so an element that is itself an array (decodeArray calls
decodeReply at depth+1) is a protocol error rather than a recursive
call. A crafted "*1\r\n*1\r\n..." stream repeated could otherwise
recurse to Go's stack limit — a fatal, unrecoverable error, not a
panic recoverPanic can catch — from only a few megabytes of wire
input. Also lower respMaxArrayLen from 1<<20 to 1024: none of this
client's commands (AUTH, SELECT, INCRBY, EXPIRE, GET) ever return an
array reply at all, so even 1024 is generous headroom, not a limit
tuned to real traffic.
Add a store-down latch to the limiter (limits.go): after any store
operation fails, record the failure time; for storeDownLatchFor (5s)
afterward, every storeIncrBy/storeGet call short-circuits straight to
the fail-open/fail-closed policy without attempting the store at all.
One request can touch several counters (its own and its group's,
request/token/cost, day/month) — each respClient call is already
bounded to ~respCallTimeout on its own, but without the latch every
one of those still paid a fresh bounded wait against a store that had
just proven unreachable. After the window, the next operation probes
the real store again: a natural half-open retry, no separate recovery
state. Guarded by the same logMu mutex lastLogAt already uses for this
kind of small timestamp state.
Clamp respClient's dial timeout to the remaining call budget:
dialTimeoutFor returns min(respDialTimeout, time.Until(deadline)),
with a respDeadlineExceededErr (implements net.Error, Timeout() true)
returned outright if the deadline has already elapsed — e.g. this
call queued behind the client's mutex for its whole budget while an
earlier call was stuck. Without this, a future increase to
respDialTimeout alone, leaving respCallTimeout untouched, could
silently let one dial attempt consume more of a call's total budget
than the call-level deadline is supposed to allow.
New tests: a nested-array reply and an array length one over the new
1024 cap, both asserted as protocol errors; a countingErrorStore stub
proving a second checkAndCount within the latch window makes zero
additional store calls, and that the store is probed again once the
window elapses. Tightened TestLimiter_FailOpenTrue_
HungRedisServer_FallsBackQuickly's bound from <6s to <3s now that the
latch collapses its two store ops into one real network attempt.
Add resp.go: a minimal stdlib-only RESP2 client (respClient) for a
single Redis-compatible server. It lazily dials one mutex-guarded
connection, applies 2s dial/read/write deadlines, sends AUTH (if a
password is set) then SELECT on every new connection, and reconnects
once on any I/O error before giving up. do and pipeline encode
commands as "*N\r\n$len\r\n..." and decode +/-/:/$/* replies.
Add redis_store.go: redisStore implements counterStore over
respClient. incrBy pipelines INCRBY and EXPIRE in one round trip and
returns INCRBY's value; get treats a missing key (RESP null bulk) as
0.
Wire the limiter's fail-open/fail-closed policy in limits.go, which
Task 5 stored but never read: a configured store's error now routes
through storeIncrBy/storeGet. failOpen=true falls back to the
in-process memoryStore transparently; failOpen=false makes
checkAndCount return a storeDown violation and makes account drop the
sample. Both paths log through an injectable, 30s-rate-limited logf.
Wire newGateway (llmgateway.go) to build a redisStore-backed limiter
when Config.Redis is set (password through resolveSecret, failOpen
defaulting to true unless RedisConfig.FailOpen says otherwise) and a
memory-only limiter otherwise, and to route pricing.go's unknown-model
warnings through the gateway's own logf.
Validate LimitsConfig at construction: a new LimitsConfig.validate
rejects a negative int64 count limit or a negative/NaN/Inf cost limit,
called from newAuthStore for every group and every user (inline and
file-sourced) so a malformed limit is a constructor error instead of
silently behaving as unlimited.
Add RequestsPerDay, TokensPerMonth, and CostPerMonthUSD enforcement
tests alongside the existing per-minute/day/cost-per-day cases, plus
an erroringStore-backed test pair proving the fail-open (fallback
counted) and fail-closed (storeDown violation) branches directly.
Folded the validation work into this single commit rather than a
second one: it interleaves with the fail-open changes across
limits.go, limits_test.go, and llmgateway_test.go closely enough that
splitting by hunk would be artificial.
lukaszraczylo