feat: admin web UI POC (dbdeployer admin ui)#56
Conversation
- `dbdeployer admin ui` starts localhost dashboard on port 9090 - OTP authentication: CLI prints token, browser validates - Dashboard shows sandbox cards grouped by topology - HTMX-powered start/stop/destroy actions (no page reload) - Dark theme, Go templates embedded via //go:embed - Reads existing sandbox catalog, checks PID for status
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a localhost admin web UI: an embedded HTTP server with OTP one-time login and in-memory sessions, sandbox discovery and running-state detection, HTMX-driven start/stop/destroy actions, sandbox lifecycle helpers, embedded templates/static assets, and a new Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant Server as AdminServer
participant Auth as AuthManager
participant FS as Catalog/FileSystem
Browser->>Server: GET /login
Server-->>Browser: Render login.html
Browser->>Server: GET /login?token=...
Server->>Auth: ValidateOTP(token)
Auth-->>Server: true / false
alt valid
Server->>Auth: CreateSession()
Auth-->>Server: sessionID
Server-->>Browser: Set cookie + Redirect to /
else invalid
Server-->>Browser: Render login.html
end
sequenceDiagram
participant Browser as Client
participant Server as AdminServer
participant FS as Catalog/FileSystem
participant Shell as ShellScript
Browser->>Server: POST /api/sandboxes/:name/start
Server->>Server: AuthMiddleware validates cookie
Server->>FS: Resolve sandbox directory (catalog)
Server->>Shell: ExecuteSandboxScript(sandbox, "start")
Shell-->>Server: stdout/stderr + exit status
Server->>FS: GetAllSandboxes() (refresh)
Server-->>Browser: Render sandbox-list.html (HTMX swap)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces an experimental web-based administration interface for dbdeployer, transforming it from a purely command-line tool into a platform with a graphical dashboard. The UI allows users to view their deployed sandboxes, monitor their status, and perform lifecycle operations like starting, stopping, and destroying them through an intuitive web browser interface. This proof-of-concept aims to demonstrate the feasibility of a richer, interactive management experience for dbdeployer users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new dbdeployer admin ui command, which launches a local web server providing an OTP-authenticated dashboard for managing sandboxes. The UI allows users to view, start, stop, and destroy sandboxes through HTMX-powered interactions. Feedback from the review highlights several areas for improvement, including the need to implement session expiration and robust error handling for cryptographic operations (rand.Read), sandbox lifecycle scripts, and sandbox data retrieval. Additionally, the openBrowser function should handle potential errors and extend support to Windows, and API error messages should be more specific for enhanced usability.
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| sessionID := generateOTP() | ||
| a.sessions[sessionID] = time.Now() |
There was a problem hiding this comment.
The created session currently has no expiration. This is a security risk as a compromised session ID could remain valid indefinitely. It's crucial to implement a session timeout mechanism to invalidate sessions after a period of inactivity or a fixed duration. This could involve storing an expiration timestamp with the session and checking it during ValidateSession.
| a.sessions[sessionID] = time.Now() | |
| a.sessions[sessionID] = time.Now().Add(1 * time.Hour) // Example: session expires in 1 hour |
|
|
||
| func generateOTP() string { | ||
| b := make([]byte, 16) | ||
| _, _ = rand.Read(b) |
There was a problem hiding this comment.
Ignoring the error return from rand.Read can lead to a security vulnerability. If rand.Read fails, the byte slice b might not be filled with cryptographically secure random bytes, potentially making the OTP predictable. Always check for errors when dealing with cryptographic functions.
if _, err := rand.Read(b); err != nil {
// Handle the error appropriately, e.g., log it and return an error or panic.
// For now, we'll panic as a critical security failure.
panic(fmt.Sprintf("failed to generate random bytes for OTP: %v", err))
}| sbDir := sandboxDirFor(sandboxName) | ||
|
|
||
| // Try stop (ignore errors — sandbox may already be stopped). | ||
| _ = ExecuteSandboxScript(sandboxName, "stop") |
There was a problem hiding this comment.
Ignoring the error from ExecuteSandboxScript can hide important issues during sandbox destruction. While a sandbox might already be stopped, other errors could occur (e.g., permission issues, script not found for unexpected reasons). It's better to log this error for debugging purposes, even if it doesn't prevent the DestroySandbox operation from continuing.
if err := ExecuteSandboxScript(sandboxName, "stop"); err != nil {
// Log the error, but continue with destruction as stopping might not be critical.
fmt.Printf("Warning: failed to stop sandbox %s during destruction: %v\n", sandboxName, err)
}|
|
||
| // Parse: /api/sandboxes/<name>/<action> | ||
| parts := strings.Split(strings.TrimPrefix(r.URL.Path, "/api/sandboxes/"), "/") | ||
| if len(parts) != 2 { |
There was a problem hiding this comment.
The error message "Bad request" is too generic. Providing a more specific message, such as "Invalid sandbox action path format. Expected /api/sandboxes//", would greatly improve API usability and debugging for clients.
| if len(parts) != 2 { | |
| if len(parts) != 2 { | |
| http.Error(w, "Invalid sandbox action path format. Expected /api/sandboxes/<name>/<action>", http.StatusBadRequest) | |
| return | |
| } |
| } | ||
|
|
||
| // Return updated sandbox list fragment for HTMX. | ||
| sandboxes, _ := GetAllSandboxes() |
There was a problem hiding this comment.
Ignoring the error returned by GetAllSandboxes() can lead to unexpected behavior if the function fails. If an error occurs, sandboxes might be nil, which could cause issues when passed to the template. It's best to handle this error explicitly, perhaps by logging it and returning an http.Error to the client.
| sandboxes, _ := GetAllSandboxes() | |
| sandboxes, err := GetAllSandboxes() | |
| if err != nil { | |
| http.Error(w, "Failed to retrieve sandboxes: "+err.Error(), http.StatusInternalServerError) | |
| return | |
| } |
|
|
||
| openBrowser(url) | ||
|
|
||
| listener, err := net.Listen("tcp", addr) |
There was a problem hiding this comment.
The openBrowser function is called without checking for potential errors. If the browser fails to open (e.g., due to missing xdg-open or open commands, or permission issues), the user might not see the URL and OTP, leading to a poor user experience. It's advisable to log any errors from openBrowser to aid debugging.
if err := openBrowser(url); err != nil {
fmt.Printf("Warning: failed to open browser: %v\n", err)
}| var cmd *exec.Cmd | ||
| switch runtime.GOOS { | ||
| case "linux": | ||
| cmd = exec.Command("xdg-open", url) | ||
| case "darwin": | ||
| cmd = exec.Command("open", url) | ||
| } | ||
| if cmd != nil { | ||
| _ = cmd.Start() | ||
| } |
There was a problem hiding this comment.
The openBrowser function currently only supports Linux and macOS. For Windows users, the browser will not open automatically, which can be a significant usability issue. Consider adding support for Windows using cmd /c start or start command.
func openBrowser(url string) error {
var cmd *exec.Cmd
switch runtime.GOOS {
case "linux":
cmd = exec.Command("xdg-open", url)
case "darwin":
cmd = exec.Command("open", url)
case "windows":
cmd = exec.Command("cmd", "/c", "start", url)
default:
return fmt.Errorf("unsupported operating system to open browser: %s", runtime.GOOS)
}
if cmd != nil {
return cmd.Start()
}
return nil
}There was a problem hiding this comment.
Pull request overview
POC implementation of a localhost-only “admin web UI” for dbdeployer, adding a new dbdeployer admin ui command that runs an HTTP server with OTP-based login and a sandbox dashboard with start/stop/destroy actions.
Changes:
- Add
dbdeployer admin uiCobra subcommand wiring. - Introduce new
admin/package (HTTP server, OTP/session auth, sandbox listing/status, lifecycle actions). - Add embedded Go HTML templates (login, dashboard, sandbox list component) and initial static directory.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md | Adds a design/spec doc for the admin web UI POC. |
| cmd/admin.go | Wires new admin ui subcommand and --port flag. |
| admin/server.go | Implements embedded-template HTTP server, routes, and browser opener. |
| admin/auth.go | Implements OTP + in-memory session cookie auth. |
| admin/handlers.go | Implements login, dashboard rendering, JSON listing, and action endpoints. |
| admin/sandbox_status.go | Reads catalog and infers running status from PID files. |
| admin/actions.go | Executes sandbox lifecycle scripts and supports destroy (rm -rf + catalog delete). |
| admin/templates/login.html | Adds login page template. |
| admin/templates/dashboard.html | Adds dashboard template and HTMX wiring. |
| admin/templates/components/sandbox-list.html | Adds sandbox card/list fragment used for HTMX swaps. |
| admin/static/placeholder.txt | Placeholder for embedded static assets. |
| admin/static/.gitkeep | Keeps static directory in git. |
Comments suppressed due to low confidence (1)
cmd/admin.go:31
- Import block is not gofmt/goimports formatted (stdlib imports are mixed with module imports and there’s no blank line separation). This tends to drift further over time and makes diffs noisier; running gofmt/goimports on this file should reorder/group the imports correctly.
import (
"fmt"
"github.com/ProxySQL/dbdeployer/admin"
"github.com/ProxySQL/dbdeployer/defaults"
"os"
"path"
"sort"
"github.com/ProxySQL/dbdeployer/common"
"github.com/ProxySQL/dbdeployer/globals"
"github.com/ProxySQL/dbdeployer/sandbox"
"github.com/pkg/errors"
"github.com/spf13/cobra"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func ExecuteSandboxScript(sandboxName string, script string) error { | ||
| sbDir := sandboxDirFor(sandboxName) | ||
|
|
There was a problem hiding this comment.
sandboxName comes from the URL and is directly joined into a filesystem path. As-is, values like ../... can escape SandboxHome and make this endpoint execute/remove arbitrary paths after auth (and auth here is only a session cookie). Resolve the target sandbox via the catalog and validate the identifier strictly (e.g., only allow exact catalog keys or a validated name set), and reject names containing path separators or ...
| // DestroySandbox stops a sandbox and removes its directory and catalog entry. | ||
| func DestroySandbox(sandboxName string) error { | ||
| sbDir := sandboxDirFor(sandboxName) | ||
|
|
||
| // Try stop (ignore errors — sandbox may already be stopped). | ||
| _ = ExecuteSandboxScript(sandboxName, "stop") | ||
|
|
||
| // Remove directory. | ||
| if err := os.RemoveAll(sbDir); err != nil { | ||
| return fmt.Errorf("removing %s: %w", sbDir, err) | ||
| } | ||
|
|
||
| // Remove from catalog. The catalog key is the full destination path. | ||
| if err := defaults.DeleteFromCatalog(sbDir); err != nil { | ||
| return fmt.Errorf("removing %s from catalog: %w", sandboxName, err) | ||
| } |
There was a problem hiding this comment.
Sandbox directory resolution doesn’t match how the catalog is keyed. The catalog key is the full destination path (defaults.UpdateCatalog(sandboxDef.SandboxDir, ...)), but here actions always assume SandboxHome/<name>, and DeleteFromCatalog is called with that derived path. This will break for sandboxes deployed outside the default home and may fail to delete the correct catalog entry; use the catalog destination path consistently for actions and catalog deletes.
| // Return updated sandbox list fragment for HTMX. | ||
| sandboxes, _ := GetAllSandboxes() | ||
| data := map[string]interface{}{ | ||
| "Sandboxes": sandboxes, | ||
| "Count": len(sandboxes), | ||
| } |
There was a problem hiding this comment.
Error from GetAllSandboxes() is ignored when returning the updated HTMX fragment. If catalog reading fails here, the handler will still try to render with a nil/partial list and hide the underlying failure. Handle the error and return an appropriate HTTP error instead of discarding it.
| hx-post="/api/sandboxes/{{.Name}}/start" | ||
| hx-target="#sandbox-list" | ||
| hx-swap="innerHTML"> | ||
| Start | ||
| </button> | ||
| <button class="btn btn-stop" | ||
| hx-post="/api/sandboxes/{{.Name}}/stop" | ||
| hx-target="#sandbox-list" | ||
| hx-swap="innerHTML"> | ||
| Stop | ||
| </button> | ||
| <button class="btn btn-destroy" | ||
| hx-post="/api/sandboxes/{{.Name}}/destroy" |
There was a problem hiding this comment.
.Name is interpolated directly into the hx-post URL path. Go templates will HTML-escape, not URL-path-escape, so names containing spaces/special chars will generate broken endpoints (e.g., & becomes &). Either ensure sandbox names are restricted to URL-safe characters, or apply proper URL path escaping / use a stable ID (e.g., encoded catalog key) for API routes.
| hx-post="/api/sandboxes/{{.Name}}/start" | |
| hx-target="#sandbox-list" | |
| hx-swap="innerHTML"> | |
| Start | |
| </button> | |
| <button class="btn btn-stop" | |
| hx-post="/api/sandboxes/{{.Name}}/stop" | |
| hx-target="#sandbox-list" | |
| hx-swap="innerHTML"> | |
| Stop | |
| </button> | |
| <button class="btn btn-destroy" | |
| hx-post="/api/sandboxes/{{.Name}}/destroy" | |
| hx-post="/api/sandboxes/{{.Name | urlquery}}/start" | |
| hx-target="#sandbox-list" | |
| hx-swap="innerHTML"> | |
| Start | |
| </button> | |
| <button class="btn btn-stop" | |
| hx-post="/api/sandboxes/{{.Name | urlquery}}/stop" | |
| hx-target="#sandbox-list" | |
| hx-swap="innerHTML"> | |
| Stop | |
| </button> | |
| <button class="btn btn-destroy" | |
| hx-post="/api/sandboxes/{{.Name | urlquery}}/destroy" |
| addr := fmt.Sprintf("127.0.0.1:%d", s.port) | ||
| url := fmt.Sprintf("http://%s/login?token=%s", addr, s.auth.OTP()) | ||
|
|
||
| fmt.Printf("\n dbdeployer admin\n") | ||
| fmt.Printf(" ────────────────────────────────\n") | ||
| fmt.Printf(" URL: %s\n", url) | ||
| fmt.Printf(" (opening browser...)\n\n") | ||
|
|
||
| openBrowser(url) | ||
|
|
||
| listener, err := net.Listen("tcp", addr) | ||
| if err != nil { | ||
| return fmt.Errorf("listen on %s: %w", addr, err) | ||
| } | ||
| return http.Serve(listener, mux) |
There was a problem hiding this comment.
The browser is opened before the TCP listener is created. On a fast browser or slow startup, the initial navigation can race and fail to connect. Bind/listen first (or start the server in a goroutine and wait until it’s accepting connections) and only then open the browser.
| // Check if the process exists by sending signal 0. | ||
| proc, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| err = proc.Signal(os.Signal(nil)) | ||
| return err == nil |
There was a problem hiding this comment.
Process existence check is broken: proc.Signal(os.Signal(nil)) will panic on Unix (nil doesn’t implement the expected signal type) and won’t reliably check liveness. Use a proper signal-0 check (e.g., syscall.Kill(pid, 0) / proc.Signal(syscall.Signal(0))) and handle platform differences accordingly.
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>dbdeployer admin</title> | ||
| <script src="https://unpkg.com/htmx.org@1.9.12/dist/htmx.min.js" crossorigin="anonymous"></script> |
There was a problem hiding this comment.
This loads HTMX from a CDN, but the PR description calls out “HTMX from CDN, embedded via //go:embed” and the design doc suggests embedding static assets. If the intent is zero external runtime deps, switch this to /static/htmx.min.js (and embed that file). Otherwise, update the PR description/spec to match the CDN approach.
|
|
||
| func generateOTP() string { | ||
| b := make([]byte, 16) | ||
| _, _ = rand.Read(b) |
There was a problem hiding this comment.
generateOTP ignores the error from rand.Read. If the system CSPRNG fails, this will silently generate an all-zero token, undermining auth/session IDs. Propagate the error (and fail server startup) or retry/exit on failure so token generation can’t silently degrade.
| _, _ = rand.Read(b) | |
| if _, err := rand.Read(b); err != nil { | |
| // Fail fast if the CSPRNG is unavailable rather than generating weak tokens. | |
| panic("crypto/rand.Read failed while generating OTP: " + err.Error()) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
admin/actions.go (1)
26-31: Consider adding a timeout for script execution.The script execution has no timeout, which could cause the HTTP request to hang indefinitely if the sandbox script doesn't terminate. For a web UI, this could lead to poor user experience.
♻️ Proposed fix using context with timeout
+import ( + "context" + "time" + ... +) + func ExecuteSandboxScript(sandboxName string, script string) error { sbDir := sandboxDirFor(sandboxName) scriptPath := filepath.Join(sbDir, script) if _, err := os.Stat(scriptPath); err != nil { // Try the _all variant for multi-node sandboxes. scriptPath = filepath.Join(sbDir, script+"_all") if _, err2 := os.Stat(scriptPath); err2 != nil { return fmt.Errorf("script %q not found in %s", script, sbDir) } } - cmd := exec.Command("bash", scriptPath) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "bash", scriptPath) output, err := cmd.CombinedOutput() if err != nil { + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("%s timed out after 60s", script) + } return fmt.Errorf("%s failed: %s: %w", script, string(output), err) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/actions.go` around lines 26 - 31, The script executor currently uses exec.Command("bash", scriptPath) with no timeout; change it to use a context with timeout (e.g., context.WithTimeout) and exec.CommandContext(ctx, "bash", scriptPath) inside the same function so the process is cancelled when the timeout elapses; ensure you capture cmd.CombinedOutput(), detect context.DeadlineExceeded (or ctx.Err()) and return a clear timeout error that includes any captured output, and cancel the context (or defer cancel()) to free resources (refer to the cmd/exec invocation and the error-return block around CombinedOutput).docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md (2)
29-37: Optional: Add language specifier to fenced code blocks.Per markdownlint, fenced code blocks should have a language specified. For ASCII diagrams, use
textorplaintext.📝 Suggested fix
-``` +```text dbdeployer admin └─ starts HTTP server on 127.0.0.1:<port>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md` around lines 29 - 37, The fenced ASCII diagram block (the one starting with "dbdeployer admin" and listing behaviors like "starts HTTP server...", "generates OTP...", "opens browser to http://127.0.0.1:<port>/login?token=<otp>", "serves embedded HTML templates via Go's html/template", and "HTMX handles dynamic actions") needs a language specifier; update the triple-backtick fence to use ```text (or ```plaintext) so markdownlint stops flagging it—i.e., replace the opening ``` with ```text for that code block in the docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md file.
131-132: Spec mentions auto-port selection, but implementation doesn't include it.The spec states "If busy, find next free port" but the current implementation in
cmd/admin.gosimply uses the provided port without fallback. This is fine for a POC, but consider updating either the spec or implementation to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md` around lines 131 - 132, The spec requires auto-port selection when the desired port is busy; update cmd/admin.go (the routine that parses --port and starts the HTTP server—e.g., startAdminServer/runAdminServer) to attempt binding starting at the configured port (default 9090) and, on EADDRINUSE, increment the port and retry until a free port is found (or a reasonable max attempts), then start the server on the successful port and print the full URL to the terminal. Ensure the loop only reacts to "address in use" errors, preserves any explicit --port value as the starting point, and logs the final chosen port.admin/auth.go (1)
40-46: Sessions accumulate without expiration.The
time.Timevalue stored for each session is never used—sessions remain valid indefinitely. For a localhost POC this is acceptable, but consider adding session expiration or a logout endpoint if this evolves beyond a proof-of-concept.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/auth.go` around lines 40 - 46, CreateSession currently stores time.Now() so sessions never expire; update it to record an expiration time (e.g., time.Now().Add(sessionTTL)) instead of the creation time and add logic to enforce expiry: introduce a sessionTTL constant and either implement a ValidateSession(sessionID string) bool that checks the stored expiry and deletes expired entries from a.sessions, or run a background cleanup goroutine on AuthManager that periodically removes expired sessions; reference AuthManager, CreateSession, a.sessions, generateOTP and add ValidateSession or Logout as appropriate.admin/server.go (1)
64-70: Browser may open before server is ready.
openBrowser(url)is called before the listener is created. If the browser is fast, it could attempt to connect before the server is listening, showing a connection error. Consider opening the browser after the listener is established.Proposed fix
listener, err := net.Listen("tcp", addr) if err != nil { return fmt.Errorf("listen on %s: %w", addr, err) } + + openBrowser(url) + return http.Serve(listener, mux)And remove the earlier
openBrowser(url)call on line 64.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/server.go` around lines 64 - 70, openBrowser(url) is currently invoked before the server listener is created, which can cause the browser to try connecting too early; remove the earlier openBrowser(url) call and instead call openBrowser(url) after net.Listen succeeds (after listener, err := net.Listen("tcp", addr) returns without error) and before returning http.Serve(listener, mux) — you may invoke it synchronously or in a goroutine to avoid blocking; update code around net.Listen, listener, http.Serve, addr, mux and url accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/auth.go`:
- Around line 69-73: generateOTP currently ignores errors from rand.Read,
risking predictable tokens on failure; change generateOTP to return (string,
error) and return an error if rand.Read fails, then update callers
(NewAuthManager and CreateSession) to handle the returned error by propagating
it (or failing session creation) rather than using a zeroed token—ensure
function signatures and all call sites are adjusted to handle the error path and
log/return a clear error when OTP generation fails.
In `@admin/handlers.go`:
- Around line 94-102: GetAllSandboxes() error is being ignored which can lead to
rendering an empty HTMX fragment; update the handler to check the error returned
by GetAllSandboxes() and handle it before calling s.templates.ExecuteTemplate:
if GetAllSandboxes() returns an error, log or return an http.Error (with a clear
message and StatusInternalServerError) and do not call ExecuteTemplate;
otherwise build the data map ("Sandboxes" and "Count") and call
s.templates.ExecuteTemplate(w, "sandbox-list.html", data) as before.
- Around line 68-74: The sandbox name extracted from the path (name from
parts[0]) is not validated and can contain path traversal characters; update the
handler so that before calling ExecuteSandboxScript or DestroySandbox you reject
any invalid sandbox names: ensure name is non-empty, does not contain path
separators ('/' or '\\'), and does not include directory traversal segments like
".." (for example by comparing name to filepath.Base(name) or by using
path/filepath.Clean and ensuring it equals the original and contains no "..");
return http.StatusBadRequest on validation failure. Reference the handler that
sets name, and the downstream functions ExecuteSandboxScript, DestroySandbox and
sandboxDirFor when adding this check.
In `@admin/sandbox_status.go`:
- Around line 144-150: The process-existence check incorrectly calls
proc.Signal(os.Signal(nil)); replace that with sending signal 0 using the
syscall package (i.e. proc.Signal(syscall.Signal(0))) so the check uses the
standard Unix "signal 0" semantics; update imports to include "syscall" and
adjust the proc.Signal call (the code around os.FindProcess and proc.Signal)
accordingly (or add a platform-specific implementation if you need Windows
compatibility).
In `@admin/server.go`:
- Around line 64-70: Replace the direct call to http.Serve with an http.Server
instance that has sensible ReadTimeout, WriteTimeout and IdleTimeout values to
mitigate Slowloris/resource exhaustion; after creating listener via
net.Listen(addr) build server := &http.Server{Handler: mux, ReadTimeout:
<duration>, WriteTimeout: <duration>, IdleTimeout: <duration>} and call
server.Serve(listener); remember to add "time" to imports and keep
openBrowser(url) call as-is.
In `@admin/templates/dashboard.html`:
- Line 7: The HTMX CDN script tag in admin/templates/dashboard.html is missing a
correct Subresource Integrity attribute; update the <script> that loads
"https://unpkg.com/htmx.org@1.9.12/dist/htmx.min.js" to include the proper
integrity attribute value (use the actual SRI SHA256/SHA384/SHA512 hash computed
for that exact file) and keep crossorigin="anonymous" so the browser can
validate the resource; ensure the integrity hash matches the file served by the
CDN to avoid integrity mismatch errors.
---
Nitpick comments:
In `@admin/actions.go`:
- Around line 26-31: The script executor currently uses exec.Command("bash",
scriptPath) with no timeout; change it to use a context with timeout (e.g.,
context.WithTimeout) and exec.CommandContext(ctx, "bash", scriptPath) inside the
same function so the process is cancelled when the timeout elapses; ensure you
capture cmd.CombinedOutput(), detect context.DeadlineExceeded (or ctx.Err()) and
return a clear timeout error that includes any captured output, and cancel the
context (or defer cancel()) to free resources (refer to the cmd/exec invocation
and the error-return block around CombinedOutput).
In `@admin/auth.go`:
- Around line 40-46: CreateSession currently stores time.Now() so sessions never
expire; update it to record an expiration time (e.g.,
time.Now().Add(sessionTTL)) instead of the creation time and add logic to
enforce expiry: introduce a sessionTTL constant and either implement a
ValidateSession(sessionID string) bool that checks the stored expiry and deletes
expired entries from a.sessions, or run a background cleanup goroutine on
AuthManager that periodically removes expired sessions; reference AuthManager,
CreateSession, a.sessions, generateOTP and add ValidateSession or Logout as
appropriate.
In `@admin/server.go`:
- Around line 64-70: openBrowser(url) is currently invoked before the server
listener is created, which can cause the browser to try connecting too early;
remove the earlier openBrowser(url) call and instead call openBrowser(url) after
net.Listen succeeds (after listener, err := net.Listen("tcp", addr) returns
without error) and before returning http.Serve(listener, mux) — you may invoke
it synchronously or in a goroutine to avoid blocking; update code around
net.Listen, listener, http.Serve, addr, mux and url accordingly.
In `@docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md`:
- Around line 29-37: The fenced ASCII diagram block (the one starting with
"dbdeployer admin" and listing behaviors like "starts HTTP server...",
"generates OTP...", "opens browser to
http://127.0.0.1:<port>/login?token=<otp>", "serves embedded HTML templates via
Go's html/template", and "HTMX handles dynamic actions") needs a language
specifier; update the triple-backtick fence to use ```text (or ```plaintext) so
markdownlint stops flagging it—i.e., replace the opening ``` with ```text for
that code block in the
docs/superpowers/specs/2026-03-24-admin-webui-poc-design.md file.
- Around line 131-132: The spec requires auto-port selection when the desired
port is busy; update cmd/admin.go (the routine that parses --port and starts the
HTTP server—e.g., startAdminServer/runAdminServer) to attempt binding starting
at the configured port (default 9090) and, on EADDRINUSE, increment the port and
retry until a free port is found (or a reasonable max attempts), then start the
server on the successful port and print the full URL to the terminal. Ensure the
loop only reacts to "address in use" errors, preserves any explicit --port value
as the starting point, and logs the final chosen port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d0c9aac-cf9a-45a5-850b-f0cfddf3399a
📒 Files selected for processing (12)
admin/actions.goadmin/auth.goadmin/handlers.goadmin/sandbox_status.goadmin/server.goadmin/static/.gitkeepadmin/static/placeholder.txtadmin/templates/components/sandbox-list.htmladmin/templates/dashboard.htmladmin/templates/login.htmlcmd/admin.godocs/superpowers/specs/2026-03-24-admin-webui-poc-design.md
| func generateOTP() string { | ||
| b := make([]byte, 16) | ||
| _, _ = rand.Read(b) | ||
| return hex.EncodeToString(b) | ||
| } |
There was a problem hiding this comment.
Handle rand.Read error to avoid weak tokens on failure.
If rand.Read fails (rare but possible on systems with exhausted entropy), the buffer may contain zeros, producing a predictable token. For a security-sensitive value like an OTP or session ID, this error should be handled.
Proposed fix
-func generateOTP() string {
+func generateOTP() (string, error) {
b := make([]byte, 16)
- _, _ = rand.Read(b)
- return hex.EncodeToString(b)
+ if _, err := rand.Read(b); err != nil {
+ return "", fmt.Errorf("generating OTP: %w", err)
+ }
+ return hex.EncodeToString(b), nil
}This would require updating NewAuthManager and CreateSession to propagate or handle the error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/auth.go` around lines 69 - 73, generateOTP currently ignores errors
from rand.Read, risking predictable tokens on failure; change generateOTP to
return (string, error) and return an error if rand.Read fails, then update
callers (NewAuthManager and CreateSession) to handle the returned error by
propagating it (or failing session creation) rather than using a zeroed
token—ensure function signatures and all call sites are adjusted to handle the
error path and log/return a clear error when OTP generation fails.
- auth.go: panic on crypto/rand failure; store session expiry and check it in ValidateSession - handlers.go: path traversal guard on sandbox name; improved error message; handle GetAllSandboxes error after action - actions.go: log warning instead of silently ignoring stop failure during destroy - server.go: listen before opening browser to avoid race; add ReadTimeout/WriteTimeout/IdleTimeout; Windows support; openBrowser returns error - sandbox_status.go: replace os.Signal(nil) with syscall.Signal(0) for portable process liveness check - dashboard.html: add SRI integrity hash to HTMX CDN script tag - sandbox-list.html: URL-encode sandbox names in hx-post attributes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
admin/sandbox_status.go (1)
46-119: Sandbox list ordering is unstable due to map iteration.At Line 46, iterating a map directly makes dashboard card order nondeterministic between refreshes. Sorting keys first will keep HTMX updates stable.
Proposed refactor
import ( "fmt" "os" "path/filepath" + "sort" "strconv" "strings" "syscall" @@ - for name, item := range catalog { + keys := make([]string, 0, len(catalog)) + for name := range catalog { + keys = append(keys, name) + } + sort.Strings(keys) + + for _, name := range keys { + item := catalog[name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/sandbox_status.go` around lines 46 - 119, The loop iterates the map variable catalog directly which yields nondeterministic ordering; to stabilize sandbox list order, collect the map keys into a slice, sort them (e.g., using sort.Strings), and then iterate over the sorted keys to build SandboxStatus entries (the block that currently ranges "for name, item := range catalog"). Keep the rest of the logic intact (uses sandboxHome, SandboxStatus, item.Destination, item.Nodes, common.ReadSandboxDescription, isRunning and appends to result) but replace the direct map iteration with an iteration over the sorted key slice so HTMX/dashboard card order is stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/sandbox_status.go`:
- Around line 54-56: The code only strips '/' before using filepath.Base, so
Windows backslashes can leave full paths in sbName; update the normalization to
always call filepath.Base(sbName) (remove the conditional) where sbName is set
in the sandbox status logic so the Name is consistently the base name on all
OSes.
---
Nitpick comments:
In `@admin/sandbox_status.go`:
- Around line 46-119: The loop iterates the map variable catalog directly which
yields nondeterministic ordering; to stabilize sandbox list order, collect the
map keys into a slice, sort them (e.g., using sort.Strings), and then iterate
over the sorted keys to build SandboxStatus entries (the block that currently
ranges "for name, item := range catalog"). Keep the rest of the logic intact
(uses sandboxHome, SandboxStatus, item.Destination, item.Nodes,
common.ReadSandboxDescription, isRunning and appends to result) but replace the
direct map iteration with an iteration over the sorted key slice so
HTMX/dashboard card order is stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cee119e6-2fc1-45dc-a4a9-f36fafffebf7
📒 Files selected for processing (7)
admin/actions.goadmin/auth.goadmin/handlers.goadmin/sandbox_status.goadmin/server.goadmin/templates/components/sandbox-list.htmladmin/templates/dashboard.html
✅ Files skipped from review due to trivial changes (1)
- admin/templates/dashboard.html
🚧 Files skipped from review as they are similar to previous changes (3)
- admin/actions.go
- admin/auth.go
- admin/handlers.go
| if strings.Contains(sbName, "/") { | ||
| sbName = filepath.Base(sbName) | ||
| } |
There was a problem hiding this comment.
Normalize sandbox name with filepath.Base unconditionally.
At Line 54, checking only "/" misses Windows-style separators (\), so full paths can leak into .Name on Windows. Use filepath.Base(sbName) directly.
Proposed fix
- // If the name looks like a full path, extract just the base name.
- if strings.Contains(sbName, "/") {
- sbName = filepath.Base(sbName)
- }
+ // Extract display name from catalog key/path in a cross-platform way.
+ sbName = filepath.Base(sbName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.Contains(sbName, "/") { | |
| sbName = filepath.Base(sbName) | |
| } | |
| // Extract display name from catalog key/path in a cross-platform way. | |
| sbName = filepath.Base(sbName) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@admin/sandbox_status.go` around lines 54 - 56, The code only strips '/'
before using filepath.Base, so Windows backslashes can leave full paths in
sbName; update the normalization to always call filepath.Base(sbName) (remove
the conditional) where sbName is set in the sandbox status logic so the Name is
consistently the base name on all OSes.
- isRunning now executes the sandbox's own status script instead of hardcoding PID file paths. Falls back to PID check if no script. - URL-decode sandbox name in action handler (urlquery encoding in templates was causing mismatches).
- Add 2s delay after start/stop before reading status (scripts are async) - Add /api/sandboxes/refresh endpoint returning sandbox list HTML fragment - Dashboard auto-polls every 5s via HTMX hx-trigger="every 5s" - Status badges now update without manual page refresh
The status script always exits 0 regardless of sandbox state. It outputs '<name> on' or '<name> off'. Parse the text.
- Remove dbdeployer admin ui from README, quickstarts, and landing page (PR #56 not yet merged) - PostgreSQL README example now uses safe apt-get download + unpack approach instead of destructive apt-get install - Added "Why not just apt-get install?" section to PG quickstart explaining: no root needed, no conflict with running PG, multiple versions side by side - Restored "No Root, No Docker" feature card on landing page
Summary
POC for a localhost web dashboard that turns dbdeployer from a CLI tool into a platform.
dbdeployer admin uistarts HTTP server on 127.0.0.1:9090//go:embedArchitecture
Security
Test Plan