I don't know of a good way to demonstrate this with the current subset of modules, because I think only the watchdog module uses the content channels, but this started affecting me as I was impatiently refreshing my page with the ajax comments integration module :)

The problem:

When the page loads, if you call nodejs_send_content_channel_token() a random token is sent to the node server, and the same token is added in the JS so your auth requests will use that token value, they'll match up and all will be happy in the world.

However, if the node server is down when the token is sent there, the token won't make it into the tokenChannels[tokenChannel] list, but, your authData will be cached, causing its list of contentTokens to be used in every subsequent auth request.

So, if the node server is down and you connect, a random token is cached in authData.contentTokens and the tokenChannels list is empty. But now if you refresh, a new random token is inserted into tokenChannels[tokenChannel], but you'll still be using the old authData.contentTokens data which doesn't match and you'll never be able to connect to the channel.

The solution:

Make the token not random, and instead use the channel's name to generate.

CommentFileSizeAuthor
say_no_to_random_content_tokens.patch1.2 KBinolen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

inolen’s picture

Issue summary: View changes

brap

inolen’s picture

Issue summary: View changes

brap

inolen’s picture

Actually, while this solves part of a problem, the real problem still is that contentTokens should be unique with each connection (I think), but they're cached in authData.

I threw in this hack

    // update contentTokens (these are unique to each connection, so it's ok to nuke the old ones)
    authenticatedClients[message.authToken].contentTokens = message.contentTokens;

right before it calls setupClientConnection() in authenticateClient(), which seems to provide the desired functionality.

Does anyone else think this is the desired functionality, that is, per connection contentTokens? If so, I can look into a nicer way to fix it :)

Anonymous’s picture

yes, that's right, we definitely want contentTokens to have a life cycle in sync with the socket itself.

will review and commit this today - thanks!

Anonymous’s picture

Status: Needs review » Fixed
Anonymous’s picture

and i found a bunch more bugs - thanks for pushing me with new ideas and code.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

I need to proofread before I post. brap.

  • Commit 02f299e on 7.x-1.x, auth-refactor, 8.x-1.x, 8.x-1.x-head by beejeebus:
    #1282464: don't cache contentTokens, thanks inolen for spotting the bug
    
    
  • Commit aed84fc on 7.x-1.x, auth-refactor, 8.x-1.x, 8.x-1.x-head by beejeebus:
    #1282464: fixes to token channel code, now with 100% more GC
    
    
  • Commit ae4ba2e on 7.x-1.x, auth-refactor, 8.x-1.x, 8.x-1.x-head by beejeebus:
    #1282464 by inolen, beejeebus: inolen was right, make...