[Feature:System] Configurable password requirements by sushen123 · Pull Request #12455 · Submitty/Submitty

sushen123 · 2026-02-21T09:49:49Z

Why is this Change Important & Necessary?

Password requirements are currently hardcoded (12+ chars, uppercase, lowercase, number, special character). Institutions cannot customize these rules to match their own password policies. Additionally, the Change Password modal has no validation and does not display any requirements to the user.

What is the New Behavior?

Password requirements are now configurable via password_requirements in submitty.json
The Change Password modal now displays requirements with a "Show Requirements" button (matching the Sign Up page)
The Change Password modal now enforces password validation server-side (previously accepted any password)
A system migration adds default password_requirements to existing installations
The Sign Up page now dynamically renders requirements from config instead of hardcoded HTML

Change Password modal now shows requirements:

Sign Up page reflecting custom config (min_length changed to 8):

What steps should a reviewer take to reproduce or test the bug or new feature?

Log in as instructor/instructor, go to My Profile, click the Change Password pencil icon, verify requirements are displayed with a Show Requirements button
Try submitting a weak password (e.g., abc) — verify it is rejected
Visit /authentication/create_account — verify requirements are still displayed correctly
Modify password_requirements in /usr/local/submitty/config/submitty.json (e.g., set min_length to 8, require_special_chars to false) — verify both Sign Up and Change Password reflect the updated config

Automated Testing & Documentation

Unit tests added for isValidPassword() with custom requirements and max length validation in UtilsTester.php.

Other information

Includes a system migration (20260220120000_add_password_requirements.py) to add default password_requirements config to existing installations.

IDzyre

Looks like a good start, and pretty close for a full security review.

I personally think that we shouldn't allow the password requirements to "not be defined", and require them to be in the config.
The goal of this is to consolidate the code that we have to change if the password requirements were to be updated, and this adds multiple extra places where we have to double check to make sure they are the same.

Also, could you update the Cypress login tests to reflect the different password requirements shown?

codecov · 2026-02-21T19:43:26Z

Codecov Report

❌ Patch coverage is 67.85714% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.50%. Comparing base (8fa4355) to head (b3d76c1).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12455      +/-   ##
============================================
+ Coverage     21.48%   21.50%   +0.01%     
- Complexity     9842     9861      +19     
============================================
  Files           268      268              
  Lines         36851    36863      +12     
  Branches        495      495              
============================================
+ Hits           7919     7928       +9     
- Misses        28441    28444       +3     
  Partials        491      491

