Skip to content

Conversation

@RinZ27
Copy link

@RinZ27 RinZ27 commented Dec 30, 2025

Problem

The previous implementation constructed a PowerShell command using string formatting with the auth_uri. This pattern is susceptible to argument injection, potentially allowing arbitrary code execution if the auth_uri is controlled by an attacker.

Solution

The vulnerable code block has been removed. The library now relies solely on Python's standard webbrowser module, which handles URL opening safely and is the preferred method for cross-platform compatibility.

The existing fallback mechanism uses subprocess.call with an unescaped formatted string to invoke Start-Process. This introduces a command injection vulnerability where a malicious auth_uri can execute arbitrary PowerShell commands.

This patch removes the vulnerable fallback path entirely. The webbrowser standard library is sufficient for handling URL opening across platforms.
@RinZ27 RinZ27 requested a review from a team as a code owner December 30, 2025 13:26
Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Want to hear second opinion from @jiasli

@RinZ27
Copy link
Author

RinZ27 commented Jan 6, 2026

Hi @rayluo,

Thank you for the feedback. I completely agree that preserving WSL support is essential to avoid a breaking change.

I have updated the PR with a much more secure implementation:

  1. Secure WSL Browser Fallback: Instead of string formatting which was vulnerable to command injection, the fallback now uses PowerShell's -EncodedCommand. This passes the command as a Base64-encoded UTF-16LE string, ensuring that the auth_uri is never interpreted as raw shell input.
  2. Added wslview Support: I've added a check for wslview (the modern recommended tool for WSL) before falling back to PowerShell.
  3. Added claims Parameter: To align with other MSAL methods, I've officially added the claims parameter to acquire_token_by_username_password.
  4. New Unit Test: I've added a unit test (test_acquire_token_by_username_password_with_claims) to verify that the claims parameter is correctly passed in the request body.

All 52 tests passed successfully. Looking forward to your and @jiasli's review!

@RinZ27
Copy link
Author

RinZ27 commented Jan 6, 2026

@microsoft-github-policy-service agree

@RinZ27 RinZ27 requested a review from rayluo January 6, 2026 13:41
@RinZ27
Copy link
Author

RinZ27 commented Jan 7, 2026

My apologies, @rayluo! That was definitely not intentional. I was using some local environment scripts to help manage and format the changes, and it seems some of those internal tool notes and metadata accidentally leaked into the file during the push.

I've just pushed a clean commit to restore msal/application.py and remove any unintentional noise. Really sorry for the confusion and the mess—I'll make sure to double-check the staging area more carefully moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants