Carefully escape request & response data in performance explanations

pimterry · pimterry · commit a2b6c1e5337b · 2024-06-25T20:48:58.000+02:00
Previously, a carefully crafted header could plausibly have injected a
confusing message here like (click here for more info [link]) using an
HTML link or an autolinked URL, and maybe used this for phishing or
something. Very limited risk imo but definitely good to protect against
properly. We now filter all active markdown (even bold etc) from values
shown here, and disable autolinking for all those cases.
diff --git a/src/components/common/text-content.tsx b/src/components/common/text-content.tsx
@@ -144,11 +144,15 @@ export const Content = styled.div`
     }
 `;
 
-export const Markdown = (p: { content: string | undefined }) =>
+export const Markdown = (p: {
+    content: string | undefined,
+    linkify?: boolean
+}) =>
     p.content ?
         <ExternalContent htmlContent={fromMarkdown(
             p.content
                 .replace(/:suggestion:/g, suggestionIconHtml)
-                .replace(/:warning:/g, warningIconHtml)
+                .replace(/:warning:/g, warningIconHtml),
+            { linkify: p.linkify }
         )} />
     : null;
diff --git a/src/model/api/jsonrpc.ts b/src/model/api/jsonrpc.ts
@@ -130,7 +130,7 @@ export class JsonRpcApiService implements ApiService {
             ?? this.name.split(' ')[0];
 
         this.logoUrl = api.spec.info['x-logo']?.url;
-        this.description = fromMarkdown(api.spec.info.description);
+        this.description = fromMarkdown(api.spec.info.description, { linkify: true });
         this.docsUrl = api.spec.externalDocs?.url;
     }
 
@@ -154,7 +154,7 @@ export class JsonRpcApiOperation implements ApiOperation {
         this.description = fromMarkdown([
             methodSpec.summary,
             methodSpec.description
-        ].filter(x => !!x).join('\n\n'));
+        ].filter(x => !!x).join('\n\n'), { linkify: true });
         this.docsUrl = methodSpec.externalDocs?.url
             ?? (methodDocsBaseUrl
                 ? methodDocsBaseUrl + methodSpec.name.toLowerCase()
@@ -204,7 +204,7 @@ export class JsonRpcApiRequest implements ApiRequest {
                             ]
                             : []
                         )
-                    ].filter(x => !!x).join('\n\n')),
+                    ].filter(x => !!x).join('\n\n'), { linkify: true }),
                     in: 'body',
                     required: !!param.required,
                     deprecated: !!param.deprecated,
@@ -234,7 +234,7 @@ export class JsonRpcApiResponse implements ApiResponse {
     constructor(rpcMethod: MatchedOperation) {
         const resultSpec = rpcMethod.methodSpec.result as ContentDescriptorObject;
 
-        this.description = fromMarkdown(resultSpec.description);
+        this.description = fromMarkdown(resultSpec.description, { linkify: true });
         this.bodySchema = {
             type: 'object',
             properties: {
diff --git a/src/model/api/openapi.ts b/src/model/api/openapi.ts
@@ -103,7 +103,7 @@ export function getParameters(
                 specParam: param,
                 name: param.name,
                 in: param.in,
-                description: fromMarkdown(param.description),
+                description: fromMarkdown(param.description, { linkify: true }),
                 required: param.required || param.in === 'path',
                 type: schema && schema.type,
                 defaultValue: schema && schema.default,
@@ -333,7 +333,7 @@ class OpenApiService implements ApiService {
             ?? this.name.split(' ')[0];
 
         this.logoUrl = service['x-logo']?.url;
-        this.description = fromMarkdown(service.description);
+        this.description = fromMarkdown(service.description, { linkify: true });
         this.docsUrl = spec?.externalDocs?.url;
     }
 
@@ -383,14 +383,15 @@ class OpenApiOperation implements ApiOperation {
                     () => (get(op.spec, 'description', 'length') || Infinity) < 40, op.spec.description!
                 ],
                 op.pathSpec.summary
-            ) || `${op.method.toUpperCase()} ${op.path}`
+            ) || `${op.method.toUpperCase()} ${op.path}`,
+            { linkify: true }
         ).__html);
 
         this.description = fromMarkdown(firstMatch<string>(
             [() => get(op.spec, 'description') !== this.name, get(op.spec, 'description')],
             [() => get(op.spec, 'summary') !== this.name, get(op.spec, 'summary')],
             op.pathSpec.description
-        ));
+        ), { linkify: true });
 
         if (!op.matched) this.warnings.push(
             `Unknown operation '${this.name}'.`
@@ -449,7 +450,7 @@ class OpenApiResponse implements ApiResponse {
             : undefined;
 
         this.description = bodySpec && bodySpec.description !== response.statusMessage
-            ? fromMarkdown(bodySpec.description)
+            ? fromMarkdown(bodySpec.description, { linkify: true })
             : undefined;
         this.bodySchema = getBodySchema(spec, bodySpec, response);
     }
diff --git a/src/model/http/caching.ts b/src/model/http/caching.ts
@@ -10,6 +10,7 @@ import {
 import { HttpExchange, ExchangeMessage } from '../../types';
 import { lastHeader, asHeaderArray } from '../../util/headers';
 import { joinAnd } from '../../util/text';
+import { escapeForMarkdownEmbedding } from '../ui/markdown';
 
 // https://tools.ietf.org/html/draft-ietf-httpbis-semantics-04#section-7.2.3
 const CACHEABLE_METHODS = ['GET', 'HEAD', 'POST'];
@@ -53,7 +54,13 @@ const CORS_SIMPLE_HEADERS = [
 
 function formatHeader(headerName: string): string {
     // Upper case the first letter of each word (including hyphenated parts)
-    return headerName.toLowerCase().replace(/(\b\w)/g, v => v.toUpperCase())
+    return headerName.toLowerCase().replace(/(\b\w)/g, v => v.toUpperCase());
+}
+
+function escapeAndFormatHeader(headerName: string): string {
+    return `<code>${escapeForMarkdownEmbedding(
+        formatHeader(headerName)
+    )}</code>`;
 }
 
 const THE_DAWN_OF_TIME = parseDate(0);
@@ -126,9 +133,10 @@ export function explainCacheability(exchange: HttpExchange): (
                         mechanisms, but the CORS result itself can be cached if a
                         Access-Control-Max-Age header is provided.
 
-                        In this case that header is set to ${maxAgeHeader}, explicitly
-                        requesting that this result should not be cached, and that clients
-                        should not reuse this CORS response in future.
+                        In this case that header is set to ${escapeForMarkdownEmbedding(
+                            maxAgeHeader!
+                        )}, explicitly requesting that this result should not be cached,
+                        and that clients should not reuse this CORS response in future.
                     `
                 };
             }
@@ -146,7 +154,9 @@ export function explainCacheability(exchange: HttpExchange): (
             return {
                 cacheable: false,
                 summary: 'Not cacheable',
-                explanation: `${request.method} requests are never cacheable.`
+                explanation: `${escapeForMarkdownEmbedding(
+                    request.method
+                )} requests are never cacheable.`
             };
         }
     }
@@ -550,7 +560,7 @@ export function explainCacheMatching(exchange: HttpExchange): Explanation | unde
         const allowedHeaders = _.union(
             CORS_SIMPLE_HEADERS,
             asHeaderArray(response.headers['access-control-allow-headers'])
-                .map(formatHeader)
+                .map(escapeAndFormatHeader)
         );
         const allowsCredentials = response.headers['Access-Control-Allow-Credentials'] === 'true';
 
@@ -561,37 +571,44 @@ export function explainCacheMatching(exchange: HttpExchange): Explanation | unde
                 request for future CORS requests, when:
 
                 * The CORS request would be sent to the same URL
-                * The origin is \`${request.headers['origin']}\`
+                * The origin is <code>${escapeForMarkdownEmbedding(
+                    request.headers['origin'].toString()
+                )}</code>
                 ${!allowsCredentials ? '* No credentials are being sent\n' : ''
-                }* The request method would be ${joinAnd(allowedMethods, ', ', ' or ')}
+                }* The request method would be ${escapeForMarkdownEmbedding(
+                    joinAnd(allowedMethods, ', ', ' or ')
+                )}
                 * There are no extra request headers other than ${joinAnd(allowedHeaders)}
             `
         };
     }
 
-    const varyHeaders = asHeaderArray(response.headers['vary']).map(formatHeader);
-
-    const hasVaryHeaders = varyHeaders.length > 0;
+    const rawVaryHeaders = asHeaderArray(response.headers['vary']);
+    const hasVaryHeaders = rawVaryHeaders.length > 0;
 
     // Don't need to handle Vary: *, as that would've excluded cacheability entirely.
 
     const varySummary = hasVaryHeaders ?
         ` that have the same ${
-            joinAnd(varyHeaders)
-        } header${varyHeaders.length > 1 ? 's' : ''}`
+            joinAnd(rawVaryHeaders.map(h => `'${formatHeader(h)}'`)) // No escaping - summary (non-markdown) only
+        } header${rawVaryHeaders.length > 1 ? 's' : ''}`
         : '';
 
     const varyExplanation = hasVaryHeaders ? dedent`
-        , as long as those requests have ${joinAnd(varyHeaders.map(headerName => {
+        , as long as those requests have ${joinAnd(rawVaryHeaders.map(headerName => {
             const realHeaderValue = request.headers[headerName.toLowerCase()];
 
+            const formattedName = escapeAndFormatHeader(headerName);
+
             return realHeaderValue === undefined ?
-                `no ${headerName} header` :
-                `a ${headerName} header set to \`${realHeaderValue}\``
+                `no ${formattedName} header` :
+                `a ${formattedName} header set to <code>${escapeForMarkdownEmbedding(
+                    realHeaderValue.toString()
+                )}</code>`
         }))}.
 
-        ${varyHeaders.length > 1 ? 'These headers are' : 'This header is'}
-        required because ${varyHeaders.length > 1 ? "they're" : "it's"} listed in
+        ${rawVaryHeaders.length > 1 ? 'These headers are' : 'This header is'}
+        required because ${rawVaryHeaders.length > 1 ? "they're" : "it's"} listed in
         the Vary header of the response.
     ` : dedent`
         , regardless of header values or other factors.
@@ -760,7 +777,9 @@ export function explainCacheLifetime(exchange: HttpExchange): Explanation | unde
             some percentage of the time since the content was last modified, often using
             the Last-Modified header value${
                 response.headers['last-modified'] ?
-                    ` (${response.headers['last-modified']})`
+                    ` (<code>${escapeForMarkdownEmbedding(
+                        response.headers['last-modified'].toString()
+                    )}</code>)`
                     : ', although that is not explicitly defined in this response either'
             }
         ` :
@@ -770,11 +789,11 @@ export function explainCacheLifetime(exchange: HttpExchange): Explanation | unde
                     a \`max-age\` directive set to ${maxAge} seconds
                 ` :
                 dateHeader ? dedent `
-                    an Expires header set to ${expiresHeader}, which is
-                    not after its Date header value (${dateHeader})
+                    an Expires header set to ${escapeForMarkdownEmbedding(expiresHeader!.toString())}, which is
+                    not after its Date header value (${escapeForMarkdownEmbedding(dateHeader)})
                 `
                 : dedent`
-                    an Expires header set to ${expiresHeader}, which is
+                    an Expires header set to ${escapeForMarkdownEmbedding(expiresHeader!)}, which is
                     before the response was received
                 `
             }${explainSharedCacheLifetime}
@@ -784,7 +803,7 @@ export function explainCacheLifetime(exchange: HttpExchange): Explanation | unde
             as specified by its \`max-age\` directive${explainSharedCacheLifetime}
         `
         : dedent`
-            This response expires at ${expiresHeader} (after ${formatDuration(lifetime!)}),
+            This response expires at ${escapeForMarkdownEmbedding(expiresHeader!)} (after ${formatDuration(lifetime!)}),
             as specified by its Expires header${explainSharedCacheLifetime}
         `;
 
diff --git a/src/model/ui/markdown.ts b/src/model/ui/markdown.ts
@@ -3,12 +3,17 @@ import * as DOMPurify from 'dompurify';
 
 import { Html } from '../../types';
 
-const md = new Remarkable({
+const linkedMarkdown = new Remarkable({
     html: true,
     linkify: true,
     linkTarget: '_blank' // Links should always open elsewhere
 });
 
+const linklessMarkdown = new Remarkable({
+    html: true,
+    linkify: false
+});
+
 // Add an extra hook to DOMPurify to enforce link target. Without this, DOMPurify strips
 // every link target entirely.
 DOMPurify.addHook('afterSanitizeAttributes', function (node: Element | HTMLElement) {
@@ -38,13 +43,39 @@ DOMPurify.addHook('afterSanitizeAttributes', function (node: Element | HTMLEleme
     }
 });
 
-export function fromMarkdown(input: string): Html;
-export function fromMarkdown(input: string | undefined): Html | undefined;
-export function fromMarkdown(input: string | undefined): Html | undefined {
+export interface MarkdownRenderingOptions {
+    linkify?: boolean // False by default
+}
+
+export function fromMarkdown(input: string, options?: MarkdownRenderingOptions): Html;
+export function fromMarkdown(input: string | undefined, options?: MarkdownRenderingOptions): Html | undefined;
+export function fromMarkdown(input: string | undefined, options?: MarkdownRenderingOptions): Html | undefined {
     if (!input) return undefined;
     else {
+        const md = options?.linkify ? linkedMarkdown : linklessMarkdown;
         const unsafeMarkdown = md.render(input).replace(/\n$/, '');
         const safeHtml = DOMPurify.sanitize(unsafeMarkdown);
         return { __html: safeHtml };
     }
+}
+
+/**
+ * Takes an input string, and turns it into a value that will appear the same when
+ * rendered in markdown (and will not render any active HTML, e.g. links). This
+ * goes further than DOMPurify above by disabling _all_ non-plain text content.
+ *
+ * Important notes:
+ * - This escapes input for use as content, and doesn't cover cases like
+ *   escaping for HTML attribute values or similar.
+ * - This cannot fully escape _closing_ backticks - it's impossible to do this in
+ *   markdown, as even \\\` will close a previous \` block. Use &lt;code&gt; instead.
+ * - If linkify: true is used in later rendering, recognized URLs will still autolink.
+ */
+export function escapeForMarkdownEmbedding(input: string) {
+    const htmlEscaped = input
+        .replace(/&/g, '&amp;')
+        .replace(/</g, '&lt;')
+        .replace(/>/g, '&gt;');
+    const markdownEscaped = htmlEscaped.replace(/([\\`*_{}\[\]()#+\-.!~|])/g, '\\$1');
+    return markdownEscaped;
 }
diff --git a/test/unit/model/http/caching.spec.ts b/test/unit/model/http/caching.spec.ts
@@ -818,11 +818,11 @@ describe('Caching explanations', () => {
 
         it("should explain the detailed CORS matching behaviour", () => {
             const result = explainCacheMatching(exchange);
-            expect(result!.explanation).to.include('The origin is \`http://example2.com\`');
+            expect(result!.explanation).to.include('The origin is <code>http://example2\\.com</code>');
             expect(result!.explanation).to.include(
                 'The request method would be GET, HEAD, POST or DELETE'
             );
-            expect(result!.explanation).to.include('X-Header');
+            expect(result!.explanation).to.include('X\\-Header');
         });
 
         it("should say when it expires", () => {
@@ -936,14 +936,14 @@ describe('Caching explanations', () => {
         it("should include the Vary in the cache key", () => {
             const result = explainCacheMatching(exchange);
             expect(result!.summary).to.include(
-                'this URL that have the same Cookie header'
+                'this URL that have the same \'Cookie\' header'
             );
         });
 
         it("should explain the Vary in the cache key", () => {
             const result = explainCacheMatching(exchange);
             expect(result!.explanation).to.include(
-                'as long as those requests have a Cookie header set to `abc`'
+                'as long as those requests have a <code>Cookie</code> header set to <code>abc</code>'
             );
         });
     });
@@ -969,16 +969,17 @@ describe('Caching explanations', () => {
 
         it("should include the Vary in the cache key", () => {
             const result = explainCacheMatching(exchange);
-            expect(result!.summary).to.include(
-                'this URL that have the same Cookie, Accept-Language and Accept headers'
-            );
+            expect(result!.summary).to.include(dedent`
+                this URL that have the same 'Cookie', 'Accept-Language'
+                and 'Accept' headers
+            `.replace(/\n/g, ' '));
         });
 
         it("should explain the Vary in the cache key", () => {
             const result = explainCacheMatching(exchange);
             expect(result!.explanation).to.include(dedent`
-                as long as those requests have a Cookie header set to \`abc\`,
-                a Accept-Language header set to \`en\` and no Accept header
+                as long as those requests have a <code>Cookie</code> header set to <code>abc</code>,
+                a <code>Accept\-Language</code> header set to <code>en</code> and no <code>Accept</code> header
             `.replace(/\n/g, ' '));
         });
     });
diff --git a/test/unit/model/ui/markdown.spec.ts b/test/unit/model/ui/markdown.spec.ts
