Skip to content

feat(AUT-266): enforce fail-fast allowlist validation and prevent raw input reflection - #419

Open
kramakrushna wants to merge 3 commits into
release-ulmofrom
AUT-266_pen_test_fixes
Open

feat(AUT-266): enforce fail-fast allowlist validation and prevent raw input reflection#419
kramakrushna wants to merge 3 commits into
release-ulmofrom
AUT-266_pen_test_fixes

Conversation

@kramakrushna

Copy link
Copy Markdown

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/)

  • Added fail-fast allowlist validation for the email_or_username / email field at the very start of the request handler, before any auth lookup, database call, or downstream service logic runs.
  • Email input is validated against RFC format using Django's validate_email.
  • Username input is validated using the existing platform validate_username function from registration_form.py, which respects ENABLE_UNICODE_USERNAME and USERNAME_REGEX_PARTIAL settings, eliminating a previously hardcoded ASCII-only regex that would have caused false negatives.
  • Raw user input is no longer reflected in error responses. The email field 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 the email field to repopulate forms.

Registration endpoint (/api/user/v{1,2}/account/registration/)

  • Added strict allowlist validation for the optional telemetry field total_registration_time / totalRegistrationTime which previously accepted arbitrary strings including SQL-like payloads.
  • Only numeric decimal values (e.g. 57.664) are accepted. Malformed values are rejected immediately with HTTP 400 and error_code: invalid-total-registration-time before any account creation, filter, or CAPTCHA logic runs.
  • Invalid payload attempts are logged to the audit log using a SHA digest of the malformed value, not the raw payload, to avoid storing or emitting attacker-controlled content in logs.
  • Validation logic is centralized in utils.py as a shared utility function so it can be reused consistently across registration-related endpoints.

User roles impacted

  • Learner: no visible change for legitimate users. Validation is allowlist-based and accepts all valid email/username formats including Unicode.
  • Developer / Operator: new error code invalid-total-registration-time and additional audit log entries for suspicious registration telemetry.

Testing instructions

Prerequisites

pip install -r requirements/edx/base.txt
pip install -r requirements/edx/testing.txt

Automated tests

# Login validation and non-reflection behavior
python manage.py lms test openedx.core.djangoapps.user_authn.views.tests.test_login.LoginSessionViewTest.test_invalid_credentials
python manage.py lms test openedx.core.djangoapps.user_authn.views.tests.test_login.LoginSessionViewTest.test_login_rejects_invalid_login_identifier
python manage.py lms test openedx.core.djangoapps.user_authn.views.tests.test_login.LoginTest.test_login_fail_no_user_exists
python manage.py lms test openedx.core.djangoapps.user_authn.views.tests.test_login.LoginTest.test_login_fail_wrong_password
python manage.py lms test openedx.core.djangoapps.user_authn.views.tests.test_login.LoginTest.test_login_not_activated_no_pii

# Registration telemetry validation and audit logging
python manage.py lms test openedx.core.djangoapps.user_authn.views.tests.test_register.RegistrationViewTestV2.test_register_invalid_total_registration_time

Manual verification — Login

  1. Send a POST to /api/user/v2/account/login_session/ with email_or_username=trtrtr' AND '1'='1' -- and any password.
  2. Confirm response is HTTP 400 with success: false.
  3. Confirm the response body does not contain the SQL payload in any field.
  4. Confirm email field is [invalid input], not the raw submitted string.

Manual verification — Registration

  1. Send a POST to /api/user/v2/account/registration/ with all required fields and total_registration_time=57.664" AND "1"="1" --.
  2. Confirm response is HTTP 400 with error_code: invalid-total-registration-time.
  3. Confirm the request is rejected before account creation occurs.
  4. Confirm the audit log contains a warning with a digest but not the raw payload string.

Regression check — Valid values still work

  1. Login with a valid email and correct password — confirm HTTP 200.
  2. Login with a valid username (including Unicode if ENABLE_UNICODE_USERNAME is on) — confirm HTTP 200.
  3. Register with total_registration_time=57.664 (valid numeric) — confirm HTTP 200 and account is created.

…fe error responses, and centralized registration telemetry validation
@github-actions

Copy link
Copy Markdown

Thank you for your pull request! Congratulations on completing the Open edX tutorial! A team member will be by to take a look shortly.
To those watching community pull requests: No need to worry about this one, a tCRIL team member will be taking care of it.
For this PR's author: If this is a PR that is NOT coming from the Open edX tutorial, please comment and let us know to disregard this message.

@kramakrushna
kramakrushna requested a lite review from Copilot August 10, 2026 07:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@kramakrushna
kramakrushna changed the base branch from master to release-ulmo August 10, 2026 07:56
@kramakrushna
kramakrushna requested a lite review from Copilot August 10, 2026 07:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_time consisting 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

Comment on lines +1213 to +1214
self.assertHttpBadRequest(response)
self._assert_response(response, success=False, absent_keys=["email"])
Comment on lines +1221 to +1222
self.assertHttpBadRequest(response)
self._assert_response(response, success=False, absent_keys=["email"])
Copilot AI review requested due to automatic review settings August 10, 2026 08:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 email and email_or_username are supplied: this block validates email_or_username first, but later _get_user_by_email_or_username prefers email, 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_username currently prefers the email POST param even for the v2 API, which means a caller can supply an unvalidated email alongside a valid email_or_username and 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>
Copilot AI review requested due to automatic review settings August 10, 2026 16:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_user validates email_or_username first (falling back to email), but _get_user_by_email_or_username prefers email when both are present. A request that includes a valid email_or_username plus an invalid/malicious email would 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants