Open
Conversation
Add opt-in SSH access via a new [ssh] config section (disabled by default). When enabled, spacebot manages an sshd child process with key-only auth, host keys persisted on the data volume, and authorized keys set via API. - openssh-server added to Docker image - SshManager handles sshd lifecycle (start/stop/restart) - API endpoints: GET /ssh/status, PUT /ssh/authorized-key - SSH settings exposed in global settings API - Settings UI SSH tab with toggle, port config, live status
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SSH support end-to-end: config schema and defaults, an SshManager for sshd lifecycle, HTTP API endpoints for status and authorized-key management, settings persistence/start-stop logic, and a frontend Settings section plus client API for SSH controls. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
src/api/settings.rs
Outdated
| .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; | ||
|
|
||
| // Start or stop sshd based on the new config | ||
| if request.ssh_enabled.is_some() { |
Contributor
There was a problem hiding this comment.
Port-only updates won’t take effect right now: this block only runs when ssh_enabled is present, and even if you widen the condition, SshManager::start() early-returns when already running (so the old port keeps listening). Might be worth restarting sshd when ssh_port changes while enabled.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/settings.rs`:
- Around line 363-373: The code only triggers the ssh lifecycle apply when
request.ssh_enabled is present, so a port-only change (request.ssh_port) updates
doc["ssh"]["port"] but does not reload/rebind sshd; change the logic to run the
same ssh lifecycle/apply path whenever either request.ssh_enabled or
request.ssh_port is Some. Also stop swallowing apply errors: replace any "let _
= ..." or logging-only handling in the ssh apply block with proper error
propagation—return an Err or an HTTP error response when the sshd apply/restart
(the ssh lifecycle/apply function called after updating doc["ssh"]) fails, and
include the error details in the response/log so clients see the failure.
In `@src/api/ssh.rs`:
- Around line 75-89: The current SSH public key validation (around
request.public_key in src/api/ssh.rs, inside the handler that returns
SetAuthorizedKeyResponse) must enforce a single-key, single-line minimal
structure: after trimming, reject if the string contains any newline characters;
split the trimmed pubkey by ASCII spaces and require 2 or 3 fields (key-type,
base64 blob, optional comment); ensure the first field starts with one of the
allowed prefixes ("ssh-", "ecdsa-") and the second field is non-empty (base64
blob present); on failure return the same Ok(Json(SetAuthorizedKeyResponse {
success: false, message: "Invalid SSH public key format"... })) response. Update
the validation block that currently only checks starts_with to include these
checks before accepting and writing the key.
In `@src/main.rs`:
- Around line 743-751: The SshManager is being initialized and registered
(spacebot::ssh::SshManager::new and api_state.set_ssh_manager) after the HTTP
API listener is created, allowing /api/ssh/* requests to reach an uninitialized
manager; move the block that constructs SshManager and calls
api_state.set_ssh_manager(...) to execute before the API/server startup code so
the manager is registered prior to accepting requests, and then keep the
existing conditional start logic (if config.ssh.enabled { let mut ssh =
api_state.ssh_manager.lock().await; if let Err(error) =
ssh.start(config.ssh.port).await { ... } }) unchanged except for its new earlier
position.
In `@src/ssh.rs`:
- Line 43: Replace uses of host_key.to_str().unwrap() (and similar
to_str().unwrap() at the other two sites) with fallible conversion that returns
a structured error instead of panicking; validate UTF-8 using Path::to_str() and
if it returns None map that to a proper Error/Result (e.g., propagate a custom
error variant or anyhow::Error) in the surrounding function (the code that
constructs process args which uses host_key), and update the function signature
to return Result so callers can handle non-UTF8 paths rather than allowing
unwrap() to panic.
- Around line 68-87: The spawned sshd process sets
.stderr(std::process::Stdio::piped()) but never reads from the pipe, risking a
deadlock; modify the sshd spawn logic (the call that includes
.stderr(std::process::Stdio::piped()) in src/ssh.rs) to either (a) not pipe
stderr (use Stdio::null() or Stdio::inherit()) or (b) immediately take the
Child's stderr (child.stderr.take()) and spawn a dedicated thread/task that
continuously reads and drains it (forwarding to logging or discarding) so the
pipe never fills; implement the chosen approach where the Child is created and
ensure the drain thread uses non-blocking/looped reads until the child exits.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Dockerfileinterface/src/api/client.tsinterface/src/routes/Settings.tsxsrc/api.rssrc/api/server.rssrc/api/settings.rssrc/api/ssh.rssrc/api/state.rssrc/config.rssrc/lib.rssrc/main.rssrc/ssh.rs
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main.rs (1)
743-751:⚠️ Potential issue | 🟠 MajorInitialize
SshManagerbefore binding the HTTP API.At Line 642 the HTTP server can start accepting
/api/ssh/*requests before Line 743 registers the SSH manager, creating a startup race for early SSH API calls.🔧 Suggested fix (move SSH initialization above HTTP startup)
- let _http_handle = if config.api.enabled { + // Initialize SSH manager before exposing SSH API endpoints. + let ssh_manager = spacebot::ssh::SshManager::new(&config.instance_dir); + api_state.set_ssh_manager(ssh_manager).await; + if config.ssh.enabled { + let mut ssh = api_state.ssh_manager.lock().await; + if let Err(error) = ssh.start(config.ssh.port).await { + tracing::error!(%error, "failed to start sshd"); + } + } + + let _http_handle = if config.api.enabled { // IPv6 addresses need brackets when combined with port: [::]:19898 let raw_bind = config .api @@ - // Initialize SSH manager and start sshd if enabled - let ssh_manager = spacebot::ssh::SshManager::new(&config.instance_dir); - api_state.set_ssh_manager(ssh_manager).await; - if config.ssh.enabled { - let mut ssh = api_state.ssh_manager.lock().await; - if let Err(error) = ssh.start(config.ssh.port).await { - tracing::error!(%error, "failed to start sshd"); - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 743 - 751, The SshManager is registered with api_state too late, allowing the HTTP server to accept /api/ssh/* requests before api_state.set_ssh_manager(...) runs; move the block that creates SshManager (spacebot::ssh::SshManager::new), calls api_state.set_ssh_manager(...).await, and starts ssh (the ssh.start(...) call and its error handling) to execute before the HTTP server bind/startup so the SSH API always has a registered manager when the server begins accepting requests.src/api/ssh.rs (1)
89-95:⚠️ Potential issue | 🟠 MajorTighten SSH key validation beyond prefix checks.
This still accepts malformed values (e.g., missing key body) and can return success for unusable keys.
Proposed fix
- // Validate it looks like an SSH public key - if !pubkey.starts_with("ssh-") && !pubkey.starts_with("ecdsa-") { + // Validate minimal SSH public key structure: "<type> <base64> [comment]" + let parts: Vec<&str> = pubkey.split_whitespace().collect(); + let key_type = parts.first().copied().unwrap_or_default(); + let key_body = parts.get(1).copied().unwrap_or_default(); + let valid_type = key_type.starts_with("ssh-") + || key_type.starts_with("ecdsa-") + || key_type.starts_with("sk-ssh-") + || key_type.starts_with("sk-ecdsa-"); + let valid_len = parts.len() == 2 || parts.len() == 3; + if !valid_type || !valid_len || key_body.is_empty() { return Ok(Json(SetAuthorizedKeyResponse { success: false, message: "Invalid SSH public key format".to_string(), })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ssh.rs` around lines 89 - 95, The current prefix-only check for the pubkey variable is too permissive; tighten validation in the SetAuthorizedKey handler by trimming pubkey, splitting on whitespace and ensuring there are at least two fields (key-type and key-body), verify the key-type is one of allowed SSH types (e.g., starts_with "ssh-" or "ecdsa-"/explicit allowed list), attempt to base64-decode the second field and treat decode failures or empty decoded bytes as invalid, and return the same Json(SetAuthorizedKeyResponse { success: false, message: ... }) when validation fails; keep optional third field (comment) allowed but ignored for validation.src/ssh.rs (1)
43-43:⚠️ Potential issue | 🟠 MajorRemove
Path::to_str().unwrap()in SSH startup path.At Line 43, Line 70, and Line 80, non-UTF8 paths will panic the process. Convert paths fallibly and propagate a structured error instead.
Proposed fix
// Generate host key if missing let host_key = self.ssh_dir.join("ssh_host_ed25519_key"); + let host_key_str = host_key + .to_str() + .context("ssh host key path is not valid UTF-8")?; if !host_key.exists() { let output = Command::new("ssh-keygen") .args([ "-t", "ed25519", "-f", - host_key.to_str().unwrap(), + host_key_str, "-N", "", "-q", ]) @@ let authorized_keys = self.ssh_dir.join("authorized_keys"); + let authorized_keys_str = authorized_keys + .to_str() + .context("authorized_keys path is not valid UTF-8")?; let child = Command::new("/usr/sbin/sshd") .args([ "-D", // foreground "-e", // log to stderr "-h", - host_key.to_str().unwrap(), + host_key_str, @@ - &format!("AuthorizedKeysFile={}", authorized_keys.to_str().unwrap()), + &format!("AuthorizedKeysFile={authorized_keys_str}"), "-o", "ListenAddress=[::]", ])#!/bin/bash # Verify all panic-prone UTF-8 path conversions are removed from src/ssh.rs rg -n 'to_str\(\)\.unwrap\(\)' src/ssh.rsAlso applies to: 70-70, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ssh.rs` at line 43, Replace the panic-prone Path::to_str().unwrap() calls in src/ssh.rs (e.g., the host_key.to_str().unwrap() usage and the two other occurrences) with a fallible conversion that returns a structured error instead of panicking: call to_str().ok_or_else(|| SshError::InvalidPath(path.clone())) (or map to your crate's error type), propagate that Result via the ? operator, and update the enclosing functions' signatures (e.g., start_ssh / load_host_key or whatever function encloses host_key) to return Result<..., SshError> so the error can bubble up; do this for all three occurrences to remove any direct unwraps and ensure non-UTF8 paths produce an error rather than a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/ssh.rs`:
- Around line 100-103: The API currently returns raw internal errors in the JSON
response (see the SetAuthorizedKeyResponse construction returned via return
Ok(Json(...)) where message uses format!("Failed to write authorized key:
{error}")) — change the response to a generic client-facing message like "Failed
to write authorized key" and move the detailed error into the server log instead
(e.g., log::error!("Failed to write authorized key: {error:?}") or use the
existing logger), and apply the same change for the other similar return at
lines handling SetAuthorizedKeyResponse around 119-122 so clients never see
internal filesystem/runtime details.
In `@src/config.rs`:
- Around line 4154-4157: SshConfig is being created directly from TOML,
bypassing the required per-subsystem resolution order; add a resolve path for
SSH and call it from the config loader instead. Implement a SshConfig::resolve
(or impl Resolve for SshConfig) that accepts the TOML fragment plus DB and env
accessors and returns a fully resolved SshConfig following the order env > DB >
default, then replace the direct construction in the loader (the code creating
SshConfig { enabled: toml.ssh.enabled, port: toml.ssh.port }) with a call to
that resolve method so SSH settings are resolved via the same subsystem pattern
as other configs.
---
Duplicate comments:
In `@src/api/ssh.rs`:
- Around line 89-95: The current prefix-only check for the pubkey variable is
too permissive; tighten validation in the SetAuthorizedKey handler by trimming
pubkey, splitting on whitespace and ensuring there are at least two fields
(key-type and key-body), verify the key-type is one of allowed SSH types (e.g.,
starts_with "ssh-" or "ecdsa-"/explicit allowed list), attempt to base64-decode
the second field and treat decode failures or empty decoded bytes as invalid,
and return the same Json(SetAuthorizedKeyResponse { success: false, message: ...
}) when validation fails; keep optional third field (comment) allowed but
ignored for validation.
In `@src/main.rs`:
- Around line 743-751: The SshManager is registered with api_state too late,
allowing the HTTP server to accept /api/ssh/* requests before
api_state.set_ssh_manager(...) runs; move the block that creates SshManager
(spacebot::ssh::SshManager::new), calls api_state.set_ssh_manager(...).await,
and starts ssh (the ssh.start(...) call and its error handling) to execute
before the HTTP server bind/startup so the SSH API always has a registered
manager when the server begins accepting requests.
In `@src/ssh.rs`:
- Line 43: Replace the panic-prone Path::to_str().unwrap() calls in src/ssh.rs
(e.g., the host_key.to_str().unwrap() usage and the two other occurrences) with
a fallible conversion that returns a structured error instead of panicking: call
to_str().ok_or_else(|| SshError::InvalidPath(path.clone())) (or map to your
crate's error type), propagate that Result via the ? operator, and update the
enclosing functions' signatures (e.g., start_ssh / load_host_key or whatever
function encloses host_key) to return Result<..., SshError> so the error can
bubble up; do this for all three occurrences to remove any direct unwraps and
ensure non-UTF8 paths produce an error rather than a panic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
interface/src/api/client.tsinterface/src/routes/Settings.tsxsrc/api/ssh.rssrc/config.rssrc/main.rssrc/ssh.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/routes/Settings.tsx
src/config.rs
Outdated
Comment on lines
+4154
to
+4157
| let ssh = SshConfig { | ||
| enabled: toml.ssh.enabled, | ||
| port: toml.ssh.port, | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Route SSH config through a subsystem resolver instead of direct TOML mapping.
SshConfig is constructed directly from TOML here, which skips the required per-subsystem resolution pattern (env > DB > default). Please introduce a dedicated SSH resolve path and call it from config loading.
As per coding guidelines src/config.rs: "Use config resolution order: env > DB > default with per-subsystem resolve() methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` around lines 4154 - 4157, SshConfig is being created directly
from TOML, bypassing the required per-subsystem resolution order; add a resolve
path for SSH and call it from the config loader instead. Implement a
SshConfig::resolve (or impl Resolve for SshConfig) that accepts the TOML
fragment plus DB and env accessors and returns a fully resolved SshConfig
following the order env > DB > default, then replace the direct construction in
the loader (the code creating SshConfig { enabled: toml.ssh.enabled, port:
toml.ssh.port }) with a call to that resolve method so SSH settings are resolved
via the same subsystem pattern as other configs.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/config.rs (1)
5105-5108:⚠️ Potential issue | 🟠 MajorRoute SSH through subsystem resolution instead of direct TOML mapping.
At Line 5105,
SshConfigis constructed directly from TOML, which bypasses the required per-subsystem resolution order and prevents consistentenv > DB > defaultbehavior for SSH. Please resolve SSH via a dedicatedSshConfig::resolve()path and call it here.As per coding guidelines
src/config.rs: "Use config resolution order:env > DB > defaultwith per-subsystemresolve()methods".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 5105 - 5108, The current code directly constructs SshConfig from toml (let ssh = SshConfig { enabled: toml.ssh.enabled, port: toml.ssh.port }) which bypasses the required per-subsystem resolution order; replace this direct mapping with a call to SshConfig::resolve(...) so SSH values are determined using the standard env > DB > default resolution path (i.e., remove the direct struct literal and invoke SshConfig::resolve with the toml.ssh source and the existing config/DB/env context used by other resolve() calls).
🧹 Nitpick comments (1)
interface/src/routes/Settings.tsx (1)
1277-1287: Minor styling inconsistency with other sections.The message banner uses different Tailwind classes (
border-green-800 bg-green-950/50) compared to other settings sections which useborder-green-500/20 bg-green-500/10. Consider aligning the styling for visual consistency across all settings tabs.♻️ Proposed fix to align message styling
{message && ( <div className={`mt-4 rounded-md border px-3 py-2 text-sm ${ message.type === "success" - ? "border-green-800 bg-green-950/50 text-green-400" - : "border-red-800 bg-red-950/50 text-red-400" + ? "border-green-500/20 bg-green-500/10 text-green-400" + : "border-red-500/20 bg-red-500/10 text-red-400" }`} > {message.text} </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/Settings.tsx` around lines 1277 - 1287, The message banner in Settings.tsx (the JSX block that renders when the message variable is truthy, using message.type and message.text) uses inconsistent Tailwind classes; update the conditional class strings to match other settings sections by replacing the success classes `border-green-800 bg-green-950/50 text-green-400` with `border-green-500/20 bg-green-500/10 text-green-400` and similarly replace the error classes `border-red-800 bg-red-950/50 text-red-400` with `border-red-500/20 bg-red-500/10 text-red-400` so the banner matches the visual style used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/config.rs`:
- Around line 5105-5108: The current code directly constructs SshConfig from
toml (let ssh = SshConfig { enabled: toml.ssh.enabled, port: toml.ssh.port })
which bypasses the required per-subsystem resolution order; replace this direct
mapping with a call to SshConfig::resolve(...) so SSH values are determined
using the standard env > DB > default resolution path (i.e., remove the direct
struct literal and invoke SshConfig::resolve with the toml.ssh source and the
existing config/DB/env context used by other resolve() calls).
---
Nitpick comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 1277-1287: The message banner in Settings.tsx (the JSX block that
renders when the message variable is truthy, using message.type and
message.text) uses inconsistent Tailwind classes; update the conditional class
strings to match other settings sections by replacing the success classes
`border-green-800 bg-green-950/50 text-green-400` with `border-green-500/20
bg-green-500/10 text-green-400` and similarly replace the error classes
`border-red-800 bg-red-950/50 text-red-400` with `border-red-500/20
bg-red-500/10 text-red-400` so the banner matches the visual style used
elsewhere.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
interface/src/api/client.tsinterface/src/routes/Settings.tsxsrc/api/server.rssrc/config.rssrc/lib.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
# Conflicts: # Dockerfile # interface/src/routes/Settings.tsx # src/api/server.rs # src/api/settings.rs # src/api/state.rs # src/config.rs
- Harden SSH public key validation: enforce single-line, validate key structure (type + base64 body + optional comment), support sk-* types - Stop leaking internal errors in SSH API responses (log details server-side, return generic messages to clients) - Replace to_str().unwrap() with fallible conversion in SshManager to prevent panics on non-UTF8 paths - Move SshManager initialization before HTTP server startup to prevent race where /api/ssh/* requests hit an uninitialized manager - Fix port-only SSH settings updates being silently ignored: restart sshd when either enabled or port changes, propagate apply errors to the client instead of swallowing them - Align SSH settings banner styling with other settings sections
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main.rs (1)
1491-1499:⚠️ Potential issue | 🟠 MajorInstall the per-instance
SshManagerbefore opening/api.
start_http_server()has already run at Lines 1390-1408, so/api/ssh/*can race this block during startup and hit SSH state before it has been bound toconfig.instance_dir. Move the manager registration/startup block above the HTTP listener startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1491 - 1499, Move initialization and registration of the per-instance SshManager (SshManager::new(&config.instance_dir) and api_state.set_ssh_manager(...).await) and the conditional startup (ssh.start(config.ssh.port).await) so they execute before calling start_http_server(); this ensures api_state.ssh_manager is bound to config.instance_dir before the HTTP listener opens and prevents /api/ssh/* handlers from racing to access uninitialized SSH state. Keep the same logic (create SshManager, set via api_state.set_ssh_manager, then if config.ssh.enabled lock api_state.ssh_manager and call ssh.start) but place that block above the start_http_server() invocation.src/api/settings.rs (1)
363-374:⚠️ Potential issue | 🟠 MajorPort changes still won't apply to the running SSH daemon.
This only applies SSH lifecycle changes when
ssh_enabledis present, andSshManager::start()is a no-op once sshd is already running. A user can save a new port and still have sshd bound to the old one, and failedstart()/stop()calls still returnsuccess: true—which is especially risky when disabling SSH. Recompute/apply SSH state whenever either field changes, restart when an enabled server’s port changes, and surface apply failures to the client.Also applies to: 380-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/settings.rs` around lines 363 - 374, The current update only writes ssh_enabled/ssh_port to the toml but doesn't ensure the running daemon follows those changes; update the handler that processes request.ssh_enabled and request.ssh_port to (1) read the previous values from doc["ssh"]["enabled"] and doc["ssh"]["port"], (2) determine the desired state from request.* (if present) or previous values, (3) if desired.enabled differs then call SshManager::start() or SshManager::stop() accordingly, and (4) if desired.enabled is true and the port changed, perform a restart (stop then start) so sshd binds the new port; ensure SshManager::start()/stop() return results are checked and any failures are propagated back in the API response (do not always return success), referencing request.ssh_enabled, request.ssh_port, doc["ssh"]["enabled"], doc["ssh"]["port"], and SshManager::start/stop in your changes.
🧹 Nitpick comments (1)
src/lib.rs (1)
28-28: Keep the SSH module crate-private unless external consumers need it.This export turns the SSH process manager into part of the public crate API, even though the usage in this PR appears to be entirely internal.
♻️ Suggested change
- pub mod ssh; + pub(crate) mod ssh;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` at line 28, Change the public module export "pub mod ssh;" to a crate-private module by removing the pub (i.e., use "mod ssh;") so the SSH process manager is not part of the public API; update any internal references to the module (functions/types inside the ssh module) to continue to use ssh::... within the crate and remove any public re-exports that expose ssh types if present (or explicitly re-export only the specific items you intend to be public).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 1902-1906: The ssh-status query currently only reads { data:
sshStatus } from useQuery (queryKey: ["ssh-status"], queryFn: api.sshStatus) so
when it fails the card disappears; update the hook usage to also destructure
error and isLoading (e.g., { data: sshStatus, error: sshError, isLoading }) and
update the Settings status card render to explicitly show an error state/message
when sshError is present (and a loading state when isLoading) instead of
treating undefined data as “no status”; apply the same change to the other
useQuery instance that uses queryKey ["ssh-status"] at the second location.
In `@src/api/server.rs`:
- Around line 245-249: The set_authorized_key handler currently accepts
prefix-matching input and must instead fully validate the submitted public key
line before writing; update the set_authorized_key function in src/api/ssh.rs to
parse the line into algorithm, base64 blob, and optional comment, ensure the
algorithm is one of the allowed types (e.g., ssh-ed25519, ssh-rsa,
ecdsa-sha2-...), verify the base64 blob decodes successfully and matches
expected length/format for the algorithm, and return a 400 error (without
writing to disk) for any malformed key; only write the key and report success if
validation passes.
In `@src/config/load.rs`:
- Around line 2136-2139: In from_toml()/Config::validate_toml() reject an
ssh.port of 0 by validating toml.ssh.port before constructing SshConfig: if
toml.ssh.port == 0 return an Err (or propagate a ConfigValidation error)
indicating an invalid SSH port instead of building SshConfig with port 0; update
any call sites that expect a Result to propagate this validation failure so
invalid configs fail fast.
---
Duplicate comments:
In `@src/api/settings.rs`:
- Around line 363-374: The current update only writes ssh_enabled/ssh_port to
the toml but doesn't ensure the running daemon follows those changes; update the
handler that processes request.ssh_enabled and request.ssh_port to (1) read the
previous values from doc["ssh"]["enabled"] and doc["ssh"]["port"], (2) determine
the desired state from request.* (if present) or previous values, (3) if
desired.enabled differs then call SshManager::start() or SshManager::stop()
accordingly, and (4) if desired.enabled is true and the port changed, perform a
restart (stop then start) so sshd binds the new port; ensure
SshManager::start()/stop() return results are checked and any failures are
propagated back in the API response (do not always return success), referencing
request.ssh_enabled, request.ssh_port, doc["ssh"]["enabled"],
doc["ssh"]["port"], and SshManager::start/stop in your changes.
In `@src/main.rs`:
- Around line 1491-1499: Move initialization and registration of the
per-instance SshManager (SshManager::new(&config.instance_dir) and
api_state.set_ssh_manager(...).await) and the conditional startup
(ssh.start(config.ssh.port).await) so they execute before calling
start_http_server(); this ensures api_state.ssh_manager is bound to
config.instance_dir before the HTTP listener opens and prevents /api/ssh/*
handlers from racing to access uninitialized SSH state. Keep the same logic
(create SshManager, set via api_state.set_ssh_manager, then if
config.ssh.enabled lock api_state.ssh_manager and call ssh.start) but place that
block above the start_http_server() invocation.
---
Nitpick comments:
In `@src/lib.rs`:
- Line 28: Change the public module export "pub mod ssh;" to a crate-private
module by removing the pub (i.e., use "mod ssh;") so the SSH process manager is
not part of the public API; update any internal references to the module
(functions/types inside the ssh module) to continue to use ssh::... within the
crate and remove any public re-exports that expose ssh types if present (or
explicitly re-export only the specific items you intend to be public).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a11a4cdc-f425-4d61-a899-dfef466cd1b5
📒 Files selected for processing (11)
interface/src/api/client.tsinterface/src/routes/Settings.tsxsrc/api.rssrc/api/server.rssrc/api/settings.rssrc/api/state.rssrc/config/load.rssrc/config/toml_schema.rssrc/config/types.rssrc/lib.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/api/client.ts
Comment on lines
+1902
to
+1906
| const { data: sshStatus } = useQuery({ | ||
| queryKey: ["ssh-status"], | ||
| queryFn: api.sshStatus, | ||
| refetchInterval: 5_000, | ||
| }); |
Contributor
There was a problem hiding this comment.
Surface ssh-status fetch failures explicitly.
If this query fails, sshStatus stays undefined and the entire status card disappears. That makes “status request failed” look the same as “no status available,” which is confusing when someone is debugging why SSH will not start.
💡 Suggested fix
- const { data: sshStatus } = useQuery({
+ const { data: sshStatus, isError: sshStatusError, error } = useQuery({
queryKey: ["ssh-status"],
queryFn: api.sshStatus,
refetchInterval: 5_000,
});- {sshStatus && (
+ {sshStatusError ? (
+ <div className="rounded-lg border border-red-800 bg-red-950/50 p-4 text-sm text-red-400">
+ Failed to load SSH status: {error instanceof Error ? error.message : "Unknown error"}
+ </div>
+ ) : sshStatus ? (
<div className="rounded-lg border border-app-line bg-app-box p-4">
...
</div>
- )}
+ ) : null}Also applies to: 1960-1976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/routes/Settings.tsx` around lines 1902 - 1906, The ssh-status
query currently only reads { data: sshStatus } from useQuery (queryKey:
["ssh-status"], queryFn: api.sshStatus) so when it fails the card disappears;
update the hook usage to also destructure error and isLoading (e.g., { data:
sshStatus, error: sshError, isLoading }) and update the Settings status card
render to explicitly show an error state/message when sshError is present (and a
loading state when isLoading) instead of treating undefined data as “no status”;
apply the same change to the other useQuery instance that uses queryKey
["ssh-status"] at the second location.
Comment on lines
+245
to
+249
| .route("/ssh/status", get(ssh::ssh_status)) | ||
| .route( | ||
| "/ssh/authorized-key", | ||
| put(ssh::set_authorized_key).delete(ssh::clear_authorized_keys), | ||
| ) |
Contributor
There was a problem hiding this comment.
Reject malformed public keys before exposing this endpoint.
The handler behind this route (src/api/ssh.rs:71-110) only prefix-matches the submitted line before writing it. Inputs like ssh-ed25519 not-base64 will be accepted and reported as success, but sshd will reject every login while status still claims an authorized key is configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/server.rs` around lines 245 - 249, The set_authorized_key handler
currently accepts prefix-matching input and must instead fully validate the
submitted public key line before writing; update the set_authorized_key function
in src/api/ssh.rs to parse the line into algorithm, base64 blob, and optional
comment, ensure the algorithm is one of the allowed types (e.g., ssh-ed25519,
ssh-rsa, ecdsa-sha2-...), verify the base64 blob decodes successfully and
matches expected length/format for the algorithm, and return a 400 error
(without writing to disk) for any malformed key; only write the key and report
success if validation passes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/api/settings.rs (1)
363-373:⚠️ Potential issue | 🟠 MajorReject SSH port
0before persisting config.Direct API clients can bypass the UI validation and still send
ssh_port: 0. That writes an invalid SSH config and only fails later when sshd is started or restarted.Suggested fix
if request.ssh_enabled.is_some() || request.ssh_port.is_some() { + if matches!(request.ssh_port, Some(0)) { + return Ok(Json(GlobalSettingsUpdateResponse { + success: false, + message: "Invalid SSH port: 0".to_string(), + requires_restart: false, + })); + } + if doc.get("ssh").is_none() { doc["ssh"] = toml_edit::Item::Table(toml_edit::Table::new()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/settings.rs` around lines 363 - 373, Validate and reject a zero SSH port before writing to the config: when handling request.ssh_port (the code block that sets doc["ssh"]["port"]), check if the provided port == 0 and return an appropriate error response (or validation error) instead of persisting; do not write doc["ssh"]["port"] = toml_edit::value(...) for zero. Ensure the same validation path applies to direct API clients by performing this check in the settings update handler that processes request.ssh_port and request.ssh_enabled so invalid values never reach the persisted doc.interface/src/routes/Settings.tsx (1)
1902-1906:⚠️ Potential issue | 🟡 MinorSurface SSH status query failures explicitly.
When
api.sshStatusfails,sshStatusstaysundefinedand the whole status card disappears, so a broken backend looks the same as “no status yet.” Render explicit loading/error states here.Also applies to: 1960-1976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/Settings.tsx` around lines 1902 - 1906, The SSH status query currently only reads `data: sshStatus` from useQuery (queryKey ["ssh-status"], queryFn: api.sshStatus) so failures make the status card disappear; update the useQuery call to destructure isLoading, isError, and error (e.g., const { data: sshStatus, isLoading, isError, error } = useQuery(...)) and change the component rendering for the status card to explicitly show a loading state (spinner/placeholder) when isLoading, and an error state (visible message/icon using error.message) when isError, instead of hiding the card; apply the same change to the other useQuery instance referenced around lines 1960-1976.
🧹 Nitpick comments (1)
src/api/ssh.rs (1)
75-106: Preferpublic_keyoverpubkey.The request field is already named
public_key, so reusing that name locally is clearer and matches the repo naming rule.As per coding guidelines, "Don't abbreviate variable names. Use queue not q, message not msg. Common abbreviations like config are fine"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ssh.rs` around lines 75 - 106, The local variable named `pubkey` should be renamed to `public_key` to match the request field and coding guidelines; update all usages in this block (the trim, newline checks, is_empty check, splitting into `parts`, `key_type`, `key_body`, `valid_type`, `valid_field_count`, and the call to `manager.set_authorized_key(...)`) to use `public_key` instead of `pubkey`, keeping the same logic and preserving the `SetAuthorizedKeyResponse` error returns and the `set_authorized_key` call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/settings.rs`:
- Around line 397-405: Collapse the nested if by combining the running check and
the awaited stop call into a single conditional: call ssh_manager.is_running()
and, if true, await ssh_manager.stop().await in the same if expression and match
its Err branch; on Err, log with tracing::error!(%error, "failed to stop sshd
before restart") and return the same Json(GlobalSettingsUpdateResponse {
success: false, message: "Failed to apply SSH settings".to_string(),
requires_restart: false }). Ensure the code still references
ssh_manager.is_running(), ssh_manager.stop().await, tracing::error!, and
GlobalSettingsUpdateResponse so behavior is unchanged.
In `@src/api/ssh.rs`:
- Around line 90-98: The SSH public key validation currently enforces
parts.len() == 2 || parts.len() == 3 which rejects keys with multi-word
comments; update the logic around parts, key_type, key_body, and
valid_field_count so that you require at least two fields (parts.len() >= 2),
keep key_type as parts.first(), set key_body from parts.get(1) and ensure it is
non-empty, and treat any additional parts (index >=2) as the optional comment
rather than invalidating the key; adjust the conditional that uses
valid_field_count to accept parts.len() >= 2 instead of == 2 || == 3.
In `@src/ssh.rs`:
- Around line 161-167: The current has_authorized_key() only checks file size,
which treats newline- or comment-only files as valid; update has_authorized_key
to open the authorized_keys file (self.ssh_dir.join("authorized_keys")) and scan
its lines, returning true only if at least one line remains after trimming
whitespace and skipping comment lines (lines starting with '#')—alternatively
enforce the same non-blank/non-comment validation in set_authorized_key() so
blank or comment-only writes are rejected; reference the has_authorized_key and
set_authorized_key symbols when making the change.
---
Duplicate comments:
In `@interface/src/routes/Settings.tsx`:
- Around line 1902-1906: The SSH status query currently only reads `data:
sshStatus` from useQuery (queryKey ["ssh-status"], queryFn: api.sshStatus) so
failures make the status card disappear; update the useQuery call to destructure
isLoading, isError, and error (e.g., const { data: sshStatus, isLoading,
isError, error } = useQuery(...)) and change the component rendering for the
status card to explicitly show a loading state (spinner/placeholder) when
isLoading, and an error state (visible message/icon using error.message) when
isError, instead of hiding the card; apply the same change to the other useQuery
instance referenced around lines 1960-1976.
In `@src/api/settings.rs`:
- Around line 363-373: Validate and reject a zero SSH port before writing to the
config: when handling request.ssh_port (the code block that sets
doc["ssh"]["port"]), check if the provided port == 0 and return an appropriate
error response (or validation error) instead of persisting; do not write
doc["ssh"]["port"] = toml_edit::value(...) for zero. Ensure the same validation
path applies to direct API clients by performing this check in the settings
update handler that processes request.ssh_port and request.ssh_enabled so
invalid values never reach the persisted doc.
---
Nitpick comments:
In `@src/api/ssh.rs`:
- Around line 75-106: The local variable named `pubkey` should be renamed to
`public_key` to match the request field and coding guidelines; update all usages
in this block (the trim, newline checks, is_empty check, splitting into `parts`,
`key_type`, `key_body`, `valid_type`, `valid_field_count`, and the call to
`manager.set_authorized_key(...)`) to use `public_key` instead of `pubkey`,
keeping the same logic and preserving the `SetAuthorizedKeyResponse` error
returns and the `set_authorized_key` call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bed1ea11-7d42-40d8-9ced-1626cc6883ba
📒 Files selected for processing (5)
interface/src/routes/Settings.tsxsrc/api/settings.rssrc/api/ssh.rssrc/main.rssrc/ssh.rs
Comment on lines
+397
to
+405
| if ssh_manager.is_running() { | ||
| if let Err(error) = ssh_manager.stop().await { | ||
| tracing::error!(%error, "failed to stop sshd before restart"); | ||
| return Ok(Json(GlobalSettingsUpdateResponse { | ||
| success: false, | ||
| message: "Failed to apply SSH settings".to_string(), | ||
| requires_restart: false, | ||
| })); | ||
| } |
Contributor
There was a problem hiding this comment.
Collapse this branch to clear the current Clippy failure.
CI is already flagging this nested if; folding it into one condition should unblock the lint job without changing behavior.
Suggested fix
- if ssh_manager.is_running() {
- if let Err(error) = ssh_manager.stop().await {
- tracing::error!(%error, "failed to stop sshd before restart");
- return Ok(Json(GlobalSettingsUpdateResponse {
- success: false,
- message: "Failed to apply SSH settings".to_string(),
- requires_restart: false,
- }));
- }
- }
+ if ssh_manager.is_running()
+ && let Err(error) = ssh_manager.stop().await
+ {
+ tracing::error!(%error, "failed to stop sshd before restart");
+ return Ok(Json(GlobalSettingsUpdateResponse {
+ success: false,
+ message: "Failed to apply SSH settings".to_string(),
+ requires_restart: false,
+ }));
+ }📝 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.
Suggested change
| if ssh_manager.is_running() { | |
| if let Err(error) = ssh_manager.stop().await { | |
| tracing::error!(%error, "failed to stop sshd before restart"); | |
| return Ok(Json(GlobalSettingsUpdateResponse { | |
| success: false, | |
| message: "Failed to apply SSH settings".to_string(), | |
| requires_restart: false, | |
| })); | |
| } | |
| if ssh_manager.is_running() | |
| && let Err(error) = ssh_manager.stop().await | |
| { | |
| tracing::error!(%error, "failed to stop sshd before restart"); | |
| return Ok(Json(GlobalSettingsUpdateResponse { | |
| success: false, | |
| message: "Failed to apply SSH settings".to_string(), | |
| requires_restart: false, | |
| })); | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 397-399: Clippy: this if statement can be collapsed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/settings.rs` around lines 397 - 405, Collapse the nested if by
combining the running check and the awaited stop call into a single conditional:
call ssh_manager.is_running() and, if true, await ssh_manager.stop().await in
the same if expression and match its Err branch; on Err, log with
tracing::error!(%error, "failed to stop sshd before restart") and return the
same Json(GlobalSettingsUpdateResponse { success: false, message: "Failed to
apply SSH settings".to_string(), requires_restart: false }). Ensure the code
still references ssh_manager.is_running(), ssh_manager.stop().await,
tracing::error!, and GlobalSettingsUpdateResponse so behavior is unchanged.
- Accept SSH keys with multi-word comments (parts.len() >= 2 instead of == 2 || == 3) - Reject ssh.port = 0 in both the settings API handler and at config load time - Harden has_authorized_key() to scan for real key lines instead of just checking file size (skip blank lines and comments) - Rename pubkey variable to public_key per coding guidelines
load_from_path returns Result<Config, error::Error>, not anyhow::Result.
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
1 participant
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.
Summary
Adds opt-in SSH server support so hosted users can SSH into their instances. The feature is disabled by default and toggled via the settings UI or
[ssh]config section. Uses key-only authentication with authorized keys managed via API. Host keys persist on the data volume across restarts.New SSH settings tab includes an enable toggle, port configuration, and live status indicator showing whether the server is running and if authorized keys are configured.
Implementation
New SSH module manages sshd lifecycle with host key generation and persistence. Settings UI includes enable toggle, port configuration, and live status indicator with authorized key warnings. API endpoints for SSH status (
GET /ssh/status) and key management (PUT/DELETE /ssh/authorized-key). Configuration stored in TOML and synced to sshd on updates. Docker image includes openssh-server dependency. Process auto-starts/stops based on enabled flag at startup and on config changes.Note
Written by Tembo for commit 6161a5a.