Better security and UX (#82)

- Use the state parameter when auth'ing to validate source - Update comment wording - Show success page before redirecting user - Add pull request link to auth url for redirection back to PR - Refactor: Extract some commonly used variables
philip-gai · May 30, 2022 · 920eb6c · 920eb6c
1 parent 5d11471
commit 920eb6c
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 24 deletions.
diff --git a/web-app/package-lock.json b/web-app/package-lock.json
diff --git a/web-app/package.json b/web-app/package.json
@@ -25,11 +25,13 @@
     "@octokit/plugin-paginate-rest": "^2.17.0",
     "@octokit/plugin-rest-endpoint-methods": "^5.13.0",
     "@probot/octokit-plugin-config": "^1.1.2",
+    "crypto-js": "^4.1.1",
     "probot": "^11.0.1",
     "query-string": "^7.0.1",
     "yaml": "^1.10.2"
   },
   "devDependencies": {
+    "@types/crypto-js": "^4.1.1",
     "@types/jest": "^26.0.19",
     "@types/node": "^14.14.19",
     "@typescript-eslint/eslint-plugin": "^5.5.0",

diff --git a/web-app/src/eventHandlers/pullRequestEventHandler.ts b/web-app/src/eventHandlers/pullRequestEventHandler.ts
@@ -50,20 +50,23 @@ export class PullRequestEventHandler {
     const appGitHubService = GitHubService.buildForApp(context.octokit as unknown as OctokitPlus, logger, appConfig);
 
     const payload = context.payload;
+
+    const pullRepo = payload.repository;
+    const pullRequest = payload.pull_request;
     const pullInfo: PullInfo = {
-      owner: payload.repository.owner.login,
-      repo: payload.repository.name,
-      repoName: payload.repository.name,
-      pull_number: payload.pull_request.number,
+      owner: pullRepo.owner.login,
+      repo: pullRepo.name,
+      repoName: pullRepo.name,
+      pull_number: pullRequest.number,
     };
 
-    const isDefaultBranch = payload.pull_request.base.ref === payload.repository.default_branch;
+    const isDefaultBranch = pullRequest.base.ref === pullRepo.default_branch;
     if (!isDefaultBranch) {
       logger.info("The PR is not targeting the default branch, will not post anything");
       return;
     }
 
-    if (payload.pull_request.draft) {
+    if (pullRequest.draft) {
       logger.info("This is a draft PR. Exiting.");
       return;
     }
@@ -100,15 +103,15 @@ export class PullRequestEventHandler {
       const shouldCreateDiscussionForFile = this.shouldCreateDiscussionForFile(appConfig.appSettings, filepath);
       if (shouldCreateDiscussionForFile) {
         try {
-          const fileref = payload.pull_request.head.ref;
+          const fileref = pullRequest.head.ref;
           // Parse the markdown to get the discussion metadata and details
           const parsedMarkdown = await this.getParsedMarkdownDiscussion(appGitHubService, logger, {
             filepath: filepath,
             pullInfo: pullInfo,
             fileref: fileref,
           });
 
-          const authorLogin = payload.pull_request.user.login;
+          const authorLogin = pullRequest.user.login;
 
           if (!parsedMarkdown.repo && !parsedMarkdown.team) {
             throw new Error("Markdown is missing a repo or team to post the discussion to");
@@ -127,16 +130,16 @@ export class PullRequestEventHandler {
             await this._tokenService.deleteRefreshToken(authorLogin);
           }
           if (!userRefreshToken || isNonProd) {
-            const fullAuthUrl = `${appConfig.base_url}${appConfig.auth_url}`;
-            commentBody += `- @${authorLogin} must [authenticate](${fullAuthUrl}) before merging this PR\n`;
+            const fullAuthUrl = `${appConfig.base_url}${appConfig.auth_url}?pull_url=${pullRequest.html_url}`;
+            commentBody += `- @${authorLogin}: you must [authorize the app](${fullAuthUrl}) before merging this pull request so the discussion can be created as you. This is not required every time.\n`;
           }
           commentBody +=
-            "- Do not use relative links to files in your repo. Instead, use full URLs and for media drag/drop or paste the file into the markdown. The link generated for media should contain `https://user-images.githubusercontent.com`\n";
+            "- Do not use relative links to files in your repo. Instead, use full URLs and for media drag/drop or paste the file into the markdown. The link generated for media should contain `https://user-images.githubusercontent.com`.\n";
 
           if (!mostRecentBotCommentForFile) {
             await appGitHubService.createPullRequestComment({
               ...pullInfo,
-              commit_id: payload.pull_request.head.sha,
+              commit_id: pullRequest.head.sha,
               start_line: 1,
               end_line: parsedMarkdown.headerEndLine,
               body: commentBody,
@@ -177,7 +180,7 @@ export class PullRequestEventHandler {
           } else {
             await appGitHubService.createPullRequestComment({
               ...pullInfo,
-              commit_id: payload.pull_request.head.sha,
+              commit_id: pullRequest.head.sha,
               start_line: 1,
               end_line: 1,
               body: errorMessage,
@@ -198,14 +201,16 @@ export class PullRequestEventHandler {
     const appGitHubService = GitHubService.buildForApp(context.octokit as unknown as OctokitPlus, logger, appConfig);
 
     const payload = context.payload;
+    const pullRepo = payload.repository;
+    const pullRequest = payload.pull_request;
     const pullInfo: PullInfo = {
-      owner: payload.repository.owner.login,
-      repo: payload.repository.name,
-      repoName: payload.repository.name,
-      pull_number: payload.pull_request.number,
+      owner: pullRepo.owner.login,
+      repo: pullRepo.name,
+      repoName: pullRepo.name,
+      pull_number: pullRequest.number,
     };
 
-    const isDefaultBranch = payload.pull_request.base.ref === payload.repository.default_branch;
+    const isDefaultBranch = pullRequest.base.ref === pullRepo.default_branch;
 
     if (!isDefaultBranch) {
       logger.info("The PR is not targeting the default branch, will not post anything");
@@ -215,7 +220,7 @@ export class PullRequestEventHandler {
     // Get pull request comments
     const app = await appGitHubService.getAuthenticatedApp();
     const appLogin = `${app.slug}[bot]`;
-    const optionalPullRequestFooterMarkdown = payload.repository.private ? "" : `<a href='${payload.pull_request.html_url}'>from a pull request</a> `;
+    const optionalPullRequestFooterMarkdown = pullRepo.private ? "" : `<a href='${pullRequest.html_url}'>from a pull request</a> `;
     const postFooter = `\n\n<hr /><em>This discussion was created ${optionalPullRequestFooterMarkdown}using <a href='${app.html_url}'>${app.name}</a>.</em>\n`;
 
     const pullRequestComments = await appGitHubService.getPullRequestComments({
@@ -233,7 +238,7 @@ export class PullRequestEventHandler {
     }
 
     // Assume the pull request author is the intended post author
-    const authorLogin = payload.pull_request.user.login;
+    const authorLogin = pullRequest.user.login;
     const authorToken = await this._tokenService.refreshUserToken(authorLogin);
 
     // For each file to post

diff --git a/web-app/src/services/authService.ts b/web-app/src/services/authService.ts
@@ -2,6 +2,7 @@ import { exchangeWebFlowCode, GitHubAppAuthenticationWithRefreshToken } from "@o
 import { AppConfig } from "../models/appConfig";
 import * as queryString from "query-string";
 import { DeprecatedLogger } from "probot/lib/types";
+import CryptoJS from "crypto-js";
 
 export class AuthService {
   private _appConfig: AppConfig;
@@ -20,9 +21,17 @@ export class AuthService {
     const protocol = req.headers["x-forwarded-proto"] || req.protocol;
     const host = req.headers["x-forwarded-host"] || req.get("host");
 
+    // Encrypt
+    const state = {
+      pull_url: req.query.pull_url,
+      github_client_id: this._appConfig.github_client_id,
+    };
+    const encryptedState = CryptoJS.AES.encrypt(JSON.stringify(state), this._appConfig.github_client_secret).toString();
+
     const params = queryString.stringify({
       client_id: this._appConfig.github_client_id,
       redirect_uri: `${protocol}://${host}${this._appConfig.github_callback_url}`,
+      state: encryptedState,
     });
 
     const url = `https://github.com/login/oauth/authorize?${params}`;

diff --git a/web-app/src/services/routerService.ts b/web-app/src/services/routerService.ts
@@ -3,13 +3,16 @@ import { Router } from "express";
 import { AppConfig } from "../models/appConfig";
 import { TokenService } from "./tokenService";
 import { AuthService } from "./authService";
+import CryptoJS from "crypto-js";
 
 export class RouterService {
   private _router: Router;
   private _logger: DeprecatedLogger;
   private _appConfig: AppConfig;
   private _tokenService: TokenService;
   private _authService: AuthService;
+  static readonly SECONDS_TO_REDIRECT = 6;
+  static readonly DEFAULT_REDIRECT = "https://github.com/philip-gai/announcement-drafter";
 
   private constructor(
     logger: DeprecatedLogger,
@@ -47,16 +50,63 @@ export class RouterService {
   }
 
   public addOAuthCallbackRoute(): this {
-    this._router.get(this._appConfig.github_callback_url, async (req, res) => {
+    this._router.get(this._appConfig.github_callback_url, async (req, res): Promise<void> => {
       this._logger.info(`Received OAuth callback...`);
-      const code = (req.query.code as string) || "Bad code";
-      this._logger.debug(`Code: ${code}`);
+      const code = req.query.code as string;
+      if (!code) {
+        res.status(400).send("No code");
+        return;
+      }
+
+      const encryptedState = req.query.state as string;
+      if (!encryptedState) {
+        res.status(400).send("No state");
+        return;
+      }
+
+      // Redirect to the DEFAULT_REDIRECT if no pull request url is found
+      let redirectUrl = RouterService.DEFAULT_REDIRECT;
+      try {
+        // Decrypt the state and validate it
+        const bytes = CryptoJS.AES.decrypt(encryptedState, this._appConfig.github_client_secret);
+
+        const state = JSON.parse(bytes.toString(CryptoJS.enc.Utf8));
+        if (state.github_client_id !== this._appConfig.github_client_id) {
+          res.status(400).send("Invalid state");
+          return;
+        }
+        // Get the pull request url from the state for redirection
+        if (state.pull_url) {
+          redirectUrl = state.pull_url;
+        }
+      } catch (error) {
+        res.status(400).send("Unable to parse state");
+        return;
+      }
 
       const { token, refreshToken, refreshTokenExpiresAt } = await this._authService.authenticateUser(code);
 
       await this._tokenService.upsertRefreshToken(token, refreshToken, refreshTokenExpiresAt);
 
-      res.redirect("/");
+      // Display authentication success message and redirect after SECONDS_TO_REDIRECT seconds
+      const redirectLocationText = redirectUrl !== RouterService.DEFAULT_REDIRECT ? "pull request" : "Announcement Drafter repository";
+      res.setHeader("Content-Type", "text/html");
+      res.status(200).send(`
+        <html>
+        <head>
+          <title>announcement-drafter | authorization</title>
+        </head>
+        <body>
+          <div><h2>Success! Now Announcement Drafter can create discussions for you 🚀</h2></div>
+          <div><p>Sending you back to the ${redirectLocationText} in just a few seconds...</p></div>
+        </body>
+        <script>
+          setTimeout(function () {
+            window.location = "${redirectUrl}";
+          }, ${RouterService.SECONDS_TO_REDIRECT * 1000})
+        </script>
+        </html>
+        `);
     });
     return this;
   }