Skip to content

Commit e6130ed

Browse files
committed
fix: address remaining CodeRabbit findings on PR #6
- network: add shared http_client() helper with bounded connect_timeout + total timeout; replace bare Client::new() - network: stream tarball into a tempfile and extract from there so peak memory no longer scales with archive size - update: use saturating_sub for the 24h throttle so a future guard timestamp (clock skew, restored snapshot, manual edit) cannot underflow and disable the check - ls_remote: make the version-prefix filter segment-aware so "8.2" no longer matches "8.20.x" - install: do not wipe a pre-existing version dir if a follow-up package install fails; only the cli package is offered for immediate "use now" - shell: shell-escape values in set_env_var/path via single-quote quoting so embedded "$, backticks, quotes in PVM_DIR cannot break eval; add coverage for hostile inputs - docs: GEMINI.md uses canonical macOS casing
1 parent 19efe4c commit e6130ed

7 files changed

Lines changed: 117 additions & 35 deletions

File tree

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ semver = "1.0.28"
2626
serde = { version = "1.0.228", features = ["derive"] }
2727
serde_json = "1.0.149"
2828
tar = "0.4.45"
29+
tempfile = "3.27.0"
2930
tokio = { version = "1.52.1", features = ["full"] }
3031

3132
[dev-dependencies]

GEMINI.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ When running `git commit -m "..."` in the shell (like Zsh/Bash), backticks are i
2828

2929
### static-php-cli Integration
3030
- **Endpoint:** `https://dl.static-php.dev/static-php-cli/bulk/`
31-
- **Supported OS:** `linux`, `macos`.
31+
- **Supported OS:** `linux`, `macOS`.
3232
- **Supported Arch:** `x86_64`, `aarch64`.
3333
- **Packages:** `cli`, `fpm`, `micro`.
3434
- **Format:** `tar.gz` only.

