Feature/add sensitive metadata #414
Conversation
WalkthroughThis pull request introduces the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 49 50 +1
Lines 1095 1118 +23
=========================================
+ Hits 1095 1118 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
cuenca/resources/otps.py (1)
Line range hint
15-19: Security: Remove sensitive data from example.The example configuration shows a full
secretvalue. Even though it's a dummy value, it's recommended to use a placeholder or masked value in examples to reinforce security best practices.'example': { 'id': 'OTNEUInh69SuKXXmK95sROwQ', - 'secret': 'somesecret', + 'secret': '****', }
🧹 Nitpick comments (4)
tests/resources/test_jwt_tokens.py (1)
20-28: Enhance token security validation.The test should include additional assertions to verify token security properties:
- Token format validation
- Token expiration
- Token length requirements
jwt_token = JwtToken.create(session=session) assert jwt_token assert isinstance(jwt_token.token, str) + # Verify token format (e.g., JWT has three parts) + assert len(jwt_token.token.split('.')) == 3 + # Verify token meets minimum length requirement + assert len(jwt_token.token) >= 100 + # Verify token expiration exists and is in future + assert jwt_token.expires_at > datetime.datetime.now(datetime.timezone.utc)Also consider using parameterized testing for different user scenarios.
cuenca/__init__.py (1)
45-45: Maintain alphabetical ordering in exports and imports.The
JwtTokenadditions should maintain the alphabetical ordering convention.Move the entries to their correct alphabetical positions:
- 'JwtToken', + 'JwtToken', # Move after 'IdentityEvent' in both listsAlso applies to: 69-69
cuenca/resources/__init__.py (1)
41-41: Maintain alphabetical ordering in resource declarations.The
JwtTokenadditions should maintain the alphabetical ordering convention.Move the entries to their correct alphabetical positions:
# In __all__ - 'JwtToken', + 'JwtToken', # Move after 'IdentityEvent' # In imports -from .jwt_tokens import JwtToken +from .jwt_tokens import JwtToken # Move after .identity_events # In resource_classes - JwtToken, + JwtToken, # Move after IdentityEventAlso applies to: 63-63, 128-128
tests/resources/cassettes/test_jwt_tokens.yaml (1)
3-3: Enhance security of test credentials.Using weak, hardcoded passwords in test fixtures can lead to security issues if accidentally used in production. Consider:
- Using environment variables for test credentials
- Using a stronger password pattern even in test environments
- body: '{"password": "111111"}' + body: '{"password": "${TEST_USER_PASSWORD}"}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cuenca/__init__.py(2 hunks)cuenca/resources/__init__.py(3 hunks)cuenca/resources/api_keys.py(2 hunks)cuenca/resources/jwt_tokens.py(1 hunks)cuenca/resources/login_tokens.py(2 hunks)cuenca/resources/otps.py(2 hunks)cuenca/resources/sessions.py(2 hunks)cuenca/version.py(1 hunks)requirements.txt(1 hunks)tests/resources/cassettes/test_jwt_tokens.yaml(1 hunks)tests/resources/test_jwt_tokens.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cuenca/version.py
- requirements.txt
🔇 Additional comments (8)
cuenca/resources/otps.py (1)
12-12: LGTM! Proper implementation of sensitive data handling.The
secretfield is correctly marked as sensitive with appropriate logging restrictions.cuenca/resources/login_tokens.py (2)
13-13: LGTM! Proper implementation of sensitive data handling.The
idfield is correctly marked as sensitive with appropriate logging restrictions.
Line range hint
21-27: Verify token expiration handling.The documentation mentions a 7-day expiration period, but there's no validation in the code to ensure this constraint.
✅ Verification successful
Token expiration is properly handled through JWT validation
The 7-day expiration mentioned in the documentation is enforced through the JWT layer, which includes proper expiration validation via
is_expiredproperty andget_expiration_datemethod. The actual expiration period is set by the backend during token creation, while the client-side JWT implementation ensures tokens are validated against their expiration before use.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token expiration validation in the codebase rg -A 5 "expires_at|expiration|valid_for" --type pyLength of output: 4772
Script:
#!/bin/bash # Search for token creation implementation ast-grep --pattern 'def _create($$$)' # Search for token creation with expiration or duration settings rg -A 5 "token.*days|days.*token" --type pyLength of output: 488
Script:
#!/bin/bash # View complete implementation of login_tokens.py cat cuenca/resources/login_tokens.pyLength of output: 826
Script:
#!/bin/bash # View Creatable base class implementation cat cuenca/resources/base.pyLength of output: 5772
cuenca/resources/sessions.py (1)
15-15: LGTM! Proper implementation of sensitive data handling.The
idfield is correctly marked as sensitive with appropriate logging restrictions.cuenca/resources/jwt_tokens.py (1)
14-15: LGTM! Proper handling of sensitive data.The sensitive fields
idandtokenare correctly marked withMetadata(sensitive=True, log_chars=4), ensuring only the first 4 characters are logged.cuenca/resources/api_keys.py (1)
15-15: LGTM! Proper handling of API key secret.The
secretfield is correctly marked as sensitive with appropriate logging restrictions.tests/resources/cassettes/test_jwt_tokens.yaml (2)
46-91: LGTM! Proper authentication flow implementation.The login token generation follows security best practices:
- Includes necessary authentication headers
- Uses proper session context (X-Cuenca-LoginId)
- Returns minimal required information
111-111: Protect sensitive JWT token data in test fixtures.The response contains sensitive JWT token data that should be handled carefully:
- Consider masking or truncating the JWT token in test fixtures
- The token contains sensitive claims (sub, uid) that should be marked as sensitive metadata
- Consider using shorter expiration times in test environments
Run this script to check for other instances of exposed JWT tokens in test fixtures:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cuenca/resources/user_logins.py (1)
15-15: Consider adding field-level documentation.While the field is documented in the model's example, consider adding a docstring or field description to explicitly document the masking behavior and purpose of the ID field.
- id: Annotated[str, LogConfig(masked=True, unmasked_chars_length=4)] + id: Annotated[str, LogConfig(masked=True, unmasked_chars_length=4)] # Unique login identifier, masked except last 4 chars
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cuenca/resources/jwt_tokens.py(1 hunks)cuenca/resources/otps.py(2 hunks)cuenca/resources/user_logins.py(2 hunks)tests/resources/test_jwt_tokens.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/resources/test_jwt_tokens.py
- cuenca/resources/otps.py
- cuenca/resources/jwt_tokens.py
🔇 Additional comments (3)
cuenca/resources/user_logins.py (3)
2-2: LGTM! Necessary imports added for sensitive data handling.The addition of
AnnotatedandLogConfigimports supports the new sensitive field masking functionality.Also applies to: 4-4
15-15: Verify consistent masking configuration across related models.The masking configuration for the
idfield looks good, showing only the last 4 characters for security while maintaining usability.Let's verify that similar ID fields across related models use consistent masking configurations:
✅ Verification successful
ID field masking configuration is consistent across related models ✓
All authentication-related resources (
user_logins,sessions,login_tokens,jwt_tokens) use identical masking configuration showing only the last 4 characters of IDs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other ID field declarations with LogConfig ast-grep --pattern 'id: Annotated[$_, LogConfig($$$)]'Length of output: 481
Line range hint
34-37: Review security implications of ID in headers.The
idfield is used in theX-Cuenca-LoginIdheader. While the field itself is properly masked in logs, verify that:
- The header is used over HTTPS only
- The header is not exposed in any logging middleware
Let's check for any security-related configurations and header logging:
Also applies to: 45-48
✅ Verification successful
Security review completed: Implementation is secure ✅
The
X-Cuenca-LoginIdheader is properly protected:
- HTTPS is strictly enforced in
client.py- Headers are managed securely within the requests session
- No logging middleware exposes the headers
- Proper cleanup during logout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for HTTPS enforcement rg -l "https|ssl|tls" --type python # Check for header logging in middleware rg -l "middleware|logging.*header" --type pythonLength of output: 150
Script:
#!/bin/bash # Check for HTTPS/TLS enforcement and configurations rg -l "https|ssl|tls|verify" --type py # Check for middleware and header logging rg -l "middleware|logging.*header|header.*log" --type py # Search for specific header usage rg "X-Cuenca-LoginId" --type pyLength of output: 1081
Script:
#!/bin/bash # Check HTTP client implementation for security configurations cat cuenca/http/client.py # Look for any logging configurations related to headers rg -A 5 "log.*header|header.*log" --type pyLength of output: 5314
Description
Added
LogConfigto mark sensitive fields in logs, and introduced theJwtTokenresponse model for/tokenChanges
cuenca-validations, which now includes theLogConfigclass for marking sensitive fields in logs.LogConfigclass, ensuring that they are properly masked in logs.JwtTokenclass, a response model for the/tokenendpoint, where sensitive fields are explicitly marked usingLogConfig.Summary by CodeRabbit
Release Notes
New Features
JwtTokenresource for generating authentication tokens.idfield to theUserLoginclass for enhanced security.Improvements
ApiKey,Otp,Session, andCard.Dependency Updates
cuenca-validationspackage to development version 2.0.5.dev5.Version