Fix reflected XSS in shielding middleware (#59958)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: heiskr

src/shielding/middleware/handle-invalid-paths.ts

@@ -79,8 +79,7 @@ export default function handleInvalidPaths(
     // We can all the CDN to cache these responses because they're
     // they're not going to suddenly work in the next deployment.
     defaultCacheControl(res)
-    res.setHeader('content-type', 'text/plain')
-    res.status(404).send('Not found')
+    res.status(404).type('text').send('Not found')
     return
   }
 

src/shielding/middleware/handle-invalid-query-string-values.ts

@@ -71,9 +71,9 @@ export default function handleInvalidQuerystringValues(
 
       // For example ?foo[bar]=baz (but not ?foo=bar&foo=baz)
       if (value instanceof Object && !Array.isArray(value)) {
-        const message = `Invalid query string key (${key})`
+        const message = 'Invalid query string'
         defaultCacheControl(res)
-        res.status(400).send(message)
+        res.status(400).type('text').send(message)
 
         const tags = ['response:400', `path:${req.path}`, `key:${key}`]
         statsd.increment(STATSD_KEY, 1, tags)

src/shielding/middleware/handle-invalid-query-strings.ts

@@ -70,8 +70,7 @@ export default function handleInvalidQuerystrings(
 
     if (invalidKeys.length > 0) {
       noCacheControl(res)
-      const invalidKey = invalidKeys[0].replace(/\[.*$/, '') // Get the base key name
-      res.status(400).send(`Invalid query string key (${invalidKey})`)
+      res.status(400).type('text').send('Invalid query string')
 
       const tags = [
         'response:400',
@@ -105,7 +104,7 @@ export default function handleInvalidQuerystrings(
       noCacheControl(res)
 
       const message = honeypotted ? 'Honeypotted' : 'Too many unrecognized query string parameters'
-      res.status(400).send(message)
+      res.status(400).type('text').send(message)
 
       const tags = [
         'response:400',

src/shielding/middleware/handle-malformed-urls.ts

@@ -22,8 +22,7 @@ export default function handleMalformedUrls(
   } catch {
     // If any decoding fails, this is a malformed URL
     defaultCacheControl(res)
-    res.setHeader('content-type', 'text/plain')
-    res.status(400).send('Bad Request: Malformed URL')
+    res.status(400).type('text').send('Bad Request: Malformed URL')
     return
   }
 

src/shielding/tests/invalid-querystrings.ts

@@ -21,6 +21,7 @@ describe('invalid query strings', () => {
     const url = `/?${sp}`
     const res = await get(url)
     expect(res.statusCode).toBe(400)
+    expect(res.headers['content-type']).toMatch('text/plain')
     expect(res.headers['cache-control']).toMatch('no-store')
     expect(res.headers['cache-control']).toMatch('private')
   })
@@ -69,14 +70,20 @@ describe('invalid query strings', () => {
     const url = `/en?query[foo]=bar`
     const res = await get(url)
     expect(res.statusCode).toBe(400)
-    expect(res.body).toMatch('Invalid query string key (query)')
+    expect(res.headers['content-type']).toMatch('text/plain')
+    expect(res.body).toMatch('Invalid query string')
+    // Must not reflect the user-supplied key name
+    expect(res.body).not.toContain('(query)')
   })
 
   test('query string keys with square brackets', async () => {
     const url = `/?constructor[foo][bar]=buz`
     const res = await get(url)
     expect(res.statusCode).toBe(400)
-    expect(res.body).toMatch('Invalid query string key (constructor)')
+    expect(res.headers['content-type']).toMatch('text/plain')
+    expect(res.body).toMatch('Invalid query string')
+    // Must not reflect the user-supplied key name
+    expect(res.body).not.toContain('(constructor)')
   })
 
   test('bad tool query string with Chinese URL-encoded characters', async () => {
@@ -86,6 +93,14 @@ describe('invalid query strings', () => {
     expect(res.statusCode).toBe(302)
     expect(res.headers.location).toBe('/?tool=azure_data_studio')
   })
+
+  test('XSS payloads in bracket query keys are not reflected', async () => {
+    const res = await get('/en?%3Cscript%3Ealert()%3C/script%3E[]')
+    expect(res.statusCode).toBe(400)
+    expect(res.headers['content-type']).toMatch('text/plain')
+    expect(res.body).not.toContain('<script>')
+    expect(res.body).not.toContain('alert')
+  })
 })
 
 function randomCharacters(length: number) {

src/shielding/tests/middleware-security.ts

@@ -0,0 +1,59 @@
+import fs from 'fs'
+import path from 'path'
+
+import { describe, expect, test } from 'vitest'
+
+const middlewareDir = path.join(__dirname, '..', 'middleware')
+
+describe('shielding middleware security patterns', () => {
+  const middlewareFiles = fs.readdirSync(middlewareDir).filter((f) => f.endsWith('.ts'))
+
+  test("every .send() call uses .type('text')", () => {
+    const violations: string[] = []
+
+    for (const file of middlewareFiles) {
+      // index.ts is just the router, skip it
+      if (file === 'index.ts') continue
+
+      const content = fs.readFileSync(path.join(middlewareDir, file), 'utf-8')
+      const lines = content.split('\n')
+
+      for (let i = 0; i < lines.length; i++) {
+        const line = lines[i]
+        if (line.includes('.send(') && !line.includes(".type('text')")) {
+          violations.push(`${file}:${i + 1}: ${line.trim()}`)
+        }
+      }
+    }
+
+    expect(
+      violations,
+      `All .send() calls in shielding middleware must use .type('text') to prevent XSS.\n` +
+        `Violations:\n${violations.join('\n')}`,
+    ).toHaveLength(0)
+  })
+
+  test('no .send() call reflects user input via template literals', () => {
+    const violations: string[] = []
+
+    for (const file of middlewareFiles) {
+      if (file === 'index.ts') continue
+
+      const content = fs.readFileSync(path.join(middlewareDir, file), 'utf-8')
+      const lines = content.split('\n')
+
+      for (let i = 0; i < lines.length; i++) {
+        const line = lines[i]
+        if (line.includes('.send(') && line.includes('${')) {
+          violations.push(`${file}:${i + 1}: ${line.trim()}`)
+        }
+      }
+    }
+
+    expect(
+      violations,
+      `Shielding middleware must not reflect user input in .send() responses.\n` +
+        `Violations:\n${violations.join('\n')}`,
+    ).toHaveLength(0)
+  })
+})

src/shielding/tests/shielding.ts

@@ -7,6 +7,7 @@ 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.headers['content-type']).toMatch('text/plain')
     expect(res.body).toMatch(/Honeypotted/)
     expect(res.headers['cache-control']).toMatch('private')
   })