Samin061 commented on PR #71423:
URL: https://github.com/apache/airflow/pull/71423#issuecomment-5925950576
Pushed an update addressing the review. Agreed on the framing, that read is
right.
- Reframed the in-code comment, commit message and PR description as
defence-in-depth, dropped the exploit claim, and spelled out that
`no_network=True`/`load_dtd=False` already match lxml's defaults so
`resolve_entities=False` is the only flag that changes behaviour. Squashed to
one commit so the git-log line that lands in the changelog reads as hardening,
not a past XXE.
- Called out the behaviour change (internal literal entity now raises
`ValueError("Invalid SAML Assertion")`) in the description and commit.
- Rewrote the tests into one parametrized
`test_fetch_saml_assertion_does_not_resolve_doctype_entities` that asserts the
`ValueError` outcome directly, added `autospec=True` on the `_get_idp_response`
patch, and moved it into `TestSessionFactory` with the other direct-factory
tests.
- Good point on the lxml floor via the `requests_gssapi` path. It doesn't
change the no-vuln read but it's another reason to pin the parser rather than
trust whatever lxml the environment ships.
I couldn't run the full suite locally (the uv workspace won't resolve
metadata on my machine, unrelated to this change), but I verified the
parse/xpath path against lxml directly and it reproduces your table: both
payloads hit the `ValueError` with the fix, and on `main` the internal one
returns the literal while the external errors, so both fail on revert. Also
ticked the AI-use box on the description.
--
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]
