504411 - [MS-SQL] Bugzilla::User->is_available_user() breaks when no ':' in the eventdata field

Status: ASSIGNED

(Bugzilla :: Database, defect)

Description

[Michael Thomas (Mockodin)]

Creating a new user or changing a email will return an error when querying the token table when using a mssql DB. This is caused when the ':' is not present in eventdata field causing a position return value of 0. In the code below the return value is then substracted by 1 causing a negative value of -1. A negative length value will always return a error under MSSQL

Reason:
http://msdn.microsoft.com/en-us/library/ms187748.aspx
 length_expression

    Is a positive integer or bigint expression that specifies how many characters of the value_expression will be returned. If length_expression is negative, an error is generated and the statement is terminated. If the sum of start_expression and length_expression is greater than the number of characters in value_expression, the whole value expression beginning at start_expression is returned.


my $eventdata = $dbh->selectrow_array(
        "SELECT eventdata
           FROM tokens
          WHERE (tokentype = 'emailold'
                AND SUBSTRING(eventdata, 1, (" .
                    $dbh->sql_position(q{':'}, 'eventdata') . "-  1)) = ?)
             OR (tokentype = 'emailnew'
                AND SUBSTRING(eventdata, (" .
                    $dbh->sql_position(q{':'}, 'eventdata') . "+ 1 ), length( eventdata ) ) = ?)",
         undef, ($username, $username));

Opening a separate ticket for the fix, after discussing with Mkanat, for splitting the eventdata in the token table to to multiple fields to allow easier look up.

Comment 1

[Frédéric Buclin]

You cannot trigger this problem, AFAIK. ":" is always present, see Token.pm line 97:

 my ($token, $token_ts) = _create_token($user->id, 'emailold', $old_email . ":" . $new_email);

How did you get this error?

Comment 2

[Frédéric Buclin]

The SQL query first looks for tokentype. MS-SQL shouldn't look for ":" unless the tokentype has the expected value.

Comment 3

[Frédéric Buclin]

I checked, and both MySQL and PostgreSQL ignore the 2nd condition if the first one is false. So MS-SQL is broken.

Comment 4

[Max Kanat-Alexander]

Architecturally, though, it would still be better to have a second column, so I don't really object to that refactoring if Michael wants to do it. And MS-SQL may be broken, but complaining about it isn't going to get us MS-SQL support. :-)

Comment 5

[Frédéric Buclin]

The 2nd column only makes sense if we can guarantee that a token will never store more than 2 information. We are not going to add a new column everytime we want to store more data. That's what I was saying in the other bug (I don't have it's ID in mind).

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> The 2nd column only makes sense if we can guarantee that a token will never
> store more than 2 information. We are not going to add a new column everytime
> we want to store more data. That's what I was saying in the other bug (I don't
> have it's ID in mind).

  We don't need more than two right now, and I don't see any need on the horizon for more than two. I'm not worried about it for now. I'm also OK with storing a lot of data in one column *so long as we don't have to separate it out programatically before retrieving it*, because that's the job of separate columns.

Comment 7

[Michael Thomas (Mockodin)]

(In reply to comment #6)
>   We don't need more than two right now, and I don't see any need on the
> horizon for more than two. I'm not worried about it for now. I'm also OK with
> storing a lot of data in one column *so long as we don't have to separate it
> out programatically before retrieving it*, because that's the job of separate
> columns.

Agreed on stuffing data in field then getting it back.

(In reply to comment #5 and Bug 504411)
Storing data wise, any more than three data columns of the type we are addressing and the purpose of the tokens table would have shifted from simple auditing and would likely warrant a dedicated table for what ever we wanted to accomplish anyhow.

Another option would be field level XML, not that I'm in favor of it particularly as it introduces more complexity not less. XML is however a better method than : separators.

re comment 3 I actually will likely file a bug with MS, and not hold my breath, might as well use up some support pack incidents now and then.

Comment 9

[Michael Thomas (Mockodin)]

Looking at some of the other data in eventdata we are storing things like
edit_parameters
edit_products
edit_user_prefs

For tokentype we appear to always populate 'session', is this right? The only exception being change email oddly enough where we store the text 'oldemail' or 'newemail' which as a whole seems out of place. In reality the values for email change make the most sense in the field..

Ought not the tokentype be in effect the event type? With the data being blank or referencing which parameter, product or pref name that was changed? If you're really looking to track what change occurred we would want to store something for each object with its old and new values.

Comment 10

[Michael Thomas (Mockodin)]

Alright incorrect thinking on my part, ignore comment 9

Comment 11

[Michael Thomas (Mockodin)]

Attachment: Patch v1

Initial Patch - Still Reviewing

Thus far it looks like the only process that needed to be updated was change email. Password change working more in line with a standard session token required no change.

Comment 12

[Michael Thomas (Mockodin)]

Comment on attachment 389962 [details] [diff] [review]
Patch v1

Haven't seen anything else needing addressing in the last few days of poking.

Requested review from mkanat

Comment 13

[Max Kanat-Alexander]

Comment on attachment 389962 [details] [diff] [review]
Patch v1

I think LpSolit is more familiar with this code than I am.

Comment 14

[Michael Thomas (Mockodin)]

LpSolit any time for a review?

Comment 15

[Frédéric Buclin]

(In reply to comment #14)
> LpSolit any time for a review?

It's not a top-priority right now, but I should have time before the end of the month (I'm going on vacation in a few hours, coming back in a week).

Comment 16

[Frédéric Buclin]

Comment on attachment 389962 [details] [diff] [review]
Patch v1

>Index: token.cgi

>-    my ($userid, $tokentype, $eventdata) = $dbh->selectrow_array(
>+    my ($userid, $tokentype, $old_email) = $dbh->selectrow_array(
>                               q{SELECT userid, tokentype, eventdata FROM tokens
>                                 WHERE token = ?}, undef, $token);

cancelChangeEmail() can be called either using the emailold or emailnew token. If the emailnew token is used, eventdata contains the new email, not the old one, in which case your logic is broken.

Also, I see no code to fix old eventdata content, i.e. those of the form old:new. I didn't check what would happen if a token generated before the upgrade is used after the upgrade (e.g. what would be the email address in that case? Or could it be possible to bypass security checks due to a now invalid token content?).


>Index: Bugzilla/User.pm

>-    if ($eventdata) {
>+    if ( $old_email && $new_email ) {

$eventdata was true if either the emailold or emailnew token was present in the DB. So you must use || here instead of && to reflect that.


Otherwise looks good and is a nice cleanup.

Comment 17

[Michael Thomas (Mockodin)]

(In reply to comment #16)
> $eventdata was true if either the emailold or emailnew token was present in the
> DB. So you must use || here instead of && to reflect that.

Shouldn't both always exist? token.cgi deletes both after an action occurs.

Comment 18

[Frédéric Buclin]

(In reply to comment #17)
> Shouldn't both always exist? token.cgi deletes both after an action occurs.

No, token.cgi only deletes the emailnew one, as the old account has 3 days to revert the email address change.

Comment 19

[Michael Thomas (Mockodin)]

Attachment: v2

I'm not setup to test this fully as my environment is built for ENV Auth, but I believe this should work as intended.

Comment 20

[Frédéric Buclin]

Comment on attachment 409974 [details] [diff] [review]
v2

>Index: bugzilla/token.cgi

>+    # Get the user's ID, the tokentype and the associated email from the tokens table.
>+    my ($userid, $tokentype, $email) = $dbh->selectrow_array(   
>+                              q{SELECT userid, tokentype, eventdata FROM tokens
>+                                WHERE token = ?}, undef, $token);

changeEmail() can only be called with tokentype = 'emailnew', read the code earlier in token.cgi. So you already know what tokentype is.


>+    # Reverse the tokentype
>+    my $tokentype_to_find = ($tokentype eq 'emailnew' ? 'emailold' : 'emailnew');
>+
>+    # Get the other email
>+    $emails{$tokentype} = $dbh->selectrow_array(qq{SELECT eventdata FROM tokens
>+                                WHERE userid = ? AND tokentype = '$tokentype_to_find'}, undef, $userid);

This code is wrong. $emails{$tokentype} is already populated. Also, put $tokentype_to_find (which is a useless variable, per my previous comment) in a placeholder.


I didn't read further. Please test your patch first.

Comment 21

[Michael Thomas (Mockodin)]

(In reply to comment #20)
> changeEmail() can only be called with tokentype = 'emailnew', read the code
> earlier in token.cgi. So you already know what tokentype is.

Not really something I can see just glancing at the code, 'changeEmail()' is only referenced one other place in token.cgi:

} elsif ($action eq 'chgem') {
    changeEmail($token);
}

Given you wrote most of this I'll take you word for it. My changes where designed to not care if it got the newemail token or the oldemail token.

Question: Since token is always emailnew aren't these redundant to one another?
    $dbh->do('DELETE FROM tokens WHERE token = ?', undef, $token);
    $dbh->do(q{DELETE FROM tokens WHERE userid = ?
               AND tokentype = 'emailnew'}, undef, $userid);

Even if not, can't they be condensed to:
$dbh->do("DELETE FROM tokens WHERE
          token = ?' OR (userid = ? AND tokentype = 'emailnew'", undef, $token);

If they are redundant this should work on its own?:
$dbh->do("DELETE FROM tokens WHERE userid = ? AND tokentype = 'emailnew'", undef, $token);


Note that my testing environment is specifically for the environment I'm building for, in which email changes are not possible. ENV auth is rather static login wise. This change is owing to differences in db functionality requirements and simplifying the token lookups so that substring() is not required in the first place.

>This code is wrong. $emails{$tokentype} 
Correct should have been $emails{$tokentype_to_find}, given your previous point doesn't matter I guess as I will not need to reverse the token types any longer.

Comment 22

[Michael Thomas (Mockodin)]

Attachment: v3

Comment 23

[Frédéric Buclin]

Comment on attachment 429014 [details] [diff] [review]
v3

Hum, unless I miss something, you also have to fix Bugzilla::Token::IssueEmailChangeToken() to use the new token format.

Comment 24

[Michael Thomas (Mockodin)]

Attachment: v4

er yes, was omitted in error.

Comment 25

[Frédéric Buclin]

Comment on attachment 436375 [details] [diff] [review]
v4

Still incomplete. Install/DB.pm is now missing.

Comment 26

[Michael Thomas (Mockodin)]

Attachment: v5

Comment 27

[Frédéric Buclin]

Targetting to 4.2 as Bugzilla won't support MS-SQL in 4.0.

Comment 28

[Frédéric Buclin]

Comment on attachment 445129 [details] [diff] [review]
v5

>=== modified file Bugzilla/Install/DB.pm

>+sub _convert_email_tokens {

>+        "SELECT token, tokentype, eventdata FROM tokens WHERE eventdata like '%:%' AND (tokentype = 'newemail' OR tokentype = 'oldemail')",

tokentype is either emailold or emailnew. newemail and oldemail do not exist as token types.


>+    foreach my $row (@$email_tokens) {
>+        my ($token, $tokentype, $eventdata) = $row;

$row is an arrayref, not an array. You must write @$row.


Make sure to test each piece of your patch carefully before requesting review. I didn't read further.

Comment 29

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.