Skip to content

Commit 7742c9c

Browse files
committed
fix(session,security): close 11 correctness and hardening gaps
CRITICAL - Grace period for first request after login: JWT-verify wrapper now accepts a freshly-issued JWT within a configurable `sessionCreationGraceMs` window (default 5s) even if the session-create DB write has not yet propagated. Closes the race where strictSessionEnforcement blocked the very first authenticated request. - Cleanup no longer leaves sessions reactivatable. Inactivity cleanup now sets terminatedManually=true, and the JWT-verify wrapper refuses to reactivate sessions that have been idle longer than inactivityTimeout. The idle-logout feature actually works now. - SESSION_ENCRYPTION_KEY is mandatory in production (boot self-test in bootstrap). Fallback to APP_KEYS is kept only for non-production, with a one-time WARN; this prevents APP_KEYS rotation from silently bricking every stored session. HIGH - mountRefreshTokenInterceptor now documents that users-permissions ships no /api/auth/refresh by default and the middleware is a passthrough until a refresh plugin is installed. - ensureContentApiPermissions migrated from the deprecated strapi.entityService to strapi.documents (Strapi v5 API). - extractBearerToken token-length range widened from 10..4096 to 40..8192 to accommodate modern JWTs carrying user-context claims (magic-link). - session.touch() now coalesces concurrent callers through an in-memory per-session stamp so two parallel requests cannot both skip the rate-limit and issue duplicate UPDATEs. MEDIUM - Missing JWT service is now logged at ERROR (not WARN) because session revocation silently stops enforcing — ops needs to see it. - register.js afterUpdate hook checks params.data.blocked to only scan sessions when THIS update set blocked=true, not on every unrelated user update. - Per-caller rate-limit middleware (plugin::magic-sessionmanager.rate-limit) mounted on all content-api routes: 10/min for writes (logout, terminate), 120/min for reads. - cleanupInactiveSessions gained a useDbDirect option (exposed via the cleanupUseDbDirect setting) for a single-statement SQL UPDATE path on installations that outgrow the 50k Document-Service scan cap.
1 parent 34f64a4 commit 7742c9c

9 files changed

Lines changed: 393 additions & 66 deletions

File tree

server/src/bootstrap.js

