o-nikolas commented on code in PR #71423:
URL: https://github.com/apache/airflow/pull/71423#discussion_r3984563415
##########
providers/amazon/tests/unit/amazon/aws/hooks/test_base_aws.py:
##########
@@ -812,6 +812,74 @@ def mock_assume_role_with_saml(**kwargs):
),
]
+ def test_saml_assertion_parsing_does_not_resolve_external_entities(self,
tmp_path):
+ secret_file = tmp_path / "secret.txt"
+ secret_file.write_text("TOPSECRET")
+ xxe_response = (
+ '<?xml version="1.0"?>'
+ f'<!DOCTYPE r [<!ENTITY xxe SYSTEM "file://{secret_file}">]>'
+ "<r><assertion>&xxe;</assertion></r>"
+ ).encode()
+
+ saml_config = {
+ "idp_url": "https://my-idp.local.corp";,
+ "idp_auth_method": "http_spegno_auth",
+ "saml_response_xpath": "//assertion/text()",
+ }
+
+ orig_import = __import__
+
+ def import_mock(name, *args, **kwargs):
+ if name == "requests_gssapi":
+ return mock.Mock()
+ return orig_import(name, *args, **kwargs)
+
+ factory = BaseSessionFactory(conn=None)
+ with (
+ mock.patch("builtins.__import__", side_effect=import_mock),
+ mock.patch.object(factory, "_get_idp_response") as mock_idp,
+ ):
+ mock_idp.return_value.content = xxe_response
+ try:
+ assertion =
factory._fetch_saml_assertion_using_http_spegno_auth(saml_config)
+ except ValueError:
+ assertion = ""
+
+ assert "TOPSECRET" not in str(assertion)
+
+ def test_saml_assertion_parsing_does_not_resolve_internal_entities(self):
Review Comment:
This is the genuine regression test — it yields `'INJECTED'` on main and
`[]` with the fix, so it fails without the change for the right reason.
Since it and the external-entity test differ only in payload and expected
substring, they'd be better consolidated into one `@pytest.mark.parametrize`d
test (AGENTS.md asks for parametrize when tests differ only in input/expected
values).
##########
providers/amazon/tests/unit/amazon/aws/hooks/test_base_aws.py:
##########
@@ -812,6 +812,74 @@ def mock_assume_role_with_saml(**kwargs):
),
]
+ def test_saml_assertion_parsing_does_not_resolve_external_entities(self,
tmp_path):
+ secret_file = tmp_path / "secret.txt"
+ secret_file.write_text("TOPSECRET")
+ xxe_response = (
+ '<?xml version="1.0"?>'
+ f'<!DOCTYPE r [<!ENTITY xxe SYSTEM "file://{secret_file}">]>'
+ "<r><assertion>&xxe;</assertion></r>"
+ ).encode()
+
+ saml_config = {
+ "idp_url": "https://my-idp.local.corp";,
+ "idp_auth_method": "http_spegno_auth",
+ "saml_response_xpath": "//assertion/text()",
+ }
+
+ orig_import = __import__
+
+ def import_mock(name, *args, **kwargs):
+ if name == "requests_gssapi":
+ return mock.Mock()
+ return orig_import(name, *args, **kwargs)
+
+ factory = BaseSessionFactory(conn=None)
+ with (
+ mock.patch("builtins.__import__", side_effect=import_mock),
+ mock.patch.object(factory, "_get_idp_response") as mock_idp,
+ ):
+ mock_idp.return_value.content = xxe_response
+ try:
+ assertion =
factory._fetch_saml_assertion_using_http_spegno_auth(saml_config)
+ except ValueError:
Review Comment:
`etree.XMLSyntaxError` derives from `SyntaxError`, not `ValueError`, so on
`main` this test *errors* rather than fails — and it errors on "entity not
defined", not on a file disclosure. It passes with the fix only because the
empty xpath result trips `raise ValueError("Invalid SAML Assertion")` a few
lines down in the hook, which this `except` then swallows. So the test doesn't
exercise the scenario it's named for.
Separately, `except ValueError: assertion = ""` followed by `assert
"TOPSECRET" not in str(assertion)` is vacuously true for *any* ValueError
raised anywhere in the call. Prefer `pytest.raises` on the specific expected
outcome.
Suggest either dropping this test or rewriting it to assert the hardening
directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
