diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 157345f..d8b5466 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -2634,6 +2634,17 @@ partial clone — is reported as a surface this run could not read, and so is ex `2`. It is not skipped: an object the audit could not open is not an object the audit found clean. +Each blob is decoded the way the guards decode it: a byte-order mark is +honoured, so a UTF-16 file is searched as the text it holds rather than as +replacement characters. A blob that is neither text nor binary — Latin-1, or a +UTF-16 mark over bytes that do not decode as UTF-16 — is reported as a surface +this run could not read, is left out of the count of surfaces read, and so is +exit `2`. It is still searched for what its bytes spell in ASCII, so a name +written in it is reported too, and that finding makes the exit `1`: a violation +outranks an unread surface, and both are printed. A binary blob is still +searched, where the guards skip it, because a name written into one as plain +bytes is published by the flip all the same. + The edit history is a **standing caveat**, not an unreadable surface. It is true of every run, on every repository, and nothing about this run could change it — so it is stated in the body of every report and is *not* counted as something diff --git a/src/audit.rs b/src/audit.rs index e54ef5a..ecaee54 100644 --- a/src/audit.rs +++ b/src/audit.rs @@ -47,6 +47,7 @@ use std::path::Path; use crate::config::{CheckKind, Policy, Rule}; use crate::error::{Exit, Fatal, Result, verdict}; use crate::git; +use crate::guard::scope::{self, Decoded}; use crate::guard::{Refusal, names}; /// True of every run of this subcommand, on every repository, forever. @@ -67,6 +68,11 @@ const STANDING_CAVEATS: &[&str] = &[ struct Surface { label: String, text: String, + /// Whether the text is what the surface says. False for a blob whose bytes + /// would not decode: it is still searched, for whatever its bytes spell in + /// ASCII, but it is listed as unread and not counted among the surfaces + /// read, because nobody read the rest of it. + read: bool, } /// The rule to judge with, forced to the visibility being flipped TO. @@ -178,6 +184,7 @@ fn commit_surfaces(listed: &str, what: &str) -> Vec { .map(|(sha, body)| Surface { label: format!("{what} {}", &sha[..sha.len().min(8)]), text: body.to_owned(), + read: true, }) .collect() } @@ -371,6 +378,7 @@ fn read_conversation( Ok(text) => surfaces.push(Surface { label: format!("{kind} #{number} title, body and comments"), text, + read: true, }), Err(reason) => unreadable.push(format!("{kind} #{number} could not be read: {reason}")), } @@ -392,6 +400,7 @@ fn read_conversation( Ok(text) => surfaces.push(Surface { label: format!("pr #{number} review bodies"), text, + read: true, }), Err(reason) => unreadable.push(format!( "pr #{number} review bodies could not be read: {reason}" @@ -406,6 +415,7 @@ fn read_conversation( Ok(text) => surfaces.push(Surface { label: format!("pr #{number} review-thread comments"), text, + read: true, }), Err(reason) => unreadable.push(format!( "pr #{number} review-thread comments could not be read: {reason}" @@ -529,13 +539,42 @@ fn reachable_blobs(root: &Path) -> Result<(Vec, Vec)> { let absent = git::each_blob(root, &shas, |sha, bytes| { read += 1; progress(read, total); + let path = paths.get(sha).map_or("?", String::as_str); + // Through the one decoder the guards read the same blobs with. This was + // `String::from_utf8_lossy`, which turned a UTF-16 file into replacement + // characters with NULs between them: `github.com/acme/secret` inside one + // matched nothing, the blob was counted among the surfaces read, and a + // run with a reachable forge ended on "every one of them is clean", exit + // 0, over a name the same text in UTF-8 is refused for -- immediately + // before the flip that cannot be taken back. + let (text, decoded) = match scope::decode(bytes) { + Decoded::Text(text) => (text, true), + // Still read, and read the way this audit always read it. The + // guards skip a binary blob because it has no lines to point at; + // this audit has no lines to point at either, and a name written + // into a binary as plain bytes is served by the flip all the same. + // Narrowing to the guards' reading would turn a finding this made + // into silence. + Decoded::Binary => (String::from_utf8_lossy(bytes).into_owned(), true), + // Two facts, and both are reported. The blob is not a surface read + // clean, so it goes to the list of what this run failed to open and + // is not counted as read. And it is still searched as it always + // was, because a lossy reading keeps every ASCII byte: a Latin-1 + // file with `github.com/acme/secret` in it was a finding before + // this decoder existed, and recording the file as unread alone + // would turn that finding into a line saying nothing about it. + Decoded::Unreadable(why) => { + unreadable.push(format!( + "{path} (blob {sha}) is reachable and could not be read as text ({why}); \ + only what its bytes spell in ASCII was searched" + )); + (String::from_utf8_lossy(bytes).into_owned(), false) + } + }; surfaces.push(Surface { - label: format!( - "{} (blob {})", - paths.get(sha).map_or("?", String::as_str), - &sha[..8.min(sha.len())] - ), - text: String::from_utf8_lossy(bytes).into_owned(), + label: format!("{path} (blob {})", &sha[..8.min(sha.len())]), + text, + read: decoded, }); })?; // An object the audit could not open is not an object the audit found clean. @@ -695,7 +734,18 @@ pub(crate) fn for_publication(root: &Path, policy: &Policy) -> Result { } } - println!("{} surface(s) read", surfaces.len()); + // Counted by what was read, not by what was searched: a blob that would not + // decode was searched for what it spells in ASCII and is listed as unread + // below, and a total that included it would claim a reading nobody did. + let read = surfaces.iter().filter(|surface| surface.read).count(); + println!("{read} surface(s) read"); + let partly = surfaces.len().saturating_sub(read); + if partly > 0 { + println!( + "{partly} more searched only for what their bytes spell in ASCII, and listed as \ + unread below" + ); + } for refusal in &refusals { eprintln!("would be republished: {}", refusal.report.trim_end()); eprintln!(); diff --git a/tests/audit_publication_cli.rs b/tests/audit_publication_cli.rs index c18fcd9..b4b74d9 100644 --- a/tests/audit_publication_cli.rs +++ b/tests/audit_publication_cli.rs @@ -730,3 +730,139 @@ fn a_commit_only_on_a_retained_pull_ref_is_read() { assert!(report.contains("retained pull ref"), "{report}"); let _ = std::fs::remove_dir_all(&origin); } + +/// UTF-16 bytes as a file on disk holds them, byte-order mark first. +fn utf16le(text: &str) -> Vec { + let mut bytes = vec![0xFF, 0xFE]; + for unit in text.encode_utf16() { + bytes.extend_from_slice(&unit.to_le_bytes()); + } + bytes +} + +/// A private name in a UTF-16 file is found. +/// +/// The blobs were read with `String::from_utf8_lossy`, which makes a UTF-16 +/// file replacement characters with NULs between them. The name inside matched +/// nothing and the blob was still counted as read, so with a forge that +/// answered, this fixture ended on "every one of them is clean" and exit 0 -- +/// the report the flip is taken on. +#[test] +fn a_name_in_a_utf16_file_is_found() { + let root = repository(); + let origin = origin_for(&root, "publication-utf16-origin"); + std::fs::write( + root.join("NOTES.txt"), + utf16le("we hit this in PrivateOrg first\n"), + ) + .unwrap(); + support::git(&root, &["add", "-A"]); + support::git(&root, &["commit", "-qm", "notes", "--no-verify"]); + support::git(&root, &["push", "-q", "origin", "main"]); + + // A stub `gh`, for the reason `audit_with_gh` gives, and so that the forge + // half reads clean: what is left to decide the exit is the blob. + let output = audit_with_gh(&root, "exit 0\n"); + let report = text(&output); + assert_eq!( + code(&output), + 1, + "the name is in the file, written in UTF-16:\n{report}" + ); + assert!(report.contains("NOTES.txt (blob "), "{report}"); + let _ = std::fs::remove_dir_all(&origin); +} + +/// A blob that is neither text nor binary is a surface this run did not read. +/// +/// Latin-1 is the ordinary case: one byte that is not UTF-8 and no NUL to call +/// the file an image. Read lossily it was counted among the surfaces read and +/// the run could exit 0, a coverage claim over text nobody decoded. +#[test] +fn a_blob_that_does_not_decode_is_reported_unread() { + let root = repository(); + let origin = origin_for(&root, "publication-undecodable-origin"); + std::fs::write(root.join("latin1.txt"), b"caf\xe9 au lait\n").unwrap(); + support::git(&root, &["add", "-A"]); + support::git(&root, &["commit", "-qm", "one", "--no-verify"]); + support::git(&root, &["push", "-q", "origin", "main"]); + + let output = audit_with_gh(&root, "exit 0\n"); + let report = text(&output); + assert_eq!( + code(&output), + 2, + "a blob nobody could decode is not a blob found clean:\n{report}" + ); + let (_, measured) = report.split_once("could NOT be read:").unwrap(); + assert!(measured.contains("latin1.txt (blob "), "{report}"); + assert!( + !report.contains("every surface a flip would republish"), + "{report}" + ); + // Searched, but not counted as read: the total names only what was. + assert!( + report.contains("1 more searched only for what their bytes spell in ASCII"), + "{report}" + ); + let _ = std::fs::remove_dir_all(&origin); +} + +/// A name in ASCII inside a blob that does not decode is still found. +/// +/// Recording the blob as unread is half the answer. A lossy reading keeps every +/// ASCII byte, so the name in this Latin-1 file was a finding before the audit +/// learned to decode blobs, and reporting only "could not be read" would turn +/// that finding into a line that says nothing about it. Both are reported; the +/// finding decides the exit, as a violation outranks an unread surface. +#[test] +fn a_name_in_a_blob_that_does_not_decode_is_still_found() { + let root = repository(); + let origin = origin_for(&root, "publication-undecodable-name-origin"); + std::fs::write( + root.join("latin1.txt"), + b"caf\xe9 au lait, as PrivateOrg serves it\n", + ) + .unwrap(); + support::git(&root, &["add", "-A"]); + support::git(&root, &["commit", "-qm", "one", "--no-verify"]); + support::git(&root, &["push", "-q", "origin", "main"]); + + let output = audit_with_gh(&root, "exit 0\n"); + let report = text(&output); + assert_eq!( + code(&output), + 1, + "the name is plain ASCII in a file that is not UTF-8:\n{report}" + ); + // The finding, on stderr where every finding is printed. + let findings = String::from_utf8_lossy(&output.stderr); + assert!(findings.contains("would be republished"), "{report}"); + assert!(findings.contains("latin1.txt (blob "), "{report}"); + // And the same blob listed as unread, on stdout with the rest of the report. + let stdout = String::from_utf8_lossy(&output.stdout); + let (_, measured) = stdout.split_once("could NOT be read:").unwrap(); + assert!(measured.contains("latin1.txt (blob "), "{report}"); + let _ = std::fs::remove_dir_all(&origin); +} + +/// A name written into a binary blob as plain bytes is still found. +/// +/// The guards skip a binary blob, for want of lines to point at. This audit +/// read every blob's bytes before it learned to decode them, and the decoder +/// is not a reason to stop: the flip serves the binary with the name in it. +#[test] +fn a_name_in_a_binary_blob_is_still_found() { + let root = repository(); + let origin = origin_for(&root, "publication-binary-origin"); + std::fs::write(root.join("blob.bin"), b"\x00\x01\xffPrivateOrg\x00\n").unwrap(); + support::git(&root, &["add", "-A"]); + support::git(&root, &["commit", "-qm", "one", "--no-verify"]); + support::git(&root, &["push", "-q", "origin", "main"]); + + let output = audit_with_gh(&root, "exit 0\n"); + let report = text(&output); + assert_eq!(code(&output), 1, "{report}"); + assert!(report.contains("blob.bin (blob "), "{report}"); + let _ = std::fs::remove_dir_all(&origin); +}