From 54af47e286d0fb601e2e60aeb19002f6b7937574 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Sun, 7 Jun 2026 11:20:23 +0900 Subject: feat(shirabe): resolve advisory instanceof TODOs via AnySecurityAdvisory Add an Ignored variant to the advisory enum (renamed to AnySecurityAdvisory) so the PHP three-class hierarchy PartialSecurityAdvisory -> SecurityAdvisory -> IgnoredSecurityAdvisory maps one-to-one onto enum variants. Replace the hard-coded auditor downcasts with as_security_advisory() (PHP `instanceof SecurityAdvisory`, true for both full and ignored) and as_ignored(), implementing severity/cve/source ignore filtering, the toIgnoredAdvisory conversion, and the table/plain row output faithfully. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../shirabe/src/advisory/any_security_advisory.rs | 52 ++++++ crates/shirabe/src/advisory/auditor.rs | 199 ++++++++++----------- .../src/advisory/ignored_security_advisory.rs | 6 +- crates/shirabe/src/advisory/mod.rs | 4 +- .../advisory/partial_or_full_security_advisory.rs | 25 --- .../src/advisory/partial_security_advisory.rs | 8 +- crates/shirabe/src/advisory/security_advisory.rs | 4 + 7 files changed, 161 insertions(+), 137 deletions(-) create mode 100644 crates/shirabe/src/advisory/any_security_advisory.rs delete mode 100644 crates/shirabe/src/advisory/partial_or_full_security_advisory.rs (limited to 'crates/shirabe/src/advisory') diff --git a/crates/shirabe/src/advisory/any_security_advisory.rs b/crates/shirabe/src/advisory/any_security_advisory.rs new file mode 100644 index 0000000..c48df33 --- /dev/null +++ b/crates/shirabe/src/advisory/any_security_advisory.rs @@ -0,0 +1,52 @@ +use crate::advisory::IgnoredSecurityAdvisory; +use crate::advisory::PartialSecurityAdvisory; +use crate::advisory::SecurityAdvisory; +use shirabe_semver::constraint::AnyConstraint; + +#[derive(Debug, Clone)] +pub enum AnySecurityAdvisory { + Partial(PartialSecurityAdvisory), + Full(SecurityAdvisory), + Ignored(IgnoredSecurityAdvisory), +} + +impl AnySecurityAdvisory { + pub fn advisory_id(&self) -> &str { + match self { + AnySecurityAdvisory::Partial(p) => &p.advisory_id, + AnySecurityAdvisory::Full(s) => s.advisory_id(), + AnySecurityAdvisory::Ignored(i) => i.as_security_advisory().advisory_id(), + } + } + + pub fn package_name(&self) -> &str { + match self { + AnySecurityAdvisory::Partial(p) => &p.package_name, + AnySecurityAdvisory::Full(s) => s.package_name(), + AnySecurityAdvisory::Ignored(i) => i.as_security_advisory().package_name(), + } + } + + pub fn affected_versions(&self) -> &AnyConstraint { + match self { + AnySecurityAdvisory::Partial(p) => &p.affected_versions, + AnySecurityAdvisory::Full(s) => s.affected_versions(), + AnySecurityAdvisory::Ignored(i) => i.as_security_advisory().affected_versions(), + } + } + + pub fn as_security_advisory(&self) -> Option<&SecurityAdvisory> { + match self { + AnySecurityAdvisory::Partial(_) => None, + AnySecurityAdvisory::Full(s) => Some(s), + AnySecurityAdvisory::Ignored(i) => Some(i.as_security_advisory()), + } + } + + pub fn as_ignored(&self) -> Option<&IgnoredSecurityAdvisory> { + match self { + AnySecurityAdvisory::Ignored(i) => Some(i), + _ => None, + } + } +} diff --git a/crates/shirabe/src/advisory/auditor.rs b/crates/shirabe/src/advisory/auditor.rs index ffe1a12..d50fe70 100644 --- a/crates/shirabe/src/advisory/auditor.rs +++ b/crates/shirabe/src/advisory/auditor.rs @@ -6,12 +6,11 @@ use indexmap::IndexMap; use shirabe_external_packages::composer::pcre::Preg; use shirabe_external_packages::symfony::console::formatter::OutputFormatter; use shirabe_php_shim::{ - DATE_ATOM, InvalidArgumentException, PhpMixed, array_all, array_any, array_key_exists, - array_keys, array_reduce, get_class, is_string, sprintf, str_starts_with, + InvalidArgumentException, PhpMixed, array_all, array_any, array_key_exists, array_keys, + array_reduce, get_class, sprintf, str_starts_with, }; -use crate::advisory::IgnoredSecurityAdvisory; -use crate::advisory::PartialOrFullSecurityAdvisory; +use crate::advisory::AnySecurityAdvisory; use crate::advisory::SecurityAdvisory; use crate::io::ConsoleIO; use crate::io::IOInterface; @@ -178,7 +177,7 @@ impl Auditor { let error_or_warn = if warning_only { "warning" } else { "error" }; if affected_packages_count > 0 || ignored_advisories.len() > 0 { let passes: Vec<( - &IndexMap>, + &IndexMap>, String, )> = vec![ ( @@ -239,12 +238,12 @@ impl Auditor { Ok(audit_bitmask) } - /// @param array> $advisories + /// @param array> $advisories /// @param array $ignoreList /// @return bool pub fn needs_complete_advisory_load( &self, - advisories: &IndexMap>, + advisories: &IndexMap>, ignore_list: &IndexMap>, ) -> bool { if advisories.len() == 0 { @@ -252,20 +251,13 @@ impl Auditor { } // no partial advisories present - let advisories_values: Vec<&Vec> = - advisories.values().collect(); + let advisories_values: Vec<&Vec> = advisories.values().collect(); if array_all( &advisories_values, - |pkg_advisories: &&Vec| { - array_all( - pkg_advisories, - |_advisory: &PartialOrFullSecurityAdvisory| { - // TODO(phase-b): `$advisory instanceof SecurityAdvisory` — needs an advisory - // enum or trait downcast; SecurityAdvisoriesResult currently only holds - // PartialOrFullSecurityAdvisory so this is hard-coded to false - false - }, - ) + |pkg_advisories: &&Vec| { + array_all(pkg_advisories, |advisory: &AnySecurityAdvisory| { + advisory.as_security_advisory().is_some() + }) }, ) { return false; @@ -309,7 +301,7 @@ impl Auditor { pub fn process_advisories( &self, - all_advisories: IndexMap>, + all_advisories: IndexMap>, ignore_list: &IndexMap>, ignored_severities: &IndexMap>, ) -> ProcessAdvisoriesResult { @@ -320,8 +312,8 @@ impl Auditor { }; } - let mut advisories: IndexMap> = IndexMap::new(); - let mut ignored: IndexMap> = IndexMap::new(); + let mut advisories: IndexMap> = IndexMap::new(); + let mut ignored: IndexMap> = IndexMap::new(); let mut ignore_reason: Option = None; for (package, pkg_advisories) in all_advisories { @@ -341,40 +333,30 @@ impl Auditor { .unwrap_or(None); } - // TODO(phase-b): `$advisory instanceof SecurityAdvisory` — needs an advisory enum - // or trait downcast; the block below is skipped while SecurityAdvisoriesResult - // only holds PartialOrFullSecurityAdvisory - let advisory_as_full: Option<&SecurityAdvisory> = None; - if let Some(full) = advisory_as_full { - if is_string(&PhpMixed::String(full.severity.clone().unwrap_or_default())) - && array_key_exists( - full.severity.as_deref().unwrap_or(""), - ignored_severities, - ) - { - is_active = false; - let sev = full.severity.as_deref().unwrap_or(""); - ignore_reason = ignored_severities - .get(sev) - .cloned() - .unwrap_or_else(|| Some(format!("{} severity is ignored", sev))); + if let Some(full) = advisory.as_security_advisory() { + if let Some(severity) = &full.severity { + if array_key_exists(severity, ignored_severities) { + is_active = false; + ignore_reason = ignored_severities + .get(severity) + .cloned() + .flatten() + .or_else(|| Some(format!("{} severity is ignored", severity))); + } } - if is_string(&PhpMixed::String(full.cve.clone().unwrap_or_default())) - && array_key_exists(full.cve.as_deref().unwrap_or(""), ignore_list) - { - is_active = false; - ignore_reason = ignore_list - .get(full.cve.as_deref().unwrap_or("")) - .cloned() - .unwrap_or(None); + if let Some(cve) = &full.cve { + if array_key_exists(cve, ignore_list) { + is_active = false; + ignore_reason = ignore_list.get(cve).cloned().flatten(); + } } for source in &full.sources { let remote_id = source.get("remoteId").cloned().unwrap_or_default(); if array_key_exists(&remote_id, ignore_list) { is_active = false; - ignore_reason = ignore_list.get(&remote_id).cloned().unwrap_or(None); + ignore_reason = ignore_list.get(&remote_id).cloned().flatten(); break; } } @@ -390,9 +372,12 @@ impl Auditor { // Partial security advisories only used in summary mode // and in that case we do not need to cast the object. - // TODO(phase-b): `$advisory instanceof SecurityAdvisory` -> $advisory->toIgnoredAdvisory($ignoreReason) - let _: Option = None; - let _ = &ignore_reason; + let advisory = if advisory.as_security_advisory().is_some() { + let full = advisory.as_security_advisory().unwrap(); + AnySecurityAdvisory::Ignored(full.to_ignored_advisory(ignore_reason.clone())) + } else { + advisory + }; ignored .entry(package.clone()) @@ -410,7 +395,7 @@ impl Auditor { /// @return array{int, int} Count of affected packages and total count of advisories fn count_advisories( &self, - advisories: &IndexMap>, + advisories: &IndexMap>, ) -> (i64, i64) { let mut count: i64 = 0; for package_advisories in advisories.values() { @@ -425,7 +410,7 @@ impl Auditor { fn output_advisories( &self, io: &mut dyn IOInterface, - advisories: &IndexMap>, + advisories: &IndexMap>, format: &str, ) -> Result<()> { match format { @@ -463,7 +448,7 @@ impl Auditor { fn output_advisories_table( &self, io: &ConsoleIO, - advisories: &IndexMap>, + advisories: &IndexMap>, ) { for package_advisories in advisories.values() { for advisory in package_advisories { @@ -477,31 +462,43 @@ impl Auditor { "Affected versions".to_string(), "Reported at".to_string(), ]; - // TODO(phase-b): advisory typed PartialOrFullSecurityAdvisory; PHP accesses - // SecurityAdvisory fields (title, link, reportedAt, etc.) - let _ = advisory; - let row: Vec = vec![ - /* advisory.packageName */ String::new(), - /* self.get_severity(advisory) */ String::new(), - /* self.get_advisory_id(advisory) */ String::new(), - /* self.get_cve(advisory) */ String::new(), - /* advisory.title */ String::new(), - /* self.get_url(advisory) */ String::new(), - /* advisory.affectedVersions.getPrettyString() */ String::new(), - /* advisory.reportedAt.format(DATE_ATOM) */ String::new(), + let sa = advisory + .as_security_advisory() + .expect("output_advisories_table only receives full advisories"); + let mut row: Vec = vec![ + sa.package_name().to_string(), + self.get_severity(sa), + self.get_advisory_id(sa), + self.get_cve(sa), + sa.title.clone(), + self.get_url(sa), + sa.affected_versions().get_pretty_string(), + // TODO(phase-b): PHP uses `$advisory->reportedAt->format(DATE_ATOM)`, but + // shim DATE_ATOM ("Y-m-d\TH:i:sP") is a PHP format string incompatible with + // chrono. Using the chrono equivalent directly; revisit once a PHP-style date + // formatter exists (see also locker.rs DATE_RFC3339). + sa.reported_at.format("%Y-%m-%dT%H:%M:%S%:z").to_string(), ]; - let _ = DATE_ATOM; - // TODO(phase-b): `$advisory instanceof IgnoredSecurityAdvisory` downcast - let advisory_as_ignored: Option<&IgnoredSecurityAdvisory> = None; - if let Some(_ignored) = advisory_as_ignored { + if let Some(ignored) = advisory.as_ignored() { headers.push("Ignore reason".to_string()); - // row.push(ignored.ignore_reason.clone().unwrap_or_else(|| "None specified".to_string())); + row.push( + ignored + .ignore_reason + .clone() + .unwrap_or_else(|| "None specified".to_string()), + ); } - let _ = row; io.get_table() .set_horizontal(true) .set_headers(headers.into_iter().map(|h| h.into()).collect()) - .add_row(ConsoleIO::sanitize(PhpMixed::Null, false)) + .add_row(ConsoleIO::sanitize( + PhpMixed::List( + row.into_iter() + .map(|s| Box::new(PhpMixed::String(s))) + .collect(), + ), + false, + )) .set_column_width(1, 80) .set_column_max_width(1, 80) .render(); @@ -513,7 +510,7 @@ impl Auditor { fn output_advisories_plain( &self, io: &mut dyn IOInterface, - advisories: &IndexMap>, + advisories: &IndexMap>, ) { let mut error: Vec = vec![]; let mut first_advisory = true; @@ -522,40 +519,34 @@ impl Auditor { if !first_advisory { error.push("--------".to_string()); } - // TODO(phase-b): advisory typed PartialOrFullSecurityAdvisory; PHP accesses - // SecurityAdvisory fields - let _ = advisory; - error.push(format!("Package: {}", /* advisory.packageName */ "")); - error.push(format!( - "Severity: {}", - /* self.get_severity(advisory) */ "" - )); - error.push(format!( - "Advisory ID: {}", - /* self.get_advisory_id(advisory) */ "" - )); - error.push(format!("CVE: {}", /* self.get_cve(advisory) */ "")); - error.push(format!( - "Title: {}", - OutputFormatter::escape(/* advisory.title */ "") - )); - error.push(format!("URL: {}", /* self.get_url(advisory) */ "")); + let sa = advisory + .as_security_advisory() + .expect("output_advisories_plain only receives full advisories"); + error.push(format!("Package: {}", sa.package_name())); + error.push(format!("Severity: {}", self.get_severity(sa))); + error.push(format!("Advisory ID: {}", self.get_advisory_id(sa))); + error.push(format!("CVE: {}", self.get_cve(sa))); + error.push(format!("Title: {}", OutputFormatter::escape(&sa.title))); + error.push(format!("URL: {}", self.get_url(sa))); error.push(format!( "Affected versions: {}", - OutputFormatter::escape( - /* advisory.affectedVersions.getPrettyString() */ "" - ) + OutputFormatter::escape(&sa.affected_versions().get_pretty_string()) )); error.push(format!( "Reported at: {}", - /* advisory.reportedAt.format(DATE_ATOM) */ "" + // TODO(phase-b): PHP uses `$advisory->reportedAt->format(DATE_ATOM)`, but + // shim DATE_ATOM ("Y-m-d\TH:i:sP") is a PHP format string incompatible with + // chrono. Using the chrono equivalent directly; revisit once a PHP-style date + // formatter exists (see also locker.rs DATE_RFC3339). + sa.reported_at.format("%Y-%m-%dT%H:%M:%S%:z") )); - // TODO(phase-b): `$advisory instanceof IgnoredSecurityAdvisory` downcast - let advisory_as_ignored: Option<&IgnoredSecurityAdvisory> = None; - if let Some(_ignored) = advisory_as_ignored { + if let Some(ignored) = advisory.as_ignored() { error.push(format!( "Ignore reason: {}", - /* ignored.ignore_reason.unwrap_or("None specified") */ "" + ignored + .ignore_reason + .clone() + .unwrap_or_else(|| "None specified".to_string()) )); } first_advisory = false; @@ -680,9 +671,7 @@ impl Auditor { } fn get_advisory_id(&self, advisory: &SecurityAdvisory) -> String { - // TODO(phase-b): advisory.advisory_id lives on inner PartialOrFullSecurityAdvisory - let advisory_id: &str = ""; - let _ = advisory; + let advisory_id = advisory.advisory_id(); if str_starts_with(advisory_id, "PKSA-") { return format!( "{}", @@ -740,6 +729,6 @@ impl Auditor { #[derive(Debug)] pub struct ProcessAdvisoriesResult { - pub advisories: IndexMap>, - pub ignored_advisories: IndexMap>, + pub advisories: IndexMap>, + pub ignored_advisories: IndexMap>, } diff --git a/crates/shirabe/src/advisory/ignored_security_advisory.rs b/crates/shirabe/src/advisory/ignored_security_advisory.rs index 140c00b..fa1f8ae 100644 --- a/crates/shirabe/src/advisory/ignored_security_advisory.rs +++ b/crates/shirabe/src/advisory/ignored_security_advisory.rs @@ -6,7 +6,7 @@ use indexmap::IndexMap; use shirabe_php_shim::PhpMixed; use shirabe_semver::constraint::AnyConstraint; -#[derive(Debug, serde::Serialize)] +#[derive(Debug, Clone, serde::Serialize)] #[serde(rename_all = "camelCase")] pub struct IgnoredSecurityAdvisory { #[serde(flatten)] @@ -44,4 +44,8 @@ impl IgnoredSecurityAdvisory { ignore_reason, } } + + pub fn as_security_advisory(&self) -> &SecurityAdvisory { + &self.inner + } } diff --git a/crates/shirabe/src/advisory/mod.rs b/crates/shirabe/src/advisory/mod.rs index df80735..dbb90bd 100644 --- a/crates/shirabe/src/advisory/mod.rs +++ b/crates/shirabe/src/advisory/mod.rs @@ -1,13 +1,13 @@ +pub mod any_security_advisory; pub mod audit_config; pub mod auditor; pub mod ignored_security_advisory; -pub mod partial_or_full_security_advisory; pub mod partial_security_advisory; pub mod security_advisory; +pub use any_security_advisory::*; pub use audit_config::*; pub use auditor::*; pub use ignored_security_advisory::*; -pub use partial_or_full_security_advisory::*; pub use partial_security_advisory::*; pub use security_advisory::*; diff --git a/crates/shirabe/src/advisory/partial_or_full_security_advisory.rs b/crates/shirabe/src/advisory/partial_or_full_security_advisory.rs deleted file mode 100644 index bb1df78..0000000 --- a/crates/shirabe/src/advisory/partial_or_full_security_advisory.rs +++ /dev/null @@ -1,25 +0,0 @@ -use crate::advisory::PartialSecurityAdvisory; -use crate::advisory::SecurityAdvisory; -use shirabe_semver::constraint::AnyConstraint; - -#[derive(Debug, Clone)] -pub enum PartialOrFullSecurityAdvisory { - Partial(PartialSecurityAdvisory), - Full(SecurityAdvisory), -} - -impl PartialOrFullSecurityAdvisory { - pub fn advisory_id(&self) -> &str { - match self { - PartialOrFullSecurityAdvisory::Partial(p) => &p.advisory_id, - PartialOrFullSecurityAdvisory::Full(s) => s.advisory_id(), - } - } - - pub fn affected_versions(&self) -> &AnyConstraint { - match self { - PartialOrFullSecurityAdvisory::Partial(p) => &p.affected_versions, - PartialOrFullSecurityAdvisory::Full(s) => s.affected_versions(), - } - } -} diff --git a/crates/shirabe/src/advisory/partial_security_advisory.rs b/crates/shirabe/src/advisory/partial_security_advisory.rs index 8815103..2062570 100644 --- a/crates/shirabe/src/advisory/partial_security_advisory.rs +++ b/crates/shirabe/src/advisory/partial_security_advisory.rs @@ -1,6 +1,6 @@ //! ref: composer/src/Composer/Advisory/PartialSecurityAdvisory.php -use crate::advisory::PartialOrFullSecurityAdvisory; +use crate::advisory::AnySecurityAdvisory; use crate::advisory::SecurityAdvisory; use crate::package::version::VersionParser; use anyhow::Result; @@ -32,7 +32,7 @@ impl PartialSecurityAdvisory { package_name: &str, data: &IndexMap, parser: &VersionParser, - ) -> Result { + ) -> Result { let affected_versions_str = data["affectedVersions"].as_string().unwrap_or(""); let constraint: AnyConstraint = match parser.parse_constraints(affected_versions_str) { @@ -95,10 +95,10 @@ impl PartialSecurityAdvisory { .and_then(|v| v.as_string()) .map(|s| s.to_string()), ); - return Ok(PartialOrFullSecurityAdvisory::Full(advisory)); + return Ok(AnySecurityAdvisory::Full(advisory)); } - Ok(PartialOrFullSecurityAdvisory::Partial(Self { + Ok(AnySecurityAdvisory::Partial(Self { advisory_id: data["advisoryId"].as_string().unwrap_or("").to_string(), package_name: package_name.to_string(), affected_versions: constraint, diff --git a/crates/shirabe/src/advisory/security_advisory.rs b/crates/shirabe/src/advisory/security_advisory.rs index c766e0f..5cb5c59 100644 --- a/crates/shirabe/src/advisory/security_advisory.rs +++ b/crates/shirabe/src/advisory/security_advisory.rs @@ -48,6 +48,10 @@ impl SecurityAdvisory { &self.inner.advisory_id } + pub fn package_name(&self) -> &str { + &self.inner.package_name + } + pub fn affected_versions(&self) -> &AnyConstraint { &self.inner.affected_versions } -- cgit v1.3.1