Server: accounts and permissions, browse server files, file explorer window, self-update/restart/stop, profiles onto the server by epell-development · Pull Request #21 · epell-development/RadioMaster

epell-development · 2026-10-25T22:39:35Z

Requested by epell · project thread

Makes the desktop app work properly against RadioMaster Server (v0.2.9 list: server update/stop/restart, remote user management, profile load with mapping, generator categories and cart wall browsing server files, file explorer window, server side for integrations).

What changes

Server accounts and permissions. The server keeps its own operators.json (auth::Accounts); a pre-0.2.9 server's single account becomes the first administrator automatically. Every API call is checked against the signed-in account: Cmd::allowed per action, sync::cfg_denied per settings section, edit_playlists for playlist pushes, admin-only for server settings/users/update/restart/stop. A disabled or deleted account is signed out at once. Login replies carry admin and perms; the app shows only what both the local and server account allow. File → Server users… reuses the Users grid (now users_editor, shared with the local window). Refused edits come back as the server's copy with a message instead of a retry loop.
Server files. New /api/v1/fs/{list,expand,m3u,upload} and GET /fs/file, plus link::Fs (local or server). Every audio/folder/M3U picker goes through App::pick: system dialog locally, a server folder browser when connected (with Upload from this computer…). Covers Add files/folder, library folders, generator categories, cart wall, adverts and rules, events, speech sounds, track lists, containers, voice tracks, open/insert playlist. Files dropped from the computer and recorded voice tracks are uploaded to a chosen server folder.
File explorer: also a window (Tools → File explorer), All files toggle, works on the server's folders.
Self-update, restart, stop (selfupdate.rs): the server checks GitHub every 6 h (--no-update-check to disable); the app shows a blue ⬆ Server button like its own update button and opens the update window once for admins. Update downloads the server package for Linux/Windows x64, swaps the program and restarts (systemd: exit 75 + Restart=on-failure; RADIOMASTER_SUPERVISED=1 for NSSM; otherwise it starts a new copy that waits for the ports). Restart/Stop ask "Restart!/Keep" first.
Station profiles onto the server: Load while connected lists the profile's folders to map to server folders (type or browse), rewrites every path under them, optionally loads its playlists; Import… adds a profile file.
Integrations: Reports read the server's history (/api/v1/plays); Library cleanup is off while connected (it works on local files).
Docs: SERVER.md sections on accounts, updating, server files, profiles; the systemd unit installs to /opt/radiomaster so the service account can update it.

Note for the other open server branches: their new Cmd variants need an arm in Cmd::allowed, and new Api::call routes should check p/me.admin like the rest.

Testing

cargo test (1.99.0): new server_accounts_and_permissions (presenter refused settings/encoders/users/stop, allowed playlist edits, disabled account signed out, restart reaches the host) and server_files (listing, audio filter, upload without overwrite, file download, auth), plus sync tests for path mapping and permission rules and selfupdate tests for the tar.gz/zip readers.
clippy -D warnings, fmt, extract_strings, check_glyphs, check_versions, smoke_server.sh all pass locally.
Not tried on real hardware: the in-app server update (needs a release with server packages), the server file browser UI by eye.

CHANGELOG lines for v0.2.9

New

RadioMaster Server has its own user accounts with permissions; administrators manage them with File → Server users. The server checks every action.
While connected, file and folder pickers (playlist generator categories, the jingle cart wall, adverts, events, library folders…) browse the server's folders, with Upload from this computer.
The file explorer can open in its own window (Tools → File explorer) and show all files; it works with the server.
The app says when the server is behind, and administrators can update, restart or stop it from the app.
Station profiles load onto the server with their folders mapped to the server's; profiles can be imported.

Changed

Reports use the server's play history while connected; dropped files and recorded voice tracks are uploaded to the server.

Generated by Claude Code

… restart/stop, profiles onto the server - The server keeps its own accounts (operators.json; the old single account becomes the first administrator) and checks every call against the signed-in account's permissions. Administrators manage them in the app with File > Server users. - File pickers (playlist files and folders, library folders, generator categories, the cart wall, adverts, rules, events, speech sounds, M3Us) browse the server's folders while connected, with Upload from this computer. Dropped files and recorded voice tracks are uploaded too. - File explorer: also a window of its own (Tools), All files, works with the server. - The server checks GitHub for releases; the app shows a Server update button and administrators can update, restart or stop the server. - Station profiles load onto the server with their folders mapped to the server's; Import. - Reports use the server's history; Library cleanup is off while connected. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

