Skip to content

fix(auth,orgs,audit): replace silent .catch(() => {}) with error logging #3366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PierreBrisorgueil merged 3 commits into from
Mar 31, 2026
Merged

fix(auth,orgs,audit): replace silent .catch(() => {}) with error logging #3366

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d649bf
fix(auth,orgs,audit): replace silent .catch(() => {}) with error logging
PierreBrisorgueil Mar 31, 2026
67269ac
test(auth,orgs): add unit tests for error logging on silent-catch paths
PierreBrisorgueil Mar 31, 2026
ebad7a3
fix(auth,orgs,audit): use serializable error format in logger calls
PierreBrisorgueil Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/services/tests/analytics.identify.unit.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ describe('Analytics identify on auth events:', () => {
jest.unstable_mockModule('../../helpers/getBaseUrl.js', () => ({
default: jest.fn().mockReturnValue('http://localhost:3000'),
}));

jest.unstable_mockModule('../logger.js', () => ({
default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
}));
};

test('should call AnalyticsService.identify after successful signup', async () => {
Expand Down Expand Up @@ -260,6 +264,10 @@ describe('Analytics identify on auth events:', () => {
default: jest.fn().mockReturnValue('http://localhost:3000'),
}));

jest.unstable_mockModule('../logger.js', () => ({
default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
}));

const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js');

const req = { user: mockUser };
Expand Down
3 changes: 2 additions & 1 deletion modules/audit/middlewares/audit.middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Module dependencies
*/
import AuditService from '../services/audit.service.js';
import logger from '../../../lib/services/logger.js';

/**
* Default route prefixes to skip when auto-capturing audit events.
Expand Down Expand Up @@ -126,7 +127,7 @@ const createAuditMiddleware = (options = {}) => {
req,
targetType,
targetId,
}).catch(() => {});
}).catch((err) => logger.error('audit.middleware: audit log write failed', err));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AuditService.log() already wraps AuditRepository.create() in a try/catch and never rejects (it logs and returns null on failure). That means this .catch(...) will never run, and the extra logger import is effectively dead code. Consider removing this .catch (and the logger import) and relying on AuditService.log()'s internal logging, or adjust AuditService.log() to rethrow if you want the middleware-level catch to be meaningful.

Copilot uses AI. Check for mistakes.
});

return next();
Expand Down
3 changes: 2 additions & 1 deletion modules/auth/controllers/auth.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import AuthOrganizationService from '../../organizations/services/organizations.
import OrganizationCrudService from '../../organizations/services/organizations.crud.service.js';
import MembershipService from '../../organizations/services/organizations.membership.service.js';
import AnalyticsService from '../../../lib/services/analytics.js';
import logger from '../../../lib/services/logger.js';

const tokenCookieOptions = {
httpOnly: true,
Expand Down Expand Up @@ -79,7 +80,7 @@ const signup = async (req, res) => {
emailVerificationExpires: Date.now() + 24 * 3600000, // 24 hours
}, 'recover');
// Send verification email (best-effort, do not block signup)
sendVerificationEmail(user, verificationToken).catch(() => {});
sendVerificationEmail(user, verificationToken).catch((err) => logger.warn('auth.signup: verification email failed', err));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Production config enables JSON logging, and the logger uses winston.format.json() without format.errors(), so passing an Error as the second argument may serialize to {} (dropping message/stack). Consider logging { message: err?.message, stack: err?.stack } (or other serializable metadata) so verification email failures are diagnosable.

Suggested change
sendVerificationEmail(user, verificationToken).catch((err) => logger.warn('auth.signup: verification email failed', err));
sendVerificationEmail(user, verificationToken).catch((err) => logger.warn(
'auth.signup: verification email failed',
{
message: err && err.message,
stack: err && err.stack,
name: err && err.name,
},
));

Copilot uses AI. Check for mistakes.
} else {
// No mailer configured — auto-verify so dev/test are not blocked
const brutUser = await UserService.getBrut({ id: user.id });
Expand Down
3 changes: 2 additions & 1 deletion modules/auth/controllers/auth.password.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import getBaseUrl from '../../../lib/helpers/getBaseUrl.js';
import errors from '../../../lib/helpers/errors.js';
import responses from '../../../lib/helpers/responses.js';
import config from '../../../config/index.js';
import logger from '../../../lib/services/logger.js';

const tokenCookieOptions = {
httpOnly: true,
Expand Down Expand Up @@ -108,7 +109,7 @@ const reset = async (req, res) => {
appName: config.app.title,
appContact: config.app.contact,
},
}).catch(() => {});
}).catch((err) => logger.warn('auth.password.reset: confirmation email failed', err));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same JSON-logging issue: passing err directly as metadata may not include message/stack in production logs. Prefer serializable error fields (e.g. { message: err?.message, stack: err?.stack }) so the warning is actionable.

Suggested change
}).catch((err) => logger.warn('auth.password.reset: confirmation email failed', err));
}).catch((err) => logger.warn('auth.password.reset: confirmation email failed', { error: { message: err?.message, stack: err?.stack } }));

Copilot uses AI. Check for mistakes.
return res
.status(200)
.cookie('TOKEN', jwt.sign({ userId: user.id }, config.jwt.secret, { expiresIn: config.jwt.expiresIn }), tokenCookieOptions)
Expand Down
5 changes: 3 additions & 2 deletions modules/organizations/services/organizations.crud.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import AppError from '../../../lib/helpers/AppError.js';
import { assertEmailVerified } from '../../../lib/helpers/emailVerification.js';
import config from '../../../config/index.js';
import logger from '../../../lib/services/logger.js';

/**
* @desc Escape regex-special characters in a user-provided string.
Expand Down Expand Up @@ -112,8 +113,8 @@ const create = async (body, user) => {
await UserService.updateById(user.id || user._id, { currentOrganization: result._id });
} catch (err) {
// Rollback partially created artifacts
if (membership) await MembershipRepository.deleteMany({ _id: membership._id }).catch(() => {});
await OrganizationsRepository.remove(result).catch(() => {});
if (membership) await MembershipRepository.deleteMany({ _id: membership._id }).catch((e) => logger.error('organizations.crud.create: rollback membership failed', e));
await OrganizationsRepository.remove(result).catch((e) => logger.error('organizations.crud.create: rollback organization failed', e));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as other new logger calls: with JSON logging enabled in production, passing an Error object directly as metadata often results in {} and drops message/stack. Prefer logging serializable fields (e.g. { message: e?.message, stack: e?.stack }) so rollback failures are diagnosable from logs.

Suggested change
if (membership) await MembershipRepository.deleteMany({ _id: membership._id }).catch((e) => logger.error('organizations.crud.create: rollback membership failed', e));
await OrganizationsRepository.remove(result).catch((e) => logger.error('organizations.crud.create: rollback organization failed', e));
if (membership) {
await MembershipRepository.deleteMany({ _id: membership._id }).catch((e) =>
logger.error('organizations.crud.create: rollback membership failed', {
message: e?.message,
stack: e?.stack,
}),
);
}
await OrganizationsRepository.remove(result).catch((e) =>
logger.error('organizations.crud.create: rollback organization failed', {
message: e?.message,
stack: e?.stack,
}),
);

Copilot uses AI. Check for mistakes.
throw err;
}

Expand Down
7 changes: 4 additions & 3 deletions modules/organizations/services/organizations.membership.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import crypto from 'crypto';

import config from '../../../config/index.js';
import logger from '../../../lib/services/logger.js';
import getBaseUrl from '../../../lib/helpers/getBaseUrl.js';
import mailer from '../../../lib/helpers/mailer/index.js';
import { assertEmailVerified } from '../../../lib/helpers/emailVerification.js';
Expand Down Expand Up @@ -164,7 +165,7 @@ const createJoinRequest = async (userId, organizationId) => {
url: `${getBaseUrl()}/users/organizations/${organizationId}`,
appName: config.app.title,
},
}).catch(() => {});
}).catch((err) => logger.warn('organizations.membership.createJoinRequest: admin notification email failed', err));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Winston JSON logging enabled in production, passing the raw err object as the second argument may serialize as {} and omit the message/stack. Consider logging { message: err?.message, stack: err?.stack } (or similar serializable metadata) so email failures are actionable.

Suggested change
}).catch((err) => logger.warn('organizations.membership.createJoinRequest: admin notification email failed', err));
}).catch((err) =>
logger.warn(
'organizations.membership.createJoinRequest: admin notification email failed',
{ message: err?.message, stack: err?.stack }
)
);

Copilot uses AI. Check for mistakes.
}
}
}
Expand Down Expand Up @@ -206,7 +207,7 @@ const approveRequest = async (membership) => {
orgName: org.name,
appName: config.app.title,
},
}).catch(() => {});
}).catch((err) => logger.warn('organizations.membership.approveRequest: approval email failed', err));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same JSON-logging concern: logging the raw Error object as metadata may not include message/stack. Prefer serializable fields (e.g. { message: err?.message, stack: err?.stack }) so the warning is useful in production logs.

Copilot uses AI. Check for mistakes.
}
}

Expand Down Expand Up @@ -235,7 +236,7 @@ const rejectRequest = async (membership) => {
orgName: org.name,
appName: config.app.title,
},
}).catch(() => {});
}).catch((err) => logger.warn('organizations.membership.rejectRequest: rejection email failed', err));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same JSON-logging concern here: passing err directly may serialize to {} under winston.format.json(). Prefer logging serializable error details (message/stack) so this warning remains informative in production.

Suggested change
}).catch((err) => logger.warn('organizations.membership.rejectRequest: rejection email failed', err));
}).catch((err) =>
logger.warn(
'organizations.membership.rejectRequest: rejection email failed',
{
error: {
message: err && err.message,
stack: err && err.stack,
name: err && err.name,
},
},
),
);

Copilot uses AI. Check for mistakes.
}
}
return MembershipRepository.remove(membership);
Expand Down
5 changes: 3 additions & 2 deletions modules/organizations/services/organizations.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import config from '../../../config/index.js';
import mailer from '../../../lib/helpers/mailer/index.js';
import logger from '../../../lib/services/logger.js';
import policy from '../../../lib/middlewares/policy.js';
import serializeAbilities from '../../../lib/helpers/abilities.js';
import OrganizationsRepository from '../repositories/organizations.repository.js';
Expand Down Expand Up @@ -98,10 +99,10 @@ const createOrganizationForUser = async ({ name, slug, domain, user, slugGenerat
} catch (err) {
// Clean up any partially created artifacts to avoid orphaned records
if (membership) {
await MembershipRepository.deleteMany({ _id: membership._id }).catch(() => {});
await MembershipRepository.deleteMany({ _id: membership._id }).catch((e) => logger.error('organizations.service.createOrganizationForUser: rollback membership failed', e));
}
if (organization) {
await OrganizationsRepository.remove(organization).catch(() => {});
await OrganizationsRepository.remove(organization).catch((e) => logger.error('organizations.service.createOrganizationForUser: rollback organization failed', e));

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In production config.log.json is true (config/defaults/production.config.js), and the logger uses winston.format.json() without format.errors(). Passing an Error as the second argument will typically serialize to {} in JSON logs, so you'll lose the error message/stack. Consider logging a plain object (e.g. { message: e?.message, stack: e?.stack }) or otherwise ensure the error is serialized.

Copilot uses AI. Check for mistakes.
}
// Retry on MongoDB duplicate key error for slug collisions (TOCTOU race)
if (err.code === 11000 && err.message?.includes('slug') && attempt < maxRetries - 1) {
Expand Down
Loading