Skip to content
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

PCRE match limits backward incompatible behavior #2902

Closed
azurit opened this issue May 16, 2023 · 2 comments
Closed

PCRE match limits backward incompatible behavior #2902

azurit opened this issue May 16, 2023 · 2 comments

Comments

@azurit
Copy link

azurit commented May 16, 2023

Describe the bug

This PR #2736 introduced a PCRE match limits which, when hit, sets the variable MSC_PCRE_LIMITS_EXCEEDED. This behavior is incompatible with ModSecurity v2, which is in the same situation setting variable TX:MSC_PCRE_LIMITS_EXCEEDED.

Expected behavior

Variable names must be unified to keep backward compatibility with v2. Please rename MSC_PCRE_LIMITS_EXCEEDED to TX:MSC_PCRE_LIMITS_EXCEEDED remove MSC_PCRE_LIMITS_EXCEEDED and set TX:MSC_PCRE_LIMITS_EXCEEDED instead in v3.

@martinhsv
Copy link
Contributor

martinhsv commented May 29, 2023

Hello @azurit ,

It has been the position of the ModSecurity team that not every aspect of functionality in ModSecurity v2 must be replicated exactly in ModSecurity v3. Keep in mind that it is common in software for major version changes to indicate differences that might affect a seamless upgrade. Sometimes differences between v2 and v3 may mean a better way of doing things in v3, and that v2 oughtn't be slavishly imitated.

In this particular case, I am not convinced that changing the variable in question to be TX:MSC_PCRE_LIMITS_EXCEEDED would be an improvement.

Note that other error variables that may be set by the ModSecurity engine do not use TX: -- the engine sets REQBODY_ERROR and not TX:REQBODY_ERROR. Likewise with all of the MULTIPART error variables, and several others.

To my mind, it's potentially confusing to have all other error flags use non-TX names but to have pcre error flags require TX:.

@martinhsv
Copy link
Contributor

Closing this as a duplicate of the recently merged #2901

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

No branches or pull requests

2 participants