How to compare passwords in TXP?

net-carver · 2011-03-22 22:02:23

From the next released version of TXP, or from any version of TXP you now pull from SVN, yes.

Edited to add: That’s only one of the advantages of using PHPass, there are others that are more important (in my view), but removing dependencies on MySQL hashing functions is definitely a worthwhile plus.

Last edited by net-carver (2011-03-22 22:06:02)

skrishi · 2011-03-22 22:06:39

Thank you.

ruud · 2011-03-22 22:19:06

net-carver wrote:

That’s only one of the advantages of using PHPass, there are others that are more important (in my view), but removing dependencies on MySQL hashing functions is definitely a worthwhile plus.

What are the other, more important advantages?

skrishi · 2011-03-23 01:10:39

I made a plugin, I would like to know your opinion.
I open a new thread, or here in its place? And in what section to open?
And as a code to show? Print this right here, or better yet a link to the page?
Thanks in advance.

Bloke · 2011-03-23 01:33:24

skrishi wrote:

I made a plugin, I would like to know your opinion.

Cool. What three-letter prefix have you chosen for your plugin and all its functions / global variables / class names / etc? I’ll add it to the reserved list in the wiki.

I open a new thread, or here in its place? And in what section to open?

Normally you open a topic in the Plugin Author Support forum but you probably don’t have access yet. Some kind soul will grant you access (I’ve never done it and I’m just about to go to bed, sorry).

And as a code to show? Print this right here, or better yet a link to the page?

Did you make the plugin using the raw template or the plugin composer? Either way it’s usually best to host the resulting .txt file (and/or gzipped .txt file) on your own site and add an entry over at textpattern.org that contains the details and download link. You can register yourself an account I think. There’s an optional link for a Forum Thread URL which directs people here so they can discuss the plugin.

Hope that helps.

skrishi · 2011-03-23 06:00:23

I think it’s too early to talk about full-fledged plugin. He was to have only three tags that you can either insert or not insert.
Only the Russian language. And with my English, it is unlikely I’ll make a good English version.
I would like that to experienced users and plugin authors, have looked the code for vulnerabilities, security bugs.

It is may plugin

If it really looks like a plugin, I can make a theme …

ruud · 2011-03-23 11:49:05

Security:
If this is a public interface, you’re now offering a way to try username/password combinations that is only limited by the amount of page requests your website can handle (brute force attack).

Use proper mime-encoding when sending email, since you’re using characters outside 7-bit us-ascii.

You’re not imposing any limits or restrictions on the passwords entered.

You’re doSlashing ‘$name’ twice. Once is enough.

Style:
Instead of multiple “if empty” constructs which all lead to the same result”, combine them into one (makes the code easier to read):

if (!empty($pass) and !empty($cpass) and !empty($oldpass))
{
  return gvv_gTxt('password_all_back');
}

And then continue with other if constructs. You don’t need “else” here, because the “return” gets you out of the function anyway.

Replace ‘acaunt’ with ‘account’.

Make the forms more flexible instead of hardcoding them (if this is a plugin that will be released to the public).

skrishi · 2011-03-23 17:40:29

Thank you for your opinion. I’ll work on it.

ruud wrote:

You’re doSlashing ‘$name’ twice. Once is enough.

Where better to use doSlashing(): when accessing the database or reading the variables from the form?

Tell me again: how do I do? Next issue will not match the title. I open a new thread, or you can continue here?

ruud · 2011-03-23 18:55:26

I prefer to escape late instead of early, so just before accessing the database.

skrishi · 2011-03-25 22:23:02

Another version of the plugin.
I would be very grateful for opinions.
Tried to make comments that would be easier to read.
Sorry for my English.

New vesion plugin

Gocom · 2011-03-26 00:06:32

Nice to see new up-coming plugin authors :)

skrishi wrote:

I would be very grateful for opinions.

As Ruud said earlier, return ends the execution of the current function. You do not need (messy) nested if/else structure when you are already using return.

