@Tobi_406 are you sure this is due to the api sandbox and not the api itself? The api should just be calling the relevant action specified and not override any of the behavior

The api should just be calling the relevant action specified and not override any of the behavior

Do you mean the api samdbox in that sentence?
But no, I'm not sure, I wasn't able to get it working to check with the API directly (I don't use it often, sorry).
So I used ApiSandbox and to make the task as detailed as possible I specified I used ApiSandbox.
But yes, I do think it has to do with the API, just don't have any evidence for that.

In T270713#6709472, @Tobi_406 wrote:

The api should just be calling the relevant action specified and not override any of the behavior

Do you mean the api samdbox in that sentence?
But no, I'm not sure, I wasn't able to get it working to check with the API directly (I don't use it often, sorry).
So I used ApiSandbox and to make the task as detailed as possible I specified I used ApiSandbox.
But yes, I do think it has to do with the API, just don't have any evidence for that.

Yes, sorry, I meant "the api sandbox should just be calling the relevant action specified and not override any of the behavior"

https://www.mediawiki.org/w/api.php?action=help&modules=protect
https://www.mediawiki.org/w/api.php?action=paraminfo&modules=protect

I will note that allowing users to protect higher than their rights allow isn't necessarily an issue. Being able to remove that is more of an issue.

And there's the discrepency between endpoints

The user facing form does

		$levels = $this->permManager->getNamespaceRestrictionLevels(
			$this->mTitle->getNamespace(), $this->mContext->getUser()
		);

and

			$val = $request->getVal( "mwProtect-level-$action" );
			if ( isset( $val ) && in_array( $val, $levels ) ) {
				$this->mRestrictions[$action] = $val;
			}

The API doesn't do anything with getNamespaceRestrictionLevels. No validation

We can't fix the API allowed parameters, to be documented in the autogenerated page, as it can vary between pages (or, well, Namespaces)

			'protections' => [
				ApiBase::PARAM_ISMULTI => true,
				ApiBase::PARAM_REQUIRED => true,
			],

Ok, so looking further, the problem is the way the API does the validation;

			if ( !in_array( $p[1], $this->getConfig()->get( 'RestrictionLevels' ) ) && $p[1] != 'all' ) {
				$this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
			}

It just uses effectively $wgRestrictionLevels which is not modified for the user

> var_dump( $wgRestrictionLevels );
/var/www/wiki/mediawiki/core/maintenance/eval.php(82) : eval()'d code:1:
array(3) {
  [0] =>
  string(0) ""
  [1] =>
  string(13) "autoconfirmed"
  [2] =>
  string(5) "sysop"
}

The error message is

	"apierror-protect-invalidlevel": "Invalid protection level \"$1\".",

So for a patch that should be good for production.. As yet untested...

diff --git a/includes/api/ApiProtect.php b/includes/api/ApiProtect.php
index 16f7a55a56..50f2521caf 100644
--- a/includes/api/ApiProtect.php
+++ b/includes/api/ApiProtect.php
@@ -67,6 +67,10 @@ class ApiProtect extends ApiBase {
                }
 
                $restrictionTypes = $titleObj->getRestrictionTypes();
+               $levels = $this->getPermissionManager()->getNamespaceRestrictionLevels(
+                       $titleObj->getNamespace(),
+                       $user
+               );
 
                $protections = [];
                $expiryarray = [];
@@ -85,7 +89,7 @@ class ApiProtect extends ApiBase {
                        if ( !in_array( $p[0], $restrictionTypes ) && $p[0] != 'create' ) {
                                $this->dieWithError( [ 'apierror-protect-invalidaction', wfEscapeWikiText( $p[0] ) ] );
                        }
-                       if ( !in_array( $p[1], $this->getConfig()->get( 'RestrictionLevels' ) ) && $p[1] != 'all' ) {
+                       if ( !in_array( $p[1], $levels ) && $p[1] != 'all' ) {
                                $this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
                        }

It will give the "Invalid protection level" error, which technically isn't correct, but is better than just allowing it.

Also, adding i18n messages into WMF production in security patches is painful. So we won't do that

We can do a followup/add on patch for this to add another message to be used in this case. It depends whether we really care about differentiating between "invalid" restrictions, and restrictions that are "invalid" (or rather, not allowed) for that user

Which means a patch more like

diff --git a/includes/api/ApiProtect.php b/includes/api/ApiProtect.php
index 16f7a55a56..7abedede93 100644
--- a/includes/api/ApiProtect.php
+++ b/includes/api/ApiProtect.php
@@ -67,6 +67,10 @@ class ApiProtect extends ApiBase {
                }
 
                $restrictionTypes = $titleObj->getRestrictionTypes();
+               $levels = $this->getPermissionManager()->getNamespaceRestrictionLevels(
+                       $titleObj->getNamespace(),
+                       $user
+               );
 
                $protections = [];
                $expiryarray = [];
@@ -88,6 +92,10 @@ class ApiProtect extends ApiBase {
                        if ( !in_array( $p[1], $this->getConfig()->get( 'RestrictionLevels' ) ) && $p[1] != 'all' ) {
                                $this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
                        }
+                       if ( !in_array( $p[1], $levels ) && $p[1] != 'all' ) {
+                               // TODO: Add new message for "user isn't allowed to add this protection"
+                               $this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
+                       }
 
                        if ( wfIsInfinity( $expiry[$i] ) ) {
                                $expiryarray[$p[0]] = 'infinity';

I don't imagine this is actually necessary, so we can probably just re-purpose this message for the master/deployment branch patches

Wouldn't returning a permissions error be the better message? That message should already exist as well and the user is in fact missing the permission to protect to that level.

If you can find a better pre-existing message, sure.

Like I say, I'm not adding (or altering an existing) message for the patch deployed to Wikimedia production. We can do so for a patch that eventually lands in master and release branches.

There's some similar, but not quite right messages, for example:

	"protect-locked-access": "Your account does not have permission to change page protection levels.\nHere are the current settings for the page <strong>$1</strong>:",

It kinda looks like the form for protect just silently discards it where the user doesn't have the right. so some error from the API is better than the apparent silent failure

			$val = $request->getVal( "mwProtect-level-$action" );
			if ( isset( $val ) && in_array( $val, $levels ) ) {
				$this->mRestrictions[$action] = $val;
			}

Should probably get this deployed...

Deployed the patch from T270713#6720158 to wmf.27. Logs seem fine.

What's the status here? Is this fixed in master?

In T270713#6944605, @daniel wrote:

What's the status here? Is this fixed in master?

The security patch is deployed per T270713#6794141 (also tracked at T276237) and being held for the next security release, which I'd imagine will be out in the next couple of weeks. So no, not on master yet. The tracking task for embargoed security bugs is here: T270459.

Change 678032 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_31] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/678032

Change 678038 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_35] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/678038

Change 678032 merged by jenkins-bot:

[mediawiki/core@REL1_31] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/678032

Change 678038 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/678038

Change 678073 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/678073

Change 678073 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/678073

Change 677962 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_36] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/677962

Change 677962 merged by jenkins-bot:

[mediawiki/core@REL1_36] SECURITY: Allow user to only apply protection they have right to do so via action=protect

https://gerrit.wikimedia.org/r/677962