From 4c11f36b292e090fd1dc2aa2ff7001b371bdb8cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luismi=20Ram=C3=ADrez?= Date: Fri, 19 Nov 2021 09:48:42 +0100 Subject: [PATCH] Add requestHeaders allow list feature `requestHeaders` allow list is now implemented. Now users are able to define a list of headers they want to be shown in AppSignal. All integrations except from Express make use of the core HTTP instrumentation. As we don't have helpers yet to access the Client stored in global object, the code is repeated in HTTP instrumentation, and in Express integration. This will be removed in favor of global helpers in the future. --- ...theaders-config-option-is-now-available.md | 6 +++++ packages/express/src/middleware/index.ts | 23 +++++++++++++++++-- ...theaders-config-option-is-now-available.md | 8 +++++++ packages/nodejs/global.d.ts | 4 +++- packages/nodejs/src/__tests__/config.test.ts | 8 +------ packages/nodejs/src/config.ts | 8 +------ .../http/lifecycle/incoming.ts | 20 ++++++++++++++-- test/integration/diagnose | 2 +- 8 files changed, 59 insertions(+), 20 deletions(-) create mode 100644 packages/express/.changesets/requestheaders-config-option-is-now-available.md create mode 100644 packages/nodejs/.changesets/requestheaders-config-option-is-now-available.md diff --git a/packages/express/.changesets/requestheaders-config-option-is-now-available.md b/packages/express/.changesets/requestheaders-config-option-is-now-available.md new file mode 100644 index 00000000..90055f92 --- /dev/null +++ b/packages/express/.changesets/requestheaders-config-option-is-now-available.md @@ -0,0 +1,6 @@ +--- +bump: "patch" +--- + +Collect request headers from express requests by default. See also our new `requestHeaders` config +option, added in the @appsignal/nodejs package. diff --git a/packages/express/src/middleware/index.ts b/packages/express/src/middleware/index.ts index 5162823f..17915ee7 100644 --- a/packages/express/src/middleware/index.ts +++ b/packages/express/src/middleware/index.ts @@ -1,5 +1,5 @@ import { Client } from "@appsignal/nodejs" - +import { HashMap } from "@appsignal/types" import { Request, Response, @@ -7,6 +7,7 @@ import { ErrorRequestHandler, RequestHandler } from "express" +import { IncomingHttpHeaders } from "http" /** * Returns an Express middleware that can augment the current span @@ -33,7 +34,8 @@ export function expressMiddleware(appsignal: Client): RequestHandler { res.end = function (this: Response) { res.end = originalEnd - const { method = "GET", params = {}, query = {} } = req + const { method = "GET", params = {}, query = {}, headers } = req + const filteredHeaders = filterHeaders(headers, appsignal) // if there is no error passed to `next()`, the span name will // be updated to match the current path @@ -48,6 +50,7 @@ export function expressMiddleware(appsignal: Client): RequestHandler { // @TODO: keep an eye on this // @ts-ignore span.setSampleData("params", { ...params, ...query }) + span.setSampleData("environment", filteredHeaders) return res.end.apply(this, arguments as any) } @@ -80,3 +83,19 @@ export function expressErrorHandler(appsignal: Client): ErrorRequestHandler { return next(err) } } + +function filterHeaders( + headers: IncomingHttpHeaders, + appsignal: Client +): HashMap { + let filtered: HashMap = {} + const headersAllowList = appsignal.config.data.requestHeaders || [] + + headersAllowList.forEach(key => { + if (headers[key] != undefined) { + filtered[key] = headers[key] + } + }) + + return filtered +} diff --git a/packages/nodejs/.changesets/requestheaders-config-option-is-now-available.md b/packages/nodejs/.changesets/requestheaders-config-option-is-now-available.md new file mode 100644 index 00000000..88ea1f2c --- /dev/null +++ b/packages/nodejs/.changesets/requestheaders-config-option-is-now-available.md @@ -0,0 +1,8 @@ +--- +bump: "patch" +--- + +The `requestHeaders` config option is now available. An allow list that gives the ability to define +which request headers you want to be shown in sample detail views. The default is a list of common +headers that do not include [personal identifiable information](https://docs.appsignal.com/appsignal/gdpr.html#allowed-request-headers-only). +Read more about [request headers](https://docs.appsignal.com/application/header-filtering.html) on our documentation website. diff --git a/packages/nodejs/global.d.ts b/packages/nodejs/global.d.ts index 8ba1668d..f5f5a696 100644 --- a/packages/nodejs/global.d.ts +++ b/packages/nodejs/global.d.ts @@ -1,5 +1,7 @@ export {} +import { Client } from "./src/interfaces" + declare global { - var __APPSIGNAL__: any + var __APPSIGNAL__: Client } diff --git a/packages/nodejs/src/__tests__/config.test.ts b/packages/nodejs/src/__tests__/config.test.ts index ac98799f..10aea9df 100644 --- a/packages/nodejs/src/__tests__/config.test.ts +++ b/packages/nodejs/src/__tests__/config.test.ts @@ -37,13 +37,7 @@ describe("Configuration", () => { "cache-control", "connection", "content-length", - "path-info", - "range", - "request-method", - "request-uri", - "server-name", - "server-port", - "server-protocol" + "range" ], transactionDebugMode: false } diff --git a/packages/nodejs/src/config.ts b/packages/nodejs/src/config.ts index 11ec2da5..d427006a 100644 --- a/packages/nodejs/src/config.ts +++ b/packages/nodejs/src/config.ts @@ -128,13 +128,7 @@ export class Configuration { "cache-control", "connection", "content-length", - "path-info", - "range", - "request-method", - "request-uri", - "server-name", - "server-port", - "server-protocol" + "range" ], transactionDebugMode: false } diff --git a/packages/nodejs/src/instrumentation/http/lifecycle/incoming.ts b/packages/nodejs/src/instrumentation/http/lifecycle/incoming.ts index c1a1d01e..f26dbf52 100644 --- a/packages/nodejs/src/instrumentation/http/lifecycle/incoming.ts +++ b/packages/nodejs/src/instrumentation/http/lifecycle/incoming.ts @@ -5,8 +5,9 @@ */ import { Tracer } from "../../../interfaces" +import { HashMap } from "@appsignal/types" import { parse } from "url" -import { IncomingMessage, ServerResponse } from "http" +import { IncomingMessage, ServerResponse, IncomingHttpHeaders } from "http" // explicitly ignore some urls that we can't guarantee groupings on, or // routes that cause known issues. @@ -32,8 +33,9 @@ function incomingRequest( } const [req, res] = args as [IncomingMessage, ServerResponse] - const { method = "GET", url = "/" } = req + const { method = "GET", url = "/", headers } = req const { pathname, query } = parse(url) + const allowedHeaders = filterHeaders(headers) // don't start a span for ignored urls if (url && DEFAULT_IGNORED_URLS.some(el => el.test(url))) { @@ -58,6 +60,7 @@ function incomingRequest( .setName(`${method} ${pathname === "/" ? pathname : "[unknown route]"}`) .setCategory("process_request.http") .set("method", method) + .setSampleData("environment", allowedHeaders) .setSampleData("params", query ? { query } : {}) return tracer.withSpan(rootSpan, span => { @@ -83,6 +86,19 @@ function incomingRequest( } } +function filterHeaders(headers: IncomingHttpHeaders): HashMap { + let filtered: HashMap = {} + const headersAllowList = global.__APPSIGNAL__.config.data.requestHeaders || [] + + headersAllowList.forEach(key => { + if (headers[key] != undefined) { + filtered[key] = headers[key] + } + }) + + return filtered +} + export function getPatchIncomingRequestFunction(tracer: Tracer) { return function (original: (event: string, ...args: unknown[]) => boolean) { return incomingRequest(original, tracer) diff --git a/test/integration/diagnose b/test/integration/diagnose index b2bb42a3..38d035a3 160000 --- a/test/integration/diagnose +++ b/test/integration/diagnose @@ -1 +1 @@ -Subproject commit b2bb42a3c6f37e4eccf78cd473b11d485ae4f81e +Subproject commit 38d035a31e69e48e430cb17b848848783bdf81a4