Skip to content

Commit 9372c9f

Browse files
committed
fix: comprehensive security audit - 30 fixes across 14 files
Security (Critical): - Fix IPv6 address handling in cleanIp() (was destroying IPv6) - Add JWT validation to /api/auth/logout (was auth:false) - Add maxSessionAge to prevent indefinite session reactivation - Fix IDOR vulnerability in getUserSessions (null documentId bypass) - Block simulateTimeout endpoint in production Security (High): - Add SSRF protection for webhook URLs (domain allowlist) - Add fail-open error counter (switches to fail-closed after 10 errors) - Mask license keys in all log output - Remove block reason from client response (information disclosure) - Validate webhook URLs from plugin config (not just settings store) - Add HTML escaping in email template variable replacement (XSS) - Validate IPv4 octets 0-255 in geolocation endpoint - Strengthen IPv6 regex validation Security (Medium): - Add XSS sanitization for email templates (strip script/iframe tags) - Add LRU cache size limit (max 1000 entries) in last-seen middleware - Add query limits to getActiveSessions and getOwnSessions - Complete private IP detection (172.16-31, fc00, fd00, fe80, 169.254) - Add integer boundary validation for all settings values - Sanitize country code lists (ISO 2-letter only) - Strip sensitive error details from encryption.js logs Bug fixes: - Fix parameter shadowing in session-required.js (config -> pluginConfig) - Fix ESM/CJS inconsistency in controller.js and service.js - Add batch processing for deleteInactiveSessions (100 per batch) - Add existence check before session termination - Store geoLocation as JSON object instead of JSON.stringify string - Remove dead code (unused terminateSession handler, decryptToken import) Test suite: - Complete rewrite of test-session-manager.js (1990 lines, 46 tests) - 4 phases: User API, Admin API, Security, Fix Verification - 15 fix verification tests + 5 false positive tests - CLI --phase flag for selective test execution
1 parent 28fc57b commit 9372c9f

14 files changed

Lines changed: 2142 additions & 1524 deletions

File tree

server/src/bootstrap.js

