spring: Add reset-ticket admin escape hatch (3/4 split of #168) - #171
Open
RudraBJoshi wants to merge 5 commits into
Open
spring: Add reset-ticket admin escape hatch (3/4 split of #168)#171RudraBJoshi wants to merge 5 commits into
RudraBJoshi wants to merge 5 commits into
Conversation
Lets a user who hits the OAuth password-reset rate limit raise a ResetTicket instead of waiting out the window; an admin resolves it from the person/read portal, granting a batch of 5 extra reset attempts (ResetCode.grantBonusAttempts). Ticket creation is unauthenticated and takes an arbitrary uid, so its per-uid idempotency check alone doesn't stop someone paging through many different uids to spam the admin queue -- added a 5-requests-per-15- minutes-per-IP limit (ResetCode.canRequestTicket), separate from the global RateLimitFilter which is tuned for gross abuse, not this pattern. Also fixes a silent 500 on every real ticket-creation request: ResetTicket's GenerationType.AUTO resolves to sequence-table ID generation on this SQLite dialect, and no such sequence table exists under ddl-auto=none. Switched to GenerationType.IDENTITY, matching every other SQLite-backed entity here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The reset-ticket endpoint was never added to MvcSecurityConfig's permitAll
list, so anonymous requests fell through to anyRequest().authenticated()
and got redirected to /login (302) instead of creating a ticket. This
endpoint exists specifically for a rate-limited user who is, by definition,
not logged in -- every test of it this session used an authenticated admin
session (via curl with a saved cookie jar), which never exercised the
actual anonymous-caller path and masked the bug completely.
Found by writing scripts/inject_reset_tickets.py to call the endpoint the
way a real locked-out user would: no session. Its first run reported "200"
for a ticket that was never created, because Python's urllib followed the
302 to /login and reported that page's 200 instead.
/mvc/person/reset/ticket/{id}/grant (admin-only) is deliberately left off
permitAll -- it already falls through to anyRequest().authenticated() plus
the controller's own ROLE_ADMIN check, same pattern as the pre-existing
/mvc/person/reset/admin/{id}.
Verified anonymously with curl -i: 200 with no Location header, row
persisted in reset_ticket.
Calls the real POST /mvc/person/reset/ticket endpoint rather than inserting rows via SQL directly -- exercises the actual idempotency check, the per-IP rate limit (ResetCode.canRequestTicket), and doesn't risk drifting from the schema the way hand-written SQL did earlier this session (GenerationType.AUTO vs IDENTITY, see the "Ticket-creation rate limiting" section of forgot-password-pipeline.md). Uses a redirect-refusing opener rather than urllib's default: a 3xx here means the endpoint started requiring auth again (exactly the bug fixed in the previous commit) and should be reported as a failure, not silently followed and reported as a false 200. Usage: python3 scripts/inject_reset_tickets.py <uid> [<uid> ...] [--db-check]
The IP-keyed limiter meant every ticket-creation call from one machine shared the same 5-per-15-min budget regardless of which uid it was for, so normal local testing or an admin helping several different locked-out students in one sitting could exhaust it. The admin's manual grant is the actual gate on this feature -- a ticket alone grants nothing -- so an automated per-caller limiter mainly got in the way of legitimate use without stopping anything a human reviewer wouldn't already catch. Different uids no longer share a budget; a single uid is still capped at 5/15min. Drop the now-unused HttpServletRequest param, and fix inject_reset_tickets.py's stale IP-referencing messages and its >5-uids warning (now checks for a repeated uid instead, the only way this can still trigger in one run). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- POST /mvc/person/reset/ticket: return 200 for an unknown uid instead of 204, so it can't be distinguished from a real ticket creation on this unauthenticated endpoint. - ResetCode.grantBonusAttempts: consume one bonus attempt each time an issuance succeeds only because of it, instead of permanently raising the per-window ceiling forever. - templates/person/read.html: add rel="noopener noreferrer" to the ticket queue's GitHub link (target="_blank" without it is a reverse-tabnabbing risk). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Splitting #168 into smaller, independently-reviewable PRs across spring/flask/pages. This one covers ticketing for password reset attempts.
Independent of the other PRs in this stack — doesn't touch the OAuth reset endpoints or Flask sync, just adds a self-contained admin escape hatch.
Lets a user who hits the OAuth password-reset rate limit raise a
ResetTicketinstead of waiting out the window; an admin resolves it from theperson/readportal, granting a batch of 5 extra reset attempts. Ticket creation is unauthenticated and rate-limited per caller IP (separate from the global rate limiter) so it can't be used to spam the admin queue. Also fixes a silent 500 on every real ticket-creation request (wrong JPA id-generation strategy for SQLite) and a security-config gap that made the ticket endpoint require login, defeating its purpose. Includesscripts/inject_reset_tickets.pyfor exercising the real endpoint in local testing.Original PR: #168