333688 - event/task title is misrecorded in database when evaluating to a number

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[snotling]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.8.0.1) Gecko/20060313 Debian/1.5.dfsg+1.5.0.1-4 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.8) Gecko/20060412 Mozilla Sunbird/0.3a1+

When the title of an event/task is set to a number, it is stored in database as a number and not as a string.
So, for example, if the title is set to "10e4", the title is in fact recorded as and then displayed as "10000".
Not too much of a problem as event/task titles like this must be rare, but it may be a security issue though (possibility of kind of SQL injection ??).

Reproducible: Always

Steps to Reproduce:
1. Create a new event or task
2. Set its title to a number (e.g. 10e4), click "OK"
3. Look at its title => "10000".

Comment 1

[Joey Minta]

WFM in a build I made today, can anyone still reproduce?

Comment 2

[Joey Minta]

Oh, I see, you have to switch views to get the event from the actual db and see this bug.

Comment 3

[Matthew (lilmatt) Willis]

WFM using
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060909 Calendar/0.3a2+

I even quit and restarted Sunbird in an attempt to reproduce.

Marking qawanted.  If we can't reproduct this soon, wfm.

Comment 4

[Matthew (lilmatt) Willis]

I _know_ vlad's not working on this.

Comment 5

[Stefan Sitter [:ssitter]]

Confirmed using storage calendar and Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060910 Calendar/0.3a2+ Build-Id 2006091003.

Create event with number only as title, switch view and you'll see different title, e.g. 1e2 --> 100; 45e-3 --> 0.045; 00000 --> Untitled. 
If the number is part of string (e.g. '1e2 foobar') the issue doesn't exist.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Since something is not validating the incoming data correctly, I agree that there is some possibility for SQL-injection via iMIP.  At the very least, we need to rule out the possibility of that to get this off the blocking list.

Comment 7

[Matthew (lilmatt) Willis]

Would changing cal_events.title from a STRING to a VARCHAR possibly help?

I'm not entirely familiar with SQLite, and I'm wondering if STRING perhaps has some goofy "feature" we're seeing here.

I'm reaching.

Comment 8

[Matthew (lilmatt) Willis]

