SimpleMachines / smf-mw-auth

SMF MediaWiki Authentication
https://www.simplemachines.org
Other
23 stars 18 forks source link

Public View on MW 1.27.1 #10

Closed tazinator closed 6 years ago

tazinator commented 7 years ago

MW 1.27.1 is throwing the following error on the backend when any public user tries to view the wiki and isnt logged in to an SMF account.

PHP Catchable fatal error: Method User::__toString() must return a string value in .../includes/user/User.php on line 3747

Basically throws a 500 and no anonymous users can load any Wiki pages when the integration is enabled. Works just fine for logged in users, just not public.

jdarwood007 commented 7 years ago

Are you using the Latest commit (f096e19 on Jul 6, 2014)?

We initially had an issue with our wiki on upgrading to 1.27.1. The only change I can see in our User.php is this:

Zeus:Downloads jeremy$ diff -u original/includes/user/User.php modified/includes/user/User.php
--- original/includes/user/User.php 2016-08-22 13:53:01.000000000 -0700
+++ modified/includes/user/User.php 2016-09-28 17:22:28.000000000 -0700
@@ -3744,7 +3744,7 @@
        } elseif ( !$session->getUser()->equals( $this ) ) {
            \MediaWiki\Logger\LoggerFactory::getInstance( 'session' )
                ->warning( __METHOD__ .
-                   ": Cannot log user \"$this\" out of a user \"{$session->getUser()}\"'s session"
+                   ": Cannot log user \"this\" out of a user \"{$session->getUser()}\"'s session"
                );
            // But we still may as well make this user object anon
            $this->clearInstanceCache( 'defaults' );

Most likely this to me seems like a bug as they shouldn't be using $this since its a special reference to the current object. I hadn't debugged anymore at the time.

jdarwood007 commented 7 years ago

Actually looking at it more, it seems that the ip function isn't returning a string for some reason.

When you use $this in a string, it gets handled by the magic function __toString().

    /**
     * @return string
     */
    public function __toString() {
        return $this->getName();
    }

Which of course is handled by getName.

    /**
     * Get the user name, or the IP of an anonymous user
     * @return string User's name or IP address
     */
    public function getName() {
        if ( $this->isItemLoaded( 'name', 'only' ) ) {
            // Special case optimisation
            return $this->mName;
        } else {
            $this->load();
            if ( $this->mName === false ) {
                // Clean up IPs
                $this->mName = IP::sanitizeIP( $this->getRequest()->getIP() );
            }
            return $this->mName;
        }
    }

As it is a guest, it should return their IP

However the IP function has 2 places where a non string could be returned

    public static function sanitizeIP( $ip ) {
        $ip = trim( $ip );
        if ( $ip === '' ) {
            return null;
        }
        if ( self::isIPv4( $ip ) || !self::isIPv6( $ip ) ) {
            return $ip; // nothing else to do for IPv4 addresses or invalid ones
        }

If $ip was null or if it somehow doesn't fall into a valid ipv4 or ipv6 address. Only way to test this at this point would be to see what $this->getRequest()->getIP() is returning.

tazinator commented 7 years ago

Just as a shot, changed my user/User.php to reflect what you guys have and it seems to have resolved the issue.

Changing line 3747 to:

": Cannot log user \"this\" out of a user \"{$session->getUser()}\"'s session"

legoktm commented 7 years ago

I filed https://phabricator.wikimedia.org/T148486 upstream for this.

jdarwood007 commented 6 years ago

This was fixed by MediaWiki and I haven't had any additional reports on this since then. Closing for now, if it still occurs we can reinvestigate this.