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?