search

y-mehta / ssrf-req-filter

Module to prevent SSRF when sending requests in NodeJS. Blocks request to local and private IP addresses
https://github.com/y-mehta/ssrf-req-filter
MIT License
22 stars 1 forks source link

Question about DNS Rebinding unit test #37

Open craig-davis opened 1 year ago

craig-davis commented 1 year ago

I have a concern that the DNS rebinding test may be ambiguous in asserting the correct response.

In the rebinding test it appears that the code sets check = 1 in the initial success .then on line 76, and in the .catch if the status code is a 400 on line 80.

https://github.com/y-mehta/ssrf-req-filter/blob/170b64da2f40fbb62cb0723c1912043d4f30fb53/test/index.js#L73-L88

The test then asserts that check has a value of 1. This seems to say that the test will pass with either a 200 or a 400 response from the server.

Is this the correct interpretation of this test?

y-mehta commented 1 year ago

@craig-davis, I agree; it's ambiguous and need to figure out a better approach to handle it.

The rebind.it URL being used for the test resolves to a public IP first and subsequently resolves to an internal IP (127.0.0.1) - If response from the public server is received, it'll pass and fail otherwise. So, the test was added to ensure it's safe from DSN rebinding.

craig-davis commented 1 year ago

What about using sinon to spy on the checkIp() function?

The function would have to get relocated so it could be accessed. Having a spy on it or a stub in place would allow the tests to make assertions about the IP it was called with and how many times it's been called.