elastic / elastic/apm-agent-nodejs

`context.request.url` does not properly handle http2

Open
#2,147 0 comments 0 reactions 0 assignees View on GitHub
agent-nodejs bug
Dominant language
JavaScript
Stars
594
Forks
244
Avg merge
1d 8h
Merged PRs (30d)
16

Description

`{transaction,span,error}.context.request.url` is set in the agent by `getContextFromRequest` in lib/parsers.js: https://github.com/elastic/apm-agent-nodejs/blob/v3.17.0/lib/parsers.js#L18-L24

This uses the ["original-url"](https://github.com/watson/original-url#readme) package, to extract a URL object (in the form specified by the apm-server JSON specs -- https://github.com/elastic/apm-server/tree/master/docs/spec/v2/) from a node.js http server request object ((`http.IncomingMessage`)[https://nodejs.org/api/http.html#http_class_http_incomingmessage]).

With an http2 server the wrapped ["stream" event](https://nodejs.org/api/http2.html#http2_event_stream) does not have a direct IncomingMessage object. Currently the http2 instrumentation is setting [this artificial req object](https://github.com/elastic/apm-agent-nodejs/blob/v3.17.0/lib/instrumentation/modules/http2.js#L53-L59) on the transaction, which is later passed to `original-url` to create the URL fields. The result is something like `{ raw: '/', protocol: 'http:', pathname: '/' }` (see [this test code](https://github.com/elastic/apm-agent-nodejs/blob/v3.17.0/test/instrumentation/modules/http2.js#L381-L385)) instead of the more typical:

```
{
"raw": "/",
"protocol": "https:",
"hostname": "localhost",
"port": "$port",
"pathname": "/",
"full": "https://localhost:$port/"
},
```

from an https request.

Is that `protocol: 'http:'` wrong here? Shouldn't it be 'https:'?

Here is a smaller sample script that does the equivalent calls for dev testing:

```js
const fs = require('fs')
const http2 = require('http2')
const originalUrl = require('original-url')

const server = http2.createSecureServer({
allowHTTP1: true,
key: fs.readFileSync('localhost-privkey.pem'),
cert: fs.readFileSync('localhost-cert.pem')
})

server.on('stream', (stream, headers) => {
stream.respond({
'content-type': 'text/html; charset=utf-8',
':status': 200
})
const req = {
headers,
socket: stream.session.socket,
method: headers[':method'],
url: headers[':path'],
httpVersion: '2.0'
}
const url = originalUrl(req)
console.warn('XXX url: ', url)
console.warn('XXX from req: ', req)
stream.end(`Original URL: ${url.full}\n`)
})

server.listen(1337)

// 1. Create a local self-signed cert:
// openssl req -x509 -newkey rsa:2048 -nodes -sha256 -subj '/CN=localhost' -keyout localhost-privkey.pem -out localhost-cert.pem
// 2. Run the server:
// node playorigurl.js
// 3. Make an HTTP/2 request (use '-vv' curl option to see ALPN negotiation)
// curl https://localhost:8443 -ki
//
// Also worth testing is an HTTP/1.1 request:
// curl https://localhost:8443 -i --http1.1
```

Perhaps original-url could be updated (or the logic pulled into the agent) to support https://nodejs.org/api/http2.html classes like `Http2Session` and `Http2Stream` or the different HTTP/2 headers and/or the compatibility API classes http2.Http2ServerRequest (which would require instrumenting the "request" event).

```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.