Fix error_page internal redirect false positive logging #204
Conversation
|
@zimmerle @martinhsv @defanator @victorhora Any thoughts here? Should be good to |
|
still pending? oof. |
|
@defanator Can you have a look at this? |
|
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days |
|
Github bot being bad again 💀. |
martinhsv
added
nostale
|
Can say I have ran this in my fork of code for a long time 10/10 does indeed work 😆 . |
|
@jeremyjpj0916 copying here as well - it is believed that 4f26b48 should resolve at least some of observed misbehaviours reported in #182. If it doesn't, I would appreciate a test case demonstrating the issue with minimal configuration involved. |
|
What was wrong with the simple original change from my PR to ignore processings on error_page redirects since ModSecurity will have the wrong context post an error_page redirect? From my OP this is still relevant:
cc @airween |
|
Fixes: #182
Problem here was
error_pageredirects don't maintain original client request data. Hence ModSecurity has false information when evaluating the clients HTTP request a second time around during the request(HTTP Method Header is always reset to GET, URI is also not the original from client request). It is also inefficient to evaluate the request data a second time around after the internal redirect, hence cutting out those phases the second time around after the redirect makes sense and is a performance improvement.To figure it out I was reviewing prior commits, specifically:
ce1d438
Which regarding this commit did indeed ensure the log phase still runs during
error_pageinternal redirect, but just had not considered the implications of re-processing the NGX request phases(phases prior to theproxy_passdirective for instance) again.Currently running these changes in our dev and stage environments personally. Will be pushing to our prod in the next week or so, will run as a fork until this is merged.