attempted to fix the email injection issue #9139
Conversation
Reviewer's Guide by SourceryThis pull request addresses the email injection issue by implementing regex-based email validation and sanitization using the 'bleach' library. Additionally, it improves code security and clarity by adding type hints and replacing 'random' with 'secrets' for password generation. Error handling for password generation has also been enhanced. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ankitdey-marsh - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests to verify the effectiveness of the email validation and sanitization changes.
- Evaluate the performance impact of using the 'bleach' library for email sanitization, especially if it's applied to all email processing.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -328,15 +330,21 @@ def add_in_room_virtual_room( | |||
| raise RocketChatException('Error while adding user', response=res) | |||
|
|
|||
|
|
|||
| def generate_pass(size=10, chars=string.ascii_lowercase + string.digits): | |||
| return ''.join(random.choice(chars) for _ in range(size)) | |||
| def generate_pass(size=10, chars=string.digits + string.ascii_letters + string.punctuation)->str: | |||
There was a problem hiding this comment.
🚨 suggestion (security): Consider using a more limited set of special characters for password generation
While including punctuation increases password strength, it might cause compatibility issues with some systems. Consider using a subset of special characters that are widely accepted.
| def generate_pass(size=10, chars=string.digits + string.ascii_letters + string.punctuation)->str: | |
| def generate_pass(size=10, chars=string.digits + string.ascii_letters + "!@#$%^&*()-_=+")->str: |
| import re | ||
| from bleach import clean | ||
|
|
||
| EMAIL_REGEX = r"^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$" |
There was a problem hiding this comment.
suggestion: Email regex might be too restrictive for some valid email addresses
The current regex doesn't allow for some valid email formats, such as addresses with subdomains or certain allowed special characters. Consider using a more comprehensive regex or a dedicated email validation library.
import email_validator
def validate_email(email):
try:
email_validator.validate_email(email)
return True
except email_validator.EmailNotValidError:
return False
Fixes #9120
Short description of what this resolves: Used regex to limit the character set and sanitized the emails.
Changes proposed in this pull request:
Checklist
developmentbranch.Summary by Sourcery
Fix email injection vulnerability by sanitizing email inputs and limiting character sets. Enhance code with type hints and improve password generation security.
Bug Fixes:
Enhancements: