Fix browse mode failing to read pages with malformed ARIA attributes

[akj]

Link to issue number:

Fixes #7173

Summary of the issue:

When a page contains malformed markup such as <section aria-label"almenük" role="region"> (note the missing =), the browser exposes an IA2 object attribute whose name contains quote characters. The virtual buffer emits attribute names verbatim into its XML markup (only spaces were sanitized, per #6249), producing attribute names like IAccessible2::attribute_label"almenük" which are not well-formed XML. NVDA's XMLFormatting.XMLTextParser (expat) then fails with ExpatError: not well-formed (invalid token), so NVDA plays an error sound and cannot read any content in ranges containing the affected node.

Minimal test case (from @jcsteh) — paste into the Firefox address bar:

data:text/html,<section aria-label"almen%C3%BCk" role="region">test

Description of user facing changes:

Pages containing this kind of malformed markup are now read normally in browse mode; the broken attribute is ignored and the content is navigable, with no error sound.

Description of developer facing changes:

• sanitizeXMLAttribName in nvdaHelper/common/xml.h now replaces every character outside a conservative allowlist (A-Z a-z 0-9 - . : _) with _, instead of only replacing spaces. A new isValidXMLNameChar helper defines the allowlist. Names that would be empty or start with a character that is only valid in non-initial positions (digits, -, .) get a leading underscore, since XML's NameStartChar rules are stricter than NameChar.
• VBufStorage_fieldNode_t::addAttribute in nvdaHelper/vbufBase/storage.cpp sanitizes names once at the attribute map's single insertion point, so the buffer only ever holds names that are valid in the XML markup it serializes to. A sanitized name never overwrites an existing attribute whose name was genuinely valid, and because the map's keys are the sanitized names, duplicate attributes (also rejected by expat) cannot be emitted.
• New unit tests in tests/unit/test_XMLFormatting.py pin the contract that every name the sanitizer can produce is accepted by the expat configuration NVDA parses buffer markup with.

Description of development approach:

Sanitization is applied in vbufBase, which is shared by all virtual buffer backends (Gecko, Chromium, MSHTML, Adobe), so no per-backend changes are needed. It happens once, when an attribute is added to the buffer (at page render), rather than on the serialization path that runs on every browse-mode text fetch. Names NVDA actually consumes are all valid already and pass through unchanged, so attribute matching (e.g. getAttributesString, find-by-attributes) is unaffected; only names that would previously have broken parsing entirely are altered.

The allowlist is deliberately conservative ASCII rather than the full XML 1.0 fifth edition NameChar production: the expat build shipped with Python enforces the stricter fourth edition name rules and rejects many BMP characters the fifth edition allows (e.g. U+0132). Since every attribute name NVDA's Python code consumes is ASCII, replacing everything else loses nothing meaningful while guaranteeing the generated names are always parseable. The unit tests encode this as an executable contract, so a future change to Python's expat behaviour will be caught by CI.

An existing XML library was considered and rejected; see the review discussion below for the detailed survey.

Testing strategy:

• Unit tests (tests/unit/test_XMLFormatting.py): every character the sanitizer can leave in a name is asserted to be accepted by expat in both interior and leading positions; the leading-character restrictions are asserted; the exact failing name from this issue (IAccessible2::attribute_label"almenük" → IAccessible2::attribute_label_almen_k_), the Broken Google Chrome support from NVDA #6249 Chrome case (fai clic → fai_clic), and the fourth-edition-only character U+0132 are covered; and markup containing sanitized names is parsed end to end through XMLFormatting.XMLTextParser. Full unit suite passes (1392 tests).
• Built with scons source; the affected C++ compiles cleanly (warnings as errors). runlint passes.
• Manual: Loaded the example from above in Firefox and moved through the page in browse mode; NVDA read "test" with no error sound.

Known issues with pull request:

None known.

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

Notes on the checklist: a change log entry is included; no user/developer documentation or GUI help changes are needed. There is no unit test harness for the vbufBase C++ code itself, so the sanitizer's contract is instead pinned from the Python side: tests/unit/test_XMLFormatting.py mirrors the allowlist and asserts everything it permits is accepted by the expat parser NVDA uses, alongside end-to-end parsing cases; a manual test case is also provided. The fix applies to all virtual buffer backends and all output paths (speech and braille both consume the parsed buffer text). Add-on facing behaviour only changes for attribute names that previously broke parsing entirely.

[SaschaCowley]

@akj have you investigated using an existing XML sanitization library to do this?

• If so, which ones, what were the results, and why did you go with this approach?
• If not, why not?
  I'm a little concerned about this kind of bespoke logic, especially for future maintainers.

[akj]

Yes, I did look into this before settling on the current approach — and I share the concern about bespoke logic; it's a big part of why the fix ended up shaped the way it is.

• What I investigated: pugixml, TinyXML-2, XmlLite, and libxml2 on the C++ side, and lenient parsing with lxml on the Python side.
• The results: none of them solve this particular problem. XML libraries are good at escaping attribute values (which the buffer already does), but their writers all assume the caller supplies valid attribute names — none validate or repair them. The closest thing is libxml2's xmlValidateName, but that only tells you a name is bad; the repair logic would still be ours to write, and it would mean adding a C library to the code we inject into other processes. So a library wouldn't remove the bespoke part — it would just sit next to it.
• Why this approach: there's also a subtlety that made me warier of a library: "valid" depends on the parser doing the reading, and Python's expat follows the older, stricter XML name rules — a modern spec-conformant validator passes names our own parser then rejects. The safest definition of valid here is "whatever NVDA's parser accepts", and no library encodes that.

On the maintainer burden specifically: rather than asking future maintainers to trust a character list in a C++ header, I've added unit tests (tests/unit/test_XMLFormatting.py) asserting that every name the sanitizer can produce is accepted by the same expat parser NVDA uses, plus the specific problem cases from this issue and #6249. If expat's rules ever shift, CI will say so rather than a user hitting it. For scale, the sanitizer is about 20 lines, in the same header where appendCharToXML has been doing the equivalent bespoke job for attribute values since 2016.

While revisiting this I also tightened the fix based on further review: names are now sanitized once when an attribute is added to the buffer rather than on every serialization, and a couple of edge cases (invalid leading characters, name collisions after sanitization) are handled.