Automattic / Automattic/xliff

Handle XML-illegal control characters in source/target/note text consistently

Open
#29 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Ruby
Stars
3
Forks
2
Avg merge
2h 46m
Merged PRs (30d)
3

Description

XML 1.0 forbids the C0 control characters (`0x00`–`0x1F` except tab, LF, and CR) anywhere in a document, including text content. `Xliff` neither rejects nor consistently sanitizes them in `source`/`target`/`note` text, and the two serialization paths disagree on what happens — so the same string produces different (both lossy) output depending on which one you call.

Confirmed with a form feed (`0x0c`) in `source`:

- **`Entry#to_s`** replaces it with the Unicode replacement char: `Line1�Line2` — lossy, but valid XML.
- **`Bundle#to_s`** emits the **raw `0x0c` byte** into the output — which is not well-formed XML 1.0 (Nokogiri itself rejects it: `FATAL: PCDATA invalid Char value 12`). On re-parse the library recovers by **silently dropping** the character, so `Bundle.from_string(bundle.to_s)` loses data:

```ruby
ff = "Line1\x0cLine2"
Xliff::Entry.new(id: "x", source: ff, target: "T").to_s
#=> "Line1�Line2..." # U+FFFD

b = Xliff::Bundle.new
f = Xliff::File.new(original: "x", source_language: "en")
f.add_entry(Xliff::Entry.new(id: "ctrl", source: ff))
b.add_file(f)
out = b.to_s
out.bytes.include?(0x0c) #=> true (raw, invalid byte emitted)
Xliff::Bundle.from_string(out).files[0].entries[0].source #=> "Line1Line2" (char vanished)
```

So `Bundle#to_s` can emit a document a conformant parser rejects, and a tolerant re-parse silently corrupts the text — different from `Entry#to_s`'s `U+FFFD`.

## Proposed fix

Pick one policy and apply it in a single shared place (the text path that both `Entry#to_s` and `Bundle#to_s` flow through), so the two paths can't diverge:

1. **Reject** — raise on an XML-illegal control character at construction/parse. Consistent with the rest of #15 ("surface malformed input instead of fabricating it"): a string that can't be represented in XLIFF 1.2 is malformed input, not something to silently mangle.
2. **Sanitize** — strip or replace illegal chars consistently (e.g. always `U+FFFD`, matching the current `Entry#to_s` behavior), and document it as lossy.

Rejecting is the better fit for the library's current stance; sanitizing is the more permissive option if callers are expected to pass through arbitrary upstream text. Either way the two serialization paths must agree.

## Scope

Split out of the #15 review sweep — text encoding is **pre-existing** and untouched by that PR. Xcode's own exports don't contain control characters, so this isn't required for that use case, but it's silent data loss (and invalid output) for general input, and the path divergence is a latent footgun. Same "decide a policy, apply it once" shape as the conformance fixes in #15.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by tracing the text serialization paths used by Entry#to_s and Bundle#to_s, then reproduce the form-feed example from the issue. Decide with the maintainer whether illegal XML characters should be rejected or sanitized, and apply that policy through their shared path. Done means both serializers agree, output is well-formed XML, and the behavior is covered for source, target, and note text.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby, xml
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.