schlampig gecodet?

tomstig

OC Addicted

Registered: Nov 2003
Location: /home/tomstig/
Posts: 1341

22.06.2005 - 18:53

Da ich für eine Party eine Gästeliste brauchte, die von mehreren editiert werden kann, bastelte ich eine mit PHP (+MySQL).

Der Code enstand in ca. 30-45min.

Ist der folgende Code gut bzw. in Ordnung oder ist der einfach unter aller Sau und nur schnell hingfetzt?

Code: PHP

<html>
<head>
<title>Gästeliste</title>
<style type="text/css">
body, th, td, a{
	color: #000;
	font-family: Verdana;
	font-size: 10pt;
	padding: 5px;
}

td{
	border: 1px solid #000;
}

a:link, a:visited{
	text-decoration: underline;
}

a:hover{
	text-decoration: none;
}

input.none{
	border: none;
}
</style>
</head>
<body>
<?php

error_reporting(E_ALL);

$db = mysql_connect("localhost", "root", "");
mysql_select_db("test", $db);

if( isset($_GET['submit']) )
	$sql = "INSERT INTO guestlist SET name='" . $_GET['name'] . "', fix=" . ( $_GET['fix'] == 1 ? 1 : 0 ) . ", time_added='" . time() . "'";	
elseif( isset($_GET['delete']) )
	$sql = "DELETE FROM guestlist WHERE guest_id=" . $_GET['delete'];	
elseif( isset($_GET['changed_name']) )
	$sql = "UPDATE guestlist SET name='" . $_GET['changed_name'] . "' WHERE guest_id=" . $_GET['guest_id'];
elseif( isset($_GET['fix']) ){
	$sql = "SElECT fix FROM guestlist WHERE guest_id=" . $_GET['fix'];
	$query = mysql_query($sql);
	$fix = mysql_result($query, 0);
	
	$sql = "UPDATE guestlist SET fix=" . ($fix == 0 ? 1 : 0) . " WHERE guest_id=" . $_GET['fix'];
}
	
if( !empty($sql) ){
	mysql_query($sql);
	echo '<script type="text/javascript">window.location.href=\'guestlist.php\';</script>';
}
if( isset($_GET['order']) )
	echo '<a href="' . $_SERVER['PHP_SELF'] . '">Wieder umordnen...</a><br /><br />';
	
echo '<form name="form2" action="' . $_SERVER['PHP_SELF'] . '" method="GET"><input type="text" name="name" value"G&aumlstename"><input type="checkbox" name="fix" value="1" title="Kommt dieser Gast sicher?"><input type="submit" name="submit" value="Gast eintragen"></form>';
echo '<script type="text/javascript">document.getElementsByName(\'name\')[0].focus();</script>';

$sql = "SELECT * FROM guestlist ORDER BY " . (isset($_GET['order']) ? $_GET['order'] : 'time_added' ) . " ASC";
$query = mysql_query($sql);

if( mysql_num_rows($query) != 0){
	echo '<table>';
	echo '<tr><th>#</th><th><a href="' . $_SERVER['PHP_SELF'] . '?order=name">Name</a></th><th><a href="' . $_SERVER['PHP_SELF'] . '?order=fix">Kommt fix?</a></th><th>Optionen</th><th>Eingetragen am</th></tr>';
	$counter = 0;
	while($data = mysql_fetch_array($query)){
		echo '<form name="formular" action="' . $_SERVER['PHP_SELF'] . '" method="GET">
<tr>
<td>' . ($counter+1) . '</td>
<td><input type="hidden" name="guest_id" value="' . $data['guest_id'] . '"><input type="text" name="changed_name" value="' . $data['name'] . '" class="none"></td><td>' . ( $data['fix'] == 1 ? 'Ja' : 'Nein' ) . '</td>
<td><a href="' . $_SERVER['PHP_SELF'] . '?fix=' . $data['guest_id'] . '">[ ' . ($data['fix']==1 ? 'de' : '') . 'fixieren ]</a>
 <a href="' . $_SERVER['PHP_SELF'] . '?delete=' . $data['guest_id'] . '">[ löschen ]</a>
 <a href="#" onclick="document.getElementsByName(\'formular\')[' . $counter . '].submit()">[ Name ändern ]</a></td>
<td>' . date('d.m.Y, H:i:s', $data['time_added']) . '</td>
</tr></form>';
		$counter++;
	}
	echo '</table>';
}
?>
</body>
</html>


mysql tabelle:
CREATE TABLE `guestlist` (
  `guest_id` int(11) NOT NULL auto_increment,
  `name` varchar(255) NOT NULL default '',
  `fix` int(11) NOT NULL default '0',
  `time_added` varchar(255) NOT NULL default '',
  PRIMARY KEY  (`guest_id`)
) ENGINE=MyISAM AUTO_INCREMENT=26 ;

