Added update command and version flag#13
Open
Roee-87 wants to merge 7 commits into
Open
Conversation
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.
Motivation
Minor version upgrade that:
updatecommand to automatically update the localaleo-devnodeto the latest version.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.lockpinned snarkvm at 4.6.3 whileCargo.tomldeclared 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 runscargo build --release --locked, which refuses to update the lockfile and would fail on every release build.Fix: Author updated
Cargo.tomland regeneratedCargo.lockdirectly.C-2 — Panic on
num_blocks = 0inPOST /block/createDescription: The handler accepted
num_blocks: 0via the request body. With zero blocks, the loopfor _ in 0..0never executes, leavinglast_blockasNone. The subsequentlast_block.unwrap()panics, crashing the blocking thread. Any unauthenticated caller could trigger this.Change —
src/rest/routes.rs:Motivation: The early return exits the async function before
spawn_blockingis ever called, makinglast_block.unwrap()unreachable for the zero case. A400 Bad Requestwith a clear message is returned to the caller instead.High
H-1 — Path Traversal via Snapshot Name
Description:
POST /snapshotaccepted a user-suppliednamefield and passed it directly tosnapshots_dir.join(&name)without sanitization.Path::joinresolves 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: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 a400.H-2 — Unauthenticated
/shutdownEndpointDescription:
POST /shutdowntriggered a process-level shutdown with no authentication or access control. When the devnode is bound to0.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:Motivation:
ConnectInfowas already available —spawn_serveralready usedinto_make_service_with_connect_info::<SocketAddr>()and the extractor is imported viasuper::*. Non-loopback callers receive403 Forbiddenimmediately. 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 —
updateCommand Does Not Verify Binary Signatures (Deferred)Description: The
updatecommand 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-apiis 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 theUpdatebuilder) is straightforward but meaningless without signed release artifacts.H-4 — Race Condition in Auto-Block Mode
Description: In auto-block mode,
transaction_broadcastused two separatespawn_blockingcalls with an.awaitsuspension point between them — one forprepare_advance_to_next_beacon_blockand one foradvance_to_next_block. Two concurrent broadcast requests could both callpreparewhile the ledger was still at height H, producing two blocks at the same height. When both then calledadvance, the second would fail because the ledger had already moved to H+1.Change —
src/rest/routes.rs: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
ledgerclone was also unnecessary and has been removed, simplifying the code.H-5 —
--private-keyExposed in Process ListDescription:
relaunch_as_start(invoked by therestorecommand) rebuilt the command line for the re-executed process and passed the private key as a--private-keyCLI argument. On any multi-user system, the full command line of every process is visible to all users viaps aux, exposing the private key.Change —
src/restore.rs:Motivation: Environment variables are not shown in
ps auxby default. Reading them requires explicit access to/proc/<pid>/environ(Linux), which is restricted to the process owner. Thestartcommand already readsPRIVATE_KEYfrom the environment as its primary input source, so no other changes are needed. Theprintln!that logs the restart command is also safe since the key is no longer inargs.Medium
M-1 — No Upper Bound on
num_blocksDescription:
POST /block/createacceptednum_blocksas an unboundedu32. 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:Motivation: 1000 blocks is a generous ceiling for development use. Consistent with how
GET /blockscaps its range at 50 blocks per request.M-2 —
advanceCLI Silently Swallows ErrorsDescription:
advance.rsassigned the HTTP response to_response(discarded) and always returnedOk(()). 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: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) usedlet _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: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_snapshotsReimplementssnapshots_sibling_dirDescription:
create_snapshotcorrectly called the sharedsnapshots_sibling_dir(&storage_path)function to locate the snapshots directory.list_snapshotsreimplemented the same path computation inline. If the directory layout ever changed, one would silently diverge from the other, causinglist_snapshotsto read from a different location thancreate_snapshotwrites to.Change —
src/rest/routes.rs: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_governorerror handler checkederror.to_string().contains("Too Many Requests")to decide whether to return429 Too Many Requestsor500 Internal Server Error. If the crate changed its error message wording in a future version, all rate-limited requests would silently return500.Change —
src/rest/mod.rs:Motivation: Enum variant matching is stable across message wording changes and is idiomatic Rust. The compiler will flag unhandled variants if
GovernorErrorgains new ones in a future version.M-6 — Unguarded
unwrap()on Runtime CreationDescription:
tokio::runtime::Runtime::new().unwrap()instart.rspanics 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:Motivation: Propagates the error to
mainvia?, which prints it and exits with code 1 with a human-readable message.M-7 — Unguarded
unwrap()onTEST_CONSENSUS_VERSION_HEIGHTSDescription:
TEST_CONSENSUS_VERSION_HEIGHTS.last().unwrap().1panics 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 namedlast_height, which sounds like the current ledger height rather than the target consensus version height.Change —
src/start.rs:Motivation: If the slice is empty,
target_heightbecomes 0,blocks_to_advancebecomes 0 viasaturating_sub, and the auto-advance step is skipped cleanly. The rename totarget_heightmakes 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:
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.rsL-2 — Dead
leo_devnode_tcpLog DirectivesDescription:
logger.rsadded tracing filter directives targetingleo_devnode_tcp— a module that does not exist anywhere in aleo-devnode. Bothleo_devnode_tcp=trace(for verbosity ≥ 3) andleo_devnode_tcp=off(for verbosity < 3) were pure no-ops, copy-pasted from the Leo CLI.Change —
src/logger.rs:Motivation: Removing dead code eliminates confusion about what verbosity levels above 2 were supposed to do. The
show_target = verbosity > 2logic is valid and was preserved.L-3 — Verbosity Accepted 0–255 Despite Docs Saying 0–2
Description: The
verbosityCLI flag was documented as accepting0-2in both help text and field comments, but the type wasu8with 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.rsandsrc/restore.rs:Motivation: Enforces at parse time what the documentation already promised. Values outside
0..=2now produce an immediate, clear error:L-5 — Repeated Fragile
"Missing"String MatchingDescription: Three endpoint handlers (
get_transaction,get_confirmed_transaction,get_unconfirmed_transaction) each contained an identical inline pattern to map snarkvm's genericanyhow::Errorto either404 Not Foundor500 Internal Server Errorby 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: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 ModuleDescription:
#![forbid(unsafe_code)]was declared insrc/rest/mod.rsonly, 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
unsafeanywhere in the codebase will get a compile error, not just in the REST module.Outstanding Items
updatecheck_transaction=falsestill enforces size limit