From 6a126eb2720bcf1880c44ba4aca4435d4732be03 Mon Sep 17 00:00:00 2001 From: joshlf's Agent Date: Tue, 25 Aug 2026 17:14:16 +0000 Subject: [PATCH] [ci] Validate all inputs before CI planning Add one loading boundary which reads the typed policy, audits live Cargo metadata and repository files, inventories every workflow job, and strictly parses the frozen legacy baseline. Return planning inputs only after every layer succeeds. Wiring the workflow audit here also removes the temporary unused-code allowance from its introductory commit. This keeps future planners from depending on call order or accidentally skipping a validation pass. It also keeps the policy-owned baseline paths in one place rather than repeating their coordination in each consumer. Resolve each input through the canonical repository boundary before opening it. Reject baseline roles which identify the same underlying file even when their policy spellings differ through symlinks or hard links, so supposedly independent review evidence cannot silently alias. Cross-platform open handles keep each identity stable for the complete comparison. Tests: CARGO_NET_OFFLINE=true ./ci/check_tools.sh Tests: ./ci/check_fmt.sh gherrit-pr-id: Gsizgstaatljzyuwyc4tp4ha6krvibz64 --- tools/zc/src/baseline.rs | 63 +++- tools/zc/src/ci.rs | 569 ++++++++++++++++++++++++++++++++ tools/zc/src/lib.rs | 1 + tools/zc/src/repository_file.rs | 16 +- tools/zc/src/workflow.rs | 163 ++++++--- 5 files changed, 768 insertions(+), 44 deletions(-) create mode 100644 tools/zc/src/ci.rs diff --git a/tools/zc/src/baseline.rs b/tools/zc/src/baseline.rs index ae65f7d287..dcf1ee6bf9 100644 --- a/tools/zc/src/baseline.rs +++ b/tools/zc/src/baseline.rs @@ -24,7 +24,9 @@ use std::{ collections::{BTreeMap, BTreeSet}, error::Error, - fmt, fs, io, + fmt, + fs::{self, File}, + io::{self, Read}, path::{Component, Path, PathBuf}, str::FromStr, }; @@ -56,6 +58,23 @@ pub struct LegacyBaselinePaths { pub command_goldens: PathBuf, } +/// Already-open files corresponding exactly to [`LegacyBaselinePaths`]. +/// +/// The checked CI boundary retains these handles while it compares file +/// identities and parses bytes. Keep the fields coordinated with +/// [`LegacyBaselinePaths`], [`LegacyBaselines::read_open`], and the bundle +/// construction in `ci.rs`; adding a baseline role requires updating all four. +pub(crate) struct LegacyBaselineFiles<'a> { + pub manifest: &'a File, + pub build_reduced: &'a File, + pub build_full: &'a File, + pub miri_reduced: &'a File, + pub miri_full: &'a File, + pub logical_obligations: &'a File, + pub standalone_obligations: &'a File, + pub command_goldens: &'a File, +} + /// One lowercase, stable identifier used by the old workflow matrices. /// /// This deliberately matches the identifier grammar used by the typed policy. @@ -842,6 +861,36 @@ impl LegacyBaselines { ) } + /// Reads and parses baseline bytes through already-validated handles. + pub(crate) fn read_open( + paths: &LegacyBaselinePaths, + files: LegacyBaselineFiles<'_>, + ) -> Result { + let manifest = read_open_source(&paths.manifest, files.manifest)?; + let build_reduced = read_open_source(&paths.build_reduced, files.build_reduced)?; + let build_full = read_open_source(&paths.build_full, files.build_full)?; + let miri_reduced = read_open_source(&paths.miri_reduced, files.miri_reduced)?; + let miri_full = read_open_source(&paths.miri_full, files.miri_full)?; + let logical_obligations = + read_open_source(&paths.logical_obligations, files.logical_obligations)?; + let standalone_obligations = + read_open_source(&paths.standalone_obligations, files.standalone_obligations)?; + let command_goldens = read_open_source(&paths.command_goldens, files.command_goldens)?; + Self::parse_sources( + paths, + BaselineSources { + manifest: &manifest, + build_reduced: &build_reduced, + build_full: &build_full, + miri_reduced: &miri_reduced, + miri_full: &miri_full, + logical_obligations: &logical_obligations, + standalone_obligations: &standalone_obligations, + command_goldens: &command_goldens, + }, + ) + } + fn parse_sources( paths: &LegacyBaselinePaths, sources: BaselineSources<'_>, @@ -1081,6 +1130,18 @@ fn read_source(path: &Path) -> Result { }) } +fn read_open_source(path: &Path, file: &File) -> Result { + let mut source = String::new(); + let mut reader = file; + reader.read_to_string(&mut source).map_err(|source| BaselineError { + path: path.to_path_buf(), + line: None, + message: format!("failed to read file: {source}"), + source: Some(source), + })?; + Ok(source) +} + struct BaselineSources<'a> { manifest: &'a str, build_reduced: &'a str, diff --git a/tools/zc/src/ci.rs b/tools/zc/src/ci.rs new file mode 100644 index 0000000000..59015f1a37 --- /dev/null +++ b/tools/zc/src/ci.rs @@ -0,0 +1,569 @@ +// Copyright 2026 The Fuchsia Authors +// +// Licensed under a BSD-style license , Apache License, Version 2.0 +// , or the MIT +// license , at your option. +// This file may not be copied, modified, or distributed except according to +// those terms. + +//! One checked boundary between repository state and CI planning. +//! +//! Loading CI inputs is intentionally all-or-nothing. A caller cannot obtain a +//! [`CiInputs`] until the policy is valid, its references agree with live Cargo +//! metadata and repository files, every workflow job has an exact reviewed +//! role, and every independently recorded legacy baseline parses canonically. +//! Planners therefore consume checked data rather than remembering which +//! validation passes must precede which lookups. + +use std::{ + collections::HashMap, + io, + path::{Path, PathBuf}, +}; + +use thiserror::Error; + +use crate::{ + baseline::{BaselineError, LegacyBaselineFiles, LegacyBaselinePaths, LegacyBaselines}, + inventory::{AuditError, RepositoryInventory}, + policy::{Baselines, Policy, ReadPolicyError}, + repository_file::{self, OpenRepositoryFileError, OpenedRepositoryFile}, + workflow::{ + audit_workflows, ReviewedWorkflowJobs, WorkflowAuditError, WorkflowRegistryError, + WORKFLOW_REGISTRY_PATH, + }, +}; + +/// The repository-relative location of the typed CI policy. +pub const POLICY_PATH: &str = "ci/zc.toml"; + +/// All repository-owned inputs accepted for CI planning. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct CiInputs { + policy: Policy, + repository: RepositoryInventory, + workflow_jobs: ReviewedWorkflowJobs, + legacy: LegacyBaselines, +} + +impl CiInputs { + /// Loads and checks every planning input below `repository_root`. + pub fn load(repository_root: impl AsRef) -> Result { + let supplied_root = repository_root.as_ref(); + let repository_root = supplied_root.canonicalize().map_err(|source| { + LoadCiError::RepositoryRoot { path: supplied_root.to_path_buf(), source } + })?; + let policy_file = open_repository_file(&repository_root, Path::new(POLICY_PATH))?; + let policy_source = policy_file.read_to_string().map_err(|source| { + LoadCiError::Policy(ReadPolicyError::Read { + path: policy_file.path().to_path_buf(), + source, + }) + })?; + let policy = Policy::parse(&policy_source).map_err(|source| { + LoadCiError::Policy(ReadPolicyError::Policy { + path: policy_file.path().to_path_buf(), + source, + }) + })?; + // The registry is a fixed runtime input rather than a policy-selected + // path. Open it through the same containment boundary, then give the + // workflow module only the parsed registry. A checked-in or concurrent + // replacement therefore cannot redirect or change the reviewed role + // assignments after validation. + let workflow_registry = + open_repository_file(&repository_root, Path::new(WORKFLOW_REGISTRY_PATH))?; + let workflow_registry_source = workflow_registry.read_to_string().map_err(|source| { + LoadCiError::Workflow(Box::new(WorkflowAuditError::Registry( + WorkflowRegistryError::Read { + path: workflow_registry.path().to_path_buf(), + source, + }, + ))) + })?; + let reviewed_workflow_jobs = + ReviewedWorkflowJobs::parse(workflow_registry.path(), &workflow_registry_source) + .map_err(|error| { + LoadCiError::Workflow(Box::new(WorkflowAuditError::Registry(error))) + })?; + let (workflow_jobs, _workflow_sources) = + audit_workflows(&repository_root, reviewed_workflow_jobs) + .map_err(|error| LoadCiError::Workflow(Box::new(error)))?; + let repository = RepositoryInventory::audit(&repository_root, &policy) + .map_err(LoadCiError::Inventory)?; + let baseline_files = OpenLegacyBaselineFiles::open(&repository_root, policy.baselines())?; + let paths = baseline_files.paths(); + // Policy validation rejects two fields with the same lexical path. + // This check establishes the stronger file-system identity used at + // runtime. Keep both checks: symlinks and hard links can give one file + // different in-tree names, which would make supposedly independent + // review evidence alias. + reject_duplicate_baseline_inputs(&baseline_files)?; + let legacy = LegacyBaselines::read_open(&paths, baseline_files.files()) + .map_err(LoadCiError::Baseline)?; + Ok(Self { policy, repository, workflow_jobs, legacy }) + } + + /// Returns the checked coverage policy. + pub fn policy(&self) -> &Policy { + &self.policy + } + + /// Returns facts collected from the live checkout. + pub fn repository(&self) -> &RepositoryInventory { + &self.repository + } + + /// Returns the checked role assignment for every live workflow job. + pub fn workflow_jobs(&self) -> &ReviewedWorkflowJobs { + &self.workflow_jobs + } + + /// Returns the independent description of legacy CI behavior. + pub fn legacy(&self) -> &LegacyBaselines { + &self.legacy + } +} + +/// The complete open-handle counterpart to [`LegacyBaselinePaths`]. +/// +/// Keep this field list, [`Self::paths`], [`Self::files`], [`Self::named`], and +/// [`LegacyBaselineFiles`] synchronized. Repeating the shape explicitly is +/// intentional: adding a new independent evidence role must fail to compile or +/// require a visible edit at every identity and parsing boundary. +struct OpenLegacyBaselineFiles { + manifest: OpenedRepositoryFile, + build_reduced: OpenedRepositoryFile, + build_full: OpenedRepositoryFile, + miri_reduced: OpenedRepositoryFile, + miri_full: OpenedRepositoryFile, + logical_obligations: OpenedRepositoryFile, + standalone_obligations: OpenedRepositoryFile, + command_goldens: OpenedRepositoryFile, +} + +impl OpenLegacyBaselineFiles { + fn open(repository_root: &Path, baselines: &Baselines) -> Result { + let configured = LegacyBaselinePaths { + manifest: baselines.manifest().as_path().to_path_buf(), + build_reduced: baselines.build_reduced().as_path().to_path_buf(), + build_full: baselines.build_full().as_path().to_path_buf(), + miri_reduced: baselines.miri_reduced().as_path().to_path_buf(), + miri_full: baselines.miri_full().as_path().to_path_buf(), + logical_obligations: baselines.logical_obligations().as_path().to_path_buf(), + standalone_obligations: baselines.standalone_obligations().as_path().to_path_buf(), + command_goldens: baselines.command_goldens().as_path().to_path_buf(), + }; + Self::open_paths(repository_root, &configured) + } + + fn open_paths( + repository_root: &Path, + paths: &LegacyBaselinePaths, + ) -> Result { + Ok(Self { + manifest: open_repository_file(repository_root, &paths.manifest)?, + build_reduced: open_repository_file(repository_root, &paths.build_reduced)?, + build_full: open_repository_file(repository_root, &paths.build_full)?, + miri_reduced: open_repository_file(repository_root, &paths.miri_reduced)?, + miri_full: open_repository_file(repository_root, &paths.miri_full)?, + logical_obligations: open_repository_file(repository_root, &paths.logical_obligations)?, + standalone_obligations: open_repository_file( + repository_root, + &paths.standalone_obligations, + )?, + command_goldens: open_repository_file(repository_root, &paths.command_goldens)?, + }) + } + + fn paths(&self) -> LegacyBaselinePaths { + LegacyBaselinePaths { + manifest: self.manifest.path().to_path_buf(), + build_reduced: self.build_reduced.path().to_path_buf(), + build_full: self.build_full.path().to_path_buf(), + miri_reduced: self.miri_reduced.path().to_path_buf(), + miri_full: self.miri_full.path().to_path_buf(), + logical_obligations: self.logical_obligations.path().to_path_buf(), + standalone_obligations: self.standalone_obligations.path().to_path_buf(), + command_goldens: self.command_goldens.path().to_path_buf(), + } + } + + fn files(&self) -> LegacyBaselineFiles<'_> { + LegacyBaselineFiles { + manifest: self.manifest.file(), + build_reduced: self.build_reduced.file(), + build_full: self.build_full.file(), + miri_reduced: self.miri_reduced.file(), + miri_full: self.miri_full.file(), + logical_obligations: self.logical_obligations.file(), + standalone_obligations: self.standalone_obligations.file(), + command_goldens: self.command_goldens.file(), + } + } + + fn named(&self) -> [(&'static str, &OpenedRepositoryFile); 8] { + [ + ("baselines.manifest", &self.manifest), + ("baselines.build_reduced", &self.build_reduced), + ("baselines.build_full", &self.build_full), + ("baselines.miri_reduced", &self.miri_reduced), + ("baselines.miri_full", &self.miri_full), + ("baselines.logical_obligations", &self.logical_obligations), + ("baselines.standalone_obligations", &self.standalone_obligations), + ("baselines.command_goldens", &self.command_goldens), + ] + } +} + +/// Rejects baseline fields which identify the same already-open file. +/// +/// The identity handle and parsed byte handle were cloned from one open file. +/// Keeping both alive through comparison and parsing prevents a replacement +/// path from changing either the alias relation or the accepted evidence. +fn reject_duplicate_baseline_inputs(inputs: &OpenLegacyBaselineFiles) -> Result<(), LoadCiError> { + let mut first_input_by_identity = HashMap::new(); + for (field, input) in inputs.named() { + if let Some((first_field, first_path)) = + first_input_by_identity.insert(input.identity(), (field, input.path())) + { + return Err(LoadCiError::DuplicateBaselineInput { + first_field, + first_path: first_path.to_path_buf(), + second_field: field, + second_path: input.path().to_path_buf(), + }); + } + } + Ok(()) +} + +fn open_repository_file( + repository_root: &Path, + configured: &Path, +) -> Result { + repository_file::open(repository_root, configured).map_err(|error| match error { + OpenRepositoryFileError::Path { path, source } => LoadCiError::InputPath { path, source }, + OpenRepositoryFileError::Identity { path, source } => { + LoadCiError::InputIdentity { path, source } + } + OpenRepositoryFileError::ChangedDuringOpen { path, first, second } => { + LoadCiError::InputChangedDuringOpen { path, first, second } + } + OpenRepositoryFileError::OutsideRepository { path, resolved, repository_root } => { + LoadCiError::InputOutsideRepository { path, resolved, repository_root } + } + OpenRepositoryFileError::NotFile { path } => LoadCiError::InputNotFile { path }, + }) +} + +/// A failure loading one layer of CI planning input. +#[derive(Debug, Error)] +pub enum LoadCiError { + /// The supplied repository root could not be resolved. + #[error("failed to resolve repository root `{path}`: {source}")] + RepositoryRoot { + path: PathBuf, + #[source] + source: io::Error, + }, + /// A configured input could not be resolved or inspected. + #[error("failed to resolve CI input `{path}`: {source}")] + InputPath { + path: PathBuf, + #[source] + source: io::Error, + }, + /// A configured input's stable file-system identity could not be read. + #[error("failed to inspect filesystem identity of CI input `{path}`: {source}")] + InputIdentity { + path: PathBuf, + #[source] + source: io::Error, + }, + /// A path no longer named the file which was opened and retained. + #[error( + "CI input `{path}` changed while it was opened: first resolved to `{first}`, then to `{second}`" + )] + InputChangedDuringOpen { + /// Configured path joined to the repository root. + path: PathBuf, + /// Canonical destination checked before opening. + first: PathBuf, + /// Canonical destination checked after opening. + second: PathBuf, + }, + /// A configured input resolved outside the checkout. + #[error("CI input `{path}` resolves to `{resolved}`, outside repository `{repository_root}`")] + InputOutsideRepository { path: PathBuf, resolved: PathBuf, repository_root: PathBuf }, + /// A configured input resolved to a directory or other non-file object. + #[error("CI input `{path}` is not a regular file")] + InputNotFile { path: PathBuf }, + /// Two independently reviewed baseline fields identify the same file. + #[error( + "CI baseline inputs `{first_field}` (`{first_path}`) and `{second_field}` (`{second_path}`) identify the same file" + )] + DuplicateBaselineInput { + first_field: &'static str, + first_path: PathBuf, + second_field: &'static str, + second_path: PathBuf, + }, + /// The typed policy was unreadable or invalid. + #[error(transparent)] + Policy(ReadPolicyError), + /// Live repository state did not satisfy the policy. + #[error(transparent)] + Inventory(AuditError), + /// Workflow files or their reviewed role assignments were invalid. + #[error(transparent)] + Workflow(Box), + /// The frozen legacy evidence was unreadable or noncanonical. + #[error(transparent)] + Baseline(BaselineError), +} + +#[cfg(test)] +mod tests { + use std::{fs, path::Path}; + + use super::{ + open_repository_file, reject_duplicate_baseline_inputs, CiInputs, LoadCiError, + OpenLegacyBaselineFiles, + }; + use crate::baseline::LegacyBaselinePaths; + + /// Creates the complete input shape consumed by + /// `reject_duplicate_baseline_inputs`, with one distinct regular file per + /// field. Keep this list coordinated with `LegacyBaselinePaths` and the + /// production field-name list in that function. + fn write_distinct_baselines(repository: &Path) -> LegacyBaselinePaths { + let paths = LegacyBaselinePaths { + manifest: repository.join("manifest.tsv"), + build_reduced: repository.join("build-reduced.tsv"), + build_full: repository.join("build-full.tsv"), + miri_reduced: repository.join("miri-reduced.tsv"), + miri_full: repository.join("miri-full.tsv"), + logical_obligations: repository.join("logical.tsv"), + standalone_obligations: repository.join("standalone.tsv"), + command_goldens: repository.join("commands.tsv"), + }; + for path in [ + &paths.manifest, + &paths.build_reduced, + &paths.build_full, + &paths.miri_reduced, + &paths.miri_full, + &paths.logical_obligations, + &paths.standalone_obligations, + &paths.command_goldens, + ] { + fs::write(path, format!("{}\n", path.display())).unwrap(); + } + paths + } + + #[test] + fn loads_every_current_planning_input_together() { + let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("../.."); + let inputs = CiInputs::load(root).unwrap(); + + assert_eq!(inputs.legacy().build_reduced().len(), 60); + assert_eq!(inputs.legacy().build_full().len(), 182); + assert!(inputs.legacy().miri_reduced().is_empty()); + assert_eq!(inputs.legacy().miri_full().len(), 64); + assert_eq!(inputs.repository().policy_packages().len(), 2); + } + + #[cfg(unix)] + #[test] + fn rejects_an_input_symlink_which_escapes_the_repository() { + use std::{ + fs, + os::unix::fs::symlink, + process, + sync::atomic::{AtomicU64, Ordering}, + }; + + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); + let unique = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let temporary = + std::env::temp_dir().join(format!("zerocopy-ci-input-test-{}-{unique}", process::id())); + let repository = temporary.join("repository"); + let outside = temporary.join("outside.tsv"); + fs::create_dir_all(repository.join("ci")).unwrap(); + fs::write(&outside, "external\n").unwrap(); + symlink(&outside, repository.join("ci/baseline.tsv")).unwrap(); + let repository = repository.canonicalize().unwrap(); + + let error = open_repository_file(&repository, Path::new("ci/baseline.tsv")).unwrap_err(); + assert!(matches!(error, LoadCiError::InputOutsideRepository { .. })); + + fs::remove_dir_all(temporary).unwrap(); + } + + #[cfg(unix)] + #[test] + fn path_replacement_cannot_change_bytes_read_from_an_open_input() { + use std::{ + os::unix::fs::symlink, + process, + sync::atomic::{AtomicU64, Ordering}, + }; + + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); + let unique = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let temporary = std::env::temp_dir() + .join(format!("zerocopy-ci-open-handle-test-{}-{unique}", process::id())); + let repository = temporary.join("repository"); + let configured = repository.join("ci/input.txt"); + let retained = repository.join("ci/retained.txt"); + let outside = temporary.join("outside.txt"); + fs::create_dir_all(configured.parent().unwrap()).unwrap(); + fs::write(&configured, "validated bytes\n").unwrap(); + fs::write(&outside, "replacement bytes\n").unwrap(); + let repository = repository.canonicalize().unwrap(); + + let input = open_repository_file(&repository, Path::new("ci/input.txt")).unwrap(); + fs::rename(&configured, &retained).unwrap(); + symlink(&outside, &configured).unwrap(); + + assert_eq!(input.read_to_string().unwrap(), "validated bytes\n"); + + drop(input); + fs::remove_dir_all(temporary).unwrap(); + } + + #[cfg(unix)] + #[test] + fn baseline_identity_and_bytes_share_the_retained_handles() { + use std::{ + process, + sync::atomic::{AtomicU64, Ordering}, + }; + + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); + let unique = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let temporary = std::env::temp_dir() + .join(format!("zerocopy-ci-baseline-retained-handle-test-{}-{unique}", process::id())); + fs::create_dir_all(&temporary).unwrap(); + let repository = temporary.canonicalize().unwrap(); + let paths = write_distinct_baselines(&repository); + fs::write(&paths.build_reduced, "first bytes\n").unwrap(); + fs::write(&paths.build_full, "second bytes\n").unwrap(); + let inputs = OpenLegacyBaselineFiles::open_paths(&repository, &paths).unwrap(); + + fs::remove_file(&paths.build_full).unwrap(); + fs::hard_link(&paths.build_reduced, &paths.build_full).unwrap(); + + // The path names now alias, but the retained handles still identify + // and read the two distinct files which were originally opened. + reject_duplicate_baseline_inputs(&inputs).unwrap(); + assert_eq!(inputs.build_reduced.read_to_string().unwrap(), "first bytes\n"); + assert_eq!(inputs.build_full.read_to_string().unwrap(), "second bytes\n"); + + drop(inputs); + fs::remove_dir_all(temporary).unwrap(); + } + + #[cfg(unix)] + #[test] + fn rejects_baseline_fields_which_identify_one_file_through_a_symlink() { + use std::{ + os::unix::fs::symlink, + process, + sync::atomic::{AtomicU64, Ordering}, + }; + + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); + let unique = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let temporary = std::env::temp_dir() + .join(format!("zerocopy-ci-baseline-alias-test-{}-{unique}", process::id())); + let repository = temporary.join("repository"); + fs::create_dir_all(&repository).unwrap(); + let repository = repository.canonicalize().unwrap(); + let paths = write_distinct_baselines(&repository); + fs::remove_file(&paths.build_full).unwrap(); + symlink("build-reduced.tsv", &paths.build_full).unwrap(); + + let inputs = OpenLegacyBaselineFiles::open_paths(&repository, &paths).unwrap(); + let resolved = inputs.paths(); + assert_eq!(resolved.build_reduced, resolved.build_full); + + let error = reject_duplicate_baseline_inputs(&inputs).unwrap_err(); + assert!(matches!( + error, + LoadCiError::DuplicateBaselineInput { + first_field: "baselines.build_reduced", + first_path, + second_field: "baselines.build_full", + second_path, + } if first_path == resolved.build_reduced && second_path == resolved.build_full + )); + + drop(inputs); + fs::remove_dir_all(temporary).unwrap(); + } + + #[test] + fn rejects_hard_linked_baseline_fields() { + use std::{ + process, + sync::atomic::{AtomicU64, Ordering}, + }; + + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); + let unique = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let temporary = std::env::temp_dir() + .join(format!("zerocopy-ci-baseline-hard-link-test-{}-{unique}", process::id())); + fs::create_dir_all(&temporary).unwrap(); + let repository = temporary.canonicalize().unwrap(); + let paths = write_distinct_baselines(&repository); + fs::remove_file(&paths.build_full).unwrap(); + fs::hard_link(&paths.build_reduced, &paths.build_full).unwrap(); + + let inputs = OpenLegacyBaselineFiles::open_paths(&repository, &paths).unwrap(); + let resolved = inputs.paths(); + assert_ne!(resolved.build_reduced, resolved.build_full); + + let error = reject_duplicate_baseline_inputs(&inputs).unwrap_err(); + assert!(matches!( + error, + LoadCiError::DuplicateBaselineInput { + first_field: "baselines.build_reduced", + first_path, + second_field: "baselines.build_full", + second_path, + } if first_path == resolved.build_reduced && second_path == resolved.build_full + )); + + drop(inputs); + fs::remove_dir_all(temporary).unwrap(); + } + + #[test] + fn accepts_distinct_baseline_files_with_identical_contents() { + use std::{ + process, + sync::atomic::{AtomicU64, Ordering}, + }; + + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); + let unique = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let temporary = std::env::temp_dir().join(format!( + "zerocopy-ci-baseline-identical-content-test-{}-{unique}", + process::id() + )); + fs::create_dir_all(&temporary).unwrap(); + let repository = temporary.canonicalize().unwrap(); + let paths = write_distinct_baselines(&repository); + fs::write(&paths.build_reduced, "identical\n").unwrap(); + fs::write(&paths.build_full, "identical\n").unwrap(); + + let inputs = OpenLegacyBaselineFiles::open_paths(&repository, &paths).unwrap(); + reject_duplicate_baseline_inputs(&inputs).unwrap(); + + drop(inputs); + fs::remove_dir_all(temporary).unwrap(); + } +} diff --git a/tools/zc/src/lib.rs b/tools/zc/src/lib.rs index ff9cc4250e..4502971942 100644 --- a/tools/zc/src/lib.rs +++ b/tools/zc/src/lib.rs @@ -9,6 +9,7 @@ //! Shared, typed access to repository configuration and CI behavior. pub mod baseline; +pub mod ci; mod identifier; pub mod inventory; pub mod metadata; diff --git a/tools/zc/src/repository_file.rs b/tools/zc/src/repository_file.rs index d9d34679eb..c026dff9ab 100644 --- a/tools/zc/src/repository_file.rs +++ b/tools/zc/src/repository_file.rs @@ -28,6 +28,10 @@ use thiserror::Error; pub(crate) struct OpenedRepositoryFile { path: PathBuf, file: File, + // Retain this independently open handle for callers which compare file + // identity. Some platforms may reuse an identifier once its last handle + // closes, so a numeric identity captured and then dropped is insufficient. + identity: Handle, } impl OpenedRepositoryFile { @@ -36,6 +40,16 @@ impl OpenedRepositoryFile { &self.path } + /// Returns the retained file used for all subsequent structured reads. + pub(crate) fn file(&self) -> &File { + &self.file + } + + /// Returns the retained filesystem identity derived from [`Self::file`]. + pub(crate) fn identity(&self) -> &Handle { + &self.identity + } + /// Reads text from the retained file rather than reopening its path. pub(crate) fn read_to_string(&self) -> io::Result { let mut file = &self.file; @@ -122,7 +136,7 @@ pub(crate) fn open( }); } - Ok(OpenedRepositoryFile { path: rechecked, file }) + Ok(OpenedRepositoryFile { path: rechecked, file, identity }) } /// A failure to open and retain one repository file safely. diff --git a/tools/zc/src/workflow.rs b/tools/zc/src/workflow.rs index a4292bb3d1..e4b2dbe903 100644 --- a/tools/zc/src/workflow.rs +++ b/tools/zc/src/workflow.rs @@ -23,14 +23,14 @@ use std::{ collections::{BTreeMap, BTreeSet}, - fmt, - fs::{self, File}, - io::{self, Read}, + fmt, fs, io, path::{Path, PathBuf}, }; use thiserror::Error; +use crate::repository_file::{self, OpenRepositoryFileError}; + const WORKFLOW_DIRECTORY: &str = ".github/workflows"; const REGISTRY_HEADER: &str = "workflow\tjob\trole"; @@ -202,7 +202,7 @@ impl ReviewedWorkflowJobs { Self::parse(path, &source) } - fn parse(path: &Path, source: &str) -> Result { + pub(crate) fn parse(path: &Path, source: &str) -> Result { let mut saw_header = false; let mut previous: Option = None; let mut jobs = BTreeMap::new(); @@ -285,26 +285,18 @@ impl ReviewedWorkflowJobs { /// Checks every live workflow job against its reviewed role assignment. /// -/// `reviewed_registry` must be the canonical path returned by the CI input -/// boundary's containment check. Keeping resolution there prevents a caller -/// from validating one path and reopening a different spelling here. This -/// function then performs the remaining workflow-specific boundary exactly -/// once: it reads the strict registry, scans every workflow, and reports all -/// missing or unreviewed jobs together in deterministic order. -// This validator deliberately lands before the all-input boundary in `ci.rs` -// so its scanner and reviewed registry can be examined independently. The -// later wiring commit removes this temporary allowance when production code -// begins calling it; tests exercise it in the meantime. -#[allow(dead_code)] +/// `reviewed` must have been parsed from the open handle retained by the CI +/// input boundary. Taking the checked value instead of a path makes reopening +/// a replaced registry impossible here. This function scans every workflow and +/// reports all missing or unreviewed jobs together in deterministic order. pub(crate) fn audit_workflows( repository_root: impl AsRef, - reviewed_registry: impl AsRef, -) -> Result { - let reviewed = ReviewedWorkflowJobs::read(reviewed_registry)?; + reviewed: ReviewedWorkflowJobs, +) -> Result<(ReviewedWorkflowJobs, WorkflowSources), WorkflowAuditError> { let actual = read_workflow_sources(repository_root)?; let violations = compare_with_reviewed(&actual.inventories, &reviewed); if violations.is_empty() { - Ok(reviewed) + Ok((reviewed, actual)) } else { Err(WorkflowAuditError::Violations(WorkflowAuditViolations(violations))) } @@ -326,6 +318,19 @@ fn read_workflow_sources( let repository_root = supplied_root.canonicalize().map_err(|source| { WorkflowInventoryError::ResolveRepositoryRoot { path: supplied_root.to_path_buf(), source } })?; + let workflow_files = discover_workflow_files(&repository_root)?; + read_workflow_files(&repository_root, workflow_files) +} + +/// Inventories workflow directory entries before any candidate is opened. +/// +/// Keep this separate from [`read_workflow_files`]. In addition to making the +/// two filesystem observations explicit, the separation lets tests replace a +/// candidate after `DirEntry::file_type` and prove that the retained-handle +/// boundary rejects the exact race which this inventory must defend against. +fn discover_workflow_files( + repository_root: &Path, +) -> Result, WorkflowInventoryError> { let directory = repository_root.join(WORKFLOW_DIRECTORY); let resolved_directory = directory.canonicalize().map_err(|source| { WorkflowInventoryError::ReadDirectory { path: directory.clone(), source } @@ -382,24 +387,30 @@ fn read_workflow_sources( if workflow_files.is_empty() { return Err(WorkflowInventoryError::NoWorkflowFiles { path: directory }); } + Ok(workflow_files) +} +/// Opens and reads candidates collected by [`discover_workflow_files`]. +fn read_workflow_files( + repository_root: &Path, + workflow_files: Vec<(PathBuf, WorkflowPath)>, +) -> Result { let mut inventories = Vec::with_capacity(workflow_files.len()); let mut sources = BTreeMap::new(); for (path, workflow_path) in workflow_files { - let file = File::open(&path).map_err(|source| WorkflowInventoryError::ReadWorkflow { - path: path.clone(), - source, - })?; - let metadata = file.metadata().map_err(|source| { - WorkflowInventoryError::InspectWorkflow { path: path.clone(), source } - })?; - // Recheck the opened object, rather than relying only on the earlier - // directory-entry observation. An ordinary replacement which changes - // the candidate to a directory or special file must not be read. - if !metadata.is_file() { - return Err(WorkflowInventoryError::NotAFile { path }); + let opened = repository_file::open(repository_root, Path::new(workflow_path.as_str())) + .map_err(map_workflow_file_error)?; + // GitHub discovers a workflow only when the repository-tree entry is a + // direct regular file. The shared opener permits contained symlinks + // for policy-selected inputs, so impose this workflow-specific rule + // after it has checked and retained the opened object. + if opened.path() != path { + return Err(WorkflowInventoryError::RedirectedWorkflowFile { + path, + resolved: opened.path().to_path_buf(), + }); } - let source = read_open_workflow(&file).map_err(|source| { + let source = opened.read_to_string().map_err(|source| { WorkflowInventoryError::ReadWorkflow { path: path.clone(), source } })?; let inventory = scan_workflow(workflow_path.clone(), &source)?; @@ -409,11 +420,20 @@ fn read_workflow_sources( Ok(WorkflowSources { inventories, sources }) } -fn read_open_workflow(file: &File) -> io::Result { - let mut file = file; - let mut source = String::new(); - file.read_to_string(&mut source)?; - Ok(source) +fn map_workflow_file_error(error: OpenRepositoryFileError) -> WorkflowInventoryError { + match error { + OpenRepositoryFileError::Path { path, source } + | OpenRepositoryFileError::Identity { path, source } => { + WorkflowInventoryError::InspectWorkflow { path, source } + } + OpenRepositoryFileError::ChangedDuringOpen { path, first, second } => { + WorkflowInventoryError::WorkflowChangedDuringOpen { path, first, second } + } + OpenRepositoryFileError::OutsideRepository { path, resolved, .. } => { + WorkflowInventoryError::RedirectedWorkflowFile { path, resolved } + } + OpenRepositoryFileError::NotFile { path } => WorkflowInventoryError::NotAFile { path }, + } } fn is_workflow_path(path: &Path) -> Result { @@ -628,6 +648,26 @@ pub enum WorkflowInventoryError { /// Canonical local target which must not be followed. resolved: PathBuf, }, + /// A workflow entry was redirected after discovery or by a symbolic link. + #[error("workflow file `{path}` resolves to redirected path `{resolved}`")] + RedirectedWorkflowFile { + /// Exact repository-tree path GitHub inspects. + path: PathBuf, + /// Canonical local target which must not be followed. + resolved: PathBuf, + }, + /// A workflow entry changed between its containment and identity checks. + #[error( + "workflow file `{path}` changed while it was opened: first resolved to `{first}`, then to `{second}`" + )] + WorkflowChangedDuringOpen { + /// Exact repository-tree path GitHub inspects. + path: PathBuf, + /// Canonical destination checked before opening. + first: PathBuf, + /// Canonical destination checked after opening. + second: PathBuf, + }, /// One directory entry could not be read. #[error("failed to read an entry in workflow directory `{path}`: {source}")] ReadDirectoryEntry { @@ -854,9 +894,10 @@ mod tests { }; use super::{ - audit_workflows, compare_with_reviewed, discover_workflows, read_workflow_sources, - scan_workflow, JobId, ReviewedWorkflowJobs, WorkflowAuditError, WorkflowInventoryError, - WorkflowInventoryViolation, WorkflowJob, WorkflowPath, WORKFLOW_REGISTRY_PATH, + audit_workflows, compare_with_reviewed, discover_workflow_files, discover_workflows, + read_workflow_files, read_workflow_sources, scan_workflow, JobId, ReviewedWorkflowJobs, + WorkflowAuditError, WorkflowInventoryError, WorkflowInventoryViolation, WorkflowJob, + WorkflowPath, WORKFLOW_REGISTRY_PATH, }; static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(0); @@ -1125,7 +1166,9 @@ mod tests { let repository_root = manifest.join("../.."); let registry = repository_root.join(WORKFLOW_REGISTRY_PATH); - audit_workflows(repository_root, registry).unwrap(); + let reviewed = ReviewedWorkflowJobs::read(registry).unwrap(); + let (_, sources) = audit_workflows(repository_root, reviewed).unwrap(); + assert!(sources.source(".github/workflows/ci.yml").is_some()); } #[test] @@ -1146,7 +1189,8 @@ mod tests { ) .unwrap(); - let error = audit_workflows(&repository, ®istry).unwrap_err(); + let reviewed = ReviewedWorkflowJobs::read(®istry).unwrap(); + let error = audit_workflows(&repository, reviewed).unwrap_err(); let WorkflowAuditError::Violations(violations) = &error else { panic!("expected workflow violations, got {error:?}"); }; @@ -1305,6 +1349,41 @@ mod tests { } } + #[cfg(unix)] + #[test] + fn workflow_replacement_after_directory_inventory_cannot_redirect_the_open() { + use std::os::unix::fs::symlink; + + for target_inside_repository in [false, true] { + let temporary = temporary_directory("replaced-workflow"); + let repository = temporary.join("repository"); + let workflows = repository.join(".github/workflows"); + let candidate = workflows.join("ci.yml"); + let retained = workflows.join("retained.txt"); + let target = if target_inside_repository { + repository.join("redirected-ci.yml") + } else { + temporary.join("outside-ci.yml") + }; + fs::create_dir_all(&workflows).unwrap(); + let source = "name: CI\non:\n push:\njobs:\n test:\n runs-on: ubuntu-latest\n"; + fs::write(&candidate, source).unwrap(); + fs::write(&target, source).unwrap(); + let repository = repository.canonicalize().unwrap(); + + // Complete the same `DirEntry::file_type` observation production + // uses, then replace that known regular file before the open. + let workflow_files = discover_workflow_files(&repository).unwrap(); + fs::rename(&candidate, &retained).unwrap(); + symlink(&target, &candidate).unwrap(); + + let error = read_workflow_files(&repository, workflow_files).unwrap_err(); + assert!(matches!(error, WorkflowInventoryError::RedirectedWorkflowFile { .. })); + + fs::remove_dir_all(temporary).unwrap(); + } + } + #[cfg(unix)] #[test] fn invalid_workflow_filenames_return_escaped_typed_errors() {