Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send sql error #534

Merged
merged 1 commit into from Jun 7, 2019

Conversation

Projects
None yet
4 participants
@bramley
Copy link
Contributor

commented May 12, 2019

Description

An sql syntax error was reported when running the send page from the command line

phpList - Database error 1064 while doing query  You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
phpList - Sql error SELECT * FROM phplist_message where id = 148  and owner = 

This was caused by incorrect permissions being derived when phplist is run from the command line. There are two changes

  1. The function accessLevel() returns 0 as a default value when the an admin is not logged-in. The usual way of the calling code testing this value is through a switch statement with cases such as 'owner' or 'all'. But those comparisons are "loose" which means 0 will match the first case, usually 'owner', instead of falling through to the default. The change is for accessLevel() to return 'none' instead of 0.
  2. Regardless of that problem, when run from the command line phplist doesn't seem to have a "current admin", so the result of accessLevel() is going to be wrong. The second change is to mimic 'all' permissions.

Related Issue

Screenshots (if appropriate):

@xh3n1
Copy link
Member

left a comment

Not sure, why the tests are failing on this.

@bramley

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

The change to function accessLevel() is highlighting an error elsewhere.
In home.php, when the database needs to be initialised the function PageLink2() is called to create a link to the initialise page.

    PageLink2('initialise&firstinstall=1', $GLOBALS['I18N']->get('Initialise Database')).' '.

But that function uses accessLevel() to restrict creating a link to only those admins with permission. In this case there is no admin because phplist hasn't been initialised, so an empty string is returned.

if ($access == 'owner' || $access == 'all' || $access == 'view') {
    if ($name == 'processqueue' && $hideProcessQueue) {
        return '';
    }//'<!-- '.$desc.'-->';
    elseif ($name == 'processbounces' && !MANUALLY_PROCESS_BOUNCES) {
        return '';
    } //'<!-- '.$desc.'-->';
    else {
        if (!$no_plugin && !preg_match('/&amp;pi=/i',
                $name) && isset($_GET['pi']) && isset($GLOBALS['plugins'][$_GET['pi']]) && is_object($GLOBALS['plugins'][$_GET['pi']])
        ) {
            $pi = '&amp;pi='.$_GET['pi'];
        } else {
            $pi = '';
        }

        if (!empty($_SESSION[$GLOBALS['installation_name'].'_csrf_token'])) {
            $token = '&amp;tk='.$_SESSION[$GLOBALS['installation_name'].'_csrf_token'];
        } else {
            $token = '';
        }
        $linktext = $desc;
        $linktext = str_ireplace('phplist', 'phpList', $linktext);

        return sprintf('<a href="./?page=%s%s%s%s" title="%s">%s</a>', $name, $url, $pi, $token,
            htmlspecialchars(strip_tags($title)), $linktext);
    }
}

return '';

That then makes the automated test fail.
image

Previously accessLevel() returning 0 would match with 'owner', which is wrong, and the link would be created.

@bramley

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

The same problem happens with the link to "Continue with phpList setup" after initialising the database. With the change to accessLevel() that also generates an empty link.

echo '<div id="continuesetup" style="display:none;" class="fleft">'.$GLOBALS['I18N']->get('Continue with').' '.PageLinkButton('setup',
        $GLOBALS['I18N']->get('phpList Setup')).'</div>';

@michield Any insight on this? Having accessLevel() return 0 is wrong but it's not clear whether that then allows any unauthorised access.

@michield

This comment has been minimized.

Copy link
Member

commented May 19, 2019

There used to be some code to check if the DB had been initialised and if not, it would return true for the access function. I'm not sure if that has been removed and is causing this issue?

It was in index.php. It may be that I removed it as a security issue, but it needs adding back in.

@michield

This comment has been minimized.

Copy link
Member

commented May 19, 2019

I think it was a "require_login" flag that was false when there's no admin table found. That should be used for the install process, so that the accessLevel function is not called at all.

@bramley bramley force-pushed the bramley:send_sql_error branch from a0c74fb to fe2d4f5 May 27, 2019

@bramley

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

I have reverted the change to the accessLevel() function so that can be handled separately if necessary. The change to sendemaillib.php is simply to stop an SQL syntax warning being displayed.

@samtuke samtuke requested a review from michield Jun 7, 2019

@samtuke samtuke merged commit 3298fff into phpList:master Jun 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bramley bramley deleted the bramley:send_sql_error branch Jun 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.