jfleck25;11032731 wrote:
if ($_POST['vibe']) {
Why do you only allow login based on vibe being posted? Not that I know what vibe is supposed to be, but usually login checks username and password.
jfleck25;11032731 wrote:
$vibe = stripslashes($_POST['vibe']);
Why stripslashes? Are you using magic_quotes_gpc? If so, upgrade php to a current version. If you can't, at least turn off magic_quotes_gpc and remove this.
jfleck25;11032731 wrote:
$vibe = strip_tags($vibe);
Why strip tags? You take input, and if it's not what you allow, you silently transform it? Most likely not a good idea.
jfleck25;11032731 wrote:
$vibe = mysql_real_escape_string($vibe);
This is the only thing you need to make your input safe for use in database queries. Since you are preparing data to be inserted, this is the way to escape data - when you insert it. When you display it as part of an html page, you should however use htmlspecialchars or htmlentities, since that escapes characters to be used as parts of html documents.
jfleck25;11032731 wrote:
$password = preg_replace("[^A-Za-z0-9]", "", $_POST['password']); // filter everything but numbers and letters
$password = md5($password);
Once again you take input and transform it. In this case it's certainly a very bad idea. If I believe I have chosen "!go" as my password, you will be silently transforming it into "go" every time. If you ever decide to change what characters you allow in a password and include !, then my input will suddenly no longer be transformed from "go!" to "go" and no longer matched what's stored. Do not do this.
Validation is done by validating input, not by transforming it. Use preg_match() to perform either one of these checks
1. There are only allowed characters. I.e. the pattern only_allowed$ (^ as used here marks beginning of input, $ marks end)
2. There is at least one character which is not allowed. I.e. the pattern [abcd] (as used here negates the character class, so that any other character than a, b, c or d will match.
But do note that these are the actual patterns, without pattern delimiters. And as input to preg, you must use some kind of delimiter. Please refer to the php manual to see what you can choose. For readability, it's a good idea to choose a character which isn't part of the pattern.
However, your pattern is doing something entirely different than what you believe it is doing, and it is related to pattern delimiters. Try using this as password input and inspect the output.
A-Za-z0-9hello world!
jfleck25;11032731 wrote:
// Make query and then register all database data that -
// cannot be changed by member into SESSION variables.
// Data that you want member to be able to change -
// should never be set into a SESSION variable.
This is not an absolute truth. You may keep data in session storage. But if you ever update it in the database, then you also have to invalidate the session data. Unless it's ok to use stale cache data until it expires naturally. This is always an issue when using cached data, and you choose to deal with it depends on your needs. The above works fine, until you need to cache it for performance reasons.
As for the first part, "that which cannot be changed by user", would be user_id, and possibly username. But since you will have to retrieve everything else from the database anyway if you do not somehow cache it, you might as well stick with only id in session and retrieve username from the database too. Thus you don't really have to do this distinction.
jfleck25;11032731 wrote:
$sql = mysql_query("SELECT * FROM members WHERE vibe='$vibe' AND password='$password' AND emailactivated='1'");
$login_check = mysql_num_rows($sql);
if($login_check > 0){
And here's the particular reason for your post. Why it would work for a second login is beyond me. Unless you never specify "vibe" when you login a second time. Or if you just coincidentally always inputs the same vibe the second time as what's stored in your database.
Apart from comparing other things than what you should be (login check is done against name and password as previously stated) - unless you're supposed to remember what your vibe was whenever it was set.
jfleck25;11032731 wrote:
$vibe = $row["vibe"];
$_SESSION['vibe'];
$_SESSION['vibe'] = $vibe;
Try to avoid useless clutter in your code like this. $SESSION['vibe']; does nothing at all. And if you never user $vibe anywhere else, why bother with it here?
$_SESSION['vibe'] = $row["vibe"];
This line tells me that you most likely have some serious problems here. According to your code comment earlier
Data that you want member to be able to change -
// should never be set into a SESSION variable.
So if your comment is correct, user should not be able to change vibe. Please note that there is a difference between persistent storage and temporary storage, but that both may be changed. You may store something in session, then change it and overwrite the previous value - thus allowing change to nonpersistent data. I'm neither saying that you do, nor that you don't. And it may be that you're aware of the distinctions but that your language isn't precise enough, but if not, you need to realize these differences.
However, you also
jfleck25;11032731 wrote:
// Update last_log_date field for this member now
mysql_query("UPDATE members SET lastlogin=now() WHERE id='$id'");
where you do not set vibe. So in your first query (which you call login check), you compare user input vs database stored value for vibe. But you never update it. Also note that even keeping vibe in the database is entirely pointless if the user isn't allowed to (permanently) store (note the difference between store and change) it, while being allowed to change it.
jfleck25;11032731 wrote:
// … if all went well then exit the script
This comment implies that a conditional will follow, e.g.
if ($all_went_well)
while you always execute the lines following that comment. With the exception of an error which aborts execution, but then your code doesn't work at all and the comment is pointless to begin with. Never imply that code does things it doens't using comments. You may trick yourself or someone else, thus making debugging harder.
jfleck25;11032731 wrote:
// Print success message here
header("location: http://www.sourcevibe.com");
exit();
And the same admonition again: you are not printing a success message, you are using a location redirect header to tell the browser to retrieve some other page. And does it actually need a comment? Is anyone reading the code likely to not understand this?
HTML 5 browsers are likely to ignore your dtd. Well, IE 9 and 10 might put themselves into some older browser mode for all I know. But if you want them to do that, you might as well do it with propietary headers telling them exactly which IE version to parse your documents as.
It should be
<!DOCTYPE html>
for HTML documents. And the html 5 spec clearly states that it supersedes older versions. Thus, anything else is superflous and ignored.
As for XHTML, I personally no longer see the point of using it for a few different reasons - unless there actually is a point to using them (see 4. below)
1. Now that html document no longer uses a dtd to specify the document rules, but have actual parsing rules which enforces the same behaviour across conforming browsers, there is no longer the same need for the additional aid you got from the stricter xhtml rules. And you may still explicitly close p tags etc.
2. If you ever host ads on your pages that are created by others, and these ads are sometimes delievered as javascript code, there is a good chance they will be using document.write, which isn't allowed in xml documents.
But if you do, you will have to
1. serve your document as an xml document - these always begin with an xml document declaration: <?xml ...>
2. serve your document as the html doctype: <!DOCTYPE html>
3. default xhtml namespace - same as in the old xhtml 1.0 documents.
4. secondary namespace - if you don't need one, there is no real point to having an xml document in the first place.
jfleck25;11032731 wrote:
<html>
needs a default namespace
jfleck25;11032731 wrote:
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
Use utf-8.
As for the rest of the html code
1. validate it!
2. Do not use inline styles.
3. Do not use tables for layout. It's harder to read and maintain even if it's quicker to learn than css layouts. And once again validate your html code.
3. The align attribute is deprecated. Use css text-align attribute for this.
