feat(AUT-266): enforce fail-fast allowlist validation and prevent raw input reflection - #419
feat(AUT-266): enforce fail-fast allowlist validation and prevent raw input reflection#419kramakrushna wants to merge 3 commits into
Conversation
…fe error responses, and centralized registration telemetry validation
|
Thank you for your pull request! Congratulations on completing the Open edX tutorial! A team member will be by to take a look shortly. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
openedx/core/djangoapps/user_authn/views/utils.py:48
total_registration_timeconsisting only of whitespace will currently be treated as invalid (because it's stripped to "" and then fails the regex), even though the empty string is treated as "not provided" just above. If this field is optional telemetry, it’s safer and more consistent to treat whitespace-only values as empty and skip validation.
normalized_value = str(total_registration_time).strip()
if TOTAL_REGISTRATION_TIME_RE.fullmatch(normalized_value):
return None
| self.assertHttpBadRequest(response) | ||
| self._assert_response(response, success=False, absent_keys=["email"]) |
| self.assertHttpBadRequest(response) | ||
| self._assert_response(response, success=False, absent_keys=["email"]) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
openedx/core/djangoapps/user_authn/views/login.py:598
- The login identifier validation can be bypassed when both
emailandemail_or_usernameare supplied: this block validatesemail_or_usernamefirst, but later_get_user_by_email_or_usernameprefersemail, which may be unvalidated. To preserve the fail-fast allowlist guarantee, validate only the identifier field that will actually be used for this API version.
login_identifier = request.POST.get("email_or_username")
if login_identifier is None:
login_identifier = request.POST.get("email")
if login_identifier is not None:
_validate_login_identifier(login_identifier)
openedx/core/djangoapps/user_authn/views/login.py:165
_get_user_by_email_or_usernamecurrently prefers theemailPOST param even for the v2 API, which means a caller can supply an unvalidatedemailalongside a validemail_or_usernameand still trigger DB lookup/audit logging on the unvalidated value. Use the API-version-specific key to ensure the looked-up identifier is the one that was validated.
This issue also appears on line 594 of the same file.
if any(f not in request.POST.keys() for f in login_fields):
raise AuthFailedError(LOGIN_INFO_ERROR)
email_or_username = request.POST.get("email", None) or request.POST.get("email_or_username", None)
user = _get_user_by_email(email_or_username)
Co-authored-by: kramakrushna <293011905+kramakrushna@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
openedx/core/djangoapps/user_authn/views/login.py:598
- The login identifier selected for fail-fast validation is not the same identifier that is later used for lookup. Here,
login_uservalidatesemail_or_usernamefirst (falling back toemail), but_get_user_by_email_or_usernameprefersemailwhen both are present. A request that includes a validemail_or_usernameplus an invalid/maliciousemailwould bypass validation for the value actually used in auth/user lookup. Align the selection order so the validated identifier matches the one used downstream.
login_identifier = request.POST.get("email_or_username")
if login_identifier is None:
login_identifier = request.POST.get("email")
if login_identifier is not None:
_validate_login_identifier(login_identifier)
This PR hardens input validation and output handling across the login and registration API endpoints to address two security findings: improper input validation and raw user input reflection in API responses.
Login endpoint (
/api/user/v{1,2}/account/login_session/)email_or_username/emailfield at the very start of the request handler, before any auth lookup, database call, or downstream service logic runs.validate_email.validate_usernamefunction from registration_form.py, which respectsENABLE_UNICODE_USERNAMEandUSERNAME_REGEX_PARTIALsettings, eliminating a previously hardcoded ASCII-only regex that would have caused false negatives.emailfield in 400 responses now returns the user's actual account email when the user is known, or the safe sentinel value[invalid input]when the identifier is unknown or malformed. This maintains backward API compatibility for clients that read theemailfield to repopulate forms.Registration endpoint (
/api/user/v{1,2}/account/registration/)total_registration_time/totalRegistrationTimewhich previously accepted arbitrary strings including SQL-like payloads.57.664) are accepted. Malformed values are rejected immediately with HTTP 400 anderror_code: invalid-total-registration-timebefore any account creation, filter, or CAPTCHA logic runs.User roles impacted
invalid-total-registration-timeand additional audit log entries for suspicious registration telemetry.Testing instructions
Prerequisites
Automated tests
Manual verification — Login
/api/user/v2/account/login_session/withemail_or_username=trtrtr' AND '1'='1' --and any password.success: false.emailfield is[invalid input], not the raw submitted string.Manual verification — Registration
/api/user/v2/account/registration/with all required fields andtotal_registration_time=57.664" AND "1"="1" --.error_code: invalid-total-registration-time.Regression check — Valid values still work
ENABLE_UNICODE_USERNAMEis on) — confirm HTTP 200.total_registration_time=57.664(valid numeric) — confirm HTTP 200 and account is created.