Skip to content

Added update command and version flag#13

Open
Roee-87 wants to merge 7 commits into
mainfrom
feat/version-and-help-flags
Open

Added update command and version flag#13
Roee-87 wants to merge 7 commits into
mainfrom
feat/version-and-help-flags

Conversation

@Roee-87
Copy link
Copy Markdown
Collaborator

@Roee-87 Roee-87 commented May 20, 2026

Motivation

Minor version upgrade that:

  • adds a version flag
  • includes a checksum for releases
  • includes an update command to automatically update the local aleo-devnode to the latest version.
  • updates snarkVM to v4.6.4.

aleo-devnode Security & Code Quality Audit

Summary

A full audit of all source files was conducted. Findings are organized by severity. For each finding, the description, the change made, and the motivation are documented. Items marked Deferred or Fixed by author were not implemented in this session.


Critical

C-1 — Stale Cargo.lock (Fixed by author)

Description: Cargo.lock pinned snarkvm at 4.6.3 while Cargo.toml declared a requirement of 4.6.4, and still recorded the package version as 0.1.1 after it had been bumped to 0.1.2. The release workflow runs cargo build --release --locked, which refuses to update the lockfile and would fail on every release build.

Fix: Author updated Cargo.toml and regenerated Cargo.lock directly.


C-2 — Panic on num_blocks = 0 in POST /block/create

Description: The handler accepted num_blocks: 0 via the request body. With zero blocks, the loop for _ in 0..0 never executes, leaving last_block as None. The subsequent last_block.unwrap() panics, crashing the blocking thread. Any unauthenticated caller could trigger this.

Change — src/rest/routes.rs:

let num_blocks = req.num_blocks.unwrap_or(1);
if num_blocks == 0 {
    return Err(RestError::bad_request(anyhow!("num_blocks must be at least 1")));
}

Motivation: The early return exits the async function before spawn_blocking is ever called, making last_block.unwrap() unreachable for the zero case. A 400 Bad Request with a clear message is returned to the caller instead.


High

H-1 — Path Traversal via Snapshot Name

Description: POST /snapshot accepted a user-supplied name field and passed it directly to snapshots_dir.join(&name) without sanitization. Path::join resolves traversal components literally, so a caller supplying "name": "../../etc/cron.d/evil" could cause ledger files to be written outside the intended snapshots directory.

Change — src/rest/routes.rs:

let name = req.get("name").and_then(|v| v.as_str()).map(|s| s.to_string());
if let Some(ref n) = name {
    if n.contains('/') || n.contains('\\') || n.contains("..") {
        return Err(RestError::bad_request(anyhow!(
            "Invalid snapshot name: must not contain path separators or '..'"
        )));
    }
}
let name = name.unwrap_or_else(|| format!("snapshot-{height}"));

Motivation: User input is validated at the boundary before it reaches the filesystem. The check is explicit and conservative — any name containing a path separator or .. is rejected with a 400.


H-2 — Unauthenticated /shutdown Endpoint

Description: POST /shutdown triggered a process-level shutdown with no authentication or access control. When the devnode is bound to 0.0.0.0 (common in Docker or CI environments), any host on the network can kill the process. The CORS policy allows any origin.

Change — src/rest/routes.rs:

pub(crate) async fn shutdown(
    ConnectInfo(addr): ConnectInfo<SocketAddr>,
    State(rest): State<Self>,
) -> StatusCode {
    if !addr.ip().is_loopback() {
        return StatusCode::FORBIDDEN;
    }
    ...
}

Motivation: ConnectInfo was already available — spawn_server already used into_make_service_with_connect_info::<SocketAddr>() and the extractor is imported via super::*. Non-loopback callers receive 403 Forbidden immediately. This is proportionate for a dev tool: it does not require API keys, but it enforces that the shutdown signal must originate from the local machine.


H-3 — update Command Does Not Verify Binary Signatures (Deferred)

Description: The update command downloads and self-replaces the binary from GitHub over HTTPS without cryptographic verification of the artifact. A compromised release or MITM on the download URL would silently replace the running binary. zipsign-api is already a transitive dependency and supports this.

