-
Notifications
You must be signed in to change notification settings - Fork 299
Fix client_body_in_file_only config setting not respected #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi @martinhsv, any story behind this change?
I mean, if #187 was the reason, then just the request_body_in_clean_file flag should not be touched:
https://github.com/nginx/nginx/blob/master/src/http/ngx_http_core_module.c#L1316-L1317
I'm not sure about another couple, as I barely remember there were some issues with connector tests. Happy to be wrong, but it's worth to check.
Hi @defanator ,
The line:
r->request_body_in_clean_file = 1;
cannot be left in place or the current issue will not be fixed.
The nginx code that you have pointed to sets request_body_in_clean_file to 1 only if the configuration file has 'client_body_in_file_only clean;'
If the config file has 'client_body_in_file_only on;' as in the poster's case, then the nginx code will set the variable request_body_in_clean_file=0 ... and then the ModSecurity connector code will effectively override that and always set it to 1.
Everything I've seen suggests that the three line-deletes in this pull request should proceed, since those lines being present amount to arbitrarily override nginx's own configuration setup. (I did also note the code comments associated with those three lines ... the original coder obviously wasn't confident that they ought to be there.)
Nevertheless, if there is a particular configuration+use case that you're worried won't work correctly with this pull request, feel free to let me know and I'll be happy to try it out.
@martinhsv well, the proposed patch is breaking the following tests:
Test Summary Report
-------------------
modsecurity-request-body-h2.t (Wstat: 3072 Tests: 38 Failed: 12)
Failed tests: 2, 6, 8, 10, 14, 16, 18, 22, 24, 26, 30
32
Non-zero exit status: 12
Files=13, Tests=230, 34 wallclock secs ( 0.07 usr 0.02 sys + 1.06 cusr 0.24 csys = 1.39 CPU)
Result: FAIL
I did a quick check on past issues both in library and nginx connector to bring more context, but haven't found anything significant (I'll also check our internal KB/support cases when time permits).
It seems like from all the following
r->request_body_in_single_buf = 1;
r->request_body_in_persistent_file = 1;
r->request_body_in_clean_file = 1;
only r->request_body_in_persistent_file = 1 is really essential (from tests point of view).
I think that removing another couple would resolve #187 and keep tests happy.
Thoughts?
PS: I've just checked that the client_body_in_file_only on works just fine using TEST_NGINX_LEAVE=yes and the following patch for connector tests:
diff --git a/tests/modsecurity-request-body.t b/tests/modsecurity-request-body.t
index 2d71c81..75441e8 100644
--- a/tests/modsecurity-request-body.t
+++ b/tests/modsecurity-request-body.t
@@ -35,6 +35,7 @@ events {
http {
%%TEST_GLOBALS_HTTP%%
+ client_body_in_file_only on;
server {
listen 127.0.0.1:8080;
Once prove is finished, I'm seeing all the body parts in expected place:
test@vagrant:~/ModSecurity-nginx$ find /tmp/nginx-test-4gB6tpc3Dz/client_body_temp/ -type f
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000019
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000016
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000029
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000007
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000027
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000018
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000040
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000015
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000020
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000030
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000044
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000043
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000022
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000034
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000012
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000036
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000003
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000013
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000038
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000010
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000014
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000031
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000008
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000039
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000028
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000032
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000041
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000001
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000005
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000021
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000033
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000035
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000024
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000042
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000037
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000009
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000004
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000023
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000006
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000011
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000026
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000002
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000025
/tmp/nginx-test-4gB6tpc3Dz/client_body_temp/0000000017
Hi @defanator ,
Thanks for that. I'm a bit surprised by the result, but good to know.
I'll alter/replace this pull request with one where the only change is to delete the single line 'r->request_body_in_clean_file = 1;'
That's probably the safest thing to do for now since 1) That's the only change that I've identified as absolutely necessary to resolve the current issue and 2) You've already identified some side-effects to a broader change.
Closing this in favour of pull request #194
No description provided.