Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 5 additions & 37 deletions modules/auth/controllers/auth.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -636,43 +636,11 @@ const oauthErrorRedirect = (res, err, fallbackTitle) => {
*/
const oauthCallback = async (req, res, next) => {
const strategy = req.params.strategy;
// app Auth with Strategy managed on client side
if (req.body?.strategy === false && req.body?.key) {
const allowedKeys = ['id', 'sub', 'email'];
if (!allowedKeys.includes(req.body.key)) {
return responses.error(res, 422, 'Unprocessable Entity', 'Invalid provider key')();
}
try {
let user = {
firstName: req.body.firstName,
lastName: req.body.lastName,
email: req.body.email,
providerData: {},
};
user.providerData[req.body.key] = req.body.value;
user = await checkOAuthUserProfile(user, req.body.key, strategy);
const token = jwt.sign({ userId: user.id }, config.jwt.secret, {
expiresIn: config.jwt.expiresIn,
});
return res
.status(200)
.cookie('TOKEN', token, tokenCookieOptions)
.json({
user,
tokenExpiresIn: Date.now() + config.jwt.expiresIn * 1000,
type: 'success',
message: 'oAuth Ok',
});
} catch (err) {
return responses.error(
res,
422,
err instanceof AppError && err.code === 'VALIDATION_ERROR' ? errors.getMessage(err) : 'Unprocessable Entity',
errors.getMessage(err.details || err),
)(err);
}
}
// classic web oAuth
// The identity is always derived server-side from the provider's response via
// passport.authenticate(). The part that was previously unsafe was trusting a
// client-asserted identity from the request body (email, key, value) with no
// provider-token verification — a caller who knows a victim's identifier could
// have minted that victim's session. That branch is now removed entirely.
passport.authenticate(strategy, (err, user) => {
if (err) {
logger.error(
Expand Down
6 changes: 3 additions & 3 deletions modules/auth/routes/auth.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export default (app) => {
app.route('/api/auth/token').get(passport.authenticate('jwt', { session: false }), auth.token);

// Setting the oauth routes
app.route('/api/auth/:strategy').get(auth.oauthCall);
app.route('/api/auth/:strategy/callback').get(auth.oauthCallback);
app.route('/api/auth/:strategy/callback').post(auth.oauthCallback); // specific for apple call back
app.route('/api/auth/:strategy').get(authLimiter, auth.oauthCall);
app.route('/api/auth/:strategy/callback').get(authLimiter, auth.oauthCallback);
app.route('/api/auth/:strategy/callback').post(authLimiter, auth.oauthCallback); // specific for apple call back
};
61 changes: 27 additions & 34 deletions modules/auth/tests/auth.integration.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -571,47 +571,40 @@ describe('Auth integration tests:', () => {
createSpy.mockRestore();
});

test('should authenticate via client-side OAuth and set tokenCookieOptions on response', async () => {
const oauthEmail = 'oauthcb-appauth@test.com';
try {
test('should NOT mint a session from a client-asserted identity on the OAuth callback', async () => {
// Security regression: the callback must never trust a client-supplied
// identity payload. A request that claims to be a known account by email
// (no server-side provider-token verification) must be rejected, not
// turned into a victim session. The callback always delegates to passport.
const victimEmail = 'oauthcb-victim@test.com';
let victim;
try {
const existing = await UserService.getBrut({ email: victimEmail });
if (existing) await UserService.remove(existing);
victim = await UserService.create({
firstName: 'Victim',
lastName: 'Owner',
email: victimEmail,
provider: 'google',
providerData: { email: victimEmail },
roles: ['user'],
});
const result = await agent
.post('/api/auth/google/callback')
.send({ strategy: false, key: 'id', value: 'cb-app-auth-id-999', firstName: 'OAuth', lastName: 'Callback', email: oauthEmail })
.expect(200);
.send({ strategy: false, key: 'email', value: victimEmail, email: victimEmail });

// No session may be issued to the attacker — the real security invariant.
// The failure path may 302-redirect to an error route rather than returning
// a 4xx, so we assert the outcome (no TOKEN cookie, no successful session)
// rather than the transport status code.
const tokenCookie = result.headers['set-cookie']?.find((c) => c.startsWith('TOKEN='));
expect(tokenCookie).toBeDefined();
expect(tokenCookie).toMatch(/HttpOnly/i);
expect(tokenCookie).toMatch(/SameSite=Strict/i);
expect(result.body.message).toBe('oAuth Ok');
} catch (err) {
console.log(err);
expect(err).toBeFalsy();
expect(tokenCookie).toBeUndefined();
expect(result.status).not.toBe(200);
} finally {
try {
const u = await UserService.getBrut({ email: oauthEmail });
if (u) await UserService.remove(u);
} catch (_) { /* cleanup */ }
try { if (victim) await UserService.remove(victim); } catch (_) { /* cleanup */ }
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

test('should return 422 when client-side OAuth callback receives an invalid profile', async () => {
const result = await agent
.post('/api/auth/google/callback')
.send({
strategy: false,
key: 'id',
value: 'cb-app-auth-id-invalid-999',
firstName: 'Invalid1',
lastName: 'Callback',
email: 'oauthcb-invalid@test.com',
})
.expect(422);

expect(result.body.type).toBe('error');
expect(result.body.message).toMatch(/^Schema validation error/);
expect(result.body.description).toEqual(expect.any(String));
});

test('should set tokenCookieOptions and redirect on classic web oAuth success', async () => {
const mockUserId = 'mock-oauth-user-id-123';
const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce(
Expand Down
Loading