Skip to content

Commit

Permalink
refactor(core): Use consistent CSRF state validation across oauth con…
Browse files Browse the repository at this point in the history 
…trollers
  • Loading branch information
@netroy
netroy committed Apr 29, 2024
1 parent 8a26f42 commit 9c895b8
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 58 deletions.
42 changes: 42 additions & 0 deletions packages/cli/src/controllers/oauth/abstractOAuth.controller.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { Service } from 'typedi';
import Csrf from 'csrf';
import type { Response } from 'express';
import { Credentials } from 'n8n-core';
import type { ICredentialDataDecryptedObject, IWorkflowExecuteAdditionalData } from 'n8n-workflow';
import { jsonParse, ApplicationError } from 'n8n-workflow';

import config from '@/config';
import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
import type { User } from '@db/entities/User';
Expand All @@ -17,6 +21,11 @@ import { UrlService } from '@/services/url.service';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { NotFoundError } from '@/errors/response-errors/not-found.error';

export interface CsrfStateParam {
cid: string;
token: string;
}

@Service()
export abstract class AbstractOAuthController {
abstract oauthVersion: number;
Expand Down Expand Up @@ -107,4 +116,37 @@ export abstract class AbstractOAuthController {
protected async getCredentialWithoutUser(credentialId: string): Promise<ICredentialsDb | null> {
return await this.credentialsRepository.findOneBy({ id: credentialId });
}

protected createCsrfState(credentialsId: string): [string, string] {
const token = new Csrf();
const csrfSecret = token.secretSync();
const state: CsrfStateParam = {
token: token.create(csrfSecret),
cid: credentialsId,
};
return [csrfSecret, Buffer.from(JSON.stringify(state)).toString('base64')];
}

protected decodeCsrfState(encodedState: string): CsrfStateParam {
const errorMessage = 'Invalid state format';
const decoded = jsonParse<CsrfStateParam>(Buffer.from(encodedState, 'base64').toString(), {
errorMessage,
});
if (typeof decoded.cid !== 'string' || typeof decoded.token !== 'string') {
throw new ApplicationError(errorMessage);
}
return decoded;
}

protected verifyCsrfState(decrypted: ICredentialDataDecryptedObject, state: CsrfStateParam) {
const token = new Csrf();
return (
decrypted.csrfSecret === undefined ||
!token.verify(decrypted.csrfSecret as string, state.token)
);
}

protected renderCallbackError(res: Response, message: string, reason?: string) {
res.render('oauth-error-callback', { error: { message, reason } });
}
}
39 changes: 25 additions & 14 deletions packages/cli/src/controllers/oauth/oAuth1Credential.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { RESPONSE_ERROR_MESSAGES } from '@/constants';
import { Get, RestController } from '@/decorators';
import { OAuthRequest } from '@/requests';
import { sendErrorResponse } from '@/ResponseHelper';
import { AbstractOAuthController } from './abstractOAuth.controller';
import { AbstractOAuthController, type CsrfStateParam } from './abstractOAuth.controller';
import { NotFoundError } from '@/errors/response-errors/not-found.error';
import { ServiceUnavailableError } from '@/errors/response-errors/service-unavailable.error';

Expand Down Expand Up @@ -44,6 +44,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
decryptedDataOriginal,
additionalData,
);
const [csrfSecret, state] = this.createCsrfState(credential.id);

const signatureMethod = oauthCredentials.signatureMethod;

Expand All @@ -61,7 +62,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
};

const oauthRequestData = {
oauth_callback: `${this.baseUrl}/callback?cid=${credential.id}`,
oauth_callback: `${this.baseUrl}/callback?state=${state}`,
};

