Skip to content

Commit

Permalink
feat(core): Make OAuth2 error handling consistent with success handli…
Browse files Browse the repository at this point in the history
…ng (#5555)

Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <[email protected]>
  • Loading branch information
ManishDhanwal07 and netroy authored Mar 22, 2023
1 parent 135b0d3 commit 40aacf9
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 35 deletions.
1 change: 1 addition & 0 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@
"curlconverter": "^3.0.0",
"dotenv": "^8.0.0",
"express": "^4.18.2",
"express-handlebars": "^7.0.2",
"express-async-errors": "^3.1.1",
"express-openapi-validator": "^4.13.6",
"express-prom-bundle": "^6.6.0",
Expand Down
5 changes: 5 additions & 0 deletions packages/cli/src/Server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { createHmac } from 'crypto';
import { promisify } from 'util';
import cookieParser from 'cookie-parser';
import express from 'express';
import { engine as expressHandlebars } from 'express-handlebars';
import type { ServeStaticOptions } from 'serve-static';
import type { FindManyOptions } from 'typeorm';
import { Not, In } from 'typeorm';
Expand Down Expand Up @@ -183,6 +184,10 @@ class Server extends AbstractServer {
constructor() {
super();

this.app.engine('handlebars', expressHandlebars({ defaultLayout: false }));
this.app.set('view engine', 'handlebars');
this.app.set('views', TEMPLATES_DIR);

this.loadNodesAndCredentials = Container.get(LoadNodesAndCredentials);
this.credentialTypes = Container.get(CredentialTypes);
this.nodeTypes = Container.get(NodeTypes);
Expand Down
48 changes: 18 additions & 30 deletions packages/cli/src/credentials/oauth2Credential.api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import type {
WorkflowExecuteMode,
INodeCredentialsDetails,
ICredentialsEncrypted,
IDataObject,
} from 'n8n-workflow';
import { LoggerProxy } from 'n8n-workflow';
import { resolve as pathResolve } from 'path';
Expand Down Expand Up @@ -112,7 +111,7 @@ oauth2CredentialController.get(
);

const token = new Csrf();
// Generate a CSRF prevention token and send it as a OAuth2 state stringma/ERR
// Generate a CSRF prevention token and send it as an OAuth2 state string
const csrfSecret = token.secretSync();
const state = {
token: token.create(csrfSecret),
Expand Down Expand Up @@ -174,6 +173,9 @@ oauth2CredentialController.get(
}),
);

const renderCallbackError = (res: express.Response, errorMessage: string) =>
res.render('oauth-error-callback', { error: { message: errorMessage } });

/**
* GET /oauth2-credential/callback
*
Expand All @@ -188,12 +190,12 @@ oauth2CredentialController.get(
const { code, state: stateEncoded } = req.query;

if (!code || !stateEncoded) {
const errorResponse = new ResponseHelper.ServiceUnavailableError(
return renderCallbackError(
res,
`Insufficient parameters for OAuth2 callback. Received following query parameters: ${JSON.stringify(
req.query,
)}`,
);
return ResponseHelper.sendErrorResponse(res, errorResponse);
}

let state;
Expand All @@ -203,31 +205,21 @@ oauth2CredentialController.get(
token: string;
};
} catch (error) {
const errorResponse = new ResponseHelper.ServiceUnavailableError(
'Invalid state format returned',
);
return ResponseHelper.sendErrorResponse(res, errorResponse);
return renderCallbackError(res, 'Invalid state format returned');
}

const credential = await getCredentialWithoutUser(state.cid);

if (!credential) {
LoggerProxy.error('OAuth2 callback failed because of insufficient permissions', {
const errorMessage = 'OAuth2 callback failed because of insufficient permissions';
LoggerProxy.error(errorMessage, {
userId: req.user?.id,
credentialId: state.cid,
});
const errorResponse = new ResponseHelper.NotFoundError(
RESPONSE_ERROR_MESSAGES.NO_CREDENTIAL,
);
return ResponseHelper.sendErrorResponse(res, errorResponse);
return renderCallbackError(res, errorMessage);
}

let encryptionKey: string;
try {
encryptionKey = await UserSettings.getEncryptionKey();
} catch (error) {
throw new ResponseHelper.InternalServerError((error as IDataObject).message as string);
}
const encryptionKey = await UserSettings.getEncryptionKey();

const mode: WorkflowExecuteMode = 'internal';
const timezone = config.getEnv('generic.timezone');
Expand All @@ -251,14 +243,12 @@ oauth2CredentialController.get(
decryptedDataOriginal.csrfSecret === undefined ||
!token.verify(decryptedDataOriginal.csrfSecret as string, state.token)
) {
LoggerProxy.debug('OAuth2 callback state is invalid', {
const errorMessage = 'The OAuth2 callback state is invalid!';
LoggerProxy.debug(errorMessage, {
userId: req.user?.id,
credentialId: state.cid,
});
const errorResponse = new ResponseHelper.NotFoundError(
'The OAuth2 callback state is invalid!',
);
return ResponseHelper.sendErrorResponse(res, errorResponse);
return renderCallbackError(res, errorMessage);
}

let options = {};
Expand Down Expand Up @@ -298,12 +288,12 @@ oauth2CredentialController.get(
}

if (oauthToken === undefined) {
LoggerProxy.error('OAuth2 callback failed: unable to get access tokens', {
const errorMessage = 'Unable to get OAuth2 access tokens!';
LoggerProxy.error(errorMessage, {
userId: req.user?.id,
credentialId: state.cid,
});
const errorResponse = new ResponseHelper.NotFoundError('Unable to get access tokens!');
return ResponseHelper.sendErrorResponse(res, errorResponse);
return renderCallbackError(res, errorMessage);
}

if (decryptedDataOriginal.oauthTokenData) {
Expand Down Expand Up @@ -336,9 +326,7 @@ oauth2CredentialController.get(

return res.sendFile(pathResolve(TEMPLATES_DIR, 'oauth-callback.html'));
} catch (error) {
// Error response
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
return ResponseHelper.sendErrorResponse(res, error);
return renderCallbackError(res, (error as Error).message);
}
},
);
19 changes: 19 additions & 0 deletions packages/cli/templates/oauth-error-callback.handlebars
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<html>
<head>
<title>n8n - OAuth Callback</title>
<style>
body { font-family: 'Open Sans', sans-serif; padding: 10px;}
pre.error { background: #f7f7f7; border: 1px solid #ddd; border-radius: 3px; padding: 10px; overflow: auto; overflow-wrap: break-word; white-space: pre-wrap; }
</style>
</head>
<body>
{{#if error}}
<h4>Error:</h4>
<pre class='error'>{{error.message}}</pre>
{{/if}}
Failed to connect. The window can be closed now.
<script>
(function messageParent() { window.opener.postMessage('error', '*'); })();
</script>
</body>
</html>
49 changes: 44 additions & 5 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 40aacf9

Please sign in to comment.