Revert "[FIX] core: remove ?session_id= way to provide sid"

This reverts commit ebb5939.

odoo/addons/test_http/tests/test_session.py

@@ -3,7 +3,6 @@
 import datetime
 import json
 import pytz
-from urllib.parse import urlencode
 from unittest.mock import patch
 
 import odoo
@@ -238,22 +237,3 @@ def test_session7_serializable(self):
             with self.assertRaises(TypeError):
                 dict.update(session, foo=value)
             self.assertFalse(session.foo)
-
-    def test_session8_logout(self):
-        sid = self.authenticate('admin', 'admin').sid
-        self.assertTrue(odoo.http.root.session_store.get(sid), "session should exist")
-        self.url_open('/web/session/logout', allow_redirects=False).raise_for_status()
-        self.assertFalse(odoo.http.root.session_store.get(sid), "session should not exist")
-
-    def test_session9_explicit_session(self):
-        forged_sid = 'da39a3ee5e6b4b0d3255bfef95601890afd80709'
-        admin_session = self.authenticate('admin', 'admin')
-        with self.assertLogs('odoo.http') as capture:
-            qs = urlencode({'debug': 1, 'session_id': forged_sid})
-            self.url_open(f'/web/session/logout?{qs}').raise_for_status()
-        self.assertEqual(len(capture.output), 1)
-        self.assertRegex(capture.output[0],
-            r"^WARNING:odoo.http:<function odoo\.addons\.\w+\.controllers\.\w+\.logout> "
-            r"called ignoring args {('session_id', 'debug'|'debug', 'session_id')}$"
-        )
-        self.assertEqual(admin_session.debug, '1')

odoo/http.py

@@ -862,8 +862,6 @@ class FilesystemSessionStore(sessions.FilesystemSessionStore):
     """ Place where to load and save session objects. """
     def get_session_filename(self, sid):
         # scatter sessions across 256 directories
-        if not self.is_valid_key(sid):
-            raise ValueError(f'Invalid session id {sid!r}')
         sha_dir = sid[:2]
         dirname = os.path.join(self.path, sha_dir)
         session_path = os.path.join(dirname, sid)
@@ -909,14 +907,15 @@ def vacuum(self, max_lifetime=SESSION_LIFETIME):
 
 class Session(collections.abc.MutableMapping):
     """ Structure containing data persisted across requests. """
-    __slots__ = ('can_save', '_Session__data', 'is_dirty', 'is_new',
+    __slots__ = ('can_save', '_Session__data', 'is_dirty', 'is_explicit', 'is_new',
                  'should_rotate', 'sid')
 
     def __init__(self, data, sid, new=False):
         self.can_save = True
         self.__data = {}
         self.update(data)
         self.is_dirty = False
+        self.is_explicit = False
         self.is_new = new
         self.should_rotate = False
         self.sid = sid
@@ -1233,12 +1232,25 @@ def _post_init(self):
         self._post_init = None
 
     def _get_session_and_dbname(self):
-        sid = self.httprequest.cookies.get('session_id')
-        if not sid or not root.session_store.is_valid_key(sid):
+        # The session is explicit when it comes from the query-string or
+        # the header. It is implicit when it comes from the cookie or
+        # that is does not exist yet. The explicit session should be
+        # used in this request only, it should not be saved on the
+        # response cookie.
+        sid = (self.httprequest.args.get('session_id')
+            or self.httprequest.headers.get("X-Openerp-Session-Id"))
+        if sid:
+            is_explicit = True
+        else:
+            sid = self.httprequest.cookies.get('session_id')
+            is_explicit = False
+
+        if sid is None:
             session = root.session_store.new()
         else:
             session = root.session_store.get(sid)
             session.sid = sid  # in case the session was not persisted
+        session.is_explicit = is_explicit
 
         for key, val in get_default_session().items():
             session.setdefault(key, val)
@@ -1434,6 +1446,7 @@ def get_http_params(self):
             **self.httprequest.form,
             **self.httprequest.files
         }
+        params.pop('session_id', None)
         return params
 
     def get_json_data(self):
@@ -1572,8 +1585,17 @@ def _save_session(self):
             sess['_geoip'] = self.geoip
             root.session_store.save(sess)
 
+        # We must not set the cookie if the session id was specified
+        # using a http header or a GET parameter.
+        # There are two reasons to this:
+        # - When using one of those two means we consider that we are
+        #   overriding the cookie, which means creating a new session on
+        #   top of an already existing session and we don't want to
+        #   create a mess with the 'normal' session (the one using the
+        #   cookie). That is a special feature of the Javascript Session.
+        # - It could allow session fixation attacks.
         cookie_sid = self.httprequest.cookies.get('session_id')
-        if sess.is_dirty or cookie_sid != sess.sid:
+        if not sess.is_explicit and (sess.is_dirty or cookie_sid != sess.sid):
             self.future_response.set_cookie('session_id', sess.sid, max_age=SESSION_LIFETIME, httponly=True)
 
     def _set_request_dispatcher(self, rule):
@@ -1812,7 +1834,7 @@ def handle_error(self, exc: Exception) -> collections.abc.Callable:
             was_connected = session.uid is not None
             session.logout(keep_db=True)
             response = self.request.redirect_query('/web/login', {'redirect': self.request.httprequest.full_path})
-            if was_connected:
+            if not session.is_explicit and was_connected:
                 root.session_store.rotate(session, self.request.env)
                 response.set_cookie('session_id', session.sid, max_age=SESSION_LIFETIME, httponly=True)
             return response