wargio / naxsi

NAXSI is an open-source, high performance, low rules maintenance WAF for NGINX
GNU General Public License v3.0
305 stars 38 forks source link

[Windows] IgnoreCIDR test failure #46

Closed staticlibs closed 2 years ago

staticlibs commented 2 years ago

There is an intermittent test failure, that happens in about 1 in 10 test runs and can be reproduced only on Windows:

======================================================================
FAIL: test_1_5 (test_34ignorecidr.ignorecidr)
TEST 1.5: Verify IgnoreCIDR x.x.x.x./32 is converted to IgnoreIP
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\naxsi\naxsi\unit-tests\python\test_34ignorecidr.py", line 196, in test_1_5
    self.assertEqual(ec, 200)
AssertionError: 412 != 200

----------------------------------------------------------------------

It may or may not be related to a real Windows-specific problem in IP conversion area, I intend to investigate it.

wargio commented 2 years ago

i had a partial idea to rewrite naxsi and make it more like a library with proper unit tests and resolve these sort of issues. Are you sure that the IP is correct? are you parsing the forwarded headers correctly from the test?

staticlibs commented 2 years ago

Yes, a separate library possibly will make it easier to write unit tests.

I've reproduced the issue on Windows and submitted the patch in #48. Just a couple of related questions:

  1. strlen is used to determine the length of X-Forwarded-For header value: here, and here. Should it just use value.len instead? It also formats (for logging) value.data with %s, should it be value and %V instead?

  2. Should I add something like (not sure about the appropriate naxsi_error_fatal usage):

    if (!ip.data) {
    naxsi_error_fatal(ctx, r, "failed alloc");
    return;
    }
wargio commented 2 years ago

yes this check should be done. if it happens we should throw a 500