Bearbeitet von tomstig am 22.06.2005, 19:51

funka

Legend
ex-prophet(down below)

Registered: Sep 2000
Location: Vienna / SF
Posts: 6131

22.06.2005 - 19:33

if its stupid but works its not stupid

it does the job so let it do the job

never change a running system

willst noch mehr kluge sprueche?

moidaschl

Vollzeit-Hackler

Registered: Aug 2002
Location: 1210, ABK-D/L
Posts: 4029

22.06.2005 - 19:36

Zitat von funka
if its stupid but works its not stupid

it does the job so let it do the job

never change a running system

willst noch mehr kluge sprueche?

i kenns nur unter "never touch a running system"

das einzige vor dem es mich ein bissl graust is deine 3km lange echo anweisung, die könntest irgendwo abschneiden und darunter schön einrichten.

also zb

Code:

echo "irgendwas das eindeutig zu
      lang ist für eine zeile";

wenn ein tag zu lang is würd ich ihn nach einem argument kürzen, also so

Code:

echo '<input name="hans" value="hans123"
             size="20" type="text">\n';

Bearbeitet von moidaschl am 22.06.2005, 19:40

tomstig

OC Addicted

Registered: Nov 2003
Location: /home/tomstig/
Posts: 1341

22.06.2005 - 19:55

Zitat von funka
if its stupid but works its not stupid

it does the job so let it do the job

never change a running system

willst noch mehr kluge sprueche?

"If a code was hard to write it should also be hard to unterstand" - mein fav

naja trotzdem - verbesserungsvorschläge kann man sich doch immer holen

@moidaschl: in meinem editor war das lange echo eh nur in einer zeile

aber ich wollte hier keinen horizontalen scrollbalken erzwingen

watchout

Legend
undead

Registered: Nov 2000
Location: Off the grid.
Posts: 6845

22.06.2005 - 19:58

funka hat zwar recht - ich würd jedoch aufpassen weil das Script extrem unsicher ist.

funka

Legend
ex-prophet(down below)

Registered: Sep 2000
Location: Vienna / SF
Posts: 6131

22.06.2005 - 20:00

watch hat recht

wegen code wie solchem gibs "features" wie magic_quotes u.ä.

kleinerChemiker

Here to stay

Registered: Feb 2002
Location: Wien
Posts: 4282

22.06.2005 - 21:31

oder mysql_escape_string()

tomstig

OC Addicted

Registered: Nov 2003
Location: /home/tomstig/
Posts: 1341

22.06.2005 - 21:34

Zitat von watchout
funka hat zwar recht - ich würd jedoch aufpassen weil das Script extrem unsicher ist.

hmm... auf sicherheit hab ich nicht viel wert gelegt, aber kannst du mir vielleicht sagen, was so unsicher ist?

watchout

Legend
undead

Registered: Nov 2000
Location: Off the grid.
Posts: 6845

22.06.2005 - 21:37

du übernimmst (mehrmals) direkt werte die vom user kommen unquoted in einen query. mysql injection wird möglich.

Obermotz

Fünfzylindernazi

Registered: Nov 2002
Location: OÖ/RI
Posts: 5262

22.06.2005 - 21:50

Ich würds auch noch besser absichern, alle haben es drauf abgesehen, deine Party-Gästeliste zu modifizieren!

scnr

watchout

Legend
undead

Registered: Nov 2000
Location: Off the grid.
Posts: 6845

22.06.2005 - 22:00

Wie du schon andeutest ist eine Party-Gästeliste eher weniger wichtig - deswegen wird sie auch wohl kaum auf einem eigenen Server liegen - Was denkst du wie gross die Freude is wenn du wegen einer kleinen besch. Gästeliste einen kompletten Server geschrottet hast, wo vielleicht auch noch wichtige Firmendaten oben sind...

@Oper8or: Verwarnung - scnr

tomstig

OC Addicted

Registered: Nov 2003
Location: /home/tomstig/
Posts: 1341

22.06.2005 - 22:39

Zitat von watchout
du übernimmst (mehrmals) direkt werte die vom user kommen unquoted in einen query. mysql injection wird möglich.

was ist unquoted :confused:

wie behandelt man sonst die werte, wenn man sie mit $_GET ausliest?

kleinerChemiker

Here to stay

Registered: Feb 2002
Location: Wien
Posts: 4282

22.06.2005 - 22:41

mysql_escape_string($_GET[''])

watchout

Legend
undead

Registered: Nov 2000
Location: Off the grid.
Posts: 6845

22.06.2005 - 22:42

zumindest mysql_escape_string($_GET[...])

edit:

Ringding

Pilot