(In reply to comment #7)
> Would changing cal_events.title from a STRING to a VARCHAR possibly help?
> 
> I'm not entirely familiar with SQLite, and I'm wondering if STRING perhaps has
> some goofy "feature" we're seeing here.

Holy crow, I'm actually spot on.

For those playing along at home, we're using SQLite v3.

From section 2.1 of http://www.sqlite.org/datatype3.html

2.1 Determination Of Column Affinity

The type affinity of a column is determined by the declared type of the column, according to the following rules:

1. If the datatype contains the string "INT" then it is assigned INTEGER
   affinity.

2. If the datatype of the column contains any of the strings "CHAR", "CLOB", or
   "TEXT" then that column has TEXT affinity. Notice that the type VARCHAR
   contains the string "CHAR" and is thus assigned TEXT affinity.

3. If the datatype for a column contains the string "BLOB" or if no datatype is
   specified then the column has affinity NONE.

4. If the datatype for a column contains any of the strings "REAL", "FLOA", or
   "DOUB" then the column has REAL affinity

5. Otherwise, the affinity is NUMERIC.

</blockquote>

Note that "STRING" is not listed any of these, most notably rule 2.

Note the type of "title" in our schema:
http://lxr.mozilla.org/mozilla/source/calendar/providers/storage/calStorageCalendar.js#2029

Ya. STRING.

So does this mean we should be changing all the "STRING" to "VARCHAR" or something?  The fact that "id" is STRING also concerns me, since it could get messed up by this 10e4 -> 10000 behaviour also.

Next steps?

Comment 9

[Matthew (lilmatt) Willis]

(In reply to comment #7)
> Would changing cal_events.title from a STRING to a VARCHAR possibly help?

I just tested this using a fresh profile and today's nightly with a slight change to calStorageCalendar.js. (changed calevents.title from STRING to VARCHAR on line 2029).

An event with a title of 10e4 _stays_ 10e4, even after changing the view and after restarting the app.

I guess this means we need to do _another_ db schema update?

If we're doing that, we should add a uid column to cal_calendars so we can have a unique identifier there (unless we want to move away from ?id=1 to ?id=13413-12341234-12341234134-12341234134

Comment 10

[Matthew (lilmatt) Willis]

Attachment: Updates db schema to replace STRINGs with VARCHARs

So we don't have multiple schemas known as "v6" in nightlies out there, I've included some additional columns we need for better rfc2445 compliance and imip/itip.

If we need to add columns to store foreign tz info, let's commit that the same time as this.

Comment 11

[Matthew (lilmatt) Willis]

Attachment: fixes columns with hyphens

The columns I added late had hyphens which make sqlite complain.
Changed to underscores

Comment 12

[cmtalbert]

Comment on attachment 238699 [details] [diff] [review]
fixes columns with hyphens

This version Looks great! 
Tested by building lightning and installing into clean profile. Lilmatt has tested upgrade of database.

Comment 13

[Stefan Sitter [:ssitter]]

I get the following errors when upgrading existing profile:

Error: Upgrade failed! DB Error: duplicate column name: organizer
Source File: file:///Y:/obj/ltn/dist/bin/components/calStorageCalendar.js
Line: 1185

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Y:/obj/ltn/dist/bin/components/calStorageCalendar.js :: addColumn :: line 917"  data: no]

Comment 14

[Matthew (lilmatt) Willis]

Attachment: removes double column creation

Tables are created with the additional columns when the "_v6" versions are made.

Therefore the add column statements were redundant.

Comment 15

[Matthew (lilmatt) Willis]

Attachment: reverts pkeys to integers

Leaves the pkeys of cal_calendars and cal_calendars_prefs alone.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

While this does have a patch in hand, the level of risk here is not trivial, and number of events that evaluate to numbers is almost certainly infinitesimal. Minusing.

Comment 17

[Matthew (lilmatt) Willis]

Now that we're out of the 0.3 release cycle, the foreign timezone bug 314339 shouldn't block this. We can have another schema rev for that. Removing dependency.

Comment 18

[Matthew (lilmatt) Willis]

Comment on attachment 238724 [details] [diff] [review]
reverts pkeys to integers

dmose is on vaca.
Over to jminta (per jminta and dmose)

Comment 19

[Joey Minta]

Comment on attachment 238724 [details] [diff] [review]
reverts pkeys to integers

I'd really like to see the cal_calendars and cal_calendars_prefs parts factored into calCalendarManager.js.  The providers are currently completely independent of the manager, and that's a purity I think we should keep.  (This will help with unit-testing the storage provider too.)

+                this.mDB.executeSimpleSQL("INSERT INTO cal_calendars_v6(" + calendarCols.join(",") + ") " +
+                                          "     SELECT " + calendarCols.join(",") +
+                                          "       FROM cal_calendars");
+
This can easily be a helper-function, something like
function foo(aPropArray, aTableName) {}

Comment 20

[Joey Minta]

Comment on attachment 238724 [details] [diff] [review]
reverts pkeys to integers


+                var eventCols = ["cal_id", "id", "time_created",
+                                 "last_modified", "title", "priority",
+                                 "privacy", "ical_status", "recurrence_id",
+                                 "recurrence_id_tz", "flags", "event_start",
+                                 "event_start_tz", "event_end",
+                                 "event_end_tz", "event_stamp", "alarm_time",
+                                 "alarm_time_tz", "alarm_offset",
+                                 "alarm_related", "alarm_last_ack"];

You can also do a quick SELECT * on each table, and then use mozIStorageStatement's getColumnName, to avoid these long arrays

Comment 21

[Matthew (lilmatt) Willis]

Attachment: fixes for jminta

(In reply to comment #20)
> You can also do a quick SELECT * on each table, and then use
> mozIStorageStatement's getColumnName, to avoid these long arrays

My reasoning for not doing so is a) if you don't have any data in that particular table, you don't have a result set to getColumnName on  and b) I'd prefer to be explicit when upgrading/migrating all of a user's data. I don't want to accidentally pick up some data I didn't mean to do something with.

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

Why are you using VARCHAR instead of TEXT? VARCHAR typically has a limited length (at least in other databases i've used), while TEXT can have any length. We don't want to restrict the length of a description, for example...

Comment 23

[Matthew (lilmatt) Willis]

Attachment: Uses TEXT instead of VARCHAR per mvl

Per mvl, I switched from VARCHAR to TEXT. TEXT generally has an unlimited length on other dbs, whereas VARCHAR does not. TEXT still fixes the 10e4 -> 10000 problem though, so we win both ways.

Comment 24

[Matthew (lilmatt) Willis]

Comment on attachment 243906 [details] [diff] [review]
Uses TEXT instead of VARCHAR per mvl

bad merge. pulling review request

Comment 25

[Matthew (lilmatt) Willis]

Attachment: rev 6. removes alert bug detrius

There were pieces of bug 357458 scattered about in the previous patch.
Since bug 357458 landed, I've cleaned up this bug's patch.

Comment 26

[Joey Minta]

Comment on attachment 243991 [details] [diff] [review]
rev 6.  removes alert bug detrius

+
+    upgradeDB: function (oldVersion) {
This poor function is nameless!
+            dump ("**** Upgrading calCalendarManager schema to 6\n");
+
I'm not convinced of the value of this dump.

+                for (var i in tableNames) {
+                    this.mDB.executeSimpleSQL("DROP TABLE " + tableNames[i] + ";" +
+                                              "ALTER TABLE " + tableNames[i] + "_v6 " + 
+                                              "  RENAME TO " + tableNames[i] + ";");
+                }
If you're not explicitly using |i|, then this should be a for each loop. (Here and elsewhere)

                 // the change between 3 and 4 is the addition of
                 // recurrence_id and recurrence_id_tz columns to
                 // cal_events, cal_todos, cal_attendees, and cal_properties
                 addColumn(this.mDB, "cal_events", "recurrence_id", "INTEGER");
-                addColumn(this.mDB, "cal_events", "recurrence_id_tz", "VARCHAR");
+                addColumn(this.mDB, "cal_events", "recurrence_id_tz", "TEXT");
 
WOAH! Altering past, fixed upgrade paths seems extremely, extremely scary.  These columns are going to end up as TEXT at the end of this function anyway.

+                    for (var i in tableNames) {
+                        query += "DROP TABLE " + tableNames[i] + "_v6;"
+                    }
+                    this.mDB.executeSimpleSQL(query);
Since you've already used beginTransaction(), I don't think there's any real need to combine these into a single command.

I need to take another pass when I'm not about to pass out...

Comment 27

[Matthew (lilmatt) Willis]

Attachment: rev 7 - addresses jminta's comments so far

(In reply to comment #26)
> +    upgradeDB: function (oldVersion) {
> This poor function is nameless!
Fixed. Sorry about that.

> +            dump ("**** Upgrading calCalendarManager schema to 6\n");
> I'm not convinced of the value of this dump.
Removed.

> +                for (var i in tableNames) {
> If you're not explicitly using |i|, then this should be a for each loop. (Here
> and elsewhere)
Fixed in all 3 places.

> -                addColumn(this.mDB, "cal_events", "recurrence_id_tz",
> "VARCHAR");
> +                addColumn(this.mDB, "cal_events", "recurrence_id_tz", "TEXT");
> WOAH! Altering past, fixed upgrade paths seems extremely, extremely scary. 
> These columns are going to end up as TEXT at the end of this function anyway.
Good call. Removed.

> +                    for (var i in tableNames) {
> +                        query += "DROP TABLE " + tableNames[i] + "_v6;"
> +                    }
> +                    this.mDB.executeSimpleSQL(query);
> Since you've already used beginTransaction(), I don't think there's any real
> need to combine these into a single command.
I bet there is a performance benefit to not having the overhead of executing a SQL statement 6 times. At least, that's why I did it that way. I added your space too.

Comment 28

[Joey Minta]

Comment on attachment 244383 [details] [diff] [review]
rev 7 - addresses jminta's comments so far

+                // - Add sequence and organizer columns to cal_events and
+                //   cal_todos, and delegated-by, delegated-to, and sent-by
+                //   columns to cal_attendees. (RFC2445)
I'm not convinced of the value of combining all of the bugs this bug is blocking into this patch.  It has several drawbacks as I can see:
- Inadequate discussion/notice of the various changes.
- Larger patch size
- Longer review
- One set of fixes gets delayed because others aren't ready

We're not going to run out of integers, and our upgrade process is robust and tested.  Someone convince me I shouldn't ask these changes to be separated out into their own bugs.

Comment 29

[Matthew (lilmatt) Willis]

Attachment: rev8 - Fixes _only_ this bug

In the interest of getting this, the major schema change, in well before the test day, I've removed the schema changes that don't involve this bug.

When we were in the 0.3 afterglow, we thought it made more sense to make one schema bump with all the changes, however if you want to do it separately, that's okay.  It just means the upgrade section gets bigger :)

Comment 30

[Matthew (lilmatt) Willis]

Attachment: rev9 - typo fix

Fixes minor typo with trailing commas.

Comment 31

[Joey Minta]

Comment on attachment 246968 [details] [diff] [review]
rev9 - typo fix

+                // Copy in the data.
+                var cal_events_cols = ["cal_id", "id", "time_created",
+                                 "last_modified", "title", "priority",
None of these arrays are properly aligned

r=jminta with that, but consult with ctalbert about landing this close to the test day.

Comment 32

[Stefan Sitter [:ssitter]]

Comment on attachment 246968 [details] [diff] [review]
rev9 - typo fix

>-        var sqlTables = { cal_calendars: "id INTEGER PRIMARY KEY, type STRING, uri STRING",
>-                          cal_calendars_prefs: "id INTEGER PRIMARY KEY, calendar INTEGER, name STRING, value STRING"
>-        };
>+        var sqlTables = { cal_calendars: "id INTEGER PRIMARY KEY, type TEXT, uri TEXT",
>+                          cal_calendars_prefs: "id INTEGER PRIMARY KEY, calendar INTEGER, name TEXT, value TEXT",
>+                        };

I did a quick test on win32, so far this works good. The only nit I noticed is the following strict warning:

  Warning: trailing comma is not legal in ECMA-262 object initializers
  Source File: file:///Y:/obj/sb/dist/bin/js/calCalendarManager.js 
  Line: 215, Column: 24

Comment 33

[Matthew (lilmatt) Willis]

Removing bugs no longer blocked by this.

Patch checked in (with fix for strict warning error) on MOZILLA_1_8_BRANCH and trunk.

-> FIXED