fix: third-party login is broken (#37707)

Author: d-gubert

.changeset/rich-dogs-wonder.md

@@ -0,0 +1,5 @@
+---
+'@rocket.chat/meteor': patch
+---
+
+Fixes an issue that caused Third-party login to not work properly

apps/meteor/app/api/server/ApiClass.ts

@@ -28,7 +28,6 @@ import type {
 	NotFoundResult,
 	Operations,
 	Options,
-	PartialThis,
 	SuccessResult,
 	TypedThis,
 	TypedAction,
@@ -37,6 +36,7 @@ import type {
 	RedirectStatusCodes,
 	RedirectResult,
 	UnavailableResult,
+	GenericRouteExecutionContext,
 } from './definition';
 import { getUserInfo } from './helpers/getUserInfo';
 import { parseJsonQuery } from './helpers/parseJsonQuery';
@@ -156,12 +156,7 @@ const generateConnection = (
 	clientAddress: ipAddress,
 });
 
-export class APIClass<
-	TBasePath extends string = '',
-	TOperations extends {
-		[x: string]: unknown;
-	} = {},
-> {
+export class APIClass<TBasePath extends string = '', TOperations extends Record<string, unknown> = Record<string, never>> {
 	public typedRoutes: Record<string, Record<string, Route>> = {};
 
 	protected apiPath?: string;
@@ -170,7 +165,7 @@ export class APIClass<
 
 	private _routes: { path: string; options: Options; endpoints: Record<string, string> }[] = [];
 
-	public authMethods: ((...args: any[]) => any)[];
+	public authMethods: ((routeContext: GenericRouteExecutionContext) => Promise<IUser | undefined>)[];
 
 	protected helperMethods: Map<string, () => any> = new Map();
 
@@ -248,11 +243,11 @@ export class APIClass<
 		};
 	}
 
-	async parseJsonQuery(this: PartialThis) {
-		return parseJsonQuery(this);
+	async parseJsonQuery(routeContext: GenericRouteExecutionContext) {
+		return parseJsonQuery(routeContext);
 	}
 
-	public addAuthMethod(func: (this: PartialThis, ...args: any[]) => any): void {
+	public addAuthMethod(func: (routeContext: GenericRouteExecutionContext) => Promise<IUser | undefined>): void {
 		this.authMethods.push(func);
 	}
 
@@ -822,8 +817,12 @@ export class APIClass<
 				const api = this;
 				(operations[method as keyof Operations<TPathPattern, TOptions>] as Record<string, any>).action =
 					async function _internalRouteActionHandler() {
+						this.queryOperations = options.queryOperations;
+						this.queryFields = options.queryFields;
+						this.logger = logger;
+
 						if (options.authRequired || options.authOrAnonRequired) {
-							const user = await api.authenticatedRoute.call(this, this.request);
+							const user = await api.authenticatedRoute(this);
 							this.user = user!;
 							this.userId = this.user?._id;
 							const authToken = this.request.headers.get('x-auth-token');
@@ -918,9 +917,7 @@ export class APIClass<
 									connection: connection as unknown as IMethodConnection,
 								}));
 
-							this.queryOperations = options.queryOperations;
-							(this as any).queryFields = options.queryFields;
-							this.parseJsonQuery = api.parseJsonQuery.bind(this as unknown as PartialThis);
+							this.parseJsonQuery = () => api.parseJsonQuery(this);
 
 							result = (await DDP._CurrentInvocation.withValue(invocation as any, async () => originalAction.apply(this))) || api.success();
 						} catch (e: any) {
@@ -972,12 +969,9 @@ export class APIClass<
 		});
 	}
 
-	protected async authenticatedRoute(req: Request): Promise<IUser | null> {
-		const headers = Object.fromEntries(req.headers.entries());
-
-		const { 'x-user-id': userId } = headers;
-
-		const userToken = String(headers['x-auth-token']);
+	protected async authenticatedRoute(routeContext: GenericRouteExecutionContext): Promise<IUser | null> {
+		const userId = routeContext.request.headers.get('x-user-id');
+		const userToken = routeContext.request.headers.get('x-auth-token');
 
 		if (userId && userToken) {
 			return Users.findOne(
@@ -990,6 +984,16 @@ export class APIClass<
 				},
 			);
 		}
+
+		for (const method of this.authMethods) {
+			// eslint-disable-next-line no-await-in-loop -- we want serial execution
+			const user = await method(routeContext);
+
+			if (user) {
+				return user;
+			}
+		}
+
 		return null;
 	}
 

