Refine SecRequestBodyNoFilesLimit test cases to cover edge scenarios

[hnakamur]

what

Extend and adjust existing tests for the SecRequestBodyNoFilesLimit directive in config-body_limits.json to ensure correct handling of edge boundary values.

Also fix the Content-Length of the requests. Note that an LF is added for each line in the body field. see

ModSecurity/test/regression/regression_test.cc

Line 51 in cebcde3

i << key << "\n";

$ echo "param1=value1&param2=value2&param3=value3" | wc -c
42

why

The existing tests do not cover edge boundary lengths. As a result, we cannot verify whether a request body whose length is exactly equal to SecRequestBodyNoFilesLimit is accepted.

references

None.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[airween]

Hi @hnakamur,

thanks for this PR.

Extend and adjust existing tests for the SecRequestBodyNoFilesLimit directive in config-body_limits.json to ensure correct handling of edge boundary values.

Also fix the Content-Length of the requests. Note that an LF is added for each line in the body field.

I think Content-Length field does not contain the LF in reality.

Also note, that libmodsecurity3 does not correspond to Content-Length value with length of request body.

It counts the added bytes and compares the current length with the given value:

ModSecurity/src/transaction.cc

Lines 936 to 938 in 09fec93

	if (this->m_rules->m_requestBodyLimit.m_value > 0
	&& this->m_rules->m_requestBodyLimit.m_value < len + current_size) {
	m_variableInboundDataError.set("1", m_variableOffset);

see:

ModSecurity/test/regression/regression_test.cc

Line 51 in cebcde3

i << key << "\n";

you're right, but I think we should remove that "extra" \n at the end of the concatenation instead of increasing the (unused) Content-Length value.

$ echo "param1=value1&param2=value2&param3=value3" | wc -c
42

Yes, but as I explained above, I think we should calculate the length without LF:

$ echo -n "param1=value1&param2=value2&param3=value3" | wc -c
41

The existing tests do not cover edge boundary lengths.

Do we need that edge boundary (probably you mean the "extra" \n)?

As a result, we cannot verify whether a request body whose length is exactly equal to SecRequestBodyNoFilesLimit is accepted.

Well, to see the valid value could be helpfully, eg. if we put a valid number into CL header. But again: CL header does not have any effect, it's just there... To show the info about the request length is important, but I think that's only an informative thing.

What do you think?

[hnakamur]

I agree with you about removing an extra \n at the end.

Actually I found it gets in the way for testing the edge cases, so I removed an extra \n at commit 400e871 in the #3476 and add \n to test cases at commit 6382a3c.

For regression tests, the value of Content-Length header is not used. It is tedious for updating the values, so I think it is better to remove Content-Length headers which are not needed.

Or we can modify regression test to add the calculated Content-Length from the body specified in tests automatically when running if the Content-Length is not specified in tests. If a Content-Length header is specified in tests, those values are used without being overwritten by the calculated values.

Maybe I should close this pull request for now. I’d like to hear your thoughts.

[airween]

I agree with you about removing an extra \n at the end.

I think now I understand the idea why was it added... Probably the author wanted to substitute the empty lines in some special cases, but... I'm not sure the result is what they wanted.

Actually I found it gets in the way for testing the edge cases, so I removed an extra \n at commit 400e871 in the #3476 and add \n to test cases at commit 6382a3c.

Thank you for this - this helped me to understand the original idea.

I'm not sure we should add \n to everywhere, eg this one is not necessary.

But in most cases, eg. where the payload contains more lines (XML, multipart), there it needs the \n.

For regression tests, the value of Content-Length header is not used. It is tedious for updating the values, so I think it is better to remove Content-Length headers which are not needed.
Or we can modify regression test to add the calculated Content-Length from the body specified in tests automatically when running if the Content-Length is not specified in tests. If a Content-Length header is specified in tests, those values are used without being overwritten by the calculated values.

Personally I don't like implicit use of values, and as we discussed it does not have any role. I think it's more informative than usable. Therefore I wouldn't support to remove or implicit calculation, let the developer to fill that with a correct value.

Maybe I should close this pull request for now. I’d like to hear your thoughts.

What if we add extra \n only if the node->u.array.len is greater than 1? (Just a quick idea...)

[airween]

Also, could you rebase your V3 related PR branches with the current v3/master? I fixed some cppcheck issues in #3485.