ITEP-70317 User provided data sanitization #476
Conversation
Host header sanitization Path argument sanitization User data sanitization
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a security issue by sanitizing user provided input to prevent log spoofing during password reset requests.
- Introduces a helper function (_sanitize_input) to remove newline and carriage return characters
- Applies sanitization on headers, email, and URL paths in both the password reset request and token validity endpoints
Comments suppressed due to low confidence (2)
platform/services/user_directory/app/endpoints/user_management/password_reset.py:52
- Consider expanding the docstring for _sanitize_input to clarify that only newline and carriage return characters are removed, and outline any limitations of the sanitization approach.
def _sanitize_input(input_data: str) -> str:
platform/services/user_directory/app/endpoints/user_management/password_reset.py:56
- Add unit tests for the _sanitize_input function to ensure it correctly handles various combinations of newline and carriage return characters, preventing future regressions.
return input_data.replace("\n", "").replace("\r", "")
| try: | ||
| UsersHandler.is_email_valid(user_data.email) | ||
| except InvalidEmail as msg: | ||
| logger.error(msg) | ||
| sanitized_msg = _sanitize_input(str(msg)) |
There was a problem hiding this comment.
Wouldn't it be better to sanitize the email before checking validity of this email? As I can see - only email included in this message may lead to the error described in the ticket. Thanks this we wouldn't have to have this sanitization twice in the code - once in except clause and then afterwards.
There was a problem hiding this comment.
As far as I understand, root cause is logger.error(msg) in password_reset.py - UsersHandler.is_email_valid does not log anything but just return exception with msg in case of invalid email. If we sanitize the whole msg (that contains user supplied email) before output - i think it also should be fine.
It will also protect us from the theoretical case where returned msg will be enriched with some other data that potentially can be untrusted, so I would propose to leave whole msg sanitization here and optionally add email sanitization before is_email_valid invocation.
There was a problem hiding this comment.
I tested the current state dynamically - data properly sanitized, so please do not remove existing msg sanitization but if necessary add additional email sanitization before is_email_valid invocation.
User data sanitization during password reset request.
📝 Description
User provided data (in headers or payload) may contain new line characters which might be used to spoof log messages.
This PR removes such characters from user provided data before processing.
JIRA: ITEP-70317
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: