Skip to content

repo sync #25590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions middleware/handle-invalid-querystrings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import statsd from '../lib/statsd.js'
import { noCacheControl, defaultCacheControl } from './cache-control.js'

const STATSD_KEY = 'middleware.handle_invalid_querystrings'

// Exported for the sake of end-to-end tests
export const MAX_UNFAMILIAR_KEYS_BAD_REQUEST = 15
export const MAX_UNFAMILIAR_KEYS_REDIRECT = 3

const RECOGNIZED_KEYS_BY_PREFIX = {
'/_next/data/': ['versionId', 'productId', 'restPage', 'apiVersion', 'category', 'subcategory'],
'/api/search': ['query', 'language', 'version', 'page', 'product', 'autocomplete', 'limit'],
'/api/anchor-redirect': ['hash', 'path'],
'/api/webhooks': ['category', 'version'],
'/api/pageinfo': ['pathname'],
}

const RECOGNIZED_KEYS_BY_ANY = new Set([
// Learning track pages
'learn',
'learnProduct',
// Platform picker
'platform',
// Tool picker
'tool',
// When apiVersion isn't the only one. E.g. ?apiVersion=XXX&tool=vscode
'apiVersion',
// Search
'query',
// The drop-downs on "Webhook events and payloads"
'actionType',
])

export default function handleInvalidQuerystrings(req, res, next) {
const { method, query, path } = req
if (method === 'GET' || method === 'HEAD') {
const originalKeys = Object.keys(query)
let keys = originalKeys.filter((key) => !RECOGNIZED_KEYS_BY_ANY.has(key))
if (keys.length > 0) {
// Before we judge the number of query strings, strip out all the ones
// we're familiar with.
for (const [prefix, recognizedKeys] of Object.entries(RECOGNIZED_KEYS_BY_PREFIX)) {
if (path.startsWith(prefix)) {
keys = keys.filter((key) => !recognizedKeys.includes(key))
}
}
}

if (keys.length >= MAX_UNFAMILIAR_KEYS_BAD_REQUEST) {
noCacheControl(res)

res.status(400).send('Too many unrecognized query string parameters')

const tags = [
'response:400',
`url:${req.url}`,
`ip:${req.ip}`,
`path:${req.path}`,
`keys:${originalKeys.length}`,
]
statsd.increment(STATSD_KEY, 1, tags)

return
}

if (keys.length >= MAX_UNFAMILIAR_KEYS_REDIRECT) {
defaultCacheControl(res)
const sp = new URLSearchParams(query)
keys.forEach((key) => sp.delete(key))
let newURL = req.path
if (sp.toString()) newURL += `?${sp}`

res.redirect(302, newURL)

const tags = [
'response:302',
`url:${req.url}`,
`ip:${req.ip}`,
`path:${req.path}`,
`keys:${originalKeys.length}`,
]
statsd.increment(STATSD_KEY, 1, tags)

return
}
}

return next()
}
2 changes: 2 additions & 0 deletions middleware/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ import fastlyBehavior from './fastly-behavior.js'
import mockVaPortal from './mock-va-portal.js'
import dynamicAssets from './dynamic-assets.js'
import rateLimit from './rate-limit.js'
import handleInvalidQuerystrings from './handle-invalid-querystrings.js'

const { DEPLOYMENT_ENV, NODE_ENV } = process.env
const isTest = NODE_ENV === 'test' || process.env.GITHUB_ACTIONS === 'true'
Expand Down Expand Up @@ -198,6 +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(instrument(handleNextDataPath, './handle-next-data-path'))

Expand Down
47 changes: 47 additions & 0 deletions tests/rendering-fixtures/invalid-querystrings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { describe } from '@jest/globals'

import { get } from '../helpers/e2etest.js'
import {
MAX_UNFAMILIAR_KEYS_BAD_REQUEST,
MAX_UNFAMILIAR_KEYS_REDIRECT,
} from '../../middleware/handle-invalid-querystrings.js'

const alpha = Array.from(Array(26)).map((e, i) => i + 65)
const alphabet = alpha.map((x) => String.fromCharCode(x))

describe('invalid query strings', () => {
test('400 for too many unrecognized query strings', async () => {
// This test depends on knowing exactly the number
// of unrecognized query strings that will trigger a 400.
const sp = new URLSearchParams()
alphabet.slice(0, MAX_UNFAMILIAR_KEYS_BAD_REQUEST).forEach((letter) => sp.set(letter, '1'))
const url = `/?${sp}`
const res = await get(url)
expect(res.statusCode).toBe(400)
expect(res.headers['cache-control']).toMatch('no-store')
expect(res.headers['cache-control']).toMatch('private')
})

test('302 redirect for many unrecognized query strings', async () => {
// This test depends on knowing exactly the number
// of unrecognized query strings that will trigger a 400.
const sp = new URLSearchParams()
alphabet.slice(0, MAX_UNFAMILIAR_KEYS_REDIRECT).forEach((letter) => sp.set(letter, '1'))
const url = `/?${sp}`
const res = await get(url)
expect(res.statusCode).toBe(302)
expect(res.headers.location).toBe('/')
expect(res.headers['cache-control']).toMatch(/max-age=[1-9]/)
expect(res.headers['cache-control']).toMatch('public')
})

test('302 redirect but keeping recognized query strings', async () => {
const sp = new URLSearchParams()
alphabet.slice(0, MAX_UNFAMILIAR_KEYS_REDIRECT).forEach((letter) => sp.set(letter, '1'))
sp.set('platform', 'concrete')
const url = `/en/pages?${sp}`
const res = await get(url)
expect(res.statusCode).toBe(302)
expect(res.headers.location).toBe('/en/pages?platform=concrete')
})
})