opentelemetry-instrumentation-http throws on requests with malformed Forwarded headers

[pmlanger]

What happened?

Steps to Reproduce

I used the minimal/standard setup from the repository README to reproduce.
Then, ran the server with node -r ./tracing.js app.js.
Then, ran two requests against the server:

$ curl http://localhost:9654
Hello World!%                                                                                                                                                                                                                                                 

$ curl http://localhost:9654 -H "Forwarded: malformed"
curl: (52) Empty reply from server

Expected Result

For the server not to crash/still being able to handle the request; and the open-telemetry-instrumentation-http plugin to handle bad requests gracefully.

Actual Result

Server/node process crashed with uncaughtException:

$ node -r ./tracing.js app.js
Example app listening on port 9654

/Users/philipp.langer/dev/otel-minimal/node_modules/forwarded-parse/index.js:146
    throw new ParseError('Unexpected end of input', header);
    ^
ParseError: Unexpected end of input
    at parse (/Users/philipp.langer/dev/otel-minimal/node_modules/forwarded-parse/index.js:146:11)
    at getServerAddress (/Users/philipp.langer/dev/otel-minimal/node_modules/@opentelemetry/instrumentation-http/build/src/utils.js:419:29)
    at getIncomingRequestAttributes (/Users/philipp.langer/dev/otel-minimal/node_modules/@opentelemetry/instrumentation-http/build/src/utils.js:497:27)
    at Server.incomingRequest (/Users/philipp.langer/dev/otel-minimal/node_modules/@opentelemetry/instrumentation-http/build/src/http.js:363:77)
    at parserOnIncoming (node:_http_server:1140:12)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
  input: 'malformed'
}

Additional Details

The error thrown comes from forward-parse, in this line: https://github.com/lpinca/forwarded-parse/blob/master/index.js#L146
which is called by open-telemetry-instrumentation-http in getIncomingRequestAttributes (

opentelemetry-js/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts

Line 689 in eb3ca4f

const serverAddress = getServerAddress(request, options.component);

) -> getServerAddress -> usage of forwarded-parse

It feels like the open-telemetry-instrumentation-http should handle errors in the plugin itself, i.e., the error should not "leave" the plugin. It is quite expected that a client may send a malformed request, and the server should not crash on those. I could not find a really good way to prevent these errors. That is why I filed this as a bug report.

But also happy to be educated whether there's a way to handle errors from plugins that I am not aware of.

Workaround

You might laugh at this, but for completeness: calling getIncomingRequestAttributes from ignoreIncomingRequestHook like this is a hacky way to fix the issue for clients:

  ignoreIncomingRequestHook: (request: IncomingMessage) => {
    try {
      getIncomingRequestAttributes(request, {
        component: 'http',
        semconvStability: SemconvStability.STABLE,
      })
      return false
    } catch {
      return true
    }

Possible fix

A possible fix could be wrapping this part of the code

opentelemetry-js/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts

Line 575 in eb3ca4f

for (const entry of forwardedParse(forwardedHeader)) {

(and the other occurrence in getRemoteClientAddress) into a try / catch and treating a parsing error gracefully in the same way as an absent Forwarded header. This seems to be in line with how the other x-forwarded-* headers are handled.

Happy to take care of that and submit a PR, should you agree about the problem and suggestion.

OpenTelemetry Setup Code

"use strict";

const process = require("process");
const opentelemetry = require("@opentelemetry/sdk-node");
const {
  getNodeAutoInstrumentations,
} = require("@opentelemetry/auto-instrumentations-node");
const { ConsoleSpanExporter } = require("@opentelemetry/sdk-trace-base");
const { Resource } = require("@opentelemetry/resources");
const {
  SEMRESATTRS_SERVICE_NAME,
} = require("@opentelemetry/semantic-conventions");

// configure the SDK to export telemetry data to the console
// enable all auto-instrumentations from the meta package
const traceExporter = new ConsoleSpanExporter();
const sdk = new opentelemetry.NodeSDK({
  resource: new Resource({
    [SEMRESATTRS_SERVICE_NAME]: "my-service",
  }),
  traceExporter,
  instrumentations: [getNodeAutoInstrumentations()],
});

// initialize the SDK and register with the OpenTelemetry API
// this enables the API to record telemetry
sdk.start();

// gracefully shut down the SDK on process exit
process.on("SIGTERM", () => {
  sdk
    .shutdown()
    .then(() => console.log("Tracing terminated"))
    .catch((error) => console.log("Error terminating tracing", error))
    .finally(() => process.exit(0));
});

package.json

{
  "name": "my-app",
  "dependencies": {
    "@opentelemetry/api": "^1.9.0",
    "@opentelemetry/auto-instrumentations-node": "^0.52.0",
    "@opentelemetry/sdk-node": "^0.54.0",
    "express": "^4.21.1"
  }
}

Relevant log output

/Users/philipp.langer/dev/otel-minimal/node_modules/forwarded-parse/index.js:146
    throw new ParseError('Unexpected end of input', header);
    ^
ParseError: Unexpected end of input
    at parse (/Users/philipp.langer/dev/otel-minimal/node_modules/forwarded-parse/index.js:146:11)
    at getServerAddress (/Users/philipp.langer/dev/otel-minimal/node_modules/@opentelemetry/instrumentation-http/build/src/utils.js:419:29)
    at getIncomingRequestAttributes (/Users/philipp.langer/dev/otel-minimal/node_modules/@opentelemetry/instrumentation-http/build/src/utils.js:497:27)
    at Server.incomingRequest (/Users/philipp.langer/dev/otel-minimal/node_modules/@opentelemetry/instrumentation-http/build/src/http.js:363:77)
    at parserOnIncoming (node:_http_server:1140:12)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
  input: 'malformed'
}

[dyladan]

Happy to take care of that and submit a PR, should you agree about the problem and suggestion.

That would be great if you could. I'm happy to help shepherd the PR if needed. We shouldn't be crashing end user apps so I'd like to get this fixed ASAP.

[pmlanger]

@dyladan I gave it a shot: #5099

[LuiFerPereira]

We have the same problem. I would like to solve it as soon as possible. How to help?

[pmlanger]

@LuiFerPereira #5099 has the fix; you can test it out if you like to. The PR is missing approval of workflow/reviews from maintainers and me addressing any comments (I am in CET timezone) :)