fix(core): scope app password gate per user - #989
Conversation
The API login check relied on did_action(), which is global for the whole request. After one user authenticated with an application password, any other user checked in the same request passed the same check, for example via XML-RPC system.multicall. Track the user IDs reported by the application_password_did_authenticate action and only allow API login for those users. Fixes WordPress#987
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@wordpress/scripts 35.0.0 (bumped by dependabot on master) ships ESLint v10, which no longer supports eslint-env comments. The Gruntfile.js has 19 lint errors: one for the eslint-env comment and 18 prettier formatting errors. Remove the eslint-env comment. Node globals come from the wp-scripts lint config. Reformat the file per prettier. Build config only. No plugin behavior changes.
faisalahammad
left a comment
There was a problem hiding this comment.
CI fix summary
Problem: The "Lint JS & CSS" job failed. wp-scripts lint-js reported 19 errors in Gruntfile.js. Dependabot bumped @wordpress/scripts to 35.0.0 on master. That version ships ESLint v10, and ESLint v10 no longer supports eslint-env comments. This PR only touches PHP files, so the failure was already there on master.
Fix in commit e645c5a:
| Item | Detail |
|---|---|
| Failed job | Lint JS & CSS, step "Lint JS" |
| Errors | 19 in Gruntfile.js (1 eslint-env, 18 prettier) |
| Change 1 | Remove /* eslint-env node,es6 */ comment. ESLint v10 does not support it. Node globals come from the wp-scripts config. |
| Change 2 | Reformat the file with npm run format:js to match prettier. |
| Verified | npm run lint:js and npm run lint:css both pass locally. |
Build config only. No plugin behavior changes.
Note: PR #788 carries the same fix in commit 55ae3ed. If that PR merges first, this commit can be dropped after a rebase.
kasparsd
left a comment
There was a problem hiding this comment.
This looks good, thanks for the fix!
What?
Scope the application password check for API logins to the user who actually authenticated, instead of the whole request.
Fixes #987
Why?
Two_Factor_Core::is_user_api_login_enabled()useddid_action( 'application_password_did_authenticate' )as its default.did_action()counts actions for the entire request, so once one user authenticated with an application password, the check passed for every other user tested in the same request. With XML-RPCsystem.multicall, several logins share one request, so a second user with only a password could log in after a first user logged in with an application password.How?
app_password_did_authenticate()callback runs on theapplication_password_did_authenticateaction and records the user IDs that authenticated with an application password during the current request.is_user_api_login_enabled()now checks whether the given user ID is in that list. Thetwo_factor_user_api_login_enablefilter still works the same.filter_authenticate()path with two different users.Testing Instructions
wp.getUsersBlogsrequest for the two factor user using the application password. The login should still work.system.multicallrequest that first authenticates the two factor user with the application password and then the second user with only a password. The first call should succeed and the second should fail. On 0.16.0 both succeeded because the first call enabled API login for the whole request.npm test.Changelog Entry