Skip to content

Conversation

tomsommer
Copy link

@tomsommer tomsommer commented Sep 22, 2025

Hook into Nginx NGX_HTTP_ACCESS_PHASE instead of NGX_HTTP_PREACCESS_PHASE and NGX_HTTP_REWRITE_PHASE.

Having ModSecurity run before NGX_HTTP_ACCESS_PHASE cripples Nginx during DDOS attacks, because ModSecurity runs before Nginx limit_* functions and consumes too much CPU/memory even in phase:1.

Fixes #293

@airween
Copy link
Member

airween commented Sep 22, 2025

Hi @tomsommer!

Thanks for this PR. Could you add some test(s) (see others here) to check that the behavior is that the modification expects? I mean set up the engine with some very low rate limit and try to send more parallel requests than that. Or you can add a new job in our workflow.

Thanks again!

@tomsommer
Copy link
Author

What do you want to test? It sounds like you want to test if Nginx limit_* works?
The current tests should be able to test if this PR breaks anything.

@airween
Copy link
Member

airween commented Sep 22, 2025

What do you want to test? It sounds like you want to test if Nginx limit_* works?

No, I want to test if this patch solves the mentioned issue and that you wrote (in a removed comment): "nginx cannot survive a DDOS if modsec is before limit*". It would be nice to see that if the user sets a limit and requests exceed then Nginx drops the connections and the requests do not reach ModSecurity.

The current tests should be able to test if this PR breaks anything.

Yes, that's a necessary but not sufficient condition.

@tomsommer tomsommer marked this pull request as draft September 22, 2025 18:56
@tomsommer
Copy link
Author

tomsommer commented Sep 23, 2025

I did a lot of testing and I had to merge the rewrite stuff into the access handler, instead of registering two handlers in the same phase (I don't know if it's the order, or whatever), but it works now. It runs after limit_*

@tomsommer tomsommer marked this pull request as ready for review September 23, 2025 07:37
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModSecurity header phase runs before nginx rate limiting
2 participants