feat(admin): backup, upgrade, downgrade in System tab#601
Conversation
Add Deployment Management UI to the System tab that calls the host PHP updater API using the Comet admin session. Point update checks and commit links at master3395/comet and document stream deployment setup.
Keep the System tab commit link on the upstream repository.
Use upstream g0ldyy/comet for COMET_GITHUB_REPO default and deployment docs.
WalkthroughAdds a Deployment Management UI to the admin dashboard's System tab, including current tag/image display, update/refresh controls, job progress and logs, and a backups list with restore actions, along with client-side JavaScript to drive polling and API calls. Also makes the GitHub repository used for update checks configurable via an environment variable, and adds deployment documentation. ChangesDeployment Management UI
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard as Admin Dashboard JS
participant API as /operator/api/*
participant Job as Update/Restore Job
User->>Dashboard: Click "Update now"
Dashboard->>Dashboard: Confirm dialog, disable controls
Dashboard->>API: POST action (update, CSRF token)
API->>Job: Trigger job
loop Poll status
Dashboard->>API: GET status/logs
API-->>Dashboard: Return job status/progress/log
Dashboard->>Dashboard: Update progress bar, render backups
end
User->>Dashboard: Click "Restore" on backup
Dashboard->>Dashboard: Confirm dialog, disable restore button
Dashboard->>API: POST action (restore, CSRF token)
API->>Job: Trigger restore job
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comet/templates/admin_dashboard.html`:
- Around line 2517-2551: `systemUpdaterRenderBackups` is injecting untrusted
backup fields directly into `innerHTML`, which leaves the admin dashboard open
to stored XSS. Update this renderer to escape or safely insert `backup.id`,
`backup.created_at`, and `backup.tag`, and avoid raw string interpolation in the
`data-backup-id` attribute. Apply the same escaping/safe rendering approach in
`systemUpdaterRenderJob` for `job.status` and `job.message`, using a shared
escaping helper if available.
- Around line 2685-2744: Both systemUpdaterStartUpdate and
systemUpdaterStartRestore currently return immediately when systemUpdaterCsrf is
missing, which causes the Update now/Restore buttons to do nothing without
explanation. Update the early-exit path in these functions to show a user-facing
notice through systemUpdaterShowNotice (using a danger/error message) before
returning, and keep the existing button/progress reset behavior so the UI
clearly communicates that the action cannot start until CSRF is available.
- Around line 2631-2669: The dashboard state becomes stale after a job finishes
because systemUpdaterRenderJob() stops polling and updates UI notices, but never
refreshes the main operator status. In the completed/failed branch of
systemUpdaterRenderJob(), trigger a re-fetch of /operator/api/status (or the
existing status refresh helper used elsewhere) after stopping polling so the
Running tag, Image, and backups list update immediately. Keep the existing
button/notice handling intact, but ensure the status refresh runs when
job.status becomes completed or failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dc60ce6-2e05-4037-9360-961d237e0134
📒 Files selected for processing (3)
comet/templates/admin_dashboard.htmlcomet/utils/update.pydeployment/stream-updater.md
| function systemUpdaterRenderBackups(backups) { | ||
| const container = document.getElementById("system-updater-backups"); | ||
| if (!container) { | ||
| return; | ||
| } | ||
| if (!Array.isArray(backups) || backups.length === 0) { | ||
| container.innerHTML = '<div style="color: var(--sl-color-neutral-500); font-size: 0.9rem;">No backups yet. Run an update to create one automatically.</div>'; | ||
| return; | ||
| } | ||
| let rows = backups.map((backup) => { | ||
| const id = backup.id || ""; | ||
| const created = backup.created_at || "-"; | ||
| const tag = backup.tag || "-"; | ||
| return `<tr> | ||
| <td style="padding: 8px;"><code style="font-size: 0.75rem;">${id}</code></td> | ||
| <td style="padding: 8px;">${created}</td> | ||
| <td style="padding: 8px;">${tag}</td> | ||
| <td style="padding: 8px;"> | ||
| <sl-button size="small" variant="warning" outline class="system-updater-restore-btn" data-backup-id="${id}">Restore</sl-button> | ||
| </td> | ||
| </tr>`; | ||
| }).join(""); | ||
| container.innerHTML = `<table style="width: 100%; border-collapse: collapse; font-size: 0.85rem;"> | ||
| <thead><tr style="text-align: left; color: var(--sl-color-neutral-400);"> | ||
| <th style="padding: 8px;">ID</th><th style="padding: 8px;">Created</th><th style="padding: 8px;">Tag</th><th style="padding: 8px;">Action</th> | ||
| </tr></thead><tbody>${rows}</tbody></table>`; | ||
| container.querySelectorAll(".system-updater-restore-btn").forEach((btn) => { | ||
| btn.addEventListener("click", () => { | ||
| const backupId = btn.getAttribute("data-backup-id"); | ||
| if (backupId) { | ||
| systemUpdaterStartRestore(backupId, btn); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unescaped server data injected via innerHTML — stored XSS risk.
systemUpdaterRenderBackups interpolates backup.id, backup.created_at, and backup.tag directly into an HTML string assigned to innerHTML (Line 2530-2537), including inside an unquoted-safe attribute context (data-backup-id="${id}", Line 2535) — a value containing " would break out of the attribute and inject arbitrary markup/attributes. Similarly, systemUpdaterRenderJob's banner (Line 2643) interpolates job.status/job.message the same way.
These values ultimately originate from the update/restore action payload (tag, backup_id) which a caller with API access could set to an arbitrary string bypassing the <sl-select> UI constraint, then have it persisted and rendered to any admin viewing the dashboard later — a stored XSS against an authenticated admin session.
🔒 Proposed fix using an escaping helper
+ function systemUpdaterEscapeHtml(value) {
+ return String(value == null ? "" : value).replace(/[&<>"']/g, (c) => (
+ { "&": "&", "<": "<", ">": ">", '"': """, "'": "&`#39`;" }[c]
+ ));
+ }
+
function systemUpdaterRenderBackups(backups) {
const container = document.getElementById("system-updater-backups");
if (!container) {
return;
}
if (!Array.isArray(backups) || backups.length === 0) {
container.innerHTML = '<div style="color: var(--sl-color-neutral-500); font-size: 0.9rem;">No backups yet. Run an update to create one automatically.</div>';
return;
}
let rows = backups.map((backup) => {
- const id = backup.id || "";
- const created = backup.created_at || "-";
- const tag = backup.tag || "-";
+ const id = systemUpdaterEscapeHtml(backup.id || "");
+ const created = systemUpdaterEscapeHtml(backup.created_at || "-");
+ const tag = systemUpdaterEscapeHtml(backup.tag || "-");
return `<tr>Apply the same escaping to job.status/job.message in systemUpdaterRenderJob.
Also applies to: 2631-2649
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 2538-2541: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: container.innerHTML = <table style="width: 100%; border-collapse: collapse; font-size: 0.85rem;"> <thead><tr style="text-align: left; color: var(--sl-color-neutral-400);"> <th style="padding: 8px;">ID</th><th style="padding: 8px;">Created</th><th style="padding: 8px;">Tag</th><th style="padding: 8px;">Action</th> </tr></thead><tbody>${rows}</tbody></table>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@comet/templates/admin_dashboard.html` around lines 2517 - 2551,
`systemUpdaterRenderBackups` is injecting untrusted backup fields directly into
`innerHTML`, which leaves the admin dashboard open to stored XSS. Update this
renderer to escape or safely insert `backup.id`, `backup.created_at`, and
`backup.tag`, and avoid raw string interpolation in the `data-backup-id`
attribute. Apply the same escaping/safe rendering approach in
`systemUpdaterRenderJob` for `job.status` and `job.message`, using a shared
escaping helper if available.
Source: Linters/SAST tools
| function systemUpdaterRenderJob(job) { | ||
| const banner = document.getElementById("system-updater-job-banner"); | ||
| const logBox = document.getElementById("system-updater-log"); | ||
| const updateBtn = document.getElementById("system-updater-btn-update"); | ||
| if (banner && job) { | ||
| banner.hidden = false; | ||
| const color = job.status === "completed" ? "var(--sl-color-success-500)" : job.status === "failed" ? "var(--sl-color-danger-500)" : "var(--sl-color-primary-500)"; | ||
| banner.style.padding = "10px 12px"; | ||
| banner.style.borderRadius = "6px"; | ||
| banner.style.fontSize = "0.9rem"; | ||
| banner.style.color = color; | ||
| banner.style.background = "rgba(255,255,255,0.05)"; | ||
| banner.innerHTML = `<strong>Job:</strong> ${job.status || "unknown"} - ${job.message || ""}`; | ||
| } | ||
| systemUpdaterUpdateProgress(job); | ||
| if (logBox && job.job_log_tail) { | ||
| logBox.hidden = false; | ||
| logBox.textContent = job.job_log_tail; | ||
| } | ||
| if (job && (job.status === "completed" || job.status === "failed")) { | ||
| systemUpdaterStopPolling(); | ||
| systemUpdaterLockNoticeShown = false; | ||
| if (updateBtn) { | ||
| updateBtn.disabled = false; | ||
| } | ||
| document.querySelectorAll(".system-updater-restore-btn").forEach((b) => { | ||
| b.disabled = false; | ||
| }); | ||
| if (job.status === "completed") { | ||
| systemUpdaterShowNotice(job.message || "Job completed successfully.", "success"); | ||
| } else if (job.status === "failed") { | ||
| systemUpdaterShowNotice(job.message || "Job failed. Check the log below.", "danger"); | ||
| } | ||
| } else if (job && (job.status === "running" || job.status === "queued")) { | ||
| if (updateBtn) { | ||
| updateBtn.disabled = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dashboard state goes stale after a job finishes.
When a job transitions to completed/failed (Line 2650-2663), the code stops polling and updates the notice/button state, but never re-fetches /operator/api/status. After a successful update, "Running tag"/"Image" and the backups list (which should now include a fresh pre-update backup) stay stale until the admin manually clicks Refresh or switches tabs — misleading feedback right after the primary action of this feature.
♻️ Proposed fix
if (job.status === "completed") {
systemUpdaterShowNotice(job.message || "Job completed successfully.", "success");
+ loadSystemUpdater();
} else if (job.status === "failed") {
systemUpdaterShowNotice(job.message || "Job failed. Check the log below.", "danger");
+ loadSystemUpdater();
}📝 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.
| function systemUpdaterRenderJob(job) { | |
| const banner = document.getElementById("system-updater-job-banner"); | |
| const logBox = document.getElementById("system-updater-log"); | |
| const updateBtn = document.getElementById("system-updater-btn-update"); | |
| if (banner && job) { | |
| banner.hidden = false; | |
| const color = job.status === "completed" ? "var(--sl-color-success-500)" : job.status === "failed" ? "var(--sl-color-danger-500)" : "var(--sl-color-primary-500)"; | |
| banner.style.padding = "10px 12px"; | |
| banner.style.borderRadius = "6px"; | |
| banner.style.fontSize = "0.9rem"; | |
| banner.style.color = color; | |
| banner.style.background = "rgba(255,255,255,0.05)"; | |
| banner.innerHTML = `<strong>Job:</strong> ${job.status || "unknown"} - ${job.message || ""}`; | |
| } | |
| systemUpdaterUpdateProgress(job); | |
| if (logBox && job.job_log_tail) { | |
| logBox.hidden = false; | |
| logBox.textContent = job.job_log_tail; | |
| } | |
| if (job && (job.status === "completed" || job.status === "failed")) { | |
| systemUpdaterStopPolling(); | |
| systemUpdaterLockNoticeShown = false; | |
| if (updateBtn) { | |
| updateBtn.disabled = false; | |
| } | |
| document.querySelectorAll(".system-updater-restore-btn").forEach((b) => { | |
| b.disabled = false; | |
| }); | |
| if (job.status === "completed") { | |
| systemUpdaterShowNotice(job.message || "Job completed successfully.", "success"); | |
| } else if (job.status === "failed") { | |
| systemUpdaterShowNotice(job.message || "Job failed. Check the log below.", "danger"); | |
| } | |
| } else if (job && (job.status === "running" || job.status === "queued")) { | |
| if (updateBtn) { | |
| updateBtn.disabled = true; | |
| } | |
| } | |
| } | |
| function systemUpdaterRenderJob(job) { | |
| const banner = document.getElementById("system-updater-job-banner"); | |
| const logBox = document.getElementById("system-updater-log"); | |
| const updateBtn = document.getElementById("system-updater-btn-update"); | |
| if (banner && job) { | |
| banner.hidden = false; | |
| const color = job.status === "completed" ? "var(--sl-color-success-500)" : job.status === "failed" ? "var(--sl-color-danger-500)" : "var(--sl-color-primary-500)"; | |
| banner.style.padding = "10px 12px"; | |
| banner.style.borderRadius = "6px"; | |
| banner.style.fontSize = "0.9rem"; | |
| banner.style.color = color; | |
| banner.style.background = "rgba(255,255,255,0.05)"; | |
| banner.innerHTML = `<strong>Job:</strong> ${job.status || "unknown"} - ${job.message || ""}`; | |
| } | |
| systemUpdaterUpdateProgress(job); | |
| if (logBox && job.job_log_tail) { | |
| logBox.hidden = false; | |
| logBox.textContent = job.job_log_tail; | |
| } | |
| if (job && (job.status === "completed" || job.status === "failed")) { | |
| systemUpdaterStopPolling(); | |
| systemUpdaterLockNoticeShown = false; | |
| if (updateBtn) { | |
| updateBtn.disabled = false; | |
| } | |
| document.querySelectorAll(".system-updater-restore-btn").forEach((b) => { | |
| b.disabled = false; | |
| }); | |
| if (job.status === "completed") { | |
| systemUpdaterShowNotice(job.message || "Job completed successfully.", "success"); | |
| loadSystemUpdater(); | |
| } else if (job.status === "failed") { | |
| systemUpdaterShowNotice(job.message || "Job failed. Check the log below.", "danger"); | |
| loadSystemUpdater(); | |
| } | |
| } else if (job && (job.status === "running" || job.status === "queued")) { | |
| if (updateBtn) { | |
| updateBtn.disabled = true; | |
| } | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 2642-2642: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: banner.innerHTML = <strong>Job:</strong> ${job.status || "unknown"} - ${job.message || ""}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@comet/templates/admin_dashboard.html` around lines 2631 - 2669, The dashboard
state becomes stale after a job finishes because systemUpdaterRenderJob() stops
polling and updates UI notices, but never refreshes the main operator status. In
the completed/failed branch of systemUpdaterRenderJob(), trigger a re-fetch of
/operator/api/status (or the existing status refresh helper used elsewhere)
after stopping polling so the Running tag, Image, and backups list update
immediately. Keep the existing button/notice handling intact, but ensure the
status refresh runs when job.status becomes completed or failed.
| async function systemUpdaterStartUpdate() { | ||
| const tagSelect = document.getElementById("system-updater-tag"); | ||
| const updateBtn = document.getElementById("system-updater-btn-update"); | ||
| if (!tagSelect || !systemUpdaterCsrf) { | ||
| return; | ||
| } | ||
| const tag = tagSelect.value || "main"; | ||
| if (!confirm('Create a backup and update Comet to tag "' + tag + '"?')) { | ||
| return; | ||
| } | ||
| systemUpdaterClearNotice(); | ||
| systemUpdaterLockNoticeShown = false; | ||
| if (updateBtn) { | ||
| updateBtn.disabled = true; | ||
| } | ||
| systemUpdaterUpdateProgress({ status: "queued", progress_percent: 2, progress_label: "Starting update..." }); | ||
| try { | ||
| const data = await systemUpdaterApi("action", { | ||
| method: "POST", | ||
| body: JSON.stringify({ action: "update", tag: tag, csrf: systemUpdaterCsrf }), | ||
| }); | ||
| systemUpdaterShowNotice(data.message || "Update started.", "success"); | ||
| systemUpdaterStartPolling(); | ||
| } catch (err) { | ||
| if (updateBtn) { | ||
| updateBtn.disabled = false; | ||
| } | ||
| systemUpdaterUpdateProgress(null); | ||
| systemUpdaterShowNotice(err.message || "Update failed to start.", "danger"); | ||
| } | ||
| } | ||
|
|
||
| async function systemUpdaterStartRestore(backupId, button) { | ||
| if (!systemUpdaterCsrf) { | ||
| return; | ||
| } | ||
| if (!confirm('Restore Comet from backup "' + backupId + '"?')) { | ||
| return; | ||
| } | ||
| systemUpdaterClearNotice(); | ||
| systemUpdaterLockNoticeShown = false; | ||
| if (button) { | ||
| button.disabled = true; | ||
| } | ||
| systemUpdaterUpdateProgress({ status: "queued", progress_percent: 2, progress_label: "Starting restore..." }); | ||
| try { | ||
| const data = await systemUpdaterApi("action", { | ||
| method: "POST", | ||
| body: JSON.stringify({ action: "restore", backup_id: backupId, csrf: systemUpdaterCsrf }), | ||
| }); | ||
| systemUpdaterShowNotice(data.message || "Restore started.", "success"); | ||
| systemUpdaterStartPolling(); | ||
| } catch (err) { | ||
| if (button) { | ||
| button.disabled = false; | ||
| } | ||
| systemUpdaterUpdateProgress(null); | ||
| systemUpdaterShowNotice(err.message || "Restore failed to start.", "danger"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent no-op when CSRF token isn't loaded yet.
Both systemUpdaterStartUpdate (Line 2688) and systemUpdaterStartRestore (Line 2718) return early with no user feedback if systemUpdaterCsrf hasn't been populated yet (e.g., the initial status call failed). Clicking Update now/Restore then silently does nothing, which is confusing.
💡 Proposed fix
if (!tagSelect || !systemUpdaterCsrf) {
+ systemUpdaterShowNotice("Deployment status not loaded yet. Please wait or refresh.", "warning");
return;
}Apply similarly in systemUpdaterStartRestore.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@comet/templates/admin_dashboard.html` around lines 2685 - 2744, Both
systemUpdaterStartUpdate and systemUpdaterStartRestore currently return
immediately when systemUpdaterCsrf is missing, which causes the Update
now/Restore buttons to do nothing without explanation. Update the early-exit
path in these functions to show a user-facing notice through
systemUpdaterShowNotice (using a danger/error message) before returning, and
keep the existing button/progress reset behavior so the UI clearly communicates
that the action cannot start until CSRF is available.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6681ec05c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (tag === recommended) { | ||
| option.selected = true; |
There was a problem hiding this comment.
When the updater API returns a recommended_tag other than the current empty/default value, this does not actually select it because Shoelace options only expose value/disabled; selection is controlled by the containing sl-select.value. As a result, the dropdown can remain unset and systemUpdaterStartUpdate() falls back to main, so an admin who accepts the default can update to main instead of the recommended release/tag. Set select.value = recommended after appending the options instead.
Useful? React with 👍 / 👎.
1 participant
Summary
Test plan
Summary by CodeRabbit
New Features
Documentation
Chores