isValid() returns false for undefined string

[rcarton]

Commit 500dca0
introduced a regression for existing code that
passed an undefined string to isValid().

[whitequark]

I'm not sure why do you think it's a regression. I didn't ever intend to have non-strings be an input that can be passed to isValid, hence the lack of a test! Is there some widely used library doing this? There was also #31...

[rcarton]

Yeah, we updated our version of express and this started happening, I think it's when you use express' req.ip, it resolves the request's original ip based on the X-FORWARDED-FOR header that's added by proxies.

express -> proxy-addr -> ipaddr.js

https://github.com/jshttp/proxy-addr/releases, the release from 3 days ago started breaking.

[whitequark]

OK, I still think it's proxy-addr that should be fixed, unless you can make a compelling API design argument that undefined should be gracefully rejected. I'm a bit out of touch with the API design in JS-land so I'm all ears...

[rcarton]

To me there's no clear cut argument for or against. I'd say if it's cheap to add the verification, why not (kind of a null check in java). I totally understand your point.

[whitequark]

OK, then I would consider this an issue in proxy-addr. Would you please report it over there?

[whitequark]

Don't bother, I've decided I will fix this in ipaddr.js.

[rcarton]

👍

[dougwilson]

Hi @rcarton , I just published an updated proxy-addr to npm as 1.0.10 pulling in the updated version of this module.

@whitequark , as far as passing undefined, I don't personally care either way :) Typically when I see a function that does validation, I assume it could accept any type of argument (which would validate if it matched the necessary criteria). We don't validate what we are providing it, so the value is whatever Node.js just happens to return from socket.remoteAddress (https://nodejs.org/api/net.html#net_socket_remoteaddress). I didn't have a test for undefined in my module either (until now, haha).

In order to get a quick update out, I just updated the ipaddr.js module, but I'll add a typeof check in there as well before calling the isValid in a future update :) I hope this helps explain why undefined was getting in there (because, Node.js is returning undefined, even though they document the property as just "The string representation of the remote IP address.").

[baloo]

@dougwilson remoteAddress could actually be undefined as per https://github.com/nodejs/node/blob/master/lib/net.js#L564-L580 see nodejs/node#4198 which I hope to be fixed soon. I also made a PR to forwarded which I believe should handle this case jshttp/forwarded#2

[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.