Fehler in der admin_users.php

MrMind · Beitrag von **MrMind** » 23.09.2004 09:41

Moin Leutz,

habe mir gestern mal local das neue phpBB 2.0.10 installiert und wollte auch glei was bearbeiten bzw. neues machen.

Hab ein bissl Copy & Paste gemacht und einiges verändert, aber es hat nie geklappt.
Es gab noch net mal eine Meldung, das was fehlgeschlagen hat.

Nach dem ich mir meine Datei angeguckt habe, ist mir auch aufgefallen wieso.
Dachte erst, das war bestimmt ein doofer Bug von mir und wollte sicherheitshalber nochmal in der orginalen Datei nachgucken (admin_users.php): Doch da ist es genauso.

Und zwar erklärt mir mal wie er einen Fehler ausgeben soll, wenn die die DB-Abfrage nicht klappt:

Code: Alles auswählen

if( !$error )
		{
			$sql = "UPDATE " . USERS_TABLE . "
				SET " . $username_sql . $passwd_sql . "user_email = '" . str_replace("\'", "''", $email) . "', user_icq = '" . str_replace("\'", "''", $icq) . "', user_website = '" . str_replace("\'", "''", $website) . "', user_occ = '" . str_replace("\'", "''", $occupation) . "', user_from = '" . str_replace("\'", "''", $location) . "', user_interests = '" . str_replace("\'", "''", $interests) . "', user_sig = '" . str_replace("\'", "''", $signature) . "', user_viewemail = $viewemail, user_aim = '" . str_replace("\'", "''", $aim) . "', user_yim = '" . str_replace("\'", "''", $yim) . "', user_msnm = '" . str_replace("\'", "''", $msn) . "', user_attachsig = $attachsig, user_sig_bbcode_uid = '$signature_bbcode_uid', user_allowsmile = $allowsmilies, user_allowhtml = $allowhtml, user_allowavatar = $user_allowavatar, user_allowbbcode = $allowbbcode, user_allow_viewonline = $allowviewonline, user_notify = $notifyreply, user_allow_pm = $user_allowpm, user_notify_pm = $notifypm, user_popup_pm = $popuppm, user_lang = '" . str_replace("\'", "''", $user_lang) . "', user_style = $user_style, user_timezone = $user_timezone, user_dateformat = '" . str_replace("\'", "''", $user_dateformat) . "', user_active = $user_status, user_rank = $user_rank" . $avatar_sql . "
				WHERE user_id = $user_id";

			if( $result = $db->sql_query($sql) )
			{
				if( isset($rename_user) )
				{
					$sql = "UPDATE " . GROUPS_TABLE . "
						SET group_name = '".str_replace("\'", "''", $rename_user)."'
						WHERE group_name = '".str_replace("'", "''", $this_userdata['username'] )."'";
					if( !$result = $db->sql_query($sql) )
					{
						message_die(GENERAL_ERROR, 'Could not rename users group', '', __LINE__, __FILE__, $sql);
					}
				}
				
				// Delete user session, to prevent the user navigating the forum (if logged in) when disabled
				if (!$user_status)
				{
					$sql = "DELETE FROM " . SESSIONS_TABLE . " 
						WHERE session_user_id = " . $user_id;

					if ( !$db->sql_query($sql) )
					{
						message_die(GENERAL_ERROR, 'Error removing user session', '', __LINE__, __FILE__, $sql);
					}
				}
				
				$message .= $lang['Admin_user_updated'];
			}
			else
			{
				$error = TRUE;
				$error_msg .= ( ( isset($error_msg) ) ? '<br />' : '' ) . $lang['Admin_user_fail'];
			}

			$message .= '<br /><br />' . sprintf($lang['Click_return_useradmin'], '<a href="' . append_sid("admin_users.$phpEx") . '">', '</a>') . '<br /><br />' . sprintf($lang['Click_return_admin_index'], '<a href="' . append_sid("index.$phpEx?pane=right") . '">', '</a>');

			message_die(GENERAL_MESSAGE, $message);
		}
		else
		{
			$template->set_filenames(array(
				'reg_header' => 'error_body.tpl')
			);

			$template->assign_vars(array(
				'ERROR_MESSAGE' => $error_msg)
			);

			$template->assign_var_from_handle('ERROR_BOX', 'reg_header');

			$username = htmlspecialchars(stripslashes($username));
			$email = stripslashes($email);
			$password = '';
			$password_confirm = '';

			$icq = stripslashes($icq);
			$aim = htmlspecialchars(str_replace('+', ' ', stripslashes($aim)));
			$msn = htmlspecialchars(stripslashes($msn));
			$yim = htmlspecialchars(stripslashes($yim));

			$website = htmlspecialchars(stripslashes($website));
			$location = htmlspecialchars(stripslashes($location));
			$occupation = htmlspecialchars(stripslashes($occupation));
			$interests = htmlspecialchars(stripslashes($interests));
			$signature = htmlspecialchars(stripslashes($signature));

			$user_lang = stripslashes($user_lang);
			$user_dateformat = htmlspecialchars(stripslashes($user_dateformat));
		}
	}

