Repository navigation
Conversation
The UI is served by the binary itself (server-rendered templates + a
vendored htmx), so there is no Node toolchain and no asset directory to
deploy. It binds to loopback, exchanges a one-time URL token for a
SameSite=Strict cookie, and refuses cross-site writes.
Editing would be dangerous with the existing writer: config::load_config
expands ${VAR} before parsing, and manager::add/remove re-serialise the
whole document, dropping comments. src/config_source.rs exists so that
neither happens: it keeps the raw bytes, edits through a CST, and writes
atomically (tmp + fsync + rename, 0600, timestamped backup) only after
the document validates and its hash still matches the one the form was
rendered from. Field-level patches leave comments and placeholders in
the untouched parts of an entry intact.
Screens: overview, servers (add/remove/edit), tool catalog, playground
with a form generated from inputSchema, audit trail, ACL with a
simulator that calls the proxy's own evaluator, and settings with
per-instance reload.
Adds a guide covering the editing model (plan/apply, atomic write, 409 on concurrent change), the browser threat model, and the limits that matter: the ACL is not enforced locally, and an audit trail needs auditing enabled.
`mcp ui` aborted on the first `db::create_pool` error. On platforms where ChronDB has no native library for the release (macOS x86_64, for one) that meant the UI could not start at all, even though only two screens need the database. The rest of the binary already degrades here (`mcp logs` warns and continues); the UI now does the same: disabled pool, a warning on stderr, and a banner that says what is off. Discovery would have been useless in that mode because the tool list was only read back from the database, so UiState keeps what it discovers in memory for the session and uses it as an overlay. Overview now reports the audit sink as "on · sink unavailable" instead of "on" when the config asks for audit but the database is down.
`mcp serve --http … --management` mounts the same pages at / and swaps the local session guard for the proxy's own auth. The sign-in form validates a token through the configured provider, stores it in an HttpOnly SameSite=Strict cookie, and every request re-authenticates — revoking a token signs the browser out on the next click, with no session table to keep in sync. The viewer decides three things: - the catalog is filtered by the same ACL `tools/list` uses, so the UI cannot show a tool the same call would be denied; - the activity trail is limited to the viewer's own entries unless they hold an admin role; - writes need a role from `serverAuth.adminRoles`. Access and Settings are admin-only, and readers do not see host instances or the config path. Writes reload in process. Going through the instance registry would have meant flock-ing a file this process already holds: the registry prunes what it cannot lock, so the UI would have unregistered its own proxy and no reload would have happened.
Covers the flags, the role matrix, per-request token validation, the in-process reload, and the two ways to get this wrong: enabling the UI without provider (everyone is anonymous and read-only) and using the forwarded provider without a reverse proxy in front.
A container binds 0.0.0.0 and is reached through a published port, so the startup line printed http://0.0.0.0:7332/?token=… — not something anyone can open. Print localhost:port instead (the bind address is still logged), for both `mcp ui` and `serve --management`. Also documents running the UI in Docker, including the two things that bite: the default loopback bind is unreachable from the host, and the image ships with audit off, so Activity is empty until MCP_AUDIT_ENABLED=true and a real volume are supplied.
Clicking "Connect now" on a stdio backend whose binary is absent (the scratch Docker image has no npx) failed only after the spawn attempt. The UI now runs the same preflight `mcp --list` uses and shows the reason on the server page, disables the button, and refuses a hand-crafted discovery POST with the same message. Small but pointed: the detail template defaults `available` to true, so a missing context key can no longer make an HTTP backend claim it cannot start — which is how the first version of this change behaved.
The Access page was a read-only view. It now performs the two edits that actually change who has access: mint/revoke a token, and change the roles a subject holds (plus create a role by copying an existing grant list). Three rules exist because the alternative locks people out: - a new token is displayed once, on a page reached by an unguessable id that is consumed on first view — the config keeps the token as a map key and the UI only ever shows its fingerprint; - revoking the token you are signed in with is refused, otherwise the next click signs you out mid-action; - the last admin *credential* is protected. A role 10BC8 on a subject does not count: if no token maps to that subject, nobody can present it. When identities come from forwarded/oauth_as the guard steps aside, since they cannot be enumerated here. Role grants are deliberately not editable: creating a role copies one, changing grants stays a file edit. Writes reuse the plan/apply path — atomic, backup, 409 on a moved file, audited with the admin's subject.
Every colour now comes from a custom property, so a theme is one block of values rather than a second stylesheet. The light palette is not an inversion: the accent darkens (#0b8f68) to keep contrast on white, and the semantic colours (warn, error, read) get their own values — the dark ones are unreadable on a light panel. The page follows prefers-color-scheme until the reader picks a theme. The topbar toggle cycles auto → light → dark, the choice goes to localStorage, and theme.js runs in <head> so the palette is right before first paint — otherwise every light-mode reload flashes dark. The login and one-time token pages carry the toggle too: they are the first thing an operator sees, and a token page that flips to dark when you sign in looks broken.
White on #0b8f68 measures 4.08:1, below the 4.5 minimum for normal-size button text. #0a7a58 measures 5.33:1 and still reads as the same green.
The how-to explained the published tags but never how to build the image they correspond to. That gap is what turns a correct config into `command not found in PATH: npx`: `docker build .` picks the last stage, which is scratch. Adds a stage table (default/scratch, full-source, release/full, builder), spells out why --target matters, and shows running the UI from a container with the bind and env-var details that the image requires.
302 MB on alpine + node, measured from a clean build; the published :full tag is a separate artifact, so the two numbers are not the same thing.
There was a problem hiding this comment.
Great work on this. The CST-based surgical JSONC editing (ConfigSource), optimistic locking, and keeping everything embedded in the binary without a JS build step fit the project really well.
Before merging, there are a few bugs and security edge cases we need to fix:
-
Literal secrets are rendered unredacted on
/servers/{name}/editand in/servers/{name}/plandiffs (src/ui/handlers/servers.rs:249, 278, 452-509,src/ui/data.rs:423-453)- The security model in
src/ui/mod.rs:14-15and the docs state that literal secrets inheadersandenvnever reach the template and are replaced with•••• (redacted). - While
detailcallsdata::redact(&raw),edit_formpasses the unredactedrawvalue toform_view(&name, &raw), putting plaintextheadersandenvvalues into the<textarea>fields inedit.html. - Similarly,
planrunsdata::line_diff(&source.raw, &proposed)against the unredacted config, exposing literal secret values in the diff preview (including context lines). - Please redact literal secret values in
form_viewandline_diff, and teachEditForm::apply_to(src/ui/data.rs:478-504) to preserve the existing value fromcurrentwhen a header/env key is submitted unchanged with the•••• (redacted)sentinel.
- The security model in
-
Role deletion in
/accessis broken in both the handler and the template (src/ui/handlers/access.rs:248-256, 528-532,src/ui/templates/access.html:53)- In
delete_role(src/ui/handlers/access.rs:528):if subjects_using(&source, &name)?.iter().any(|s| s == &name) {
subjects_usingreturns the list of subject names holding the role, sos == &namechecks whether a subject has the same name as the role instead of checking!assigned.is_empty()(and the error message formats{name}twice instead of listing the assigned subjects). - In
acl_view(src/ui/handlers/access.rs:248-256), the RBAC branch setssubject_count(a formatted string) but never setssubjects(the numeric count used by the legacy branch on line 197). Because of that,r.subjects > 0insrc/ui/templates/access.html:53is always false for RBAC configs and the Remove button is never disabled for roles in use.
- In
-
same_siterejectsOriginwhenmcp uiis bound to a non-loopback IP (src/ui/session.rs:95-129)host_is_localallows reachingmcp uivia a non-loopback IP literal when--addris used, butsame_sitehardcodes["127.0.0.1", "localhost", "[::1]"]when checking theOriginheader. AnyPOSTrequest from a browser over a non-loopback IP fails with403 Forbidden: cross-site write refused.host_is_localalso pushes any IP literal fromHostintoallowedeven whenboundis loopback. Both checks should derive the allowed non-loopback host frombound.
-
stdio/cliserverargsare joined with spaces and split withsplit_whitespace()on edit (src/ui/handlers/servers.rs:463-472,src/ui/data.rs:487-495)- Opening
/servers/{name}/editon an existing server whoseargsarray contains an element with whitespace (e.g. a path with spaces or--header "Authorization: Bearer ...") and saving any field will split that argument on whitespace and corruptargson disk. Sinceshell-wordsis already inCargo.toml, please useshell_words::join/shell_words::split(or preserve the original array when theargsfield was not modified).
- Opening
-
Minor team-mode and UI polish:
src/serve/team.rs:146-163(credentials_from): the__mcp_authcookie is inserted intocredsasauthorizationbefore request headers, so a stale cookie shadows a validAuthorizationheader. Inlogin_submit(src/serve/team.rs:107-126), request headers shouldn't allow aforwardedprovider to mint a__mcp_authcookie holding an arbitrarytokenform value.src/ui/handlers/servers.rs:22andsrc/ui/handlers/playground.rs:76-83: scopeaudit_snapshotand PlaygroundRecent runstoown_identitywhen!viewer.admin(matchingoverview::indexandactivity::index), and use{{ subject }}instead of hardcodedidentity: localinsrc/ui/templates/playground.html:96.src/ui/templates/access_token.html:36: says "The config now holds only a fingerprint", whereas the config file stores the token as a key inserverAuth.bearer.tokensand only the UI displays the fingerprint (as noted indocs/guides/web-ui.md:176).src/ui/templates/settings.rs/src/ui/handlers/settings.rs:20-27:query_filteredwithlimit: usize::MAXloads and deserializes the entire audit table into memory on everyGET /settingsjust to count entries.
Both leaked, and both are worse than they look: the form is the one place
a secret was handed to the browser in full, and a diff is a preview that
gets screenshotted into tickets.
- form_view now shows the redaction sentinel instead of a literal header
or env value (`${VAR}` references stay, they are not secrets);
- EditForm::apply_to reads the stored value back when the field comes
back exactly as the sentinel, and refuses a value that mixes typed text
with the placeholder rather than writing it into the config;
- the plan diff is computed on the raw bytes (it has to be, or comments
would vanish) and then sealed: every literal secret from either side of
the change is replaced with the sentinel before escaping.
Also fixes the argument round-trip in the same path: args were joined
with spaces and split on whitespace, so an argument containing a space
(`--header "Authorization: Bearer …"`) was corrupted by any save, even
one that never touched it. It now uses shell-words in both directions and
keeps the array untouched when the field was not edited.
Two bugs in one guard. The check compared each *subject name* against the role name, so it only refused when a subject happened to be called like the role; and the error formatted the role name twice instead of listing the subjects. The in-use count the template reads was also never set on RBAC rows, so the Remove button stayed enabled for roles that could not be removed. It now carries the numeric count the disabled condition expects, with the display string kept alongside it. The guard is a function so it can be tested without a router: unused role, role in use, role in adminRoles.
Binding to a non-loopback address (a contain F438 er, a LAN address) made every browser POST fail with 403: host_is_local accepted the IP the browser used, then same_site rejected the Origin because it only knew the loopback names. The two checks now share one source of truth: - Host is accepted when it is loopback, or the bound address, or — when bound to 0.0.0.0/:: — any IP literal with the right port. A domain name is never accepted, which is what the rebinding guard is for. - Origin is compared to the Host header, which host_is_local has already vouched for, instead of a hardcoded list.
The login form also fed request headers into the provider chain, so a deployment that trusts x-forwarded-user would authenticate on the header and then store whatever string the form submitted as the session cookie. The form's token is now the only credential considered, and an anonymous identity is refused outright: with no provider configured every token "validates" as anonymous, and a cookie for nobody is worse than a clear error.
…oding them Three smaller things the review caught, plus one copy fix: - the servers list and the playground's recent runs were global reads in team mode; a reader now sees their own counts and their own runs, the way Overview and Activity already did; - the playground footer said "identity: local" on every deployment — it now shows the authenticated subject; - `GET /settings` counted audit entries with a `limit: usize::MAX` query, which deserializes every entry in the table on every page load. AuditLogger gained `count()`, which reads the key list and stops there (ChronDB has no count API, so the read remains O(n), but the decode no longer happens); - the one-time token page claimed the config stores only a fingerprint. It stores the token as the key; the fingerprint is only how the UI lists it.
|
Thanks — every item was real, and two of them (the secret in the form, the corrupted args) would have bitten in normal use. Fixed in five commits; Secrets in the edit form and the diff —
|
Credential storage (SEC-18)
- auth.json and auth_server.json were written with std::fs::write, which
respects only the umask: 0644 on a normal host, readable by every local
user. Both now go through src/secure_file.rs (0600 file, 0700 directory)
and the proxy warns at startup when an older file is still permissive.
Sign-in (SEC-03, SEC-05, SEC-13)
- POST /logout, same-origin checked; the GET version could be triggered by
any page.
- sign-in is rate-limited per peer address (10/min) and failures are
audited as auth/failure, like the /mcp path already did.
- the bind refusal no longer claims to be about a TLS this binary does not
speak, and --cookie-secure lets a TLS terminator that does not forward
X-Forwarded-Proto still get a Secure cookie.
Identity (SEC-19)
- providers=["forwarded"] on a non-loopback address now warns at boot:
those roles can rewrite the config through the management UI.
Browser (SEC-01, SEC-07, SEC-10, SEC-11, SEC-16, SEC-17, SEC-21)
- every inline handler and the inline script are gone; a CSP with
script-src 'self' now backs that up. The stored-XSS proof (a subject named
x'+(alert(1),'')+') is reduced to a literal confirm message.
- the startup token is single-use and expires in ten minutes; it is
exchanged for a session token, so a URL that leaks is worthless after the
first visit. Sessions are capped, and comparisons are constant-time.
- the UI no longer offers write buttons to a reader whose requests would be
refused, hides backends the reader has no tool on (404 on their detail),
and shows internal error chains only in the log (with a request id).
- db errors, and the paths in them, are shown to admins only.
Secrets (SEC-06, SEC-20)
- credential-looking query parameters and --token-style args are redacted.
- the plan diff seals secrets from the whole document, not just the edited
entry, and matches the quoted JSON form so short values and values with
escapes are covered too.
Writes and audit (SEC-08, SEC-09, SEC-22)
- config writes take an flock on a sibling lock file across read-hash-rename.
- audit reads iterate newest-first and stop at the limit instead of
deserializing the whole table.
- the legacy SSE session map is capped.
Footguns (SEC-04, SEC-23)
- mcp ui requires --insecure for a non-loopback bind, like serve.
- an unset ${VAR} still expands to an empty string, but now says so once.
Single-use startup token, --insecure for the UI, POST /logout, CSP, the reader's filtered view, and the --cookie-secure flag. The Docker examples now include --insecure, which the non-loopback bind requires.
|
All review findings are addressed in Credentials — XSS — every inline handler and the inline Startup token — single-use, expires in 10 minutes, exchanged for a session token held in the server's set; a URL that leaks is worthless after the first visit. Comparisons are constant-time, sessions capped, pending token pages expire. Team mode — sign-in is rate-limited per peer (10/min, verified: 9×401 then 429), failures are audited as Authorization surface — a reader no longer sees write buttons (the UI's Secrets — credential-shaped query params and Writes/limits — config writes take an Footguns — Not done, deliberately: |
What this adds
mcp ui: a web UI served by the binary itself, plus a team mode where the proxy serves the same pages behindserverAuth.No Node, no bundler, no asset directory to deploy: server-rendered templates (minijinja) and a vendored htmx are compiled into the binary.
cargo buildstays the only build step.Screens: Overview (backends, cache, 24h calls/denials, instances), Servers (add from registry/URL/stdio, remove, detail), Edit (diff → validate → atomic write → reload), Playground (form generated from
inputSchema), Activity (audit trail with the ACL decision and matched rule), Access (roles, subjects, tokens, simulator), Settings.Why the writes are the interesting part
The existing writer could not be reused:
config::load_configexpands${VAR}before parsing, so writing the loaded value back would bake in secrets or blank the placeholders out;manager::add/remove/updatere-serialise the whole document, dropping comments and reordering keys.src/config_source.rskeeps the raw bytes, edits through a CST (jsonc-parser), and only writes after the result validates:mcpuses at load time;409if it moved;tmp+fsync+ rename),0600, timestamped backup;{ // kept warm on purpose: sentry is the busiest backend "mcpServers": { "sentry": { "headers": { "Authorization": "Bearer ${SENTRY_TOKEN}" }, "idle_timeout": "adaptive" // the proxy should keep it } } }Editing
idle_timeoutchanges one line; both comments and${SENTRY_TOKEN}survive. A bug found exactly here while testing: the form read from the loaded config and turnedBearer ${SENTRY_TOKEN}intoBearer.Team mode
--managementmounts the UI at/and replaces the local session with the proxy's own auth: a token validated by the configured provider chain, kept in anHttpOnly; SameSite=Strictcookie and re-validated on every request (revoking a token signs the browser out on the next click, with no session table).adminRoles)Reload in team mode is in-process. Going through the instance registry would mean
flock-ing a file this process already holds, and the registry prunes what it cannot lock — the proxy would unregister itself and nothing would reload. That was the first integration test that failed.Managing people
Admins can mint/revoke tokens, change a subject's roles, and create a role by copying an existing grant list. Three guards, all tested: a new token is displayed once (unguessable id, consumed on first view, only a fingerprint afterwards); you cannot revoke the token you are signed in with; and the last admin credential is protected — a role on a subject does not count, because without a token nobody can authenticate as it.
Also in here
prefers-color-schemeuntil the reader picks one; toggle in the topbar.docs/howto/docker.mdgained a "building the image yourself" section, sincedocker build .picks the last stage (scratch) and a stdio backend then fails withcommand not found in PATH: npx.mcp --listruns and disables Connect now instead of failing after the click.mcp uistarts even when the audit database cannot be opened (no ChronDB native library for the platform), with a banner explaining what is off. Discovery results are kept in memory for the session.Security
Loopback bind, 48-char token in the URL exchanged for an
HttpOnly; SameSite=Strictcookie,Hostrestricted to loopback (DNS rebinding),Origin/Sec-Fetch-Sitechecked on POST, no CORS, no CDN, secrets redacted server-side before rendering (any literal value that is not a${VAR}reference becomes•••• (redacted)), and every write audited with the real subject.Tests
cargo test: 1036 passed.cargo clippy -- -D warningsandcargo fmt --checkclean. Manual smoke against a running proxy (login, per-role ACL, apply + reload, token/subject/role management and their guards) and against both Docker images.Not in here
/api/v1: the surface is the UI and the/mcpendpoint.Reviewing
The commits are ordered and each one builds on the last: config source → UI → team mode → people → themes → docs. Happy to split this into separate PRs if you would rather review it in pieces.
The design write-up and the static prototype live in the fork: lichti#1