diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7cd009631b..469ff7c72f 100644 --- 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 index 31451ce92d..02167f64c7 100644 --- 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/observability/middleware/handle-invalid-paths.js b/src/shielding/middleware/handle-invalid-paths.js similarity index 100% rename from src/observability/middleware/handle-invalid-paths.js rename to src/shielding/middleware/handle-invalid-paths.js diff --git a/src/observability/middleware/handle-invalid-query-strings.js b/src/shielding/middleware/handle-invalid-query-strings.js similarity index 98% rename from src/observability/middleware/handle-invalid-query-strings.js rename to src/shielding/middleware/handle-invalid-query-strings.js index 1ca0b8faf1..43a6e22648 100644 --- a/src/observability/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 new file mode 100644 index 0000000000..7f4b8f10ff --- /dev/null +++ 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/middleware/rate-limit.js b/src/shielding/middleware/rate-limit.js similarity index 98% rename from middleware/rate-limit.js rename to src/shielding/middleware/rate-limit.js index 88bf7e60d9..7ca847e305 100644 --- a/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/tests/rendering-fixtures/invalid-querystrings.js b/src/shielding/tests/invalid-querystrings.js similarity index 91% rename from tests/rendering-fixtures/invalid-querystrings.js rename to src/shielding/tests/invalid-querystrings.js index c40fd728f0..e26ab6c5af 100644 --- a/tests/rendering-fixtures/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 new file mode 100644 index 0000000000..307f98f47d --- /dev/null +++ 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() + }) +})