-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Clear original response code in send_error_bucket function #2850
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
If this is left intact, then apache thinks that this code was generated during processing of ErrorDocument and does not handle it properly Fix owasp-modsecurity#2849
Could this be merged? I can provide further help if needed.
marcstern
commented
Mar 7, 2023
FYI: We're using it on prod for 2 months on several huge sites
I haven't had opportunity to do a detailed analysis of this.
It looks simple, but this is the type of change that could potentially have unintended consequences -- perhaps only for some configurations and/or use cases.
Some questions that spring to mind immediately:
- why is the new line of code setting status to a hard-coded 200, when the assignment to status_line (just above the new code) has been set using the status value passed in to the function?
- is 200 really always the correct setting here? What if the original response was some other non-error response code like 301?
I haven't had opportunity to do a detailed analysis of this.
It looks simple, but this is the type of change that could potentially have unintended consequences -- perhaps only for some configurations and/or use cases.
Some questions that spring to mind immediately:
* why is the new line of code setting status to a hard-coded 200, when the assignment to status_line (just above the new code) has been set using the status value passed in to the function?
I admit it seemed strange to me as well but fortunately i described the breaking point in #2849, so let me clarify.
There is a code in Apache httpd which assumes that when ap_die function is called and request record contains status code other than 200, then this error code was encountered while trying to access custom response URL. Setting response code to 200 here has in fact no effect on the final response because just after the check for recursive error, original response code gets overwritten by the output of filter.
r->status = type;
* is 200 really always the correct setting here? What if the original response was some other non-error response code like 301?
I think so, because it looks to me that send_error_bucket is used only when mod_security wants to change status code of the response and if the original status code, so it does not matter what the old response code was.
Please correct me if i am wrong.
Hi @martinhsv ,
Just wanted to check in on my pull request - any updates or plans to merge it? Thanks!
Please let me know if there are further actions required from my side.
Hi @TomasKorbar ,
No imminent plans. As I suggested previously, I'd need to gain higher confidence of the correctness of this change before merging. In the meantime, the PR is available for anyone who wishes to merge in their own environment to make use of it.
If this is left intact, then apache thinks that this code was generated during processing of ErrorDocument and does not handle it properly
Fix #2849