Hi Till,

Thanks for the response. I'd like to just quote everything and stick it in the database, but ticket 1463946: http://trac.roundcube.net/ticket/1463946

suggests that there is a set of characters that are undesirable to store and may cause difficulty sending mail to users with strange names.

Which puts us in the position of picking and choosing what should go into the database. And then without proper feedback to the user, they have to play a guessing game about what they can and cannot use. So how about something like:

1.) A server side match against a regex like: /^[a-zA-Z _-]*$^/ (I'll bet there's lots more characters people will want in there) 2.) On failure a message below the input box explaining that only such and such characters are allowed. (I'm not sure the transient nature of the existing error message display method is suitable for this task).

What would be icing on top of that cake would be a client side (js) check which would change the color of the input box to a red outline if it has bad characters (or something like that).

Thoughts?

Thanks, Ziba

Webmaster Team University of Michigan

till wrote:

On Wed, Oct 15, 2008 at 5:18 PM, Ziba Scott ziba@umich.edu wrote:

...

When editing contacts, some invalid characters are not stripped or handled in some way. They make it all the way to the sql statement before things trip up. (Using a prepare statement thankfully prevents injecting a second statement. More details in: http://trac.roundcube.net/ticket/1485504)

I can work on a patch, but I'd appreciate some guidance first:

Should the backend explicitly validate the input against a regular expression? What is valid/invalid? How should the interface report bad characters and/or failed contact saves to the user?

Thanks, Ziba

Webmaster Team University of Michigan

I replied, let me know if this helps. :)

Thanks for all input!

Till

List info: http://lists.roundcube.net/dev/

On Wed, Oct 15, 2008 at 6:08 PM, Ziba Scott ziba@umich.edu wrote:

Hi Till,

Thanks for the response. I'd like to just quote everything and stick it in the database, but ticket 1463946: http://trac.roundcube.net/ticket/1463946

Well, with no offense to Rich, but that ticket should not have been closed like that. As far as I know ' is allowed in the local part of an address. Even though if it's not a good idea - I mean, we could argue for hours what is and what is not a good idea. It should not be on us to decide for the user/administrator.

suggests that there is a set of characters that are undesirable to store and may cause difficulty sending mail to users with strange names.

Sorry, I think I mis-understood you earlier. We are talking about email addresses, I see that now. Not just the rest of the "profile" data, e.g. name etc..

Data should be properly quoted - no matter what.

Which puts us in the position of picking and choosing what should go into the database. And then without proper feedback to the user, they have to play a guessing game about what they can and cannot use. So how about something like:

1.) A server side match against a regex like: /^[a-zA-Z _-]*$^/ (I'll bet there's lots more characters people will want in there)

Yes, lots.

till+roundcube@whatever.com is valid too.

Think about the characters that some list servers allow.

As Charles said, the RFC is good point to start: http://en.wikipedia.org/wiki/E-mail_address#RFC_specification

Maybe we can take some BSD-licensed code from the Zend Framework: http://framework.zend.com/svn/framework/standard/trunk/library/Zend/Validate...

As you can see, email validation is sort of complex. ;-)

