From 5bf8408fd3b85d20beb736d9e7348314f3f32b8a Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Tue, 9 Apr 2024 13:47:21 -0300 Subject: [PATCH] fix!: api login should not suggest which credential is wrong (#32159) --- .changeset/fuzzy-cherries-buy.md | 7 +++++++ .../lib/server/lib/loginErrorMessageOverride.js | 14 -------------- .../lib/server/lib/loginErrorMessageOverride.ts | 16 ++++++++++++++++ .../client/meteorOverrides/login/google.ts | 10 ---------- .../externals/meteor/accounts-base.d.ts | 10 +++++++++- .../end-to-end/api/failed-login-attempts.ts | 2 +- 6 files changed, 33 insertions(+), 26 deletions(-) create mode 100644 .changeset/fuzzy-cherries-buy.md delete mode 100644 apps/meteor/app/lib/server/lib/loginErrorMessageOverride.js create mode 100644 apps/meteor/app/lib/server/lib/loginErrorMessageOverride.ts diff --git a/.changeset/fuzzy-cherries-buy.md b/.changeset/fuzzy-cherries-buy.md new file mode 100644 index 0000000000000..e185a148c9172 --- /dev/null +++ b/.changeset/fuzzy-cherries-buy.md @@ -0,0 +1,7 @@ +--- +"@rocket.chat/meteor": major +--- + +Api login should not suggest which credential is wrong (password/username) + +Failed login attemps will always return `Unauthorized` instead of the internal fail reason diff --git a/apps/meteor/app/lib/server/lib/loginErrorMessageOverride.js b/apps/meteor/app/lib/server/lib/loginErrorMessageOverride.js deleted file mode 100644 index 4e054b81b2cf6..0000000000000 --- a/apps/meteor/app/lib/server/lib/loginErrorMessageOverride.js +++ /dev/null @@ -1,14 +0,0 @@ -// Do not disclose if user exists when password is invalid -import { Accounts } from 'meteor/accounts-base'; -import { Meteor } from 'meteor/meteor'; - -const { _runLoginHandlers } = Accounts; -Accounts._runLoginHandlers = function (methodInvocation, options) { - const result = _runLoginHandlers.call(Accounts, methodInvocation, options); - - if (result.error && result.error.reason === 'Incorrect password') { - result.error = new Meteor.Error(403, 'User not found'); - } - - return result; -}; diff --git a/apps/meteor/app/lib/server/lib/loginErrorMessageOverride.ts b/apps/meteor/app/lib/server/lib/loginErrorMessageOverride.ts new file mode 100644 index 0000000000000..e2a6e0d105812 --- /dev/null +++ b/apps/meteor/app/lib/server/lib/loginErrorMessageOverride.ts @@ -0,0 +1,16 @@ +// Do not disclose if user exists when password is invalid +import { Accounts } from 'meteor/accounts-base'; +import { Meteor } from 'meteor/meteor'; + +const { _runLoginHandlers } = Accounts; + +Accounts._options.ambiguousErrorMessages = true; +Accounts._runLoginHandlers = async function (methodInvocation, options) { + const result = await _runLoginHandlers.call(Accounts, methodInvocation, options); + + if (result.error instanceof Meteor.Error) { + result.error = new Meteor.Error(401, 'User not found'); + } + + return result; +}; diff --git a/apps/meteor/client/meteorOverrides/login/google.ts b/apps/meteor/client/meteorOverrides/login/google.ts index 2742cade15d2c..4e99ac3a281b5 100644 --- a/apps/meteor/client/meteorOverrides/login/google.ts +++ b/apps/meteor/client/meteorOverrides/login/google.ts @@ -8,16 +8,6 @@ import { overrideLoginMethod, type LoginCallback } from '../../lib/2fa/overrideL import { wrapRequestCredentialFn } from '../../lib/wrapRequestCredentialFn'; import { createOAuthTotpLoginMethod } from './oauth'; -declare module 'meteor/accounts-base' { - // eslint-disable-next-line @typescript-eslint/no-namespace - namespace Accounts { - export const _options: { - restrictCreationByEmailDomain?: string | (() => string); - forbidClientAccountCreation?: boolean | undefined; - }; - } -} - declare module 'meteor/meteor' { // eslint-disable-next-line @typescript-eslint/no-namespace namespace Meteor { diff --git a/apps/meteor/definition/externals/meteor/accounts-base.d.ts b/apps/meteor/definition/externals/meteor/accounts-base.d.ts index 0d30eed0430de..29445a5a0218a 100644 --- a/apps/meteor/definition/externals/meteor/accounts-base.d.ts +++ b/apps/meteor/definition/externals/meteor/accounts-base.d.ts @@ -23,7 +23,7 @@ declare module 'meteor/accounts-base' { function _insertLoginToken(userId: string, token: { token: string; when: Date }): void; - function _runLoginHandlers(methodInvocation: T, loginRequest: Record): LoginMethodResult | undefined; + function _runLoginHandlers(methodInvocation: T, loginRequest: Record): Promise; function registerLoginHandler(name: string, handler: (options: any) => undefined | object): void; @@ -57,6 +57,14 @@ declare module 'meteor/accounts-base' { const _accountData: Record; + interface AccountsServerOptions { + ambiguousErrorMessages?: boolean; + restrictCreationByEmailDomain?: string | (() => string); + forbidClientAccountCreation?: boolean | undefined; + } + + export const _options: AccountsServerOptions; + // eslint-disable-next-line @typescript-eslint/no-namespace namespace oauth { function credentialRequestCompleteHandler( diff --git a/apps/meteor/tests/end-to-end/api/failed-login-attempts.ts b/apps/meteor/tests/end-to-end/api/failed-login-attempts.ts index ee0236c591b6a..ebdc1737391b3 100644 --- a/apps/meteor/tests/end-to-end/api/failed-login-attempts.ts +++ b/apps/meteor/tests/end-to-end/api/failed-login-attempts.ts @@ -54,7 +54,7 @@ describe('[Failed Login Attempts]', () => { .expect(401) .expect((res) => { expect(res.body).to.have.property('status', 'error'); - expect(res.body).to.have.property('message', 'Incorrect password'); + expect(res.body).to.have.property('message', 'Unauthorized'); }); }