MergeGuard review
Risk: 🛑 83/100 — high risk
Findings: 🛑 1 critical · 🔴 1 high · 🟠 1 medium

Recommended fixes

src/server.rs — Harden send_file by canonicalizing the requested path and verifying it resides within approved roots or matches known station files, preventing path traversal via '..'. (confidence 88%)
src/server.rs — Restrict uploads to configured library folders by canonicalizing the target directory and ensuring it is within whitelisted roots; otherwise return 403. (confidence 86%)
src/server.rs — Restrict fs/list browsing to paths under configured library folders by canonicalizing the requested path and enforcing a whitelist. (confidence 78%)

Security review — 🛑 1 critical · 🔴 1 high · 🟠 1 medium (3 findings)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 3 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-25T22:40:21Z

+        let ok = me.admin || {
+            let c = self.core.lock();
+            c.cfg.library_folders.iter().any(|f| path.starts_with(f))
+                || c.library.iter().any(|t| t.path == path)


🛡️ MergeGuard · 🛑 critical · security

Path traversal / access control bypass in file download endpoint: path prefix check uses Path::starts_with on uncanonicalized input, allowing sequences like .. to escape library roots and download arbitrary files.

💡 Canonicalize the requested path and allowed roots before comparison. Reject paths that fail canonicalization or are outside whitelisted roots (library folders and explicitly referenced files).

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Confirmed and fixed: Path::starts_with compares components, so <library>/../x passed. send_file now checks library folders with within(), which canonicalizes both sides (links and .. resolved). The other allowed case is an exact match on a path the station itself uses. The new server_files test asks for <library>/../<file> as a presenter and gets a refusal.

Generated by Claude Code

mergeguards · 2026-10-25T22:40:21Z

+        let name = Path::new(name).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default();
+        if dir.is_empty() || name.is_empty() || name.starts_with('.') {
+            return error(400, "a folder and a file name are needed");
+        }


🛡️ MergeGuard · 🔴 high · security

Arbitrary file write via upload endpoint: accepts any existing directory on the server. A non-admin user with edit_playlists or jingles permission can write files anywhere the process can, potentially leading to privilege escalation or overwriting sensitive files.

💡 Restrict uploads to configured library folders (or a dedicated upload root) by canonicalizing the target dir and ensuring it is within an allowed root. Return 403 otherwise.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Fixed. Non-admin uploads now go only into a library folder, checked on the canonical path. Otherwise the server answers 403. server_files covers both <library>/.. and the temp dir.

Generated by Claude Code

mergeguards · 2026-10-25T22:40:21Z

+                }
+            }
+            ("/api/v1/fs/list" | "/api/v1/fs/expand" | "/api/v1/fs/m3u", true) if !files_ok => denied("browse the server's files"),
+            ("/api/v1/fs/list", true) => {


🛡️ MergeGuard · 🟠 medium · security

Server-side folder listing endpoint exposes arbitrary filesystem paths to non-admins with certain permissions. Without restricting to whitelisted roots, this can leak directory structure or filenames outside the station library.

💡 Canonicalize requested paths and enforce they are within approved roots (e.g., library folders) before listing; otherwise return 403.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Fixed for accounts without the settings permission. fs/list, fs/expand and fs/m3u now stay inside the canonicalized library folders, and places and parent are filtered to match. Accounts with settings keep wide browsing on purpose: they choose the library folders, so they could add any folder anyway.

Generated by Claude Code

mergeguards · 2026-10-25T22:40:24Z

MergeGuard agent actions

Fix all findings

All issues (3)

medium · src/server.rs:821 — Information disclosure: error responses from the upload endpoint include full filesystem paths (e.g., " is not a folder on the server"). Disclosing absolute paths can aid attackers in reconna…
medium · src/server.rs:1123 — Potentially lossy query decoding for '+' characters. query_str replaces '+' with space before URL-decoding, which will incorrectly turn literal '+' in file/folder names into spaces for non-app clie…
medium · src/server.rs:862 — Wide file read capability for admins via GET /api/v1/fs/file. If an admin token is compromised, arbitrary files on the server can be exfiltrated.

Apply auto-fixes for the findings above on this branch:

Fix All MergeGuard Findings

_{Or reply with @mergeguards fix-all on this pull request.}

Downloads, uploads, folder listing, expand and m3u resolve links and '..' before checking the library folders, so a path can't climb out of them. Accounts that may change settings keep wide browsing (they set the library folders). Also fixes a race in the accounts test: stopping the link signs its session out in the background. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟢 24/100 — low risk
Findings: 🟠 3 medium

Recommended fixes

src/link.rs — Stream client-side uploads to avoid loading entire files into memory. (confidence 78%)

Security review — 🟠 1 medium (1 finding)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 3 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-25T22:44:16Z

+
+/// Download `asset` and put its program in place of the one running (blocks). `progress` gets a
+/// short status line now and then. The running program carries on until it is started again.
+pub fn install(asset: &Asset, progress: &dyn Fn(String)) -> Result<(), String> {


🛡️ MergeGuard · 🟠 medium · security

Self-update downloads and installs a new server binary without verifying a cryptographic checksum or signature. This relies solely on TLS and basic file magic checks, which does not protect against supply-chain tampering or compromised distribution endpoints.

💡 Verify a signed checksum (e.g., SHA-256) or a detached signature for the downloaded asset before replacing the running binary. Fetch a checksum/signature artifact from the release and validate the content before proceeding.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Not fixing this in this PR. The releases publish no checksum or signature file, and the app's own updater (src/updater.rs) trusts the same GitHub release over TLS. A SHA-256 file served from that same release would not protect against a tampered release, so it only adds something with real signing. That means a signing key kept outside GitHub plus a release workflow change, which is a separate decision for the maintainer. Only administrators can trigger the update, and only from the official repo's releases.

Generated by Claude Code

mergeguards · 2026-10-25T22:44:16Z

+    /// Copy a file from this computer into `dir` on the server; returns where it went there.
+    pub fn upload(&self, file: &std::path::Path, dir: &std::path::Path) -> Result<std::path::PathBuf, String> {
+        let Fs::Server(s) = self else { return Err("not connected to a server".into()) };
+        let data = std::fs::read(file).map_err(|e| format!("{}: {e}", file.display()))?;


🛡️ MergeGuard · 🟠 medium · performance

Client-side upload reads the entire file into memory before POSTing it to the server. This can cause excessive memory usage for large files.

💡 Stream file uploads using a File reader with the HTTP client (e.g., ureq::Agent::send_reader) to avoid loading the entire file into memory.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Fixed in cc604cc. The app now passes the open File to ureq send() with an explicit Content-Length, so the file is read as it is sent and never held in memory.

Generated by Claude Code

mergeguards · 2026-10-25T22:44:16Z

+            };
+        }
        let mut body = Vec::new();
        if req.as_reader().take(256 * 1024 * 1024).read_to_end(&mut body).is_err() {


🛡️ MergeGuard · 🟠 medium · performance

All POST bodies (including file uploads) are read into memory up to 256 MB before handling. This can lead to high memory usage and prevents streaming large files.

💡 Special-case the /api/v1/fs/upload route to stream the request body directly to disk (e.g., using Request::as_reader() into a tempfile) rather than buffering the entire upload in memory.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Fixed in cc604cc. /api/v1/fs/upload is now handled before any body is read: it checks the sign-in and permission first, then streams the upload straight to disk with a 4 GB cap and removes a partial file if the upload fails. The general body cap drops from 256 MB to 64 MB.

Generated by Claude Code

The app sends the file as it reads it; the server writes it straight to disk (up to 4 GB, removing a partial file on failure) and only after the sign-in and permission check. Other request bodies are capped at 64 MB. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🔴 60/100 — risky
Findings: 🔴 1 high · 🟠 1 medium

Recommended fixes

src/selfupdate.rs — Harden self-update by enforcing content-length match and verifying the downloaded asset against a release-provided SHA-256 checksum file when available. (confidence 62%)
src/server.rs — Make maximum upload size configurable to mitigate disk exhaustion; add a new ServerCfg field and enforce it in the upload path. (confidence 55%)

Security review — 🔴 1 high · 🟠 1 medium (2 findings)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 2 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-25T22:46:34Z

+/// Download `asset` and put its program in place of the one running (blocks). `progress` gets a
+/// short status line now and then. The running program carries on until it is started again.
+pub fn install(asset: &Asset, progress: &dyn Fn(String)) -> Result<(), String> {
+    let resp = agent()


🛡️ MergeGuard · 🔴 high · security

Server self-update downloads and installs an executable from GitHub without any cryptographic verification. Only basic content-type heuristics and minimal magic-bytes checks are performed, leaving the update path vulnerable if the download is tampered with (e.g., account takeover, compromised CDN, or TOFU of GitHub’s infra).

💡 Verify the downloaded asset with a strong integrity check before replacing the server binary. Recommended approaches: (1) verify a detached signature (e.g., minisign/ed25519) using an embedded public key; or (2) compare the computed SHA-256 of the asset against a trusted checksum provided alongside the release (and pinned to the repository or embedded). Also ensure the full content length matches expectations.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Partly addressed in 8a5383b. check_download now requires the download to match the asset's full size and the SHA-256 that GitHub's release API lists for it (digest), so a truncated or altered download is refused. A test covers this. It does not protect against a compromised repo or release account; that needs detached signing with a key kept outside GitHub, which is a release-process decision for the maintainer and out of scope here.

Generated by Claude Code

mergeguards · 2026-10-25T22:46:34Z

 }

+/// The biggest file an app may upload.
+const UPLOAD_MAX: u64 = 4 * 1024 * 1024 * 1024;


🛡️ MergeGuard · 🟠 medium · security

The file upload endpoint allows uploads up to 4 GB (UPLOAD_MAX) and stores directly to disk. While path restrictions are enforced, there is no server-side quota or configurable limit per account/session, risking storage exhaustion/DoS via large or repeated uploads.

💡 Introduce a configurable upload size limit (per system and/or per account), enforce cumulative quotas, and add rate limiting. Consider making the upload limit part of ServerCfg (server.json) and exposing an admin setting to adjust it safely.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Not changing this. Only signed-in accounts the station's administrator created, with playlist or jingle permission, can upload, and only into the library folders. They are trusted staff of the station, the same people who can already fill its playlists. Per-account quotas and rate limits would be a new feature rather than a fix for this PR. The 4 GB cap is per file, so one request can't fill the disk by itself.

Generated by Claude Code

The download must be the release file's full size and match the SHA-256 GitHub lists for it (asset digest), when the release has one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟠 35/100 — moderate
Findings: 🟠 2 medium

Recommended fixes

src/selfupdate.rs — Enforce integrity verification: require a SHA-256 digest or a checksum asset; fail if missing. This prevents installing unverified binaries. (confidence 68%)
src/server.rs — Reduce upload size for non-admins and restrict uploads to audio files using known extensions. This mitigates disk exhaustion and abuse. (confidence 73%)

Security review — 🟠 2 medium (2 findings)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 2 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-25T22:48:26Z

+    Ok(r)
+}
+
+/// Download `asset` and put its program in place of the one running (blocks). `progress` gets a


🛡️ MergeGuard · 🟠 medium · security

Server self-update downloads and installs binary without guaranteed integrity verification. If the asset digest is absent (common on GitHub Releases API), only size and HTTPS protect the download, increasing supply-chain risk.

💡 Require a verified checksum or signature before installing. Fail the update if no digest is available, or fetch and verify against a published checksum/signature asset.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Done in eaf65da: if the release lists no sha256: digest, the update fails ("update by hand") instead of installing. GitHub gives a digest to every release file uploaded now, so normal updates are unaffected. The test covers the missing-digest case.

Generated by Claude Code

mergeguards · 2026-10-25T22:48:26Z

+    }
+
+    /// A file from this computer, put in a folder on the server (never over a file that is there).
+    fn upload(&self, dir: &str, name: &str, body: &mut dyn Read, me: &Operator) -> Reply {


🛡️ MergeGuard · 🟠 medium · security

File upload endpoint allows any file type up to 4 GB for non-admins with edit_playlists/jingles permission. This can enable disk exhaustion or abuse by lower-privileged accounts.

💡 Enforce stricter limits for non-admin uploads (smaller size cap) and restrict to known audio extensions. Consider making the limit configurable and tracking quota/usage.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Done in eaf65da. Non-admins can upload only audio files and .m3u/.m3u8 playlists, with a cap of 1 GB per file; administrators keep the 4 GB cap. server_files checks that a non-audio file is refused. Quotas and rate limits stay out of scope, as I said above.

Generated by Claude Code

…oads An update download with no SHA-256 listed is not installed. Accounts other than administrators upload only audio files and playlists, up to 1 GB each. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟠 34/100 — moderate
Findings: 🟠 2 medium

Recommended fixes

src/selfupdate.rs — Reduce in-memory buffering during self-update by tightening the cap and failing early on oversized assets. (confidence 68%)
src/server.rs — Lower default per-file upload limit for non-admin accounts to reduce resource exhaustion risk. (confidence 72%)

Security review — 🟠 1 medium (1 finding)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 2 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-25T22:50:29Z

+        .call()
+        .map_err(|e| e.to_string())?;
+    let total = resp.header("Content-Length").and_then(|v| v.parse().ok()).unwrap_or(asset.size).max(1);
+    let mut reader = resp.into_reader().take(512 * 1024 * 1024);


🛡️ MergeGuard · 🟠 medium · performance

The server self-update downloads and buffers the entire release archive into memory (up to 512 MB) before processing it. This can cause significant memory pressure or OOM on constrained systems during updates.

💡 Stream the download to a temporary file and compute SHA-256 incrementally, then extract the target program from the archive using streaming readers. Avoid holding the full archive in memory.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Tightened in bc94c39. The cap is now 200 MB, down from 512 MB. An asset whose listed size is larger is refused before anything is downloaded, and the buffer's starting capacity is clamped to that cap. The server archives are tens of MB, so holding one in memory during an update that an admin starts is acceptable. A temp-file streaming extractor would add complexity without a real gain here.

Generated by Claude Code

mergeguards · 2026-10-25T22:50:30Z

 }