Auszug aus der Orginalen admin_users.php

Der If-Block wird nur dann aktiv, wenn die Variable $error vorher noch net auf true gesetzt wird.

Der Witz bei der Sache ist:

Scheitert die DB-Abfrage wird $error auf True gesetzt. Da aber die Abfrage mit $error schon längst vorbei ist, kann das Script damit nichts mehr anfangen und nach dem If-Block kommt der (wie üblich) Else-Block, der die Fehler behandelt (wenn sie vor der If-Else-Konstruktion passiert sind).

Lösung wäre folgendes:

Code: Alles auswählen

if( !$error )
		{
			$sql = "UPDATE " . USERS_TABLE . "
				SET " . $username_sql . $passwd_sql . "user_email = '" . str_replace("\'", "''", $email) . "', user_icq = '" . str_replace("\'", "''", $icq) . "', user_website = '" . str_replace("\'", "''", $website) . "', user_occ = '" . str_replace("\'", "''", $occupation) . "', user_from = '" . str_replace("\'", "''", $location) . "', user_interests = '" . str_replace("\'", "''", $interests) . "', user_sig = '" . str_replace("\'", "''", $signature) . "', user_viewemail = $viewemail, user_aim = '" . str_replace("\'", "''", $aim) . "', user_yim = '" . str_replace("\'", "''", $yim) . "', user_msnm = '" . str_replace("\'", "''", $msn) . "', user_attachsig = $attachsig, user_sig_bbcode_uid = '$signature_bbcode_uid', user_allowsmile = $allowsmilies, user_allowhtml = $allowhtml, user_allowavatar = $user_allowavatar, user_allowbbcode = $allowbbcode, user_allow_viewonline = $allowviewonline, user_notify = $notifyreply, user_allow_pm = $user_allowpm, user_notify_pm = $notifypm, user_popup_pm = $popuppm, user_lang = '" . str_replace("\'", "''", $user_lang) . "', user_style = $user_style, user_timezone = $user_timezone, user_dateformat = '" . str_replace("\'", "''", $user_dateformat) . "', user_active = $user_status, user_rank = $user_rank" . $avatar_sql . "
				WHERE user_id = $user_id";

			if( $result = $db->sql_query($sql) )
			{
				if( isset($rename_user) )
				{
					$sql = "UPDATE " . GROUPS_TABLE . "
						SET group_name = '".str_replace("\'", "''", $rename_user)."'
						WHERE group_name = '".str_replace("'", "''", $this_userdata['username'] )."'";
					if( !$result = $db->sql_query($sql) )
					{
						message_die(GENERAL_ERROR, 'Could not rename users group', '', __LINE__, __FILE__, $sql);
					}
				}
				
				// Delete user session, to prevent the user navigating the forum (if logged in) when disabled
				if (!$user_status)
				{
					$sql = "DELETE FROM " . SESSIONS_TABLE . " 
						WHERE session_user_id = " . $user_id;

					if ( !$db->sql_query($sql) )
					{
						message_die(GENERAL_ERROR, 'Error removing user session', '', __LINE__, __FILE__, $sql);
					}
				}
				
				$message .= $lang['Admin_user_updated'];
                                $message .= '<br /><br />' . sprintf($lang['Click_return_useradmin'], '<a href="' . append_sid("admin_users.$phpEx") . '">', '</a>') . '<br /><br />' . sprintf($lang['Click_return_admin_index'], '<a href="' . append_sid("index.$phpEx?pane=right") . '">', '</a>');

			message_die(GENERAL_MESSAGE, $message);
			}
			else
			{
				$error = TRUE;
				$error_msg .= ( ( isset($error_msg) ) ? '<br />' : '' ) . $lang['Admin_user_fail'];
			}
		}
		
                 if( $error)
		{
			$template->set_filenames(array(
				'reg_header' => 'error_body.tpl')
			);

			$template->assign_vars(array(
				'ERROR_MESSAGE' => $error_msg)
			);

			$template->assign_var_from_handle('ERROR_BOX', 'reg_header');

			$username = htmlspecialchars(stripslashes($username));
			$email = stripslashes($email);
			$password = '';
			$password_confirm = '';

			$icq = stripslashes($icq);
			$aim = htmlspecialchars(str_replace('+', ' ', stripslashes($aim)));
			$msn = htmlspecialchars(stripslashes($msn));
			$yim = htmlspecialchars(stripslashes($yim));

			$website = htmlspecialchars(stripslashes($website));
			$location = htmlspecialchars(stripslashes($location));
			$occupation = htmlspecialchars(stripslashes($occupation));
			$interests = htmlspecialchars(stripslashes($interests));
			$signature = htmlspecialchars(stripslashes($signature));

			$user_lang = stripslashes($user_lang);
			$user_dateformat = htmlspecialchars(stripslashes($user_dateformat));
		}
	}

