Repository navigation
feat: give the dashboard a write API - #38
Merged
Merged
Conversation
The dashboard is read only. Changing anything means going back to a terminal. That is fine while a route is the only thing worth changing. It stops being fine when tunnels land and the config grows a relay address, an auth token and a share list. This makes it a GUI over the CLI for everything that does not need sudo. Routes, dashboard_port and the inspect settings become editable. Doctor gets a page. The frontend moves to a built SPA so tunnels have somewhere to land. Covers the origin model DESIGN.md deferred twice, the version guard that stops a click clobbering a hand edit, and why the privileged ports stay read only.
Plan 1 of 2 for the write surface. This one is the Go API only. The SPA that consumes it is plan 2, written after this lands, because its tasks depend on the shapes this one settles. Nine tasks, each ending green and committable. The order is deliberate: the version hash first, then the read endpoints, then the origin model, then the writes. The origin model lands before any config write exists and proves itself against handleInspectClear, which already mutates. A guard test that found no mutating routes would pass while proving nothing, so it fails instead.
A write API needs to detect that the file changed between the read a client rendered from and the write it sends back. LoadWithVersion returns the hash of the same bytes it parsed, so the guard cannot race the read it guards. Load keeps its signature and becomes a wrapper, so the daemon, the CLI and doctor are untouched.
doctor.Run is already a pure function over a config, a path and a data dir. The dashboard just did not know two of the three, so SetPaths tells it, the same way SetInspector already tells it about capture. The endpoint reads the config from disk rather than from s.cfg. Reporting a config that will not parse is half of what doctor is for, and s.cfg only ever holds one that parsed.
Doctor says whether the daemon is reachable. This says whether launchd knows about it, which is the difference between 'start it' and 'install it'. Reports unsupported rather than erroring on platforms where service.Status is not implemented. A Linux user has not hit a failure.
service.Status() can fail for reasons beyond platform unsupport. A broken home directory on macOS returns a plain error, not ErrUnsupported. Telling that user 'not supported on this platform' masks a real fixable problem. Extract the error classification into serviceStateFor() so both branches can be tested on any platform. Three outcomes: - ErrUnsupported: platform has no service manager, Supported: false - Other errors: platform has one but lookup failed, Supported: true - No error: return actual state, Supported: true
Sends both what the file says and what the daemon actually runs with. A settings form needs both: the effective value to show, and the file value to know whether a field is set or inherited. Those differ for every unset port. Inspect settings go through the accessors, because the zero value of every one of them means default and Enabled defaults to true.
Task 7's test expected a bare port to resolve to 127.0.0.1:5000. Route UpstreamAddr joins localhost, not 127.0.0.1, so the real value is localhost:5000. Only an explicitly set upstream comes back verbatim, so the neighbouring test that sets one is already right. Task 4 hit the same mistake in its own test and its implementer caught it. Fixing it here before Task 7 runs into it too.
…nfig readConfigView asserted only Effective and two Inspect fields, so Ports could be dropped, renamed, or echo Effective and every test would still pass. Add Ports to the shape, assert it stays zero when the file sets no port, and add a case with an explicit port to prove Ports actually reads the file rather than being zeroed. Also assert the remaining Inspect fields resolve to their package defaults.
sameOrigin accepted loopback at any port. That is right for reads, because 127.0.0.1:8484 is the only way in when the resolver or the CA is broken. It is wrong for writes: every dev server this proxy sits in front of is on loopback too, so one bad npm dependency would be inside the boundary. Writes now need the dashboard's own origin, or loopback at the dashboard port. A per-process CSRF token sits behind that as defence in depth. handleInspectClear is the only mutating route today, so it is what proves the machinery before any config write exists to use it. The guard test walks routes() and fails if a mutating entry skips the wrapper, and fails outright if it finds no mutating routes at all.
sameOriginStrict compared an origin's port against cfg.EffDashboardPort(). Start binds once, and a dashboard_port edit hot-reloads into s.cfg without rebinding anything, so from that moment the configured value names a port nothing is serving. The check would then 403 http://127.0.0.1:<old>, the URL that still works and the break-glass path ADR 0004 argues must keep working, and would accept writes from http://127.0.0.1:<new>, whatever process is listening there. The server now records the port off its own listener and the check reads that, so it is true by construction. A server that never started has bound nothing and matches no loopback origin; without that, an origin at port 0 would have matched an unset field. Also from review: the mutating route walk asserts that mutating implies guarded, since the two table walks skip on different fields and neither caught an unguarded write. The no-origin test case no longer claims browsers send form posts without an Origin. They send one on every method other than GET and HEAD, which is the fact the whole check rests on, and a comment saying otherwise is an argument for treating absence as neutral.
Writes go through the config file, not the running proxy, so the fsnotify watcher applies them exactly as it applies a hand edit. The GUI and a text editor behave identically, which is the property worth protecting. Every write carries the version of the file it read. A stale write gets a 409 carrying the current config, so the client re-renders in one round trip instead of two. The mutex serializes this process; the version is what covers the CLI, which does its own read-modify-write on the same file.
The lookup happens before the write helper runs, so a missing route is a 404 rather than the helper's blanket 422. That read is unlocked and can go stale, but the write re-checks under the lock and answers 409 if the file moved, so the worst case is a 404 for a route deleted microseconds ago. Setting a port clears the upstream and vice versa. Exactly one of the two may be set, and leaving both would fail validation on a field the user never touched.
Every patch field is a pointer, so omitting a field leaves it alone. A settings form sending one changed value must not reset everything else, and a plain bool cannot tell unset from false. The suffix and the three non-dashboard ports are refused with an error naming the command to run instead. They need sudo or a resolver rewrite. Accepting them and doing nothing would be worse than refusing, which is exactly what setting https_port does today under a launch daemon.
TestPatchConfigLeavesOmittedFieldsAlone seeded enabled at its default, so the assertion passed whether Enabled was preserved or dropped: the accessor reads nil as true either way. Add a test that seeds enabled = false explicitly, so a dropped pointer assignment reads back as a flipped value instead of hiding behind the default. Also add a test that a refused sudo-tier field aborts the whole patch, including a writable field sent alongside it, and that the file version is untouched. The control flow already does this; nothing proved it before.
Save only runs Validate. proxy.Load can fail for reasons Validate does not model, so a write can return 200 while the daemon keeps serving the old config. Making writes synchronous would not fix that, because a hand edit has the same problem and no request to block. So the reload reports itself instead. The dashboard compares the file version against the last one the daemon loaded and says whether they agree. This also surfaces a failed reload from a hand edit, which until now only reached the log.
Four rows in routes() are method scoped. The walk always issued GET, so a GET to "POST /api/routes" was answered by the sibling read row and the row under test was never entered. Deleting s.guard from any of the four left the walk green, which is the one mistake it exists to catch. The walk now builds the request from rt.method, defaulting to GET for a row that serves every method, and carries the write token so guard is proved to refuse a foreign Host even for a request that would otherwise pass mutate. It also asserts that a row serving anything other than GET is marked mutating. TestEveryMutatingRouteRejectsABadWrite walks that flag rather than the wrapper, so an unmarked write row was invisible to it. With the assertion the ADR's claim that a route cannot skip mutate by accident is true rather than aspirational.
config.Validate checked routes, the suffix and every inspect field, but no
port. PATCH /api/config {"dashboardPort":80} answered 200, wrote the file,
and the daemon then failed net.Listen and would not start. The dashboard
dies with the daemon, so the GUI could not undo its own write, and
restartRequired invited the user to trigger exactly that.
The split is deliberate. Validate rejects only 1-65535 violations, because
it runs on every load and a rule it applies retroactively could turn a
config someone already has into one nothing will read. The privileged range
is refused in handleConfigPatch alone, where the refusal is reversible and
the failure it prevents is not: a hand edit is undone by editing the file
again, a GUI edit that kills the dashboard is not undone at all.
Zero stays valid on both paths. It is the field unset, not a port.
Three separate ways the dashboard claimed a config was live when it was not. SetApplied fired between dash.Start and proxy.Load, so the dashboard answered applied:true while nothing was proxying. Usually milliseconds, but a first run mints and installs the CA inside proxy.Load and can block on a keychain dialog for as long as the user takes to answer it. The call now happens after proxy.Load returns. ensureInspector swallowed every failure as a log warning. The inspector's settings are one of the three things this branch made writable, so a user could enable capture, have inspect.Open fail, and be told the config was applied while the inspector endpoints answered 503. It now returns the failure as well as logging it, and both call sites feed that into SetApplied. Returning an error suited the function: it already had four early returns, each an outcome the caller wants, and the caller was already passing an error into SetApplied on the two neighbouring paths. The reload comment claimed the dashboard shows the last good config as stale when the file will not parse. It does not, because writeConfigView re-reads the file, so GET /api/config answers 500 until the file parses. That is the right answer and the comment now says so, along with the reason the 500 is not a dead end: /api/doctor reads the file separately and reports the parse failure.
Save wrote path+".tmp", a name shared by every process that calls it. The daemon's write mutex only serializes writers inside the daemon, and the version hash only catches a CLI write between a client's read and its write. Neither covers two processes saving at the same moment: both opened the same config.toml.tmp, both wrote from offset zero on their own descriptor, and the longer content's tail outlived the shorter one, so the rename published a file that was half of each. os.CreateTemp in the same directory gives each writer a name only it knows, so each rename publishes bytes only that writer produced, and the rename stays on one filesystem and therefore stays atomic. The temp file is removed on every path that does not reach the rename. The mode is restored to 0644 after the write. CreateTemp makes the file 0600, and tightening the permission on the user's config as a side effect of an unrelated fix is not this change's business.
/api/routes answered version: <build string> and /api/config answers version: <config file hash>. The second one is the token every write has to echo back, and a stale one is a 409. Two sibling endpoints in one package using one name for two unrelated values invites a client to read it off the wrong one and never be able to write again. Free today: the console fetches /api/routes for the rail and reads only .routes, and nothing else in the repo reads the field.
The write surface paragraph was wrong on all three of its clauses. It said the surface covers the suffix, which sudoTierRefusal refuses; it omitted dns_port, which is also refused; and it omitted dashboard_port, which is writable. Rewritten against the code, with the reason each refusal exists. Added a consequence for the one writable field whose edit does damage before the restart it asks for. proxy.Generate builds Caddy's dashboard upstream from the configured port while the dashboard's listener binds once at startup, so a dashboard_port edit 502s https://switchboard.<suffix> the moment the reload lands. applied stays true, correctly: proxy.Load succeeded. Generating the upstream from the bound port is the fix and it belongs to internal/proxy, so this records the interaction rather than widening this branch to reach into that package. The config view carries the same explanation. restartRequired is now accompanied by restartReason, which names the port mismatch, the URL that has stopped answering and the loopback URL that still works, so the banner tells the user what broke instead of only that something did. effectiveView's doc comment claimed it is what the daemon runs with. That is untrue for dashboardPort after an edit, which is the whole subject above, so it now says defaults resolved and names the exception. The by-construction claim about mutate is now accurate rather than softened. It held only if somebody remembered to set mutating: true, and the guard walk now asserts that flag for every non-GET row.
Adds the Unreleased section for this branch. The API has no UI consuming it yet, so the only thing an existing user can notice is the tightened origin check on /api/inspect/clear and the appVersion rename, both listed under Changed.
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.
The dashboard is read only. Changing anything means going back to a terminal. That is fine while a route is the only thing worth changing. It stops being fine when tunnels land and the config grows a relay address, an auth token and a share list.
This is the API half. There is no UI in here, so nothing looks different yet. The SPA that consumes it is a separate branch.
Writes go through the config file, never the running proxy. A handler saves and stops, and the existing fsnotify watcher reloads exactly as it does for a hand edit. That equivalence is the property worth protecting, and it is why there is no new apply path.
What is in here
GET /api/doctorservesdoctor.Runverbatim,GET /api/servicereports whether launchd knows about the daemon,GET /api/configreturns the config both as the file writes it and as the daemon resolves it. A settings form needs both: the effective value to show, the file value to know whether a field is set or inherited.dashboard_portand everyinspectsetting.sameOriginaccepted loopback at any port, which put every dev server this proxy fronts inside the boundary. Writes now need the dashboard's own origin, or loopback at the port the listener actually bound, plus a per-process CSRF token. Reads keep the permissive check, becausehttp://127.0.0.1:8484is the only way in when the resolver or the CA is broken.https_portdoes today under a launch daemon, and it is worse than refusing.appliedandrestartRequired.Saveonly runsValidate, andproxy.Loadcan fail for reasonsValidatedoes not model, so a write can return 200 while the daemon serves the old config. The reload reports itself instead. This also surfaces a failed hand edit, which until now only reached the log.Worth a reviewer's attention
The origin check originally keyed on
EffDashboardPort().Startbinds once and never rebinds, and adashboard_portedit hot-reloads into the config, so after such an edit the check compared against a port nothing was serving: the break-glass URL would 403 on writes at exactly the moment it is the only way in. It now keys on the port the listener actually bound, with an explicit rejection when that is zero.TestEveryGuardedRouteRejectsAForeignHostwas passing vacuously for all four new method-scoped routes. It always issuedGET, so those rows were never invoked and their guard could have been deleted with the suite green. That is the same failure the test's own comment recounts, along a new axis. It now drives each row with its own method, and asserts that any non-GET row is markedmutating, so the by-construction claim is actually true.A
dashboard_portedit still breakshttps://switchboard.testat reload, becauseinternal/proxybuilds Caddy's dashboard upstream from the configured port. Pre-existing, but this branch supplies the one-click trigger. Documented in ADR 0004 and surfaced inrestartRequiredrather than fixed here; generating that upstream from the bound port belongs in its own change.switchboard suffixis deliberately the repair path and reads a broken config leniently. Putting the port range check inValidatemeans a config broken in both the suffix and a port can no longer be repaired by that command. The port is unbindable either way and the error names the field, so this was accepted rather than worked around.Savenow chmods 0644 unconditionally whereos.WriteFilewas subject to umask. No secrets in the config today. Worth revisiting when tunnels add a relay token, along with a redaction pass onGET /api/config.Not in here
console.htmldeleted, is the next branch. DESIGN.md decision 7 still says "zero framework cost" and gets rewritten there, where the framework actually arrives.setup,uninstall,suffix,dns_port, and the privileged ports.internal/proxydashboard upstream fix described above.Design:
docs/superpowers/specs/2026-08-18-dashboard-write-surface-design.mdPlan:
docs/superpowers/plans/2026-08-18-dashboard-write-api.md