+/// The biggest file an administrator may upload…
+const UPLOAD_MAX: u64 = 4 * 1024 * 1024 * 1024;


🛡️ MergeGuard · 🟠 medium · security

Non-admin file uploads allow up to 1 GB per file (UPLOAD_MAX_STAFF). Malicious or compromised presenter accounts could abuse this to exhaust disk space (resource exhaustion DoS).

💡 Lower the default non-admin per-file upload limit and/or make it configurable. Optionally check available free space before accepting uploads and reject when below a safe threshold.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Keeping 1 GB. Presenters upload long recordings (a 2-hour show as WAV is over 1 GB), and these are accounts the station's administrator created. A configurable limit or disk-space quota is a possible follow-up feature, but it isn't needed to merge this.

Generated by Claude Code

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🔴 60/100 — risky
Findings: 🔴 1 high · 🟠 1 medium

Recommended fixes

src/server.rs — Harden /api/v1/fs/file so non-admins can only download files that are both within library folders AND referenced by the station. This closes the exfiltration path via crafted playlist/cart/tab entries. (confidence 90%)
src/server.rs — Reject playlist/tab pushes from non-admins that reference files outside library folders. This reduces the chance of planting out-of-library paths that could be abused elsewhere. (confidence 70%)

Security review — 🔴 1 high · 🟠 1 medium (2 findings)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 2 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-25T22:51:48Z

