help needed

[dayo]

hiya me again i have wrothe this code can you tell me whet is wrong and where it can be improved, the aim of this it to update the users pin number and the other aim is to change the users username and password

Update Pin

<?php 
//Require Database Connection
require_once('../config.php');
	//old pin numbers
	$oldpin1 = $_POST['oldpin1'];
	$oldpin2 = $_POST['oldpin2'];
	$oldpin3 = $_POST['oldpin3'];
	$oldpin4 = $_POST['oldpin4'];
	//newpin numbers
	$newpin1 = $_POST['newpin1'];
	$newpin2 = $_POST['newpin2'];
	$newpin3 = $_POST['newpin3'];
	$newpin4 = $_POST['newpin4'];

$query = "UPDATE Mysite SET pin1=$newpin1, pin2=$newpin2, pin3=$newpin3, pin1=$newpin4 WHERE pin1=$oldpin1, pin2=$oldpin2, pin3=$oldpin3, pin1=$oldpin4";
header("location: update-success.php");
else
header("location: profile.php");
}
?>

Update Username and Password

<?php 
//Require Database Connection
require_once('../config.php');

//New Login Details
$newuser = $_POST['newusername'];
$newpass = $_POST['newpassword'];
$confirmpass = $_POST['confirmpassword'];

//Old login Details
$username = $_POST['oldusername'];
$password = $_POST['oldpassword'];

if  ($newpass == $Confirmpass) {	 
$query = "UPDATE members SET login=$newuser, passwd=$newpass WHERE login=$username, passwd=$password";
header("location: update-success.php");
else
header("location: profile.php");
}
?>

this is what i have done at school today in 1st lesson so sorry if it is reli bad

[coldwerturkey]

firstly, your query's aren't executing, you've just set up the variables, you need to use mysql_query() or similar function depending on what database you're using

adding an or die(mysql_error()) statement to the query for troubleshooting is helpful

your "else" statement in user/pass is missing brackets (should look similar to: } else {)

your else statement in update pin is running off of no if or similar statement... and you have a single } close bracket doing nothing there

you should escape data inserted into database with mysql_real_escape_string()

you should be verifying your data, making sure fields aren't empty, proper characters are/nt in place etc.

you should use some form of hash+salt to hash the passwords to kept them private and safe

your update statements should use a unique id as the WHERE clause, instead of user/pass match and oldpin's as you have no check for duplicate entries, etc.

your query column=$value should be more like column='".$value."' or column='{$value}'

lastly, please make better titles for your threads.

..you may want to look at small tutorials on the web that go over each step to do small php/database scripts like http://www.w3schools.com/PHP/

(note: I'm amateur, please anyone correct me if I pointed out something wrong)

[dayo]

thanx for the help
this look better?

<?php 
//Require Database Connection
require_once('../config.php');

//New Login Details
$newuser = $_POST['newusername'];
$newpass = $_POST['newpassword'];
$confirmpass = $_POST['confirmpassword'];

//Old login Details
$username = $_POST['oldusername'];
$password = $_POST['oldpassword'];

//User verification
if($newuser == '') {
	$errmsg_arr[] = 'please enter your current username';
	$errflag = true;
}
if($password == '') {
	$errmsg_arr[] = 'Please enter your curtrent password';
	$errflag = true;
}
if($newuser == '') {
	$errmsg_arr[] = 'Please enter a new username';
	$errflag = true;
}
if($newpass == '') {
	$errmsg_arr[] = 'Please enter a new password';
	$errflag = true;
}
if($confirmpass == '') {
	$errmsg_arr[] = 'Please confirm your password';
	$errflag = true;
}
if($errflag) {
	$_SESSION['ERRMSG_ARR'] = $errmsg_arr;
	session_write_close();
	header("location: profile.php");
	exit();
}

//check if the passwords match
if  ($newpass == $Confirmpass) {	 
$query = "UPDATE members SET `login`='".$newuser."', `passwd`='".$newpass."' WHERE `login`='".$username."', `passwd`='".$password."'";
or die(mysql_error()) 
header("location: update-success.php");
} 
else {
header("location: profile.php");
}
?>

[bradgrafelman]

Close; $Confirmpass is never defined (hint: check the case).

Also, your code is vulnerable to SQL injection attacks. User-supplied data should never be placed directly into a SQL query. Instead, you should first sanitize it with a function such as [man]mysql_real_escape_string/man.

[dayo]

Close; $Confirmpass is never defined (hint: check the case).

Done Thanx

Also, your code is vulnerable to SQL injection attacks. User-supplied data should never be placed directly into a SQL query. Instead, you should first sanitize it with a function such as mysql_real_escape_string().

where would i put that on the script, im a real newb when it comes to php

[coldwerturkey]

$value = mysql_real_escape_string($_POST['value']);

additionally, you're still not executing the query you need to place $query within the mysql_query() function (assuming you're using MySQL), then the or die() statement follows the query ie;

mysql_query($query) or die (mysql_error());

I (and most members) are really against spoon feeding, so please take a look on google, php.net/manual, or small teaching tutorials like http://www.w3schools.com/PHP/

[bradgrafelman]

Also note that such 'or die()' statements are useful in development/testing environments only! You should never have code on your production server (i.e. what you put on the web for the world to see) such as 'or die(mysql_error())'.