Address Staticman issue eduardoboucas#416 - "Improve support for mult…

…iple environments (e.g., dev, staging, prod)" - Add configuration properties that allow for code promotion (without modification of staticman.yml) through different environments. - Add logic checks that help prevent cross-talk between tiers/layers of different environments. For example, Staticman dev and Mailgun prod. - Flag config properties as sensitive, where appropriate. - Prepend various artifacts such as commit messages, mailing list addresses, email subjects, etc. with a value to help identify the source environment. - Allow for the "entry" endpoint to be "locked down" to specific origins.
hispanic · Mar 21, 2021 · db62e71 · db62e71
1 parent e778686
commit db62e71
Show file tree

Hide file tree

Showing 9 changed files with 264 additions and 73 deletions.
diff --git a/config.js b/config.js
@@ -5,6 +5,12 @@ const path = require('path')
 
 const schema = {
   akismet: {
+    enabled: {
+      doc: 'Whether to use Akismet to check entries for spam. This requires an Akismet account to be configured. If either this value or the same-named value in the repository-based configuration is set to true, Akismet will be enabled.',
+      format: Boolean,
+      default: false,
+      env: 'AKISMET_ENABLED'
+    },
     site: {
       doc: 'URL of an Akismet account used for spam checking.',
       docExample: 'http://yourdomain.com',
@@ -16,13 +22,15 @@ const schema = {
       doc: 'API key to be used with Akismet.',
       format: String,
       default: null,
-      env: 'AKISMET_API_KEY'
+      env: 'AKISMET_API_KEY',
+      sensitive: true
     },
     bypassValue: {
-      doc: 'Value to pass in as comment author, author email, author URL, or content in order to bypass Akismet spam-checking. Intended to be used for testing in lieu of disabling Akismet via the site config.',
+      doc: 'Value to pass in as comment author, author email, author URL, or content in order to bypass Akismet spam-checking. Intended to be used for testing in lieu of disabling Akismet environment-wide via the configuration(s).',
       format: String,
       default: null,
-      env: 'AKISMET_BYPASS_VALUE'
+      env: 'AKISMET_BYPASS_VALUE',
+      sensitive: true
     }
   },
   analytics: {
@@ -34,15 +42,23 @@ const schema = {
       env: 'UA_TRACKING_ID'
     }
   },
+  branch: {
+    doc: 'Name of the branch in the target git service repository to be referenced. Will be overridden by a `branch` parameter in the site/repo config, if one is set.',
+    docExample: 'main',
+    format: String,
+    default: null,
+    env: 'BRANCH'
+  },
   email: {
     apiKey: {
-      doc: 'Mailgun API key to be used for email notifications. Will be overridden by a `notifications.apiKey` parameter in the site config, if one is set.',
+      doc: 'Mailgun API key to be used for email notifications. Will be overridden by a `notifications.apiKey` parameter in the site/repo config, if one is set.',
       format: String,
       default: null,
-      env: 'EMAIL_API_KEY'
+      env: 'EMAIL_API_KEY',
+      sensitive: true
     },
     domain: {
-      doc: 'Domain to be used with Mailgun for email notifications. Will be overridden by a `notifications.domain` parameter in the site config, if one is set.',
+      doc: 'Domain to be used with Mailgun for email notifications. Will be overridden by a `notifications.domain` parameter in the site/repo config, if one is set.',
       format: String,
       default: 'staticman.net',
       env: 'EMAIL_DOMAIN'
@@ -66,6 +82,18 @@ const schema = {
     default: 'development',
     env: 'NODE_ENV'
   },
+  exeEnv: {
+    doc: 'Identifies the application execution environment, which is allowed to deviate from NODE_ENV. Typically a shorter value that is prepended to commit messages, mailing list addresses, email subjects, etc. to help identify the source environment.',
+    format: String,
+    default: null,
+    env: 'EXE_ENV'
+  },
+  exeEnvProduction: {
+    doc: 'The value for exeEnv that identifies the production environment. In instances where the value for exeEnv is made visible, in the production environment, it won\'t be.',
+    format: String,
+    default: 'prod',
+    env: 'EXE_ENV_PRODUCTION'
+  },
   githubAccessTokenUri: {
     doc: 'URI for the GitHub authentication provider.',
     format: String,
@@ -96,6 +124,13 @@ const schema = {
     default: null,
     env: 'GITHUB_TOKEN'
   },
+  githubWebhookSecret: {
+    doc: 'Token to verify that webhook requests are from GitHub. Will be overridden by a `githubWebhookSecret` parameter in the site/repo config, if one is set.',
+    format: 'String',
+    default: null,
+    env: 'GITHUB_WEBHOOK_SECRET',
+    sensitive: true
+  },
   gitlabAccessTokenUri: {
     doc: 'URI for the GitLab authentication provider.',
     format: String,
@@ -114,6 +149,20 @@ const schema = {
     default: null,
     env: 'GITLAB_TOKEN'
   },
+  gitlabWebhookSecret: {
+    doc: 'Token to verify that webhook requests are from GitLab. Will be overridden by a `gitlabWebhookSecret` parameter in the site/repo config, if one is set.',
+    format: 'String',
+    default: null,
+    env: 'GITLAB_WEBHOOK_SECRET',
+    sensitive: true
+  },
+  origins: {
+    doc: 'CORS-compliant origins which are allowed to make requests against the \'entry\' endpoint. Regular expressions supported. In the case of a mismatch, a 403 response is returned. This is not intended to act as a security measure. Rather, it is meant to help avoid misconfigurations of the sort where a dev site is accidentally pointed at a prod staticman.',
+    docExample: 'http://localhost:.*',
+    format: Array,
+    default: null,
+    env: 'ORIGINS'
+  },
   port: {
     doc: 'The port to bind the application to.',
     format: 'port',
@@ -125,14 +174,16 @@ const schema = {
     docExample: 'rsaPrivateKey: "-----BEGIN RSA PRIVATE KEY-----\\nkey\\n-----END RSA PRIVATE KEY-----"',
     format: String,
     default: null,
-    env: 'RSA_PRIVATE_KEY'
+    env: 'RSA_PRIVATE_KEY',
+    sensitive: true
   },
   cryptoPepper: {
     doc: 'Shared (app-wide) secret that can be used to verify the authenticity of hashed/encrypted strings that we create. Should be long to defend against brute force attacks.',
     docExample: 'bcacf76bef428bf6115abfaa664e73481657e5068b9534227dca6ec96c6931b113105be81cb177b4e22d42fbc32d04ea5a8133e97296de7852328',
     format: String,
     default: null,
-    env: 'CRYPTO_PEPPER'
+    env: 'CRYPTO_PEPPER',
+    sensitive: true
   },
   logging: {
     slackWebhook: {

diff --git a/controllers/confirmSubscription.js b/controllers/confirmSubscription.js
@@ -36,6 +36,10 @@ module.exports = async (req, res, next) => {
       throw new Error('Authenticity check failed.')
     }
 
+    if (confirmData === null || confirmData.exeEnv !== config.get('exeEnv')) {
+      throw new Error('Environment check failed.')
+    }
+
     const staticman = await new Staticman(req.params)
     staticman.setConfigPath()
 

diff --git a/controllers/webhook.js b/controllers/webhook.js
@@ -18,6 +18,7 @@ module.exports = async (req, res, next) => {
   let service = req.params.service
   const version = req.params.version
   let staticman = null
+  let configBranch = null
   // v1 of the webhook endpoint assumed GitHub.
   if (!service && version === '1') {
     service = 'github'
@@ -29,21 +30,34 @@ module.exports = async (req, res, next) => {
     service = req.params.service
     staticman = await new Staticman(req.params)
     staticman.setConfigPath()
+
+    await staticman.getSiteConfig().then((siteConfig) => {
+      configBranch = siteConfig.get('branch') || config.get('branch')
+    }).catch((error) => {
+      errorsRaised = errorsRaised.concat(error)
+    })
+
+    if (configBranch && (req.params.branch !== configBranch)) {
+      console.log(`Branch check failed - configBranch = ${configBranch}, paramsBranch = ${req.params.branch}`)
+      errorsRaised.push('Branch mismatch. Ignoring request.')
+    }
   }
 
-  switch (service) {
-    case 'github':
-      await _handleWebhookGitHub(req, service, staticman).catch((errors) => {
-        errorsRaised = errorsRaised.concat(errors)
-      })
-      break
-    case 'gitlab':
-      await _handleWebhookGitLab(req, service, staticman).catch((errors) => {
-        errorsRaised = errorsRaised.concat(errors)
-      })
-      break
-    default:
-      errorsRaised.push('Unexpected service specified.')
+  if (errorsRaised.length === 0) {
+    switch (service) {
+      case 'github':
+        await _handleWebhookGitHub(req, service, staticman, configBranch).catch((errors) => {
+          errorsRaised = errorsRaised.concat(errors)
+        })
+        break
+      case 'gitlab':
+        await _handleWebhookGitLab(req, service, staticman, configBranch).catch((errors) => {
+          errorsRaised = errorsRaised.concat(errors)
+        })
+        break
+      default:
+        errorsRaised.push('Unexpected service specified.')
+    }
   }
 
   if (errorsRaised.length > 0) {
@@ -57,7 +71,7 @@ module.exports = async (req, res, next) => {
   }
 }
 
-const _handleWebhookGitHub = async function (req, service, staticman) {
+const _handleWebhookGitHub = async function (req, service, staticman, configBranch) {
   let errorsRaised = []
 
   const event = req.headers['x-github-event']
@@ -69,7 +83,7 @@ const _handleWebhookGitHub = async function (req, service, staticman) {
       if (staticman) {
         // Webhook request authentication is NOT supported in v1 of the endpoint.
         await staticman.getSiteConfig().then((siteConfig) => {
-          webhookSecretExpected = siteConfig.get('githubWebhookSecret')
+          webhookSecretExpected = siteConfig.get('githubWebhookSecret') || config.get('githubWebhookSecret')
         })
       }
 
@@ -89,7 +103,7 @@ const _handleWebhookGitHub = async function (req, service, staticman) {
       }
 
       if (reqAuthenticated) {
-        await _handleMergeRequest(req.params, service, req.body, staticman).catch((errors) => {
+        await _handleMergeRequest(req.params, service, req.body, staticman, configBranch).catch((errors) => {
           errorsRaised = errors
         })
       }
@@ -101,7 +115,7 @@ const _handleWebhookGitHub = async function (req, service, staticman) {
   }
 }
 
-const _handleWebhookGitLab = async function (req, service, staticman) {
+const _handleWebhookGitLab = async function (req, service, staticman, configBranch) {
   let errorsRaised = []
 
   const event = req.headers['x-gitlab-event']
@@ -113,7 +127,7 @@ const _handleWebhookGitLab = async function (req, service, staticman) {
       if (staticman) {
         // Webhook request authentication is NOT supported in v1 of the endpoint.
         await staticman.getSiteConfig().then((siteConfig) => {
-          webhookSecretExpected = siteConfig.get('gitlabWebhookSecret')
+          webhookSecretExpected = siteConfig.get('gitlabWebhookSecret') || config.get('gitlabWebhookSecret')
         })
       }
 
@@ -137,7 +151,7 @@ const _handleWebhookGitLab = async function (req, service, staticman) {
       }
 
       if (reqAuthenticated) {
-        await _handleMergeRequest(req.params, service, req.body, staticman).catch((errors) => {
+        await _handleMergeRequest(req.params, service, req.body, staticman, configBranch).catch((errors) => {
           errorsRaised = errors
         })
       }
@@ -154,51 +168,21 @@ const _verifyGitHubSignature = function (secret, data, signature) {
   return bufferEq(Buffer.from(signature), Buffer.from(signedData))
 }
 
-const _handleMergeRequest = async function (params, service, data, staticman) {
+const _handleMergeRequest = async function (params, service, data, staticman, configBranch) {
   // Allow for multiple errors to be raised and reported back.
   const errors = []
 
   const ua = config.get('analytics.uaTrackingId')
     ? require('universal-analytics')(config.get('analytics.uaTrackingId'))
     : null
 
-  const version = params.version
-  let username = params.username
-  let repository = params.repository
-  let branch = params.branch
-
-  let gitService = null
   let mergeReqNbr = null
+  let webhookBranch = null
   if (service === 'github') {
-    /*
-     * In v1 of the endpoint, the service, username, repository, and branch parameters were
-     * ommitted. As such, if not provided in the webhook request URL, pull them from the webhook
-     * payload.
-     */
-    if (username === null || typeof username === 'undefined') {
-      username = data.repository.owner.login
-    }
-    if (repository === null || typeof repository === 'undefined') {
-      repository = data.repository.name
-    }
-    if (branch === null || typeof branch === 'undefined') {
-      branch = data.pull_request.base.ref
-    }
-
-    gitService = await gitFactory.create('github', {
-      version: version,
-      username: username,
-      repository: repository,
-      branch: branch
-    })
+    webhookBranch = data.pull_request.base.ref
     mergeReqNbr = data.number
   } else if (service === 'gitlab') {
-    gitService = await gitFactory.create('gitlab', {
-      version: version,
-      username: username,
-      repository: repository,
-      branch: branch
-    })
+    webhookBranch = data.object_attributes.target_branch
     mergeReqNbr = data.object_attributes.iid
   } else {
     errors.push('Unable to determine service.')
@@ -210,12 +194,18 @@ const _handleMergeRequest = async function (params, service, data, staticman) {
     return Promise.reject(errors)
   }
 
+  const gitService = await _buildGitService(params, service, configBranch, webhookBranch).catch((error) => {
+    errors.push(error)
+    return Promise.reject(errors)
+  })
+
   let review = await gitService.getReview(mergeReqNbr).catch((error) => {
     const msg = `Failed to retrieve merge request ${mergeReqNbr} - ${error}`
     console.error(msg)
     errors.push(msg)
     return Promise.reject(errors)
   })
+  //console.log('review = %o', review)
 
   /*
    * We might receive "real" (non-bot) pull requests for files other than Staticman-processed
@@ -252,6 +242,53 @@ const _handleMergeRequest = async function (params, service, data, staticman) {
   }
 }
 
+const _buildGitService = async function (params, service, configBranch, webhookBranch) {
+  const version = params.version
+  let username = params.username
+  let repository = params.repository
+  let branch = params.branch
+
+  if (service === 'github') {
+    /*
+     * In v1 of the endpoint, the service, username, repository, and branch parameters were
+     * omitted. As such, if not provided in the webhook request URL, pull them from the webhook
+     * payload.
+     */
+    if (username === null || typeof username === 'undefined') {
+      username = data.repository.owner.login
+    }
+    if (repository === null || typeof repository === 'undefined') {
+      repository = data.repository.name
+    }
+    if (branch === null || typeof branch === 'undefined') {
+      branch = data.pull_request.base.ref
+    }
+  }
+
+  let gitService = null
+  /*
+   * A merge request processed (i.e., opened, merged, closed) against one branch in a repository 
+   * will trigger ALL webhooks triggered by merge request events in that repository. Meaning, 
+   * the webhook controller running in a (for example) prod Staticman instance will receive 
+   * webhook calls triggered by merge request events against a (for example) dev branch. As such, 
+   * we should expect plenty of extraneous webhook requests. The critical criterion is the branch 
+   * in the webhook payload matching the branch specified in the configuration.
+   */
+  if ((configBranch && (configBranch !== webhookBranch)) || branch !== webhookBranch) {
+    console.log(`Merge branch mismatch - configBranch = ${configBranch}, webhookBranch = ${webhookBranch}, paramsBranch = ${branch}`)
+    return Promise.reject('Merge branch mismatch. Ignoring request.')
+  } else {
+    gitService = await gitFactory.create(service, {
+      version: version,
+      username: username,
+      repository: repository,
+      branch: branch
+    })
+  }
+
+  return gitService
+}
+
 const _createNotifyMailingList = async function (review, staticman, ua) {
   /*
    * The "staticman_notification" comment section of the pull/merge request comment only