feat: multi-profile routing with auth and admin protection#165
feat: multi-profile routing with auth and admin protection#165davetha wants to merge 8 commits intorynfar:mainfrom
Conversation
The health endpoint was calling getClaudeAuthStatusAsync() with no env overrides, checking ~/.claude instead of the configured profile's CLAUDE_CONFIG_DIR. In multi-profile Docker setups where only profile dirs are mounted, this always returned degraded.
|
Same here i'll review in the morning! |
rynfar
left a comment
There was a problem hiding this comment.
Review
Nice work on the overall design — clean module extraction, good test coverage structure, and fully backwards compatible. The profile routing and config loading are well thought out. A few things to address before this can merge:
🔴 Tests will fail
-
WWW-Authenticate realm mismatch — server sends
Basic realm="Admin"but the test inproxy-admin-auth.test.tsexpectsBasic realm="Meridian Admin". One needs to match the other. -
Landing page is not actually public — the README says
/ remains publicwhenprotectAdminRoutesis enabled, and the test"keeps the landing page route public"assertsstatus === 200with no credentials. Butserver.tsappliesapp.use("/", validateAdminAccess)whenprotectAdminRoutesis true, which will return 401 when API keys are configured. Either remove the middleware registration on"/"or change the test/docs.
The test plan checkboxes are all unchecked — please run npm test to verify.
🟡 Security
-
API key comparison is not constant-time —
auth.tsusesnormalizedKeys.includes(providedApiKey)which is a plain string comparison vulnerable to timing attacks. Same for the Basic Auth===comparison. For network-exposed auth, usecrypto.timingSafeEqual()with a fixed-length buffer comparison. -
No rate limiting on auth failures — might be fine for a private-network use case, but worth noting. Could be a follow-up.
🟡 Architecture
-
getProfileId()doesn't belong onAgentAdapter— thex-meridian-profileheader is a proxy-level (Meridian infrastructure) concern, not an agent-specific one. Every adapter would implement it identically. This should be read directly inserver.tsrather than forced through the adapter interface. The adapter pattern is for abstracting agent differences (OpenCode vs. future agents). -
resolveProfile()throws on unknown profile — if a client sendsx-meridian-profile: nonexistent, this throws into the generic catch handler and returns a 500 with a stack trace. Should be caught and mapped to a 400 with a clear error message.
🟠 Suggestions
-
"requested" vs "effective" profile — the distinction between these two isn't documented. If a session was started with profile A but the client now sends profile B, the cached profile silently wins. Should this warn or error on conflict?
-
storeSession()is at 7 positional params now — consider an options object to keep it readable. -
Silent
undefinedfor missing env references —"env:MISSING_VAR"silently resolves toundefined. ForapiKey/authToken, this will cause a confusing runtime error later. Consider validating and warning on startup. -
Minor indentation issue — the stale-session retry block in
server.tshas misaligned indentation after the edit.
|
Thanks for the feedback. I'll work on it as time permits. |
Summary
/healthand/telemetryCLAUDE_CONFIG_DIRinstead of the unmounted default pathUse Case
Run a single Meridian instance for multiple machines over a private network or Tailscale, routing requests to the right Claude profile from each client. Tighter control over who can see health/telemetry vs who can call
/v1/messages.Health Check Fix
The
/healthendpoint was callinggetClaudeAuthStatusAsync()with no env overrides, so it checked~/.claudeinstead of the configured profile'sCLAUDE_CONFIG_DIR. In multi-profile Docker setups where only profile dirs are mounted, this always returned "degraded" even though requests worked fine.Test plan
npm test— all tests passnpm run build— clean build/healthreturns healthyx-meridian-profileheaderSupersedes #164, #163, #162. Builds on #161.