Flag	Coverage Δ
autograder	`21.32% <ø> (ø)`
js	`2.01% <ø> (ø)`
migrator	`100.00% <ø> (ø)`
php	`20.54% <67.85%> (+0.02%)`	⬆️
python_submitty_utils	`80.08% <ø> (ø)`
submitty_daemon_jobs	`91.13% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…down(), and add Cypress password validation tests

sushen123 · 2026-02-23T10:36:42Z

@IDzyre I've addressed your feedback. Can you take another look?

IDzyre

Looks good on the requirements, and functionality code, I need to do a review on the visual side, but a small question.

sushen123 · 2026-02-27T10:00:37Z

@IDzyre i have implemented all your feedback, can you look into it

Eli-J-Schwartz · 2026-02-27T20:34:47Z

I tested the functionality of this PR, by first attempting to create a new account, and then attempting to change the password for an already existing account. In both instances, the password requirement behavior worked as expected, with only matching passwords that fulfilled the minimum standard being accepted.

However, when the password validation failed, somewhat annoying behavior occurred. In the account creation page, all fields were cleared, erasing the email, user id, and name. In the profile page, I was automatically redirected to the homepage, even when the password didn't update. This seems to be because the logic for password validation is fully server-side, so an invalid post request will redirect to a given page, possibly clearing all fields. If this logic could be added to the client-side, it would prevent this from being an issue.

Additionally, I have a minor concern about the isValidPassword in site/app/libraries/Utils.php. The function repeatedly uses preg_match to check whether a password fulfills a given requirement. As far as I can tell, preg_match is not a constant-time function, meaning it can leak secret data (in this case a password) based on the time it takes to evaluate the function. This does not appear to be a critical security issue, but I think it would be wise to fix now.

sushen123 · 2026-02-28T06:32:26Z

I tested the functionality of this PR, by first attempting to create a new account, and then attempting to change the password for an already existing account. In both instances, the password requirement behavior worked as expected, with only matching passwords that fulfilled the minimum standard being accepted.

However, when the password validation failed, somewhat annoying behavior occurred. In the account creation page, all fields were cleared, erasing the email, user id, and name. In the profile page, I was automatically redirected to the homepage, even when the password didn't update. This seems to be because the logic for password validation is fully server-side, so an invalid post request will redirect to a given page, possibly clearing all fields. If this logic could be added to the client-side, it would prevent this from being an issue.

Additionally, I have a minor concern about the isValidPassword in site/app/libraries/Utils.php. The function repeatedly uses preg_match to check whether a password fulfills a given requirement. As far as I can tell, preg_match is not a constant-time function, meaning it can leak secret data (in this case a password) based on the time it takes to evaluate the function. This does not appear to be a critical security issue, but I think it would be wise to fix now.

@Eli-J-Schwartz I've noticed the same field-clearing issue happens when email or user ID validation fails too, all fields get cleared on the redirect. Should I create a separate issue to add client-side validation for those fields, or would you prefer I include it in this PR?

IDzyre · 2026-02-28T19:10:49Z

Since the purpose of this PR is for the passwords, if you fix them in this PR, I suggest that you create an issue, outlining what you did to fix it for passwords in this PR. (You can solve that issue in another PR if you wish as well)

sushen123 · 2026-03-03T06:45:57Z

@IDzyre I’ve made the requested changes. Could you please review them

IDzyre · 2026-03-03T17:08:08Z

I will review it when I have time, I am currently a student in University, and it is an exam week, so I haven't done much reviewing. Sorry for the wait.

NicholasCiuica

I've tested the changes in this PR, and found everything to be working as expected. However, I've found some issues with the tests for this feature, and I have also left some feedback on opportunities for code reuse and bettering the password-matching frontend feedback.

Additionally, I don't see any tests for changing a current account's password. Given the new password requirements, I feel it will be valuable to test changing passwords in a similar way to how this PR tests account creation.

NicholasCiuica · 2026-06-09T15:33:57Z

        cy.get('[data-testid="sign-up-button"]').click();
        cy.get('[data-testid="popup-message"]').should('contain.text', 'Password does not meet the requirements');
+        // Password missing uppercase
+        inputData(undefined, undefined, 'nouppercase#123', 'nouppercase#123');


It doesn't look like the Cypress tests are properly testing password complexity anymore, now that the password complexity features are disabled by default. See below, where I edited the Cypress test locally to click Show Requirements.

I am not sure how to edit configurations within a Cypress test, but I believe that would solve this issue; set "require_uppercase", "require_lowercase", "require_numbers", and "require_special_chars" to true in /usr/local/submitty/config/submitty.json at the start of the test

addresses the following issues: now clears text fields after failing to sign in (in case page isn't reloaded after sign in fails); randomizes username and email that is used to sign in so the current contents of the database (likely) wont affect the current test run; commented out a number of password requirement tests that currently don't work because we are not yet capable of changing configs within cypress tests

I made the ChangePasswordForm and the CreateNewAccount form use the same frontend password validation. I ended up not reusing the checkPasswordsMatch function within the checkSubmittedPassword function, in the case that we want them to do different things later down the line (checkPasswordsMatched is currently called whenever you unfocus from the Confirm Password field of the Create Account form)

NicholasCiuica · 2026-06-10T16:32:12Z

I made some changes to fix the failing password tests. Importantly, I commented out a number of the password complexity Cypress tests because the password complexity requirements are now disabled by default, so a GitHub action will likely need to be made which enables these configs before the test runs and disables them afterward.

I also consolidated the frontend password validation logic for the Change Password/Create Account forms into a single function within authentication.js.

A note to people newly looking at this PR: you have to enable DatabaseAuthentication and create_user_account to test the features added, https://submitty.org/sysadmin/configuration/self_account_creation

removed leftover onclick tags from submit forms, and clarified comments on clearTextFields tag after retesting the Cypress password tests

### Why is this Change Important & Necessary? Relates to [#12455](#12455) ### What is the New Behavior? The self_create_account Cypress test now randomizes the user id and email used when trying to create a new account during testing, so that the database contents of the testing VM don't affect this test. Originally, this test would break whenever it was run more than once locally or in CI, because the new user that the first attempt at the test made affects how the next attempts run. ### What steps should a reviewer take to reproduce or test the bug or new feature? enable DatabaseAuthentication and create_user_account to test the features added, https://submitty.org/sysadmin/configuration/self_account_creation run the self_create_account Cypress test locally multiple times. You will see the "User ID or email already attached to an account" display where it shouldn't be. <img width="743" height="511" alt="Screenshot 2026-06-08 170523" src="https://github.com/user-attachments/assets/d58f4d67-cc9e-477b-9d22-4f23b6695b6f" /> ### Other information This PR is a portion of the changes I have committed to [#12455](#12455), but this bugfix is important for working on other database-related PRs.

NicholasCiuica · 2026-06-11T14:51:06Z

Another note to reviewers, when testing the Cypress test self_account_creation locally, the failure below is expected once the test makes it to the email verification steps. Something about the email verification process is not set up to run properly locally, but it succeeds in CI.

IDzyre · 2026-06-11T19:28:17Z

It passed locally when I added the feature, not sure what caused that.

changes configs in CI to enable password complexity requirements for the duration of the password tests. These tests will fail locally because password complexity is disabled by default

new spec to test changing passwords. i need to make a cleanup step after i disable password complexity requirements to revert the password set in this spec.

opted to keep change password spec self-contained, so I make a new account just for that test instead of changing the password of any other account made during setup or testing.

NicholasCiuica · 2026-06-17T16:22:53Z

I have added steps in the CI to the enable password complexity requirements in the configs for the duration of this PR's Cypress tests, and have re-enabled my password complexity tests within the self_account_creation spec. I also made a new change_password spec that tests the ability for a user to change their password given the password complexity configs introduced in this PR.

I believe this PR is ready to be reviewed. Please see original post and my above comments for testing instructions.

dagemcn

I did a functionality test on this PR, I tested the rendering of password requirements, the enforcements of password requirements and that changing the password still actually updated the password. All worked as intended. The cypress tests for database authentication are a good addition as previously this functionality was untested to my knowledge. Looks good to me, failing CIs seem to be the ones we already know are flaky or failing.

Add configurable password requirements and update Change Password modal

a4cc399

github-project-automation Bot added this to Submitty Development Feb 21, 2026

github-project-automation Bot moved this to Seeking Reviewer in Submitty Development Feb 21, 2026

sushen123 changed the title ~~Add configurable password requirements and update Change Password modal~~ [Feature:System] Configurable password requirements Feb 21, 2026

bmcutler requested a review from IDzyre February 21, 2026 15:54

automateprojectmangement Bot moved this from Seeking Reviewer to In Review in Submitty Development Feb 21, 2026

IDzyre requested changes Feb 21, 2026

View reviewed changes

Comment thread migration/migrator/migrations/system/20260220120000_add_password_requirements.py Outdated

Comment thread migration/migrator/migrations/system/20260220120000_add_password_requirements.py Outdated

github-project-automation Bot moved this from In Review to Work in Progress in Submitty Development Feb 21, 2026

Make password requirements mandatory from config, simplify migration …

c607dc5

…down(), and add Cypress password validation tests

sushen123 force-pushed the feat/configurable-password-requirements branch from 45dce5c to c607dc5 Compare February 23, 2026 10:31

IDzyre requested changes Feb 24, 2026

View reviewed changes

Comment thread site/app/templates/ChangePasswordForm.twig Outdated

Remove is not defined fallback checks per review

ee66e07

sushen123 force-pushed the feat/configurable-password-requirements branch from 99593a9 to ee66e07 Compare February 26, 2026 14:09

Eli-J-Schwartz self-requested a review February 27, 2026 20:35

automateprojectmangement Bot moved this from Work in Progress to In Review in Submitty Development Feb 27, 2026

Eli-J-Schwartz removed their request for review February 27, 2026 20:46

Add client-side password validation and fix timing leak

9867f81

sushen123 force-pushed the feat/configurable-password-requirements branch from aa8283d to 9867f81 Compare March 1, 2026 02:30

Merge branch 'main' into feat/configurable-password-requirements

2e835a4

sushen123 mentioned this pull request Mar 1, 2026

Fix Sign Up form clearing fields on validation error #12490

Closed

Merge branch 'main' into feat/configurable-password-requirements

6a44379

github-actions Bot added the Abandoned PR - Needs New Owner No activity on PR for more than 2 weeks -- seeking new owner to complete label Apr 1, 2026

Merge branch 'main' into feat/configurable-password-requirements

481e49c

NicholasCiuica requested changes Jun 9, 2026

View reviewed changes

NicholasCiuica added 3 commits June 9, 2026 16:57

fix ConfigTester.php wrong max_length

9212228

NicholasCiuica self-assigned this Jun 10, 2026

cleanup

c7b0c98

removed leftover onclick tags from submit forms, and clarified comments on clearTextFields tag after retesting the Cypress password tests

NicholasCiuica mentioned this pull request Jun 10, 2026

[Bugfix:System] Create Account Test Randomization Fix #12898

Merged

Merge branch 'main' into feat/configurable-password-requirements

1355ad1

NicholasCiuica added 2 commits June 16, 2026 13:14

fixing password complexity cypress tests

e9c1a4a

changes configs in CI to enable password complexity requirements for the duration of the password tests. These tests will fail locally because password complexity is disabled by default

Merge branch 'main' into feat/configurable-password-requirements

17b1fd7

NicholasCiuica moved this from Work in Progress to In Review in Submitty Development Jun 16, 2026

NicholasCiuica added 7 commits June 16, 2026 15:27

colons don't need to be escaped

f42dad8

fixing issue in sed ci commands

da0daaf

another ci sed fix

6c0b06e

change password spec part 1

82c3e85

new spec to test changing passwords. i need to make a cleanup step after i disable password complexity requirements to revert the password set in this spec.

change password spec part 2

e451745

opted to keep change password spec self-contained, so I make a new account just for that test instead of changing the password of any other account made during setup or testing.

formatting

d0d080f

password change test cleanup

b3d76c1

dagemcn approved these changes Jun 18, 2026

View reviewed changes

NicholasCiuica approved these changes Jun 18, 2026

View reviewed changes

github-actions Bot removed the Abandoned PR - Needs New Owner No activity on PR for more than 2 weeks -- seeking new owner to complete label Jun 19, 2026

bmcutler merged commit 876cb4f into Submitty:main Jun 19, 2026
23 of 26 checks passed

github-project-automation Bot moved this from In Review to Done in Submitty Development Jun 19, 2026

dandrecollins07-ctrl mentioned this pull request Jul 21, 2026

Enforce Character Requirements for Database Passwords #11884

Closed

Uh oh!

Conversation

sushen123 commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why is this Change Important & Necessary?

What is the New Behavior?

What steps should a reviewer take to reproduce or test the bug or new feature?

Automated Testing & Documentation

Other information

Uh oh!

IDzyre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sushen123 commented Feb 23, 2026

Uh oh!

IDzyre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sushen123 commented Feb 27, 2026

Uh oh!

Eli-J-Schwartz commented Feb 27, 2026

Uh oh!

sushen123 commented Feb 28, 2026

Uh oh!

IDzyre commented Feb 28, 2026

Uh oh!

sushen123 commented Mar 3, 2026

Uh oh!

IDzyre commented Mar 3, 2026

Uh oh!

NicholasCiuica left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicholasCiuica Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicholasCiuica Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NicholasCiuica commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicholasCiuica commented Jun 11, 2026

Uh oh!

IDzyre commented Jun 11, 2026

Uh oh!

NicholasCiuica commented Jun 17, 2026

Uh oh!

dagemcn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sushen123 commented Feb 21, 2026 •

edited

Loading

codecov Bot commented Feb 21, 2026 •

edited

Loading

NicholasCiuica Jun 9, 2026 •

edited

Loading

NicholasCiuica commented Jun 10, 2026 •

edited

Loading