Skip to content

Commit

Permalink
STCOR-895 wait a loooong time for a "stale" rotation request (#1547)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
zburke authored Oct 15, 2024
1 parent 0e4d2b4 commit cc8ef65
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
15 changes: 14 additions & 1 deletion src/components/Root/token-util.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { isEmpty } from 'lodash';
import ms from 'ms';

import { getTokenExpiry, setTokenExpiry } from '../../loginServices';
import { RTRError, UnexpectedResourceError } from './Errors';
Expand All @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/components/Root/token-util.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 () => {
Expand Down

0 comments on commit cc8ef65

Please sign in to comment.