Keep NONET when overriding Nokogiri parse options#176
Merged
Conversation
Passing RECOVER explicitly to Nokogiri::XML::Reader replaces the defaults (RECOVER | NONET | BIG_LINES) and silently drops NONET, allowing a crafted document to trigger outbound network requests via external DTD/entity references. Restore NONET at both streaming reader call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ollietulloch
approved these changes
Jul 10, 2026
ollietulloch
left a comment
ollietulloch
left a comment
Contributor
There was a problem hiding this comment.
Thanks @timgentry 🙇
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Details
What?
Restores Nokogiri's
NONETparse option in our two streaming XML readers, so that parsing an XML file can never trigger outbound network requests (e.g. fetching external DTDs or entities referenced by the document).Why?
When we pass
RECOVERexplicitly toNokogiri::XML::Reader, we replace Nokogiri's defaults (RECOVER | NONET | BIG_LINES) rather than adding to them — silently droppingNONET. Since this library ingests files from external sources, a crafted document referencing a remote DTD could cause the importing server to make attacker-directed network requests (SSRF-style). This closes that gap as defence in depth; entity substitution (NOENT/DTDLOAD) was already off, so no data-disclosure XXE path existed.How?
A one-line change at each of the two
Nokogiri::XML::Readercall sites (lib/ndr_import/file/xml.rb,lib/ndr_import/helpers/file/xml_streaming.rb):RECOVERbecomesRECOVER | NONET, with a comment explaining why the flag must be kept when overriding options.Testing?
Existing XML streaming test suite covers these code paths; the change only removes a capability (network fetches during parse) that no legitimate input relies on. A security review of the change and the wider
lib/tree confirmed no remaining XML parsing sink is missingNONET.AI Contribution?
Claude (Fable 5, via Claude Code) performed the security review that identified the dropped
NONETflag and drafted this PR description. The code change was human-authored and human-reviewed.Screenshots (optional)
N/A — no user-facing output changes.
Anything Else?
If we ever add another explicit-options Nokogiri call site, the same trap applies: prefer starting from
Nokogiri::XML::ParseOptions::DEFAULT_XMLand adding flags, rather than building the option set from scratch.🤖 Generated with Claude Code