Elevating from DRUPAL_BOOTSTRAP_DATABASE (which takes <100 ms) to DRUPAL_BOOTSTRAP_SESSION (which can take 1500-1700 ms on my local dev) in order to only get the user ID and potentially a role ID or something like that is probably avoidable. In my own use case, I'm showing the user a lot of different data, such as comments, via modals, and that data is privileged to the current user only if they're attached to the particular content in some way, such as a user reference field.

In looking through session.inc, which is actually not as hairy as I anticipated, I've come to realize that we don't need to do much more than grab the session ID from the user's $_COOKIE and query the sessions table. I was blindly under the impression that there would be more security caveats here and, if so, I'd love to hear about them. Here's an example of some code at the top of a JS callback that can get the current user's ID and check if they're the author of a node:

<?php
// Helper function that can be called at the top of JS callbacks to get the user's UID.
function _mymodule_js_session_uid() {
  $session_name = session_name();
  if (!empty($session_name)) {
    if ($uid = db_query("SELECT uid FROM {sessions} WHERE sid = :sid", array(':sid' => $_COOKIE[$session_name]))->fetchColumn()) {
      return $uid;
    }
  }
  return FALSE;
}

function mymodule_js_callback_view_node($nid) {
  $uid = _mymodule_js_session_uid();
  if (!$uid) {
    return 'access denied';
  }

  // Only view this node if the current user is the author.
  if (db_query("SELECT nid FROM {node} WHERE nid = :nid AND uid = :uid", array(':nid' => $nid, ':uid' => $uid))->fetchColumn()) {
    return 'You are the author of this node and you can do whatever you want with it.'; 
  } else {
    return 'access denied';
  }
}
?>

The above code is missing the HTTPS SID (the ssid) which I'll play with a bit on my own, but the real purpose of this issue is to answer the question – what security concerns lie in using this method? I am able to serve up modals that contain all of the comments on a particular node, or to perform ajax actions on a node (change a status field, change a boolean field, etc) in ~ 90 ms, which is amazing!

On a side note, sorry for opening so many issues in such a short amount of time. I've been delighted with this module in D7 and am trying to use it for a lot of different things. Hopefully I can close most of these issues myself and they don't take up too much time by the maintainers.

Comments

michielnugter’s picture

michielnugter commented

Bootstrapping to session may take this long because along the way it also does a lot of other stuff as SESSION is a very late step in the bootstrap.

There might be something to this, maybe there is a way to initialize the session without the rest, minimizing the overhead. I really want to be able to use the default user_access method for access validation, anything else feels a bit too custom for the js module itself. The code above really focuses on node ownership but fails to respect the normal permissions (maybe the current user can view/edit all nodes). I really think it should focus on features which are at least similar to the normal Drupal process.
There is of course, nothing stopping you from adding this kind of access control in your own modules and validate permissions in the js callbacks themselves.

No problem at all on opening the issues, it's great to see the module being used, that's the whole reason for releasing it :)

charlie-s’s picture

charlie-s commented

I agree completely – the logic above was just to show a simple usage of this concept, but ultimately it should follow the existing access logic. I have node_access() / user_access() working locally:

<?php
/**
 * Helper function to get the current user's UID in a js callback.
 * TODO add support for ssid
 * @return user object with 'uid' and 'roles' properties.
 */
function _mymodule_js_session_user() {
  $session_name = session_name();
  if (!empty($session_name)) {
    if ($uid = db_query("SELECT uid FROM {sessions} WHERE sid = :sid", array(':sid' => $_COOKIE[$session_name]))->fetchColumn()) {
      $account = new stdClass();
      $account->uid = $uid;
      $account->roles = array();
      $account->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
      $account->roles += db_query("SELECT r.rid, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.rid = r.rid WHERE ur.uid = :uid", array(':uid' => $account->uid))->fetchAllKeyed(0, 1);
      return $account;
    }
  }
  return FALSE;
}

/**
 * Display comments from a node in a modal.
 */
function mymodule_view_comments($nid) {
  $account = _mymodule_js_session_user();
  $node = node_load($nid);
  if (!node_access('view', $node, $account)) {
   exit('access denied');
  }

  $output = '';
  $comments = db_query("SELECT c.created, c.name, b.comment_body_value AS body FROM field_data_comment_body b LEFT JOIN comment c ON c.cid = b.entity_id WHERE c.nid = {$nid} ORDER BY c.created DESC")->fetchAll();
  foreach ($comments as $comment) {
    $date = date("m/d/Y h:i", $comment->created);
    $comment_body = check_markup($comment->body, 'full_html');
    $output .= "<hr /><div><strong>Posted by {$comment->name} on {$date}</strong><br /><br />{$comment_body}</div>";
  }

  print $output;
  exit();
}

?>
coltrane’s picture

coltrane
he/him
commented

If you allow comments from anonymous or untrusted users you'll be vulnerable to an XSS attack via mymodule_view_comments().

You should not assume the comment body can be rendered in Full HTML and you should sanitize the comment name:

  $comment_body = check_markup($comment->body, 'filtered_html');
  $name  = check_plain($comment->name);
  $output .= "<hr /><div><strong>Posted by {$name} on {$date}</strong><br /><br />{$comment_body}</div>";
charlie-s’s picture

charlie-s commented

Thanks coltrane! A couple of questions:

1. What's wrong with Full HTML if I need it? I do not allow javascript to be sent through the full HTML filter. I also only allow (in this instance) comments to come from trusters users, but that's completely beside the point as even a trusted user could do something evil.

2. Why should I filter the user name?

coltrane’s picture

coltrane
he/him
commented

If you've configured the Full HTML format to filter dangerous tags then that's OK. By default Full HTML is just that, all HTML tags including dangerous ones. You can't just filter script though, embed, object are also dangerous. Edit, here's a list of dangerous tags http://drupalcode.org/project/security_review.git/blob/refs/heads/7.x-1.... but know that whitelisting (Filtered HTML) is better than blacklisting.

If you allow anonymous commenting you should filter the user name. Comment module stores the name in the comment table so it doesn't always pass through validation that would revoke control characters.

charlie-s’s picture

charlie-s commented

Good points, again. Thanks Coltrane.

michielnugter’s picture

michielnugter commented

I'm closing this as I don't think I'll ever implement it for the module. There are too many possible complications and security issues here.

Thank you very much for your input and thoughts on the module, they are very much appreciated!

michielnugter’s picture

michielnugter commented
Status: Active » Closed (won't fix)