2.) On failure a message below the input box explaining that only such and such characters are allowed. (I'm not sure the transient nature of the existing error message display method is suitable for this task).

IMHO, a generic "invalid email address" is plenty. We don't need to confuse people with too much information.

What would be icing on top of that cake would be a client side (js) check which would change the color of the input box to a red outline if it has bad characters (or something like that).

You can put that in your template, use Jquery and attach a function to the appropriate events on the box.

Cheers, Till

Thoughts?

Thanks, Ziba

Webmaster Team University of Michigan

till wrote:

...

On Wed, Oct 15, 2008 at 5:18 PM, Ziba Scott ziba@umich.edu wrote:

...

When editing contacts, some invalid characters are not stripped or handled in some way. They make it all the way to the sql statement before things trip up. (Using a prepare statement thankfully prevents injecting a second statement. More details in: http://trac.roundcube.net/ticket/1485504)

I can work on a patch, but I'd appreciate some guidance first:

Should the backend explicitly validate the input against a regular expression? What is valid/invalid? How should the interface report bad characters and/or failed contact saves to the user?

Thanks, Ziba

Webmaster Team University of Michigan

I replied, let me know if this helps. :)

Thanks for all input!

Till

List info: http://lists.roundcube.net/dev/

Here is a perl-style regex that can be used to match all RFC-compliant email addresses (RFC 5322 and RFC 5321):

/^(([a-z]|\d|[!#$%*/?|^{}`~&'+=-_])+((.)?([a-z]|\d|[!#$%*/?|^{}`~&'+=-_]))*@(([a-z]|\d)+(-*([a-z]|\d))*.)+[a-z]+)$/

(it should be on a single line with no whitespace in case your mail client wraps it)

This cannot verify valid domain extensions (e.g. .museum (good) and .foo (bad) will both match this regex), but you really need a separate check with a lookup table if you want to be that extreme.

-gnul

On Wed, Oct 15, 2008 at 4:50 PM, till till@php.net wrote:

On Wed, Oct 15, 2008 at 6:08 PM, Ziba Scott ziba@umich.edu wrote:

...

Hi Till,

Thanks for the response. I'd like to just quote everything and stick it in the database, but ticket 1463946: http://trac.roundcube.net/ticket/1463946

Well, with no offense to Rich, but that ticket should not have been closed like that. As far as I know ' is allowed in the local part of an address. Even though if it's not a good idea - I mean, we could argue for hours what is and what is not a good idea. It should not be on us to decide for the user/administrator.

...

suggests that there is a set of characters that are undesirable to store and may cause difficulty sending mail to users with strange names.

Sorry, I think I mis-understood you earlier. We are talking about email addresses, I see that now. Not just the rest of the "profile" data, e.g. name etc..

Data should be properly quoted - no matter what.

...

Which puts us in the position of picking and choosing what should go into the database. And then without proper feedback to the user, they have to play a guessing game about what they can and cannot use. So how about something like:

1.) A server side match against a regex like: /^[a-zA-Z _-]*$^/ (I'll bet there's lots more characters people will want in there)

Yes, lots.

till+roundcube@whatever.com is valid too.

Think about the characters that some list servers allow.

As Charles said, the RFC is good point to start: http://en.wikipedia.org/wiki/E-mail_address#RFC_specification

Maybe we can take some BSD-licensed code from the Zend Framework: http://framework.zend.com/svn/framework/standard/trunk/library/Zend/Validate...

As you can see, email validation is sort of complex. ;-)

...

2.) On failure a message below the input box explaining that only such and such characters are allowed. (I'm not sure the transient nature of the existing error message display method is suitable for this task).

IMHO, a generic "invalid email address" is plenty. We don't need to confuse people with too much information.

...

What would be icing on top of that cake would be a client side (js) check which would change the color of the input box to a red outline if it has bad characters (or something like that).

You can put that in your template, use Jquery and attach a function to the appropriate events on the box.

Cheers, Till

...

Thoughts?

Thanks, Ziba

Webmaster Team University of Michigan

till wrote:

...

On Wed, Oct 15, 2008 at 5:18 PM, Ziba Scott ziba@umich.edu wrote:

...

When editing contacts, some invalid characters are not stripped or handled in some way. They make it all the way to the sql statement before things trip up. (Using a prepare statement thankfully prevents injecting a second statement. More details in: http://trac.roundcube.net/ticket/1485504)

I can work on a patch, but I'd appreciate some guidance first:

Should the backend explicitly validate the input against a regular expression? What is valid/invalid? How should the interface report bad characters and/or failed contact saves to the user?

Thanks, Ziba

Webmaster Team University of Michigan

I replied, let me know if this helps. :)

Thanks for all input!

Till

---

List info: http://lists.roundcube.net/dev/

List info: http://lists.roundcube.net/dev/

Hi Mike,

RC is using prepared statements. Even so, just quoting the character might not be the total answer because ticket: 1463946 claims that if this single quote were stored, it would cause problems down the line. So there is still a question of escaping, storing and fixing later problems or rejecting in the first place.

Cheers, Ziba

Michael Baierl wrote:

If you need to convert single quotes it means you don't use your DB
properly. Just use prepared statements only and this problem won't
exist any longer!

lg, Mike

List info: http://lists.roundcube.net/dev/

Hi,

Thanks for the response. There is at least one real problem I think we're all agreeing on:

Right now, if you put an apostrophe in the first or last name field of a contact and try and save it, mdb2 throws an error during prepare and the save fails. The quick solution may be to (as many people have reasonably suggested) just escape the sensitive characters.

This old ticket: http://trac.roundcube.net/ticket/1463946 Suggests a second problem: characters in contact fields other than email do impact the final string used to address the message.

Autocomplete concatenates contact fields when composing.

First name: John, Last Name: Doe, Email: jdoe@example.com becomes "John Doe jdoe@example.com".

If we allow anything into the first name field, then we allow autocomplete to put anything into the address fields on the compose screen.

Is that not a concern?

Thanks, Ziba

Webmaster Team University of Michigan

http://trac.roundcube.net/ticket/1463946

A.L.E.C wrote:

Ziba Scott wrote:

...

Hi Mike,

RC is using prepared statements. Even so, just quoting the character might not be the total answer because ticket: 1463946 claims that if this single quote were stored, it would cause problems down the line. So there is still a question of escaping, storing and fixing later problems or rejecting in the first place.

In names should be allowed any character. For email field should be used regex. That's all. Also there's quoting in rcube_contacts:

$a_insert_cols[] = $this->db->quoteIdentifier($col); $a_insert_values[] = $this->db->quote($save_data[$col]);

so really, I don't see where's the problem.

List info: http://lists.roundcube.net/dev/

On Thu, Oct 16, 2008 at 8:18 PM, Ziba Scott ziba@umich.edu wrote:

Hi,

Thanks for the response. There is at least one real problem I think we're all agreeing on:

Right now, if you put an apostrophe in the first or last name field of a contact and try and save it, mdb2 throws an error during prepare and the save fails. The quick solution may be to (as many people have reasonably suggested) just escape the sensitive characters.

Alec gave you the solution. It's called quoting data.

Another question is if we want to impose any rules on "validating" email adresses, it's basically not one solution to a single problem.

Problem a: MDB2 error => Fix: quote data

Problem b: "invalid" chars in an email address => Fix: the regex

This old ticket: http://trac.roundcube.net/ticket/1463946 Suggests a second problem: characters in contact fields other than email do impact the final string used to address the message.

Autocomplete concatenates contact fields when composing.

First name: John, Last Name: Doe, Email: jdoe@example.com becomes "John Doe jdoe@example.com".

What you bring up is yet another thing.

Whatever you put into the name field, it should be allowed.

It should look like this: "John ' Doe" john@doe.com

This is a perfectly valid email header.

And also, let's not create an issue where there is non.

And, correct - converting a string to HTML is *not* a valid fix. Never. ;-)

Till _______________________________________________ List info: http://lists.roundcube.net/dev/

till wrote:

On Thu, Oct 16, 2008 at 9:03 PM, Dennis P. Nikolaenko dennis@nikolaenko.ru wrote:

...

A.L.E.C wrote:

...

Ziba Scott wrote:

...

Hi Mike,

RC is using prepared statements. Even so, just quoting the character might not be the total answer because ticket: 1463946 claims that if this single quote were stored, it would cause problems down the line. So there is still a question of escaping, storing and fixing later problems or rejecting in the first place.

In names should be allowed any character. For email field should be used regex. That's all. Also there's quoting in rcube_contacts:

$a_insert_cols[] = $this->db->quoteIdentifier($col); $a_insert_values[] = $this->db->quote($save_data[$col]);

so really, I don't see where's the problem.

I think the problem lies in MDB2 the way it tries to avoid to substitute '?' inside quoted strings when calling prepare(). It looks that query() indirectly calls prepare() in MySQL MDB2 driver. Is it the latest version in RC tree?

I think this is not a bug, but a feature. If I remember correctly there is auto-quoting (or maybe I saw it in another DBAL).

MySQL MDD2 bails out on ' strings inside '-quoted strings, but it is a perfectly legal to have such a string in SQL. -- Dennis _______________________________________________ List info: http://lists.roundcube.net/dev/

till wrote:

On Thu, Oct 16, 2008 at 9:08 PM, Dennis P. Nikolaenko dennis@nikolaenko.ru wrote:

...

till wrote:

...

On Thu, Oct 16, 2008 at 9:03 PM, Dennis P. Nikolaenko dennis@nikolaenko.ru wrote:

...

A.L.E.C wrote:

...

Ziba Scott wrote:

...

Hi Mike,

RC is using prepared statements. Even so, just quoting the character might not be the total answer because ticket: 1463946 claims that if this single quote were stored, it would cause problems down the line. So there is still a question of escaping, storing and fixing later problems or rejecting in the first place.

In names should be allowed any character. For email field should be used regex. That's all. Also there's quoting in rcube_contacts:

$a_insert_cols[] = $this->db->quoteIdentifier($col); $a_insert_values[] = $this->db->quote($save_data[$col]);

so really, I don't see where's the problem.

I think the problem lies in MDB2 the way it tries to avoid to substitute '?' inside quoted strings when calling prepare(). It looks that query() indirectly calls prepare() in MySQL MDB2 driver. Is it the latest version in RC tree?

I think this is not a bug, but a feature. If I remember correctly there is auto-quoting (or maybe I saw it in another DBAL).

MySQL MDD2 bails out on ' strings inside '-quoted strings, but it is a perfectly legal to have such a string in SQL.

Yes, it's called "quoting data". You have to do it yourself.

Err, that is not MySQL driver specific, actually. In _skipDelimitedStrings() I get for $query when saving my identity:

SET name='Dennis P. Nikolaenko', email='dennis@nikolaenko.ru', organization='foo'', reply-to='', bcc='', standard='1', signature='Regards,\r\nDennis P. Nikolaenko', html_signature=0 WHERE identity_id=? AND user_id=? AND del<>1

This is legal SQL. But _skipDelimitedStrings() essentially counts ' to get the idea where strings start and end. It does not handle the case when string contains embedded ' -- Dennis _______________________________________________ List info: http://lists.roundcube.net/dev/

Ziba Scott wrote:

So I'm working on a patch which will make it easy to attach a regular expression for validation to every contact field so that any character in any field which will cause problems gets rejected and characters which trip up the current prepare statement but are acceptable for use down the line will get escaped and stored.

Invalid characters must not screw anything up on a DB side if you use SQL the right way - if it does it means your SQL code is wrong and not using prepared statements the right way.

The only reason for input validation is to reject chars that are invalid

• i.e. a @ in a first name does not make sense. The goal is not to fix

the SQL backend within the frontend code!

Mike

$sql = "update contacts set firstname = 'test's' where contact_id=?"; $sql_result = $RCMAIL->db->query($sql,'91');

The above SQL is not using prepared statements correctly. Every parameter in a query that may be user-defined should use the "?". I don't know the exact syntax for db->query(), but the above should look something like this:

$sql = "update contacts set firstname = ? where contact_id=?"; $sql_result = $RCMAIL->db->query($sql,"test's", "91");

Note there is NO escaping of single quotes. If using prepared statements correctly, you should never need to escape anything.

-gnul _______________________________________________ List info: http://lists.roundcube.net/dev/

gnul wrote:

...

$sql = "update contacts set firstname = 'test's' where contact_id=?"; $sql_result = $RCMAIL->db->query($sql,'91');

The above SQL is not using prepared statements correctly. Every parameter in a query that may be user-defined should use the "?".

Thank you for pointing that out. My example is modeled after what's really going on in rcube_contacts::update() _______________________________________________ List info: http://lists.roundcube.net/dev/

Michael Baierl wrote:

I agree with gnul - thanks for outlining the issue that way. As I
already said before - if prepared statements are used correctly there
is no need for quoting!

Dennis, Could you please elaborate a bit more on that issue? I don't quite
understand what you are trying to say. If the bug is fixed so it not
longer crashed the SQL statement, what remains unfixed?

lg, Mike

See http://trac.roundcube.net/ticket/1485504

Dennis _______________________________________________ List info: http://lists.roundcube.net/dev/