Skip to content

fix(redact): catch external targets missing TargetMode - #238

Merged
eliahilse merged 6 commits into
mainfrom
fix/redact-external-links
Aug 26, 2026
Merged

fix(redact): catch external targets missing TargetMode#238
eliahilse merged 6 commits into
mainfrom
fix/redact-external-links

Conversation

@eliahilse

@eliahilse eliahilse commented Aug 23, 2026

Copy link
Copy Markdown
Member

TL;DR:

Summary:

  • a relationship target is treated as external when TargetMode=External (whitespace- and case-tolerant), or when the target carries an RFC 3986 scheme, is protocol-relative (//host/share), or names a UNC share (\\host\share, \\?\UNC\...)
  • a relationship the target proves external always ends up with TargetMode="External" — synthesized when absent, normalized when the producer spelled it oddly, corrected when the producer declared Internal — so the placeholder target and the declared mode never disagree and the redacted package still parses
  • relationship handling applies only to .rels parts and to unqualified OPC attributes, so foreign namespaced attributes and Relationship-shaped markup in ordinary parts are left alone
  • fragments and queries containing URIs (part.xml#ref=https://...), package-root targets (/word/media/x.png) and relative targets stay internal and byte-identical
  • targets that climb above the package root are unchanged from today's behaviour; classifying them safely needs canonical OPC part-name resolution, tracked in fix(redact): canonical OPC part-name resolution for relationship targets #245

Test plan:

  • cargo test -p betteroffice-redact green
  • cargo test --workspace, cargo clippy --workspace --all-targets --all-features -- -D warnings and cargo fmt --all -- --check green
  • redact a package with a no-TargetMode UNC hyperlink and confirm the host does not survive in the output zip
  • confirm a redacted package still loads through docx-parse / xlsx-parse / pptx-parse
  • confirm an internal relationship that resolves to a real part is preserved byte-for-byte, including percent-encoded, ./ and backslash spellings

@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR expands relationship redaction to classify targets as external when they have an RFC 3986 scheme or are protocol-relative, even without TargetMode="External".

  • Adds scheme and protocol-relative target detection to the XML relationship rewrite.
  • Adds coverage for HTTP, file, telephone, email, custom-scheme, relative, and fragment-bearing targets.
  • Adds a patch changeset for the redaction behavior.

Confidence Score: 3/5

This PR should not merge until raw backslash UNC relationship targets are also classified and redacted.

The new fallback closes several missing-TargetMode leaks but still preserves sensitive server and share paths for reachable raw UNC targets.

Files Needing Attention: crates/ooxml-redact/src/xml.rs, crates/ooxml-redact/src/tests.rs

Security Review

The fallback still misses raw backslash UNC targets without TargetMode, allowing private server/share or user path information to remain in redacted packages.

Important Files Changed

Filename Overview
crates/ooxml-redact/src/xml.rs Adds fallback external-target classification, but misses raw backslash UNC targets and can leave sensitive paths unredacted.
crates/ooxml-redact/src/tests.rs Adds useful scheme and fragment coverage, but the UNC case is scheme-prefixed and does not cover the remaining raw UNC leak.
.changeset/redact-external-links.md Adds an appropriate patch release note for the expanded relationship redaction.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Relationship Target] --> B{TargetMode External?}
  B -- Yes --> E[Rewrite to safe placeholder]
  B -- No --> C{Starts with // or URI scheme?}
  C -- Yes --> E
  C -- No --> I[Keep as internal target]
  U[Raw backslash UNC path] --> C
  C -. Misclassified .-> I
Loading

Reviews (1): Last reviewed commit: "fix(redact): catch external targets miss..." | Re-trigger Greptile

Comment on lines +370 to +376
fn external_target(target: &str) -> bool {
let lower = target.trim().to_ascii_lowercase();
lower.starts_with("//")
|| lower
.split_once(':')
.is_some_and(|(scheme, _)| is_uri_scheme(scheme))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Raw UNC targets remain unredacted

When a relationship uses a raw backslash UNC target such as \\server\share\x.xlsx without TargetMode="External", external_target rejects both branches and preserves the sensitive server/share path in the redacted package. How this was verified: Raw UNC targets have neither the checked // prefix nor a URI-scheme colon, while only targets passing this predicate are rewritten.

Suggested change
fn external_target(target: &str) -> bool {
let lower = target.trim().to_ascii_lowercase();
lower.starts_with("//")
|| lower
.split_once(':')
.is_some_and(|(scheme, _)| is_uri_scheme(scheme))
}
fn external_target(target: &str) -> bool {
let lower = target.trim().to_ascii_lowercase();
lower.starts_with("//")
|| lower.starts_with(r"\\")
|| lower
.split_once(':')
.is_some_and(|(scheme, _)| is_uri_scheme(scheme))
}

Knowledge Base Used:

@openooxml-bot

openooxml-bot Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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

Posted by the CLA bot.

@eliahilse
eliahilse force-pushed the fix/redact-external-links branch from 1530170 to 1e68276 Compare August 25, 2026 21:30
eliahilse and others added 4 commits August 26, 2026 00:26
Relationship targets that name a UNC share or climb above the package
root are now redacted alongside scheme-bearing ones, and a relationship
whose external mode was inferred gets TargetMode="External" written out
so the placeholder is not read back as an internal part reference.

Co-Authored-By: codex <codex@openai.com>
Relationship handling now applies only to `.rels` parts and unqualified
OPC attributes, a target that climbs above the package root is external
only when the clamped path names no part, and a relationship the target
proves external always carries TargetMode="External" so the placeholder
and the declared mode agree.

Co-Authored-By: codex <codex@openai.com>
A target that climbs above the package root is no longer classified as
external. The part index it was checked against compared raw lowercased
names, so percent-encoded, dot-segment and backslash spellings of parts
the package really contains were rewritten to the placeholder and the
document lost the part. Scheme, protocol-relative and UNC detection,
TargetMode normalization and the inferred TargetMode declaration stay.

Co-Authored-By: codex <codex@openai.com>
eliahilse and others added 2 commits August 26, 2026 11:26
Scoping relationship handling to .rels parts also dropped main's rule that
an explicit TargetMode="External" rewrites the target wherever it appears.
Target inspection stays scoped to .rels; a declared external mode is honoured
again everywhere, and the mode is written back under its canonical name so a
lowercase targetmode spelling no longer leaves the redacted package with a
placeholder URI the parsers read as internal.

Co-Authored-By: codex <codex@openai.com>
Two well-formed inputs came out worse than they went in. A relationship
carrying both Target and a case-variant target let the secondary spelling
decide the mode, so the authoritative internal target was replaced and the
slide went missing; the shape is now read from the exact-case Target that
consumers resolve. A relationship carrying two case-variant mode attributes
emitted two canonical TargetMode attributes, which no longer parses; the
canonical mode is now written once.

Co-Authored-By: codex <codex@openai.com>
@eliahilse
eliahilse merged commit 188540f into main Aug 26, 2026
6 checks passed
@eliahilse
eliahilse deleted the fix/redact-external-links branch August 26, 2026 13:27
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