diff --git a/Cargo.lock b/Cargo.lock index ad53472..5a7db39 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -37,6 +37,7 @@ version = "0.1.0" name = "conceptweave-zotero" version = "0.1.0" dependencies = [ + "libc", "serde", "serde_json", "sha2", diff --git a/crates/conceptweave-zotero/Cargo.toml b/crates/conceptweave-zotero/Cargo.toml index 3f492bb..11eb58c 100644 --- a/crates/conceptweave-zotero/Cargo.toml +++ b/crates/conceptweave-zotero/Cargo.toml @@ -8,6 +8,7 @@ license.workspace = true description = "Read-only Zotero research classification for ConceptWeave" [dependencies] +libc = "0.2" serde = { version = "1", features = ["derive"] } serde_json = "1" sha2 = "0.10" diff --git a/crates/conceptweave-zotero/src/main.rs b/crates/conceptweave-zotero/src/main.rs index 6f3825c..c12bdcd 100644 --- a/crates/conceptweave-zotero/src/main.rs +++ b/crates/conceptweave-zotero/src/main.rs @@ -1,15 +1,37 @@ #![forbid(unsafe_code)] #![cfg_attr(coverage_nightly, feature(coverage_attribute))] -use conceptweave_zotero::{build_steward_review_worksheet, read_local_snapshot}; +use conceptweave_zotero::{ + ClassificationReport, GoldenSetApproval, StewardReviewWorksheet, + build_steward_review_worksheet, read_local_snapshot, reviewed_golden_set_from_worksheet, +}; +use serde::de::DeserializeOwned; +use std::collections::BTreeSet; use std::env; use std::fs::{self, File, OpenOptions}; -use std::io::{self, BufWriter, Write}; +use std::io::{self, BufWriter, Read, Write}; use std::path::{Path, PathBuf}; -const USAGE: &str = "usage: conceptweave-zotero /tmp/REPORT.json | --worksheet /tmp/REPORT.json /tmp/WORKSHEET.json"; +const USAGE: &str = "usage: conceptweave-zotero /tmp/REPORT.json | --worksheet /tmp/REPORT.json /tmp/WORKSHEET.json | --finalize /tmp/REPORT.json /tmp/WORKSHEET.json /tmp/APPROVAL.json /tmp/GOLDEN.json"; +const MAX_ARTIFACT_BYTES: u64 = 16 * 1024 * 1024; + +#[derive(Debug, PartialEq, Eq)] +enum OutputRequest { + Report(String), + Worksheet { + report: String, + worksheet: String, + }, + Finalize { + report: String, + worksheet: String, + approval: String, + output: String, + }, +} -fn parse_output_request(args: I) -> Result<(Option, String), &'static str> +/// Parses one mutually exclusive report, worksheet, or finalization request. +fn parse_output_request(args: I) -> Result where I: IntoIterator, S: Into, @@ -26,9 +48,39 @@ where if report == worksheet { return Err("report and worksheet output paths must differ"); } - (Some(report), worksheet) + OutputRequest::Worksheet { report, worksheet } + } else if first == "--finalize" { + let report = args + .next() + .ok_or("--finalize requires four artifact paths")?; + let worksheet = args + .next() + .ok_or("--finalize requires four artifact paths")?; + let approval = args + .next() + .ok_or("--finalize requires four artifact paths")?; + let output = args + .next() + .ok_or("--finalize requires four artifact paths")?; + if BTreeSet::from([ + report.as_str(), + worksheet.as_str(), + approval.as_str(), + output.as_str(), + ]) + .len() + != 4 + { + return Err("finalization artifact paths must differ"); + } + OutputRequest::Finalize { + report, + worksheet, + approval, + output, + } } else { - (None, first) + OutputRequest::Report(first) }; if args.next().is_some() { return Err("unexpected extra argument"); @@ -36,6 +88,129 @@ where Ok(request) } +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +struct ArtifactIdentity { + device: u64, + inode: u64, +} + +/// Opens, validates, bounds, and deserializes one owner-only review artifact. +fn read_private_json(raw: &str) -> io::Result<(T, ArtifactIdentity)> { + let path = PathBuf::from(raw); + if !path.is_absolute() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "review input must be an absolute path in the system temp directory", + )); + } + let parent = path + .parent() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "review input has no parent"))?; + let resolved_parent = parent.canonicalize()?; + if !allowed_output_parents().contains(&resolved_parent) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "review input must be a direct child of the system temp directory", + )); + } + let file_name = path.file_name().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "review input has no file name") + })?; + let validated_path = resolved_parent.join(file_name); + let path_metadata = fs::symlink_metadata(&validated_path)?; + if path_metadata.file_type().is_symlink() || !path_metadata.is_file() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "review input must be a regular file", + )); + } + let (file, opened_metadata) = open_with_metadata(&validated_path)?; + #[cfg(not(unix))] + { + let _ = (path_metadata, opened_metadata, file); + return Err(io::Error::new( + io::ErrorKind::Unsupported, + "private review input requires a Unix platform", + )); + } + #[cfg(unix)] + let identity = validate_opened_identity(&path_metadata, &opened_metadata)?; + #[cfg(unix)] + { + let parsed = read_bounded_json(&mut { file }, opened_metadata.len())?; + Ok((parsed, identity)) + } +} + +#[cfg_attr(coverage_nightly, coverage(off))] +/// Opens without following a symlink or waiting for a raced FIFO, then reads handle metadata. +fn open_with_metadata(path: &Path) -> io::Result<(File, fs::Metadata)> { + #[cfg(unix)] + use std::os::unix::fs::OpenOptionsExt; + + let mut options = OpenOptions::new(); + options.read(true); + #[cfg(unix)] + options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); + let file = options.open(path)?; + let metadata = file.metadata()?; + Ok((file, metadata)) +} + +#[cfg(unix)] +/// Proves the opened Unix file matches the checked path and owner-only contract. +fn validate_opened_identity( + path_metadata: &fs::Metadata, + opened_metadata: &fs::Metadata, +) -> io::Result { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + if (opened_metadata.dev(), opened_metadata.ino()) != (path_metadata.dev(), path_metadata.ino()) + || opened_metadata.nlink() != 1 + || opened_metadata.permissions().mode() & 0o777 != 0o600 + { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "review input identity or permissions are unsafe", + )); + } + Ok(ArtifactIdentity { + device: opened_metadata.dev(), + inode: opened_metadata.ino(), + }) +} + +/// Reads bounded JSON without exposing rejected field names or values in diagnostics. +fn read_bounded_json( + reader: &mut dyn Read, + advertised_len: u64, +) -> io::Result { + if advertised_len > MAX_ARTIFACT_BYTES { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "review input exceeds the artifact size limit", + )); + } + let mut content = Vec::with_capacity(advertised_len as usize); + reader + .take(MAX_ARTIFACT_BYTES + 1) + .read_to_end(&mut content)?; + if content.len() as u64 > MAX_ARTIFACT_BYTES { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "review input grew beyond the artifact size limit", + )); + } + serde_json::from_slice(&content) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "review input is invalid")) +} + +/// Preserves an input error kind while naming the rejected artifact. +fn label_input(name: &str, error: io::Error) -> io::Error { + io::Error::new(error.kind(), format!("{name}: {error}")) +} + +/// Validates canonical report and worksheet destinations before reading Zotero. fn validate_output_request( report_output: Option<&str>, output: &str, @@ -51,16 +226,25 @@ fn validate_output_request( Ok((report_output, output)) } +/// Writes one bounded create-new private artifact; failures may leave partial files. fn write_private_output(path: &Path, content: &[u8]) -> io::Result<()> { + if content.len() as u64 > MAX_ARTIFACT_BYTES { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "metadata output exceeds the artifact size limit", + )); + } write_private_output_with(path, content, write_all_and_flush) } #[cfg_attr(coverage_nightly, coverage(off))] +/// Writes and flushes the complete serialized artifact. fn write_all_and_flush(writer: &mut BufWriter, content: &[u8]) -> io::Result<()> { writer.write_all(content)?; writer.flush() } +/// Runs the private-output boundary with an injectable writer for failure testing. fn write_private_output_with( path: &Path, content: &[u8], @@ -164,20 +348,52 @@ fn create_report_file_with( #[cfg_attr(coverage_nightly, coverage(off))] /// Reads one Zotero snapshot and writes its sensitive local proposal report. fn main() -> Result<(), Box> { - let (report_output, output) = parse_output_request(env::args().skip(1))?; - let (report_output, output) = validate_output_request(report_output.as_deref(), &output)?; - let report = read_local_snapshot()?; - if report.zotero_version.starts_with("9.") { - eprintln!("Zotero 9 Local API is read-only; writing local proposal output only"); - } - if let Some(report_output) = report_output { - let worksheet = build_steward_review_worksheet(&report)?; - let report_content = serde_json::to_vec_pretty(&report)?; - let worksheet_content = serde_json::to_vec_pretty(&worksheet)?; - write_private_output(&report_output, &report_content)?; - write_private_output(&output, &worksheet_content)?; - } else { - write_private_output(&output, &serde_json::to_vec_pretty(&report)?)?; + match parse_output_request(env::args().skip(1))? { + OutputRequest::Report(output) => { + let output = validate_output_path(&output)?; + let report = read_local_snapshot()?; + if report.zotero_version.starts_with("9.") { + eprintln!("Zotero 9 Local API is read-only; writing local proposal output only"); + } + write_private_output(&output, &serde_json::to_vec_pretty(&report)?)?; + } + OutputRequest::Worksheet { report, worksheet } => { + let (report_output, worksheet_output) = + validate_output_request(Some(&report), &worksheet)?; + let report_output = report_output.expect("worksheet request includes report path"); + let report = read_local_snapshot()?; + if report.zotero_version.starts_with("9.") { + eprintln!("Zotero 9 Local API is read-only; writing local proposal output only"); + } + let worksheet = build_steward_review_worksheet(&report)?; + let report_content = serde_json::to_vec_pretty(&report)?; + let worksheet_content = serde_json::to_vec_pretty(&worksheet)?; + write_private_output(&report_output, &report_content)?; + write_private_output(&worksheet_output, &worksheet_content)?; + } + OutputRequest::Finalize { + report, + worksheet, + approval, + output, + } => { + let output = validate_output_path(&output)?; + let (report, report_identity): (ClassificationReport, _) = + read_private_json(&report).map_err(|error| label_input("report", error))?; + let (worksheet, worksheet_identity): (StewardReviewWorksheet, _) = + read_private_json(&worksheet).map_err(|error| label_input("worksheet", error))?; + let (approval, approval_identity): (GoldenSetApproval, _) = + read_private_json(&approval).map_err(|error| label_input("approval", error))?; + if BTreeSet::from([report_identity, worksheet_identity, approval_identity]).len() != 3 { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "finalization inputs must be distinct files", + ) + .into()); + } + let golden = reviewed_golden_set_from_worksheet(&report, &worksheet, approval)?; + write_private_output(&output, &serde_json::to_vec_pretty(&golden)?)?; + } } Ok(()) } @@ -186,17 +402,77 @@ fn main() -> Result<(), Box> { mod tests { use super::*; + #[test] + #[cfg(unix)] + fn opening_private_input_refuses_a_symlink_before_reading() { + use std::os::unix::fs::symlink; + let target = unique_temp_path("open-target"); + let link = unique_temp_path("open-symlink"); + write_private_output(&target, b"{}").unwrap(); + symlink(&target, &link).unwrap(); + let result = open_with_metadata(&link); + fs::remove_file(&link).unwrap(); + fs::remove_file(&target).unwrap(); + assert!(result.is_err()); + } + + #[test] + fn private_input_rejects_a_nameless_child_of_the_validated_parent() { + let path = env::temp_dir().join(".."); + let error = read_private_json::(path.to_str().unwrap()).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + assert_eq!(error.to_string(), "review input has no file name"); + } + + #[test] + #[cfg(unix)] + fn replaced_fifo_does_not_block_before_identity_rejection() { + let target = unique_temp_path("fifo-replacement"); + let retained = unique_temp_path("fifo-retained"); + write_private_output(&target, b"{}").unwrap(); + let checked_metadata = fs::symlink_metadata(&target).unwrap(); + fs::rename(&target, &retained).unwrap(); + assert!( + std::process::Command::new("mkfifo") + .arg(&target) + .status() + .unwrap() + .success() + ); + let (sender, receiver) = std::sync::mpsc::channel(); + let opened_path = target.clone(); + std::thread::spawn(move || { + let result = open_with_metadata(&opened_path).map(|(_, metadata)| metadata); + let _ = sender.send(result); + }); + let result = receiver.recv_timeout(std::time::Duration::from_secs(2)); + fs::remove_file(&target).unwrap(); + fs::remove_file(&retained).unwrap(); + let opened_metadata = result + .expect("opening a raced FIFO must not wait for a writer") + .unwrap(); + assert_eq!( + validate_opened_identity(&checked_metadata, &opened_metadata) + .unwrap_err() + .kind(), + io::ErrorKind::PermissionDenied + ); + } + #[test] fn worksheet_mode_is_explicit_and_rejects_ambiguous_arguments() { let report = "/tmp/conceptweave-zotero-report.json"; let worksheet = "/tmp/conceptweave-zotero-worksheet.json"; assert_eq!( parse_output_request(vec!["--worksheet", report, worksheet]), - Ok((Some(report.to_owned()), worksheet.to_owned())) + Ok(OutputRequest::Worksheet { + report: report.to_owned(), + worksheet: worksheet.to_owned(), + }) ); assert_eq!( parse_output_request(vec![report]), - Ok((None, report.to_owned())) + Ok(OutputRequest::Report(report.to_owned())) ); assert_eq!(parse_output_request(Vec::<&str>::new()), Err(USAGE)); assert!(parse_output_request(vec!["--worksheet"]).is_err()); @@ -205,6 +481,258 @@ mod tests { assert!(parse_output_request(vec!["--worksheet", report, report]).is_err()); } + #[test] + fn finalization_mode_requires_four_distinct_artifact_paths() { + let report = "/tmp/report.json"; + let worksheet = "/tmp/worksheet.json"; + let approval = "/tmp/approval.json"; + let output = "/tmp/golden.json"; + assert_eq!( + parse_output_request(vec!["--finalize", report, worksheet, approval, output,]), + Ok(OutputRequest::Finalize { + report: report.to_owned(), + worksheet: worksheet.to_owned(), + approval: approval.to_owned(), + output: output.to_owned(), + }) + ); + assert!(parse_output_request(vec!["--finalize"]).is_err()); + assert!(parse_output_request(vec!["--finalize", report]).is_err()); + assert!(parse_output_request(vec!["--finalize", report, worksheet]).is_err()); + assert!(parse_output_request(vec!["--finalize", report, worksheet, approval]).is_err()); + assert!( + parse_output_request(vec!["--finalize", report, worksheet, approval, report]).is_err() + ); + assert!( + parse_output_request(vec![ + "--finalize", + report, + worksheet, + approval, + output, + "extra", + ]) + .is_err() + ); + } + + #[cfg(unix)] + #[test] + fn private_json_input_is_owner_only_regular_bounded_and_valid() { + use std::os::unix::fs::{PermissionsExt, symlink}; + + let valid = unique_temp_path("valid-input"); + let _ = fs::remove_file(&valid); + fs::write(&valid, br#"{"accepted":true}"#).unwrap(); + fs::set_permissions(&valid, fs::Permissions::from_mode(0o600)).unwrap(); + + fs::set_permissions(&valid, fs::Permissions::from_mode(0o000)).unwrap(); + assert!(read_private_json::(valid.to_str().unwrap()).is_err()); + fs::set_permissions(&valid, fs::Permissions::from_mode(0o600)).unwrap(); + let (parsed, _): (serde_json::Value, _) = + read_private_json(valid.to_str().unwrap()).unwrap(); + assert_eq!(parsed["accepted"], true); + assert!(read_private_json::("relative.json").is_err()); + assert!(read_private_json::("/").is_err()); + assert!( + read_private_json::( + unique_temp_path("missing-input").to_str().unwrap() + ) + .is_err() + ); + assert!( + read_private_json::( + "/tmp/conceptweave-zotero-missing-directory/input.json" + ) + .is_err() + ); + assert!( + read_private_json::( + env::current_dir() + .unwrap() + .join("input.json") + .to_str() + .unwrap() + ) + .is_err() + ); + + let directory = env::temp_dir().join(format!( + "conceptweave-zotero-{}-input-directory", + std::process::id() + )); + let _ = fs::remove_dir_all(&directory); + fs::create_dir(&directory).unwrap(); + assert!(read_private_json::(directory.to_str().unwrap()).is_err()); + fs::remove_dir(directory).unwrap(); + + let link = unique_temp_path("input-link"); + let _ = fs::remove_file(&link); + symlink(&valid, &link).unwrap(); + assert!(read_private_json::(link.to_str().unwrap()).is_err()); + fs::remove_file(link).unwrap(); + + fs::set_permissions(&valid, fs::Permissions::from_mode(0o644)).unwrap(); + assert!(read_private_json::(valid.to_str().unwrap()).is_err()); + fs::set_permissions(&valid, fs::Permissions::from_mode(0o600)).unwrap(); + + let hardlink = unique_temp_path("input-hardlink"); + let _ = fs::remove_file(&hardlink); + fs::hard_link(&valid, &hardlink).unwrap(); + assert!(read_private_json::(valid.to_str().unwrap()).is_err()); + fs::remove_file(hardlink).unwrap(); + + fs::write(&valid, b"not-json").unwrap(); + assert!(read_private_json::(valid.to_str().unwrap()).is_err()); + fs::remove_file(valid).unwrap(); + + let oversized = unique_temp_path("oversized-input"); + let _ = fs::remove_file(&oversized); + let file = File::create(&oversized).unwrap(); + file.set_len(MAX_ARTIFACT_BYTES + 1).unwrap(); + fs::set_permissions(&oversized, fs::Permissions::from_mode(0o600)).unwrap(); + assert!(read_private_json::(oversized.to_str().unwrap()).is_err()); + fs::remove_file(oversized).unwrap(); + + let identity_left = unique_temp_path("identity-left"); + let identity_right = unique_temp_path("identity-right"); + fs::write(&identity_left, b"{}").unwrap(); + fs::write(&identity_right, b"{}").unwrap(); + fs::set_permissions(&identity_left, fs::Permissions::from_mode(0o600)).unwrap(); + fs::set_permissions(&identity_right, fs::Permissions::from_mode(0o600)).unwrap(); + assert!( + validate_opened_identity( + &fs::metadata(&identity_left).unwrap(), + &fs::metadata(&identity_right).unwrap() + ) + .is_err() + ); + fs::remove_file(identity_left).unwrap(); + fs::remove_file(identity_right).unwrap(); + } + + #[test] + fn bounded_json_reader_and_input_labels_cover_failures() { + struct FailingReader; + impl Read for FailingReader { + fn read(&mut self, _: &mut [u8]) -> io::Result { + Err(io::Error::other("injected read failure")) + } + } + + let oversized = + read_bounded_json::(&mut io::empty(), MAX_ARTIFACT_BYTES + 1) + .unwrap_err(); + assert_eq!(oversized.kind(), io::ErrorKind::InvalidData); + assert_eq!( + oversized.to_string(), + "review input exceeds the artifact size limit" + ); + + let grown = read_bounded_json::( + &mut io::repeat(b' ').take(MAX_ARTIFACT_BYTES + 1), + 0, + ) + .unwrap_err(); + assert_eq!(grown.kind(), io::ErrorKind::InvalidData); + assert_eq!( + grown.to_string(), + "review input grew beyond the artifact size limit" + ); + + let read_failure = + read_bounded_json::(&mut FailingReader, 0).unwrap_err(); + assert_eq!(read_failure.kind(), io::ErrorKind::Other); + assert_eq!(read_failure.to_string(), "injected read failure"); + + let labeled = label_input( + "worksheet", + io::Error::new(io::ErrorKind::PermissionDenied, "unsafe"), + ); + assert_eq!(labeled.kind(), io::ErrorKind::PermissionDenied); + assert_eq!(labeled.to_string(), "worksheet: unsafe"); + } + + #[test] + fn private_json_diagnostic_hides_unknown_field_names() { + #[derive(Debug, serde::Deserialize)] + #[serde(deny_unknown_fields)] + struct StrictReviewInput {} + + // Original metadata types permit additional fields. This strict test-only type + // exercises the shared reader's confidentiality contract for future callers. + let error = read_bounded_json::( + &mut br#"{"synthetic-private-field-sentinel":true}"#.as_slice(), + 0, + ) + .unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + assert_eq!(error.to_string(), "review input is invalid"); + assert_eq!( + label_input("worksheet", error).to_string(), + "worksheet: review input is invalid" + ); + } + + #[test] + fn private_json_diagnostic_hides_rejected_enum_values() { + let error = read_bounded_json::( + &mut br#""synthetic-private-enum-sentinel""#.as_slice(), + 0, + ) + .unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + assert_eq!(error.to_string(), "review input is invalid"); + } + + #[cfg(unix)] + #[test] + fn private_output_limit_accepts_exactly_the_readable_artifact_boundary() { + use std::os::unix::fs::PermissionsExt; + + let output = unique_temp_path("exact-output-limit"); + assert!(!output.exists()); + let mut content = vec![b' '; MAX_ARTIFACT_BYTES as usize]; + content[..2].copy_from_slice(b"{}"); + write_private_output(&output, &content).unwrap(); + let metadata = fs::metadata(&output).unwrap(); + let restored = read_private_json::(output.to_str().unwrap()); + fs::remove_file(output).unwrap(); + assert_eq!(metadata.len(), MAX_ARTIFACT_BYTES); + assert_eq!(metadata.permissions().mode() & 0o777, 0o600); + assert_eq!(restored.unwrap().0, serde_json::json!({})); + } + + #[cfg(unix)] + #[test] + fn private_output_limit_rejects_oversize_before_creating_or_touching_a_file() { + let output = unique_temp_path("oversized-output-limit"); + let existing = unique_temp_path("existing-output-limit"); + assert!(!output.exists()); + assert!(!existing.exists()); + write_private_output(&existing, b"original").unwrap(); + let content = vec![b' '; MAX_ARTIFACT_BYTES as usize + 1]; + let rejected = write_private_output(&output, &content); + let touched = write_private_output(&existing, &content); + let created = output.exists(); + let preserved = fs::read(&existing).unwrap(); + // Clean synthetic artifacts even when the RED implementation creates the file. + let _ = fs::remove_file(output); + fs::remove_file(existing).unwrap(); + assert!( + !created, + "oversized metadata must be rejected before creation" + ); + assert_eq!(preserved, b"original"); + for error in [rejected.unwrap_err(), touched.unwrap_err()] { + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + assert_eq!( + error.to_string(), + "metadata output exceeds the artifact size limit" + ); + } + } + #[test] fn canonical_output_aliases_are_rejected_without_creating_files() { let output = diff --git a/crates/conceptweave-zotero/tests/finalization_artifact_identity.rs b/crates/conceptweave-zotero/tests/finalization_artifact_identity.rs new file mode 100644 index 0000000..75de1ef --- /dev/null +++ b/crates/conceptweave-zotero/tests/finalization_artifact_identity.rs @@ -0,0 +1,101 @@ +#![cfg(unix)] + +use conceptweave_zotero::{ + Disposition, GoldenSetApproval, ItemData, ZoteroItem, build_steward_review_worksheet, + classify_snapshot, +}; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::process::Command; + +#[test] +fn finalization_rejects_distinct_path_spellings_for_one_input_file() { + let item = ZoteroItem { + source_record: None, + key: "ITEM".into(), + version: 7, + data: ItemData { + item_type: "journalArticle".into(), + title: "ontology alignment".into(), + abstract_note: "review context".into(), + doi: "10.1000/example".into(), + parent_item: String::new(), + collections: vec!["COLLECTION".into()], + tags: vec![], + }, + }; + let report = classify_snapshot("9.0.6".into(), None, 42, vec![item]); + let mut worksheet = build_steward_review_worksheet(&report).unwrap(); + for decision in &mut worksheet.decisions { + decision.reviewed_disposition = Some(Disposition::OutOfScope); + } + let approval = GoldenSetApproval { + receipt_id: "receipt_1".into(), + reviewer_subject: "steward_1".into(), + library_version: worksheet.library_version, + rule_revision: worksheet.rule_revision.clone(), + snapshot_digest: worksheet.snapshot_digest.clone(), + proposal_digest: conceptweave_zotero::classification_proposal_digest(&report), + snapshot_items: worksheet.snapshot_items.clone(), + }; + + // These structs intentionally accept additional owner-only fields. Without checking file + // identity, one JSON object can therefore be accepted as all three finalization inputs. + let mut combined = serde_json::to_value(&report).unwrap(); + let object = combined.as_object_mut().unwrap(); + object.insert( + "decisions".into(), + serde_json::to_value(&worksheet.decisions).unwrap(), + ); + object.insert( + "receipt_id".into(), + serde_json::to_value(&approval.receipt_id).unwrap(), + ); + object.insert( + "reviewer_subject".into(), + serde_json::to_value(&approval.reviewer_subject).unwrap(), + ); + object.insert( + "proposal_digest".into(), + serde_json::to_value(&approval.proposal_digest).unwrap(), + ); + + let temp = std::env::temp_dir().canonicalize().unwrap(); + let filename = format!( + "conceptweave-zotero-finalize-alias-{}-input.json", + std::process::id() + ); + let input = temp.join(&filename); + let output = temp.join(format!( + "conceptweave-zotero-finalize-alias-{}-output.json", + std::process::id() + )); + let _ = fs::remove_file(&input); + let _ = fs::remove_file(&output); + fs::write(&input, serde_json::to_vec(&combined).unwrap()).unwrap(); + fs::set_permissions(&input, fs::Permissions::from_mode(0o600)).unwrap(); + + let report_path = input.to_str().unwrap().to_owned(); + let worksheet_path = format!("{}/./{}", temp.display(), filename); + let approval_path = format!("{}//{}", temp.display(), filename); + assert_ne!(report_path, worksheet_path); + assert_ne!(worksheet_path, approval_path); + + let status = Command::new(env!("CARGO_BIN_EXE_conceptweave-zotero")) + .args([ + "--finalize", + &report_path, + &worksheet_path, + &approval_path, + output.to_str().unwrap(), + ]) + .status() + .unwrap(); + + let _ = fs::remove_file(&input); + let _ = fs::remove_file(&output); + assert!( + !status.success(), + "finalization must reject three path spellings that resolve to one input artifact" + ); +} diff --git a/crates/conceptweave-zotero/tests/private_json_diagnostics_cli.rs b/crates/conceptweave-zotero/tests/private_json_diagnostics_cli.rs new file mode 100644 index 0000000..6a25e02 --- /dev/null +++ b/crates/conceptweave-zotero/tests/private_json_diagnostics_cli.rs @@ -0,0 +1,146 @@ +#![cfg(unix)] + +use conceptweave_zotero::{ + Disposition, GoldenSetApproval, ItemData, ReviewedGoldenSet, ZoteroItem, + build_steward_review_worksheet, classification_proposal_digest, classify_snapshot, +}; +use std::fs::{self, OpenOptions}; +use std::io::Write; +use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +use std::path::PathBuf; +use std::process::{Command, Output}; + +/// Owns only synthetic, uniquely named direct-temp-child artifacts for one CLI case. +struct FinalizationFiles { + paths: [PathBuf; 4], + inputs: [Vec; 3], +} + +impl FinalizationFiles { + fn new(case_name: &str, mutate: impl FnOnce(&mut [serde_json::Value; 3])) -> Self { + let report = classify_snapshot( + "9.0.6".into(), + None, + 42, + vec![ZoteroItem { + source_record: None, + key: "SYNTHETIC".into(), + version: 7, + data: ItemData { + item_type: "journalArticle".into(), + title: "synthetic ontology alignment".into(), + abstract_note: String::new(), + doi: String::new(), + parent_item: String::new(), + collections: vec![], + tags: vec![], + }, + }], + ); + let mut worksheet = build_steward_review_worksheet(&report).unwrap(); + worksheet.decisions[0].reviewed_disposition = Some(Disposition::AlignmentVersioning); + let approval = GoldenSetApproval { + receipt_id: "synthetic-receipt".into(), + reviewer_subject: "synthetic-steward".into(), + library_version: worksheet.library_version, + rule_revision: worksheet.rule_revision.clone(), + snapshot_digest: worksheet.snapshot_digest.clone(), + proposal_digest: classification_proposal_digest(&report), + snapshot_items: worksheet.snapshot_items.clone(), + }; + let mut values = [ + serde_json::to_value(report).unwrap(), + serde_json::to_value(worksheet).unwrap(), + serde_json::to_value(approval).unwrap(), + ]; + mutate(&mut values); + let inputs = values.map(|value| serde_json::to_vec(&value).unwrap()); + let paths = ["report", "worksheet", "approval", "golden"].map(|role| { + std::env::temp_dir().join(format!( + "conceptweave-pr29-diagnostics-{}-{case_name}-{role}.json", + std::process::id() + )) + }); + assert!(!paths[3].exists()); + for (path, bytes) in paths.iter().zip(&inputs) { + let mut file = OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + .unwrap(); + file.set_permissions(fs::Permissions::from_mode(0o600)) + .unwrap(); + file.write_all(bytes).unwrap(); + } + Self { paths, inputs } + } + + fn run(&self) -> Output { + Command::new(env!("CARGO_BIN_EXE_conceptweave-zotero")) + .arg("--finalize") + .args(&self.paths) + .output() + .unwrap() + } + + fn assert_private_rejection(&self, role: &str) { + let output = self.run(); + assert!(!output.status.success()); + assert!(output.stdout.is_empty()); + assert!(!self.paths[3].exists()); + for (path, original) in self.paths.iter().zip(&self.inputs) { + assert_eq!(&fs::read(path).unwrap(), original); + } + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + stderr.contains(&format!("{role}: review input is invalid")), + "{stderr}" + ); + assert!(!stderr.contains("synthetic-private"), "{stderr}"); + } +} + +impl Drop for FinalizationFiles { + fn drop(&mut self) { + for path in &self.paths { + let _ = fs::remove_file(path); + } + } +} + +#[test] +fn finalization_keeps_valid_synthetic_inputs_and_private_output_compatible() { + let files = FinalizationFiles::new("valid", |_| {}); + let output = files.run(); + assert!(output.status.success(), "{:?}", output.stderr); + let golden: ReviewedGoldenSet = + serde_json::from_slice(&fs::read(&files.paths[3]).unwrap()).unwrap(); + let approval: GoldenSetApproval = serde_json::from_slice(&files.inputs[2]).unwrap(); + assert_eq!(golden.approval, approval); + assert_eq!( + golden.labels[0].expected_disposition, + Disposition::AlignmentVersioning + ); + assert_eq!( + fs::metadata(&files.paths[3]).unwrap().permissions().mode() & 0o777, + 0o600 + ); +} + +#[test] +fn finalization_diagnostic_hides_private_worksheet_enum_value() { + let files = FinalizationFiles::new("enum", |values| { + values[1]["decisions"][0]["reviewed_disposition"] = + serde_json::json!("synthetic-private-enum-sentinel"); + }); + files.assert_private_rejection("worksheet"); +} + +#[test] +fn finalization_diagnostic_hides_private_approval_scalar_value() { + let files = FinalizationFiles::new("scalar", |values| { + values[2]["library_version"] = serde_json::json!("synthetic-private-scalar-sentinel"); + }); + files.assert_private_rejection("approval"); +} diff --git a/docs/PRD.md b/docs/PRD.md index 1f51bb5..150f7c6 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -88,6 +88,7 @@ The Zotero 10+ adapter can accept a caller-owned API key and server identity at A local steward worksheet must bind the library version, rule revision, complete raw-snapshot digest, current proposal-and-retained-source digest, every observed parent/child item revision, and one blank decision slot per classified bibliographic item. It repeats item identity, proposal, and abstention reason only; titles, abstracts, tags, collections, and matched evidence remain in the separate sensitive report. Shared inventory validation rejects omitted source records, hidden pending relationships and inconsistent identity before construction. Valid unresolved sources do not prevent starting review, but prevent claiming completion. Old worksheets without the content binding require regeneration, never automatic approval backfill. After every decision is filled, worksheet finalization must verify the governance receipt coordinates, unique item identities and revisions, proposal/abstention consistency, and non-abstention truth labels before producing a reviewed golden set. Missing decisions remain incomplete and cannot reach external approval verification. +Operators must be able to finalize the saved report, completed worksheet, and approval receipt offline without rereading mutable Zotero state. Every input must be a distinct owner-only file identity, not merely a differently spelled path, and the new golden-set output must use a separate path; invalid, oversized, linked, or shared inputs fail closed. The worksheet's own required content identity must match the current report independently of the supplied receipt. Blank identity is invalid; a stale or replaced identity is a snapshot mismatch. Conversion only prepares input for independent verification. Unresolved sources can remain in locally prepared review data, but prevent whole-library completion; refreshing local digests cannot renew an independently issued approval. Evaluate classifier quality only against a steward-reviewed local golden set whose governance receipt is externally verified and binds both the complete source/classifier-input snapshot and every current proposal field, in addition to the item-key/item-version coordinates. Same-version changes to unmodeled provider metadata, absent/default fields, classifier inputs, predictions or supporting evidence must invalidate the corresponding binding. Evaluation recomputes proposal identity before contacting governance; a locally changed digest cannot renew an approval. Legacy unbound approvals require reissuance, never automatic backfill. Abstention is a prediction outcome, never an approved truth label. Evaluation emits the verified library revision, rule revision, opaque snapshot and proposal digests, and aggregate counts for exact matches, abstentions, and per-disposition true-positive/predicted/expected totals; it must not copy Zotero keys, reviewer identity, or bibliographic text into the result. diff --git a/docs/TRD.md b/docs/TRD.md index 5b6c4bd..03e14cb 100644 --- a/docs/TRD.md +++ b/docs/TRD.md @@ -115,6 +115,9 @@ The review worksheet is a deterministic item-key-ordered projection of the repor The finalization function consumes the filled worksheet plus a governance approval receipt. It rejects blank authority metadata, missing or mismatched proposal bindings, coordinate drift, duplicate or unknown decision keys, missing decisions, abstention as approved truth, and tampered proposal/reason pairs. It recomputes the complete current proposal digest, including title and evidence fields omitted from the worksheet, and compares it with the supplied approval without replacing that approval. Its output reuses the existing reviewed-golden-set evaluator; finalization itself does not verify external authority. It also rejects a blank worksheet `proposal_digest` as `InvalidReview` and compares that field with the freshly built expected worksheet as `SnapshotMismatch`, retaining existing cardinality and approval error precedence. The converter does not accept an updated receipt as proof that old decisions reviewed changed content. A caller can construct self-consistent unverified data, so the evaluator still authenticates the entire reviewed set against independent evidence. Pending sources are admitted for preparation but rejected by complete evaluation before governance is contacted. Later extracted worksheet validators must preserve this comparison. The owner-only report uses owned JSON values and supports lossless deserialization of its defined metadata projection, not the original provider record or full-text capture. Unknown provider fields and raw capture bytes are not serialized; retain captures separately. Repeated serialize/deserialize roundtrips preserve report bytes, the report-derived worksheet and proposal digest, allowing offline finalization against the stored snapshot identity rather than another live Zotero read. Shared report validation binds every retained key, version and parent coordinate; classified sources are top-level, while valid unresolved orphan and cyclic metadata remains pending. Deserialization establishes structure, not approval: changing serialized title or evidence under an unchanged source digest must still fail both finalization and evaluation against the original receipt before external verification. +`conceptweave-zotero --finalize /tmp/REPORT.json /tmp/WORKSHEET.json /tmp/APPROVAL.json /tmp/GOLDEN.json` performs that offline transition. All four path arguments must differ, and the three opened inputs must also have distinct Unix device/inode identities so alternate spellings cannot collapse artifacts. Inputs must be regular, single-link files with exact `0600` permissions, no larger than 16 MiB, and direct children of a canonical system temporary directory; the golden set uses the existing create-new `0600` output boundary. Finalization reads no Zotero state and applies the existing report-bound worksheet and approval validation before writing. + +Offline input admission pins the checked canonical parent plus file name for metadata and opening. Unix opening refuses final-component symlinks with `O_NOFOLLOW` and uses `O_NONBLOCK` so a raced FIFO cannot wait for a writer before device/inode validation rejects the replacement; a pre-open pathname check alone is insufficient. Regular-file reads remain bounded as before. Paired export retains the shared canonical-destination check before capture and serializes both artifacts before writing. Failures may leave private partial files and never trigger pathname cleanup or implicit buffer-flush retry. Finalized metadata remains unverified until the independent whole-set approval boundary succeeds. The local report can contain titles, tags, matched metadata, and abstention abstracts. It is sensitive steward-review material, remains outside the repository, and is not a publication artifact. diff --git a/docs/UML.md b/docs/UML.md index 9a03998..7b3ac2b 100644 --- a/docs/UML.md +++ b/docs/UML.md @@ -59,7 +59,9 @@ sequenceDiagram Note over Report,Steward: Pending sources prevent a whole-library completion claim; inventory is not approval Intake->>Report: derive snapshot-bound decision worksheet without bibliographic text Report->>Steward: review dispositions and merge candidates - Steward->>Intake: verified labels for every bibliographic item + Steward->>Intake: completed worksheet + approval receipt + Intake->>Report: offline finalization against the original saved report + Report-->>Steward: reviewed golden set or fail-closed validation error Steward->>Intake: reviewed labels and independently issued receipt Intake->>Intake: validate complete partitions and recompute pending ancestry Intake->>Intake: verify v2 proposal and retained-source binding diff --git a/docs/adr/0006-zotero-research-intake.md b/docs/adr/0006-zotero-research-intake.md index 6aa2f86..3c4d72a 100644 --- a/docs/adr/0006-zotero-research-intake.md +++ b/docs/adr/0006-zotero-research-intake.md @@ -42,6 +42,18 @@ Proposed worksheet source-scope amendment: in the context of continuing review a Older worksheets require regeneration, not a fabricated digest or approval. The later progress, patch and finalization owners must compare the worksheet's digest against the report, including rejecting blank or locally rewritten values, before external approval verification. Those later consumers have not yet adopted this field at this checkpoint. A valid hash proves identity rather than authorship or authorization, and no metadata worksheet downcast grants full-text or Zotero write authority. Keep this amendment Proposed until protected owner/consumer and release evidence is available; the Gap baseline records exact tests and remaining live work. +The CLI finalizes an original report, completed worksheet, and approval receipt into the reviewed golden set without another Zotero read. Each path argument and each opened input device/inode identity must be distinct. Inputs remain direct temporary-directory children, regular single-link files, exact owner-only `0600`, and bounded to 16 MiB; output retains create-new `0600` semantics. This keeps sensitive review material local and makes snapshot drift or aliased artifacts a validation failure instead of silently substituting current library state. + +### Proposed offline input continuity amendment — September 7 + +PR29 normal merge `4845200` retains offline finalization and inherits PR28 source binding plus report/worksheet preservation. Worksheet destinations reuse canonical-pair validation before capture; both artifacts serialize before either write, and second-write failure does not unlink the first artifact. This is sequential local output, not an atomic pair or approval issuance. + +The original reader canonicalized a parent for admission but subsequently opened the original pathname; the open helper also followed final-component symlinks before checking device/inode. RED `7c2c0d1` proves that direct symlink opening succeeds and records the existing nameless-input error precedence; the latter is a validation-path regression, not proof of unauthorized data access. We reuse canonical owner fixes `b06f54a53f457ee002dffccdb6f18e66fef0c3c7` and `7ccbbbe6857113d3d9fb5fd102ce9c9839b7975b` as `f97ef46` and `630d258`, preserving their attribution. The reader uses the checked canonical parent plus file name for metadata and opening, and Unix opening uses `O_NOFOLLOW` before handle-based identity checks. The existing locked libc dependency supplies the native flag; no replacement filesystem abstraction is added. + +We reject relying only on a pre-open symlink check because a pathname can change between inspection and opening. We reject copying the later full-text CLI into this earlier owner: this command finalizes metadata only and grants neither full-text decision provenance nor external approval nor Zotero write authority. OS open failure remains an error; existing private-mode, single-link, byte-limit and source-binding checks remain required. These changes remain Proposed until exact-head protected checks, independent approval and release. + +The follow-up premortem identified a separate FIFO replacement: `O_NOFOLLOW` does not prevent an open from waiting for a pipe writer before identity validation. No existing owner fix was found. RED `bce7efa` replaces a checked regular unit-test artifact with an actual FIFO and observes a two-second test timeout. `ddf3f62` adds the existing native `O_NONBLOCK` flag at the same open boundary; the open returns without a writer and existing device/inode validation rejects the replacement. This is a filesystem-open safety rule, not a model/application timeout. Regular-file private reads and size limits remain unchanged. Root and later extracted private readers must inherit it; do not transfer this head's test evidence to them. + The reader fails closed above 50,000 items or 256 MiB of cumulative response bodies, while retaining the 8 MiB per-page bound, finite request timeouts, redirect denial, total-count checks, snapshot-version checks, and duplicate-key detection. Pagination, consistency, resource-budget, and provider-contract behavior are separated from the narrow `ureq` transport so deterministic tests exercise the production reader core rather than excluding the entire reader from coverage. In the context of reading every bibliographic source before classification, facing individually timely pages that can cumulatively hold a run open for days, we decided for a five-minute monotonic admission/completion budget in the existing reader and against rejecting legitimate short pages or adding another transport, to bound accepted work without excluding papers, accepting that an already-started request or classification computation can finish after the limit before its result is rejected. This is an application read limit, not a model timeout, hard process-cancellation deadline, wall-clock/suspend guarantee or atomic snapshot claim. Each page is checked before fetch and after return, and the complete report is checked before return. The stdlib clock has a private deterministic test seam; public APIs, provider timeouts and data/byte ceilings are unchanged. The [deadline doctoring](../doctoring/zotero_metadata_deadline.md) records the original review, RED/GREEN, alternatives and exact verification. This amendment remains Proposed and grants no Zotero mutation authority. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index a695640..92a62a8 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -6,6 +6,16 @@ This file records code-current product and technical gaps. Exact PR/check/run co ## September 6 source inventory checkpoint +### September 7 PR29 offline finalization continuity repair + +Original PR29 `f73705e15f1236fa8bd34fec032bc78d9b57760c` passed 149 tests/28 suites. Normal merge `4845200` retains it and repaired PR28 `63eb0f116408372675f132b9836fe7be4bdd7134`; integrated tests passed 190/28. Offline finalization remains local unverified metadata output. The request enum retains parent canonical-pair admission before capture, both serializations before writes and no pathname cleanup on failure. Complete source/worksheet/approval binding comes from the inherited shared validation, not a new authority issuer. + +RED `7c2c0d1` compiled with two failures: direct symlink opening succeeded; nameless-input validation took the old error path. The second is not an unauthorized-read claim. Canonical fixes `b06f54a` and `7ccbbbe` were reused with attribution as `f97ef46` and `630d258`, pinning the admitted parent and refusing symlink opens. A separate review found no existing FIFO repair. RED `bce7efa` replaced a checked unit-test regular file with an actual FIFO and timed out after 2.01 seconds. `ddf3f62` adds `O_NONBLOCK` at the existing opening boundary; existing device/inode validation then rejects the replacement without waiting for a writer. Independent bounded read-only review found no additional defect. No new abstraction or unpinned dependency was introduced. + +Final source `ddf3f62` passes 193 tests/28 suites including three doctests, strict Clippy, warnings-denied rustdoc, formatting, CI contract and diff checks. Unchanged coverage passes 318/318 reported functions, 2,778/2,778 normalized regions and 494/494 normalized branches. Raw 3,798/3,860 lines, 5,873/5,980 regions and 449/494 branches are not 100%. Logs: `/tmp/conceptweave-pr29-{baseline,integrated,opening-red,fifo-red,verified,clippy-verified,rustdoc-verified,coverage-verified}.log`. TRD and Proposed ADR0006 trace alternatives, retained failures and consumer obligations. + +Root/later private readers must inherit FIFO protection and the source-scope cascade. Authentic decisions and independent approvals remain 0/3,715 plus four unresolved standalone sources. Unit fixtures do not count as real research or approval. Native Visual Inspection was retried and the Mac remains locked, so no fresh screen evidence exists. Keep Draft; no hosted GREEN, protected merge, release or Zotero mutation is claimed. + ### September 7 PR28 metadata roundtrip continuity repair Normal merge `13b5529` preserves original PR28 `ba6b3dfc71cf89ed4c57b85da0dd9ca5f983efee` and PR27 `fd5ef23c1c23fa36ef106400e15c9438eaa5cd41`. RED `97fc490` compiled with four passing and two failing roundtrip tests: inconsistent retained parent coordinates passed shared admission, while valid pending orphan metadata failed worksheet restoration. `1157b1d` centralizes parent validation and removes divergent worksheet checks; `e102894` removes an implied evaluator guard and preserves coherent-mutation stale-approval rejection. Independent read-only review found no further production defect in the bounded repair.