Repository navigation
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!
Intègre les 175 commits accumulés depuis c8c8823 (20/09), dont la release stable 0.17.0 publiée le 28/09 — la première depuis la 0.16.0 du 27/03. Correctif de sécurité déterminant pour ce fork : WordPress#989 empêche un mot de passe classique de contourner la 2FA sur REST et XML-RPC. Notre code testait `did_action( 'application_password_did_authenticate' )`, vrai dès que n'importe quel utilisateur s'était authentifié par application password pendant la requête ; upstream scope désormais la vérification par ID (`$app_password_auth_user_ids`). Les autres correctifs de sécurité de la release (WordPress#877, WordPress#973, WordPress#980) étaient déjà présents ici. Trois PR qui composaient le fork sont maintenant mergées upstream : WordPress#927 (fail-safe), WordPress#917 (rate-limit) et WordPress#882. Notre implémentation locale de WordPress#927 et celle d'upstream sont identiques, d'où l'absence de conflit sur class-two-factor-core.php. Le fork ne repose plus que sur WordPress#845 (enforcement par rôle, approuvée le 24/09 mais non mergée) et WordPress#958. Résolution des trois conflits, upstream retenu dans les trois cas : - tests/class-two-factor-core.php : nos noms de tests contredisaient leurs propres assertions (« invalidates » pour un token vérifié « preserved ») ; upstream les renomme et ajoute test_clear_login_rate_limit. - tests/providers/class-two-factor-email.php : ajout de trois tests de lockout côté upstream, aucun test perdu ici. - readme.txt : notre seul apport propre, la documentation du filtre two_factor_fallback_provider_for_user, est déjà dans leur version. Validation (wp-env, WordPress 7.1 / PHP 7.4) : PHPUnit 308 tests et 932 assertions OK sur les suites single et multisite, dont 10/10 pour le groupe enforcement ; PHPCS 29/29 ; PHPStan 0 erreur ; build OK. Le merge apporte une suite multisite et le support WP-CLI, d'où le passage de 218 à 308 tests et la nouvelle dépendance php-stubs/wp-cli-stubs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Le document servait à décider quand tuer le fork ; il était faux sur trois points depuis que la 0.17.0 est sortie. - Le fork reposait sur quatre PR upstream, il n'en porte plus que deux : WordPress#927 et WordPress#917 sont mergées upstream et distribuées. - WordPress#882 y figurait comme écartée pour conflit avec WordPress#927. Upstream a mergé les deux à un jour d'écart : elles se sont ordonnées au lieu de s'exclure. - La validation locale datait du 21/09 et annonçait 218 tests ; la synchro en apporte 308 via la suite multisite, et exige deux réinstallations de dépendances désormais documentées. Ajoute la limite du raisonnement qui justifie le fork : suivre master ne rend plus à jour que si la synchro est faite. Du 25 au 28/09, le fork était en retard sur un correctif publié (WordPress#989, bypass 2FA sur REST et XML-RPC) et donc vulnérable — l'avantage tient au rythme d'intégration, pas au fork. Documente enfin le comportement réparé de la veille : marqueur cherché dans plusieurs fichiers, alertes release et drift indépendantes, releases stables seules retenues comme distribuées. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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