Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions crates/cli/src/scaffold/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::{
env,
fs::{self, File},
process::{Command, Stdio},
};

use dialoguer::{Confirm, Input, MultiSelect, console::Style, theme::ColorfulTheme};
Expand Down Expand Up @@ -134,6 +135,58 @@ impl ProgramMetadata {
}
}

const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-skill";

/// Pinned: an unversioned `npx skills` runs whatever the registry calls latest
/// at the moment of someone's first scaffold, which is not a thing this can
/// promise anyone. An exact version is the only form that resolves the same way
/// twice; a range still floats to the newest release inside it.
const DEV_SKILL_INSTALLER: &str = "skills@1.5.22";
Comment on lines 137 to +144

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Skill source remains unpinned

DEV_SKILL_INSTALLER is pinned for repeatability, but DEV_SKILL_REPO still resolves the repository's mutable default branch and selects every skill. Identical Surfpool versions and inputs can therefore install different files over time, undermining reproducibility and making audits or rollbacks harder.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


/// Builds the skill install as issue #567 specifies it, pinned, and rooted at
/// the project being scaffolded rather than wherever the process happens to sit.
fn dev_skill_install_command(base_location: &FileLocation) -> Command {
let mut command = Command::new("npx");
command.args([
"-y",
DEV_SKILL_INSTALLER,
"add",
DEV_SKILL_REPO,
"--skill",
"*",
"-y",
]);
command.current_dir(base_location.expect_path_buf());
command
}

/// Starts the skill install and returns, whatever becomes of it.
///
/// This runs on the way to booting someone's surfnet, so it gets no say in
/// that: never waited on, output never reaching the terminal, and nothing at
/// all on a machine without Node — the usual case for a Rust user.
fn spawn_dev_skill_install(mut command: Command) {
let Ok(mut child) = command
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
else {
return;
};
// Reaped off-thread: the child leaves no defunct entry, the scaffold no wait.
let _ = hiro_system_kit::thread_named("Dev Skill Install").spawn(move || {
let _ = child.wait();
});
}

/// The install that follows the confirmation, or nothing. A declined
/// confirmation cancels the deployment, and the install is part of what it
/// cancels.
fn install_after_confirmation(confirmation: bool, base_location: &FileLocation) -> Option<Command> {
confirmation.then(|| dev_skill_install_command(base_location))
}

pub fn scaffold_in_memory_iac(
framework: &Framework,
programs: &[ProgramMetadata],
Expand Down Expand Up @@ -512,5 +565,104 @@ pub fn scaffold_iac_layout(
println!("Deployment canceled");
}

// Last, so the install only follows a scaffold that finished.
if let Some(command) = install_after_confirmation(confirmation, base_location) {
spawn_dev_skill_install(command);
Comment on lines +569 to +570

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Runbook consent triggers package install

When a fresh scaffold uses --yes or accepts the default deployment prompt, the same confirmation silently runs npx skills@1.5.22 add ... and mutates the project even though neither path specifically authorizes remote package execution. How this was verified: The runbook confirmation value flows directly into install_after_confirmation, which spawns the installer with all output suppressed.

}

Ok(())
}

#[cfg(test)]
mod tests {
use std::{
path::Path,
process::Command,
time::{Duration, Instant},
};

use super::{
DEV_SKILL_INSTALLER, DEV_SKILL_REPO, FileLocation, dev_skill_install_command,
install_after_confirmation, spawn_dev_skill_install,
};

#[cfg(unix)]
fn sh(script: &str) -> Command {
let mut command = Command::new("sh");
command.args(["-c", script]);
command
}

/// #567 supplied this invocation literally: the `-y` flags keep it off a
/// prompt and `--skill "*"` is what makes it the whole bundle. The one
/// departure from its text is the pinned version, which is here in full so
/// that dropping the pin has to fail this.
#[test]
fn the_install_is_the_command_issue_567_asked_for() {
let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap();
let command = dev_skill_install_command(&base);
assert_eq!(command.get_program(), "npx");
assert_eq!(
command.get_args().collect::<Vec<_>>(),
[
"-y",
DEV_SKILL_INSTALLER,
"add",
DEV_SKILL_REPO,
"--skill",
"*",
"-y"
]
);
let (package, version) = DEV_SKILL_INSTALLER
.split_once('@')
.expect("installer is pinned");
assert_eq!(package, "skills");
assert!(
version.split('.').count() == 3
&& version
.split('.')
.all(|p| !p.is_empty() && p.bytes().all(|b| b.is_ascii_digit())),
"the installer must stay pinned to an exact version, not a range: {version}"
);
}

/// The install writes into the project being scaffolded, which is the
/// manifest's directory rather than the shell's. `surfpool start -m
/// ../elsewhere/txtx.yml` scaffolds a tree the caller is not standing in.
#[test]
fn the_install_runs_in_the_scaffolded_project_not_the_process_cwd() {
let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap();
assert_eq!(
dev_skill_install_command(&base).get_current_dir(),
Some(Path::new("/tmp/surfpool-567-scaffold"))
);
}

/// Declining the confirmation cancels the deployment, and the install is
/// part of what it cancels. Nothing is built, so nothing is spawned.
#[test]
fn a_declined_confirmation_starts_no_install() {
let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap();
assert!(install_after_confirmation(false, &base).is_none());
assert!(install_after_confirmation(true, &base).is_some());
}

/// The three ways this goes wrong on a real machine: no Node at all, an
/// install that fails, an install that hangs. Each returns at once and
/// returns nothing, so the scaffold has neither a value to branch on nor a
/// wait to be held by — its result and its output are the same either way.
#[cfg(unix)]
#[test]
fn no_outcome_of_the_install_reaches_the_scaffold() {
let started = Instant::now();
spawn_dev_skill_install(Command::new("surfpool-567-no-such-binary"));
spawn_dev_skill_install(sh("exit 1"));
spawn_dev_skill_install(sh("sleep 30"));
assert!(
started.elapsed() < Duration::from_secs(5),
"the scaffold waited on the install: {:?}",
started.elapsed()
);
}
}
Loading