Skip to content

fix(auth): log OAuth callback errors and drop useless error={} redirect #3506

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 1 commit into from
Apr 24, 2026
Merged

fix(auth): log OAuth callback errors and drop useless error={} redirect #3506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions modules/auth/controllers/auth.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,11 +441,19 @@ const oauthCallback = async (req, res, next) => {
passport.authenticate(strategy, (err, user) => {
const url = getBaseUrl();
if (err) {

@codacy-production codacy-production Bot Apr 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MEDIUM RISK

Suggestion: The logic for logging and redirecting on failure is duplicated across the 'err' and '!user' branches. Consolidating this logic into a single path or helper function would make the controller more maintainable.

const _err = JSON.stringify(err);
logger.error(
{ err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy },
'OAuth callback failed',
);
Comment on lines +444 to +447

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

logger.error is called with (metaObject, message) here, but the rest of the codebase uses Winston as logger.error(message, meta) (e.g. logger.error('Mail send error', err)). With the current argument order, Winston will treat the object as the message and the string as extra meta/splat, producing unhelpful output. Swap the parameters (or pass a single object with message field) so the log message is emitted correctly and consistently.

Copilot uses AI. Check for mistakes.
const _err = encodeURIComponent(err?.message || err?.code || 'oauth_error');
const path = 'token?message=Unprocessable%20Entity';
res.redirect(302, `${url}/${path}&error=${_err}`);
} else if (!user) {
const _err = JSON.stringify(err);
logger.error(
{ err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy },

@codacy-production codacy-production Bot Apr 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ LOW RISK

In this branch, 'err' is guaranteed to be falsy because it follows an 'if (err)' check. Logging 'err?.message', 'err?.code', or 'err?.stack' will result in 'undefined' values in your logs, providing no context.

Suggested fix:

Suggested change
{ err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy },
{ strategy },

'OAuth callback failed',
);
Comment on lines +452 to +455

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

Same as the err branch: logger.error is invoked as (metaObject, message), which is inconsistent with existing Winston usage in this repo and likely logs [object Object] as the message. Use logger.error('OAuth callback failed', { err: ..., strategy }) (or an { message, ... } object) instead.

Copilot uses AI. Check for mistakes.
const _err = encodeURIComponent(err?.message || err?.code || 'oauth_no_user');

@codacy-production codacy-production Bot Apr 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ LOW RISK

Suggestion: Since err is null when this line is reached, the expression err?.message || err?.code is dead code. Simplify the assignment to a constant 'oauth_no_user'.

Comment on lines 441 to +456

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

In the !user branch, err is expected to be falsy, so the logged err: { message, code, stack } will always be undefined fields and won’t help debugging. Passport provides an info argument to the authenticate callback (already used in signinAuthenticate above); consider updating the callback signature to (err, user, info) and logging/redirecting based on info?.message (falling back to oauth_no_user).

Copilot uses AI. Check for mistakes.
const path = 'token?message=Could%20not%20define%20user%20in%20oAuth';
res.redirect(302, `${url}/${path}&error=${_err}`);
} else {
Expand Down
73 changes: 73 additions & 0 deletions modules/auth/tests/auth.integration.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import passport from 'passport';
import { bootstrap } from '../../../lib/app.js';
import mongooseService from '../../../lib/services/mongoose.js';
import config from '../../../config/index.js';
import logger from '../../../lib/services/logger.js';

/**
* Unit tests
Expand Down Expand Up @@ -645,6 +646,78 @@ describe('Auth integration tests:', () => {
authenticateSpy.mockRestore();
});

test('should log and redirect with message when classic web oAuth errors out', async () => {
const oauthErr = new Error('token exchange failed');
oauthErr.code = 'OAUTH_TOKEN_EXCHANGE';
const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce(
(strategy, callback) => () => callback(oauthErr, null),
);
const loggerSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
const redirectCalls = [];
const mockReq = { params: { strategy: 'google' }, body: {} };
const mockRes = {
cookie() { return this; },
redirect(code, url) { redirectCalls.push({ code, url }); },
};

await AuthController.oauthCallback(mockReq, mockRes, () => {});

expect(loggerSpy).toHaveBeenCalledWith(
expect.objectContaining({
err: expect.objectContaining({
message: 'token exchange failed',
code: 'OAUTH_TOKEN_EXCHANGE',
stack: expect.any(String),
}),
strategy: 'google',
}),
'OAuth callback failed',
);
Comment on lines +665 to +675

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

These assertions bake in the current logger.error(meta, message) call signature. If you switch the controller to the repo’s standard logger.error(message, meta) form (recommended), update this test to assert the first argument is the string message and the second contains { err: { message, code, stack }, strategy }.

Copilot uses AI. Check for mistakes.
expect(redirectCalls[0].code).toBe(302);
// Redirect must carry the actual error message, not an empty object
expect(redirectCalls[0].url).toContain('error=');
expect(redirectCalls[0].url).not.toContain('error={}');
expect(redirectCalls[0].url).toContain(encodeURIComponent('token exchange failed'));

loggerSpy.mockRestore();
authenticateSpy.mockRestore();
});
Comment on lines +655 to +684

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

loggerSpy.mockRestore() / authenticateSpy.mockRestore() only run if the test reaches the end. If an assertion throws, these spies can leak into later tests (this describe doesn’t have an afterEach(jest.restoreAllMocks)). Use a try/finally around the expectations or add an afterEach(() => jest.restoreAllMocks()) in this describe to guarantee cleanup.

Copilot uses AI. Check for mistakes.

test('should log and redirect with sensible message when no user is returned by passport', async () => {
const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce(
(strategy, callback) => () => callback(null, null),
);
const loggerSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
const redirectCalls = [];
const mockReq = { params: { strategy: 'google' }, body: {} };
const mockRes = {
cookie() { return this; },
redirect(code, url) { redirectCalls.push({ code, url }); },
};

await AuthController.oauthCallback(mockReq, mockRes, () => {});

expect(loggerSpy).toHaveBeenCalledWith(
expect.objectContaining({
err: expect.objectContaining({
message: undefined,
code: undefined,
stack: undefined,
}),
strategy: 'google',
}),
'OAuth callback failed',
);
Comment on lines +700 to +710

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

This expectation also assumes logger.error(meta, message). Once the controller uses the standard logger.error(message, meta) signature, adjust the assertion accordingly (message first, meta second).

Copilot uses AI. Check for mistakes.
expect(redirectCalls[0].code).toBe(302);
expect(redirectCalls[0].url).toContain('error=');
expect(redirectCalls[0].url).not.toContain('error={}');
expect(redirectCalls[0].url).toContain('oauth_no_user');
expect(redirectCalls[0].url).toContain('Could%20not%20define%20user%20in%20oAuth');

loggerSpy.mockRestore();
authenticateSpy.mockRestore();
Comment on lines +698 to +718

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

Same cleanup issue as the previous test: spy restoration is not guaranteed if an assertion fails, which can cause cross-test interference. Wrap the body in try/finally (restoring spies in finally) or rely on a describe-level afterEach(() => jest.restoreAllMocks()).

Suggested change
await AuthController.oauthCallback(mockReq, mockRes, () => {});
expect(loggerSpy).toHaveBeenCalledWith(
expect.objectContaining({
err: expect.objectContaining({
message: undefined,
code: undefined,
stack: undefined,
}),
strategy: 'google',
}),
'OAuth callback failed',
);
expect(redirectCalls[0].code).toBe(302);
expect(redirectCalls[0].url).toContain('error=');
expect(redirectCalls[0].url).not.toContain('error={}');
expect(redirectCalls[0].url).toContain('oauth_no_user');
expect(redirectCalls[0].url).toContain('Could%20not%20define%20user%20in%20oAuth');
loggerSpy.mockRestore();
authenticateSpy.mockRestore();
try {
await AuthController.oauthCallback(mockReq, mockRes, () => {});
expect(loggerSpy).toHaveBeenCalledWith(
expect.objectContaining({
err: expect.objectContaining({
message: undefined,
code: undefined,
stack: undefined,
}),
strategy: 'google',
}),
'OAuth callback failed',
);
expect(redirectCalls[0].code).toBe(302);
expect(redirectCalls[0].url).toContain('error=');
expect(redirectCalls[0].url).not.toContain('error={}');
expect(redirectCalls[0].url).toContain('oauth_no_user');
expect(redirectCalls[0].url).toContain('Could%20not%20define%20user%20in%20oAuth');
} finally {
loggerSpy.mockRestore();
authenticateSpy.mockRestore();
}

Copilot uses AI. Check for mistakes.
});

test('should find an existing OAuth user via checkOAuthUserProfile', async () => {
// Create an OAuth user directly first
const createdUser = await UserService.create({
Expand Down
Loading