Skip to content

Commit

Permalink
Add requestHeaders allow list feature
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
luismiramirez committed Nov 23, 2021
1 parent aae3470 commit 4c11f36
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -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.
23 changes: 21 additions & 2 deletions packages/express/src/middleware/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { Client } from "@appsignal/nodejs"

import { HashMap } from "@appsignal/types"
import {
Request,
Response,
NextFunction,
ErrorRequestHandler,
RequestHandler
} from "express"
import { IncomingHttpHeaders } from "http"

/**
* Returns an Express middleware that can augment the current span
Expand All @@ -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
Expand All @@ -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)
}
Expand Down Expand Up @@ -80,3 +83,19 @@ export function expressErrorHandler(appsignal: Client): ErrorRequestHandler {
return next(err)
}
}

function filterHeaders(
headers: IncomingHttpHeaders,
appsignal: Client
): HashMap<any> {
let filtered: HashMap<any> = {}
const headersAllowList = appsignal.config.data.requestHeaders || []

headersAllowList.forEach(key => {
if (headers[key] != undefined) {
filtered[key] = headers[key]
}
})

return filtered
}
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 3 additions & 1 deletion packages/nodejs/global.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export {}

import { Client } from "./src/interfaces"

declare global {
var __APPSIGNAL__: any
var __APPSIGNAL__: Client
}
8 changes: 1 addition & 7 deletions packages/nodejs/src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
8 changes: 1 addition & 7 deletions packages/nodejs/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
20 changes: 18 additions & 2 deletions packages/nodejs/src/instrumentation/http/lifecycle/incoming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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))) {
Expand All @@ -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 => {
Expand All @@ -83,6 +86,19 @@ function incomingRequest(
}
}

function filterHeaders(headers: IncomingHttpHeaders): HashMap<any> {
let filtered: HashMap<any> = {}
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)
Expand Down
2 changes: 1 addition & 1 deletion test/integration/diagnose

0 comments on commit 4c11f36

Please sign in to comment.