Skip to content

fix(security): show a message when password reset answers are throttled - #2325

Open
Isaries wants to merge 3 commits into
WISE-Community:developfrom
Isaries:fix/student-reset-throttle
Open

fix(security): show a message when password reset answers are throttled#2325
Isaries wants to merge 3 commits into
WISE-Community:developfrom
Isaries:fix/student-reset-throttle

Conversation

@Isaries

@Isaries Isaries commented Jul 6, 2026

Copy link
Copy Markdown

Summary

This pairs with the WISE-API change that throttles the student password reset flow after several incorrect security answers. The server now returns a throttling response code. The security answer step previously ignored any unrecognized response code, so a throttled user saw a blank error with no explanation.

Changes

  • Handle the throttling response code in both the security answer step and the password change step, telling the user to wait or to ask their teacher.
  • Add a unit test that mirrors the existing incorrect answer test.

Paired change

Paired with the WISE-API change (branch fix/student-reset-throttle) that adds the throttling.

The server now temporarily blocks the student password reset flow after
several incorrect security answers to prevent brute forcing the answer.
The security answer step previously ignored any unrecognized response
code, so a throttled user saw a blank error and no explanation. Handle
the throttling response code in both the security answer and password
change steps so the user is told to wait or to ask their teacher.
@Isaries

Isaries commented Jul 6, 2026

Copy link
Copy Markdown
Author

Paired server change: WISE-Community/WISE-API#324

@Aaron-Detre Aaron-Detre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@hirokiterashima hirokiterashima left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding this feature!

Functionality works as described, but I think we should mirror the view/flow that the teacher sees (see below). After too many failed attempts, the form controls are disabled, and we show them a link to the forgot teacher password page to start the flow again. This prevents immediate re-attempts and also adds another extra step/deterrent for the attacker.

Screenshot 2026-07-15 at 4 49 42 PM

…tled

Showing only a message let a throttled student keep submitting answers.
Mirror the teacher verification code flow: disable the form and show a link
back to the start of the flow, with the warning and the link below the form
where the teacher flow puts them.

Unlike the teacher flow there is no new verification code for a student to
generate, so the message tells them to wait and start again or to ask their
teacher rather than promising the link will unblock them.

The security answer step also had no default branch, so any response code it
did not recognise left the message undefined and the student saw nothing at
all. The server returns invalidUsername from that endpoint when the account
has gone away mid-flow, which reached exactly that dead end.

Both steps now share a base class holding the message state and the lockout,
so the two copies of the response text cannot drift apart.
@Isaries

Isaries commented Jul 17, 2026

Copy link
Copy Markdown
Author

The generate-messages check failure here is unrelated to this PR's changes (test-coverage and qlty check both pass).

Root cause: .github/workflows/generate-messages.yml fails for any PR opened from a fork, not just this one. Its actions/checkout@v6 steps use:

ref: ${{ github.head_ref || github.ref_name }}

without specifying repository:. For a cross-repo (fork) PR, github.head_ref is the branch name on the fork, but checkout defaults to looking for that branch in the base repo, which is why the job errors with:

A branch or tag with the name 'fix/student-reset-throttle' could not be found

Since develop-workflows.yml calls this reusable workflow pinned to @develop, it always runs the version currently on develop — so this can't be fixed from within this PR branch, and re-running won't help.

A few other issues in the same file worth fixing while at it:

  • The checkout step right after (no with: args) overwrites the ref specified above.
  • The step labeled "Use Node.js" uses actions/checkout@v6 instead of actions/setup-node@v4 — Node.js is never actually installed by that step.
  • Even with checkout fixed, the final git push uses the base repo's GITHUB_TOKEN, which has no write access to a fork branch, so the auto-commit-messages-back-to-PR flow won't work for fork PRs regardless.

Could a maintainer fix generate-messages.yml on develop (add repository: ${{ github.event.pull_request.head.repo.full_name }} for fork PRs, or skip the auto-commit step when github.event.pull_request.head.repo.fork == true), or override this check to merge?

For context: I checked the last 100 PRs on this repo and this is the only cross-repo (fork) one — everyone else pushes branches directly to this repo, so this code path has apparently never been exercised before.

Moving the message below the form dropped the @if (message) guard that used to
wrap it, and message is the empty string until something fails, so both cards
rendered an empty p.warn with its own vertical margin on every visit.

Restore the guard and assert its absence on load. The assertion queries .warn
rather than the paragraph itself, so it fails if anything empty is drawn there,
and it does fail against the unguarded template.
@Isaries

Isaries commented Aug 31, 2026

Copy link
Copy Markdown
Author

Pushed one more commit addressing a defect in the previous one.

When I moved the message below the form to add the "Forgot Student Password" link, I dropped the @if (message) guard that used to wrap the paragraph. message is the empty string until something fails, so both cards rendered an empty <p class="warn"> with its own vertical margin on every visit, not just after a throttled attempt.

The guard is restored and each spec now asserts that nothing is drawn there on load. The assertion queries .warn rather than the paragraph itself, so it fails if anything empty is rendered in that position; I checked that it does fail against the unguarded template rather than assuming it would.

The generate-messages check will still fail for the reason described above, which is unrelated to these changes.

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