feat(web): rebuild the dashboard UI and the API it runs on - #797
feat(web): rebuild the dashboard UI and the API it runs on#797KamilPesek wants to merge 13 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
github-actions
Bot
added
documentation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (77.12%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #797 +/- ##
==========================================
- Coverage 89.33% 89.19% -0.15%
==========================================
Files 88 91 +3
Lines 12220 12314 +94
==========================================
+ Hits 10917 10983 +66
- Misses 1003 1018 +15
- Partials 300 313 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
CybotTM
left a comment
CybotTM
left a comment
There was a problem hiding this comment.
Thanks for this — it is a lot of work, and the five bug fixes carried along with the UI are the more valuable half. Reviewed at 5d88df8. Copilot is out of review quota this month and the automated Claude review is disabled for fork PRs, so this is a manual review.
What I verified
Checked out the head, built and ran the suite locally: go build ./... and go vet ./... clean, gofmt -l . empty, go test ./web/... ./core/... ./cli/... ./config/... all packages ok (e2e not run locally). All five claimed bug fixes were traced in the code: the CopyLogs branch on info.Config.HostConfig instead of info.Config.Tty, the origin rewrite that unlocked deleting a label job, the now-projected nextRuns, the resumed paused job, and the config stripping.
Breaking change
The "Breaking change" box is unchecked, and three changes here are breaking:
core.DockerProvidergainsCopyContainerLogs— any external implementation of the exported interface fails to compile. The PR description already names this; the checkbox contradicts it.POST /api/jobs/updatereturns 403 for INI- and label-owned jobs where it previously returned 200. A script that edited label jobs through the API stops working. It being a security fix does not change the classification.Scheduler.UpdateJobnow updates disabled jobs instead of returningErrJobNotFound(core/scheduler.go:733) — exported method, changed behaviour.
recentRuns and /api/dashboard are purely additive, and the compression is negotiated with Vary: Accept-Encoding, so neither of those is a break.
In the CHANGELOG the update gate currently sits under ### Added; it belongs under a breaking/changed heading.
The stripJobs justification does not hold
The description says the old field-name list "named a field that no longer exists and missed ComposeJobs, so compose job definitions shipped to every reader of /api/config". The list it replaced was RunJobs, ExecJobs, ServiceJobs, LocalJobs, ComposeJobs. All five of those fields exist in cli/config.go:96-100, and they are the only map[string]*struct fields in Config, so the old code stripped exactly what the new code strips. The leak described here did not happen, and /api/config output does not change in this PR.
The refactor itself is fine — matching by shape is more robust against a collection added later, which is a good reason on its own. But it is presented as a security fix, and the same wording is in the commit message and will end up in the release notes. Please reword it as the hardening it is.
The compression is not gzip-only
gzhttp.NewWrapper defaults to zstdEnabled: true and preferZstd: true (gzhttp/compress.go:591-599 in the pinned v1.19.2), and on equal q-values zstd wins. Probed against the middleware as written in this PR:
Accept-Encoding: "gzip, deflate, br, zstd" -> Content-Encoding: "zstd" 41 bytes
Accept-Encoding: "gzip, deflate, br" -> Content-Encoding: "gzip" 66 bytes
Accept-Encoding: "gzip" -> Content-Encoding: "gzip" 66 bytes
Chrome, Edge and Firefox send the first header, so they get zstd; Safari below 26 falls back to gzip, which is exactly what the wrapper is for. The behaviour is good. What does not match it is the naming and the documentation written in this same PR: web/gzip.go, gzipMiddleware, web/AGENTS.md:47, docs/packages/web.md:278 and the CHANGELOG entry all say gzip. And web/gzip_test.go:33 sends gzip, deflate, br and asserts gzip, so the encoding real browsers actually receive has no test at all.
Please fix in this PR
- Sign the commit.
git commit -S— the commit isverified: false, reason: "unsigned"and the branch ruleset hasrequired_signatures, so this cannot merge as it stands. DCO sign-off is present and green. - Narrow the rate-limit exemption to
/liveand/ready./healthis auth-exempt (server.go:269) andHealthChecker.GetHealthcallsruntime.ReadMemStatson every request (health.go:297), which stops the world. Exempting it from the limiter creates an unauthenticated, unthrottled endpoint that pauses the GC per call and reports version and goroutine count. Orchestrator probes are served by/liveand/ready. - Check the breaking-change box, move the update gate to a breaking section in the CHANGELOG, and mention the
DockerProvideraddition there. - Reword the
stripJobsrationale in the description and the commit message. - Name zstd:
web/AGENTS.md:47,docs/packages/web.md:278, the CHANGELOG entry, and add aweb/gzip_test.gocase withAccept-Encoding: gzip, deflate, br, zstdassertingContent-Encoding: zstd. Renaminggzip.go/gzipMiddlewaretocompress.go/compressMiddlewarewould keep the file name honest too. app.js:721:formatTime(e.date)goes intoinnerHTMLunescaped, while line 549 escapes the same expression. Not exploitable today (the value is a slice of a date that already parsed as valid), butweb/AGENTS.md:38states the rule without exception.
Follow-ups, not this PR
These are all worth doing and none of them affects whether this change is correct. Happy to file them as issues.
- No caching at all for static assets. Neither
Cache-ControlnorETagnorLast-Modifiedis set anywhere inweb/, and embedded files have no ModTime, so conditional requests never 304.app.js(55 kB),styles.css(29 kB) andpico.min.css(83 kB) are re-transferred and re-compressed on every reload — which is the costmiddleware.gocites as the reason to keep counting assets against the limiter.gzhttpshipsSuffixETagfor exactly this. - CSP still needs
script-src 'unsafe-inline'because of the pre-paint script inlayout.html:9-37.renderUIPage(ui.go:100) already passesniland documents itself as the seam for server-injected values — a per-request nonce there would close it. - Brotli for the static assets, precompressed at build time rather than per request. Belongs with the caching work.
MinSize(0)compresses 2-byte responses (gzhttp's default is 1024); it is why/liveand/api/logoutneeded explicit content types. Worth a measurement.stripControlChars(app.js:78) removes only the ESC byte, so a coloured log line still renders[0;31mas text. Dropping the whole CSI sequence would fix it.<script src="app.js">sits between<main>and<footer>(layout.html:70) but touches#footer-version. It works only because the access is inside afetch().then();deferwould make that robust.apiJob.Configstill carries the full job definition to every/api/jobsreader, and now/api/dashboardtoo. Pre-existing, but a design question worth its own thread — the description's "job config no longer leaks through the API" reads wider than what changed, which is/api/config.
Commit structure
One commit over 49 files containing five bug fixes means those fixes cannot be cherry-picked. The log demux and the delete-gate hole are candidates for a patch release without the UI. Since the commit has to be rewritten to be signed anyway, that is a free moment to split it. Roughly:
fix(core): demux container logs instead of copying raw frames
fix(core): keep a paused job paused across UpdateJob
fix(web): anchor nextRuns/prevRuns on the cron entry
fix(web)!: refuse API updates to config-owned jobs
refactor(web): match job collections by shape in stripJobs
fix(web): exempt health probes from rate limiting
feat(web): negotiate response compression via zstd and gzip
feat(web): add /api/dashboard aggregate endpoint
feat(web): add recentRuns summary to job payloads
refactor(web): split the UI into templates, app.js and styles.css
feat(web): rebuild the dashboard UI
feat(web): serve UI assets from OFELIA_UI_DEV_DIR in development
docs: update API docs, web package docs and CHANGELOG
The ! plus a BREAKING CHANGE: trailer on the update-gate commit makes the break visible to the release tooling; the DockerProvider addition can share that trailer on the log-demux commit.
Assisted by claude-code:claude-opus-5 — Session
RunJob stored the raw reader from GetContainerLogs, so every line of a non-TTY container's output carried Docker's 8-byte stream frame header into the execution's stdout: the header bytes showed up in the web UI's run output and in the mail/slack middlewares. The demultiplexing already existed in the container adapter's CopyLogs, but it was unreachable through the provider interface, and its TTY branch tested `info.Config.HostConfig != nil` — never the TTY flag — so a real TTY container took the demux path and a non-TTY container took the raw copy. Both are fixed: CopyLogs branches on Config.Tty, and DockerProvider gains CopyContainerLogs so RunJob can write straight into the execution's stdout and stderr streams. BREAKING CHANGE: core.DockerProvider gains CopyContainerLogs. External implementations of the exported interface must add the method to compile. Users of the provided implementations are unaffected. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
UpdateJob refused disabled jobs with ErrJobNotFound, which pushed the web updateJobHandler into its RemoveJob+AddJob fallback. RemoveJob drops the disabledNames entry and files the old job under Removed, so editing a paused job silently resumed it and left a phantom row in the Removed tab for the rest of the process's life. Disabled jobs are now updated in place. go-cron replaces the entry on update, so the pause is re-asserted afterwards rather than assumed to carry over; pausing an already-paused entry is a no-op. BREAKING CHANGE: Scheduler.UpdateJob no longer returns ErrJobNotFound for a disabled job. Callers that used the error to detect "not scheduled" must check the disabled state explicitly. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
computeRunTimes projected the schedule from the poll's `now`. For an interval schedule (@every 30s) go-cron's Next(t) is t+interval with no anchor, so every 5s poll reported a next run 30s out and the countdown in the UI never moved toward zero. The cron entry already carries the scheduled Next and Prev. Use them as the first element and project the remaining ones from there; fall back to projecting from `now` only when the entry has no time yet. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
POST /api/jobs/update accepted jobs that came from the INI file or from Docker labels. The update overrode the job in memory until the next config sync — and rewrote its origin to api/web, after which the delete gate no longer recognized the job as config-owned. Editing a label job was therefore enough to unlock deleting it, past the gate added for netresearch#593. Updates on such jobs now answer 403 with a message naming the source, mirroring the delete gate. BREAKING CHANGE: POST /api/jobs/update returns 403 Forbidden for jobs defined in INI config or Docker labels, where it previously returned 200. Scripts that edited config-owned jobs through the API must change the source config, or use POST /api/jobs/disable to suppress the job. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
stripJobs zeroed the job collections of the config payload by field name. A hand-maintained name list drifts: this one had already picked up a field that no longer exists, and a collection added to cli.Config later would ship every job's full definition — commands, env, credentials — to every reader of /api/config. Match on shape instead: map[string]*struct is what every job collection in cli.Config is, and no other field there has that shape, so a new collection is stripped the day it is added. The stripped output is unchanged for the current config: the five names in the old list and the five fields matched by shape are the same set. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
The per-IP limiter counted every request, /live and /ready included. A probe answered 429 reads as unhealthy, so a busy dashboard tab could get the daemon restarted by its own orchestrator. Both probes are cheap — a constant string and a scheduler-state lookup — so they bypass the limiter now. /health and /healthz stay counted. They are token-free like the probes, but GetHealth calls runtime.ReadMemStats on every request, which stops the world, and answers with the version and the goroutine count: exempting them would leave an unauthenticated, unthrottled endpoint that pauses the GC once per call. Orchestrators poll /live and /ready. The existing limiter tests move to /api/test paths so the exempt set is never crossed by accident. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
List views want a per-job outcome history — a sparkline of the last runs, and last/avg/max durations — but the only source was the per-job history endpoint, one request per row. apiJob gains recentRuns: at most the ten newest executions, oldest first, each reduced to date, duration, failed and skipped. Additive and omitted for jobs that keep no history, so existing readers are unaffected. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
A polling client needs jobs, disabled, removed and config together, plus the open job's history: four to five requests per tick, ~60/min per browser tab, which exhausted the 100-requests-per-minute per-IP limit with two dashboard tabs open and 429'd everything including the static assets. GET /api/dashboard answers all of it in one response, optionally with a job's runs via ?history=<name>, so a tick costs one request and the sections come from a single moment in time. The per-resource endpoints are untouched — they are the documented public API that scripts and monitoring consume; this one is additive, for polling consumers. The history conversion moves into buildAPIHistory, shared by the per-job endpoint and the aggregate. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
static/ui/index.html had grown to 830 lines of markup, CSS and JavaScript in one file, which made every UI change a merge-conflict magnet and gave the browser no way to cache the parts that rarely change. The file is split with no behavior change: styles.css, app.js, and templates/layout.html plus one partial per tab, assembled server-side by uiHandler at GET /. Templates are parsed once from the embedded FS at startup, so a broken template fails the daemon instead of every request. Static assets keep going through http.FileServer; /templates/* is not served — the sources are render inputs, not assets, and the render seam is where server-injected values will land. /index.html still renders the page so old bookmarks keep working. Still vanilla CSS and JS with no build step and no new dependency. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
Every UI edit meant rebuilding the binary to refresh the embedded snapshot, which is a slow loop for CSS and template work. When OFELIA_UI_DEV_DIR names a directory, assets are read from it and the page templates are re-parsed on every request, so an edit shows up on the next browser reload. A template parse error answers 500 with the parse error in the body, so the developer sees what broke without reading the daemon log. Unset in production: the embedded assets stay the default and are still parsed once at startup. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
The split page still showed the same bare tables: no way to find a job among fifty, no sense of whether a job is healthy or slowing down, and expanding a run's output reshuffled the table's columns. The dashboard is rebuilt on top of the template split: - stat cards for active, failing (a click filters the table) and the next run with a countdown, computed from the poll already made - job search and column sorting, both rendering from cached data, so a keystroke or a sort click costs no request - a result sparkline per job and last/avg/max durations, from the recentRuns payload - history in a modal dialog; run output in a full-width subrow that survives the 5s refresh, keyed by execution timestamp and scoped to the job on screen - origin badges, disabled edit/delete on config-owned jobs with a tooltip naming the source, delete confirmation, and toasts carrying the server's message instead of silent failures - teal-on-graphite theming, SVG action icons, striped tables, a hidden tab that stops polling, and a footer version from /health The page and the stylesheet pass the W3C Nu validator. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
The UI ships 167 kB of assets and polls a 5.8 kB payload every 5 seconds, all of it uncompressed text. Responses now go through gzhttp as the innermost middleware. The wrapper enables zstd next to gzip and prefers it at equal q-values, so Chrome, Edge and Firefox — which send "gzip, deflate, br, zstd" — receive zstd, and clients without zstd fall back to gzip. First page load drops from ~140 kB to ~28 kB, a dashboard poll from 5.8 kB to 1.6 kB. Accept-Encoding qvalues, bodiless statuses, ranged requests and content sniffing are the library's problem, not ours. The middleware chain moves into wrapMiddleware so the two construction sites cannot drift apart. Handlers that call WriteHeader before their first body write (LivenessHandler, logoutHandler) now set Content-Type explicitly: the wrapper can only sniff a missing type on the first write, and that sniff would otherwise run on the compressed bytes and answer application/x-gzip. Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
- docs/API.md: /api/dashboard, the recentRuns field, the 403 on updating a config-owned job - docs/packages/web.md: what the rate limiter counts and what it exempts, and that compression negotiates zstd or gzip - docs/CONFIGURATION.md: OFELIA_UI_DEV_DIR - web/AGENTS.md: the UI is templates plus assets now, the polling and rate-limit budget, the CSP notes for external app.js - AGENTS.md: go:embed walks a matched directory recursively, so ui/* covers ui/templates/ — the previous note claimed the opposite - CHANGELOG: the Unreleased entries for all of the above, with the breaking API and interface changes called out under Changed Signed-off-by: Kamil Pešek <pesek.kamil@seznam.cz>
5d88df8 to
073eab8
Compare
|
|
Thanks — all six points are addressed.
|
CybotTM
left a comment
CybotTM
left a comment
There was a problem hiding this comment.
Thanks for this contribution — the commit hygiene (four standalone fixes, hardening, API, then UI) made a 50-file PR genuinely reviewable, and the security posture is solid: we audited all 16 innerHTML sinks (job output included) and every dynamic value goes through escapeHtml/textContent; /api/dashboard is covered by the existing auth gate and route-expectation test; the BREACH analysis in web/AGENTS.md checks out. Full go test ./... passes on your head. Verdict: request changes — all mechanical:
- Rebase onto current main and resolve the
CHANGELOG.mdconflict — pergit merge-treeit is the only conflicting file. This also clears six of the seven golangci-lint findings plus the gofumpt one: main fixed exactly those in a9f9e8b (#800), so the PR body's "needs a separate fix on main" paragraph is stale — please refresh it. - One lint finding originates on this branch:
web/compress_test.go:15—github.com/klauspost/compress/zstdneeds its own gci import group (blank line before thenetresearch/ofeliagroup). docs/openapi.yamlis untouched whiledocs/API.mdgrew by 110 lines — please add/api/dashboard, therecentRunsfield, and the 403 onPOST /api/jobs/updateso spec and docs don't diverge further.- Screenshots, please (light + dark) — a visual rebuild can't be assessed from the diff.
- Optional but valued: wrap the top-level
localStoragereads (app.js:45, 243, 269; layout.html:30–32) in try/catch — with site data blocked the current code throws before first render and the dashboard stays blank.
Noted for us maintainers, not blocking you: whether to cherry-pick your four fix commits (937a2ce, 3684990, 52c7c62, f01615b) into a patch release first; sign-off on the three properly-declared breaking changes; and a follow-up issue for the pre-existing auth-before-rate-limit ordering your refactor made visible (and, by centralizing the chain in wrapMiddleware, easier to fix — thanks for that).
2 participants
Summary
Splits the single-file UI into html/template partials plus standalone
app.jsandstyles.css, then rebuilds the dashboard on top: teal/graphite theming, stat cards,a searchable job table, sortable job and history tables, run output in an expandable
subrow, job history in a modal, and a custom tooltip system that survives overflow and
dialog stacking.
Backend changes the UI depends on:
/api/dashboardaggregates the per-tick payload so the UI polls once. Auth-gatedlike the rest of
/api/, and declared in theServer.routestable soTestRouteAuthExpectationsholds it to an auth expectation. The per-resourceendpoints are untouched — this is additive, for polling consumers
recentRunson job payloads: an additive summary of the last 10 executions(date, duration, failed, skipped) on
/api/jobs,/api/jobs/disabled,/api/jobs/removedand/api/dashboard, so list views can render outcomehistory without a fetch per job. Omitted for jobs that keep no history
klauspost/compress/gzhttp(the only new dependency).The wrapper enables zstd next to gzip and prefers it at equal q-values, so
Chrome, Edge and Firefox — which send
gzip, deflate, br, zstd— receiveContent-Encoding: zstd, and clients without zstd (Safari below 26, curldefaults, monitoring scripts) receive gzip. Both branches are pinned in
web/compress_test.go/liveand/readyexempt from rate limiting; everything else, static assetsand
/healthincluded, still counted./healthis token-free but expensive —GetHealthcallsruntime.ReadMemStatson every request, which stops the world,and reports version and goroutine count — so exempting it would leave an
unauthenticated, unthrottled endpoint that pauses the GC per call. Orchestrator
probes are served by
/liveand/readyPOST /api/jobs/updatenow returns 403 for INI- andlabel-owned jobs, mirroring the existing delete gate (Persist manually added/edited jobs #593), and the UI disables
both actions on those jobs
resumes it
nextRuns/prevRunsanchored on the cron entry, not the poll timeOFELIA_UI_DEV_DIRserves UI assets from disk during developmentFive of those bullets are pre-existing bug fixes rather than UI scaffolding. Two are
worth spelling out, because the failure mode is not obvious from the one-line summary:
api/web, after which thedelete gate no longer recognised it as config-owned — editing a label job unlocked
deleting it.
CopyLogsbranched oninfo.Config.HostConfig != nil, true for essentiallyevery container, so non-TTY containers took the TTY raw-copy path and Docker's
8-byte frame headers landed in stored job output.
And one refactor that is hardening rather than a fix, corrected from an earlier
version of this description:
stripJobsnow matches job collections by shape(
map[string]*struct) instead of by a hand-maintained field-name list. The list itreplaces —
RunJobs, ExecJobs, ServiceJobs, LocalJobs, ComposeJobs— covers exactlythe five collections
cli.Confighas today, so/api/configoutput is unchanged bythis PR and nothing leaked. The point of the change is that a collection added later
is stripped by construction, instead of shipping every job's definition until someone
remembers to update the list.
Breaking changes
core.DockerProvidergainsCopyContainerLogs, so any external implementation ofthat exported interface must add the method to compile. Inside the repo it is the
+4lines in each of the six mock providers.POST /api/jobs/updatereturns 403 for INI- and label-owned jobs, where itpreviously returned 200. Scripts that edited config-owned jobs through the API must
change the source config, or use
POST /api/jobs/disableto suppress the job.Scheduler.UpdateJobupdates a disabled job in place instead of returningErrJobNotFound. Callers that used the error to detect "not scheduled" must checkthe disabled state explicitly.
All three are source- or API-level breaks under SemVer §4 for the current 0.y.z line.
They carry
BREAKING CHANGE:trailers in the commits and are listed under### Changedin the CHANGELOG.
One security note on the compression: it now covers authenticated API responses,
which is the precondition for BREACH-style length leaks. No current endpoint has the
other half of that pattern — none reflects caller-supplied input into a response that
also carries a secret — so this is not exploitable as it stands. It is recorded in
web/AGENTS.mdso a future endpoint that echoes user input is checked against it.Docs and CHANGELOG updated. Tests added for the compression middleware (gzip and
zstd), the rate-limit scope, the update gate, config stripping, UI rendering and
refresh ordering.
The change is split into 13 commits so the bug fixes can be cherry-picked into a
patch release without the UI work.
Type of Change
Checklist
-S) and signed-off (--signoff)go-check / golangci-lintis red, with the same seven findings it reports onmain(six
nolint:goconstdirectives thatnolintlintnow considers unused, and onegofumpt formatting complaint in
cli/doctor_docker_timeout_test.go). None of themcomes from this branch; they need a separate fix on
main.Test Plan
Verified locally:
Every one of the 13 commits builds on its own (
go build ./...at each).TestComposeJobBuildCommandfails in thegolang:1.26-alpinedev container becausethe image has no
dockerbinary inPATH; it passes in CI.The e2e suite is left to CI: it needs a host with no unrelated
ofelia.*-labelledcontainers, since the daemon label scan picks them up and throws off the job-count
assertions.
To exercise the UI by hand:
For UI iteration without rebuilding the binary:
Worth clicking through: open a job's history from the name button, expand a run's
output, switch timezone and theme, search and sort the job table, and confirm that a
job defined in INI or via Docker labels shows edit and delete as disabled with a
tooltip naming the source (and that
POST /api/jobs/updateon it returns 403).