Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses an XML injection risk in WS-Trust RST construction by escaping untrusted values placed into the SOAP XML payload.
Changes:
- Introduces
escape_xml()and uses it when insertingusernameandpasswordinto the WS-Trust RST XML. - Adds unit tests to validate XML escaping and prevent username-based XML injection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
msal/wstrust_request.py |
Adds a general XML escaping helper and applies it to username/password in the RST builder. |
tests/test_wstrust.py |
Adds tests for escaping and a regression test for username-based XML injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| endpoint_address=endpoint_address, | ||
| time_now=wsu_time_format(now), | ||
| time_expire=wsu_time_format(now + timedelta(minutes=10)), | ||
| username=username, password=escape_password(password), | ||
| username=escape_xml(username), password=escape_xml(password), | ||
| wst=Mex.NS["wst"] if soap_action == Mex.ACTION_13 else Mex.NS["wst2005"], | ||
| applies_to=cloud_audience_urn, |
There was a problem hiding this comment.
_build_rst() now escapes username/password, but still interpolates other RST fields (e.g., endpoint_address and cloud_audience_urn via applies_to) into XML without escaping. This can still allow XML injection or malformed XML if those values contain <, &, etc. Apply escape_xml() to these interpolated text-node values too (or switch to building the XML via an XML library like ElementTree to avoid manual escaping entirely).
| return (password.replace('&', '&').replace('"', '"') | ||
| def escape_xml(s): | ||
| return (s.replace('&', '&').replace('"', '"') | ||
| .replace("'", ''') # the only one not provided by cgi.escape(s, True) |
There was a problem hiding this comment.
The inline comment references cgi.escape, which has been deprecated/removed in modern Python and is misleading here. Update the comment to reference an appropriate XML escaping utility (e.g., xml.sax.saxutils.escape) or remove the historical note to avoid confusion.
| .replace("'", ''') # the only one not provided by cgi.escape(s, True) | |
| .replace("'", ''') # Include apostrophes explicitly for XML attribute/content safety |
| self.assertIn(b"<saml:Assertion", result.get("token", "")) | ||
|
|
||
|
|
||
| class Test_WsTrustRequest(unittest.TestCase): |
There was a problem hiding this comment.
Test class naming is inconsistent with common unittest conventions (CamelCase without underscores). Consider renaming to TestWsTrustRequest for consistency and discoverability.
| class Test_WsTrustRequest(unittest.TestCase): | |
| class TestWsTrustRequest(unittest.TestCase): |
| from xml.etree import ElementTree as ET | ||
| import os | ||
|
|
||
| from msal.wstrust_request import _build_rst, escape_xml |
There was a problem hiding this comment.
The tests import and call _build_rst, a private (underscore-prefixed) helper. This makes the test more brittle to internal refactors. If this behavior is intended to be stable, consider making it a supported helper (public name), or alternatively test via a public API boundary with mocking around I/O.
|
|
||
| def test_username_xml_injection_is_prevented(self): | ||
| malicious = 'admin</wsse:Username><x>INJECTED' | ||
| rst = _build_rst(malicious, 'pw', 'urn:x', 'https://x', |
There was a problem hiding this comment.
The tests import and call _build_rst, a private (underscore-prefixed) helper. This makes the test more brittle to internal refactors. If this behavior is intended to be stable, consider making it a supported helper (public name), or alternatively test via a public API boundary with mocking around I/O.
DharshanBJ
changed the title
No description provided.