diff options
| author | nsfisis <nsfisis@gmail.com> | 2026-07-20 08:21:48 +0900 |
|---|---|---|
| committer | nsfisis <nsfisis@gmail.com> | 2026-07-20 08:24:45 +0900 |
| commit | 85fb9b70f4f41ead644b24f98052e6ffd9699e48 (patch) | |
| tree | ede4348cf68b94af923c1c8c0dd668c7f0aac043 | |
| parent | 087865ef50283350a3cc74a5363ba44fa7dbe5c5 (diff) | |
| download | php-shirabe-85fb9b70f4f41ead644b24f98052e6ffd9699e48.tar.gz php-shirabe-85fb9b70f4f41ead644b24f98052e6ffd9699e48.tar.zst php-shirabe-85fb9b70f4f41ead644b24f98052e6ffd9699e48.zip | |
fix(diagnose-command): stop holding a Config borrow across http calls
execute() held &config.borrow() across check_http/check_composer_repo/
check_composer_audit, which reach HttpDownloader -> CurlDownloader::
download; that method does self.config.borrow_mut() on the same
Config RefCell, panicking with "RefCell already borrowed" once the
phpinfo panic that previously masked this was fixed.
Switch those three helpers to take the Rc<RefCell<Config>> handle
(matching check_version's existing pattern) and borrow only where a
field is actually read, so no borrow spans the downstream network
call. Un-ignore the now-passing run_diagnose smoke test and
test_cmd_fail; test_cmd_success stays ignored, now for two separate
reasons: it needs real network access (as the PHP original does), and
shirabe_php_shim::OPENSSL_VERSION_NUMBER is a hardcoded stub (0) that
always trips check_platform's TLSv1.1/1.2 support check regardless of
the real linked OpenSSL, forcing a non-zero exit code.
| -rw-r--r-- | crates/shirabe/src/command/diagnose_command.rs | 36 | ||||
| -rw-r--r-- | crates/shirabe/src/lib.rs | 4 | ||||
| -rw-r--r-- | crates/shirabe/tests/command/diagnose_command_test.rs | 15 |
3 files changed, 30 insertions, 25 deletions
diff --git a/crates/shirabe/src/command/diagnose_command.rs b/crates/shirabe/src/command/diagnose_command.rs index 13fe8294..ac15e1d1 100644 --- a/crates/shirabe/src/command/diagnose_command.rs +++ b/crates/shirabe/src/command/diagnose_command.rs @@ -168,7 +168,7 @@ impl Command for DiagnoseCommand { )); io.write_no_newline("Checking Composer and its dependencies for vulnerabilities: "); - let r = self.check_composer_audit(&config.borrow())?; + let r = self.check_composer_audit(&config)?; self.output_result(r); let platform_overrides = config @@ -292,14 +292,15 @@ impl Command for DiagnoseCommand { self.output_result(PhpMixed::String(r)); io.write_no_newline("Checking http connectivity to packagist: "); - let r = self.check_http("http", &config.borrow())?; + let r = self.check_http("http", &config)?; self.output_result(r); io.write_no_newline("Checking https connectivity to packagist: "); - let r = self.check_http("https", &config.borrow())?; + let r = self.check_http("https", &config)?; self.output_result(r); - for repo in config.borrow().get_repositories() { + let repositories = config.borrow().get_repositories(); + for repo in repositories { let repo_arr = repo.1.as_array().cloned().unwrap_or_default(); if repo_arr.get("type").and_then(|v| v.as_string()) == Some("composer") && repo_arr.get("url").is_some() @@ -333,7 +334,7 @@ impl Command for DiagnoseCommand { .and_then(|v| v.as_string()) .unwrap_or("") )); - let r = self.check_composer_repo(&url, &config.borrow())?; + let r = self.check_composer_repo(&url, &config)?; self.output_result(r); } } @@ -545,7 +546,11 @@ impl DiagnoseCommand { format!("<info>OK</> <comment>git version {}</>", git_version) } - fn check_http(&self, proto: &str, config: &Config) -> anyhow::Result<PhpMixed> { + fn check_http( + &self, + proto: &str, + config: &std::rc::Rc<std::cell::RefCell<Config>>, + ) -> anyhow::Result<PhpMixed> { let result = self.check_connectivity_and_composer_network_http_enablement(); if result.as_bool() != Some(true) { return Ok(result); @@ -553,7 +558,7 @@ impl DiagnoseCommand { let mut result_list: Vec<PhpMixed> = vec![]; let mut tls_warning: Option<String> = None; - if proto == "https" && config.get("disable-tls").as_bool() == Some(true) { + if proto == "https" && config.borrow().get("disable-tls").as_bool() == Some(true) { tls_warning = Some("<warning>Composer is configured to disable SSL/TLS protection. This will leave remote HTTPS requests vulnerable to Man-In-The-Middle attacks.</warning>".to_string()); } @@ -599,7 +604,11 @@ impl DiagnoseCommand { Ok(PhpMixed::Bool(true)) } - fn check_composer_repo(&self, url: &str, config: &Config) -> anyhow::Result<PhpMixed> { + fn check_composer_repo( + &self, + url: &str, + config: &std::rc::Rc<std::cell::RefCell<Config>>, + ) -> anyhow::Result<PhpMixed> { let result = self.check_connectivity_and_composer_network_http_enablement(); if result.as_bool() != Some(true) { return Ok(result); @@ -607,7 +616,9 @@ impl DiagnoseCommand { let mut result_list: Vec<PhpMixed> = vec![]; let mut tls_warning: Option<String> = None; - if str_starts_with(url, "https://") && config.get("disable-tls").as_bool() == Some(true) { + if str_starts_with(url, "https://") + && config.borrow().get("disable-tls").as_bool() == Some(true) + { tls_warning = Some("<warning>Composer is configured to disable SSL/TLS protection. This will leave remote HTTPS requests vulnerable to Man-In-The-Middle attacks.</warning>".to_string()); } @@ -941,7 +952,10 @@ impl DiagnoseCommand { Ok(PhpMixed::Bool(true)) } - fn check_composer_audit(&self, config: &Config) -> anyhow::Result<PhpMixed> { + fn check_composer_audit( + &self, + config: &std::rc::Rc<std::cell::RefCell<Config>>, + ) -> anyhow::Result<PhpMixed> { let result = self.check_connectivity_and_composer_network_http_enablement(); if result.as_bool() != Some(true) { return Ok(result); @@ -989,7 +1003,7 @@ impl DiagnoseCommand { crate::repository::RepositoryInterfaceHandle::new(ComposerRepository::new( repo_config, std::rc::Rc::new(std::cell::RefCell::new(NullIO::new())), - config, + &config.borrow(), self.http_downloader.borrow().clone().unwrap(), None, )?); diff --git a/crates/shirabe/src/lib.rs b/crates/shirabe/src/lib.rs index 4d17f6c6..4667e7c9 100644 --- a/crates/shirabe/src/lib.rs +++ b/crates/shirabe/src/lib.rs @@ -182,10 +182,6 @@ mod cli_tests { run_config => "config", run_create_project => "create-project", run_depends => "depends", - #[ignore = "DiagnoseCommand::check_http passes &config.borrow() into a call chain that \ - reaches CurlDownloader::download, which then does self.config.borrow_mut() on \ - the same Config RefCell, panicking with 'RefCell already borrowed'; same root \ - cause as tests/command/diagnose_command_test.rs"] run_diagnose => "diagnose", run_dump_autoload => "dump-autoload", run_exec => "exec", diff --git a/crates/shirabe/tests/command/diagnose_command_test.rs b/crates/shirabe/tests/command/diagnose_command_test.rs index 521cea84..f1bb2ef5 100644 --- a/crates/shirabe/tests/command/diagnose_command_test.rs +++ b/crates/shirabe/tests/command/diagnose_command_test.rs @@ -7,11 +7,6 @@ use shirabe_php_shim::PhpMixed; #[test] #[serial] -#[ignore = "DiagnoseCommand::check_http passes &config.borrow() into a call chain that reaches \ - CurlDownloader::download, which then does self.config.borrow_mut() on the same \ - Config RefCell, panicking with 'RefCell already borrowed'; beyond that, diagnose checks \ - live http/https connectivity to packagist and the github.com rate limit, so the test \ - also requires real network access (as the PHP original does)"] fn test_cmd_fail() { let tear_down = init_temp_composer( Some(&serde_json::json!({ "name": "foo/bar", "description": "test pkg" })), @@ -51,11 +46,11 @@ Checking github.com rate limit: " #[test] #[serial] -#[ignore = "DiagnoseCommand::check_http passes &config.borrow() into a call chain that reaches \ - CurlDownloader::download, which then does self.config.borrow_mut() on the same \ - Config RefCell, panicking with 'RefCell already borrowed'; beyond that, diagnose checks \ - live http/https connectivity to packagist and the github.com rate limit, so the test \ - also requires real network access (as the PHP original does)"] +#[ignore = "shirabe_php_shim::OPENSSL_VERSION_NUMBER is a hardcoded stub (0), which always trips \ + check_platform's `< 0x1000100f` TLSv1.1/1.2 support check regardless of the real \ + linked OpenSSL, forcing a non-zero exit code; diagnose also checks live http/https \ + connectivity to packagist and the github.com rate limit (as the PHP original does), \ + so the test additionally requires real network access"] fn test_cmd_success() { let tear_down = init_temp_composer( Some(&serde_json::json!({ |