Lines changed: 112 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,17 @@ module.exports = async ({ strapi }) => {
5353

5454
log.info('[START] Bootstrap starting...');
5555

56+
// Fail fast on missing production secrets. getEncryptionKey() throws
57+
// when NODE_ENV=production and SESSION_ENCRYPTION_KEY is missing, so a
58+
// bad deploy dies here with a clear error instead of silently failing
59+
// later when the first encrypted-token read/write happens.
60+
try {
61+
encryptToken('self-test');
62+
} catch (e) {
63+
log.error(`[BOOTSTRAP] ${e.message}`);
64+
throw e;
65+
}
66+
5667
try {
5768
await ensureTokenHashIndex(strapi, log);
5869

@@ -109,13 +120,23 @@ module.exports = async ({ strapi }) => {
109120
const sessionService = strapi.plugin('magic-sessionmanager').service('session');
110121

111122
log.info('Running initial session cleanup...');
112-
await sessionService.cleanupInactiveSessions();
123+
try {
124+
const settings = await getPluginSettings(strapi);
125+
await sessionService.cleanupInactiveSessions({
126+
useDbDirect: settings.cleanupUseDbDirect === true,
127+
});
128+
} catch (cleanupErr) {
129+
log.warn('Initial cleanup failed:', cleanupErr.message);
130+
}
113131

114132
const cleanupInterval = 30 * 60 * 1000;
115133
const cleanupIntervalHandle = setInterval(async () => {
116134
try {
117135
const service = strapi.plugin('magic-sessionmanager').service('session');
118-
await service.cleanupInactiveSessions();
136+
const settings = await getPluginSettings(strapi).catch(() => ({}));
137+
await service.cleanupInactiveSessions({
138+
useDbDirect: settings.cleanupUseDbDirect === true,
139+
});
119140
} catch (err) {
120141
log.error('Periodic cleanup error:', err);
121142
}
@@ -649,8 +670,23 @@ function mountLoginInterceptor({ strapi, log, sessionService }) {
649670
}
650671

651672
/**
652-
* Blocks refresh-token requests that no longer correspond to an active
653-
* session, and rotates the tokens atomically after a successful refresh.
673+
* Refresh-token interceptor.
674+
*
675+
* NOTE ON APPLICABILITY:
676+
* Strapi's default users-permissions plugin does NOT ship a
677+
* POST /api/auth/refresh endpoint. This middleware only does something
678+
* when a refresh-token plugin (e.g. strapi-plugin-jwt-refresh) or a
679+
* custom controller is installed that exposes that route. Otherwise
680+
* the middleware is a pure passthrough — mounting it unconditionally
681+
* is safe and prepares the installation for future refresh-token
682+
* support without requiring a second plugin install/restart cycle.
683+
*
684+
* Responsibilities when a refresh endpoint IS present:
685+
* 1. Block incoming requests whose refreshToken does not match an
686+
* active session (defence against stale/stolen refresh tokens).
687+
* 2. After a successful refresh, atomically rotate the stored token +
688+
* refreshToken hashes on the session so the new JWT can be
689+
* validated by magicSessionAwareVerify.
654690
*
655691
* @param {{strapi: object, log: object}} deps
656692
*/
@@ -761,8 +797,13 @@ async function ensureContentApiPermissions(strapi, log) {
761797
const ROLE_UID = 'plugin::users-permissions.role';
762798
const PERMISSION_UID = 'plugin::users-permissions.permission';
763799

764-
const roles = await strapi.entityService.findMany(ROLE_UID, {
800+
// Document Service replaces the deprecated entityService (Strapi v5).
801+
// We still need the numeric `id` of the role for the permission's
802+
// `role` relation because users-permissions Permission stores the
803+
// foreign key by numeric id, not documentId.
804+
const roles = await strapi.documents(ROLE_UID).findMany({
765805
filters: { type: 'authenticated' },
806+
fields: ['id'],
766807
limit: 1,
767808
});
768809

@@ -782,11 +823,13 @@ async function ensureContentApiPermissions(strapi, log) {
782823
'plugin::magic-sessionmanager.session.terminateOwnSession',
783824
];
784825

785-
const existingPermissions = await strapi.entityService.findMany(PERMISSION_UID, {
826+
const existingPermissions = await strapi.documents(PERMISSION_UID).findMany({
786827
filters: {
787828
role: authenticatedRole.id,
788829
action: { $in: requiredActions },
789830
},
831+
fields: ['action'],
832+
limit: requiredActions.length,
790833
});
791834

792835
const existingActions = existingPermissions.map(p => p.action);
@@ -799,7 +842,7 @@ async function ensureContentApiPermissions(strapi, log) {
799842
}
800843

801844
for (const action of missingActions) {
802-
await strapi.entityService.create(PERMISSION_UID, {
845+
await strapi.documents(PERMISSION_UID).create({
803846
data: { action, role: authenticatedRole.id },
804847
});
805848
log.info(`[PERMISSION] Enabled ${action} for authenticated users`);
@@ -935,15 +978,25 @@ async function registerSessionAwareAuthStrategy(strapi, log) {
935978
try {
936979
const usersPermissionsPlugin = strapi.plugin('users-permissions');
937980

981+
// These two conditions are both FATAL for the plugin's purpose: without a
982+
// wrappable JWT verify, we cannot enforce server-side session revocation.
983+
// We still return instead of throwing so the rest of the plugin (admin
984+
// UI, settings) keeps working, but we log at ERROR so ops notices.
938985
if (!usersPermissionsPlugin) {
939-
strapi.log.warn('[magic-sessionmanager] [AUTH] users-permissions plugin not found');
986+
strapi.log.error(
987+
'[magic-sessionmanager] [AUTH] users-permissions plugin not found — ' +
988+
'session revocation will NOT be enforced. Install @strapi/plugin-users-permissions.'
989+
);
940990
return;
941991
}
942992

943993
const jwtService = usersPermissionsPlugin.service('jwt');
944994

945995
if (!jwtService || !jwtService.verify) {
946-
strapi.log.warn('[magic-sessionmanager] [AUTH] JWT service not found or no verify method');
996+
strapi.log.error(
997+
'[magic-sessionmanager] [AUTH] users-permissions JWT service has no .verify method — ' +
998+
'API surface changed. Session revocation will NOT be enforced until the plugin is updated.'
999+
);
9471000
return;
9481001
}
9491002

@@ -972,6 +1025,17 @@ async function registerSessionAwareAuthStrategy(strapi, log) {
9721025

9731026
const strictMode = settings.strictSessionEnforcement === true;
9741027
const maxSessionAgeDays = settings.maxSessionAgeDays || 30;
1028+
// Grace period: the login interceptor writes the Session record AFTER
1029+
// ctx.body has already left the server. A client that fires its next
1030+
// authenticated request within a few hundred ms may beat the write.
1031+
// Without this window, strictSessionEnforcement would reject the JWT
1032+
// as "no matching session" even though a session is being created.
1033+
// Configurable; defaults to 5s which comfortably covers DB commit +
1034+
// network RTT on most stacks.
1035+
const gracePeriodMs = Math.max(
1036+
0,
1037+
Number(settings.sessionCreationGraceMs) || 5000
1038+
);
9751039

9761040
try {
9771041
const userDocId = await resolveUserDocumentId(strapi, decoded.id);
@@ -1030,6 +1094,29 @@ async function registerSessionAwareAuthStrategy(strapi, log) {
10301094
return decoded;
10311095
}
10321096

1097+
// Inactive-but-not-manually-terminated sessions can be reactivated,
1098+
// but ONLY if the inactivity window has not elapsed. Otherwise the
1099+
// cleanup-job's purpose (idle logout) would be silently defeated
1100+
// every time a stale JWT is reused.
1101+
const inactivityTimeout = settings.inactivityTimeout || 15 * 60 * 1000;
1102+
const lastActiveMs = thisSession.lastActive
1103+
? new Date(thisSession.lastActive).getTime()
1104+
: thisSession.loginTime
1105+
? new Date(thisSession.loginTime).getTime()
1106+
: 0;
1107+
const idleFor = Date.now() - lastActiveMs;
1108+
1109+
if (idleFor > inactivityTimeout) {
1110+
strapi.log.info(
1111+
`[magic-sessionmanager] [JWT-IDLE] Session too idle to reactivate (${Math.round(idleFor / 1000)}s > ${inactivityTimeout / 1000}s) for user ${userDocId.substring(0, 8)}...`
1112+
);
1113+
await strapi.documents(SESSION_UID).update({
1114+
documentId: thisSession.documentId,
1115+
data: { terminatedManually: true, logoutTime: new Date() },
1116+
});
1117+
return null;
1118+
}
1119+
10331120
await strapi.documents(SESSION_UID).update({
10341121
documentId: thisSession.documentId,
10351122
data: { isActive: true, lastActive: new Date() },
@@ -1041,6 +1128,22 @@ async function registerSessionAwareAuthStrategy(strapi, log) {
10411128
return decoded;
10421129
}
10431130

1131+
// JWT has no matching session record. Before blocking, honor the
1132+
// grace period: if the JWT was issued within the last `gracePeriodMs`
1133+
// the login interceptor's DB write may simply not be visible yet.
1134+
// `iat` is seconds-since-epoch per RFC 7519.
1135+
if (gracePeriodMs > 0 && typeof decoded.iat === 'number') {
1136+
const issuedMs = decoded.iat * 1000;
1137+
const ageMs = Date.now() - issuedMs;
1138+
if (ageMs >= 0 && ageMs < gracePeriodMs) {
1139+
strapi.log.debug(
1140+
`[magic-sessionmanager] [JWT-GRACE] New JWT (age ${ageMs}ms) inside grace window — allowing`
1141+
);
1142+
resetErrorCounter();
1143+
return decoded;
1144+
}
1145+
}
1146+
10441147
if (strictMode) {
10451148
strapi.log.info(
10461149
`[magic-sessionmanager] [JWT-BLOCKED] No session matches this token (user: ${userDocId.substring(0, 8)}..., strictMode)`

server/src/middlewares/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@
22

33
module.exports = {
44
'last-seen': require('./last-seen'),
5+
'rate-limit': require('./rate-limit'),
56
};
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
'use strict';
2+
3+
/**
4+
* In-memory per-caller rate limiter for session-manager Content-API endpoints.
5+
*
6+
* The primary purpose is to contain abuse of /logout and /logout-all from
7+
* a compromised JWT: while those endpoints are "soft" (they mark the DB
8+
* row) they still do a write per call and an attacker could otherwise
9+
* drown the DB by spamming them.
10+
*
11+
* Single-process only — see middlewares/last-seen.js comment for the
12+
* multi-instance trade-off and the preferred Redis-backed upgrade path.
13+
*
14+
* Route-level config shape:
15+
* { name: 'plugin::magic-sessionmanager.rate-limit',
16+
* config: { max: 20, window: 60000 } }
17+
*/
18+
19+
const buckets = new Map();
20+
21+
const prune = (now) => {
22+
for (const [key, entry] of buckets) {
23+
if (entry.expiresAt <= now) buckets.delete(key);
24+
}
25+
};
26+
27+
/**
28+
* Returns a stable key identifying the caller: user id when authenticated,
29+
* API-token id otherwise, then the socket IP as last resort.
30+
* @param {object} ctx
31+
* @returns {string}
32+
*/
33+
const callerKey = (ctx) => {
34+
const userId = ctx.state?.user?.id;
35+
if (userId) return `u:${userId}`;
36+
const tokenId =
37+
ctx.state?.auth?.credentials?.id ??
38+
ctx.state?.auth?.credentials?.token ??
39+
null;
40+
if (tokenId) return `t:${String(tokenId).slice(-16)}`;
41+
return `ip:${ctx.request.ip || ctx.ip || 'unknown'}`;
42+
};
43+
44+
/**
45+
* @param {{ max?: number, window?: number }} cfg
46+
*/
47+
const rateLimit = (cfg = {}, { strapi }) => {
48+
const max = Number.isFinite(cfg.max) ? cfg.max : 30;
49+
const windowMs = Number.isFinite(cfg.window) ? cfg.window : 60_000;
50+
51+
return async (ctx, next) => {
52+
const key = `${ctx.path}::${callerKey(ctx)}`;
53+
const now = Date.now();
54+
55+
if (buckets.size > 5000) prune(now);
56+
57+
let entry = buckets.get(key);
58+
if (!entry || entry.expiresAt <= now) {
59+
entry = { count: 0, expiresAt: now + windowMs };
60+
buckets.set(key, entry);
61+
}
62+
63+
entry.count += 1;
64+
65+
if (entry.count > max) {
66+
const retryAfterSec = Math.ceil((entry.expiresAt - now) / 1000);
67+
ctx.set('Retry-After', String(retryAfterSec));
68+
strapi.log.warn(
69+
`[magic-sessionmanager] Rate limit exceeded on ${ctx.path} for ${callerKey(ctx)} (${entry.count}/${max})`
70+
);
71+
ctx.status = 429;
72+
ctx.body = {
73+
data: null,
74+
error: {
75+
status: 429,
76+
name: 'TooManyRequestsError',
77+
message: 'Too many requests. Please slow down.',
78+
details: { retryAfter: retryAfterSec },
79+
},
80+
};
81+
return;
82+
}
83+
84+
await next();
85+
};
86+
};
87+
88+
module.exports = rateLimit;

server/src/register.js

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,20 +48,26 @@ module.exports = async ({ strapi }) => {
4848
models: [USER_UID],
4949

5050
/**
51-
* Terminates the user's active sessions when they are blocked.
51+
* Terminates the user's active sessions the MOMENT they are blocked.
5252
*
53-
* This runs on EVERY update of a blocked user, but session termination
54-
* is idempotent — already-terminated sessions are skipped by the
55-
* filter `isActive: true`. We also only act when `blocked === true`,
56-
* so unblocking never re-terminates anything.
53+
* We only fire on the transition to `blocked: true` — i.e. when the
54+
* update's params.data actually flips the field. This avoids running
55+
* a session-scan query on every unrelated user update (email change,
56+
* profile picture, …) which at scale would be a real load problem.
5757
*
5858
* @param {object} event
5959
*/
6060
async afterUpdate(event) {
6161
try {
62-
const { result } = event;
62+
const { result, params } = event;
6363
if (!result || result.blocked !== true) return;
6464

65+
// Only act when THIS update actually set blocked=true. Without
66+
// this check, every update of an already-blocked user would
67+
// re-run the session scan for no gain.
68+
const data = params && params.data ? params.data : null;
69+
if (!data || data.blocked !== true) return;
70+
6571
const userDocId = result.documentId;
6672
if (!userDocId) return;
6773

0 commit comments

Comments
 (0)