elastic / elastic/apm-agent-nodejs

`transaction.context.request.url.*` is wrong for an incoming HTTP request with a pathname starting with a double-slash

Open
#3,137 3 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.context.request.url.*` is wrong for an incoming HTTP request with a pathname starting with a double-slash. E.g.:

```
GET //foo/bar HTTP/1.1
Host: example.com
```

## repro

Apply this patch:

```diff
diff --git a/examples/trace-http.js b/examples/trace-http.js
index f9fbb524..31e5840b 100755
--- a/examples/trace-http.js
+++ b/examples/trace-http.js
@@ -56,7 +56,12 @@ server.listen(3000, function () {
//
// Note that this there is no current "transaction" here, so this HTTP
// request is not captured by APM. See "trace-http-request.js" for more.
- const clientReq = http.request('http://localhost:3000/', function (clientRes) {
+ // const clientReq = http.request('http://localhost:3000/', function (clientRes) {
+ const clientReq = http.request({
+ host: 'localhost',
+ port: 3000,
+ path: '//user@foo'
+ }, function (clientRes) {
console.log('client response: %s %s', clientRes.statusCode, clientRes.headers)
const chunks = []
clientRes.on('data', function (chunk) {
diff --git a/lib/parsers.js b/lib/parsers.js
index 377ba50d..82ed1800 100644
--- a/lib/parsers.js
+++ b/lib/parsers.js
@@ -27,6 +27,7 @@ function getContextFromRequest (req, conf, type) {
url: getUrlFromRequest(req),
headers: undefined
}
+ console.log('XXX context.url', context.url)
if (req.socket && req.socket.remoteAddress) {
context.socket = {
remote_address: req.socket.remoteAddress
```

Then run: `node examples/trace-http.js`.

The generated transaction includes:

```json
{
"transaction": {
"name": "GET //user@foo",
...
"request": {
"http_version": "1.1",
"method": "GET",
"url": {
"raw": "//user@foo",
"protocol": "http:",
"hostname": "foo",
"port": "3000",
"full": "http://foo:3000"
},
"headers": {
"host": "localhost:3000",
"connection": "close"
},
"socket": {
"remote_address": "::ffff:127.0.0.1"
}
},
```

### details

The recent https://github.com/elastic/apm-agent-nodejs/pull/3133 fixed a similar issue in the handling of `transaction.name` for this case. The issue there was that the request path, e.g. `//foo/bar`, was used as a full URL in URL parsing (via `new URL(...)` or similar). However, this `req.url` is the "request-target" in the HTTP/1.1 request line and *not* a full URL.

The `transaction.context.request.url.*` value is calculated using the `original-url` module here: https://github.com/elastic/apm-agent-nodejs/blob/34cc2a77820ae5b52f5ffab36f33fbaece8ccaf6/lib/parsers.js#L27

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.