Also as Ruud said, you probably should limit the request using sleep() to prevent brute attacks.

Currently you are using simple DB time updating (just lockless inserts, row counts and updates) to prevent over-activity; just don’t freak out when there are two Johns and three Eves in the table.

mysql_query()

Textpattern offers safe_query() function.

As far the installer itself goes, You probably shouldn’t load installer on every request. You can use life-cycle callbacks to run installer, or hook it once on the backend. You may get support requests from users about notices popping up if you keep it like that. CREATE operation shouldn’t be thrown loosely.

CREATE TABLE IF NOT EXISTS gvv_user_edit

You shoud use safe_pfx() to prefix the table name. TXP has an option to use prefixed tables, and if prefix is used, the queries won’t be looking for prefix-less table.

name TEXT

txp_users table doesn’t store that long user names ;) Just varchar(64) might be better. Also the users have an ID (integer) which might be better than the name. Or you could store the times in the txp_users table.

If you choose to use your own table, then using unique/primary wouldn’t hurt. You don’t want duplicate rows there.

COLLATE utf8_latvian_ci

Not all users are from latvia. Just set the default character set, and let MySQL set the collations based on it, or by the parent defaults. Refer MySQL manual.

time int

datetime instead of int? Int stores integers up to set length (2147483647 for signed), while datetime stores any date. Not that you will reach it anytime soon :p

empty($_POST['stage_pass'])

You could use ps(). If you don’t want to use ps(), at least change the empty() to isset(). It’s not just empty, it’s not even set.

if (8 <= strlen($new_pass)) //if the password longer than 8 characters

strlen() isn’t unicode compatible. If you want to count the length in characters (not in bytes) use multibyte safe functions (mb_strlen).

mail()

TXP offers txpMail() function, which does take server configuration and encoding into account (which you didn’t).

<div id="accaunt_name"><label>Repeat E-mail:</div>

Label tags are unclosed. As Ruud said, you should allow customization, let the user to change the markup with a their form. Not to mention that your form is completely static, it doesn’t remember the values if error occurs. Would be great if it was error aware and remembered the input.

if (!preg_match("([0-9a-z]([-_.]?[0-9a-z])*[0-9a-z]([-.]?[0-9a-z])*\\.[a-wyz][a-z](fo|g|l|m|mes|o|op|pa|ro|seum|t|u|v|z)?)”, $email))@

Should generate false positives. Doesn’t comply with RFCs. You don’t need to check the email if you are matching it to existing users. If you really need to validate email, you can use filter_var() which is the best simple regex method.

mysql_query("DROP TABLE gvv_user_edit");
echo gvv_gTxt('db_del_ok');
return ($ok=2);
return ($ok=1);

The echos and returns don’t actually know if it’s ok.

$lang = array([..])

You can use TextPacks (and/or $textarray). That way the user doesn’t need to modify the source code if he wants to change the strings.

Last edited by Gocom (2011-03-26 00:14:08)

skrishi · 2011-03-26 18:53:36

Gocom wrote:

Nice to see new up-coming plugin authors :)

=)

As Ruud said earlier, return ends the execution of the current function. You do not need (messy) nested if/else structure when you are already using return.

This is a fatal error?

mysql_query("DROP TABLE gvv_user_edit");
echo gvv_gTxt('db_del_ok');
return ($ok=2);
return ($ok=1);
The echos and returns don’t actually know if it’s ok.

Something here that something not understand?

You can use TextPacks (and/or $textarray). That way the user doesn’t need to modify the source code if he wants to change the strings.

You can an example of plug?

ps: Thanks for the tip, I have almost everything straightened.

Last edited by skrishi (2011-03-26 19:15:41)

ruud · 2011-03-26 22:10:51

mysql_query("DROP TABLE gvv_user_edit");
echo gvv_gTxt('db_del_ok');

The assumption here is that the mysql_query never fails. If it does, then the ‘db_del_ok’ message is incorrect. You should check if the result of the mysql_query command to see if it was succesful and then echo whether it was.

