fix: prevent concurrent token cache corruption (JSONDecodeError + unbuffered I/O)

msal_extensions/cache_lock.py

@@ -1,8 +1,6 @@
 """Provides a mechanism for not competing with other processes interacting with an MSAL cache."""
 import os
 import sys
-import errno
-import time
 import logging
 
 import portalocker  # pylint: disable=import-error
@@ -18,57 +16,39 @@ class CrossPlatLock(object):
     """Offers a mechanism for waiting until another process is finished interacting with a shared
     resource. This is specifically written to interact with a class of the same name in the .NET
     extensions library.
+
+    Fix: Use blocking LOCK_EX (no LOCK_NB) and never delete the lockfile.
+    The original design had two races:
+    1. LOCK_NB caused 300+ concurrent processes to bypass the lock (LockError raised instead of
+       waiting), allowing concurrent writes to the shared .tmp file.
+    2. Deleting the lockfile after releasing let another process create a new inode at the same
+       path, so two processes could each hold "exclusive" locks on different inodes simultaneously.
+    Both races caused the shared .tmp file to receive concurrent interleaved writes, producing
+    JSONDecodeError("Extra data") in the cache.
     """
     def __init__(self, lockfile_path):
         self._lockpath = lockfile_path
         self._lock = portalocker.Lock(
             lockfile_path,
-            mode='wb+',
-            # In posix systems, we HAVE to use LOCK_EX(exclusive lock) bitwise ORed
-            # with LOCK_NB(non-blocking) to avoid blocking on lock acquisition.
-            # More information here:
-            # https://docs.python.org/3/library/fcntl.html#fcntl.lockf
-            flags=portalocker.LOCK_EX | portalocker.LOCK_NB,
-            # Support for passing through arguments to the open syscall
-            # was added in Portalocker v1.4.0 (2019-02-11).
-            buffering=0,
+            # Use append mode: opens without truncating, creates file if absent.
+            # Never truncate the lockfile — that would corrupt another holder's fd.
+            mode='a',
+            # Use blocking LOCK_EX only (no LOCK_NB).
+            # With LOCK_NB, all but one of 300 concurrent processes would raise LockError
+            # immediately instead of waiting, bypassing the critical section entirely.
+            flags=portalocker.LOCK_EX,
         )
 
-    def _try_to_create_lock_file(self):
-        timeout = 5
-        check_interval = 0.25
-        current_time = getattr(time, "monotonic", time.time)
-        timeout_end = current_time() + timeout
-        pid = os.getpid()
-        while timeout_end > current_time():
-            try:
-                with open(self._lockpath, 'x'):  # pylint: disable=unspecified-encoding
-                    return True
-            except ValueError:  # This needs to be the first clause, for Python 2 to hit it
-                logger.warning("Python 2 does not support atomic creation of file")
-                return False
-            except FileExistsError:  # Only Python 3 will reach this clause
-                logger.debug(
-                    "Process %d found existing lock file, will retry after %f second",
-                    pid, check_interval)
-                time.sleep(check_interval)
-        return False
-
     def __enter__(self):
         pid = os.getpid()
-        if not self._try_to_create_lock_file():
-            logger.warning("Process %d failed to create lock file", pid)
         file_handle = self._lock.__enter__()
-        file_handle.write('{} {}'.format(pid, sys.argv[0]).encode('utf-8'))  # pylint: disable=consider-using-f-string
+        logger.debug("Process %d acquired token cache lock", pid)
         return file_handle
 
     def __exit__(self, *args):
+        # Release the lock but leave the lockfile on disk.
+        # Deleting + recreating the file changes its inode, which breaks the POSIX guarantee
+        # that fcntl locks are per-(process, inode) pair — two processes can then each hold
+        # an "exclusive" lock on different inodes of the same path simultaneously.
         self._lock.__exit__(*args)
-        try:
-            # Attempt to delete the lockfile. In either of the failure cases enumerated below, it is
-            # likely that another process has raced this one and ended up clearing or locking the
-            # file for itself.
-            os.remove(self._lockpath)
-        except OSError as ex:  # pylint: disable=invalid-name
-            if ex.errno not in (errno.ENOENT, errno.EACCES):
-                raise
+        logger.debug("Process %d released token cache lock", os.getpid())

msal_extensions/persistence.py

@@ -150,8 +150,20 @@ def __init__(self, location):
     def save(self, content):
         # type: (str) -> None
         """Save the content into this persistence"""
-        with os.fdopen(_open(self._location), 'w+') as handle:
-            handle.write(content)
+        # Use a per-process unique temp file to avoid the shared-tmp race: if two processes
+        # (which both bypassed the lock) write to the same .tmp path concurrently, the shorter
+        # write leaves leftover bytes from the longer write, corrupting the JSON with "Extra data".
+        tmp = "{}.tmp.{}.{}".format(self._location, os.getpid(), os.urandom(4).hex())
+        try:
+            with os.fdopen(os.open(tmp, os.O_RDWR | os.O_CREAT | os.O_TRUNC, 0o600), 'w+') as handle:
+                handle.write(content)
+            os.replace(tmp, self._location)
+        except Exception:
+            try:
+                os.unlink(tmp)
+            except OSError:
+                pass
+            raise
 
     def load(self):
         # type: () -> str