Registered: Jan 2002
Location: Perchtoldsdorf/W..
Posts: 4300

22.06.2005 - 22:42

Google for sql injection.

tomstig OC Addicted Registered: Nov 2003 Location: /home/tomstig/ Posts: 1341	22.06.2005 - 18:53 Da ich für eine Party eine Gästeliste brauchte, die von mehreren editiert werden kann, bastelte ich eine mit PHP (+MySQL). Der Code enstand in ca. 30-45min. Ist der folgende Code gut bzw. in Ordnung oder ist der einfach unter aller Sau und nur schnell hingfetzt? Code: PHP <html> <head> <title>Gästeliste</title> <style type="text/css"> body, th, td, a{ color: #000; font-family: Verdana; font-size: 10pt; padding: 5px; } td{ border: 1px solid #000; } a:link, a:visited{ text-decoration: underline; } a:hover{ text-decoration: none; } input.none{ border: none; } </style> </head> <body> <?php error_reporting(E_ALL); $db = mysql_connect("localhost", "root", ""); mysql_select_db("test", $db); if( isset($_GET['submit']) ) $sql = "INSERT INTO guestlist SET name='" . $_GET['name'] . "', fix=" . ( $_GET['fix'] == 1 ? 1 : 0 ) . ", time_added='" . time() . "'"; elseif( isset($_GET['delete']) ) $sql = "DELETE FROM guestlist WHERE guest_id=" . $_GET['delete']; elseif( isset($_GET['changed_name']) ) $sql = "UPDATE guestlist SET name='" . $_GET['changed_name'] . "' WHERE guest_id=" . $_GET['guest_id']; elseif( isset($_GET['fix']) ){ $sql = "SElECT fix FROM guestlist WHERE guest_id=" . $_GET['fix']; $query = mysql_query($sql); $fix = mysql_result($query, 0); $sql = "UPDATE guestlist SET fix=" . ($fix == 0 ? 1 : 0) . " WHERE guest_id=" . $_GET['fix']; } if( !empty($sql) ){ mysql_query($sql); echo '<script type="text/javascript">window.location.href=\'guestlist.php\';</script>'; } if( isset($_GET['order']) ) echo '<a href="' . $_SERVER['PHP_SELF'] . '">Wieder umordnen...</a><br /><br />'; echo '<form name="form2" action="' . $_SERVER['PHP_SELF'] . '" method="GET"><input type="text" name="name" value"G&aumlstename"><input type="checkbox" name="fix" value="1" title="Kommt dieser Gast sicher?"><input type="submit" name="submit" value="Gast eintragen"></form>'; echo '<script type="text/javascript">document.getElementsByName(\'name\')[0].focus();</script>'; $sql = "SELECT * FROM guestlist ORDER BY " . (isset($_GET['order']) ? $_GET['order'] : 'time_added' ) . " ASC"; $query = mysql_query($sql); if( mysql_num_rows($query) != 0){ echo '<table>'; echo '<tr><th>#</th><th><a href="' . $_SERVER['PHP_SELF'] . '?order=name">Name</a></th><th><a href="' . $_SERVER['PHP_SELF'] . '?order=fix">Kommt fix?</a></th><th>Optionen</th><th>Eingetragen am</th></tr>'; $counter = 0; while($data = mysql_fetch_array($query)){ echo '<form name="formular" action="' . $_SERVER['PHP_SELF'] . '" method="GET"> <tr> <td>' . ($counter+1) . '</td> <td><input type="hidden" name="guest_id" value="' . $data['guest_id'] . '"><input type="text" name="changed_name" value="' . $data['name'] . '" class="none"></td><td>' . ( $data['fix'] == 1 ? 'Ja' : 'Nein' ) . '</td> <td><a href="' . $_SERVER['PHP_SELF'] . '?fix=' . $data['guest_id'] . '">[ ' . ($data['fix']==1 ? 'de' : '') . 'fixieren ]</a> <a href="' . $_SERVER['PHP_SELF'] . '?delete=' . $data['guest_id'] . '">[ löschen ]</a> <a href="#" onclick="document.getElementsByName(\'formular\')[' . $counter . '].submit()">[ Name ändern ]</a></td> <td>' . date('d.m.Y, H:i:s', $data['time_added']) . '</td> </tr></form>'; $counter++; } echo '</table>'; } ?> </body> </html> mysql tabelle: CREATE TABLE `guestlist` ( `guest_id` int(11) NOT NULL auto_increment, `name` varchar(255) NOT NULL default '', `fix` int(11) NOT NULL default '0', `time_added` varchar(255) NOT NULL default '', PRIMARY KEY (`guest_id`) ) ENGINE=MyISAM AUTO_INCREMENT=26 ; Bearbeitet von tomstig am 22.06.2005, 19:51
funka Legend ex-prophet(down below) Registered: Sep 2000 Location: Vienna / SF Posts: 6131	22.06.2005 - 19:33 if its stupid but works its not stupid it does the job so let it do the job never change a running system willst noch mehr kluge sprueche?
moidaschl Vollzeit-Hackler Registered: Aug 2002 Location: 1210, ABK-D/L Posts: 4029	22.06.2005 - 19:36 Zitat von funka if its stupid but works its not stupid it does the job so let it do the job never change a running system willst noch mehr kluge sprueche? i kenns nur unter "never touch a running system" das einzige vor dem es mich ein bissl graust is deine 3km lange echo anweisung, die könntest irgendwo abschneiden und darunter schön einrichten. also zb Code: `echo "irgendwas das eindeutig zu lang ist für eine zeile";` wenn ein tag zu lang is würd ich ihn nach einem argument kürzen, also so Code: `echo '<input name="hans" value="hans123" size="20" type="text">\n';` Bearbeitet von moidaschl am 22.06.2005, 19:40
tomstig OC Addicted Registered: Nov 2003 Location: /home/tomstig/ Posts: 1341	22.06.2005 - 19:55 Zitat von funka if its stupid but works its not stupid it does the job so let it do the job never change a running system willst noch mehr kluge sprueche? "If a code was hard to write it should also be hard to unterstand" - mein fav naja trotzdem - verbesserungsvorschläge kann man sich doch immer holen @moidaschl: in meinem editor war das lange echo eh nur in einer zeile aber ich wollte hier keinen horizontalen scrollbalken erzwingen
watchout Legend undead Registered: Nov 2000 Location: Off the grid. Posts: 6845	22.06.2005 - 19:58 funka hat zwar recht - ich würd jedoch aufpassen weil das Script extrem unsicher ist.
funka Legend ex-prophet(down below) Registered: Sep 2000 Location: Vienna / SF Posts: 6131	22.06.2005 - 20:00 watch hat recht wegen code wie solchem gibs "features" wie magic_quotes u.ä.
kleinerChemiker Here to stay Registered: Feb 2002 Location: Wien Posts: 4282	22.06.2005 - 21:31 oder mysql_escape_string()
tomstig OC Addicted Registered: Nov 2003 Location: /home/tomstig/ Posts: 1341	22.06.2005 - 21:34 Zitat von watchout funka hat zwar recht - ich würd jedoch aufpassen weil das Script extrem unsicher ist. hmm... auf sicherheit hab ich nicht viel wert gelegt, aber kannst du mir vielleicht sagen, was so unsicher ist?
watchout Legend undead Registered: Nov 2000 Location: Off the grid. Posts: 6845	22.06.2005 - 21:37 du übernimmst (mehrmals) direkt werte die vom user kommen unquoted in einen query. mysql injection wird möglich.
Obermotz Fünfzylindernazi Registered: Nov 2002 Location: OÖ/RI Posts: 5262	22.06.2005 - 21:50 Ich würds auch noch besser absichern, alle haben es drauf abgesehen, deine Party-Gästeliste zu modifizieren! scnr
watchout Legend undead Registered: Nov 2000 Location: Off the grid. Posts: 6845	22.06.2005 - 22:00 Wie du schon andeutest ist eine Party-Gästeliste eher weniger wichtig - deswegen wird sie auch wohl kaum auf einem eigenen Server liegen - Was denkst du wie gross die Freude is wenn du wegen einer kleinen besch. Gästeliste einen kompletten Server geschrottet hast, wo vielleicht auch noch wichtige Firmendaten oben sind... @Oper8or: Verwarnung - scnr
tomstig OC Addicted Registered: Nov 2003 Location: /home/tomstig/ Posts: 1341	22.06.2005 - 22:39 Zitat von watchout du übernimmst (mehrmals) direkt werte die vom user kommen unquoted in einen query. mysql injection wird möglich. was ist unquoted wie behandelt man sonst die werte, wenn man sie mit $_GET ausliest?
kleinerChemiker Here to stay Registered: Feb 2002 Location: Wien Posts: 4282	22.06.2005 - 22:41 mysql_escape_string($_GET[''])
watchout Legend undead Registered: Nov 2000 Location: Off the grid. Posts: 6845	22.06.2005 - 22:42 zumindest mysql_escape_string($_GET[...]) edit:
Ringding Pilot Registered: Jan 2002 Location: Perchtoldsdorf/W.. Posts: 4300	22.06.2005 - 22:42 Google for sql injection.

schlampig gecodet?

Forum Index > Software > Coding Stuff

tomstig

funka

moidaschl

tomstig

watchout

funka

kleinerChemiker

tomstig

watchout

Obermotz

watchout

tomstig

kleinerChemiker

watchout

Ringding