apps/meteor/app/api/server/definition.ts

@@ -142,21 +142,16 @@ export type SharedOptions<TMethod extends string> = (
 	};
 };
 
-export type PartialThis = {
-	user(bodyParams: Record<string, unknown>, user: any): Promise<any>;
-	readonly request: Request & { query: Record<string, string> };
-	readonly response: Response;
-	readonly userId: string;
-	readonly bodyParams: Record<string, unknown>;
-	readonly path: string;
-	readonly queryParams: Record<string, string>;
-	readonly queryOperations?: string[];
-	readonly queryFields?: string[];
-	readonly logger: Logger;
-	readonly route: string;
-};
+export type GenericRouteExecutionContext = ActionThis<any, any, any>;
+
+export type RouteExecutionContext<TMethod extends Method, TPathPattern extends PathPattern, TOptions> = ActionThis<
+	TMethod,
+	TPathPattern,
+	TOptions
+>;
 
 export type ActionThis<TMethod extends Method, TPathPattern extends PathPattern, TOptions> = {
+	readonly logger: Logger;
 	route: string;
 	readonly requestIp: string;
 	urlParams: UrlParams<TPathPattern>;
@@ -183,6 +178,8 @@ export type ActionThis<TMethod extends Method, TPathPattern extends PathPattern,
 	readonly request: Request;
 
 	readonly queryOperations: TOptions extends { queryOperations: infer T } ? T : never;
+	readonly queryFields: TOptions extends { queryFields: infer T } ? T : never;
+
 	parseJsonQuery(): Promise<{
 		sort: Record<string, 1 | -1>;
 		/**

apps/meteor/app/api/server/helpers/parseJsonQuery.ts

@@ -1,10 +1,11 @@
 import ejson from 'ejson';
 import { Meteor } from 'meteor/meteor';
 
+import { isPlainObject } from '../../../../lib/utils/isPlainObject';
 import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';
 import { apiDeprecationLogger } from '../../../lib/server/lib/deprecationWarningLogger';
 import { API } from '../api';
-import type { PartialThis } from '../definition';
+import type { GenericRouteExecutionContext } from '../definition';
 import { clean } from '../lib/cleanQuery';
 import { isValidQuery } from '../lib/isValidQuery';
 
@@ -13,7 +14,7 @@ const pathAllowConf = {
 	'def': ['$or', '$and', '$regex'],
 };
 
-export async function parseJsonQuery(api: PartialThis): Promise<{
+export async function parseJsonQuery(api: GenericRouteExecutionContext): Promise<{
 	sort: Record<string, 1 | -1>;
 	/**
 	 * @deprecated To access "fields" parameter, use ALLOW_UNSAFE_QUERY_AND_FIELDS_API_PARAMS environment variable.
@@ -24,10 +25,14 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
 	 */
 	query: Record<string, unknown>;
 }> {
-	const { userId, queryParams: params, logger, queryFields, queryOperations, response, route } = api;
+	const { userId = '', response, route, logger } = api;
+
+	const params = isPlainObject(api.queryParams) ? api.queryParams : {};
+	const queryFields = Array.isArray(api.queryFields) ? (api.queryFields as string[]) : [];
+	const queryOperations = Array.isArray(api.queryOperations) ? (api.queryOperations as string[]) : [];
 
 	let sort;
-	if (params.sort) {
+	if (typeof params?.sort === 'string') {
 		try {
 			sort = JSON.parse(params.sort);
 			Object.entries(sort).forEach(([key, value]) => {
@@ -50,7 +55,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
 		`The usage of the "${parameter}" parameter in endpoint "${endpoint}" breaks the security of the API and can lead to data exposure. It has been deprecated and will be removed in the version ${version}.`;
 
 	let fields: Record<string, 0 | 1> | undefined;
-	if (params.fields && isUnsafeQueryParamsAllowed) {
+	if (typeof params?.fields === 'string' && isUnsafeQueryParamsAllowed) {
 		try {
 			apiDeprecationLogger.parameter(route, 'fields', '8.0.0', response, messageGenerator);
 			fields = JSON.parse(params.fields) as Record<string, 0 | 1>;
@@ -100,7 +105,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
 	}
 
 	let query: Record<string, any> = {};
-	if (params.query && isUnsafeQueryParamsAllowed) {
+	if (typeof params?.query === 'string' && isUnsafeQueryParamsAllowed) {
 		apiDeprecationLogger.parameter(route, 'query', '8.0.0', response, messageGenerator);
 		try {
 			query = ejson.parse(params.query);

apps/meteor/app/api/server/middlewares/authentication.ts

@@ -27,7 +27,10 @@ export function authenticationMiddleware(
 		if (userId && authToken) {
 			req.user = (await Users.findOneByIdAndLoginToken(userId as string, hashLoginToken(authToken as string))) || undefined;
 		} else {
-			req.user = (await oAuth2ServerAuth(req))?.user;
+			req.user = await oAuth2ServerAuth({
+				headers: req.headers as Record<string, string | undefined>,
+				query: req.query as Record<string, string | undefined>,
+			});
 		}
 
 		if (config.rejectUnauthorized && !req.user) {

apps/meteor/app/integrations/server/api/api.ts

@@ -9,10 +9,11 @@ import type { RateLimiterOptionsToCheck } from 'meteor/rate-limit';
 import { WebApp } from 'meteor/webapp';
 import _ from 'underscore';
 
+import { isPlainObject } from '../../../../lib/utils/isPlainObject';
 import { APIClass } from '../../../api/server/ApiClass';
 import type { RateLimiterOptions } from '../../../api/server/api';
 import { API, defaultRateLimiterOptions } from '../../../api/server/api';
-import type { FailureResult, PartialThis, SuccessResult, UnavailableResult } from '../../../api/server/definition';
+import type { FailureResult, GenericRouteExecutionContext, SuccessResult, UnavailableResult } from '../../../api/server/definition';
 import type { WebhookResponseItem } from '../../../lib/server/functions/processWebhookMessage';
 import { processWebhookMessage } from '../../../lib/server/functions/processWebhookMessage';
 import { settings } from '../../../settings/server';
@@ -39,11 +40,10 @@ type IntegrationOptions = {
 	};
 };
 
-type IntegrationThis = Omit<PartialThis, 'user'> & {
+type IntegrationThis = GenericRouteExecutionContext & {
 	request: Request & {
 		integration: IIncomingIntegration;
 	};
-	urlParams: Record<string, string>;
 	user: IUser & { username: RequiredField<IUser, 'username'> };
 };
 
@@ -138,8 +138,8 @@ async function executeIntegrationRest(
 
 	const scriptEngine = getEngine(this.request.integration);
 
-	let { bodyParams } = this;
-	const separateResponse = this.bodyParams?.separateResponse === true;
+	let bodyParams = isPlainObject(this.bodyParams) ? this.bodyParams : {};
+	const separateResponse = bodyParams.separateResponse === true;
 	let scriptResponse: Record<string, any> | undefined;
 
 	if (scriptEngine.integrationHasValidScript(this.request.integration) && this.request.body) {
@@ -152,21 +152,22 @@ async function executeIntegrationRest(
 		const contentRaw = Buffer.concat(buffers).toString('utf8');
 		const protocol = `${this.request.headers.get('x-forwarded-proto')}:` || 'http:';
 		const url = new URL(this.request.url, `${protocol}//${this.request.headers.get('host')}`);
+		const query = isPlainObject(this.queryParams) ? this.queryParams : {};
 
 		const request = {
 			url: {
+				query,
 				hash: url.hash,
 				search: url.search,
-				query: this.queryParams,
 				pathname: url.pathname,
 				path: this.request.url,
 			},
 			url_raw: this.request.url,
 			url_params: this.urlParams,
-			content: this.bodyParams,
+			content: bodyParams,
 			content_raw: contentRaw,
 			headers: Object.fromEntries(this.request.headers.entries()),
-			body: this.bodyParams,
+			body: bodyParams,
 			user: {
 				_id: this.user._id,
 				name: this.user.name || '',
@@ -191,7 +192,7 @@ async function executeIntegrationRest(
 				return API.v1.failure(result.error);
 			}
 
-			bodyParams = result && result.content;
+			bodyParams = result?.content;
 
 			if (!('separateResponse' in bodyParams)) {
 				bodyParams.separateResponse = separateResponse;
@@ -312,8 +313,8 @@ function integrationInfoRest(): { statusCode: number; body: { success: boolean }
 }
 
 class WebHookAPI extends APIClass<'/hooks'> {
-	override async authenticatedRoute(this: IntegrationThis): Promise<IUser | null> {
-		const { integrationId, token } = this.urlParams;
+	override async authenticatedRoute(routeContext: IntegrationThis): Promise<IUser | null> {
+		const { integrationId, token } = routeContext.urlParams;
 		const integration = await Integrations.findOneByIdAndToken<IIncomingIntegration>(integrationId, decodeURIComponent(token));
 
 		if (!integration) {
@@ -322,9 +323,9 @@ class WebHookAPI extends APIClass<'/hooks'> {
 			throw new Error('Invalid integration id or token provided.');
 		}
 
-		this.request.integration = integration;
+		routeContext.request.integration = integration;
 
-		return Users.findOneById(this.request.integration.userId);
+		return Users.findOneById(routeContext.request.integration.userId);
 	}
 
 	override shouldAddRateLimitToRoute(options: { rateLimiterOptions?: RateLimiterOptions | boolean }): boolean {

apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts

@@ -5,6 +5,7 @@ import type express from 'express';
 import { Meteor } from 'meteor/meteor';
 import { WebApp } from 'meteor/webapp';
 
+import { isPlainObject } from '../../../../lib/utils/isPlainObject';
 import { OAuth2Server } from '../../../../server/oauth2-server/oauth';
 import { API } from '../../../api/server';
 
@@ -19,13 +20,18 @@ async function getAccessToken(accessToken: string) {
 }
 
 export async function oAuth2ServerAuth(partialRequest: {
-	headers: Record<string, any>;
-	query: Record<string, any>;
-}): Promise<{ user: IUser } | undefined> {
+	headers: Record<string, string | undefined>;
+	query: Record<string, string | undefined>;
+}): Promise<IUser | undefined> {
 	const headerToken = partialRequest.headers.authorization?.replace('Bearer ', '');
 	const queryToken = partialRequest.query.access_token;
+	const incomingToken = headerToken || queryToken;
 
-	const accessToken = await getAccessToken(headerToken || queryToken);
+	if (!incomingToken) {
+		return;
+	}
+
+	const accessToken = await getAccessToken(incomingToken);
 
 	// If there is no token available or the token has expired, return undefined
 	if (!accessToken || (accessToken.expires != null && accessToken.expires < new Date())) {
@@ -38,7 +44,7 @@ export async function oAuth2ServerAuth(partialRequest: {
 		return;
 	}
 
-	return { user };
+	return user;
 }
 
 oauth2server.app.disable('x-powered-by');
@@ -69,8 +75,11 @@ oauth2server.app.get('/oauth/userinfo', async (req: Request, res: Response) => {
 	});
 });
 
-API.v1.addAuthMethod(async function () {
-	return oAuth2ServerAuth(this.request);
+API.v1.addAuthMethod((routeContext) => {
+	const headers = Object.fromEntries(routeContext.request.headers.entries());
+	const query = (isPlainObject(routeContext.queryParams) ? routeContext.queryParams : {}) as Record<string, string | undefined>;
+
+	return oAuth2ServerAuth({ headers, query });
 });
 
 (WebApp.connectHandlers as unknown as ReturnType<typeof express>).use(oauth2server.app);