await this.externalHooks.run('oauth1.authenticate', [oAuthOptions, oauthRequestData]);
Expand Down Expand Up @@ -90,6 +91,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {

const returnUri = `${oauthCredentials.authUrl}?oauth_token=${responseJson.oauth_token}`;

decryptedDataOriginal.csrfSecret = csrfSecret;
await this.encryptAndSaveData(credential, decryptedDataOriginal);

this.logger.verbose('OAuth1 authorization successful for new credential', {
Expand All @@ -103,27 +105,34 @@ export class OAuth1CredentialController extends AbstractOAuthController {
/** Verify and store app code. Generate access tokens and store for respective credential */
@Get('/callback', { usesTemplates: true })
async handleCallback(req: OAuthRequest.OAuth1Credential.Callback, res: Response) {
const userId = req.user?.id;
try {
const { oauth_verifier, oauth_token, cid: credentialId } = req.query;
const { oauth_verifier, oauth_token, state: encodedState } = req.query;

if (!oauth_verifier || !oauth_token) {
if (!oauth_verifier || !oauth_token || !encodedState) {
const errorResponse = new ServiceUnavailableError(
`Insufficient parameters for OAuth1 callback. Received following query parameters: ${JSON.stringify(
req.query,
)}`,
);
this.logger.error('OAuth1 callback failed because of insufficient parameters received', {
userId: req.user?.id,
credentialId,
userId,
});
return sendErrorResponse(res, errorResponse);
}

const credential = await this.getCredentialWithoutUser(credentialId);
let state: CsrfStateParam;
try {
state = this.decodeCsrfState(encodedState);
} catch (error) {
return this.renderCallbackError(res, (error as Error).message);
}

const credentialId = state.cid;
const credential = await this.getCredentialWithoutUser(credentialId);
if (!credential) {
this.logger.error('OAuth1 callback failed because of insufficient user permissions', {
userId: req.user?.id,
userId,
credentialId,
});
const errorResponse = new NotFoundError(RESPONSE_ERROR_MESSAGES.NO_CREDENTIAL);
Expand All @@ -138,6 +147,12 @@ export class OAuth1CredentialController extends AbstractOAuthController {
additionalData,
);

if (this.verifyCsrfState(decryptedDataOriginal, state)) {
const errorMessage = 'The OAuth1 callback state is invalid!';
this.logger.debug(errorMessage, { credentialId });
return this.renderCallbackError(res, errorMessage);
}

const options: AxiosRequestConfig = {
method: 'POST',
url: oauthCredentials.accessTokenUrl,
Expand All @@ -152,10 +167,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
try {
oauthToken = await axios.request(options);
} catch (error) {
this.logger.error('Unable to fetch tokens for OAuth1 callback', {
userId: req.user?.id,
credentialId,
});
this.logger.error('Unable to fetch tokens for OAuth1 callback', { userId, credentialId });
const errorResponse = new NotFoundError('Unable to get access tokens!');
return sendErrorResponse(res, errorResponse);
}
Expand All @@ -177,8 +189,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
return res.render('oauth-callback');
} catch (error) {
this.logger.error('OAuth1 callback failed because of insufficient user permissions', {
userId: req.user?.id,
credentialId: req.query.cid,
userId,
});
// Error response
return sendErrorResponse(res, error as Error);
Expand Down
46 changes: 4 additions & 42 deletions packages/cli/src/controllers/oauth/oAuth2Credential.controller.ts
View file
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
import type { ClientOAuth2Options, OAuth2CredentialData } from '@n8n/client-oauth2';
import { ClientOAuth2 } from '@n8n/client-oauth2';
import Csrf from 'csrf';
import { Response } from 'express';
import pkceChallenge from 'pkce-challenge';
import * as qs from 'querystring';
import omit from 'lodash/omit';
import set from 'lodash/set';
import split from 'lodash/split';
import { ApplicationError, jsonParse, jsonStringify } from 'n8n-workflow';
import { Get, RestController } from '@/decorators';
import { jsonStringify } from 'n8n-workflow';
import { OAuthRequest } from '@/requests';
import { AbstractOAuthController } from './abstractOAuth.controller';

interface CsrfStateParam {
cid: string;
token: string;
}
import { AbstractOAuthController, type CsrfStateParam } from './abstractOAuth.controller';

@RestController('/oauth2-credential')
export class OAuth2CredentialController extends AbstractOAuthController {
Expand Down Expand Up @@ -123,16 +117,9 @@ export class OAuth2CredentialController extends AbstractOAuthController {
additionalData,
);

const token = new Csrf();
if (
decryptedDataOriginal.csrfSecret === undefined ||
!token.verify(decryptedDataOriginal.csrfSecret as string, state.token)
) {
if (this.verifyCsrfState(decryptedDataOriginal, state)) {
const errorMessage = 'The OAuth2 callback state is invalid!';
this.logger.debug(errorMessage, {
userId: req.user?.id,
credentialId: credential.id,
});
this.logger.debug(errorMessage, { credentialId: credential.id });
return this.renderCallbackError(res, errorMessage);
}

Expand Down Expand Up @@ -219,29 +206,4 @@ export class OAuth2CredentialController extends AbstractOAuthController {
ignoreSSLIssues: credential.ignoreSSLIssues ?? false,
};
}

private renderCallbackError(res: Response, message: string, reason?: string) {
res.render('oauth-error-callback', { error: { message, reason } });
}

private createCsrfState(credentialsId: string): [string, string] {
const token = new Csrf();
const csrfSecret = token.secretSync();
const state: CsrfStateParam = {
token: token.create(csrfSecret),
cid: credentialsId,
};
return [csrfSecret, Buffer.from(JSON.stringify(state)).toString('base64')];
}

private decodeCsrfState(encodedState: string): CsrfStateParam {
const errorMessage = 'Invalid state format';
const decoded = jsonParse<CsrfStateParam>(Buffer.from(encodedState, 'base64').toString(), {
errorMessage,
});
if (typeof decoded.cid !== 'string' || typeof decoded.token !== 'string') {
throw new ApplicationError(errorMessage);
}
return decoded;
}
}
2 changes: 1 addition & 1 deletion packages/cli/src/requests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ export declare namespace OAuthRequest {
{},
{},
{},
{ oauth_verifier: string; oauth_token: string; cid: string }
{ oauth_verifier: string; oauth_token: string; state: string }
> & {
user?: User;
};
Expand Down
8 changes: 7 additions & 1 deletion packages/cli/test/unit/controllers/oAuth1Credential.controller.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import nock from 'nock';
import Container from 'typedi';
import Csrf from 'csrf';
import { Cipher } from 'n8n-core';
import { mock } from 'jest-mock-extended';

Expand Down Expand Up @@ -30,6 +31,8 @@ describe('OAuth1CredentialController', () => {
const credentialsHelper = mockInstance(CredentialsHelper);
const credentialsRepository = mockInstance(CredentialsRepository);
const sharedCredentialsRepository = mockInstance(SharedCredentialsRepository);

const csrfSecret = 'csrf-secret';
const user = mock<User>({
id: '123',
password: 'password',
Expand Down Expand Up @@ -66,6 +69,8 @@ describe('OAuth1CredentialController', () => {
});

it('should return a valid auth URI', async () => {
jest.spyOn(Csrf.prototype, 'secretSync').mockReturnValueOnce(csrfSecret);
jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token');
sharedCredentialsRepository.findCredentialForUser.mockResolvedValueOnce(credential);
credentialsHelper.getDecrypted.mockResolvedValueOnce({});
credentialsHelper.applyDefaultsAndOverwrites.mockReturnValueOnce({
Expand All @@ -75,7 +80,8 @@ describe('OAuth1CredentialController', () => {
});
nock('https://example.domain')
.post('/oauth/request_token', {
oauth_callback: 'http://localhost:5678/rest/oauth1-credential/callback?cid=1',
oauth_callback:
'http://localhost:5678/rest/oauth1-credential/callback?state=eyJ0b2tlbiI6InRva2VuIiwiY2lkIjoiMSJ9',
})
.reply(200, { oauth_token: 'random-token' });
cipher.encrypt.mockReturnValue('encrypted');
Expand Down

0 comments on commit 9c895b8

Please sign in to comment.