Skip to content

fix(redact): scrub unknown binary parts - #241

Open
eliahilse wants to merge 1 commit into
mainfrom
fix/redact-binary-scrub
Open

fix(redact): scrub unknown binary parts#241
eliahilse wants to merge 1 commit into
mainfrom
fix/redact-binary-scrub

Conversation

@eliahilse

Copy link
Copy Markdown
Member

TL;DR:

Summary:

  • invert the binary-part policy from deny-list to allow-list: any part that is not media or XML is removed by default (PowerPivot models, VBA signatures, embedded font subsets no longer survive redaction)
  • removal is coordinated: owned .rels parts cascade (including root-level _rels/x.rels forms and nested ownership), internal targets join the removal set unless a surviving part still references them, and content-type declarations are pruned
  • path normalization matches the OPC layer exactly so noncanonical entry aliases (./x, double slashes, backslashes) are classified, pruned and redacted consistently
  • the removal cascade runs before media/XML transformation, so scrubbed binaries can no longer fail redaction via undecodable targets

Test plan:

  • cargo test -p betteroffice-redact green
  • redact a workbook with a PowerPivot model + a deck with embedded fonts; verify output opens cleanly in Excel/PowerPoint with features absent
  • confirm zero secret bytes survive in any part of both outputs

@openooxml-bot

openooxml-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA — thank you! ✍️ ✅

Posted by the CLA bot.

@greptile-apps

greptile-apps Bot commented Aug 24, 2026

Copy link
Copy Markdown

Greptile Summary

The PR changes binary redaction from a deny-list to an allow-list and coordinates removal across package relationships and content-type declarations.

  • Normalizes package-part names before classification and format detection.
  • Recursively removes relationship parts and internal targets owned by scrubbed binaries while preserving shared targets.
  • Prunes relationships and content-type declarations that point to removed parts.
  • Adds coverage for unknown binaries, noncanonical paths, nested ownership, unsupported media, and shared targets.

Confidence Score: 4/5

The PR appears safe to merge after the non-blocking documentation-style cleanup.

The package-removal flow coordinates normalized part classification, relationship cascades, and content-type pruning with substantial focused test coverage; the only accepted concern is unnecessarily verbose function documentation.

Files Needing Attention: crates/ooxml-redact/src/scrub.rs

Important Files Changed

Filename Overview
crates/ooxml-redact/src/lib.rs Replaces selective binary clearing with normalized allow-list classification, coordinated pruning, and post-pruning XML/media transformation.
crates/ooxml-redact/src/scrub.rs Introduces relationship-graph cascading and declaration pruning; implementation appears coherent, with only overly detailed docstrings requiring cleanup.
crates/ooxml-redact/src/tests.rs Adds broad regression coverage for binary removal, path aliases, ownership cascades, orphan relationships, unsupported media, and shared targets.
.changeset/redact-binary-allowlist.md Accurately describes the package-level binary scrubbing behavior as a patch release.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Unpack validated OOXML parts] --> B[Normalize part names]
    B --> C{Media or XML?}
    C -- No --> D[Seed binary removal set]
    C -- Yes --> E[Retain for transformation]
    D --> F[Add owned relationship parts]
    F --> G[Add internal relationship targets]
    G --> H{Referenced by surviving owner?}
    H -- Yes --> I[Spare shared target]
    H -- No --> J[Remove part]
    I --> F
    J --> K[Prune relationships and content types]
    K --> L[Redact XML and replace media]
    E --> L
    L --> M[Rebuild OOXML package]
Loading

Reviews (1): Last reviewed commit: "fix(redact): scrub unknown binary parts" | Re-trigger Greptile

Comment on lines +6 to +10

use crate::RedactError;

/// Drop scrubbed parts together with the relationship parts they own, the
/// internal parts those relationships target (unless a retained part also

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Keep scrub docstrings concise

This multi-line docstring explains implementation details already expressed by the function name and code, increasing maintenance cost and creating documentation that can drift as the cascade algorithm changes. The same pattern occurs on removal_closure and resolve_relationship_target.

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@eliahilse
eliahilse force-pushed the fix/redact-binary-scrub branch from 1f6daef to e6aa834 Compare August 26, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant