kirsh [kirsh] [#589074]

Hi,

I want to contribute a module that I wrote - auto increment (serial) CCK field.
The module is ready (including documentation) and works with no known issues.
As far as I know - currently no module supports auto increment (serial) CCK field.

An existing module, type_local_nids (http://drupal.org/project/type_local_nids) supports serial numbers, but:
(1) not as fields, and the author states that "This module should be reimplemented as a CCK/FIC field".
(2) It has a severe issue with atomic as explained at http://drupal.org/node/398116.

For atomic support (i.e. to handle allocation of serial numbers correctly when multiple nodes are created simultaneously) I had to use a totally different architecture, and my new module do not share anything common with type_local_nids.

type_local_nids is nice and very simple (< 200 lines) but giving its limitations, IMO, has to be replaced.

About myself: my background includes 20 years of software development experience, B.A, M.Sc. and Ph.D. in computer since, and thousands of B.Sc. and M.Sc. students that I taught web development and Java in the last 12 years.

Thanks,
Ilan Kirsh

Comment	File	Size	Author
#6	serial.zip	10.27 KB	kirsh
#1	serial.zip	9.82 KB	kirsh

Comments

Comment #1

kirsh commented 27 September 2009 at 03:11

Status	File	Size
new	serial.zip	9.82 KB

Sorry for the typo, I meant computer science.

Attached the serial module that I wrote and want to contribute.

Regards,
Ilan

Comment #2

kirsh commented 27 September 2009 at 03:20

Status:

Postponed (maintainer needs more info)

» Needs review

Comment #3

ajk commented 29 September 2009 at 10:07

Assigned:	Unassigned	» ajk
Status:	Needs review	» Needs work

function _serial_get_table_name($field) {
  return 'serial_' . $field['type_name'] . '_' . $field['field_name'];
}

function _serial_generate_value($nid, $field, $delete = TRUE) {
  // Insert a temporary record to get a new unique serial value:
  $table = _serial_get_table_name($field);
  $query = 'INSERT INTO {' . $table . '} (nid) VALUES(%d)';
  db_query($query, $nid);
  //.......
}

No use of db_escape_table() to ensure the SQL is filtered. I'm not convinced there's a chance of an SQL injection exploit here. Can you convince me otherwise?

Generally speaking, embedding PHP variables into SQL like that should be avoided. If you are happy you have it right and there's no security implications you should place a comment in the code as to why you are not using Drupal's database abstraction layer in the normal way.

Comment #4

kirsh commented 29 September 2009 at 19:56

Good point and an interesting question. Considering the constraints on names of node types and fields, I believe there is no security hole here. However, better safe than sorry, so I think it will be wise to add db_escape_table() anyway, just to be on the safe side.

This module must generate dynamic tables (one per serial field) so it cannot use simple constant database schema as done in most modules (I will emphasis this point in additional comments).

Maybe the table should be passed also as an argument, something like that:

$query = 'INSERT INTO %s (nid) VALUES(%d)';
db_query($query, db_prefix_tables('{'. $table .'}'), $nid);

This might be less readable, but should I use this form?

The only module that I know that uses dynamic schema is CCK and it also uses queries with dynamic table names (e.g. in content.admin.inc) as I did. By the way, I didn't find any db_escape_table() call in CCK. But because table names are generated from field names it is probably not a security hole there either.

Comment #5

ajk commented 30 September 2009 at 08:44

This form:-

  $query = 'INSERT INTO %s (nid) VALUES(%d)';
  db_query($query, db_prefix_tables('{'. $table .'}'), $nid);

could result in a mangled table name which would cause the query to fail so you'd be better to use db_escape_table() and place the result in {...} for the prefixing.

Good point about CCK. I'd like to think they know what they're doing :) But my hackles are always raised when I see a PHP var buried in an SQL statement.

Comment #6

kirsh commented 30 September 2009 at 12:32

Status:

Needs work

» Needs review

Status	File	Size
new	serial.zip	10.27 KB

Attached a new version with the following changes and fixes:
(1) Comment regarding PHP in SQL at the top of serial.inc
(2) All table name are now wrapped with db_escape_table().
(3) Fixed a bug that caused creating dynamic tables also for non serial fields.
(4) New message after adding a serial field (which also tells how many existing nodes have been initialized).
(5) Performance improvement - temporary tables are now cleaned once in 10 serial allocations.