acm-udayton / acm-udayton/ACM-Meeting-Records

[SECURITY] CSRF protection for sensitive actions (MFA disable)

Abierto
#147 0 comentarios 0 reacciones 1 asignado Reclamado por @ethanlally Ver en GitHub
backend bug
Lenguaje dominante
Python
Estrellas
7
Forks
2
Merge medio
4 d 23 h
PR fusionados (30 d)
2

Descripción

## Location of the Security Issue
* **File/Feature:** app/blueprints/mfa.py, app/blueprints/admin.py, tests/blueprints/test_mfa.py, tests/blueprints_admin.py, etc.
* **Version:** Latest

## Description of the Security Issue
Several sensitive operations such as MFA disable/reset change server state via simple HTTP GET requests which are not protected by CSRF currently. Can cause a logged in user to perform unintended actions if redirected without proving the user initiated them from the actual app. Authentication and authorization are still properly checked, but the server cannot distinguish intentional requests from unintentional.

## Affected Endpoints
* ```/mfa/disable-mfa/``` - completely disables MFA, removes TOTP secret, deletes recovery codes
* ```/mfa/disable-totp/``` - disables TOTP and changes the stored secret
* ```/mfa/reset-recovery-codes/``` - invalidates all exisiting recovery codes and creates replacements
* ```/mfa/setup-totp/``` - generates and commits a new TOTP secret
* ```/admin/reset-code//``` - resets the code for an active meeting

## Steps to Reproduce
1. Log in to a local app instance
2. Ensure that the test account has MFA enabled
3. From another origin, present a link such as ```
View meeting details
```
4. Follow the link while still logged in
5. Return to account page
6. Observe MFA disabled despite no specific action from the user other than visiting the link

Following other endpoints leads to other actions as described above.

## Intended Functionality
Sensitive operations should require:
* A POST request
* A valid session-bound CSRF token
* Normal authentication and authorization

## Existing Support
CSRF is already used throughout the project, so it should not be too much of a chore to implement changes to get functionality to its intended state. All the framework is in place 😸

---

## Acceptance Criteria & Solution Requirements
*To close this issue, the following must be met:*
- [ ] 1. Sensitive actions no longer mutate state through GET
- [ ] 2. All affected actions require POST and valid CSRF protection
- [ ] 3. Authentication and authorization checks remain in place
- [ ] 4. Failed CSRF validation leaves the database unchanged
- [ ] 5. MFA setup still produces recovery codes through a safe flow
- [ ] 6. Tests cover missing, invalid, and valid CSRF tokens
- [ ] 7. Documentation reflects potential changes after fixes applied

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.