Use secrets module for secure random token generation#898
Closed
Conversation
…eration Replace random.sample() with secrets.choice() for generating PKCE code verifiers, OAuth2 state parameters, and OIDC nonces. The random module uses Mersenne Twister which is not cryptographically secure. The secrets module uses os.urandom(), providing a CSPRNG suitable for security tokens. This also fixes a subtle entropy reduction caused by random.sample() drawing without replacement, which prevented character repetition.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the security of auth-code-flow protections in msal.oauth2cli by switching token/parameter generation from random.sample() (Mersenne Twister, non-CSPRNG) to secrets.choice() (CSPRNG), and also removes the “no replacement” sampling behavior that unnecessarily reduced the effective search space.
Changes:
- Replace nonce generation in OIDC auth code flow with
secrets.choice(...). - Replace PKCE code verifier generation with
secrets.choice(...)over an RFC7636-compliant alphabet. - Replace OAuth2
stategeneration withsecrets.choice(...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
msal/oauth2cli/oidc.py |
Switch OIDC nonce generation to secrets for replay-attack mitigation strength. |
msal/oauth2cli/oauth2.py |
Switch PKCE verifier and OAuth2 state generation to secrets, keeping RFC7636 character requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
CoPilot suggest adding this optional test, otherwise looks good # tests/test_oauth2_security.py - New test file (suggested)
import unittest
from msal.oauth2cli import oauth2
import string
class TestTokenGenerationSecurity(unittest.TestCase):
"""Verify secure token generation using secrets module."""
def test_pkce_verifier_format(self):
"""Verify PKCE verifier is valid and uses correct alphabet."""
pkce = oauth2._generate_pkce_code_verifier(length=43)
verifier = pkce["code_verifier"]
# Check length
self.assertEqual(len(verifier), 43)
# Check alphabet compliance (RFC 7636)
allowed_chars = set(string.ascii_letters + string.digits + "-._~")
self.assertTrue(all(c in allowed_chars for c in verifier))
# Verify code_challenge exists and is base64url encoded (no padding)
self.assertIn("code_challenge", pkce)
self.assertNotIn(b"=", pkce["code_challenge"])
def test_pkce_verifier_custom_length(self):
"""Test PKCE verifier with valid custom lengths."""
for length in [43, 64, 128]:
pkce = oauth2._generate_pkce_code_verifier(length=length)
self.assertEqual(len(pkce["code_verifier"]), length)
def test_token_uniqueness(self):
"""Verify generated tokens are unique (randomness test)."""
# Generate multiple tokens - should all be different
verifiers = [oauth2._generate_pkce_code_verifier()["code_verifier"]
for _ in range(10)]
self.assertEqual(len(set(verifiers)), 10, "Generated tokens should be unique")
# Also test state generation
from msal.oauth2cli.oauth2 import Client
from unittest.mock import Mock
client = Client(
server_configuration={"authorization_endpoint": "https://example.com/auth"},
client_id="test-client",
http_client=Mock()
)
states = [client.initiate_auth_code_flow(scope=["test"])["state"]
for _ in range(10)]
self.assertEqual(len(set(states)), 10, "Generated states should be unique")
if __name__ == '__main__':
unittest.main() |
gladjohn
approved these changes
Apr 7, 2026
gladjohn
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace random.sample() with secrets.choice() for generating PKCE code verifiers, OAuth2 state parameters, and OIDC nonces.
Why: random uses Mersenne Twister (not cryptographically secure). secrets uses os.urandom() (CSPRNG), preventing prediction-based attacks. Also fixes an entropy reduction from random.sample() drawing without replacement.
Risk: Low — secrets requires Python 3.6+ (already minimum). No public API changes