src/commands/install.rs

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ pub async fn execute_install(version: &str) -> Result<()> {
5959
.collect();
6060

6161
let dest = versions_dir.join(&resolved_version);
62+
let dest_existed = dest.exists();
6263
std::fs::create_dir_all(&dest)?;
6364

6465
for package in &selected_packages {
@@ -69,8 +70,11 @@ pub async fn execute_install(version: &str) -> Result<()> {
6970
package
7071
);
7172
if let Err(e) = network::download_and_extract(&resolved_version, package, &dest).await {
72-
// Atomic install: any package failure removes the entire version dir
73-
std::fs::remove_dir_all(&dest).ok();
73+
// Only wipe the dest if it didn't exist before this install attempt;
74+
// a pre-existing install must not be destroyed by a follow-up failure.
75+
if !dest_existed {
76+
std::fs::remove_dir_all(&dest).ok();
77+
}
7478
anyhow::bail!(
7579
"Failed to install PHP {} (package {}): {}",
7680
resolved_version,
@@ -88,17 +92,19 @@ pub async fn execute_install(version: &str) -> Result<()> {
8892
resolved_version
8993
);
9094

91-
// Ask user if they want to use it right away
92-
let use_now = dialoguer::Confirm::with_theme(&theme)
93-
.with_prompt(
94-
format!("Do you want to use PHP {} now?", resolved_version)
95-
.bold()
96-
.to_string(),
97-
)
98-
.default(true)
99-
.interact_opt()
100-
.unwrap_or(Some(false))
101-
.unwrap_or(false);
95+
// Only the cli package places a `php` binary on PATH; without it, switching is meaningless.
96+
let cli_selected = selected_packages.iter().any(|p| p == "cli");
97+
let use_now = cli_selected
98+
&& dialoguer::Confirm::with_theme(&theme)
99+
.with_prompt(
100+
format!("Do you want to use PHP {} now?", resolved_version)
101+
.bold()
102+
.to_string(),
103+
)
104+
.default(true)
105+
.interact_opt()
106+
.unwrap_or(Some(false))
107+
.unwrap_or(false);
102108

103109
if use_now {
104110
let v = crate::fs::resolve_local_version(&resolved_version)?;
@@ -124,6 +130,11 @@ pub async fn execute_install(version: &str) -> Result<()> {
124130
}
125131
}
126132
println!("{} Switched to PHP {}", "✓".green(), v.bold());
133+
} else if !cli_selected {
134+
println!(
135+
"{} The 'cli' package was not selected; this version cannot be activated via PATH.",
136+
"💡".yellow()
137+
);
127138
} else {
128139
println!(
129140
"{} To use this version later, run `{}`",

src/commands/ls_remote.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ impl LsRemote {
1616
let mut versions_info = network::get_available_versions().await?;
1717

1818
if let Some(prefix) = &self.version_prefix {
19-
versions_info.retain(|(v, _)| v.starts_with(prefix) || v == prefix);
19+
versions_info.retain(|(v, _)| v == prefix || v.starts_with(&format!("{}.", prefix)));
2020
}
2121

2222
if versions_info.is_empty() {

src/network.rs

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,20 @@ use tar::Archive;
1111
use colored::Colorize;
1212
use indicatif::{ProgressBar, ProgressStyle};
1313
use std::fs::File;
14-
use std::io::{Read, Write};
14+
use std::io::{Read, Seek, SeekFrom, Write};
1515
use std::time::Duration;
1616

1717
const CACHE_DURATION: Duration = Duration::from_secs(24 * 60 * 60); // 24 hours
18+
const HTTP_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
19+
const HTTP_REQUEST_TIMEOUT: Duration = Duration::from_secs(300);
20+
21+
fn http_client() -> Result<Client> {
22+
Client::builder()
23+
.connect_timeout(HTTP_CONNECT_TIMEOUT)
24+
.timeout(HTTP_REQUEST_TIMEOUT)
25+
.build()
26+
.context("Failed to initialize HTTP client")
27+
}
1828

1929
#[derive(Deserialize)]
2030
struct RemoteFile {
@@ -72,7 +82,7 @@ pub async fn get_available_versions() -> Result<Vec<(String, Vec<String>)>> {
7282
spinner.set_message("Fetching...");
7383
spinner.enable_steady_tick(Duration::from_millis(100));
7484

75-
let client = Client::new();
85+
let client = http_client()?;
7686
let json_url = format!("{}?format=json", BASE_URL);
7787
let res = client
7888
.get(json_url)
@@ -194,7 +204,7 @@ pub async fn download_and_extract(
194204
"{}php-{}-{}-{}.tar.gz",
195205
BASE_URL, resolved_version, package, target
196206
);
197-
let client = Client::new();
207+
let client = http_client()?;
198208
let response = client
199209
.get(&url)
200210
.send()
@@ -222,19 +232,24 @@ pub async fn download_and_extract(
222232
pb
223233
};
224234

235+
// Stream the archive into a temp file to avoid materializing the whole tarball in memory.
236+
let mut tmp = tempfile::tempfile().context("Failed to create temporary archive file")?;
237+
let mut downloaded: u64 = 0;
225238
let mut stream = response.bytes_stream();
226-
let mut buffer = Vec::new();
227-
228239
while let Some(item) = stream.next().await {
229240
let chunk = item.context("Error while downloading chunk")?;
230-
buffer.extend_from_slice(&chunk);
231-
pb.set_position(buffer.len() as u64);
241+
tmp.write_all(&chunk)
242+
.context("Failed to write archive chunk to temp file")?;
243+
downloaded += chunk.len() as u64;
244+
pb.set_position(downloaded);
232245
}
246+
tmp.flush().context("Failed to flush temp archive file")?;
247+
tmp.seek(SeekFrom::Start(0))
248+
.context("Failed to rewind temp archive file")?;
233249

234250
pb.finish_with_message(format!("Downloaded package {}", package));
235251

236-
let cursor = std::io::Cursor::new(buffer);
237-
let tar = GzDecoder::new(cursor);
252+
let tar = GzDecoder::new(tmp);
238253
let mut archive = Archive::new(tar);
239254

240255
let bin_dir = dest.join("bin");

src/shell.rs

Lines changed: 65 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,49 @@ pub trait Shell {
88
fn wrapper_fn(&self) -> String;
99
}
1010

11+
/// Quote a string for POSIX shells (bash/zsh) by wrapping it in single quotes
12+
/// and escaping any embedded single quotes via the `'\''` idiom.
13+
fn posix_single_quote(value: &str) -> String {
14+
let mut out = String::with_capacity(value.len() + 2);
15+
out.push('\'');
16+
for c in value.chars() {
17+
if c == '\'' {
18+
out.push_str("'\\''");
19+
} else {
20+
out.push(c);
21+
}
22+
}
23+
out.push('\'');
24+
out
25+
}
26+
27+
/// Quote a string for fish by wrapping it in single quotes and escaping `\` and `'`.
28+
fn fish_single_quote(value: &str) -> String {
29+
let mut out = String::with_capacity(value.len() + 2);
30+
out.push('\'');
31+
for c in value.chars() {
32+
match c {
33+
'\\' => out.push_str("\\\\"),
34+
'\'' => out.push_str("\\'"),
35+
other => out.push(other),
36+
}
37+
}
38+
out.push('\'');
39+
out
40+
}
41+
1142
pub struct Bash;
1243

1344
impl Shell for Bash {
1445
fn path(&self, path: &Path) -> String {
15-
format!("export PATH=\"{}:$PATH\"", path.display())
46+
format!(
47+
"export PATH={}:\"$PATH\"",
48+
posix_single_quote(&path.display().to_string())
49+
)
1650
}
1751

1852
fn set_env_var(&self, name: &str, value: &str) -> String {
19-
format!("export {}=\"{}\"", name, value)
53+
format!("export {}={}", name, posix_single_quote(value))
2054
}
2155

2256
fn use_on_cd(&self) -> String {
@@ -70,11 +104,14 @@ pub struct Zsh;
70104

71105
impl Shell for Zsh {
72106
fn path(&self, path: &Path) -> String {
73-
format!("export PATH=\"{}:$PATH\"", path.display())
107+
format!(
108+
"export PATH={}:\"$PATH\"",
109+
posix_single_quote(&path.display().to_string())
110+
)
74111
}
75112

76113
fn set_env_var(&self, name: &str, value: &str) -> String {
77-
format!("export {}=\"{}\"", name, value)
114+
format!("export {}={}", name, posix_single_quote(value))
78115
}
79116

80117
fn use_on_cd(&self) -> String {
@@ -125,11 +162,14 @@ pub struct Fish;
125162

126163
impl Shell for Fish {
127164
fn path(&self, path: &Path) -> String {
128-
format!("set -gx PATH \"{}\" $PATH", path.display())
165+
format!(
166+
"set -gx PATH {} $PATH",
167+
fish_single_quote(&path.display().to_string())
168+
)
129169
}
130170

131171
fn set_env_var(&self, name: &str, value: &str) -> String {
132-
format!("set -gx {} \"{}\"", name, value)
172+
format!("set -gx {} {}", name, fish_single_quote(value))
133173
}
134174

135175
fn use_on_cd(&self) -> String {
@@ -197,7 +237,7 @@ mod tests {
197237
let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin");
198238
assert_eq!(
199239
bash.path(path),
200-
"export PATH=\"/home/user/.local/share/pvm/versions/8.3.1/bin:$PATH\""
240+
"export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin':\"$PATH\""
201241
);
202242
}
203243

@@ -206,7 +246,16 @@ mod tests {
206246
let bash = Bash;
207247
assert_eq!(
208248
bash.set_env_var("PVM_MULTISHELL_PATH", "/some/path"),
209-
"export PVM_MULTISHELL_PATH=\"/some/path\""
249+
"export PVM_MULTISHELL_PATH='/some/path'"
250+
);
251+
}
252+
253+
#[test]
254+
fn test_bash_set_env_escapes_special_chars() {
255+
let bash = Bash;
256+
assert_eq!(
257+
bash.set_env_var("X", "evil$(whoami)`id`\"$PATH\"'quote"),
258+
"export X='evil$(whoami)`id`\"$PATH\"'\\''quote'"
210259
);
211260
}
212261

@@ -216,7 +265,7 @@ mod tests {
216265
let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin");
217266
assert_eq!(
218267
zsh.path(path),
219-
"export PATH=\"/home/user/.local/share/pvm/versions/8.3.1/bin:$PATH\""
268+
"export PATH='/home/user/.local/share/pvm/versions/8.3.1/bin':\"$PATH\""
220269
);
221270
}
222271

@@ -226,7 +275,13 @@ mod tests {
226275
let path = std::path::Path::new("/home/user/.local/share/pvm/versions/8.3.1/bin");
227276
assert_eq!(
228277
fish.path(path),
229-
"set -gx PATH \"/home/user/.local/share/pvm/versions/8.3.1/bin\" $PATH"
278+
"set -gx PATH '/home/user/.local/share/pvm/versions/8.3.1/bin' $PATH"
230279
);
231280
}
281+
282+
#[test]
283+
fn test_fish_set_env_escapes_special_chars() {
284+
let fish = Fish;
285+
assert_eq!(fish.set_env_var("X", "a'b\\c"), "set -gx X 'a\\'b\\\\c'");
286+
}
232287
}

src/update.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ pub async fn check_for_updates(target_version: &str) -> Result<Option<String>> {
3030
let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs();
3131
if !contents.is_empty()
3232
&& let Ok(last_check) = contents.trim().parse::<u64>()
33-
&& now - last_check < 86400
33+
&& now.saturating_sub(last_check) < 86400
3434
{
3535
file.unlock().ok();
3636
return Ok(None);

0 commit comments

Comments
 (0)