From cc8ef65411c3911f5debe8f71161a112b0584d37 Mon Sep 17 00:00:00 2001
From: Zak Burke <zburke@ebsco.com>
Date: Tue, 15 Oct 2024 10:17:17 -0400
Subject: [PATCH] STCOR-895 wait a loooong time for a "stale" rotation request
 (#1547)

As part of the RTR lifecycle, we write a rotation timestamp to local
storage when the process starts and then remove it when it ends. This
is a cheap way of making the rotation request visible across tabs,
because all tabs read the same shared storage.

To avoid the problem of a cancelled request leaving cruft in storage, we
inspect that timestamp and consider a request "stale" if it's too old.
That was the problem here: our "too old" timeout was too short; on a
busy server, or on a slow connection, or on a client far from its host
(say, in New Zealand), two seconds was not long enough. The rotation
request would still be active when stripes considered it "stale",
allowing a second request to go through. But since the first request
was just slow, not dead, the second one is treated as a token-replay
attack by the backend, causing all active sessions for that user account
to be immediately terminated.

Thus, waiting longer is a quick fix. A more detailed approach to
tracking the rotation request is detailed in the comments for
RTR_MAX_AGE.

Refs STCOR-895
---
 CHANGELOG.md                           |  1 +
 src/components/Root/token-util.js      | 15 ++++++++++++++-
 src/components/Root/token-util.test.js |  4 ++--
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 21b65b78..02e0ea9b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -34,6 +34,7 @@
 * useUserTenantPermissions hook - provide `isFetched` property. Refs STCOR-890.
 * Reword error message "Error: server is forbidden, unreachable or down. VPN issue?". Refs STCOR-893.
 * Move session timeout banner to the bottom of the page. Refs STCOR-883.
+* Wait longer before declaring a rotation request to be stale. Refs STCOR-895.
 
 
 ## [10.1.1](https://github.com/folio-org/stripes-core/tree/v10.1.1) (2024-03-25)
diff --git a/src/components/Root/token-util.js b/src/components/Root/token-util.js
index 91abf340..aa30b685 100644
--- a/src/components/Root/token-util.js
+++ b/src/components/Root/token-util.js
@@ -1,4 +1,5 @@
 import { isEmpty } from 'lodash';
+import ms from 'ms';
 
 import { getTokenExpiry, setTokenExpiry } from '../../loginServices';
 import { RTRError, UnexpectedResourceError } from './Errors';
@@ -18,6 +19,18 @@ export const RTR_IS_ROTATING = '@folio/stripes/core::rtrIsRotating';
  * RTR_MAX_AGE (int)
  * How long do we let a refresh request last before we consider it stale?
  *
+ * WARNING: The implementation described below is naive and short timeouts
+ * (e.g. 2 seconds) have led to problems in production where slow responses
+ * are interpreted as stale, leading to a second request, which then fails
+ * when the first (slooooow) request completes. This looks like a token-
+ * replay attack from the backend's view, so it will then terminate all
+ * active sessions for a given user. A better approach would be to handle
+ * rotation in a worker thread, allowing more careful tracking of the
+ * rotation request since it would only be happening in a single thread.
+ * But ... that's a lot more work. The quick fix is to use a long value,
+ * which might not provide an ideal UX, but at least it won't be a broken
+ * UX.
+ *
  * When RTR begins, the current time in milliseconds (i.e. Date.now()) is
  * cached in localStorage and the existence of that value is used as a flag
  * in subsequent requests to indicate that they just need to wait for the
@@ -32,7 +45,7 @@ export const RTR_IS_ROTATING = '@folio/stripes/core::rtrIsRotating';
  *
  * Time in milliseconds
  */
-export const RTR_MAX_AGE = 2000;
+export const RTR_MAX_AGE = ms('20s');
 
 /**
  * resourceMapper
diff --git a/src/components/Root/token-util.test.js b/src/components/Root/token-util.test.js
index 0ed8cc7f..f3d8d458 100644
--- a/src/components/Root/token-util.test.js
+++ b/src/components/Root/token-util.test.js
@@ -180,7 +180,7 @@ describe('rtr', () => {
 
       expect(ex).toBe(null);
       // expect(window.removeEventListener).toHaveBeenCalled();
-    });
+    }, 25000); // timeout must be longer than token-util's RTR_MAX_AGE
 
     it('multiple window (storage event)', async () => {
       const logger = {
@@ -214,7 +214,7 @@ describe('rtr', () => {
 
       expect(ex).toBe(null);
       // expect(window.removeEventListener).toHaveBeenCalledWith('monkey')
-    });
+    }, 25000); // timeout must be longer than token-util's RTR_MAX_AGE
   });
 
   it('on known error, throws error', async () => {