[DATABASE] Disable 'NULL' strings evaluation as SQL NULLs

Use $object->sqlValue('NULL') (identical to DataObject_Cast'ing) instead and
fix related issues like (email|sms)settings considering these NULLs as a
false positive for the E-Mail address still being set when it's been removed.

There could also be security implications to the now-disabled approach of
considering 'NULL' strings as SQL NULLs.
This commit is contained in:
Alexei Sorokin 2019-11-02 12:21:43 +03:00 committed by Diogo Peralta Cordeiro
parent fd68965255
commit eab5725698
10 changed files with 89 additions and 24 deletions

View File

@ -92,7 +92,7 @@ class EmailsettingsAction extends SettingsAction
$this->element('legend', null, _('Email address')); $this->element('legend', null, _('Email address'));
$this->hidden('token', common_session_token()); $this->hidden('token', common_session_token());
if ($user->email) { if (!$user->isNull('email')) {
$this->element('p', array('id' => 'form_confirmed'), $user->email); $this->element('p', array('id' => 'form_confirmed'), $user->email);
// TRANS: Form note in e-mail settings form. // TRANS: Form note in e-mail settings form.
$this->element('p', array('class' => 'form_note'), _('Current confirmed email address.')); $this->element('p', array('class' => 'form_note'), _('Current confirmed email address.'));
@ -163,7 +163,7 @@ class EmailsettingsAction extends SettingsAction
$this->elementStart('div', array('id' => 'emailincoming')); $this->elementStart('div', array('id' => 'emailincoming'));
if ($user->incomingemail) { if (!$user->isNull('incomingemail')) {
$this->elementStart('p'); $this->elementStart('p');
$this->element('span', 'address', $user->incomingemail); $this->element('span', 'address', $user->incomingemail);
// @todo XXX: Looks a little awkward in the UI. // @todo XXX: Looks a little awkward in the UI.
@ -180,7 +180,7 @@ class EmailsettingsAction extends SettingsAction
} }
$this->elementStart('p'); $this->elementStart('p');
if ($user->incomingemail) { if (!$user->isNull('incomingemail')) {
// TRANS: Instructions for incoming e-mail address input form, when an address has already been assigned. // TRANS: Instructions for incoming e-mail address input form, when an address has already been assigned.
$msg = _('Make a new email address for posting to; '. $msg = _('Make a new email address for posting to; '.
'cancels the old one.'); 'cancels the old one.');
@ -435,7 +435,7 @@ class EmailsettingsAction extends SettingsAction
} }
$original = clone($user); $original = clone($user);
$user->email = DB_DataObject_Cast::sql('NULL'); $user->email = $user->sqlValue('NULL');
// Throws exception on failure. Also performs it within a transaction. // Throws exception on failure. Also performs it within a transaction.
$user->updateWithKeys($original); $user->updateWithKeys($original);
@ -458,7 +458,7 @@ class EmailsettingsAction extends SettingsAction
} }
$orig = clone($user); $orig = clone($user);
$user->incomingemail = DB_DataObject_Cast::sql('NULL'); $user->incomingemail = $user->sqlValue('NULL');
$user->emailpost = false; $user->emailpost = false;
// Throws exception on failure. Also performs it within a transaction. // Throws exception on failure. Also performs it within a transaction.
$user->updateWithKeys($orig); $user->updateWithKeys($orig);

View File

@ -98,7 +98,7 @@ class SmssettingsAction extends SettingsAction
$this->element('legend', null, _('SMS address')); $this->element('legend', null, _('SMS address'));
$this->hidden('token', common_session_token()); $this->hidden('token', common_session_token());
if ($user->sms) { if (!$user->isNull('sms')) {
$carrier = $user->getCarrier(); $carrier = $user->getCarrier();
$this->element( $this->element(
'p', 'p',
@ -170,13 +170,13 @@ class SmssettingsAction extends SettingsAction
} }
$this->elementEnd('fieldset'); $this->elementEnd('fieldset');
if ($user->sms) { if (!$user->isNull('sms')) {
$this->elementStart('fieldset', ['id' => 'settings_sms_incoming_email']); $this->elementStart('fieldset', ['id' => 'settings_sms_incoming_email']);
// XXX: Confused! This is about SMS. Should this message be updated? // XXX: Confused! This is about SMS. Should this message be updated?
// TRANS: Form legend for incoming SMS settings form. // TRANS: Form legend for incoming SMS settings form.
$this->element('legend', null, _('Incoming email')); $this->element('legend', null, _('Incoming email'));
if ($user->incomingemail) { if (!$user->isNull('incomingemail')) {
$this->element('p', 'form_unconfirmed', $user->incomingemail); $this->element('p', 'form_unconfirmed', $user->incomingemail);
$this->element( $this->element(
'p', 'p',
@ -417,9 +417,9 @@ class SmssettingsAction extends SettingsAction
$original = clone($user); $original = clone($user);
$user->sms = DB_DataObject_Cast::sql('NULL'); $user->sms = $user->sqlValue('NULL');
$user->carrier = DB_DataObject_Cast::sql('NULL'); $user->carrier = $user->sqlValue('NULL');
$user->smsemail = DB_DataObject_Cast::sql('NULL'); $user->smsemail = $user->sqlValue('NULL');
// Throws exception on failure. Also performs it within a transaction. // Throws exception on failure. Also performs it within a transaction.
$user->updateWithKeys($original); $user->updateWithKeys($original);
@ -531,7 +531,7 @@ class SmssettingsAction extends SettingsAction
$orig = clone($user); $orig = clone($user);
$user->incomingemail = DB_DataObject_Cast::sql('NULL'); $user->incomingemail = $user->sqlValue('NULL');
// Throws exception on failure. Also performs it within a transaction. // Throws exception on failure. Also performs it within a transaction.
$user->updateWithKeys($orig); $user->updateWithKeys($orig);

View File

@ -106,7 +106,7 @@ class Conversation extends Managed_DataObject
common_random_hexstr(8) common_random_hexstr(8)
); );
// locally generated Conversation objects don't get static URLs stored // locally generated Conversation objects don't get static URLs stored
$conv->url = DB_DataObject_Cast::sql('NULL'); $conv->url = $conv->sqlValue('NULL');
} }
// This insert throws exceptions on failure // This insert throws exceptions on failure
$conv->insert(); $conv->insert();

View File

@ -283,6 +283,8 @@ abstract class Managed_DataObject extends Memcached_DataObject
* Memcached_DataObject doesn't have enough info to handle properly. * Memcached_DataObject doesn't have enough info to handle properly.
* *
* @return array of strings * @return array of strings
* @throws MethodNotImplementedException
* @throws ServerException
*/ */
public function _allCacheKeys() public function _allCacheKeys()
{ {
@ -440,6 +442,32 @@ abstract class Managed_DataObject extends Memcached_DataObject
return intval($this->id); return intval($this->id);
} }
/**
* Check whether the column is NULL in SQL
*
* @param string $key column property name
*
* @return bool
*/
public function isNull(string $key): bool
{
if (array_key_exists($key, get_object_vars($this))
&& is_null($this->$key)) {
// If there was no fetch, this is a false positive.
return true;
} elseif (is_object($this->$key)
&& $this->$key instanceof DB_DataObject_Cast
&& $this->$key->type === 'sql') {
// This is cast to raw SQL, let's see if it's NULL.
return (strcasecmp($this->$key->value, 'NULL') == 0);
} elseif (DB_DataObject::_is_null($this, $key)) {
// DataObject's NULL magic should be disabled,
// this is just for completeness.
return true;
}
return false;
}
/** /**
* WARNING: Only use this on Profile and Notice. We should probably do * WARNING: Only use this on Profile and Notice. We should probably do
* this with traits/"implements" or whatever, but that's over the top * this with traits/"implements" or whatever, but that's over the top
@ -516,10 +544,20 @@ abstract class Managed_DataObject extends Memcached_DataObject
// do it in a transaction // do it in a transaction
$this->query('BEGIN'); $this->query('BEGIN');
$parts = array(); $parts = [];
foreach ($this->keys() as $k) { foreach ($this->keys() as $k) {
if (strcmp($this->$k, $orig->$k) != 0) { $v = $this->table()[$k];
$parts[] = $k . ' = ' . $this->_quote($this->$k); if ($this->$k !== $orig->$k) {
if (is_object($this->$k) && $this->$k instanceof DB_DataObject_Cast) {
$value = $this->$k->toString($v, $this->getDatabaseConnection());
} elseif (DB_DataObject::_is_null($this, $k)) {
$value = 'NULL';
} elseif ($v & DB_DATAOBJECT_STR) { // if a string
$value = $this->_quote((string) $this->$k);
} else {
$value = (int) $this->$k;
}
$parts[] = "{$k} = {$value}";
} }
} }
if (count($parts) == 0) { if (count($parts) == 0) {

View File

@ -972,9 +972,15 @@ class Memcached_DataObject extends Safe_DataObject
case 'date': case 'date':
$vstr = "{$v->year} - {$v->month} - {$v->day}"; $vstr = "{$v->year} - {$v->month} - {$v->day}";
break; break;
case 'sql':
if (strcasecmp($v->value, 'NULL') == 0) {
// Very selectively handling NULLs.
$vstr = '';
break;
}
// fallthrough
case 'blob': case 'blob':
case 'string': case 'string':
case 'sql':
case 'datetime': case 'datetime':
case 'time': case 'time':
// Low level exception. No need for i18n as discussed with Brion. // Low level exception. No need for i18n as discussed with Brion.

View File

@ -4168,8 +4168,7 @@ class DB_DataObject
$method = $value; $method = $value;
$value = func_get_arg(1); $value = func_get_arg(1);
} }
//require_once 'DB/DataObject/Cast.php'; require_once 'DB/DataObject/Cast.php';
require_once 'Cast.php';
return call_user_func(array('DB_DataObject_Cast', $method), $value); return call_user_func(array('DB_DataObject_Cast', $method), $value);
} }

View File

@ -74,6 +74,7 @@ $default =
'mirror' => null, 'mirror' => null,
'utf8' => true, 'utf8' => true,
'db_driver' => 'DB', # XXX: JanRain libs only work with DB 'db_driver' => 'DB', # XXX: JanRain libs only work with DB
'disable_null_strings' => true, // 'NULL' can be harmful
'quote_identifiers' => true, 'quote_identifiers' => true,
'type' => 'mysql', 'type' => 'mysql',
'schemacheck' => 'runtime', // 'runtime' or 'script' 'schemacheck' => 'runtime', // 'runtime' or 'script'

View File

@ -75,7 +75,7 @@ class FavoriteModule extends ActivityVerbHandlerModule
while ($user->fetch()) { while ($user->fetch()) {
$user->setPref('email', 'notify_fave', $user->emailnotifyfav); $user->setPref('email', 'notify_fave', $user->emailnotifyfav);
$orig = clone($user); $orig = clone($user);
$user->emailnotifyfav = 'null'; // flag this preference as migrated $user->emailnotifyfav = $user->sqlValue('NULL'); // flag this preference as migrated
$user->update($orig); $user->update($orig);
} }
printfnq("DONE.\n"); printfnq("DONE.\n");

View File

@ -480,7 +480,7 @@ class FeedSub extends Managed_DataObject
$this->sub_end = common_sql_date(time() + $lease_seconds); $this->sub_end = common_sql_date(time() + $lease_seconds);
} else { } else {
// Backwards compatibility to StatusNet (PuSH <0.4 supported permanent subs) // Backwards compatibility to StatusNet (PuSH <0.4 supported permanent subs)
$this->sub_end = DB_DataObject_Cast::sql('NULL'); $this->sub_end = $this->sqlValue('NULL');
} }
$this->modified = common_sql_now(); $this->modified = common_sql_now();
@ -496,10 +496,10 @@ class FeedSub extends Managed_DataObject
{ {
$original = clone($this); $original = clone($this);
$this->secret = DB_DataObject_Cast::sql('NULL'); $this->secret = $this->sqlValue('NULL');
$this->sub_state = 'inactive'; $this->sub_state = 'inactive';
$this->sub_start = DB_DataObject_Cast::sql('NULL'); $this->sub_start = $this->sqlValue('NULL');
$this->sub_end = DB_DataObject_Cast::sql('NULL'); $this->sub_end = $this->sqlValue('NULL');
$this->modified = common_sql_now(); $this->modified = common_sql_now();
return $this->update($original); return $this->update($original);

View File

@ -62,6 +62,7 @@ function main()
fixupNoticeConversation(); fixupNoticeConversation();
initConversation(); initConversation();
fixupUserBadNulls();
fixupGroupURI(); fixupGroupURI();
if ($iterate_files) { if ($iterate_files) {
printfnq("Running file iterations:\n"); printfnq("Running file iterations:\n");
@ -121,6 +122,26 @@ function updateSchemaPlugins()
printfnq("DONE.\n"); printfnq("DONE.\n");
} }
function fixupUserBadNulls(): void
{
printfnq("Ensuring all users have no empty strings for NULLs...");
foreach (['email', 'incomingemail', 'sms', 'smsemail'] as $col) {
$user = new User();
$user->whereAdd("{$col} = ''");
if ($user->find()) {
while ($user->fetch()) {
$sql = "UPDATE {$user->escapedTableName()} SET {$col} = NULL "
. "WHERE id = {$user->id}";
$user->query($sql);
}
}
}
printfnq("DONE.\n");
}
function fixupNoticeConversation() function fixupNoticeConversation()
{ {
printfnq("Ensuring all notices have a conversation ID..."); printfnq("Ensuring all notices have a conversation ID...");