+    /// the station uses: in a library folder, the library, a playlist or the cart wall.
+    fn send_file(&self, req: tiny_http::Request, me: &Operator, path: &str) {
+        let path = std::path::PathBuf::from(path);
+        let ok = me.admin || within(&self.library_roots(), &path) || {


🛡️ MergeGuard · 🔴 high · security

Non-admin users can exfiltrate arbitrary files by first inserting playlist/cart entries pointing outside library folders, then downloading them via /api/v1/fs/file. send_file() currently allows any path referenced in current playlist/tabs/carts without also enforcing that it lies within library roots.

💡 Tighten send_file() authorization for non-admins to require that the requested path is within a configured library folder AND (optionally) is referenced by the station (library/playlist/tabs/carts). Do not permit paths outside library roots even if referenced.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Valid, and fixed in b49d75c. For non-admins, send_file now checks only within(library_roots). A file named by a playlist row, tab or cart no longer counts on its own, and docs/SERVER.md says the same.

Generated by Claude Code

mergeguards · 2026-10-25T22:51:48Z

+const UPLOAD_MAX: u64 = 4 * 1024 * 1024 * 1024;
+/// …and anyone else.
+const UPLOAD_MAX_STAFF: u64 = 1024 * 1024 * 1024;
+


🛡️ MergeGuard · 🟠 medium · security

Large authenticated uploads (up to 1 GiB for non-admins, 4 GiB for admins) to arbitrary folders (admins) can lead to disk exhaustion/DoS. There is no quota, free-space check, or rate limiting on /api/v1/fs/upload.

💡 Consider adding free disk space checks before accepting uploads, configurable size limits, and/or quotas per account or per session. Alternatively restrict upload destinations and sizes further and add rate limiting.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Same finding as the two earlier threads. Quotas, free-space checks and rate limits are left as a possible follow-up feature, not part of this PR.

Generated by Claude Code

A playlist row or cart naming a file elsewhere no longer makes it downloadable, since a presenter can put any path in a playlist. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟢 0/100 — low risk
Findings: none

Reviewed by MergeGuard AI

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟠 35/100 — moderate
Findings: 🟠 3 medium

Recommended fixes

src/auth.rs — Avoid exposing password_hash via Debug; implement a redacting Debug for Operator. (confidence 80%)
src/serveradmin.rs — Only show server update notification when latest is newer than the server's current version (and/or the app). (confidence 75%)
src/server.rs — Prevent accidental '+' to space conversion when decoding path and name query parameters; perform strict percent-decoding only. (confidence 55%)

Security review — 🟠 1 medium (1 finding)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 3 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-26T03:38:28Z

 }

-#[derive(Clone, Serialize, Deserialize)]
+#[derive(Clone, Serialize, Deserialize, Debug)]


🛡️ MergeGuard · 🟠 medium · security

Operator is derived with Debug, which will print sensitive fields such as password_hash if ever logged or formatted via {:?}. This increases the risk of credential hash exposure in logs or panic reports.

💡 Remove Debug on Operator or implement a custom Debug that redacts password_hash and other sensitive fields.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Fixed in 6de73c0. Operator now has a hand-written Debug that leaves out password_hash.

Generated by Claude Code

mergeguards · 2026-10-26T03:38:28Z

+        Some(info.latest.clone())
+    } else if crate::updater::is_newer(app, &info.version) {
+        Some(app.to_string())
+    } else if !info.latest.is_empty() {


🛡️ MergeGuard · 🟠 medium · bug

The update notification logic may show a Server update banner even when the server is already at the latest version. behind() returns Some(latest) whenever info.latest is present, without checking if it's newer than the currently running server version.

💡 Only surface an update when info.latest is strictly newer than info.version (and/or the app's version), otherwise return None.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

False positive. The server fills ServerInfo.latest only when the release is newer than the server itself: see info() in src/server.rs, u.latest.as_ref().filter(|r| is_newer(&r.tag_name, CURRENT)). A non-empty latest therefore already means the server is behind. I added a comment at that branch in 6de73c0 to make this clear.

Generated by Claude Code

mergeguards · 2026-10-26T03:38:28Z

+
+/// A value from a query string, URL-decoded.
+fn query_str(query: &str, k: &str) -> Option<String> {
+    query


🛡️ MergeGuard · 🟠 medium · bug

Query parameter decoding replaces '+' with a space before decoding, which can corrupt legitimate '+' characters in file and directory names for fs/file and fs/upload endpoints.

💡 Use strict percent-decoding for query parameters without converting '+' to space (e.g., percent_encoding::percent_decode_str), or apply '+' to space only for form-encoded fields not representing file paths.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

False positive. The app builds these URLs with urlencoding::encode, which sends a literal + as %2B, so it decodes back to +. Only an unencoded + becomes a space, which is the standard query-string meaning (browsers and HTML forms send spaces that way). File names containing + make the round trip intact.

Generated by Claude Code

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟠 35/100 — moderate
Findings: 🟠 3 medium

Recommended fixes

src/server.rs — Remove full-path details from upload error messages to prevent information disclosure. (confidence 86%)
src/server.rs — Stop converting '+' to spaces in query parameter decoding to preserve literal '+' characters in paths and names. (confidence 74%)

Security review — 🟠 3 medium (3 findings)

Dependency, filesystem & container image checks (OSV + Trivy + container scan) for this pull request.
All items below are also on Files changed as inline threads.
See Files changed for security threads on this diff.

__Open **Files changed** for 3 inline review threads on the diff.__

Reviewed by MergeGuard AI

mergeguards · 2026-10-26T03:40:46Z

+        }
+        let dir = Path::new(dir);
+        if !dir.is_dir() {
+            return error(400, &format!("{} is not a folder on the server", dir.display()));


🛡️ MergeGuard · 🟠 medium · security

Information disclosure: error responses from the upload endpoint include full filesystem paths (e.g., " is not a folder on the server"). Disclosing absolute paths can aid attackers in reconnaissance.

💡 Return generic error messages that do not include full server paths. Log the detailed path on the server side if needed.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Not changing this. The path in the message is the one the signed-in caller just sent, so echoing it back tells them nothing new. The message is shown to the station staff in the app, and naming the folder helps them.

Generated by Claude Code

mergeguards · 2026-10-26T03:40:46Z

+
+/// A value from a query string, URL-decoded.
+fn query_str(query: &str, k: &str) -> Option<String> {
+    query


🛡️ MergeGuard · 🟠 medium · security

Potentially lossy query decoding for '+' characters. query_str replaces '+' with space before URL-decoding, which will incorrectly turn literal '+' in file/folder names into spaces for non-app clients.

💡 Avoid converting '+' to space pre-decoding. Rely on URL decoding (%20 for space). If needed, handle form-encoded bodies separately.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

Same as the thread above. A literal + arrives as %2B from any client that encodes correctly, and + meaning a space is the standard query-string rule. No change.

Generated by Claude Code

mergeguards · 2026-10-26T03:40:46Z

+    /// A file's bytes, for previews, waveforms and copies. Others than administrators only get files
+    /// in the library folders.
+    fn send_file(&self, req: tiny_http::Request, me: &Operator, path: &str) {
+        let path = std::path::PathBuf::from(path);


🛡️ MergeGuard · 🟠 medium · security

Wide file read capability for admins via GET /api/v1/fs/file. If an admin token is compromised, arbitrary files on the server can be exfiltrated.

💡 Consider constraining file download scope (e.g., to data_dir and configured library folders) or gating broader access behind an explicit configuration flag. At minimum, document the risk and ensure short token TTLs and strong admin hygiene.

_{Reply with @mergeguards fix to apply an auto-fix on this line.}

By design. A server administrator already has full control of the machine through the server: they can install a new server program, set the library folders anywhere, and add playlist RUN commands. Limiting their downloads would add no protection. docs/SERVER.md already tells people to give admin only to trusted people and to use HTTPS and strong passwords.

Generated by Claude Code

Keeps this branch's server update check (selfupdate, ServerCfg.check_updates) as the one source of ServerInfo.latest and drops #19's second update thread; the release page now comes from the same release. AzuraCast calls from the portal need the AzuraCast permission. Playlist Insert/RemoveRows commands need edit_playlists. The generator's category folder picker (moved to genui.rs) goes through App::pick. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5uW8ddhUDidYMFx9SLghq

mergeguards

✅ MergeGuard — new commits are on this pull request.

Re-scan the latest diff: paste this line by itself (copy/paste is fine) — either on the Conversation tab (Add a comment) or on Files changed as a comment on the diff:

@mergeguard-followup

mergeguards

MergeGuard review
Risk: 🟢 0/100 — low risk
Findings: none

Reviewed by MergeGuard AI

claude added 2 commits October 25, 2026 22:33

Merge main (shortcuts and MIDI, EQ, CI checks) into the server branch

b1c198b

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

epell-development self-assigned this Oct 25, 2026

mergeguards Bot reviewed Oct 25, 2026

View reviewed changes

Cap a server update download at 200 MB and refuse bigger ones up front

bc94c39

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards Bot reviewed Oct 25, 2026

View reviewed changes

Merge main (0.2.9 app bug fixes, HTTPS docs) into the server branch

92301b1

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards Bot reviewed Oct 26, 2026

View reviewed changes

Keep the password hash out of an account's Debug output

6de73c0

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NM6xaQA85d9b4G9p1eugxt

mergeguards Bot reviewed Oct 26, 2026

View reviewed changes

epell-development merged commit 7f3c0ce into main Oct 26, 2026
5 checks passed

epell-development mentioned this pull request Oct 26, 2026

Fix the app hanging on Connect to Server #23

Merged

Conversation

epell-development commented Oct 25, 2026

What changes

Testing

CHANGELOG lines for v0.2.9

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Recommended fixes

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MergeGuard agent actions

Fix all findings

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Recommended fixes

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Recommended fixes

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mergeguards Bot left a comment

Choose a reason for hiding this comment

Recommended fixes

Uh oh!

mergeguards Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mergeguards Bot commented Oct 25, 2026 •

edited

Loading