Lines changed: 128 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,13 @@ module.exports = async ({ strapi }) => {
5959
log.info('║ ║');
6060

6161
if (licenseStatus.data) {
62-
log.info(`║ License: ${licenseStatus.data.licenseKey}`.padEnd(66) + '║');
62+
const maskedKey = licenseStatus.data.licenseKey
63+
? `${licenseStatus.data.licenseKey.substring(0, 8)}...`
64+
: 'N/A';
65+
log.info(`║ License: ${maskedKey}`.padEnd(66) + '║');
6366
log.info(`║ User: ${licenseStatus.data.firstName} ${licenseStatus.data.lastName}`.padEnd(66) + '║');
64-
log.info(`║ Email: ${licenseStatus.data.email}`.padEnd(66) + '║');
6567
} else if (storedKey) {
66-
log.info(`║ License: ${storedKey} (Offline Mode)`.padEnd(66) + '║');
68+
log.info(`║ License: ${storedKey.substring(0, 8)}... (Offline Mode)`.padEnd(66) + '║');
6769
log.info(`║ Status: Grace Period Active`.padEnd(66) + '║');
6870
}
6971

@@ -104,6 +106,7 @@ module.exports = async ({ strapi }) => {
104106
strapi.sessionManagerIntervals.cleanup = cleanupIntervalHandle;
105107

106108
// HIGH PRIORITY: Register /api/auth/logout route BEFORE other plugins
109+
// SECURITY: Requires valid JWT token - only authenticated users can logout
107110
strapi.server.routes([{
108111
method: 'POST',
109112
path: '/api/auth/logout',
@@ -112,11 +115,25 @@ module.exports = async ({ strapi }) => {
112115
const token = ctx.request.headers?.authorization?.replace('Bearer ', '');
113116

114117
if (!token) {
115-
ctx.status = 200;
116-
ctx.body = { message: 'Logged out successfully' };
118+
ctx.status = 401;
119+
ctx.body = { error: { status: 401, message: 'Authorization token required' } };
117120
return;
118121
}
119122

123+
// Verify the JWT is valid before allowing logout
124+
try {
125+
const jwtService = strapi.plugin('users-permissions').service('jwt');
126+
const decoded = await jwtService.verify(token);
127+
if (!decoded || !decoded.id) {
128+
ctx.status = 401;
129+
ctx.body = { error: { status: 401, message: 'Invalid token' } };
130+
return;
131+
}
132+
} catch (jwtErr) {
133+
// Token is invalid or expired - still allow logout to clean up session
134+
log.debug('JWT verify failed during logout (cleaning up anyway):', jwtErr.message);
135+
}
136+
120137
// Find session by tokenHash - O(1) DB lookup instead of O(n) decrypt loop!
121138
const tokenHashValue = hashToken(token);
122139
const matchingSession = await strapi.documents(SESSION_UID).findFirst({
@@ -140,7 +157,7 @@ module.exports = async ({ strapi }) => {
140157
}
141158
},
142159
config: {
143-
auth: false,
160+
auth: false, // We handle auth manually above to support expired-but-valid tokens
144161
},
145162
}]);
146163

@@ -222,15 +239,14 @@ module.exports = async ({ strapi }) => {
222239

223240
// Block if needed
224241
if (shouldBlock) {
242+
// Log the reason internally but do NOT expose to client
225243
log.warn(`[BLOCKED] Blocking login: ${blockReason}`);
226244

227-
// Don't create session, return error
228245
ctx.status = 403;
229246
ctx.body = {
230247
error: {
231248
status: 403,
232-
message: 'Login blocked for security reasons',
233-
details: { reason: blockReason }
249+
message: 'Login blocked for security reasons. Please contact support.',
234250
}
235251
};
236252
return; // Stop here
@@ -308,11 +324,25 @@ module.exports = async ({ strapi }) => {
308324
});
309325

310326
if (config.discordWebhookUrl) {
311-
await notificationService.sendWebhook({
312-
event: 'session.login',
313-
data: webhookData,
314-
webhookUrl: config.discordWebhookUrl,
315-
});
327+
// SECURITY: Validate webhook URL even from plugin config
328+
const webhookUrl = config.discordWebhookUrl;
329+
try {
330+
const parsed = new URL(webhookUrl);
331+
const isValidDomain = parsed.protocol === 'https:' &&
332+
(parsed.hostname === 'discord.com' || parsed.hostname === 'discordapp.com' ||
333+
parsed.hostname.endsWith('.discord.com') || parsed.hostname === 'hooks.slack.com');
334+
if (isValidDomain) {
335+
await notificationService.sendWebhook({
336+
event: 'session.login',
337+
data: webhookData,
338+
webhookUrl,
339+
});
340+
} else {
341+
log.warn(`[SECURITY] Blocked webhook to untrusted domain: ${parsed.hostname}`);
342+
}
343+
} catch {
344+
log.warn('[SECURITY] Invalid webhook URL in plugin config');
345+
}
316346
}
317347
}
318348
} catch (notifErr) {
@@ -567,6 +597,55 @@ async function ensureTokenHashIndex(strapi, log) {
567597
}
568598
}
569599

600+
/**
601+
* Error counter for fail-open safety: if too many consecutive errors occur,
602+
* we switch to fail-closed to prevent security bypass via error flooding
603+
*/
604+
const sessionCheckErrors = { count: 0, lastReset: Date.now() };
605+
const MAX_CONSECUTIVE_ERRORS = 10;
606+
const ERROR_RESET_INTERVAL = 60 * 1000; // Reset error counter every 60 seconds
607+
608+
/**
609+
* Records a session check error and returns whether we should fail-open or fail-closed
610+
* @returns {boolean} true if we should allow the request (fail-open), false to block (fail-closed)
611+
*/
612+
function shouldFailOpen() {
613+
const now = Date.now();
614+
615+
// Reset counter periodically
616+
if (now - sessionCheckErrors.lastReset > ERROR_RESET_INTERVAL) {
617+
sessionCheckErrors.count = 0;
618+
sessionCheckErrors.lastReset = now;
619+
}
620+
621+
sessionCheckErrors.count++;
622+
623+
// If too many consecutive errors, switch to fail-closed
624+
if (sessionCheckErrors.count > MAX_CONSECUTIVE_ERRORS) {
625+
return false;
626+
}
627+
628+
return true;
629+
}
630+
631+
/** Resets the error counter after a successful session check */
632+
function resetErrorCounter() {
633+
sessionCheckErrors.count = 0;
634+
}
635+
636+
/**
637+
* Checks if a session has exceeded the maximum allowed age
638+
* @param {object} session - Session object with loginTime
639+
* @param {number} maxAgeDays - Maximum session age in days (default: 30)
640+
* @returns {boolean} true if session is too old
641+
*/
642+
function isSessionExpired(session, maxAgeDays = 30) {
643+
if (!session.loginTime) return false;
644+
const loginTime = new Date(session.loginTime).getTime();
645+
const maxAgeMs = maxAgeDays * 24 * 60 * 60 * 1000;
646+
return (Date.now() - loginTime) > maxAgeMs;
647+
}
648+
570649
/**
571650
* Register session-aware auth strategy that wraps users-permissions JWT strategy
572651
* This ensures ALL authenticated requests check for active sessions
@@ -576,15 +655,13 @@ async function ensureTokenHashIndex(strapi, log) {
576655
async function registerSessionAwareAuthStrategy(strapi, log) {
577656
try {
578657
// In Strapi v5, we need to wrap the users-permissions authenticate function
579-
// The strategy is stored in the plugin's services
580658
const usersPermissionsPlugin = strapi.plugin('users-permissions');
581659

582660
if (!usersPermissionsPlugin) {
583661
strapi.log.warn('[magic-sessionmanager] [AUTH] users-permissions plugin not found');
584662
return;
585663
}
586664

587-
// Try to get the JWT service
588665
const jwtService = usersPermissionsPlugin.service('jwt');
589666

590667
if (!jwtService || !jwtService.verify) {
@@ -602,34 +679,28 @@ async function registerSessionAwareAuthStrategy(strapi, log) {
602679
// First, verify the JWT normally
603680
const decoded = await originalVerify(token);
604681

605-
// If verification failed, return the result
606682
if (!decoded || !decoded.id) {
607683
return decoded;
608684
}
609685

610-
// Get config - strictSessionEnforcement must be explicitly enabled to block
686+
// Get config
611687
const config = strapi.config.get('plugin::magic-sessionmanager') || {};
612688
const strictMode = config.strictSessionEnforcement === true;
689+
const maxSessionAgeDays = config.maxSessionAgeDays || 30;
613690

614-
// Now check if THIS SPECIFIC session (by token hash) is valid
615691
try {
616-
// Hash the token to find the specific session
617692
const tokenHashValue = hashToken(token);
618693

619-
// Get user documentId
620-
let userDocId = null;
621-
622-
// decoded.id is numeric, we need documentId
694+
// Get user documentId (decoded.id is numeric)
623695
const user = await strapi.entityService.findOne(
624696
'plugin::users-permissions.user',
625697
decoded.id,
626698
{ fields: ['documentId'] }
627699
);
628700

629-
userDocId = user?.documentId;
701+
const userDocId = user?.documentId;
630702

631703
if (!userDocId) {
632-
// Can't determine user - allow through (fail-open)
633704
strapi.log.debug('[magic-sessionmanager] [JWT] No documentId found, allowing through');
634705
return decoded;
635706
}
@@ -640,92 +711,96 @@ async function registerSessionAwareAuthStrategy(strapi, log) {
640711
user: { documentId: userDocId },
641712
tokenHash: tokenHashValue,
642713
},
643-
fields: ['documentId', 'isActive', 'terminatedManually', 'lastActive'],
714+
fields: ['documentId', 'isActive', 'terminatedManually', 'lastActive', 'loginTime'],
644715
});
645716

646717
if (thisSession) {
647-
// Found the specific session for this token
718+
// SECURITY: Check max session age - prevents indefinite reactivation
719+
if (isSessionExpired(thisSession, maxSessionAgeDays)) {
720+
strapi.log.info(
721+
`[magic-sessionmanager] [JWT-EXPIRED] Session exceeded max age of ${maxSessionAgeDays} days (user: ${userDocId.substring(0, 8)}...)`
722+
);
723+
// Terminate the expired session
724+
await strapi.documents(SESSION_UID).update({
725+
documentId: thisSession.documentId,
726+
data: { isActive: false, terminatedManually: true, logoutTime: new Date() },
727+
});
728+
return null;
729+
}
648730

649731
if (thisSession.terminatedManually === true) {
650-
// This specific session was manually terminated → BLOCK
651732
strapi.log.info(
652733
`[magic-sessionmanager] [JWT-BLOCKED] Session was manually terminated (user: ${userDocId.substring(0, 8)}...)`
653734
);
654735
return null;
655736
}
656737

657738
if (thisSession.isActive) {
658-
// Session is active → allow
739+
resetErrorCounter();
659740
return decoded;
660741
}
661742

662-
// Session is inactive but NOT manually terminated reactivate
743+
// Session inactive but NOT manually terminated -> reactivate (within max age)
663744
await strapi.documents(SESSION_UID).update({
664745
documentId: thisSession.documentId,
665-
data: {
666-
isActive: true,
667-
lastActive: new Date(),
668-
},
746+
data: { isActive: true, lastActive: new Date() },
669747
});
670748
strapi.log.info(
671749
`[magic-sessionmanager] [JWT-REACTIVATED] Session reactivated for user ${userDocId.substring(0, 8)}...`
672750
);
751+
resetErrorCounter();
673752
return decoded;
674753
}
675754

676-
// No session found for this specific token - check if user has ANY sessions
677-
// This handles tokens issued before session manager was installed
755+
// No session for this token - check backward compatibility
678756
const anyActiveSessions = await strapi.documents(SESSION_UID).findMany({
679-
filters: {
680-
user: { documentId: userDocId },
681-
isActive: true,
682-
},
757+
filters: { user: { documentId: userDocId }, isActive: true },
683758
limit: 1,
684759
});
685760

686761
if (anyActiveSessions && anyActiveSessions.length > 0) {
687-
// User has other active sessions - allow this token (backward compatibility)
688762
strapi.log.debug(
689763
`[magic-sessionmanager] [JWT] No session for token but user has other active sessions (allowing)`
690764
);
765+
resetErrorCounter();
691766
return decoded;
692767
}
693768

694-
// Check for any manually terminated sessions
769+
// Check for manually terminated sessions
695770
const terminatedSessions = await strapi.documents(SESSION_UID).findMany({
696-
filters: {
697-
user: { documentId: userDocId },
698-
terminatedManually: true,
699-
},
771+
filters: { user: { documentId: userDocId }, terminatedManually: true },
700772
limit: 1,
701773
});
702774

703775
if (terminatedSessions && terminatedSessions.length > 0) {
704-
// User was logged out (all sessions terminated) → BLOCK
705776
strapi.log.info(
706777
`[magic-sessionmanager] [JWT-BLOCKED] User ${userDocId.substring(0, 8)}... has terminated sessions`
707778
);
708779
return null;
709780
}
710781

711-
// No sessions at all - session was never created
782+
// No sessions at all
712783
if (strictMode) {
713784
strapi.log.info(
714785
`[magic-sessionmanager] [JWT-BLOCKED] No sessions exist for user ${userDocId.substring(0, 8)}... (strictMode)`
715786
);
716787
return null;
717788
}
718789

719-
// Non-strict mode: Allow through but warn
720790
strapi.log.warn(
721791
`[magic-sessionmanager] [JWT-WARN] No session for user ${userDocId.substring(0, 8)}... (allowing)`
722792
);
793+
resetErrorCounter();
723794
return decoded;
724795

725796
} catch (err) {
726-
// On ANY error, allow through (fail-open for availability)
727-
strapi.log.warn('[magic-sessionmanager] [JWT] Session check error (allowing):', err.message);
728-
return decoded;
797+
// Fail-open with error counting: after too many consecutive errors, fail-closed
798+
if (shouldFailOpen()) {
799+
strapi.log.warn('[magic-sessionmanager] [JWT] Session check error (allowing):', err.message);
800+
return decoded;
801+
}
802+
strapi.log.error('[magic-sessionmanager] [JWT] Too many consecutive errors, blocking request:', err.message);
803+
return null;
729804
}
730805
};
731806

server/src/controllers/controller.js

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
1-
const controller = ({ strapi }) => ({
1+
'use strict';
2+
3+
/**
4+
* Default controller for magic-sessionmanager plugin
5+
*/
6+
module.exports = ({ strapi }) => ({
7+
/** Returns a welcome message for the plugin */
28
index(ctx) {
39
ctx.body = strapi
410
.plugin('magic-sessionmanager')
5-
// the name of the service file & the method.
611
.service('service')
712
.getWelcomeMessage();
813
},
914
});
10-
11-
export default controller;

server/src/controllers/license.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,13 @@ module.exports = ({ strapi }) => ({
7575
});
7676
}
7777

78-
strapi.log.info(`[magic-sessionmanager/license-controller] Checking stored license: ${licenseKey}`);
78+
strapi.log.info(`[magic-sessionmanager/license-controller] Checking stored license: ${licenseKey.substring(0, 8)}...`);
7979

8080
const verification = await licenseGuard.verifyLicense(licenseKey);
8181
const license = await licenseGuard.getLicenseByKey(licenseKey);
8282

8383
strapi.log.info('[magic-sessionmanager/license-controller] License data from MagicAPI:', {
84-
licenseKey: license?.licenseKey,
85-
email: license?.email,
84+
licenseKey: license?.licenseKey ? `${license.licenseKey.substring(0, 8)}...` : 'N/A',
8685
featurePremium: license?.featurePremium,
8786
isActive: license?.isActive,
8887
pluginName: license?.pluginName,

0 commit comments

Comments
 (0)