FIX:: 1056 hardened xml parser#1695
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1695 +/- ##
==========================================
+ Coverage 80.55% 82.79% +2.23%
==========================================
Files 112 65 -47
Lines 18679 8567 -10112
==========================================
- Hits 15047 7093 -7954
+ Misses 3632 1474 -2158
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@microsoft-github-policy-service agree company="Microsoft" |
|
/review |
|
✅ PR Review completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR correctly addresses the XXE hardening goal — adding XMLParser(resolve_entities=False, no_network=True) to _add_narration_timing is a sound defence-in-depth measure, and the broken byzanz link fix in CONTRIBUTING.md is welcome. However, one required change and one suggestion need attention before merge.
Issue Alignment
Branch name and commit message reference issue #1056 (hardened XML parser). The security fix in embed_audio.py and the accompanying test are directly aligned with that goal. ✅
PR Template Compliance
Unable to verify PR description contents due to an integrity policy filter on the PR metadata. If the description is incomplete or missing required template sections, please fill them in.
Coding Standards — ⚠️ Required Change
CONTRIBUTING.md — list marker inconsistency (see inline comment at line 72)
The diff changes the Table of Contents bullets from * to -. The rest of CONTRIBUTING.md continues to use *, creating a mixed-marker file. This violates the repo's markdown instructions (.github/instructions/hve-core/markdown.instructions.md):
Use unordered list markers consistently across a file; for the same level, do not mix
*,+,-. Try to always use*for unordered lists.
Please revert the ToC list markers to *.
Code Quality — 💡 Suggestion
test_embed_audio.py — test validates lxml behaviour, not the production fix (see inline comment at line 125)
The new test exercises lxml's parser options directly without calling _add_narration_timing. A regression in the production function (e.g., the parser argument being dropped) would not be caught. The inline comment includes a suggested complementary test that patches etree.fromstring to assert the hardened parser is actually passed.
What's Good ✅
- Security fix in
embed_audio.pyis correct:XMLParser(resolve_entities=False, no_network=True)passed toetree.fromstringis the right approach for defence-in-depth. byzanzlink updated from the defunctgit.gnome.orgURL to the correct GitLab archive. ✅- Logger call reformatting improves readability for long argument lists. ✅
Action Items
- Required: Revert
CONTRIBUTING.mdToC list markers from-back to*. - Suggested: Add a test that calls
_add_narration_timingand asserts the hardened parser is passed toetree.fromstring.
Note
🔒 Integrity filter blocked 3 items
The following items were blocked because they don't meet the GitHub integrity level.
- #1695
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #1695
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #1695
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none… fix_1056_hardened_xml_parser
|
/review |
|
✅ PR Review completed successfully! |
There was a problem hiding this comment.
Advisory review, this PR is from a maintainer. Findings are informational only.
Overview
This PR applies a hardened lxml.etree.XMLParser(resolve_entities=False, no_network=True) to the _TIMING_TEMPLATE parse call in _add_narration_timing, and adds two new tests to validate the defence. The security intent is correct and the fix itself is minimal and well-targeted.
A codebase-wide grep confirms no other unhardened etree.fromstring / etree.parse call sites exist in .github/skills/ or scripts/ (the xml.sax.saxutils usages in generate_voiceover.py and fuzz_harness.py are for escaping/quoting output, not parsing, so they are not affected).
Issue Alignment
Partially aligned with issue #1056. The issue's acceptance criteria explicitly require:
- A documented call site inventory (file path, line number, parser config, trust level)
- Evidence that the codebase-wide grep was completed
- Follow-up issues for any findings needing larger refactoring
None of these artefacts appear in this PR. Since the grep shows no other vulnerable sites, the remediation appears complete in practice — but the checklist items remain open on the issue. Consider either closing the issue with a note that the audit found only this one site, or updating the issue body with the inventory.
PR Template Compliance
No blocking template gaps identified from the available diff context.
Coding Standards
Three test-convention deviations noted via inline comments:
- Test naming — both new methods use a descriptive-prose style instead of the
test_given_context_when_action_then_expectedformat that every pre-existing test in this class follows (python-tests.instructions.md). - AAA comment structure — the pre-existing tests all include
# Arrange,# Act,# Assertsection comments; the new tests omit them. - Dead mock setup —
mock_slide.element.find.return_value = Nonepatches the wrong attribute (.elementvs._element).
Code Quality
_parserleading underscore on a local variable is a minor style nit (see inline comment onembed_audio.py:115).- The first new test validates
lxmllibrary behaviour rather than_add_narration_timingdirectly; its value as coverage for the PR's intent is marginal compared with the second test.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #1695
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #1695
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none| assert "old-content" not in xml_str | ||
| assert 'spid="10"' in xml_str | ||
|
|
||
| def test_timing_template_parsed_with_hardened_parser_blocks_entity_expansion(self): |
There was a problem hiding this comment.
The test name doesn't follow the existing test_given_context_when_action_then_expected convention used throughout this file (e.g. test_given_slide_xml_when_add_timing_then_timing_element_appended). Also, the test body is missing # Arrange, # Act, and # Assert section comments required by python-tests.instructions.md.
Additionally, this test validates lxml.etree.XMLParser library behavior rather than _add_narration_timing itself — consider whether it adds coverage beyond the second new test, or rename/reframe to make the purpose clear.
Suggested rename:
def test_given_xxe_payload_when_hardened_parser_used_then_entity_not_expanded(self):
bindsi
left a comment
bindsi
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @vdstrizhkova — nice job closing out the XML hardening from #1053/#1056. The fix correctly applies the established XMLParser(resolve_entities=False, no_network=True) idiom (matching powerpoint/scripts/extract_content.py), and test_add_narration_timing_uses_hardened_xml_parser is a solid behavioral test that verifies the production code actually passes the hardened parser. Verified locally: all 9 tests pass and ruff check is clean. ✅
Approving. A few NIT recommendations for a future cleanup (all non-blocking):
NIT 1 — Redundant first test. test_timing_template_parsed_with_hardened_parser_blocks_entity_expansion never imports or calls _add_narration_timing/embed_audio; it builds its own parser and asserts lxml's behavior, which is effectively a library test. The second test already proves the production code uses a hardened parser, so this one could be removed (or refactored to assert against the module's actual parser).
NIT 2 — Dead stub line. In test_add_narration_timing_uses_hardened_xml_parser:
mock_slide.element.find.return_value = NoneThe production code reads slide._element.find(...), not slide.element.find(...), and _element is reassigned to a real etree.Element on the next line — so this stub is a no-op and can be dropped.
NIT 3 — Naming consistency. The new local _parser uses a leading underscore (conventionally "private/unused"), whereas the sibling skill uses parser. Renaming to parser keeps it consistent with extract_content.py.
None of these block merge. Thanks again for the contribution! 🙌
remove redundant XXE library test, drop dead mock stub line
remove redundant XXE library test, drop dead mock stub line
bindsi
left a comment
bindsi
left a comment
There was a problem hiding this comment.
Automated batch re-review: no actionable findings. The XML parser hardening remains in place and the prior concern is resolved.
4 participants
Pull Request
Description
Extends the XML parser hardening from PR #1053 to the remaining unprotected call
Related Issue(s)
"Closes #1056"
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderagent and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md)Other:
.ps1,.sh,.py)Testing
Checklist
Required Checks
Required Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run lint:psnpm run plugin:generatenpm run docs:testSecurity Considerations
Additional Notes
fixed Contributor.md broken link