FormitRetriever without sessions but cache storage causes potential data leak or data loss

[wuuti]

Bug report

Summary

If FormitRetriever is used with storeLocation "cache" (which is the default), and sessions are not used (anonymous_sessions/sessions_enabled) in the context, concurrent requests to form pages can:

• show data other users have entered in the form (data leak), - often seen on confirmation pages
• delete data which has been entered by other users (data loss), with eraseOnLoad being set

Step to reproduce

Its hard to directly reproduce, as this only happens if concurrent requests happening at almost the same time are done on the form pages. Thats also the reasons the error is not seen on small pages, because traffic and concurrent requests there are very unlikely.
But larger pages or pages with peak loads (maybe due to a newsletter or some other offline "promotion") will have these problems.

But the behaviour is obvious when looking at the code which creates the cache-key to store the form data:

  return $this->modx->context->get('key') . '/elements/formit/submission/' . md5(session_id());

In sessionless contexts, the value of session_id() is always the empty string! Which causes the md5 to be a static value (md5 of empty string is always "d41d8cd98f00b204e9800998ecf8427e") and therefore every form post will write to the same cache file!

Expected behavior

Without a session there is no out-of-the-box way to have any persistent identifier to connect formit steps with redirects together (essentially making multi-step forms and confirmation pages with form content in the "traditional" impossible).

Mitigation steps:

• explicitly warn in the documentation that FormitRetriever cannot be used in sessionless contexts (regardless of the storagelocation!)
• change the code, so that the getStoreKey method logs a warning/error if session_id is empty - so that modx admins currently having that setup at least are warned, that something may go wrong with their forms
• in the longer term, formit should instead for sessionsless contexts:
  • create a unique hash per formit call
  • hash needs to be added as hidden form input field (on every form in multistep forms)
  • redirect should automatically add the hash value as an url/query parameter
  • formitretriever should use that param instead of session_id to retrieve previous data from cache

[wuuti]

Seems the code was in there since 2010, which was ok in those days. But in when Modx 2.2.1 introduced sessionless contexts in 2012, it became wrong, but no one noticed...