Separate right for foreign user js redirects

Require a new editmyuserjsredirect permission for users to edit
Javascript redirects in their userspace when the redirect target
is not in their userspace (unless they have edituserjs and can
edit any user JS anyway). This is to prevent attacks where a
popular userscript has been moved into the system namespace or
another safe location but many users still load it through the
original userspace redirect, and the attacker manages to take
over the userspace by compromising the account or getting it
renamed.

Since this is only a concern on large community wikis, by
default all users have the editmyuserjsredirect permission.

Bug: T207750
Change-Id: I36a879d5da04cb6f49ed1bc40dbe144f6862c6a1
Depends-On: I072cf857c1fff4578402904aa9cb5a0c8833f16f

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 60d5d96..3deeaab 100644 (file)
--- a/RELEASE-NOTES-1.34
+++ b/RELEASE-NOTES-1.34
@@ -36,6 +36,9 @@ For notes on 1.33.x and older releases, see HISTORY.
 * $wgEnableSpecialMute (T218265) - This configuration controls whether
   Special:Mute is available and whether to include a link to it on emails
   originating from Special:Email.
+* editmyuserjsredirect user right – users without this right now cannot edit JS
+  redirects in their userspace unless the target of the redirect is also in
+  their userspace. By default, this right is given to everyone.
 
 ==== Changed configuration ====
 * $wgUseCdn, $wgCdnServers, $wgCdnServersNoPurge, and $wgCdnMaxAge – These four

diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 3bfc8f8..107c546 100644 (file)
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -5173,6 +5173,7 @@ $wgGroupPermissions['user']['minoredit'] = true;
 $wgGroupPermissions['user']['editmyusercss'] = true;
 $wgGroupPermissions['user']['editmyuserjson'] = true;
 $wgGroupPermissions['user']['editmyuserjs'] = true;
+$wgGroupPermissions['user']['editmyuserjsredirect'] = true;
 $wgGroupPermissions['user']['purge'] = true;
 $wgGroupPermissions['user']['sendemail'] = true;
 $wgGroupPermissions['user']['applychangetags'] = true;

diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php
index 98a5b17..483c171 100644 (file)
--- a/includes/Permissions/PermissionManager.php
+++ b/includes/Permissions/PermissionManager.php
@@ -23,6 +23,8 @@ use Action;
 use Exception;
 use Hooks;
 use MediaWiki\Linker\LinkTarget;
+use MediaWiki\Revision\RevisionLookup;
+use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Session\SessionManager;
 use MediaWiki\Special\SpecialPageFactory;
 use MediaWiki\User\UserIdentity;
@@ -55,6 +57,9 @@ class PermissionManager {
        /** @var SpecialPageFactory */
        private $specialPageFactory;
 
+       /** @var RevisionLookup */
+       private $revisionLookup;
+
        /** @var string[] List of pages names anonymous user may see */
        private $whitelistRead;
 
@@ -130,6 +135,7 @@ class PermissionManager {
                'editmyusercss',
                'editmyuserjson',
                'editmyuserjs',
+               'editmyuserjsredirect',
                'editmywatchlist',
                'editsemiprotected',
                'editsitecss',
@@ -184,6 +190,7 @@ class PermissionManager {
 
        /**
         * @param SpecialPageFactory $specialPageFactory
+        * @param RevisionLookup $revisionLookup
         * @param string[] $whitelistRead
         * @param string[] $whitelistReadRegexp
         * @param bool $emailConfirmToEdit
@@ -195,6 +202,7 @@ class PermissionManager {
         */
        public function __construct(
                SpecialPageFactory $specialPageFactory,
+               RevisionLookup $revisionLookup,
                $whitelistRead,
                $whitelistReadRegexp,
                $emailConfirmToEdit,
@@ -205,6 +213,7 @@ class PermissionManager {
                NamespaceInfo $nsInfo
        ) {
                $this->specialPageFactory = $specialPageFactory;
+               $this->revisionLookup = $revisionLookup;
                $this->whitelistRead = $whitelistRead;
                $this->whitelistReadRegexp = $whitelistReadRegexp;
                $this->emailConfirmToEdit = $emailConfirmToEdit;
@@ -1134,6 +1143,20 @@ class PermissionManager {
                                && !$user->isAllowedAny( 'editmyuserjs', 'edituserjs' )
                        ) {
                                $errors[] = [ 'mycustomjsprotected', $action ];
+                       } elseif (
+                               $page->isUserJsConfigPage()
+                               && !$user->isAllowedAny( 'edituserjs', 'editmyuserjsredirect' )
+                       ) {
+                               // T207750 - do not allow users to edit a redirect if they couldn't edit the target
+                               $rev = $this->revisionLookup->getRevisionByTitle( $page );
+                               $content = $rev ? $rev->getContent( 'main', RevisionRecord::RAW ) : null;
+                               $target = $content ? $content->getUltimateRedirectTarget() : null;
+                               if ( $target && (
+                                               !$target->inNamespace( NS_USER )
+                                               || !preg_match( '/^' . preg_quote( $user->getName(), '/' ) . '\//', $target->getText() )
+                               ) ) {
+                                       $errors[] = [ 'mycustomjsredirectprotected', $action ];
+                               }
                        }
                } else {
                        // Users need editmyuser* to edit their own CSS/JSON/JS subpages, except for

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 7d2b3cb..8f9044e 100644 (file)
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -466,6 +466,7 @@ return [
                $config = $services->getMainConfig();
                return new PermissionManager(
                        $services->getSpecialPageFactory(),
+                       $services->getRevisionLookup(),
                        $config->get( 'WhitelistRead' ),
                        $config->get( 'WhitelistReadRegexp' ),
                        $config->get( 'EmailConfirmToEdit' ),

diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 0f40121..272b97d 100644 (file)
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -1273,6 +1273,7 @@
        "right-editmyusercss": "Edit your own user CSS files",
        "right-editmyuserjson": "Edit your own user JSON files",
        "right-editmyuserjs": "Edit your own user JavaScript files",
+       "right-editmyuserjsredirect": "Edit your own user JavaScript files that are redirects",
        "right-viewmywatchlist": "View your own watchlist",
        "right-editmywatchlist": "Edit your own watchlist. Note some actions will still add pages even without this right.",
        "right-viewmyprivateinfo": "View your own private data (e.g. email address, real name)",
@@ -1403,6 +1404,7 @@
        "action-editmyusercss": "edit your own user CSS files",
        "action-editmyuserjson": "edit your own user JSON files",
        "action-editmyuserjs": "edit your own user JavaScript files",
+       "action-editmyuserjsredirect": "edit your own user JavaScript files that are redirects",
        "action-viewsuppressed": "view revisions hidden from any user",
        "action-hideuser": "block a username, hiding it from the public",
        "action-ipblock-exempt": "bypass IP blocks, auto-blocks and range blocks",
@@ -4236,6 +4238,7 @@
        "passwordpolicies-policy-passwordnotinlargeblacklist": "Password cannot be in the list of 100,000 most commonly used passwords.",
        "passwordpolicies-policyflag-forcechange": "must change on login",
        "passwordpolicies-policyflag-suggestchangeonlogin": "suggest change on login",
+       "mycustomjsredirectprotected": "You do not have permission to edit this JavaScript page because it is a redirect and it does not point inside your userspace.",
        "easydeflate-invaliddeflate": "Content provided is not properly deflated",
        "unprotected-js": "For security reasons JavaScript cannot be loaded from unprotected pages. Please only create javascript in the MediaWiki: namespace or as a User subpage",
        "userlogout-continue": "Do you want to log out?"

diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index 965f163..3032a55 100644 (file)
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -1481,7 +1481,8 @@
        "right-editsitejs": "{{doc-right|editsitejs}}",
        "right-editmyusercss": "{{doc-right|editmyusercss}}\nSee also:\n* {{msg-mw|Right-editusercss}}",
        "right-editmyuserjson": "{{doc-right|editmyuserjson}}\nSee also:\n* {{msg-mw|Right-edituserjson}}",
-       "right-editmyuserjs": "{{doc-right|editmyuserjs}}\nSee also:\n* {{msg-mw|Right-edituserjs}}",
+       "right-editmyuserjs": "{{doc-right|editmyuserjs}}\nSee also:\n* {{msg-mw|Right-edituserjs}}\n* {{msg-mw|Right-editmyuserjsredirect}}",
+       "right-editmyuserjsredirect": "{{doc-right|editmyuserjsredirect}}\nSame as {{msg-mw|Right-editmyuserjs}} except if page is a redirect.\n\nSee also:\n* {{msg-mw|Right-edituserjs}}",
        "right-viewmywatchlist": "{{doc-right|viewmywatchlist}}",
        "right-editmywatchlist": "{{doc-right|editmywatchlist}}",
        "right-viewmyprivateinfo": "{{doc-right|viewmyprivateinfo}}",
@@ -1612,6 +1613,7 @@
        "action-editmyusercss": "{{doc-action|editmyusercss}}",
        "action-editmyuserjson": "{{doc-action|editmyuserjson}}",
        "action-editmyuserjs": "{{doc-action|editmyuserjs}}",
+       "action-editmyuserjsredirect": "{{doc-action|editmyuserjsredirect}}",
        "action-viewsuppressed": "{{doc-action|viewsuppressed}}",
        "action-hideuser": "{{doc-action|hideuser}}",
        "action-ipblock-exempt": "{{doc-action|ipblock-exempt}}",
@@ -4445,6 +4447,7 @@
        "passwordpolicies-policy-passwordnotinlargeblacklist": "Password policy that enforces that a password is not in a list of 100,000 number of \"popular\" passwords.",
        "passwordpolicies-policyflag-forcechange": "Password policy flag that enforces changing invalid passwords on login.",
        "passwordpolicies-policyflag-suggestchangeonlogin": "Password policy flag that suggests changing invalid passwords on login.",
+       "mycustomjsredirectprotected": "Error message shown when user tries to edit their own JS page that is a foreign redirect without the 'mycustomjsredirectprotected' right. See also {{mw-msg|mycustomjsprotected}}.",
        "easydeflate-invaliddeflate": "Error message if the content passed to easydeflate was not deflated (compressed) properly",
        "unprotected-js": "Error message shown when trying to load javascript via action=raw that is not protected",
        "userlogout-continue": "Shown if user attempted to log out without a token specified. Probably the user clicked on an old link that hasn't been updated to use the new system. $1 - url that user should click on in order to log out."

diff --git a/tests/phpunit/includes/Permissions/PermissionManagerTest.php b/tests/phpunit/includes/Permissions/PermissionManagerTest.php
index 03b35b5..61dcac4 100644 (file)
--- a/tests/phpunit/includes/Permissions/PermissionManagerTest.php
+++ b/tests/phpunit/includes/Permissions/PermissionManagerTest.php
@@ -3,21 +3,25 @@
 namespace MediaWiki\Tests\Permissions;
 
 use Action;
+use ContentHandler;
 use FauxRequest;
-use MediaWiki\Session\SessionId;
-use MediaWiki\Session\TestUtils;
-use MediaWikiLangTestCase;
-use RequestContext;
-use stdClass;
-use Title;
-use User;
 use MediaWiki\Block\DatabaseBlock;
 use MediaWiki\Block\Restriction\NamespaceRestriction;
 use MediaWiki\Block\Restriction\PageRestriction;
 use MediaWiki\Block\SystemBlock;
+use MediaWiki\Linker\LinkTarget;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Permissions\PermissionManager;
+use MediaWiki\Revision\MutableRevisionRecord;
+use MediaWiki\Revision\RevisionLookup;
 use Wikimedia\ScopedCallback;
+use MediaWiki\Session\SessionId;
+use MediaWiki\Session\TestUtils;
+use MediaWikiLangTestCase;
+use RequestContext;
+use stdClass;
+use Title;
+use User;
 use Wikimedia\TestingAccessWrapper;
 
 /**
@@ -698,6 +702,64 @@ class PermissionManagerTest extends MediaWikiLangTestCase {
                );
        }
 
+       public function testJsConfigRedirectEditPermissions() {
+               $revision = null;
+               $user = $this->getTestUser()->getUser();
+               $otherUser = $this->getTestUser( 'sysop' )->getUser();
+               $localJsTitle = Title::newFromText( 'User:' . $user->getName() . '/foo.js' );
+               $otherLocalJsTitle = Title::newFromText( 'User:' . $user->getName() . '/foo2.js' );
+               $nonlocalJsTitle = Title::newFromText( 'User:' . $otherUser->getName() . '/foo.js' );
+
+               $services = MediaWikiServices::getInstance();
+               $revisionLookup = $this->getMockBuilder( RevisionLookup::class )
+                       ->setMethods( [ 'getRevisionByTitle' ] )
+                       ->getMockForAbstractClass();
+               $revisionLookup->method( 'getRevisionByTitle' )
+                       ->willReturnCallback( function ( LinkTarget $page ) use (
+                               $services, &$revision, $localJsTitle
+                       ) {
+                               if ( $localJsTitle->equals( Title::newFromLinkTarget( $page ) ) ) {
+                                       return $revision;
+                               } else {
+                                       return $services->getRevisionLookup()->getRevisionByTitle( $page );
+                               }
+                       } );
+               $permissionManager = new PermissionManager(
+                       $services->getSpecialPageFactory(),
+                       $revisionLookup,
+                       [],
+                       [],
+                       false,
+                       false,
+                       [],
+                       [],
+                       [],
+                       MediaWikiServices::getInstance()->getNamespaceInfo()
+               );
+               $this->setService( 'PermissionManager', $permissionManager );
+
+               $permissionManager->overrideUserRightsForTesting( $user, [ 'edit', 'editmyuserjs' ] );
+
+               $revision = $this->getJavascriptRevision( $localJsTitle, $user, '/* script */' );
+               $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle );
+               $this->assertSame( [], $errors );
+
+               $revision = $this->getJavascriptRedirectRevision( $localJsTitle, $otherLocalJsTitle, $user );
+               $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle );
+               $this->assertSame( [], $errors );
+
+               $revision = $this->getJavascriptRedirectRevision( $localJsTitle, $nonlocalJsTitle, $user );
+               $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle );
+               $this->assertSame( [ [ 'mycustomjsredirectprotected', 'edit' ] ], $errors );
+
+               $permissionManager->overrideUserRightsForTesting( $user,
+                       [ 'edit', 'editmyuserjs', 'editmyuserjsredirect' ] );
+
+               $revision = $this->getJavascriptRedirectRevision( $localJsTitle, $nonlocalJsTitle, $user );
+               $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle );
+               $this->assertSame( [], $errors );
+       }
+
        /**
         * @todo This test method should be split up into separate test methods and
         * data providers
@@ -1684,4 +1746,35 @@ class PermissionManagerTest extends MediaWikiLangTestCase {
                $this->assertFalse( $permissionManager->userHasRight( $this->user, 'move' ) );
        }
 
+       /**
+        * Create a RevisionRecord with a single Javascript main slot.
+        * @param Title $title
+        * @param User $user
+        * @param string $text
+        * @return MutableRevisionRecord
+        */
+       private function getJavascriptRevision( Title $title, User $user, $text ) {
+               $content = ContentHandler::makeContent( $text, $title, CONTENT_MODEL_JAVASCRIPT );
+               $revision = new MutableRevisionRecord( $title );
+               $revision->setContent( 'main', $content );
+               return $revision;
+       }
+
+       /**
+        * Create a RevisionRecord with a single Javascript redirect main slot.
+        * @param Title $title
+        * @param Title $redirectTargetTitle
+        * @param User $user
+        * @return MutableRevisionRecord
+        */
+       private function getJavascriptRedirectRevision(
+               Title $title, Title $redirectTargetTitle, User $user
+       ) {
+               $content = ContentHandler::getForModelID( CONTENT_MODEL_JAVASCRIPT )
+                       ->makeRedirectContent( $redirectTargetTitle );
+               $revision = new MutableRevisionRecord( $title );
+               $revision->setContent( 'main', $content );
+               return $revision;
+       }
+
 }