fix: daemon socket security, config panics, dead code cleanup - #17
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens rocm-dash’s local daemon and OAuth token handling, removes dead-code scaffolding, and eliminates several panic paths (string slicing and duration parsing) introduced in the prior merge follow-up to #16.
Changes:
- Secures daemon and token persistence paths/permissions (socket moved under
~/.rocm/..., socket chmod to0600, ChatGPT OAuth token dir under~/.rocm/...with0700). - Replaces byte-slicing truncation in the TUI with a char-aware helper and adds regression tests.
- Removes crate-wide
allow(dead_code), deletes unused helpers/modules, and improves config duration validation/clamping.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rocm-dash-tui/src/ui/tabs/overview.rs | Switches to char-aware truncation and adds multibyte regression tests. |
| crates/rocm-dash-tui/src/ui/tabs/bench.rs | Removes a now-unused formatting helper surfaced by dead-code cleanup. |
| crates/rocm-dash-tui/src/ui/monitor.rs | Deletes an unused placeholder module. |
| crates/rocm-dash-tui/src/ui/mod.rs | Removes the monitor module export. |
| crates/rocm-dash-tui/src/llm.rs | Redacts api_key in Debug output via manual Debug impl. |
| crates/rocm-dash-tui/src/lib.rs | Removes crate-wide #![allow(dead_code)]. |
| crates/rocm-dash-tui/src/agent.rs | Sets OAuth token directory and attempts to constrain permissions. |
| crates/rocm-dash-daemon/tests/end_to_end.rs | Adds a runtime permissions regression test for the unix socket. |
| crates/rocm-dash-daemon/src/server.rs | Creates socket parent dir, binds unix socket, and sets mode 0600. |
| crates/rocm-dash-daemon/src/lib.rs | Removes crate-wide #![allow(dead_code)]. |
| crates/rocm-dash-daemon/Cargo.toml | Adds tempfile for new integration testing. |
| crates/rocm-dash-core/src/lib.rs | Removes crate-wide #![allow(dead_code)]. |
| crates/rocm-dash-core/src/config.rs | Rejects invalid/negative duration values and adds tests. |
| crates/rocm-dash-collectors/src/lib.rs | Removes crate-wide #![allow(dead_code)]. |
| crates/rocm-core/src/lib.rs | Moves default socket path under user home; clamps tick durations. |
| Cargo.lock | Records tempfile addition. |
| apps/rocm/src/dash.rs | Updates embedded-daemon spawn to match new server run signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rominf
force-pushed
the
fix/dash-review-issues
branch
from
June 17, 2026 15:28
c4793cf to
4961fdf
Compare
rominf
force-pushed
the
fix/dash-review-issues
branch
2 times, most recently
from
June 17, 2026 15:55
8d4e04e to
42f0c84
Compare
rominf
force-pushed
the
fix/dash-review-issues
branch
from
June 17, 2026 16:10
42f0c84 to
1ddd30c
Compare
rominf
force-pushed
the
fix/dash-review-issues
branch
from
June 17, 2026 16:11
1ddd30c to
d1ab378
Compare
| let model = "αβγδεζηθικλμνξοπρστυφ"; // 21 Greek letters | ||
| let result = trunc(model, 20); | ||
| assert_eq!(result.chars().count(), 20); | ||
| assert!(result.is_char_boundary(result.len())); |
juhovainio
approved these changes
Jun 18, 2026
- Remove crate-wide #![allow(dead_code)] from all four rocm-dash-* crates; the scaffold suppression is no longer warranted now that they are fully wired up. - Delete the unused fmt_opt_f64_4 function in bench.rs that the removed blanket allow was hiding. - Delete the empty monitor module (monitor.rs contained only a TODO with no implementation and was referenced nowhere in the codebase). Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Duration::from_secs_f64 panics on negative, NaN, infinite, or overflow input. The duration_secs serde helper now uses try_from_secs_f64 which covers all edge cases and returns a serde error on invalid config rather than panicking. Regression tests added for negative tick values. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The bench row renderer guarded string slices with .len() (byte count) then sliced by byte index. Any multibyte character straddling byte 20 (model name) or byte 10 (cell name) panicked with 'byte index is not a char boundary'. Replaced with trunc(), the existing char-aware helper already imported and used elsewhere in the file. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Security hardening across the daemon socket stack: Socket path: default moved from /tmp/rocmdashd.sock to a user-owned location using the first available of: $XDG_RUNTIME_DIR, $HOME, or a per-user subdirectory under temp_dir(). Empty env var values are treated as unset and fall through to the next tier. $XDG_RUNTIME_DIR is already mode 0700 on systemd systems; the others are directories we create and own, so set_permissions never fails with EPERM. The $USER/$LOGNAME value used in the temp-dir fallback is sanitized (non-alphanumeric chars replaced with '_') to prevent path traversal via a hostile env var. Socket permissions: the parent directory is created via DirBuilder::mode(0o700) and set_permissions(0o700) is called afterwards to tighten any pre-existing directory. The error message now includes ownership context so EPERM is immediately actionable. The socket file is set to 0o600 immediately after bind. The directory-hardening step is skipped when parent is an empty path to avoid accidentally chmod-ing CWD. Embedded daemon startup: replaced the fixed 200 ms sleep with a bounded poll loop (20 ms interval, 5 s deadline) that waits until UnixStream::connect succeeds, eliminating the startup race on slow hosts. API change: server::run no longer takes an unused _token parameter; FS permissions are the access control for unix sockets. Regression test: verifies the 0600 mode; the poll loop waits until mode == 0600 (not just file existence) to avoid a race with post-bind set_permissions. DashboardDaemonConfig tick accessors use try_from_secs_f64 instead of the manual is_finite/>=0 guard, catching overflow too. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
OAuth token directory: - Created with DirBuilder::mode(0o700) up-front so it is never world-accessible even briefly (vs. create_dir_all + set_permissions which left a TOCTOU window). - set_permissions(0o700) kept as defense-in-depth for pre-existing dirs. - Removed temp_dir() fallback: a predictable temp path could be pre-owned by another user. Fails with AgentError::Build if no non-empty $HOME or $USERPROFILE is found ($USERPROFILE is the Windows equivalent of $HOME). Empty values are filtered the same as unset. LlmConfig: derives Debug replaced with a manual impl that prints api_key as '[redacted]'. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
rominf
force-pushed
the
fix/dash-review-issues
branch
from
June 18, 2026 12:24
d1ab378 to
356c729
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & Why
Follow-up to #16. Addresses the blocking and non-blocking issues raised in the review.
Changes
Daemon socket security (blocking)
/tmp/rocmdashd.sockto~/.rocm/data/telemetry/rocmdashd.sockso other local users on a shared host cannot connect to it or pre-empt the predictable/tmppath.0600immediately after bind; the parent directory is created withcreate_dir_allif it doesn't exist._tokenparameter fromserver::run; filesystem permissions are the documented access control for unix sockets.Config panics (blocking)
duration_secs::deserializeinrocm-dash-corenow returns a serde error for negative, NaN, or infinite values instead of panicking.DashboardDaemonConfigtick accessors inrocm-coreclamp to safe defaults for invalidf64values.Dead code (blocking)
#![allow(dead_code)]from all fourrocm-dash-*crates; one genuinely unused function (fmt_opt_f64_4) surfaced and was deleted.monitormodule (monitor.rscontained only a TODO with no implementation and was never referenced).Byte-slice panic (blocking)
&model[..20]/&r.cell[..10]in the bench overview tab withtrunc(), the existing char-aware helper already used elsewhere in the file. The old guards checked.len()(bytes) and then sliced by byte index, which panics on any multibyte character straddling the boundary.Non-blocking
LlmConfignow implementsDebugmanually, printingapi_keyas"[redacted]".ChatGptAgentClientstores OAuth tokens under~/.rocm/data/chatgpt-tokens/at mode0700instead of the provider's default which inherits the process umask.Testing
cargo fmt --all --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace -- --test-threads=1— all pass0600verified at runtime), negative/invalid duration config values, multibyte string truncation