Fix static files, admin paths and req.user in the dev server - #4
Conversation
The dev server was missing three pieces of wiring the production host sets up, so most of what http.serve plugins do could not be exercised locally. Mount public/ as PublicFS when loading a project directory, the same way the manager's loadByPath does. Without it every static file and directory index answered 404, so a plugin's own pages were unreachable. Set RequireAdmin. The plugin server returns 404 for any manifest-declared admin path when it is nil, which meant the whole UI of file-manager, admin-demo and theme-hub was unreachable. Locally it is an open pass-through, because the URL this dev server prints is the plugin root and for those plugins that is an admin path, so requiring a credential would make the advertised URL unopenable in a browser. Owncast wires the same hook to its admin Basic Auth middleware. Set GetRequestUser so req.user is populated. The Bearer user:<name> header the dev server already documents had no effect on HTTP handlers before. Also reorder two imports, which is what gofmt wants and is unrelated to the fix.
There was a problem hiding this comment.
Pull request overview
Aligns owncast-plugin-serve (dev server) with production host wiring so HTTP-serving plugins can be exercised locally: project public/ is served, manifest-declared admin pages no longer 404, and req.user is populated.
Changes:
- Mount
public/(andassets/) when loading a loose project directory so plugin HTTP/static routes work in the dev server. - Wire
RequireAdmin(dev passthrough) andGetRequestUserintoplugin.Serverto match production server behavior. - Improve startup banner output to describe available static files and admin-page behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review feedback on the dev server fix. Bind 127.0.0.1 instead of every interface. This calls itself a localhost dev server and prints localhost URLs, and now that admin paths are served with no credential, binding every interface would hand a plugin like file-manager an open file read, write and delete API to anyone on the same network. Rename the .ocpkg branch's local variable to match what it now describes, which is all static files rather than only assets.
|
Both review comments addressed in c484d71. The second one turned out to be more than a wording problem. This server was binding every interface while calling itself a localhost dev server, and this PR is what makes admin paths reachable with no credential, so for file-manager that would have been an open file read, write and delete API for anyone on the same network. It now binds 127.0.0.1. The printed URL still answers 200 over localhost, and the same path on the box IP is refused. If someone does need this reachable from another device, that should be an explicit opt-in flag rather than the default. |
The dev server was missing three pieces of wiring the production host sets up, so most of what an
http.serveplugin does could not be exercised locally. Static files answered 404, every manifest-declared admin page answered 404, andreq.userwas always empty.public/asPublicFSwhen loading a project directory, the same pair the manager'sloadByPathwires in production. Without it a plugin's own pages and directory indexes answered 404, which is most of whathttp.serveis for.RequireAdmin. The plugin server returns 404 for any manifest-declared admin path when that hook is nil, so the entire UI of file-manager, admin-demo and theme-hub was unreachable. Locally it is an open pass-through, because the URL this dev server prints is the plugin root and for those plugins that root is an admin path, so requiring a credential would make the advertised URL unopenable in a browser. Owncast wires the same hook to its admin Basic Auth middleware, and the startup banner now says so when a manifest declares admin pages.GetRequestUsersoreq.useris populated. TheBearer user:<name>header this dev server already documents did nothing for HTTP handlers before.The two-line import reorder is what gofmt wants and is unrelated to the fix.
I verified this by running the dev server against the prebuilt examples and comparing against a binary built from current
main.overlay, a project directory with
public/index.htmland a dynamic route:file-manager, whose whole UI sits under its
/adminpage:The uploaded file lands in the plugin's own sandbox on disk, and
download?name=../../../etc/passwdstill answersinvalid file namerather than returning file contents.whoami with
Authorization: Bearer user:alicegoes from 401 to 200 with the resolved user, and an anonymous request still carries no user.Packaged plugins are unaffected.
LoadPackagealready mountspublic/from inside the archive, and I checked overlay's.ocpkgstill serves before and after. The only change there is the startup line, which used to be skipped for a package that shipspublic/but noassets/.No tests added.
cmd/owncast-plugin-servehas no test harness for spinning up the server, so the evidence here is the before and after run against the real examples.Heads up that I have a separate dev server fix coming for chat command dispatch. It touches the same file, so expect a trivial conflict in the import block depending on merge order.