Diese Zeilen findet man in der admin_user.php zwischen Zeile 654 und 728.

Mfg
MrMind

Beitrag von **D@ve** » 23.09.2004 10:28

Ich sehe jetzt ehrlich gesagt nicht ganz das Problem. $error ist eine globale Variable die an mehreren Stellen abgefragt wird. Sobald irgendwo ein Fehler auftaucht wird die Variable auf true gesetzt, natürlich auch wenn sie vorher schon abgefragt wurde...

Um das nochmal zu verkürzen:

Code: Alles auswählen

$result = doSomething();
if ($result == "hat nich' jeklappt)
{
    $error = true;
}

if (! $error)
{
    echo "No Erros, operation continue";
    $result = doSomethingElse();
    if ($ result == "funktioniert nicht");
    {
        $error = true;
    }
}

Was soll daran falsch sein?

Gruß, Dave

MrMind · Beitrag von **MrMind** » 23.09.2004 10:45

Das Problem liegt daran, das nur, wenn $error = False ist die SQL-Anweisung ausgeführt wird.

Tritt dort ein Fehler auf wird zwar $error auf True gesetzt, aber durch die spätere Anweisung total ignoriert.

Mfg
MrMind

[Edit]
Habe grad nochmal getestet. Entferne mal eine Variable in der $sql Variable und verusche dann einen User zu editieren.

Das Resultat ist, das es auf den ersten Anschein alles geklappt hat, doch ist net so. Das einzigste was einem auffallen könnte, wäre, das keine Nachricht in der Box steht außer den Links. Aber einem unerfahrem User würde dies nicht auffallen da es wie bei dem ändern des Profils auf der Index aussieht.

Teste einfach mal bitte

Beitrag von **D@ve** » 23.09.2004 19:08

Das Problem liegt daran, das nur, wenn $error = False ist die SQL-Anweisung ausgeführt wird.

Tritt dort ein Fehler auf wird zwar $error auf True gesetzt, aber durch die spätere Anweisung total ignoriert.

Ich hab durchaus verstanden, was Du meinst

das hat aber nichts mit fehlerhafter Programmierung zu tun, sondern eher mit sauberer Programmierung. Mag ja sein, dass die Variable $error, im folgenden Quelltext nicht mehr benötigt wird. Aber wenn man sich einmal auf die Konvention geeinigt hat, bei jedem Fehler $error auf "true" zu setzen, sollte man das auch machen, wenn die Variable später nicht mehr gebraucht wird...

Angenommen, ein anderer Programmierer will das Script updaten und noch einen Query machen? Der fragt dann einfach nochmal ab, ob $error auch "false" steht und macht dann weiter. Wenn aber der ursprüngliche Programmierer $error nicht mehr auf true setzt wenn ein fehler aufkommt, weil er denkt, dass er die Variable ja eh nicht mehr braucht, geht das ganze in die Hose...

Gruß, Dave

Beitrag von **Acid** » 24.09.2004 12:50

Habe grad nochmal getestet. Entferne mal eine Variable in der $sql Variable und verusche dann einen User zu editieren.

Yo, stimmt scho. Es werden zwar Eingabefehler ausgegeben, aber nicht wenn am Query etwas nicht stimmt. Da fehlt dann noch ´n message_die.

MrMind · Beitrag von **MrMind** » 25.09.2004 23:14

Acid hat geschrieben:
Habe grad nochmal getestet. Entferne mal eine Variable in der $sql Variable und verusche dann einen User zu editieren.
Yo, stimmt scho. Es werden zwar Eingabefehler ausgegeben, aber nicht wenn am Query etwas nicht stimmt. Da fehlt dann noch ´n message_die.

Jo genau.

Entweder ein message_die() oder einfach so wie ich oben geschrieben habe, das der Fehler auf dem Formular wieder gegeben wird. Würde ja auch ausreichen.

Ist aber aufjeden Fall ein Bug.

Zwar kein großer aber eine Software sollte ja so Bugfrei sein wie möglich. Kann das jemand evtl. weiter an phpBB.com geben ??? mein Englisch ist net grad das bester

Mfg
MrMind

Beitrag von **Acid** » 25.09.2004 23:17

Ach lohnt sich nicht mehr.. die sind mit 2.2 beschäftigt.

Wenn da zuviele Bugs eingetragen werden, halten die sich bloss mit 2.0.11 auf.

(Zumal es das "Problem" imho von Anfang an gab.)

MrMind · Beitrag von **MrMind** » 25.09.2004 23:24

Acid hat geschrieben:(Zumal es das "Problem" imho von Anfang an gab.)

Dachte ich mir schon

Naja wenn man mal ehrlich ist, die Else Schleife wird eher sehr selten ausgeführt sonst wäre das bestimmt schon früher aufgefallen

Wäre aber gut, wenn PhillipK oder sonst jemand dies evtl. in ein Wichtigem Thread erwähnt. Bug bleibt Bug. Zwar keiner der Sicherheitsrelevant ist, aber User stark verärgern könnte, wenn er auffallen sollte.

Ich habe mein Script das von dieser Datei abgeleitet wurde bestimmt 1 std. auf Fehler überprüft bis ich dann meine Echo-Tricks angewandt habe um zu gucken wo der Fehler genau eintrifft.

Naja mir ist er zufällig aufgefallen, was ihr damit macht, ist euere Sache

Ich habe meine Pflicht erfüllt und einen Fehler gemeldet

Mfg
MrMind

Beitrag von **itst** » 28.09.2004 20:10

MrMind hat geschrieben:Naja mir ist er zufällig aufgefallen, was ihr damit macht, ist euere Sache Ich habe meine Pflicht erfüllt und einen Fehler gemeldet

Nein, hast Du nicht. Bugs gehören in den Bug-tracker: http://www.phpbb.com/bugs/ und nirgendwo sonst hin.