From 7dc94ce431f0a5a26c701e162d6d4e8d4f52d20e Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Wed, 14 Oct 2026 15:50:15 -0600 Subject: [PATCH] fix(redact): rotated and edge-clipped marks fail closed, gray JPEGs decode Review findings, each reproduced before the fix: - row_hits built each transformed row's bounding box and skipped it on a strict two-axis overlap; a 90-degree cm makes every row vertical (zero X extent), so every row was skipped and the original JPEG survived the mark untouched behind the overlay. A degenerate row now falls through to the per-pixel test; regression: a rotated scan is cleared. - a mark that clips an image's edge without covering a pixel centre left the image as-is; both the JPEG and raw paths now remove such an image whole instead (the mark touches it, fail closed). Regression: an edge-sliver mark removes the image. - identity_decode was checked against three components unconditionally, so a DeviceGray scan with an explicit /Decode [0 1] was removed whole. The check now uses the image's own component count. Regressions: gray with identity Decode is cleared; inverted Decode still fails closed. - the JPEG header's dimensions are now read before the sample allocation, so a dictionary that lies small cannot decode a huge JPEG past MAX_PIXELS; mismatches fail closed as before. 46 redact tests green; clippy/fmt clean; wasm32 check passes. --- crates/redact/src/image.rs | 56 +++++++++++++++++++++--- crates/redact/src/tests.rs | 88 +++++++++++++++++++++++++++++++++++++- 2 files changed, 138 insertions(+), 6 deletions(-) diff --git a/crates/redact/src/image.rs b/crates/redact/src/image.rs index c1660c3d0..f13941fba 100644 --- a/crates/redact/src/image.rs +++ b/crates/redact/src/image.rs @@ -69,11 +69,20 @@ fn filter_chain(s: &Stream) -> Option>> { /// Is a pixel's sample row, at row `j` of `h`, anywhere near a region? The image is walked row by /// row, so the whole rows outside every region are skipped without touching a pixel. +/// +/// A degenerate row (zero extent on either axis — a 90° rotation makes every row vertical, a +/// flip makes them horizontal, skew can collapse one) must *not* be skipped: `overlaps` is +/// strict in both axes, so a zero-width box would fail it and the mark would be silently ignored +/// while the image survived untouched. When either axis of the transformed row is degenerate the +/// conservative answer is "run the per-pixel test". fn row_hits(ctm: &Matrix, h: usize, j: usize, rects: &[[f64; 4]]) -> bool { let v = 1.0 - (j as f64 + 0.5) / h as f64; let (ax, ay) = ctm.apply(0.0, v); let (bx, by) = ctm.apply(1.0, v); let row = [ax.min(bx), ay.min(by), ax.max(bx), ay.max(by)]; + if (row[2] - row[0]).abs() < f64::EPSILON || (row[3] - row[1]).abs() < f64::EPSILON { + return true; + } rects.iter().any(|r| overlaps(*r, [row[0], row[1] - 0.01, row[2], row[3] + 0.01], 0.0)) } @@ -111,6 +120,22 @@ struct Jpeg { ncomp: usize, } +/// Does any region's rectangle intersect the image's transformed unit square? (Conservative: +/// a mark that only clips the square's edge still counts — the per-pixel centre test may find +/// no sample centre inside such a sliver, and an image the mark touches must not survive as-is.) +fn image_touched(ctm: &Matrix, rects: &[[f64; 4]]) -> bool { + let corners = [(0.0, 0.0), (1.0, 0.0), (0.0, 1.0), (1.0, 1.0)]; + let mut box_ = [f64::INFINITY, f64::INFINITY, f64::NEG_INFINITY, f64::NEG_INFINITY]; + for (u, v) in corners { + let (x, y) = ctm.apply(u, v); + box_[0] = box_[0].min(x); + box_[1] = box_[1].min(y); + box_[2] = box_[2].max(x); + box_[3] = box_[3].max(y); + } + rects.iter().any(|r| overlaps(*r, box_, 0.0)) +} + fn jpeg_clear(doc: &Document, s: &Stream, ctm: &Matrix, rects: &[[f64; 4]], w: usize, h: usize) -> Option> { let Jpeg { mut px, space, ncomp } = decode_jpeg(doc, s, w, h)?; let mut cleared = 0usize; @@ -130,6 +155,14 @@ fn jpeg_clear(doc: &Document, s: &Stream, ctm: &Matrix, rects: &[[f64; 4]], w: u } } if cleared == 0 { + // A mark that intersects the image (its unit square) but clears no pixel centre — an + // edge-only overlap, or a mark thinner than one pixel footprint — must not leave the + // original untouched: the overlay hides it visually, but the samples survive. Fail + // closed; the caller removes the image whole. (A mark entirely outside the image is + // the legitimate `Ok(None)` case.) + if image_touched(ctm, rects) { + return None; + } return Some(None); } let encoded = encode_jpeg(&px, w, h, space)?; @@ -159,15 +192,23 @@ fn decode_jpeg(doc: &Document, s: &Stream, w: usize, h: usize) -> Option { 3 => ColorType::Rgb8, _ => return None, }; - if !identity_decode(doc, s, 3) { + // `/Decode` must match the *image's* component count (one for DeviceGray, three for RGB): + // a gray scan with an identity `/Decode [0 1]` would otherwise fail the check and be + // removed whole — the very blanking this fix exists to prevent. + if !identity_decode(doc, s, if space == ColorType::L8 { 1 } else { 3 }) { + return None; + } + // The JPEG header is inspected *before* the sample allocation: a dictionary that declares a + // small image but carries a much larger JPEG must not be decoded first — the crate's + // `MAX_PIXELS` cap is enforced against the header, and only then are the samples decoded. + let reader = image::ImageReader::with_format(std::io::Cursor::new(s.raw.as_ref()), ImageFormat::Jpeg); + let (iw, ih) = reader.into_dimensions().ok()?; + if (iw as usize, ih as usize) != (w, h) { return None; } let reader = image::ImageReader::with_format(std::io::Cursor::new(s.raw.as_ref()), ImageFormat::Jpeg); let decoded = reader.decode().ok()?; - let (iw, ih) = (decoded.width() as usize, decoded.height() as usize); - if iw != w || ih != h { - return None; - } + debug_assert_eq!((decoded.width() as usize, decoded.height() as usize), (w, h)); let (px, ncomp) = match space { ColorType::L8 => (decoded.into_luma8().into_raw(), 1), _ => (decoded.into_rgb8().into_raw(), 3), @@ -228,6 +269,11 @@ fn raw_clear(doc: &Document, s: &Stream, ctm: &Matrix, rects: &[[f64; 4]], w: us } } if cleared == 0 { + // Same fail-closed rule as the JPEG path: a mark that touches this image but whose + // rectangle covers no sample centre (an edge sliver) removes the image whole. + if image_touched(ctm, rects) { + return None; + } return Some(None); } let mut dict = s.dict.clone(); diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index 102277b4a..c35d369ad 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -959,7 +959,7 @@ fn a_scanned_page_keeps_its_image_and_loses_the_text_under_the_mark() { // image — pixels 16–23 on both axes; the text mark covers R1234, x 20–40. let img = stream( "/Type /XObject /Subtype /Image /Width 40 /Height 40 /ColorSpace /DeviceRGB /BitsPerComponent 8 /Filter /DCTDecode", - &jpeg_white(40, 40), + jpeg_white(40, 40), ); let src = b"q 100 0 0 100 10 10 cm /Im1 Do Q BT /F1 10 Tf 10 200 Td (OCR1234) Tj ET"; let mut doc = one_page(src, "/XObject << /Im1 7 0 R >>", vec![img]); @@ -996,6 +996,92 @@ fn a_scanned_page_keeps_its_image_and_loses_the_text_under_the_mark() { assert!(!content(&saved, 0).contains("0031")); } +/// A redaction mark over a **rotated** scan must still clear the covered pixels. `row_hits` builds +/// the bounding box of each transformed sample row; with a 90° `cm`, every row is vertical (zero +/// X extent) and `overlaps` — strict in both axes — used to skip them all, leaving the original +/// JPEG (and its content under the mark) untouched while the overlay made it *look* redacted. +/// The row skip must be conservative for degenerate rows on either axis. +#[test] +fn a_rotated_scan_is_still_cleared_under_the_mark() { + // The same 40×40 white JPEG, drawn 100×100 at (10,10) but rotated 90°: + // `q 0 100 -100 0 110 10 cm` maps the image square to (10,10)-(110,110) sideways. + let img = stream( + "/Type /XObject /Subtype /Image /Width 40 /Height 40 /ColorSpace /DeviceRGB /BitsPerComponent 8 /Filter /DCTDecode", + jpeg_white(40, 40), + ); + let src = b"q 0 100 -100 0 110 10 cm /Im1 Do Q"; + let mut doc = one_page(src, "/XObject << /Im1 7 0 R >>", vec![img]); + mark(&mut doc, 0, &[[50.0, 50.0, 70.0, 70.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!((r.images_removed, r.images_cleared), (0, 1), "the rotated scan is cleared in place, never removed: {r:?}"); + + let (w, h, px) = page_image(&doc, 0).expect("the cleared image copy is there"); + assert_eq!((w, h), (40, 40)); + let at = |i: u32, j: u32| &px[((j * 40 + i) * 3) as usize..][..3]; + // The mark covers 20 pt = 8 image pixels; under the rotated CTM the covered sample block is + // still the middle 8×8 (the rotation is about the square's centre), so the same pixels must + // be black now as in the unrotated case. + for (i, j) in [(16, 16), (20, 20), (23, 23), (18, 21)] { + assert!(at(i, j).iter().all(|&b| b < 96), "the marked pixels are black at ({i},{j}): {:?}", at(i, j)); + } + for (i, j) in [(0, 0), (39, 0), (0, 39), (39, 39), (2, 20), (37, 20)] { + assert!(at(i, j).iter().all(|&b| b > 160), "the rest of the scan survives at ({i},{j}): {:?}", at(i, j)); + } +} + +/// A grayscale scan with an explicit identity `/Decode [0 1]` is cleared in place too — the +/// component count of the `/Decode` check must be the image's own (one), not three, or every +/// explicit-declaration gray scan would be removed whole and the page blanked again. +#[test] +fn a_gray_scan_with_an_identity_decode_is_cleared_not_removed() { + let mut gray = vec![200u8; 20 * 20]; + gray[..20].fill(20); + let mut jpeg = Vec::new(); + ::image::codecs::jpeg::JpegEncoder::new_with_quality(&mut jpeg, 90).encode(&gray, 20, 20, ::image::ExtendedColorType::L8).unwrap(); + let img = stream( + "/Type /XObject /Subtype /Image /Width 20 /Height 20 /ColorSpace /DeviceGray /BitsPerComponent 8 /Decode [0 1] /Filter /DCTDecode", + &jpeg, + ); + let mut doc = one_page(b"q 100 0 0 100 10 10 cm /Im0 Do Q", "/XObject << /Im0 7 0 R >>", vec![img]); + mark(&mut doc, 0, &[[40.0, 40.0, 60.0, 60.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!((r.images_removed, r.images_cleared), (0, 1), "the gray scan is cleared, not removed: {r:?}"); +} + +/// A gray scan with an *inverted* `/Decode [1 0]` still fails closed: cleared samples would paint +/// white on an inverted image, so the whole image goes instead. +#[test] +fn a_gray_scan_with_an_inverted_decode_still_fails_closed() { + let gray = vec![200u8; 20 * 20]; + let mut jpeg = Vec::new(); + ::image::codecs::jpeg::JpegEncoder::new_with_quality(&mut jpeg, 90).encode(&gray, 20, 20, ::image::ExtendedColorType::L8).unwrap(); + let img = stream( + "/Type /XObject /Subtype /Image /Width 20 /Height 20 /ColorSpace /DeviceGray /BitsPerComponent 8 /Decode [1 0] /Filter /DCTDecode", + &jpeg, + ); + let mut doc = one_page(b"q 100 0 0 100 10 10 cm /Im0 Do Q", "/XObject << /Im0 7 0 R >>", vec![img]); + mark(&mut doc, 0, &[[40.0, 40.0, 60.0, 60.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!(r.images_removed, 1, "the inverted scan is removed whole: {r:?}"); + assert!(!content(&doc, 0).contains("/Im0")); +} + +/// A mark that only clips the image's edge — too thin to contain any pixel centre — must not +/// leave the original image untouched behind the overlay: the image is removed whole instead. +#[test] +fn an_edge_sliver_mark_fails_closed_instead_of_leaving_the_image() { + let img = stream( + "/Type /XObject /Subtype /Image /Width 40 /Height 40 /ColorSpace /DeviceRGB /BitsPerComponent 8 /Filter /DCTDecode", + jpeg_white(40, 40), + ); + // Drawn at (10,10)-(110,110): a mark from x=10 to x=10.5 overlaps the first column's + // footprint, but its centre (x=11.25) is outside — no pixel would be cleared. + let mut doc = one_page(b"q 100 0 0 100 10 10 cm /Im1 Do Q", "/XObject << /Im1 7 0 R >>", vec![img]); + mark(&mut doc, 0, &[[10.0, 10.0, 10.5, 110.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!((r.images_removed, r.images_cleared), (1, 0), "an image the mark touches is never left as-is: {r:?}"); +} + /// An image whose codec PdfCraft cannot clear in place (a JBIG2 fax scan, say) still fails closed: /// the whole image goes, rather than the mark being quietly ignored. #[test]