Status: Deferred. Implementing this requires generating an Ed25519 keypair, storing the private key as a GitHub Actions secret, adding a signing step to the release workflow for each platform zip, and embedding the corresponding public key bytes in the binary. The code change (calling .verifying_keys(&[pub_key]) on the Update builder) is straightforward but meaningless without signed release artifacts.


H-4 — Race Condition in Auto-Block Mode

Description: In auto-block mode, transaction_broadcast used two separate spawn_blocking calls with an .await suspension point between them — one for prepare_advance_to_next_beacon_block and one for advance_to_next_block. Two concurrent broadcast requests could both call prepare while the ledger was still at height H, producing two blocks at the same height. When both then called advance, the second would fail because the ledger had already moved to H+1.

Change — src/rest/routes.rs:

tokio::task::spawn_blocking(move || {
    let new_block = rest.ledger
        .prepare_advance_to_next_beacon_block(
            &rest.private_key, vec![], vec![], vec![tx],
            &mut rand::thread_rng(),
        )
        .map_err(|e| anyhow!("{e}"))?;
    rest.ledger.advance_to_next_block(&new_block).map_err(|e| anyhow!("{e}"))
})
.await
.map_err(|e| RestError::internal_server_error(anyhow!("Task panicked: {}", e)))??;

Motivation: Combining both operations into a single blocking task eliminates the async suspension point between them. Once the blocking thread starts, no other broadcast can be interleaved between prepare and advance. The intermediate ledger clone was also unnecessary and has been removed, simplifying the code.


H-5 — --private-key Exposed in Process List

Description: relaunch_as_start (invoked by the restore command) rebuilt the command line for the re-executed process and passed the private key as a --private-key CLI argument. On any multi-user system, the full command line of every process is visible to all users via ps aux, exposing the private key.

Change — src/restore.rs:

// Removed from args:
// args.push("--private-key".to_string());
// args.push(pk.to_string());

// Added after building the command:
if let Some(pk) = private_key {
    cmd.env("PRIVATE_KEY", pk);
}

Motivation: Environment variables are not shown in ps aux by default. Reading them requires explicit access to /proc/<pid>/environ (Linux), which is restricted to the process owner. The start command already reads PRIVATE_KEY from the environment as its primary input source, so no other changes are needed. The println! that logs the restart command is also safe since the key is no longer in args.


Medium

M-1 — No Upper Bound on num_blocks

Description: POST /block/create accepted num_blocks as an unbounded u32. A caller could send {"num_blocks": 4294967295} and occupy a blocking thread indefinitely, consuming CPU and disk I/O until the process is killed or storage is exhausted.

Change — src/rest/routes.rs:

const MAX_BLOCKS_PER_REQUEST: u32 = 1000;
if num_blocks > MAX_BLOCKS_PER_REQUEST {
    return Err(RestError::bad_request(
        anyhow!("num_blocks exceeds maximum ({MAX_BLOCKS_PER_REQUEST})")
    ));
}

Motivation: 1000 blocks is a generous ceiling for development use. Consistent with how GET /blocks caps its range at 50 blocks per request.


M-2 — advance CLI Silently Swallows Errors

Description: advance.rs assigned the HTTP response to _response (discarded) and always returned Ok(()). If the devnode was unreachable, returned a non-2xx status, or the request was malformed, the user received no feedback and the process exited with code 0.

Change — src/advance.rs:

let response = client
    .post(format!("http://{}/testnet/block/create", self.socket_addr))
    .header("Content-Type", "application/json")
    .json(&payload)
    .send()
    .map_err(|e| anyhow!("Failed to reach devnode at {}: {e}", self.socket_addr))?;

if !response.status().is_success() {
    let status = response.status();
    let body = response.text().unwrap_or_default();
    return Err(anyhow!("Advance failed ({status}): {body}"));
}

Motivation: Errors now propagate to main, which prints them to stderr and exits with code 1, giving users and scripts accurate feedback.


M-3 — Startup Auto-Advance Silently Swallows Errors

