Shielding subject folder (github#37690)

Peter Bengtsson · web-flow · commit ca1d7e438c56 · 2023-06-09T13:02:34.000Z
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -57,6 +57,7 @@ jobs:
               { name: 'rest', path: 'src/rest/tests', },
               { name: 'routing', path: 'tests/routing', },
               { name: 'search', path: 'src/search/tests', },
+              { name: 'shielding', path: 'src/shielding/tests', },
               context.payload.repository.full_name === 'github/docs-internal' &&
                 { name: 'translations', path: 'tests/translations', },
               { name: 'unit', path: 'tests/unit', },
diff --git a/middleware/index.js b/middleware/index.js
@@ -16,7 +16,6 @@ import {
   setLanguageFastlySurrogateKey,
 } from './set-fastly-surrogate-key.js'
 import handleErrors from '#src/observability/middleware/handle-errors.js'
-import handleInvalidPaths from '#src/observability/middleware/handle-invalid-paths.js'
 import handleNextDataPath from './handle-next-data-path.js'
 import detectLanguage from './detect-language.js'
 import reloadTree from './reload-tree.js'
@@ -66,8 +65,7 @@ import fastlyBehavior from './fastly-behavior.js'
 import mockVaPortal from './mock-va-portal.js'
 import dynamicAssets from './dynamic-assets.js'
 import contextualizeSearch from '#src/search/middleware/contextualize.js'
-import rateLimit from './rate-limit.js'
-import handleInvalidQuerystrings from '#src/observability/middleware/handle-invalid-query-strings.js'
+import shielding from '#src/shielding/middleware/index.js'
 
 const { DEPLOYMENT_ENV, NODE_ENV } = process.env
 const isTest = NODE_ENV === 'test' || process.env.GITHUB_ACTIONS === 'true'
@@ -201,9 +199,7 @@ export default function (app) {
   }
 
   // *** Early exits ***
-  app.use(rateLimit)
-  app.use(instrument(handleInvalidQuerystrings, './handle-invalid-querystrings'))
-  app.use(instrument(handleInvalidPaths, './handle-invalid-paths'))
+  app.use(shielding)
   app.use(instrument(handleNextDataPath, './handle-next-data-path'))
 
   // *** Security ***
diff --git a/src/shielding/middleware/handle-invalid-paths.js b/src/shielding/middleware/handle-invalid-paths.js
diff --git a/src/shielding/middleware/handle-invalid-query-strings.js b/src/shielding/middleware/handle-invalid-query-strings.js
@@ -1,4 +1,4 @@
-import statsd from '../lib/statsd.js'
+import statsd from '#src/observability/lib/statsd.js'
 import { noCacheControl, defaultCacheControl } from '../../../middleware/cache-control.js'
 
 const STATSD_KEY = 'middleware.handle_invalid_querystrings'
diff --git a/src/shielding/middleware/index.js b/src/shielding/middleware/index.js
@@ -0,0 +1,13 @@
+import express from 'express'
+
+import handleInvalidQuerystrings from './handle-invalid-query-strings.js'
+import handleInvalidPaths from './handle-invalid-paths.js'
+import rateLimit from './rate-limit.js'
+
+const router = express.Router()
+
+router.use(rateLimit)
+router.use(handleInvalidQuerystrings)
+router.use(handleInvalidPaths)
+
+export default router
diff --git a/src/shielding/middleware/rate-limit.js b/src/shielding/middleware/rate-limit.js
@@ -1,7 +1,7 @@
 import rateLimit from 'express-rate-limit'
 
 import statsd from '#src/observability/lib/statsd.js'
-import { noCacheControl } from './cache-control.js'
+import { noCacheControl } from '../../../middleware/cache-control.js'
 
 const EXPIRES_IN_AS_SECONDS = 60
 
diff --git a/src/shielding/tests/invalid-querystrings.js b/src/shielding/tests/invalid-querystrings.js
@@ -1,10 +1,9 @@
-import { describe } from '@jest/globals'
+import { get } from '../../../tests/helpers/e2etest.js'
 
-import { get } from '../helpers/e2etest.js'
 import {
   MAX_UNFAMILIAR_KEYS_BAD_REQUEST,
   MAX_UNFAMILIAR_KEYS_REDIRECT,
-} from '#src/observability/middleware/handle-invalid-query-strings.js'
+} from '#src/shielding/middleware/handle-invalid-query-strings.js'
 
 const alpha = Array.from(Array(26)).map((e, i) => i + 65)
 const alphabet = alpha.map((x) => String.fromCharCode(x))