return ($ok=2);
return ($ok=1);

Same problem here. You have such return lines just below calling the safe_update function, but you’re not checking if the safe_update function actually succeeded in updating anything.
Also, I assume you’re writing “$ok=2” to improve readability somehow, but that’s something (to me at least) that’s a bit unusual. I’m more used to seeing something like this, which does the exact same thing. I’d pick a different variable name anyway, because if $ok can be 1, 2 or other values… which of these is okay? ($status or $error or $return_code would be more meaningful, IMHO):

return 2;
return 1;

This is a fatal error?

No. Gocom and I just find it easier to read code if written in the way we suggest. Code that is simpler and can easily be read, is easier to understand, maintain and debug.

if (!preg_match("([0-9a-z]([-_.]?[0-9a-z])*[0-9a-z]([-.]?[0-9a-z])*\\.[a-wyz][a-z](fo|g|l|m|mes|o|op|pa|ro|seum|t|u|v|z)?)”, $email))

TXP has a built-in function is_valid_email which you can use.

skrishi · 2011-03-27 01:02:23

Here , I made the changes. If you are comfortable, look again.
It remains to make a form that the user would be able to change them without interfering with the code.
I’ll do this later.

Syntax: I’m learning php 3 weeks. Hard for me so far to cut. I’m starting to get confused in the code. And so easy for me to find errors and change if new ideas.

Can I make a section of language as a zem_contact_lang?
On another, I do not know how.
Although it can be an example and I’ll try to do the same.
Unfortunately Russian documentation is very little, and my English is very bad.
I take http://translate.google.ru/ that would understand you. = (

There are no errors security? I was able to remove them?

ruud

TXP has a built-in function is_valid_email which you can use.

I’ll try it later.
But not sure. For example, from ps() I had to give up, I could not get it to work.

If you have any comments, I’ll be glad to listen.
Thank you.

ruud · 2011-03-27 10:10:37

if (!preg_match("([0-9a-z]([-_.]?[0-9a-z])*[0-9a-z]([-.]?[0-9a-z])*\\.[a-wyz][a-z](fo|g|l|m|mes|o|op|pa|ro|seum|t|u|v|z)?)”, $email))

can be replaced with:

if (!is_valid_email($email))

Compare what you wrote:

function gvv_edit_pass($form_edit_pass)
{
 if (!isset($_POST['stage_pass']))
 {
  return gvv_form_edit_pass();  //if stage_pass NULL, then representing the form
 }
 else 
 { //else extract
  extract(gpsa(array('new_pass', 'cnew_pass', 'oldpass', 'name')));
if (!empty($new_pass) and !empty($cnew_pass) and !empty($oldpass)) //if field is not NULL
{
 if (8 <= mb_strlen($new_pass)) //if the password longer than 8 characters
 {
  if ($new_pass != $cnew_pass) // check whether the passwords match
  {
return gvv_gTxt('pass_er');
  }
  else
  {
$a=gvv_query_db ($name); //Look, if there were attempts to change your password or email
if ($a == 1)
{
 return gvv_gTxt('time_out');
}
else
{ //Consider, if the password is entered correctly
 if(safe_count(safe_pfx('txp_users'), "(pass=password('".doSlash($oldpass)."') or pass=password(lower('".doSlash($oldpass)."'))) and name='".doSlash($name)."'") == 1)
 {
  $rs = safe_update(safe_pfx('txp_users'), "pass = password(lower('".doSlash($new_pass)."'))", "name = '".doSlash($name)."'");
  safe_delete(safe_pfx('gvv_user_edit'), "name = '".doSlash($name)."'");
  if($rs)
  { //if so then send a message
$to_address = safe_field('email',safe_pfx('txp_users')," name = '".doSlash($name)."'");
$site_name = site_name();
$site_url = site_url();
$subject = "$site_name:".gvv_gTxt('mail_subject_new_pass');
$body = "\n".gvv_gTxt('mail_body_pass').": $new_pass \n$site_url";
txpMail($to_address, $subject, $body);
return gvv_gTxt('pass_ok');
  }
  else
  {
  return gvv_gTxt('unknow_err');
  }
 }
 else 
 {
  return gvv_gTxt('old_pass_error');
 }
}
  }
 }
 else
 {
  return gvv_gTxt('smoll_pass');
 }
}
else
{
 return gvv_gTxt('password_all_back'); 
}
 } 
}

To this, which (unless I made a mistake) does the same thing):

function gvv_edit_pass($form_edit_pass)
{
	if (!isset($_POST['stage_pass']))
	{
		return gvv_form_edit_pass();  //if stage_pass NULL, then representing the form
	}

	extract(gpsa(array('new_pass', 'cnew_pass', 'oldpass', 'name')));

	if (empty($new_pass) or empty($cnew_pass) or empty($oldpass)) //if field is not NULL
	{
		return gvv_gTxt('password_all_back'); 
	}

	if (8 <= mb_strlen($new_pass)) //if the password longer than 8 characters
	{
		return gvv_gTxt('smoll_pass');
	}

	if ($new_pass != $cnew_pass) // check whether the passwords match
	{
		return gvv_gTxt('pass_er');
	}

	// Look, if there were attempts to change your password or email
	if (gvv_query_db($name) == 1)
	{
		return gvv_gTxt('time_out');
	}

	//Consider, if the password is entered correctly
	if(safe_count(safe_pfx('txp_users'), "(pass=password('".doSlash($oldpass)."') or pass=password(lower('".doSlash($oldpass)."'))) and name='".doSlash($name)."'") != 1)
	{
		return gvv_gTxt('old_pass_error')
	}

	safe_delete(safe_pfx('gvv_user_edit'), "name = '".doSlash($name)."'");

	$rs = safe_update(safe_pfx('txp_users'), "pass = password(lower('".doSlash($new_pass)."'))", "name = '".doSlash($name)."'");

	if($rs)
	{ //if so then send a message
		$to_address = safe_field('email',safe_pfx('txp_users')," name = '".doSlash($name)."'");
		$site_name = site_name();
		$site_url = site_url();
		$subject = "$site_name:".gvv_gTxt('mail_subject_new_pass');
		$body = "\n".gvv_gTxt('mail_body_pass').": $new_pass \n$site_url";
		txpMail($to_address, $subject, $body);
		return gvv_gTxt('pass_ok');
	}
	else
	{
		return gvv_gTxt('unknow_err');
	}
}

For me, it’s easier to understand the code that way, because the cause and effect of the if-constructs are close to each other. It avoids getting confused by the many levels of indenting (I use tabs instead of spaces, btw.).

Textpattern CMS

Textpattern CMS support forum

#16 2011-03-22 22:02:23

Re: How to compare passwords in TXP?

#17 2011-03-22 22:06:39

Re: How to compare passwords in TXP?

#18 2011-03-22 22:19:06

Re: How to compare passwords in TXP?

#19 2011-03-23 01:10:39

Re: How to compare passwords in TXP?

#20 2011-03-23 01:33:24

Re: How to compare passwords in TXP?

#21 2011-03-23 06:00:23

Re: How to compare passwords in TXP?

#22 2011-03-23 11:49:05

Re: How to compare passwords in TXP?

#23 2011-03-23 17:40:29

Re: How to compare passwords in TXP?

#24 2011-03-23 18:55:26

Re: How to compare passwords in TXP?

#25 2011-03-25 22:23:02

Re: How to compare passwords in TXP?

#26 2011-03-26 00:06:32

Re: How to compare passwords in TXP?

#27 2011-03-26 18:53:36

Re: How to compare passwords in TXP?

#28 2011-03-26 22:10:51

Re: How to compare passwords in TXP?

#29 2011-03-27 01:02:23

Re: How to compare passwords in TXP?

#30 2011-03-27 10:10:37

Re: How to compare passwords in TXP?

Board footer