Skip to content

Commit

Permalink
fix!: api login should not suggest which credential is wrong (#32159)
Browse files Browse the repository at this point in the history
  • Loading branch information
ggazzo committed Sep 11, 2024
1 parent 1b9a532 commit 4b841b9
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 26 deletions.
7 changes: 7 additions & 0 deletions .changeset/fuzzy-cherries-buy.md
Original file line number Diff line number Diff line change
@@ -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
14 changes: 0 additions & 14 deletions apps/meteor/app/lib/server/lib/loginErrorMessageOverride.js

This file was deleted.

16 changes: 16 additions & 0 deletions apps/meteor/app/lib/server/lib/loginErrorMessageOverride.ts
Original file line number Diff line number Diff line change
@@ -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;
};
10 changes: 0 additions & 10 deletions apps/meteor/client/meteorOverrides/login/google.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 9 additions & 1 deletion apps/meteor/definition/externals/meteor/accounts-base.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ declare module 'meteor/accounts-base' {

function _insertLoginToken(userId: string, token: { token: string; when: Date }): void;

function _runLoginHandlers<T>(methodInvocation: T, loginRequest: Record<string, any>): LoginMethodResult | undefined;
function _runLoginHandlers<T>(methodInvocation: T, loginRequest: Record<string, any>): Promise<LoginMethodResult>;

function registerLoginHandler(name: string, handler: (options: any) => undefined | object): void;

Expand Down Expand Up @@ -56,6 +56,14 @@ declare module 'meteor/accounts-base' {

const _accountData: Record<string, any>;

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(
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/tests/end-to-end/api/failed-login-attempts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
}

Expand Down

0 comments on commit 4b841b9

Please sign in to comment.