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

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

未關閉
#147 0 則留言 0 個 reaction 已指派 1 人 已被 @ethanlally 認領 在 GitHub 檢視
backend bug
主要語言
Python
星號
7
分支
2
平均合併
4 天 23 小時
30 天內合併 PR
2

描述

## 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

貢獻指南

開啟貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。