I tried to use the session id field in the database adn found that the data type is set as int(10). Session id's are alpha numeric. Trying to insert the current users session id results in a database/pdo error. To fix this error and avoid session hi-jacking using brute force the data type should be set to varchar(128).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shabana.navas’s picture

Status: Active » Needs review
FileSize
447 bytes

Thanks for pointing out the error. The patch for this has been attached.

joachim’s picture

Status: Needs review » Postponed (maintainer needs more info)

I don't think that's correct.

The session ID column here is AFAIK not for PHP session IDs, but for the numeric session IDs produced by Session API.

joachim’s picture

(BTW: @Shabana Blackborder for future reference, any changed to hook_schema() need an accompanying hook_update_N() -- https://api.drupal.org/api/drupal/modules!system!system.api.php/function...)

shabana.navas’s picture

Joachim, but the Sessions API module has also changed their data type to varchar(128) to match the core {sessions}.sid field. I think we need to reflect that in Flags as well.

any changed to hook_schema() need an accompanying hook_update_N()

My bad, yeah, I knew I forgot something when posting this patch. Will post the patch with hook_update_N shortly...

shabana.navas’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
775 bytes

Patch with the hook_update_N() is attached.

joachim’s picture

Status: Needs review » Postponed (maintainer needs more info)

This is the session api table:

  $schema['session_api'] = array(
    'description' => t('Map Session API IDs to the {sessions} sid field.'),
    'fields' => array(
      'sid' => array(
        'type' => 'serial',
        'not null' => TRUE,
      ),
      'session_id' => array(
        'type' => 'varchar',
        'length' => 128,
        'not null' => TRUE,
      ),
      'timestamp' => array(
        'type' => 'int',
        'not null' => TRUE,
        'default' => 0,
      ),
    ),

AFAIK Flag is using the sid, which is the serial ID. If we're going to refer to the session_id, then we should call it that. But if we're using the identifier from the core table, what are we depending on Session API for?

shabana.navas’s picture

@joachim, You are absolutely right. Yeah, we should leave it as int(10). I mistakenly took the session_id as sid. Just really having a bad day....:-(

But I am wondering, why @carlmcdade, was getting the exception unless he was manually trying to do something:

Trying to insert the current users session id results in a database/pdo error.

Definitely need more info...

carlmcdade’s picture

Yes,

I am adding the sid manually in my module. I need to prevent users from flagging the same item in seperate sessions. In the database table structure for {flagging} the comment is:

sid The user’s session id as stored in the session table.

So this needs to be changed if it is not a session id for the user but something else.

joachim’s picture

Title: Database table schema is incorrect data type for session ids » Database table schema description is incorrect for flagging.sid
Status: Postponed (maintainer needs more info) » Active

Let's fix that description to say it's the numeric sid from the session API table.

carlmcdade’s picture

The sessions table looks like this:

Column Type Comment
uid int(10) unsigned The users.uid corresponding to a session, or 0 for anonymous user.
sid varchar(128) A session ID. The value is generated by Drupal’s session handlers.
ssid varchar(128) [] Secure session ID. The value is generated by Drupal’s session handlers.
hostname varchar(128) [] The IP address that last used this session ID (sid).
timestamp int(11) [0] The Unix timestamp when this session last requested a page. Old records are purged by PHP automatically.
cache int(11) [0] The time of this user’s last post. This is used when the site has specified a minimum_cache_lifetime. See cache_get().
session longblob NULL The serialized contents of $_SESSION, an array of name/value pairs that persists across page requests by this session ID. Drupal loads $_SESSION from here at the start of each request and saves it at the e

Is there something in the Sessions API that is different?

shabana.navas’s picture

Title: Database table schema description is incorrect for flagging.sid » Database table schema description is incorrect for flagging.sid
Issue summary: View changes
Status: Active » Needs review
FileSize
876 bytes

Changed the description to say it is from the session_api table.

joachim’s picture

Docs for https://api.drupal.org/api/drupal/includes!database!database.inc/functio... say:

> IMPORTANT NOTE: To maintain database portability, you have to explicitly recreate all indices and primary keys that are using the changed field.

and two indices use the sid field.

Though I am honestly not sure if this warrants a hook_update_n.

Will schema module complain if descriptions don't match?

shabana.navas’s picture

Well, I tested this and schema module didn't complain and the description was correctly updated without getting any errors.

Though I am honestly not sure if this warrants a hook_update_n.

But, now, I think you are right, does this tiny change really warrant a hook_update_n? Maybe, we should just change the original table description and leave it at that. I am sure, except for this one case, everyone else who has got the flag module installed does not have any confusion with the current description.

shabana.navas’s picture

Status: Needs review » Fixed

Made the tiny change to the table schema and committed to 7.x-3.x branch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.