fix: Disable MFA without TOTP #10050
Conversation
|
🚀 Thanks for opening this pull request! |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds a validateUnlink hook for auth providers and uses it when clearing a provider (authData set to null); MFA adapter implements validateUnlink to block unlinking unless allowed (e.g., master key). Tests added to cover unlink behaviors for TOTP and SMS adapters. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Adapters/Auth/mfa.js`:
- Around line 194-196: The validateUnlink method currently always throws and
blocks master-key admin recovery; change its signature to accept the
requestObject parameter (matching validateUpdate) and implement the same
master-key bypass logic: if requestObject && requestObject.master then
return/allow the unlink, otherwise keep throwing the 'Invalid MFA data' error;
update the function name reference validateUnlink to reflect the new parameter
usage so admin/master requests can clear MFA.
In `@src/Auth.js`:
- Around line 574-578: The catch around
req.config.authDataManager.getValidatorForProvider(provider) silently swallows
all errors; change it to either narrow the catch to expected errors (e.g., known
"provider not found" type) or log the caught error at debug/verbose level before
continuing so misconfiguration/bundle-load failures are visible; specifically
update the block that assigns authAdapter to handle only the expected
exception(s) or call something like req.logger.debug/error with the exception
message and provider name when the catch fires, leaving behavior unchanged for
known "no validator" cases.
🧹 Nitpick comments (3)
spec/AuthenticationAdapters.spec.js (2)
1700-1726: Good coverage of the null-unlink vector.Consider using
toBeRejectedWith(...)with the expected error to make the assertion more precise and catch regressions where the error message changes unexpectedly.
1728-1752: Good coverage of the_unlinkFromvector.Same suggestion as above —
toBeRejectedWith(...)would strengthen the assertion. Also consider adding a backward-compatibility test verifying that a non-MFA provider (one withoutvalidateUnlink) can still be unlinked as before.src/Auth.js (1)
573-584: Double lookup of the provider adapter.
getValidatorForProvider(provider)is called here (line 575) and again at line 586 when the adapter does havevalidateUnlink. Consider reusing the result to avoid loading the adapter twice:Suggested refactor
if (authData[provider] === null) { let authAdapter; try { authAdapter = req.config.authDataManager.getValidatorForProvider(provider); } catch (e) { // Ignore error } const { adapter } = authAdapter || {}; if (!adapter || typeof adapter.validateUnlink !== 'function') { acc.authData[provider] = null; continue; } } - const { validator } = req.config.authDataManager.getValidatorForProvider(provider) || {}; + const { validator } = authAdapter || req.config.authDataManager.getValidatorForProvider(provider) || {};Note:
authAdapterwould need to be declared in the outer scope (before theif (authData[provider] === null)block) for this to work.
… master key MFA unlink
|
This has been discusses previously (can't find the issue). It is common that MFA can be disabled without entering the MFA code. That does not "bypass the intended security requirement of verifying a TOTP/code before removal", as in order to do this modification, the user needs to be authenticated with MFA in the first place. If an account security policy requires MFA to be entered before it can be disabled, that is a valid policy decision and it would be good to support that. However, it should not be made a requirement, as not requiring to enter MFA is valid as well. |
| * @param {Parse.Cloud.TriggerRequest} request | ||
| * @returns {Promise<ParseAuthResponse|void|undefined>} | ||
| */ | ||
| validateUnlink(authData, options, req) { |
There was a problem hiding this comment.
Why do we need a new validateUnlink; isn't that a subset of validateUpdate?
There was a problem hiding this comment.
It's mainly about not breaking existing code. Most adapters out there expect an actual object in validateUpdate and directly access things like authData.id
if we pass null, those adapters would blow up with TypeErrors.
There was a problem hiding this comment.
But if validateUnlink is a subset of validateUpdate, then I don't think we should introduce a validateUnlink, as it seems like an unnecessary adapter interface expansion. It's just about the adapter config then. If existing adapter can't handle adding an option to require or not MFA for unlinking then it sounds like an adapter issue, and may require a new (additional) adapter.
|
I agree it should be a policy decision. Currently, Auth.js has a hardcoded bypass that skips validators entirely when authData is null, so developers have no way to enforce a policy if they want to. validateUnlink fixes that bypass and provides the hook so each adapter can decide for itself whether to allow silent unlinking or require additional verification. |
3 participants
Pull Request
Issue
A logged-in user can currently disable MFA for their account without entering a valid verification code by simply calling the unlink function or saving null for the MFA auth data. This bypasses the intended security requirement of verifying a TOTP/code before removal.
Closes: #9623
Approach
Tasks
Summary by CodeRabbit