Skip to content

Commit 2848661

Browse files
trentmtrentm
committedAug 2, 2017
cli: don't add Host header for client_req if it already has one (#504)
·
1.8.151.8.11
1 parent 13c86f1 commit 2848661

8 files changed

+178
-10
lines changed
 

‎CHANGES.md

+7-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,13 @@ Known issues:
77

88
## not yet released
99

10-
(nothing yet)
10+
- [issue #504] The `bunyan` 1.x CLI adds a `Host: $client_req.address[:$client_req.port]`
11+
header when rendering a `client_req` field in a log record. Fix that here to:
12+
(a) not add it if `client_req.headers` already includes a host header; and
13+
(b) not include the given `port` if it is 80 or 443 (*assuming* that is the
14+
default port.
15+
Note: `bunyan` 2.x CLI will stop adding this Host header because it is a guess
16+
that can be wrong and misleading.
1117

1218

1319
## 1.8.10

‎bin/bunyan

+46-9
Original file line numberDiff line numberDiff line change
@@ -898,21 +898,57 @@ function emitRecord(rec, line, opts, stylize) {
898898
})
899899
}
900900

901+
/*
902+
* `client_req` is the typical field name for an *HTTP client request
903+
* object* serialized by the restify-clients library. Render the
904+
* client request somewhat like `curl` debug output shows it.
905+
*/
901906
if (rec.client_req && typeof (rec.client_req) === 'object') {
902907
var client_req = rec.client_req;
903908
delete rec.client_req;
909+
904910
var headers = client_req.headers;
911+
delete client_req.headers;
912+
913+
/*
914+
* `client_req.address`, and `client_req.port`, provide values for
915+
* a *likely* "Host" header that wasn't included in the serialized
916+
* headers. Node.js will often add this "Host" header in its
917+
* `http.ClientRequest`, e.g. for node v6.10.3:
918+
* // JSSTYLED
919+
* https://github.com/nodejs/node/blob/v6.10.3/lib/_http_client.js#L88-L105
920+
*
921+
* If `client_req.port` exists and is 80 or 443, we *assume* that
922+
* is the default protocol port, and elide it per the `defaultPort`
923+
* handling in the node.js link above.
924+
*
925+
* Note: This added Host header is a *guess*. Bunyan shouldn't be
926+
* doing this "favour" for users because it can be wrong and
927+
* misleading. Bunyan 2.x will drop adding this. See issue #504
928+
* for details.
929+
*/
905930
var hostHeaderLine = '';
906-
var s = '';
907-
if (client_req.address) {
908-
hostHeaderLine = '\nHost: ' + client_req.address;
909-
if (client_req.port)
910-
hostHeaderLine += ':' + client_req.port;
931+
if (!headers || !(
932+
Object.hasOwnProperty.call(headers, 'host') ||
933+
Object.hasOwnProperty.call(headers, 'Host') ||
934+
Object.hasOwnProperty.call(headers, 'HOST')
935+
)
936+
) {
937+
if (Object.hasOwnProperty.call(client_req, 'address')) {
938+
hostHeaderLine = '\nHost: ' + client_req.address;
939+
if (Object.hasOwnProperty.call(client_req, 'port')) {
940+
// XXX
941+
var port = +client_req.port;
942+
if (port !== 80 && port !== 443) {
943+
hostHeaderLine += ':' + client_req.port;
944+
}
945+
delete client_req.port;
946+
}
947+
delete client_req.address;
948+
}
911949
}
912-
delete client_req.headers;
913-
delete client_req.address;
914-
delete client_req.port;
915-
s += format('%s %s HTTP/%s%s%s', client_req.method,
950+
951+
var s = format('%s %s HTTP/%s%s%s', client_req.method,
916952
client_req.url,
917953
client_req.httpVersion || '1.1',
918954
hostHeaderLine,
@@ -925,6 +961,7 @@ function emitRecord(rec, line, opts, stylize) {
925961
delete client_req.method;
926962
delete client_req.url;
927963
delete client_req.httpVersion;
964+
928965
if (client_req.body) {
929966
s += '\n\n' + (typeof (client_req.body) === 'object' ?
930967
JSON.stringify(client_req.body, null, 2) :

‎test/cli-client-req.test.js

+120
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
/*
2+
* Copyright (c) 2017, Trent Mick.
3+
*
4+
* Test the bunyan CLI's handling of the "client_req" field.
5+
* "client_req" is a common-ish Bunyan log field from restify-clients. See:
6+
* // JSSTYLED
7+
* https://github.com/restify/clients/blob/85374f87db9f4469de2605b6b15632b317cc12be/lib/helpers/bunyan.js#L213
8+
*/
9+
10+
var exec = require('child_process').exec;
11+
var fs = require('fs');
12+
var path = require('path');
13+
var _ = require('util').format;
14+
var vasync = require('vasync');
15+
16+
// node-tap API
17+
if (require.cache[__dirname + '/tap4nodeunit.js'])
18+
delete require.cache[__dirname + '/tap4nodeunit.js'];
19+
var tap4nodeunit = require('./tap4nodeunit.js');
20+
var after = tap4nodeunit.after;
21+
var before = tap4nodeunit.before;
22+
var test = tap4nodeunit.test;
23+
24+
25+
// ---- globals
26+
27+
var BUNYAN = path.resolve(__dirname, '../bin/bunyan');
28+
29+
30+
// ---- tests
31+
32+
// The bunyan CLI shouldn't add a "Host:" line if client_req.headers has one.
33+
// See <https://github.com/trentm/node-bunyan/issues/504>.
34+
test('client_req renders without extra Host header', function (t) {
35+
exec(_('%s %s/corpus/issue504-client-req-with-host.log', BUNYAN, __dirname),
36+
function (err, stdout, stderr) {
37+
t.ifError(err)
38+
t.equal(stdout, [
39+
// JSSTYLED
40+
'[2017-05-12T23:59:15.877Z] TRACE: minfo/66266 on sharptooth.local: request sent (client_req.address=127.0.0.1)',
41+
' HEAD /dap/stor HTTP/1.1',
42+
' accept: application/json, */*',
43+
' host: foo.example.com',
44+
' date: Fri, 12 May 2017 23:59:15 GMT',
45+
''
46+
].join('\n'));
47+
t.end();
48+
});
49+
});
50+
51+
test('client_req added Host header elides port 80', function (t) {
52+
exec(_('%s %s/corpus/issue504-client-req-with-port80.log',
53+
BUNYAN, __dirname),
54+
function (err, stdout, stderr) {
55+
t.ifError(err)
56+
t.equal(stdout, [
57+
// JSSTYLED
58+
'[2017-05-12T23:59:15.877Z] TRACE: minfo/66266 on sharptooth.local: request sent',
59+
' HEAD /dap/stor HTTP/1.1',
60+
' Host: 127.0.0.1',
61+
' accept: application/json, */*',
62+
' date: Fri, 12 May 2017 23:59:15 GMT',
63+
''
64+
].join('\n'));
65+
t.end();
66+
});
67+
});
68+
69+
test('client_req added Host header elides port 443', function (t) {
70+
exec(_('%s %s/corpus/issue504-client-req-with-port443.log',
71+
BUNYAN, __dirname),
72+
function (err, stdout, stderr) {
73+
t.ifError(err)
74+
t.equal(stdout, [
75+
// JSSTYLED
76+
'[2017-05-12T23:59:15.877Z] TRACE: minfo/66266 on sharptooth.local: request sent',
77+
' HEAD /dap/stor HTTP/1.1',
78+
' Host: 127.0.0.1',
79+
' accept: application/json, */*',
80+
' date: Fri, 12 May 2017 23:59:15 GMT',
81+
''
82+
].join('\n'));
83+
t.end();
84+
});
85+
});
86+
87+
test('client_req added Host header, non-default port', function (t) {
88+
exec(_('%s %s/corpus/issue504-client-req-with-port8080.log',
89+
BUNYAN, __dirname),
90+
function (err, stdout, stderr) {
91+
t.ifError(err)
92+
t.equal(stdout, [
93+
// JSSTYLED
94+
'[2017-05-12T23:59:15.877Z] TRACE: minfo/66266 on sharptooth.local: request sent',
95+
' HEAD /dap/stor HTTP/1.1',
96+
' Host: 127.0.0.1:8080',
97+
' accept: application/json, */*',
98+
' date: Fri, 12 May 2017 23:59:15 GMT',
99+
''
100+
].join('\n'));
101+
t.end();
102+
});
103+
});
104+
105+
test('client_req without address does not add Host header', function (t) {
106+
exec(_('%s %s/corpus/issue504-client-req-without-address.log',
107+
BUNYAN, __dirname),
108+
function (err, stdout, stderr) {
109+
t.ifError(err)
110+
t.equal(stdout, [
111+
// JSSTYLED
112+
'[2017-05-12T23:59:15.877Z] TRACE: minfo/66266 on sharptooth.local: request sent',
113+
' HEAD /dap/stor HTTP/1.1',
114+
' accept: application/json, */*',
115+
' date: Fri, 12 May 2017 23:59:15 GMT',
116+
''
117+
].join('\n'));
118+
t.end();
119+
});
120+
});

‎test/corpus/issue504-client-req-with-host.log

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"name":"minfo","hostname":"sharptooth.local","pid":66266,"level":10,"client_req":{"method":"HEAD","url":"/dap/stor","address":"127.0.0.1","headers":{"accept":"application/json, */*","host":"foo.example.com","date":"Fri, 12 May 2017 23:59:15 GMT"}},"msg":"request sent","time":"2017-05-12T23:59:15.877Z","v":0}

‎test/corpus/issue504-client-req-with-port443.log

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"name":"minfo","hostname":"sharptooth.local","pid":66266,"level":10,"client_req":{"method":"HEAD","url":"/dap/stor","address":"127.0.0.1","port":"443","headers":{"accept":"application/json, */*","date":"Fri, 12 May 2017 23:59:15 GMT"}},"msg":"request sent","time":"2017-05-12T23:59:15.877Z","v":0}

‎test/corpus/issue504-client-req-with-port80.log

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"name":"minfo","hostname":"sharptooth.local","pid":66266,"level":10,"client_req":{"method":"HEAD","url":"/dap/stor","address":"127.0.0.1","port":"80","headers":{"accept":"application/json, */*","date":"Fri, 12 May 2017 23:59:15 GMT"}},"msg":"request sent","time":"2017-05-12T23:59:15.877Z","v":0}

‎test/corpus/issue504-client-req-with-port8080.log

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"name":"minfo","hostname":"sharptooth.local","pid":66266,"level":10,"client_req":{"method":"HEAD","url":"/dap/stor","address":"127.0.0.1","port":"8080","headers":{"accept":"application/json, */*","date":"Fri, 12 May 2017 23:59:15 GMT"}},"msg":"request sent","time":"2017-05-12T23:59:15.877Z","v":0}

‎test/corpus/issue504-client-req-without-address.log

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"name":"minfo","hostname":"sharptooth.local","pid":66266,"level":10,"client_req":{"method":"HEAD","url":"/dap/stor","headers":{"accept":"application/json, */*","date":"Fri, 12 May 2017 23:59:15 GMT"}},"msg":"request sent","time":"2017-05-12T23:59:15.877Z","v":0}

0 commit comments

Comments
 (0)
Please sign in to comment.