From fb0adba0c1453e5002f54cde396b0873dd160f8d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Nov 2023 10:18:10 +0100 Subject: [PATCH 1/4] feat(astro): Add distributed tracing via `` tags --- packages/astro/src/server/meta.ts | 43 ++++++++ packages/astro/src/server/middleware.ts | 31 +++++- packages/astro/test/server/meta.test.ts | 103 ++++++++++++++++++ packages/astro/test/server/middleware.test.ts | 97 ++++++++++++++++- 4 files changed, 266 insertions(+), 8 deletions(-) create mode 100644 packages/astro/src/server/meta.ts create mode 100644 packages/astro/test/server/meta.test.ts diff --git a/packages/astro/src/server/meta.ts b/packages/astro/src/server/meta.ts new file mode 100644 index 000000000000..71e1cf05465a --- /dev/null +++ b/packages/astro/src/server/meta.ts @@ -0,0 +1,43 @@ +import type { Hub } from '@sentry/core'; +import { getDynamicSamplingContextFromClient } from '@sentry/core'; +import type { Span } from '@sentry/types'; +import { dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader } from '@sentry/utils'; + +/** + * Extracts the tracing data from the current span or from the client's scope + * (via transaction or propagation context) and renders the data to tags. + * + * This function creates two serialized tags: + * - `` + * - `` + * + * TODO: Extract this later on and export it from the Core or Node SDK + * + * @param span the currently active span + * @param client the SDK's client + * + * @returns an object with the two serialized tags + */ +export function getTracingMetaTags(span: Span | undefined, hub: Hub): { sentryTrace: string; baggage?: string } { + const scope = hub.getScope(); + const client = hub.getClient(); + const { dsc, sampled, traceId } = scope.getPropagationContext(); + const transaction = span?.transaction; + + const sentryTrace = span ? span.toTraceparent() : generateSentryTraceHeader(traceId, undefined, sampled); + + const dynamicSamplingContext = transaction + ? transaction.getDynamicSamplingContext() + : dsc + ? dsc + : client + ? getDynamicSamplingContextFromClient(traceId, client, scope) + : undefined; + + const baggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + + return { + sentryTrace: ``, + baggage: baggage ? `` : undefined, + }; +} diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index ff4ac1d44c78..999a223262d6 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -1,7 +1,9 @@ -import { captureException, configureScope, startSpan } from '@sentry/node'; +import { captureException, configureScope, getCurrentHub, startSpan } from '@sentry/node'; import { addExceptionMechanism, objectify, stripUrlQueryAndFragment, tracingContextFromHeaders } from '@sentry/utils'; import type { APIContext, MiddlewareResponseHandler } from 'astro'; +import { getTracingMetaTags } from './meta'; + type MiddlewareOptions = { /** * If true, the client IP will be attached to the event by calling `setUser`. @@ -93,11 +95,30 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH }, }, async span => { - const res = await next(); - if (span && res.status) { - span.setHttpStatus(res.status); + const originalResponse = await next(); + if (span && originalResponse.status) { + span.setHttpStatus(originalResponse.status); + } + + const hub = getCurrentHub(); + const client = hub.getClient(); + const contentType = originalResponse.headers.get('content-type'); + + const isPageloadRequest = contentType && contentType.startsWith('text/html'); + if (!isPageloadRequest || !client) { + return originalResponse; } - return res; + + const html = await originalResponse.text(); + if (typeof html !== 'string' || !html.includes('')) { + return originalResponse; + } + + const { sentryTrace, baggage } = getTracingMetaTags(span, hub); + const content = `\n${sentryTrace}\n${baggage}\n`; + const modifiedHtml = html.replace('', content); + + return new Response(modifiedHtml, originalResponse); }, ); return res; diff --git a/packages/astro/test/server/meta.test.ts b/packages/astro/test/server/meta.test.ts new file mode 100644 index 000000000000..0679b10b5db8 --- /dev/null +++ b/packages/astro/test/server/meta.test.ts @@ -0,0 +1,103 @@ +import * as SentryCore from '@sentry/core'; +import { vi } from 'vitest'; + +import { getTracingMetaTags } from '../../src/server/meta'; + +const mockedSpan = { + toTraceparent: () => '123', + transaction: { + getDynamicSamplingContext: () => ({ + environment: 'production', + }), + }, +}; + +const mockedHub = { + getScope: () => ({ + getPropagationContext: () => ({ + traceId: '123', + }), + }), + getClient: () => ({}), +}; + +describe('getTracingMetaTags', () => { + it('returns the tracing tags from the span, if it is provided', () => { + { + // @ts-expect-error - only passing a partial span object + const tags = getTracingMetaTags(mockedSpan, mockedHub); + + expect(tags).toEqual({ + sentryTrace: '', + baggage: '', + }); + } + }); + + it('returns propagationContext DSC data if no span is available', () => { + const tags = getTracingMetaTags(undefined, { + ...mockedHub, + // @ts-expect-error - only passing a partial scope object + getScope: () => ({ + getPropagationContext: () => ({ + traceId: 'abc', + sampled: true, + spanId: '456', + dsc: { + environment: 'staging', + public_key: 'key', + trace_id: 'abc', + }, + }), + }), + }); + + expect(tags).toEqual({ + sentryTrace: expect.stringMatching(/', + }); + }); + + it('returns only the `sentry-trace` tag if no DSC is available', () => { + vi.spyOn(SentryCore, 'getDynamicSamplingContextFromClient').mockReturnValueOnce({ + trace_id: '', + public_key: undefined, + }); + + const tags = getTracingMetaTags( + // @ts-expect-error - only passing a partial span object + { + toTraceparent: () => '123', + transaction: undefined, + }, + mockedHub, + ); + + expect(tags).toEqual({ + sentryTrace: '', + }); + }); + + it('returns only the `sentry-trace` tag if no DSC is available', () => { + vi.spyOn(SentryCore, 'getDynamicSamplingContextFromClient').mockReturnValueOnce({ + trace_id: '', + public_key: undefined, + }); + + const tags = getTracingMetaTags( + // @ts-expect-error - only passing a partial span object + { + toTraceparent: () => '123', + transaction: undefined, + }, + { + ...mockedHub, + getClient: () => undefined, + }, + ); + + expect(tags).toEqual({ + sentryTrace: '', + }); + }); +}); diff --git a/packages/astro/test/server/middleware.test.ts b/packages/astro/test/server/middleware.test.ts index af42348a94b9..9be67cc481a0 100644 --- a/packages/astro/test/server/middleware.test.ts +++ b/packages/astro/test/server/middleware.test.ts @@ -4,8 +4,16 @@ import { vi } from 'vitest'; import { handleRequest, interpolateRouteFromUrlAndParams } from '../../src/server/middleware'; +vi.mock('../../src/server/meta', () => ({ + getTracingMetaTags: () => ({ + sentryTrace: '', + baggage: '', + }), +})); + describe('sentryMiddleware', () => { const startSpanSpy = vi.spyOn(SentryNode, 'startSpan'); + const nextResult = Promise.resolve(new Response(null, { status: 200, headers: new Headers() })); afterEach(() => { vi.clearAllMocks(); @@ -24,7 +32,6 @@ describe('sentryMiddleware', () => { id: '123', }, }; - const nextResult = Promise.resolve({ status: 200 }); const next = vi.fn(() => nextResult); // @ts-expect-error, a partial ctx object is fine here @@ -109,7 +116,7 @@ describe('sentryMiddleware', () => { params: {}, url: new URL('https://myDomain.io/users/'), }; - const next = vi.fn(); + const next = vi.fn(() => nextResult); // @ts-expect-error, a partial ctx object is fine here await middleware(ctx, next); @@ -159,7 +166,7 @@ describe('sentryMiddleware', () => { params: {}, url: new URL('https://myDomain.io/users/'), }; - const next = vi.fn(); + const next = vi.fn(() => nextResult); // @ts-expect-error, a partial ctx object is fine here await middleware(ctx, next); @@ -178,6 +185,90 @@ describe('sentryMiddleware', () => { expect.any(Function), // the `next` function ); }); + + it('injects tracing tags into the HTML of a pageload response', async () => { + vi.spyOn(SentryNode, 'getCurrentHub').mockImplementation(() => ({ + // @ts-expect-error this is fine + getClient: () => ({}), + })); + + const middleware = handleRequest(); + + const ctx = { + request: { + method: 'GET', + url: '/users', + headers: new Headers(), + }, + params: {}, + url: new URL('https://myDomain.io/users/'), + }; + const next = vi.fn(() => + Promise.resolve( + new Response('', { + headers: new Headers({ 'content-type': 'text/html' }), + }), + ), + ); + + // @ts-expect-error, a partial ctx object is fine here + const resultFromNext = await middleware(ctx, next); + + expect(resultFromNext?.headers.get('content-type')).toEqual('text/html'); + + const html = await resultFromNext?.text(); + + expect(html).toContain(' { + const middleware = handleRequest(); + + const ctx = { + request: { + method: 'GET', + url: '/users', + headers: new Headers(), + }, + params: {}, + url: new URL('https://myDomain.io/users/'), + }; + + const originalResponse = new Response('{"foo": "bar"}', { + headers: new Headers({ 'content-type': 'application/json' }), + }); + const next = vi.fn(() => Promise.resolve(originalResponse)); + + // @ts-expect-error, a partial ctx object is fine here + const resultFromNext = await middleware(ctx, next); + + expect(resultFromNext).toBe(originalResponse); + }); + + it("no-ops if there's no tag in the response", async () => { + const middleware = handleRequest(); + + const ctx = { + request: { + method: 'GET', + url: '/users', + headers: new Headers(), + }, + params: {}, + url: new URL('https://myDomain.io/users/'), + }; + + const originalResponse = new Response('

no head

', { + headers: new Headers({ 'content-type': 'text/html' }), + }); + const next = vi.fn(() => Promise.resolve(originalResponse)); + + // @ts-expect-error, a partial ctx object is fine here + const resultFromNext = await middleware(ctx, next); + + expect(resultFromNext).toBe(originalResponse); + }); }); describe('interpolateRouteFromUrlAndParams', () => { From 272cbbdde7b369c2bcce132608903c707f15c21a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 8 Nov 2023 17:54:08 +0100 Subject: [PATCH 2/4] add regex check for sentry-trace and baggage values --- packages/astro/src/server/meta.ts | 42 ++++++++++- packages/astro/test/server/meta.test.ts | 99 ++++++++++++++++++++++--- 2 files changed, 126 insertions(+), 15 deletions(-) diff --git a/packages/astro/src/server/meta.ts b/packages/astro/src/server/meta.ts index 71e1cf05465a..abb1536ab608 100644 --- a/packages/astro/src/server/meta.ts +++ b/packages/astro/src/server/meta.ts @@ -1,7 +1,12 @@ import type { Hub } from '@sentry/core'; import { getDynamicSamplingContextFromClient } from '@sentry/core'; import type { Span } from '@sentry/types'; -import { dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader } from '@sentry/utils'; +import { + dynamicSamplingContextToSentryBaggageHeader, + generateSentryTraceHeader, + logger, + TRACEPARENT_REGEXP, +} from '@sentry/utils'; /** * Extracts the tracing data from the current span or from the client's scope @@ -36,8 +41,39 @@ export function getTracingMetaTags(span: Span | undefined, hub: Hub): { sentryTr const baggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + const isValidSentryTraceHeader = TRACEPARENT_REGEXP.test(sentryTrace); + if (!isValidSentryTraceHeader) { + logger.warn('Invalid sentry-trace data. Returning empty tag'); + } + + const validBaggage = isValidBaggageString(baggage); + if (!validBaggage) { + logger.warn('Invalid baggage data. Returning empty tag'); + } + return { - sentryTrace: ``, - baggage: baggage ? `` : undefined, + sentryTrace: ``, + baggage: baggage && ``, }; } + +/** + * Tests string against baggage spec as defined in: + * + * - W3C Baggage grammar: https://www.w3.org/TR/baggage/#definition + * - RFC7230 token definition: https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6 + * + * exported for testing + */ +export function isValidBaggageString(baggage?: string): boolean { + if (!baggage || !baggage.length) { + return false; + } + const keyRegex = "[-!#$%&'*+.^_`|~A-Za-z0-9]+"; + const valueRegex = '[!#-+-./0-9:<=>?@A-Z\\[\\]a-z{-}]+'; + const spaces = '\\s*'; + const baggageRegex = new RegExp( + `^${keyRegex}${spaces}=${spaces}${valueRegex}(${spaces},${spaces}${keyRegex}${spaces}=${spaces}${valueRegex})*$`, + ); + return baggageRegex.test(baggage); +} diff --git a/packages/astro/test/server/meta.test.ts b/packages/astro/test/server/meta.test.ts index 0679b10b5db8..6298f5f2a20b 100644 --- a/packages/astro/test/server/meta.test.ts +++ b/packages/astro/test/server/meta.test.ts @@ -1,10 +1,10 @@ import * as SentryCore from '@sentry/core'; import { vi } from 'vitest'; -import { getTracingMetaTags } from '../../src/server/meta'; +import { getTracingMetaTags, isValidBaggageString } from '../../src/server/meta'; const mockedSpan = { - toTraceparent: () => '123', + toTraceparent: () => '12345678901234567890123456789012-1234567890123456-1', transaction: { getDynamicSamplingContext: () => ({ environment: 'production', @@ -28,7 +28,7 @@ describe('getTracingMetaTags', () => { const tags = getTracingMetaTags(mockedSpan, mockedHub); expect(tags).toEqual({ - sentryTrace: '', + sentryTrace: '', baggage: '', }); } @@ -40,21 +40,24 @@ describe('getTracingMetaTags', () => { // @ts-expect-error - only passing a partial scope object getScope: () => ({ getPropagationContext: () => ({ - traceId: 'abc', + traceId: '12345678901234567890123456789012', sampled: true, - spanId: '456', + spanId: '1234567890123456', dsc: { environment: 'staging', public_key: 'key', - trace_id: 'abc', + trace_id: '12345678901234567890123456789012', }, }), }), }); expect(tags).toEqual({ - sentryTrace: expect.stringMatching(/', + sentryTrace: expect.stringMatching( + //, + ), + baggage: + '', }); }); @@ -67,14 +70,14 @@ describe('getTracingMetaTags', () => { const tags = getTracingMetaTags( // @ts-expect-error - only passing a partial span object { - toTraceparent: () => '123', + toTraceparent: () => '12345678901234567890123456789012-1234567890123456-1', transaction: undefined, }, mockedHub, ); expect(tags).toEqual({ - sentryTrace: '', + sentryTrace: '', }); }); @@ -87,7 +90,7 @@ describe('getTracingMetaTags', () => { const tags = getTracingMetaTags( // @ts-expect-error - only passing a partial span object { - toTraceparent: () => '123', + toTraceparent: () => '12345678901234567890123456789012-1234567890123456-1', transaction: undefined, }, { @@ -97,7 +100,79 @@ describe('getTracingMetaTags', () => { ); expect(tags).toEqual({ - sentryTrace: '', + sentryTrace: '', }); }); }); + +describe('isValidBaggageString', () => { + it.each([ + 'sentry-environment=production', + 'sentry-environment=staging,sentry-public_key=key,sentry-trace_id=abc', + // @ is allowed in values + 'sentry-release=project@1.0.0', + // spaces are allowed around the delimiters + 'sentry-environment=staging , sentry-public_key=key ,sentry-release=myproject@1.0.0', + 'sentry-environment=staging , thirdparty=value ,sentry-release=myproject@1.0.0', + // these characters are explicitly allowed for keys in the baggage spec: + "!#$%&'*+-.^_`|~1234567890abcxyzABCXYZ=true", + // special characters in values are fine (except for ",;\ - see other test) + 'key=(value)', + 'key=[{(value)}]', + 'key=some$value', + 'key=more#value', + 'key=max&value', + 'key=max:value', + 'key=x=value', + ])('returns true if the baggage string is valid (%s)', baggageString => { + expect(isValidBaggageString(baggageString)).toBe(true); + }); + + it.each([ + // baggage spec doesn't permit leading spaces + ' sentry-environment=production,sentry-publickey=key,sentry-trace_id=abc', + // no spaces in keys or values + 'sentry-public key=key', + 'sentry-publickey=my key', + // no delimiters ("(),/:;<=>?@[\]{}") in keys + 'asdf(x=value', + 'asdf)x=value', + 'asdf,x=value', + 'asdf/x=value', + 'asdf:x=value', + 'asdf;x=value', + 'asdfx=value', + 'asdf?x=value', + 'asdf@x=value', + 'asdf[x=value', + 'asdf]x=value', + 'asdf\\x=value', + 'asdf{x=value', + 'asdf}x=value', + // no ,;\" in values + 'key=va,lue', + 'key=va;lue', + 'key=va\\lue', + 'key=va"lue"', + // baggage headers can have properties but we currently don't support them + 'sentry-environment=production;prop1=foo;prop2=bar,nextkey=value', + // no fishy stuff + 'absolutely not a valid baggage string', + 'val"/>', + 'something"/>', + '', + '/>', + '" onblur="alert("xss")', + ])('returns false if the baggage string is invalid (%s)', baggageString => { + expect(isValidBaggageString(baggageString)).toBe(false); + }); + + it('returns false if the baggage string is empty', () => { + expect(isValidBaggageString('')).toBe(false); + }); + + it('returns false if the baggage string is empty', () => { + expect(isValidBaggageString(undefined)).toBe(false); + }); +}); From 32a7a04513913947310def2cd02c8a8dfb4fd05e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 9 Nov 2023 14:00:00 +0100 Subject: [PATCH 3/4] use ReadableStream to modify html chunks --- packages/astro/src/server/meta.ts | 3 +- packages/astro/src/server/middleware.ts | 42 ++++++++++++++++++++----- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/packages/astro/src/server/meta.ts b/packages/astro/src/server/meta.ts index abb1536ab608..7257cf01db51 100644 --- a/packages/astro/src/server/meta.ts +++ b/packages/astro/src/server/meta.ts @@ -1,6 +1,5 @@ -import type { Hub } from '@sentry/core'; import { getDynamicSamplingContextFromClient } from '@sentry/core'; -import type { Span } from '@sentry/types'; +import type { Hub, Span } from '@sentry/types'; import { dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader, diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 999a223262d6..1c2933e1eccc 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -1,4 +1,5 @@ import { captureException, configureScope, getCurrentHub, startSpan } from '@sentry/node'; +import type { Hub, Span } from '@sentry/types'; import { addExceptionMechanism, objectify, stripUrlQueryAndFragment, tracingContextFromHeaders } from '@sentry/utils'; import type { APIContext, MiddlewareResponseHandler } from 'astro'; @@ -109,16 +110,29 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH return originalResponse; } - const html = await originalResponse.text(); - if (typeof html !== 'string' || !html.includes('')) { + const { body, ...restOfOriginalResponse } = originalResponse; + + // Type case necessary b/c the body's ReadableStream type doesn't include + // the async iterator that is actually available in Node + // We later on use the async iterator to read the body chunks + // see https://github.com/microsoft/TypeScript/issues/39051 + const originalBody = body as NodeJS.ReadableStream | null; + if (!originalBody) { return originalResponse; } - const { sentryTrace, baggage } = getTracingMetaTags(span, hub); - const content = `\n${sentryTrace}\n${baggage}\n`; - const modifiedHtml = html.replace('', content); - - return new Response(modifiedHtml, originalResponse); + const newResponseStream = new ReadableStream({ + start: async controller => { + for await (const chunk of originalBody) { + const html = typeof chunk === 'string' ? chunk : new TextDecoder().decode(chunk); + const modifiedHtml = addMetaTagToHead(html, hub, span); + controller.enqueue(modifiedHtml); + } + controller.close(); + }, + }); + + return new Response(newResponseStream, restOfOriginalResponse); }, ); return res; @@ -130,6 +144,20 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH }; }; +/** + * This function optimistically assumes that the HTML coming in chunks will not be split + * within the tag. If this still happens, we simply won't replace anything. + */ +function addMetaTagToHead(htmlChunk: string, hub: Hub, span?: Span): string { + if (typeof htmlChunk !== 'string') { + return htmlChunk; + } + + const { sentryTrace, baggage } = getTracingMetaTags(span, hub); + const content = `\n${sentryTrace}\n${baggage}\n`; + return htmlChunk.replace('', content); +} + /** * Interpolates the route from the URL and the passed params. * Best we can do to get a route name instead of a raw URL. From 77fdc93eca19d7851b395dbc09dc8d5d70f5e21b Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 9 Nov 2023 14:45:44 +0100 Subject: [PATCH 4/4] fix tests and encoding error --- packages/astro/src/server/middleware.ts | 9 ++++----- packages/astro/test/server/middleware.test.ts | 9 +++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 1c2933e1eccc..fd3e99692d08 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -97,6 +97,7 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH }, async span => { const originalResponse = await next(); + if (span && originalResponse.status) { span.setHttpStatus(originalResponse.status); } @@ -110,13 +111,11 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH return originalResponse; } - const { body, ...restOfOriginalResponse } = originalResponse; - // Type case necessary b/c the body's ReadableStream type doesn't include // the async iterator that is actually available in Node // We later on use the async iterator to read the body chunks // see https://github.com/microsoft/TypeScript/issues/39051 - const originalBody = body as NodeJS.ReadableStream | null; + const originalBody = originalResponse.body as NodeJS.ReadableStream | null; if (!originalBody) { return originalResponse; } @@ -126,13 +125,13 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH for await (const chunk of originalBody) { const html = typeof chunk === 'string' ? chunk : new TextDecoder().decode(chunk); const modifiedHtml = addMetaTagToHead(html, hub, span); - controller.enqueue(modifiedHtml); + controller.enqueue(new TextEncoder().encode(modifiedHtml)); } controller.close(); }, }); - return new Response(newResponseStream, restOfOriginalResponse); + return new Response(newResponseStream, originalResponse); }, ); return res; diff --git a/packages/astro/test/server/middleware.test.ts b/packages/astro/test/server/middleware.test.ts index 9be67cc481a0..058982f06cc6 100644 --- a/packages/astro/test/server/middleware.test.ts +++ b/packages/astro/test/server/middleware.test.ts @@ -259,7 +259,8 @@ describe('sentryMiddleware', () => { url: new URL('https://myDomain.io/users/'), }; - const originalResponse = new Response('

no head

', { + const originalHtml = '

no head

'; + const originalResponse = new Response(originalHtml, { headers: new Headers({ 'content-type': 'text/html' }), }); const next = vi.fn(() => Promise.resolve(originalResponse)); @@ -267,7 +268,11 @@ describe('sentryMiddleware', () => { // @ts-expect-error, a partial ctx object is fine here const resultFromNext = await middleware(ctx, next); - expect(resultFromNext).toBe(originalResponse); + expect(resultFromNext?.headers.get('content-type')).toEqual('text/html'); + + const html = await resultFromNext?.text(); + + expect(html).toBe(originalHtml); }); });