From fddea464e44d93fb1d8b7a376f2114230b93be84 Mon Sep 17 00:00:00 2001 From: Cameron G Date: Thu, 2 Jul 2026 21:15:00 -0400 Subject: [PATCH 1/3] fix(crewai-tools): use defusedxml in RAG XMLLoader to prevent XXE XMLLoader parses XML content sourced from user-supplied URLs and file paths via xml.etree.ElementTree, which allows external entity resolution and unbounded entity expansion. Documents containing DOCTYPE declarations can trigger XXE (file read / SSRF) or billion-laughs denial of service. Swap fromstring/parse over to defusedxml.ElementTree, which is API compatible and rejects entity declarations by default. Add defusedxml as a runtime dependency and drop the two noqa: S314 markers now that the underlying warning no longer applies. Adds tests/rag/test_xml_loader_security.py covering: - external-entity documents raise EntitiesForbidden - nested-entity (billion-laughs) documents raise EntitiesForbidden - benign XML still parses to the expected tree - malicious XML fetched from a URL is rejected --- lib/crewai-tools/pyproject.toml | 1 + .../crewai_tools/rag/loaders/xml_loader.py | 8 +- .../tests/rag/test_xml_loader_security.py | 83 +++++++++++++++++++ 3 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 lib/crewai-tools/tests/rag/test_xml_loader_security.py diff --git a/lib/crewai-tools/pyproject.toml b/lib/crewai-tools/pyproject.toml index 018fb5c158..c41b1ab5f3 100644 --- a/lib/crewai-tools/pyproject.toml +++ b/lib/crewai-tools/pyproject.toml @@ -16,6 +16,7 @@ dependencies = [ "python-docx~=1.2.0", "youtube-transcript-api~=1.2.2", "pymupdf~=1.26.6", + "defusedxml>=0.7.1", ] diff --git a/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py b/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py index f7f9a6d00d..1c7f8e08d0 100644 --- a/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py +++ b/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py @@ -1,5 +1,7 @@ from typing import Any -from xml.etree.ElementTree import ParseError, fromstring, parse +from xml.etree.ElementTree import ParseError + +from defusedxml.ElementTree import fromstring, parse from crewai_tools.rag.base_loader import BaseLoader, LoaderResult from crewai_tools.rag.loaders.utils import load_from_url @@ -40,9 +42,9 @@ def _load_from_file(path: str) -> str: def _parse_xml(self, content: str, source_ref: str) -> LoaderResult: try: if content.strip().startswith("<"): - root = fromstring(content) # noqa: S314 + root = fromstring(content) else: - root = parse(source_ref).getroot() # noqa: S314 + root = parse(source_ref).getroot() text_parts = [] for text_content in root.itertext(): diff --git a/lib/crewai-tools/tests/rag/test_xml_loader_security.py b/lib/crewai-tools/tests/rag/test_xml_loader_security.py new file mode 100644 index 0000000000..b8be8012ba --- /dev/null +++ b/lib/crewai-tools/tests/rag/test_xml_loader_security.py @@ -0,0 +1,83 @@ +"""Regression tests for XMLLoader against XXE and entity-expansion attacks.""" + +from unittest.mock import patch + +import pytest +from defusedxml.common import EntitiesForbidden + +from crewai_tools.rag.loaders.xml_loader import XMLLoader +from crewai_tools.rag.source_content import SourceContent + + +class TestXMLLoaderSecurity: + """Ensure XMLLoader rejects XML documents that would trigger XXE or entity + expansion attacks when parsed with the standard library. + """ + + def test_rejects_external_entity_expansion(self): + """A document referencing an external entity must not resolve it. + + Using stdlib xml.etree, the entity would either be resolved (leaking + file contents) or silently dropped depending on the parser. defusedxml + raises instead, which is the desired behavior. + """ + malicious = ( + '' + '' + ']>' + '&xxe;' + ) + + loader = XMLLoader() + source = SourceContent(malicious) + + with pytest.raises(EntitiesForbidden): + loader.load(source) + + def test_rejects_billion_laughs(self): + """Nested entity expansion (billion laughs) must be refused.""" + malicious = ( + '' + '' + '' + '' + ']>' + '&lol3;' + ) + + loader = XMLLoader() + source = SourceContent(malicious) + + with pytest.raises(EntitiesForbidden): + loader.load(source) + + def test_parses_safe_xml(self): + """A benign XML document without a DOCTYPE must still parse.""" + safe = 'helloworld' + + loader = XMLLoader() + result = loader.load(SourceContent(safe)) + + assert "hello" in result.content + assert "world" in result.content + assert result.metadata["format"] == "xml" + assert result.metadata["root_tag"] == "root" + + @patch("crewai_tools.rag.loaders.xml_loader.load_from_url") + def test_rejects_xxe_from_url_source(self, mock_load): + """XML fetched from a remote URL is still validated.""" + mock_load.return_value = ( + '' + '' + ']>' + '&xxe;' + ) + + loader = XMLLoader() + source = SourceContent("https://example.com/feed.xml") + + with pytest.raises(EntitiesForbidden): + loader.load(source) From d959e73bda5fe2378f70c771696def48380e871c Mon Sep 17 00:00:00 2001 From: Cameron G Date: Fri, 3 Jul 2026 00:55:00 -0400 Subject: [PATCH 2/3] fix(crewai-tools): contain defusedxml exceptions at loader boundary XMLLoader._parse_xml only caught xml.etree.ParseError, so any DefusedXmlException (EntitiesForbidden / DTDForbidden / ExternalReferenceForbidden) raised while parsing a hostile document would escape the loader and abort the caller. That gives a single malicious XML source the power to break a whole RAG.add() run. Catch DefusedXmlException alongside ParseError and surface it through metadata as 'security_error' (distinct from 'parse_error' so callers can tell malformed XML from actively-refused XML). Content is emptied in the security case since we deliberately did not evaluate any of it. Tests updated to assert the new contract and a new test_malformed_xml_still_returns_parse_error case guards the existing ParseError path so the two error kinds do not get conflated. --- .../crewai_tools/rag/loaders/xml_loader.py | 4 ++ .../tests/rag/test_xml_loader_security.py | 47 +++++++++++++------ 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py b/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py index 1c7f8e08d0..e47ea65bc7 100644 --- a/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py +++ b/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py @@ -2,6 +2,7 @@ from xml.etree.ElementTree import ParseError from defusedxml.ElementTree import fromstring, parse +from defusedxml.common import DefusedXmlException from crewai_tools.rag.base_loader import BaseLoader, LoaderResult from crewai_tools.rag.loaders.utils import load_from_url @@ -53,6 +54,9 @@ def _parse_xml(self, content: str, source_ref: str) -> LoaderResult: text = "\n".join(text_parts) metadata = {"format": "xml", "root_tag": root.tag} + except DefusedXmlException as e: + text = "" + metadata = {"format": "xml", "security_error": str(e)} except ParseError as e: text = content metadata = {"format": "xml", "parse_error": str(e)} diff --git a/lib/crewai-tools/tests/rag/test_xml_loader_security.py b/lib/crewai-tools/tests/rag/test_xml_loader_security.py index b8be8012ba..460312d104 100644 --- a/lib/crewai-tools/tests/rag/test_xml_loader_security.py +++ b/lib/crewai-tools/tests/rag/test_xml_loader_security.py @@ -2,9 +2,6 @@ from unittest.mock import patch -import pytest -from defusedxml.common import EntitiesForbidden - from crewai_tools.rag.loaders.xml_loader import XMLLoader from crewai_tools.rag.source_content import SourceContent @@ -12,14 +9,18 @@ class TestXMLLoaderSecurity: """Ensure XMLLoader rejects XML documents that would trigger XXE or entity expansion attacks when parsed with the standard library. + + The loader keeps its "return a LoaderResult with an error in metadata" + contract for malformed input, so malicious documents are contained at the + loader boundary rather than aborting the caller (e.g. RAG.add). """ def test_rejects_external_entity_expansion(self): """A document referencing an external entity must not resolve it. - Using stdlib xml.etree, the entity would either be resolved (leaking - file contents) or silently dropped depending on the parser. defusedxml - raises instead, which is the desired behavior. + The stdlib xml.etree parser would resolve `SYSTEM` entities (leaking + file contents). defusedxml refuses; XMLLoader captures that and + surfaces `security_error` in metadata. """ malicious = ( '' @@ -30,10 +31,12 @@ def test_rejects_external_entity_expansion(self): ) loader = XMLLoader() - source = SourceContent(malicious) + result = loader.load(SourceContent(malicious)) - with pytest.raises(EntitiesForbidden): - loader.load(source) + assert result.content == "" + assert "security_error" in result.metadata + assert result.metadata["format"] == "xml" + assert "root_tag" not in result.metadata def test_rejects_billion_laughs(self): """Nested entity expansion (billion laughs) must be refused.""" @@ -48,10 +51,11 @@ def test_rejects_billion_laughs(self): ) loader = XMLLoader() - source = SourceContent(malicious) + result = loader.load(SourceContent(malicious)) - with pytest.raises(EntitiesForbidden): - loader.load(source) + assert result.content == "" + assert "security_error" in result.metadata + assert result.metadata["format"] == "xml" def test_parses_safe_xml(self): """A benign XML document without a DOCTYPE must still parse.""" @@ -64,6 +68,19 @@ def test_parses_safe_xml(self): assert "world" in result.content assert result.metadata["format"] == "xml" assert result.metadata["root_tag"] == "root" + assert "security_error" not in result.metadata + + def test_malformed_xml_still_returns_parse_error(self): + """Non-security parse failures still land in `parse_error`, not + `security_error`, so existing consumers keep working. + """ + broken = "" + + loader = XMLLoader() + result = loader.load(SourceContent(broken)) + + assert "parse_error" in result.metadata + assert "security_error" not in result.metadata @patch("crewai_tools.rag.loaders.xml_loader.load_from_url") def test_rejects_xxe_from_url_source(self, mock_load): @@ -77,7 +94,7 @@ def test_rejects_xxe_from_url_source(self, mock_load): ) loader = XMLLoader() - source = SourceContent("https://example.com/feed.xml") + result = loader.load(SourceContent("https://example.com/feed.xml")) - with pytest.raises(EntitiesForbidden): - loader.load(source) + assert result.content == "" + assert "security_error" in result.metadata From 809b14d691c6a4263ddc457386352e34521d8594 Mon Sep 17 00:00:00 2001 From: Cameron G Date: Fri, 3 Jul 2026 01:10:00 -0400 Subject: [PATCH 3/3] fix(crewai-tools): always parse XML content as string in _parse_xml The old dual-path logic in _parse_xml fell back to parse(source_ref) when content did not start with '<'. For any URL source that returned a payload with a BOM prefix, leading whitespace, or an XML declaration that failed the startswith check, source_ref got handed to parse() as a filesystem path and raised an uncaught FileNotFoundError / OSError. load_from_url already produced a string in memory, so there was no reason to reach back through parse() at all. Drop the fallback and always parse the already-loaded content string so error handling stays inside the existing DefusedXmlException / ParseError branches. Remove the now-unused parse import. Adds test_url_payload_with_bom_is_parsed_not_treated_as_path to lock in the BOM-prefixed case. Also tightens the docstring in test_rejects_external_entity_expansion to describe what defusedxml actually does (refuses declared entities) rather than overclaiming that stdlib ElementTree leaks SYSTEM entity contents (it does not; it raises ParseError). --- .../crewai_tools/rag/loaders/xml_loader.py | 7 ++--- .../tests/rag/test_xml_loader_security.py | 28 ++++++++++++++++--- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py b/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py index e47ea65bc7..8f48ea30ff 100644 --- a/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py +++ b/lib/crewai-tools/src/crewai_tools/rag/loaders/xml_loader.py @@ -1,7 +1,7 @@ from typing import Any from xml.etree.ElementTree import ParseError -from defusedxml.ElementTree import fromstring, parse +from defusedxml.ElementTree import fromstring from defusedxml.common import DefusedXmlException from crewai_tools.rag.base_loader import BaseLoader, LoaderResult @@ -42,10 +42,7 @@ def _load_from_file(path: str) -> str: def _parse_xml(self, content: str, source_ref: str) -> LoaderResult: try: - if content.strip().startswith("<"): - root = fromstring(content) - else: - root = parse(source_ref).getroot() + root = fromstring(content) text_parts = [] for text_content in root.itertext(): diff --git a/lib/crewai-tools/tests/rag/test_xml_loader_security.py b/lib/crewai-tools/tests/rag/test_xml_loader_security.py index 460312d104..1f85f55c53 100644 --- a/lib/crewai-tools/tests/rag/test_xml_loader_security.py +++ b/lib/crewai-tools/tests/rag/test_xml_loader_security.py @@ -16,11 +16,12 @@ class TestXMLLoaderSecurity: """ def test_rejects_external_entity_expansion(self): - """A document referencing an external entity must not resolve it. + """A document referencing an external entity must be refused. - The stdlib xml.etree parser would resolve `SYSTEM` entities (leaking - file contents). defusedxml refuses; XMLLoader captures that and - surfaces `security_error` in metadata. + defusedxml refuses to process any declared entity by default, + including this external `SYSTEM` reference. XMLLoader captures + that refusal and surfaces `security_error` in metadata instead + of letting the exception escape into `RAG.add()`. """ malicious = ( '' @@ -82,6 +83,25 @@ def test_malformed_xml_still_returns_parse_error(self): assert "parse_error" in result.metadata assert "security_error" not in result.metadata + @patch("crewai_tools.rag.loaders.xml_loader.load_from_url") + def test_url_payload_with_bom_is_parsed_not_treated_as_path(self, mock_load): + """Content with a BOM or leading whitespace must still be parsed + as XML, not routed to a filesystem `parse(source_ref)` call that + would raise `FileNotFoundError` on a URL source_ref. + """ + payload = ( + "\ufeffbom-safe" + ) + mock_load.return_value = payload + + loader = XMLLoader() + result = loader.load(SourceContent("https://example.com/feed.xml")) + + assert "bom-safe" in result.content + assert result.metadata["root_tag"] == "root" + assert "parse_error" not in result.metadata + assert "security_error" not in result.metadata + @patch("crewai_tools.rag.loaders.xml_loader.load_from_url") def test_rejects_xxe_from_url_source(self, mock_load): """XML fetched from a remote URL is still validated."""