Skip to content

Escape username parameter #901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DharshanBJ merged 2 commits into from
Apr 10, 2026
Merged

Escape username parameter #901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c2e700
Escape username to protect against xml injection vulnerability
DharshanBJ Apr 8, 2026
4628770
Add test
DharshanBJ Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions msal/wstrust_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ def send_request(
return parse_response(resp.text)


def escape_password(password):
return (password.replace('&', '&').replace('"', '"')
def escape_xml(s):
return (s.replace('&', '&').replace('"', '"')
.replace("'", ''') # the only one not provided by cgi.escape(s, True)
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
.replace("'", ''') # the only one not provided by cgi.escape(s, True)
.replace("'", ''') # Include apostrophes explicitly for XML attribute/content safety

Copilot uses AI. Check for mistakes.
.replace('<', '&lt;').replace('>', '&gt;'))

Expand Down Expand Up @@ -116,7 +116,7 @@ def _build_rst(username, password, cloud_audience_urn, endpoint_address, soap_ac
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,
Comment on lines 116 to 121
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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).

Copilot uses AI. Check for mistakes.
key_type='http://docs.oasis-open.org/ws-sx/ws-trust/200512/Bearer'
Expand Down
14 changes: 14 additions & 0 deletions tests/test_wstrust.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from xml.etree import ElementTree as ET
import os

from msal.wstrust_request import _build_rst, escape_xml
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
from msal.wstrust_response import *

from tests import unittest
Expand Down Expand Up @@ -96,3 +97,16 @@ def test_token_parsing_happy_path(self):
self.assertEqual(result.get("type"), SAML_TOKEN_TYPE_V1)
self.assertIn(b"<saml:Assertion", result.get("token", ""))


class Test_WsTrustRequest(unittest.TestCase):
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test class naming is inconsistent with common unittest conventions (CamelCase without underscores). Consider renaming to TestWsTrustRequest for consistency and discoverability.

Suggested change
class Test_WsTrustRequest(unittest.TestCase):
class TestWsTrustRequest(unittest.TestCase):

Copilot uses AI. Check for mistakes.

def test_escape_xml(self):
self.assertEqual(escape_xml('<>&"\''), '&lt;&gt;&amp;&quot;&apos;')

def test_username_xml_injection_is_prevented(self):
malicious = 'admin</wsse:Username><x>INJECTED'
rst = _build_rst(malicious, 'pw', 'urn:x', 'https://x',
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
'http://docs.oasis-open.org/ws-sx/ws-trust/200512/RST/Issue')
self.assertEqual(rst.count('<wsse:Username>'), 1)
self.assertNotIn('<x>INJECTED', rst)

Loading