test: remoteAddress may be undefined

[baloo]

see nodejs/node#4198

test case:

var net = require('net');
var server = net.createServer(function(c) { //'connection' listener
  c.on('end', function() {
    console.log(c.remoteAddress);
    server.close();
  });
});
server.listen(8124, function() {
  var client = net.connect({port: 8124}, function(c) {
    client.end();
  });
});

Signed-off-by: Arthur Gautier [email protected]

[baloo]

Looks like github order commits by date ... please have a look there https://github.com/baloo/forwarded/commits/bugfixes/remoteAddress-undefined to get the correct view

[dougwilson]

Hi @baloo your pull request actually introduces a security issue, as the module would allow for blind trust in possibly-forged header if the socket address is unknown. Without knowing the socket address, you can't tell if the request came from a trust source who is setting the header properly.

[baloo]

@dougwilson true that. my problem is actually that a client disconnect before morgan has time to successfully log the query.

When morgan executes https://github.com/expressjs/morgan/blob/master/index.js#L141 the callback is triggered on next tick by https://github.com/jshttp/on-finished/blob/master/index.js#L31 time by which the client may be disconnected (especially with a local reverse proxy and no keepalive to nodejs application).
Would you accept a PR on on-finished or morgan to make this behaviour tunable? (suggestions welcome!)

[baloo]

(when using combined format in morgan which lookup the client' ip)

[dougwilson]

The bug is in Node.js itself. I remember there is a pull request that saves the address for retrieval later, so you can get the address regardless of if the socket is still connected. I recommend you need to push that pull request forward.

Another option is to use the "immediate" option in morgan or even redefine the remote-addr token to use a saved IP from your own code.

[baloo]

We did move to the immediate option as a workaround already. But that bypasses a lot of things, among them the status-code and response size.

[dougwilson]

Here is the Node.js bug you should try and champion: nodejs/node-v0.x-archive#7566

Node.js maintains even say it is a confirmed bug, so you would think it would get fixed. I'm still looking for that pull request for you, but that is the main bug as far as I can tell.

As for the "immediate" option, yes, you would have to actually use two morgan instances: one records request parameters and the other records response parameters. Another option is just redefine the remote-addr token yourself and use your own method to determine the remote address (or even call Object.defineProperty to change the ip property to a value instead of a getter at the start of the request, to lock in the value).

[baloo]

By the way, what about a value cache for ip field in the express request ? currently the value is computed every time the value is accessed. It is not supposed to change during the request' lifetime. What do you think?

Anyway thanks for the tips and referencing the bug.

[dougwilson]

The model Express follows is to use getters that recalculate the values on every access. This means middleware can re-write aspects of the request, like the forwarded headers, and the value will be automatically reflected in the property. Basically, the recalculation is intentional and is part of the Express design/philosophy.

[dougwilson]

Basically, the the property does caching, it hard for people who want it not to cache (but very simple for people who want it to cache, as they just add middleware to capture and save the value, either as a new property or by redefining the property with the value). In addition, caching can bring in weird issues if it is done on-demand (if the first access of ip is after it's gone, on demand caching didn't help you) and if not done on demand, people will also complain about the unnecessary work Express is doing to calculate things they don't need and never access. Simply making the properties dynamic places all this control into the user's hands to manage, letting people define Express to be what they need, not whatever Express just happened to provide.