diff --git a/src/shielding/tests/shielding.js b/src/shielding/tests/shielding.js
@@ -0,0 +1,76 @@
+import { get } from '../../../tests/helpers/e2etest.js'
+
+describe('honeypotting', () => {
+  test('any GET with survey-vote and survey-token query strings is 400', async () => {
+    const res = await get('/en?survey-vote=1&survey-token=2')
+    expect(res.statusCode).toBe(400)
+    expect(res.body).toMatch(/Honeypotted/)
+    expect(res.headers['cache-control']).toMatch('private')
+  })
+})
+
+describe('junk paths', () => {
+  test('junk full pathname', async () => {
+    const res = await get('/xmlrpc.php')
+    expect(res.statusCode).toBe(404)
+    expect(res.headers['content-type']).toMatch('text/plain')
+    expect(res.headers['cache-control']).toMatch('public')
+  })
+
+  test('junk base name', async () => {
+    const res = await get('/en/get-started/.env.local')
+    expect(res.statusCode).toBe(404)
+    expect(res.headers['content-type']).toMatch('text/plain')
+    expect(res.headers['cache-control']).toMatch('public')
+  })
+
+  test.each(['/_nextanything', '/_next/data', '/_next/data/'])(
+    'invalid requests for _next prefix %s',
+    async (path) => {
+      const res = await get(path)
+      expect(res.statusCode).toBe(404)
+      expect(res.headers['content-type']).toMatch('text/plain')
+      expect(res.headers['cache-control']).toMatch('public')
+    }
+  )
+
+  test('any URL that ends with /index.md redirects', async () => {
+    const res = await get('/en/get-started/index.md')
+    expect(res.statusCode).toBe(302)
+    expect(res.headers.location).toBe('/en/get-started')
+  })
+})
+
+describe('rate limiting', () => {
+  // We can't actually trigger a full rate limit because
+  // then all other tests will all fail. And we can't rely on this
+  // test always being run last.
+
+  test('only happens if you have junk query strings', async () => {
+    const res = await get('/robots.txt?foo=bar')
+    expect(res.statusCode).toBe(200)
+    const limit = parseInt(res.headers['ratelimit-limit'])
+    const remaining = parseInt(res.headers['ratelimit-remaining'])
+    expect(limit).toBeGreaterThan(0)
+    expect(remaining).toBeLessThan(limit)
+
+    // A second request
+    {
+      const res = await get('/robots.txt?foo=buzz')
+      expect(res.statusCode).toBe(200)
+      const newLimit = parseInt(res.headers['ratelimit-limit'])
+      const newRemaining = parseInt(res.headers['ratelimit-remaining'])
+      expect(newLimit).toBe(limit)
+      // Can't rely on `newRemaining == remaining - 1` because of
+      // concurrency of jest-running.
+      expect(newRemaining).toBeLessThan(remaining)
+    }
+  })
+
+  test('nothing happens if no unrecognized query string', async () => {
+    const res = await get('/robots.txt')
+    expect(res.statusCode).toBe(200)
+    expect(res.headers['ratelimit-limit']).toBeUndefined()
+    expect(res.headers['ratelimit-remaining']).toBeUndefined()
+  })
+})

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-import statsd from '../lib/statsd.js'`
	`1`	`+import statsd from '#src/observability/lib/statsd.js'`
`2`	`2`	`import { noCacheControl, defaultCacheControl } from '../../../middleware/cache-control.js'`
`3`	`3`
`4`	`4`	`const STATSD_KEY = 'middleware.handle_invalid_querystrings'`