The Target: line shows raw internals ('users: 2, new_role: author') on the Users-list role change — the control it exists to provide

[dknauss]

Found by hand-testing 4.9.0. This is #322 v2's headline control failing in a common flow, in exactly the way the code's own comment says it must not.

What a user sees

Users list → change a user's role → the challenge appears:

To continue: Change user role — please enter your password.
Target: users: 2, new_role: author

users: 2 is a bare database ID. new_role: author is a raw form field name. A human being asked to authorise something cannot tell from this which account is affected.

The tester's suggested wording is the right shape: "Demote someone to Author".

Why this is not cosmetic

Challenge::describe_stash_target() already knows this. Its own comment:

A bare numeric id ("user_id=5") tells an admin nothing about WHO they are authorizing — and this line is the only control left in the same-origin lure case, so resolve it to something a human can actually judge.

So the design explicitly identifies bare IDs as inadequate because the Target line is the last line of defence when an attacker lures a user into a gated request. In that scenario the user is shown users: 2 and has no way to judge it.

The whole argument for #322 v2 is that a coarse label is a blank cheque and naming the concrete target makes the password submit an informed decision. Here the target is named in a form only the database understands.

Root cause

The resolution is keyed to a single field name:

if ( 'user_id' === $key && current_user_can( 'list_users' ) ) {
    $user = get_userdata( (int) $value );
    if ( $user && ! empty( $user->user_login ) ) {
        $value = $user->user_login . ' (#' . (int) $value . ')';

Only user_id is resolved. The Users-list role change posts users[], so $key is users and the branch never fires. user.promote's own stash allowlist confirms the field names:

'stash' => self::stash_allowlist( array( 'users', 'new_role', 'changeit' ) ),

So the resolution and the allowlist disagree about what a user identifier is called. The lookup was written for users.php?action=promote&user_id=N (the row-action path) and never extended to the bulk/dropdown path.

What a fix needs

1. Resolve users as well as user_id, including the multi-value case — the field is an array, so several accounts can be affected by one confirmation, and naming only the first would be worse than naming none.
2. Render field names as human labels. new_role: author should read as a role name, not a form key.
3. Keep the existing current_user_can( 'list_users' ) capability gate. It is there because resolving an ID to a login is a server-side lookup, not an echo of what the requester supplied — without the gate the challenge page becomes a username-enumeration oracle for a caller who cannot otherwise list users. Any fix must preserve that.

The stated goal is a sentence a non-technical administrator can act on. Demote alice (#2) to Author is judgeable; users: 2, new_role: author is not.

Related observation, filed as a question rather than a defect

The action was a demotion (Administrator → Author) and it was still gated. user.promote's admin matcher returns true for any changeit + new_role with no escalation test:

return isset( $_REQUEST['changeit'] ) && isset( $_REQUEST['new_role'] );

Gating demotions may well be deliberate for this plugin — removing an administrator is consequential too, and the rule id being promote may simply be a naming artefact. Flagging it only because the label then reads oddly: the challenge says "Change user role", which is accurate, while the rule that fired is called promote. If demotions are intended to gate, no change is needed beyond the wording above.

Severity

Not a security hole — the gate fires correctly and nothing is replayed that should not be. But it defeats the purpose of the control 4.9.0 is built around, in a flow administrators use routinely. Release-owner call; see #429, which is in the same lane and already pulled the tag.

[dknauss]

Escalating: hand-testing shows the Target line can be wrong, not merely unreadable

Four flows driven by hand post-#429. The original report was the bulk path showing users: 2. The problem is broader and one case is actively misleading.

1. It names a field the user never changed — the worst case

Saving Settings → General with no sudo session:

To continue: Change critical site setting — please enter your password.
Target: default_role: subscriber

The tester changed the Site Title. default_role was untouched.

Cause: options.critical's allowlist is self::stash_allowlist( self::critical_option_names() ) — every critical option name. Settings → General posts all its fields on every save, so the stash captures whichever allowlisted option happens to be in the payload, not the one that changed.

So the line that exists to say what is about to happen names something that is not about to happen. Under the lure scenario this control is designed for, an attacker's request would display an unrelated-but-plausible field while the real change rides along unnamed. That is worse than showing nothing: it invites a confident wrong decision.

2. Raw form-field names, beyond users

Target: checked: akismet/akismet.php      ← plugin activation

checked is the literal checkbox name from stash_allowlist( array( 'checked' ) ). Same class as users: and new_role: — the display echoes internal field keys.

3. Resolution works where it is wired, which shows the fix is small

Target: user_id: test (#2)                ← user-edit.php

The user_id branch resolves correctly, capability-gated, exactly as designed. It only ever covered one key. That is good news for the fix: the mechanism is right and under-applied, not wrong.

Revised scope

Three distinct problems, not one:

1. Wrong field named when a form posts several allowlisted keys and only one changed (options.critical). Needs the stash to record what differs, not what is present — which is what email_change_differs() already does for user.change_email, so the pattern exists in the codebase.
2. Raw field keys shown (checked, users, new_role) — needs a key→label map.
3. Only user_id resolves to something human-readable — the original report.

Tester's summary of the target it should have produced: "Demote someone to Author" — a sentence, not a field dump.

Bearing on the tag

Still not a security hole and the gate still fires. But (1) is a different severity from (2) and (3): an unreadable target is an incomplete control, while a wrong target is a misleading one, and this line is the last defence in the same-origin lure case by the design's own account.

I am not making the release call — noting only that the reasoning for deferring #431 was "incomplete, not worse than 4.8.0." For case (1) that framing does not hold in the same way: 4.8.0 showed nothing, and showing an incorrect target is not obviously better than showing none.