Repository navigation
Revert "harden: fix security issue in prepare-overview-review.py" - #2
exsinger-hub wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It reintroduces a previously fixed XML parsing security risk by reverting from defusedxml back to stdlib minidom for parsing PPTX-provided XML.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reverts a prior security hardening change in scripts/prepare-overview-review.py by switching XML parsing imports back to Python’s standard library xml.dom.minidom.
Changes:
- Replace
defusedxml.minidomwith stdlibxml.dom.minidom. - Replace
pyexpat.ExpatErrorimport withxml.parsers.expat.ExpatError.
File summaries
| File | Description |
|---|---|
| scripts/prepare-overview-review.py | Reverts hardened XML parser imports back to stdlib minidom for PPTX XML parsing. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| from xml.dom import minidom | ||
| from xml.parsers.expat import ExpatError |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 916a527f4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from xml.dom import minidom | ||
| from xml.parsers.expat import ExpatError |
There was a problem hiding this comment.
When this helper processes an untrusted PPTX, its presentation.xml, relationship XML, or selected slide XML can contain nested internal entity declarations. xml.dom.minidom.parseString expands those entities before the script's validation runs, allowing a crafted package to consume substantial CPU and memory; the reverted defusedxml parser rejects such declarations. Keep the hardened parser (and declare its dependency) or explicitly reject DTD/entity-bearing XML.
Useful? React with 👍 / 👎.
revert