Description: The startup auto-advance to the latest consensus version (in start.rs) used let _response = client.post(...).send().await, discarding both network errors and non-2xx HTTP responses. If the advance failed due to a timing issue or ledger error, startup continued silently with the ledger at the wrong consensus height.

Change — src/start.rs:

match client.post(...).send().await {
    Ok(r) if !r.status().is_success() =>
        tracing::warn!("Auto-advance to consensus version failed with status {}", r.status()),
    Err(e) =>
        tracing::warn!("Auto-advance to consensus version failed: {e}"),
    Ok(_) => {}
}

Motivation: Unlike the CLI case, a startup auto-advance failure is not necessarily fatal — the node can still operate at an older consensus height. A warn! log is the right severity: visible to the operator without killing the process.


M-4 — list_snapshots Reimplements snapshots_sibling_dir

Description: create_snapshot correctly called the shared snapshots_sibling_dir(&storage_path) function to locate the snapshots directory. list_snapshots reimplemented the same path computation inline. If the directory layout ever changed, one would silently diverge from the other, causing list_snapshots to read from a different location than create_snapshot writes to.

Change — src/rest/routes.rs:

// Before:
let snapshots_dir = {
    let mut p = storage_path.clone();
    let dir_name = format!("{}-snapshots", p.file_name()...);
    p.pop();
    p.join(dir_name)
};

// After:
let snapshots_dir = snapshots_sibling_dir(&storage_path);

Motivation: A single implementation means a single place to update. Eliminates the divergence risk entirely.


M-5 — Governor Rate-Limit Handler Uses Fragile String Matching

Description: The tower_governor error handler checked error.to_string().contains("Too Many Requests") to decide whether to return 429 Too Many Requests or 500 Internal Server Error. If the crate changed its error message wording in a future version, all rate-limited requests would silently return 500.

Change — src/rest/mod.rs:

let status = match &error {
    tower_governor::errors::GovernorError::TooManyRequests { .. } =>
        StatusCode::TOO_MANY_REQUESTS,
    _ => StatusCode::INTERNAL_SERVER_ERROR,
};
let mut response = axum::response::Response::new(error.to_string().into());
*response.status_mut() = status;
response

Motivation: Enum variant matching is stable across message wording changes and is idiomatic Rust. The compiler will flag unhandled variants if GovernorError gains new ones in a future version.


M-6 — Unguarded unwrap() on Runtime Creation

Description: tokio::runtime::Runtime::new().unwrap() in start.rs panics on failure. While runtime creation rarely fails, this is inconsistent with the surrounding -> Result<()> return type and produces a panic with no context rather than a clean error message.

Change — src/start.rs:

let rt = tokio::runtime::Runtime::new()
    .map_err(|e| anyhow::anyhow!("Failed to create async runtime: {e}"))?;

Motivation: Propagates the error to main via ?, which prints it and exits with code 1 with a human-readable message.


M-7 — Unguarded unwrap() on TEST_CONSENSUS_VERSION_HEIGHTS

Description: TEST_CONSENSUS_VERSION_HEIGHTS.last().unwrap().1 panics at startup if the slice is empty. While this cannot happen with current snarkvm builds, it would be an unrecoverable crash at startup with no useful error message. The variable was also misleadingly named last_height, which sounds like the current ledger height rather than the target consensus version height.

Change — src/start.rs:

let target_height = TEST_CONSENSUS_VERSION_HEIGHTS
    .last()
    .map(|&(_, h)| h)
    .unwrap_or(0);
let blocks_to_advance = target_height.saturating_sub(current_height);

Motivation: If the slice is empty, target_height becomes 0, blocks_to_advance becomes 0 via saturating_sub, and the auto-advance step is skipped cleanly. The rename to target_height makes it clear this is the height the ledger needs to advance to, not the height it is currently at.


Low

L-1 — Wrong Copyright Headers in Nine Source Files

Description: Nine source files carried the full Leo library GPL header ("This file is part of the Leo library...") rather than the aleo-devnode header. This was a copy-paste artifact from the Leo CLI codebase.

Change: Replaced the 15-line Leo GPL boilerplate in all nine affected files with the correct 4-line header:

// Copyright (C) 2019-2026 Provable Inc.
// This file is part of the aleo-devnode tool.
//
// Licensed under the GNU General Public License v3.0.

Files: logger.rs, start.rs, accounts.rs, advance.rs, rest/mod.rs, rest/routes.rs, rest/helpers/mod.rs, rest/helpers/error.rs, rest/helpers/path.rs


L-2 — Dead leo_devnode_tcp Log Directives

Description: logger.rs added tracing filter directives targeting leo_devnode_tcp — a module that does not exist anywhere in aleo-devnode. Both leo_devnode_tcp=trace (for verbosity ≥ 3) and leo_devnode_tcp=off (for verbosity < 3) were pure no-ops, copy-pasted from the Leo CLI.

Change — src/logger.rs:

// Before:
let filter = EnvFilter::from_str(default_log_str).unwrap();
let filter = if verbosity >= 3 {
    filter.add_directive("leo_devnode_tcp=trace".parse().unwrap())
} else {
    filter.add_directive("leo_devnode_tcp=off".parse().unwrap())
};
Ok(filter)

// After:
Ok(EnvFilter::from_str(default_log_str).unwrap())

Motivation: Removing dead code eliminates confusion about what verbosity levels above 2 were supposed to do. The show_target = verbosity > 2 logic is valid and was preserved.


L-3 — Verbosity Accepted 0–255 Despite Docs Saying 0–2

Description: The verbosity CLI flag was documented as accepting 0-2 in both help text and field comments, but the type was u8 with no range restriction. Values 3–255 were accepted silently and behaved identically to 2 (all values ≥ 2 produce trace-level logging with no distinction).

Change — src/start.rs and src/restore.rs:

#[clap(
    ...,
    value_parser = clap::value_parser!(u8).range(0..=2)
)]
pub(crate) verbosity: u8,

Motivation: Enforces at parse time what the documentation already promised. Values outside 0..=2 now produce an immediate, clear error:

error: invalid value '3' for '--verbosity <VERBOSITY>': 3 is not in 0..=2

L-5 — Repeated Fragile "Missing" String Matching

Description: Three endpoint handlers (get_transaction, get_confirmed_transaction, get_unconfirmed_transaction) each contained an identical inline pattern to map snarkvm's generic anyhow::Error to either 404 Not Found or 500 Internal Server Error by checking whether the error message contained the string "Missing". The pattern was duplicated three times, meaning three places to update if snarkvm's error wording ever changes.

Change — src/rest/routes.rs:

/// Maps a snarkvm ledger error to a REST error, returning 404 for missing items.
/// snarkvm returns `anyhow::Error` without typed variants, so the message text is the only signal.
fn ledger_err(err: anyhow::Error) -> RestError {
    if err.to_string().contains("Missing") {
        RestError::not_found(err)
    } else {
        RestError::from(err)
    }
}

All three call sites now use ledger_err(err).

Motivation: One implementation, one place to update. The doc comment explains why string matching is necessary — it is a limitation of snarkvm's untyped error API, not a design choice.


Informational

I-3 — #![forbid(unsafe_code)] Only in REST Module

Description: #![forbid(unsafe_code)] was declared in src/rest/mod.rs only, covering only that module. The rest of the codebase — main.rs, start.rs, restore.rs, advance.rs, accounts.rs, logger.rs — was unprotected.

Change — src/main.rs:

#![forbid(unsafe_code)]

Motivation: A crate-root attribute applies to the entire binary. Any future contributor attempting to introduce unsafe anywhere in the codebase will get a compile error, not just in the REST module.


Outstanding Items

Finding Description Status
H-3 Binary signature verification on update Deferred — requires signed release infrastructure
L-4 check_transaction=false still enforces size limit No change — behavior is correct; a clarifying comment could be added

@Roee-87 Roee-87 self-assigned this May 20, 2026
@Roee-87 Roee-87 added the enhancement New feature or request label May 20, 2026
@Roee-87 Roee-87 changed the title Feat/version and help flags Added update command and version flag May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant