Skip to content
19 changes: 15 additions & 4 deletions apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { IServerEvent } from '@rocket.chat/core-typings';
import { ServerEventType } from '@rocket.chat/core-typings';
import { Logger } from '@rocket.chat/logger';
import { Rooms, ServerEvents, Users } from '@rocket.chat/models';
import { isIpInCidrRange } from '@rocket.chat/server-fetch';

import { addMinutesToADate } from '../../../../lib/utils/addMinutesToADate';
import { getClientAddress } from '../../../../server/lib/getClientAddress';
Expand Down Expand Up @@ -52,7 +53,15 @@ export const isValidLoginAttemptByIp = async (ip: string): Promise<boolean> => {
if (
!settings.get('Block_Multiple_Failed_Logins_Enabled') ||
!settings.get('Block_Multiple_Failed_Logins_By_Ip') ||
whitelist.includes(ip)
whitelist.some((entry) => {
const trimmed = entry.trim();
try {
return trimmed === ip || isIpInCidrRange(ip, trimmed);
Comment thread
namann5 marked this conversation as resolved.
} catch (error) {
logger.error({ msg: 'Invalid CIDR entry in whitelist', entry: trimmed, error });
return false;
}
})
) {
return true;
}
Expand Down Expand Up @@ -100,7 +109,9 @@ export const isValidAttemptByUser = async (login: ILoginAttempt): Promise<boolea
return true;
}

const loginUsername = login.methodArguments[0].user?.username;
const { username, email } = (login.methodArguments[0].user || {}) as { username?: string; email?: string };
const loginUsername = login.user?.username || username || email;
Comment thread
namann5 marked this conversation as resolved.

if (!loginUsername) {
return true;
}
Expand Down Expand Up @@ -147,7 +158,7 @@ export const isValidAttemptByUser = async (login: ILoginAttempt): Promise<boolea
export const saveFailedLoginAttempts = async (login: ILoginAttempt): Promise<void> => {
const user: IServerEvent['u'] = {
_id: login.user?._id,
username: login.user?.username || login.methodArguments[0].user?.username,
username: login.user?.username || login.methodArguments[0].user?.username || login.methodArguments[0].user?.email,
};

await ServerEvents.insertOne({
Expand All @@ -161,7 +172,7 @@ export const saveFailedLoginAttempts = async (login: ILoginAttempt): Promise<voi
export const saveSuccessfulLogin = async (login: ILoginAttempt): Promise<void> => {
const user: IServerEvent['u'] = {
_id: login.user?._id,
username: login.user?.username || login.methodArguments[0].user?.username,
username: login.user?.username || login.methodArguments[0].user?.username || login.methodArguments[0].user?.email,
};

await ServerEvents.insertOne({
Expand Down
18 changes: 17 additions & 1 deletion apps/meteor/lib/utils/isRelativeURL.ts
Original file line number Diff line number Diff line change
@@ -1 +1,17 @@
export const isRelativeURL = (str: string): boolean => /^[^\/]+\/[^\/].*$|^\/[^\/].*$/gim.test(str);
export const isRelativeURL = (str: string): boolean => {
// Scheme-based URLs are absolute (e.g. https:, data:, javascript:).
Comment thread
namann5 marked this conversation as resolved.
if (/^[a-z][a-z\d+\-.]*:/i.test(str)) {
Comment thread
namann5 marked this conversation as resolved.
return false;
}

// URLs starting with // are protocol-relative, not path-relative.
if (str.startsWith('//')) {
return false;
}

if (str === '/' || str === '') {
return false;
}

return true;
};
Comment thread
namann5 marked this conversation as resolved.
10 changes: 7 additions & 3 deletions apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@ import { isRelativeURL } from '../../../../lib/utils/isRelativeURL';
describe('isRelativeURL', () => {
const testCases = [
['/', false],
['test', false], // TODO: should be true?
['', false],
['test', true],
['test/test', true],
['.', false], // TODO: should be true?
['.', true],
['./test', true],
['../test', true],
['/test', true],
['//rocket.chat/path', false],
['https://rocket.chat', false],
['data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==', true], // TODO: should be false?
['data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==', false],
] as const;

testCases.forEach(([parameter, expectedResult]) => {
Expand Down
8 changes: 7 additions & 1 deletion packages/server-fetch/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,13 @@ export const isIpInCidrRange = (ip: string, cidr: string): boolean => {
groups.push(part);
}
}
return groups.reduce((value, hexGroup) => (value << 16n) + BigInt(parseInt(hexGroup, 16)), 0n);
return groups.reduce((value, hexGroup) => {
const parsed = parseInt(hexGroup, 16);
if (isNaN(parsed)) {
return value << 16n;
}
return (value << 16n) + BigInt(parsed);
}, 0n);
};
const mask = prefix === 0 ? 0n : ((1n << BigInt(prefix)) - 1n) << (128n - BigInt(prefix));
return (toBigInt(ip) & mask) === (toBigInt(network) & mask);
Expand Down
1 change: 1 addition & 0 deletions packages/server-fetch/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,5 +194,6 @@ export async function serverFetch(input: string, options?: ExtendedFetchOptions,
}

export { Response };
export { isIpInCidrRange } from './helpers';
export type { ExtendedFetchOptions };
export { parseSsrfAllowlist };
3 changes: 2 additions & 1 deletion packages/server-fetch/tests/checkForSsrf.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,9 +297,10 @@ describe('checkForSsrfHelpers', () => {
expect(isIpInCidrRange('::1', '127.0.0.0/8')).toBe(false);
});
});
it('returns false for invalid IP', () => {
it('returns false for malformed IP or CIDR', () => {
expect(isIpInCidrRange('not-an-ip', '192.168.0.0/16')).toBe(false);
expect(isIpInCidrRange('256.1.1.1', '192.168.0.0/16')).toBe(false);
expect(isIpInCidrRange('::1', '::g/128')).toBe(false); // Malformed hex group
});
Comment thread
namann5 marked this conversation as resolved.
});

Expand Down