temp : review Custom Token Exchange support #52
Conversation
Implements token exchange via Exchange Profiles for Auth0 API client. Features: - get_token_by_exchange_profile() method with HTTP Basic auth - Reserved parameter protection (case-insensitive) - DoS protection (array size limits) - Strict token validation (whitespace, Bearer prefix) - Type safety for extra params (str, sequences; rejects dict/set/bytes) - Comprehensive error handling (GetTokenByExchangeProfileError, ApiError) - Lenient expires_in parsing (coerces numeric strings) - Negative expires_in validation Testing: - 129 tests, 86% coverage - Shared test utilities in conftest.py - Table-driven validation tests - Cross-validated against auth0-auth-js Docs: - README with usage examples and Early Access warning - Related SDKs section - Error handling patterns Note: verify_request() now uses split(None, 1) for header parsing. Malformed headers with multiple spaces now raise VerifyAccessTokenError during JWT parsing instead of InvalidAuthSchemeError. Standard inputs unaffected.
kishore7snehil
changed the title
|
Claude finished @kishore7snehil's task —— View job Review Complete
Overall Assessment✅ This pull request is ready to be merged after addressing the inline comments. The Custom Token Exchange (RFC 8693) implementation is well-designed with strong security practices, comprehensive input validation, and excellent test coverage. The code follows RFC 8693 specifications correctly and includes appropriate safeguards against common vulnerabilities. Key Strengths:
Issues Identified (see inline comments for details):
No blocking security issues were found. The implementation demonstrates strong security awareness with proper credential handling, input sanitization, and protection against injection attacks. All inline comments are recommendations for improvements rather than critical fixes. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # Require client credentials | ||
| client_id = self.options.client_id | ||
| client_secret = self.options.client_secret | ||
| if not client_id or not client_secret: |
There was a problem hiding this comment.
The client credentials check occurs after input validation. While this ordering is correct for fail-fast validation, consider if credential presence should be verified earlier to avoid processing potentially sensitive tokens when the request will ultimately fail due to missing credentials. Current implementation is acceptable as token validation is lightweight.
| if "json" in response.headers.get("content-type", "").lower(): | ||
| error_data = response.json() | ||
| except ValueError: | ||
| pass # Ignore JSON parse errors, use generic error message below |
There was a problem hiding this comment.
The JSON parsing error handling silently swallows exceptions with pass. While this is acceptable for gracefully handling malformed error responses, consider logging these parse failures for debugging purposes, especially since they indicate the Auth0 endpoint returned invalid JSON in an error scenario.
| except (TypeError, ValueError): | ||
| raise ApiError("invalid_response", "expires_in is not an integer.", 502) | ||
|
|
||
| if expires_in < 0: |
There was a problem hiding this comment.
Negative expires_in validation is good for preventing immediately-expired tokens. However, consider also validating for unreasonably large values (e.g., > 1 year) which could indicate a malformed response or potential integer overflow issues in downstream systems that calculate expiry timestamps.
| f"Parameter '{k}' exceeds maximum array size of {MAX_ARRAY_VALUES_PER_KEY}" | ||
| ) | ||
| # Convert sequence items to strings | ||
| params[key] = [str(x) for x in v] |
There was a problem hiding this comment.
Converting all sequence items to strings with [str(x) for x in v] could mask issues if the sequence contains objects without proper __str__ methods. Consider validating that items are primitive types (str, int, float, bool) before conversion to prevent unexpected serialization behavior.
| MAX_ARRAY_VALUES_PER_KEY = 20 # DoS protection for extra parameter arrays | ||
|
|
||
| # OAuth parameter denylist - parameters that cannot be overridden via extras | ||
| RESERVED_PARAMS = frozenset([ |
There was a problem hiding this comment.
The reserved parameters list is comprehensive, but consider if resource vs resources distinction is necessary. RFC 8693 uses resource (singular), while the list includes both. Verify this matches Auth0's actual parameter handling to avoid blocking legitimate use cases.
|
|
||
| # Make token exchange request | ||
| try: | ||
| async with httpx.AsyncClient(timeout=httpx.Timeout(self.options.timeout)) as client: |
There was a problem hiding this comment.
The timeout is configured here but not in the get_access_token_for_connection method at line 474. This inconsistency means connection token exchange has no configurable timeout and uses httpx defaults. Consider applying the same timeout configuration to both methods for consistency.
|
|
||
| **Important:** | ||
| - Client authentication is sent via HTTP Basic (`client_id`/`client_secret`), not in the form body. | ||
| - Do not prefix `subject_token` with "Bearer " - send the raw token value only. |
There was a problem hiding this comment.
The warning states "Extra parameters are sent as form fields and may appear in logs. Do not include secrets or sensitive data." Consider strengthening this warning to explicitly mention that these parameters may also appear in Auth0 logs, monitoring systems, and potentially in error messages returned to clients.
| tok = subject_token | ||
| if not isinstance(tok, str) or not tok.strip(): | ||
| raise GetTokenByExchangeProfileError("subject_token cannot be blank or whitespace") | ||
| if tok != tok.strip(): |
There was a problem hiding this comment.
The whitespace stripping check uses .strip() to detect leading/trailing whitespace. This rejects tokens with legitimate trailing/leading whitespace that might be part of the token format. However, this is likely intentional to prevent common copy-paste errors. Consider documenting this behavior in the docstring.
| for k, v in extra.items(): | ||
| key = str(k) | ||
| # Case-insensitive check against reserved params | ||
| if key.lower() in reserved_lower: |
There was a problem hiding this comment.
The case-insensitive check for reserved parameters uses key.lower() which is correct. However, the error message uses the original key k instead of the normalized key. This is actually good UX as it shows the user's exact input, but ensure this doesn't cause confusion if mixed-case variations exist in RESERVED_PARAMS (currently all lowercase).
| if response.status_code != 200: | ||
| error_data = {} | ||
| try: | ||
| if "json" in response.headers.get("content-type", "").lower(): |
There was a problem hiding this comment.
Same Content-Type parsing issue as in get_access_token_for_connection. The check if "json" in response.headers.get("content-type", "").lower() is fragile. Consider using response.headers.get("content-type", "").startswith("application/json") or leveraging httpx's response type detection.
| result = { | ||
| "access_token": access_token, | ||
| "expires_in": expires_in, | ||
| "expires_at": int(time.time()) + expires_in, |
There was a problem hiding this comment.
The calculation int(time.time()) + expires_in could theoretically overflow for extremely large expires_in values (though validated as int). In Python 3, integers have arbitrary precision, so this is safe. However, downstream systems converting to 32-bit timestamps could overflow. The earlier suggested validation for maximum expires_in would prevent this.
|
|
||
| # Token Exchange constants | ||
| TOKEN_EXCHANGE_GRANT_TYPE = "urn:ietf:params:oauth:grant-type:token-exchange" # noqa: S105 | ||
| MAX_ARRAY_VALUES_PER_KEY = 20 # DoS protection for extra parameter arrays |
There was a problem hiding this comment.
The constant MAX_ARRAY_VALUES_PER_KEY = 20 provides DoS protection, which is good. However, this limit is somewhat arbitrary and not documented in the method's docstring. Consider documenting this limit in the get_token_by_exchange_profile method documentation so users are aware of this constraint.
| audience="<AUTH0_AUDIENCE>", | ||
| client_id="<AUTH0_CLIENT_ID>", | ||
| client_secret="<AUTH0_CLIENT_SECRET>", | ||
| timeout=10.0 # Optional: HTTP timeout in seconds (default: 10.0) |
There was a problem hiding this comment.
The important note states "Do not prefix subject_token with 'Bearer '" which is validated in code (line 597-600 in api_client.py). However, the validation is case-insensitive (token.lower().startswith("bearer ")), which is good. Consider clarifying in docs that this check is case-insensitive.
| client_secret: Optional required if you want to use get_access_token_for_connection. | ||
| client_id: Required for get_access_token_for_connection and get_token_by_exchange_profile. | ||
| client_secret: Required for get_access_token_for_connection and get_token_by_exchange_profile. | ||
| timeout: HTTP timeout in seconds for token endpoint requests (default: 10.0). |
There was a problem hiding this comment.
The docstring mentions timeout defaults to 10.0 seconds, and the parameter has timeout: float = 10.0. However, consider if this timeout should also apply to OIDC discovery and JWKS fetching operations, or only to token endpoint calls. Currently it only applies to token endpoint operations.
| ) | ||
|
|
||
| # Handle sequences (list, tuple, etc.) but reject mappings/sets/bytes | ||
| if isinstance(v, (dict, set, bytes)): |
There was a problem hiding this comment.
The type check isinstance(v, (dict, set, bytes)) rejects these types, which is good. However, the error message mentions the type name but doesn't explain why these types are unsupported. Consider enhancing the error message to say something like "unsupported type {type} - only strings, numbers, and sequences are allowed".
| raise GetTokenByExchangeProfileError( | ||
| "subject_token must not include leading or trailing whitespace" | ||
| ) | ||
| if tok.lower().startswith("bearer "): |
There was a problem hiding this comment.
The Bearer prefix validation is case-insensitive which is good. However, the error message says "must not include the 'Bearer ' prefix" (with capital B). Consider matching the case in the error message to what was actually detected, or clarifying that the check is case-insensitive to avoid confusion.
| client_id="<AUTH0_CLIENT_ID>", | ||
| client_secret="<AUTH0_CLIENT_SECRET>", | ||
| timeout=10.0 # Optional: HTTP timeout in seconds (default: 10.0) | ||
| )) |
There was a problem hiding this comment.
The documentation states "This URI identifies which profile will process the exchange and must not use reserved OAuth namespaces (IETF or vendor-controlled)". While this is excellent guidance, consider adding validation in the code to reject common reserved namespaces like urn:ietf:params:oauth:* to fail fast and provide better error messages to developers.
| "expires_at": int(time.time()) + expires_in, | ||
| } | ||
|
|
||
| # Add optional fields if present (preserves falsy values like empty scope) |
There was a problem hiding this comment.
The code preserves optional fields including falsy values with the comment "preserves falsy values like empty scope". This is good design for RFC 8693 compliance where an empty scope string has different meaning than no scope field. Consider adding a test case to verify this behavior is maintained.
| return "get_access_token_for_connection_error" | ||
|
|
||
|
|
||
| class GetTokenByExchangeProfileError(BaseAuthError): |
There was a problem hiding this comment.
The GetTokenByExchangeProfileError class returns a 400 status code. However, some scenarios (like missing credentials) might be more appropriately classified as 401 (Unauthorized) or 500 (Internal Server Error - configuration issue). Consider if different error scenarios should return different status codes.
| ) | ||
| elif isinstance(v, (list, tuple)): | ||
| if len(v) > MAX_ARRAY_VALUES_PER_KEY: | ||
| raise GetTokenByExchangeProfileError( |
There was a problem hiding this comment.
The array size limit check happens before string conversion. If an attacker provides 21+ complex objects with expensive __str__ methods, those conversions would still execute before the limit check fails. Consider checking array length before iterating for string conversion to prevent potential DoS via expensive string conversions.
| ) | ||
|
|
||
| # Result contains access_token, expires_in, expires_at | ||
| # id_token, refresh_token, and scope are profile/Action dependent (not guaranteed; scope may be empty) |
There was a problem hiding this comment.
The code example shows extra usage with device_id and session_id. These look like potentially sensitive identifiers. Consider adding a note that even non-secret identifiers may have privacy implications if logged, especially in jurisdictions with strict data protection laws (GDPR, CCPA, etc.).
| from .errors import ApiError, GetTokenByExchangeProfileError | ||
|
|
||
| __all__ = [ | ||
| "ApiClient", |
There was a problem hiding this comment.
The new GetTokenByExchangeProfileError is exported in the public API. Consider if this should be documented in a CHANGELOG or migration guide, especially if this is part of a major version release, so users know about the new exception type they should handle.
| response = await client.post( | ||
| token_endpoint, | ||
| data=params, | ||
| auth=(client_id, client_secret) |
There was a problem hiding this comment.
Client credentials are sent via HTTP Basic Auth (line 646), which is RFC 8693 compliant. However, ensure the underlying httpx library properly encodes the credentials. Special characters in client_id or client_secret (like colons) could cause parsing issues. The httpx library should handle this, but consider adding a note in documentation about special character handling.
|
|
||
| try: | ||
| token_response = response.json() | ||
| except ValueError: |
There was a problem hiding this comment.
The JSON parsing of the successful response doesn't wrap in try-except like the error response parsing does (line 651-655). If Auth0 returns a 200 status with malformed JSON, this will raise an unhandled ValueError. Consider wrapping this in try-except and raising an ApiError for consistency.
|
|
||
| # Validate required fields | ||
| access_token = token_response.get("access_token") | ||
| if not isinstance(access_token, str) or not access_token: |
There was a problem hiding this comment.
The validation if not isinstance(access_token, str) or not access_token correctly checks type and emptiness. However, consider also validating that the token doesn't contain only whitespace (similar to the subject_token validation) to catch malformed responses early.
| - Migrating to Auth0 | ||
|
|
||
| ```python | ||
| import asyncio |
There was a problem hiding this comment.
The example shows accessing result["access_token"] directly without error handling. For production code examples, consider adding a brief note about handling ApiError and GetTokenByExchangeProfileError exceptions, or showing the pattern demonstrated later (lines 175-186).
| # Reject negative values (prevent accidental "already expired" tokens) | ||
| expires_in_raw = token_response.get("expires_in", 3600) | ||
| try: | ||
| expires_in = int(expires_in_raw) |
There was a problem hiding this comment.
The coercion of expires_in to int with int(expires_in_raw) will truncate float values like 3600.5 to 3600. While this is acceptable and documented with "Lenient policy: coerce numeric strings", consider if rounding (rather than truncating) would be more appropriate for float values to avoid premature expiration.
| async def get_token_by_exchange_profile( | ||
| self, | ||
| subject_token: str, | ||
| subject_token_type: str, |
There was a problem hiding this comment.
The subject_token parameter accepts a raw string token. While the validation prevents Bearer prefix, there's no validation of token format (JWT structure, base64 encoding, etc.). This is intentional since custom tokens may have arbitrary formats, but consider documenting that malformed tokens will only be caught by Auth0's endpoint, not locally.
This is a temporary PR to run automated Claude Code review on PR #51.
Original PR: #51 by @btiernay
This PR will be closed after review is complete.
Reference: