ErrorLog code parameter always used with a hardcoded integer

[M4tteoP]

ErrorLog is exposed inside the MatchedRule interface as ErrorLog(code int) string. The code parameter should be used for logging the status code enforced by the disruptive action even if I think that the code that prints Coraza: Access denied with code %d is not working (Fix is c105d11 that is part of #792). I'm mainly wondering how we are using the exposed ErrorLog(code int) because we are always calling it with a hardcoded integer.

• Coraza-proxy-wasm and the HTTP-server go example call it as ErrorLog(0).
• Coraza-caddy as mr.ErrorLog(403).

I feel that using it that way is not meaningful, we are not printing the real status code enforced by the connector, not the one that the Coraza framework writes inside the interruption.

Should we propagate the status code expected by the triggered rule via MatchedRule (e.g. just like we are doing with mr.Disruptive_) and remove the parameter from the callback, or currently the connectors are implementing the callback in the wrong way?

Thanks!

cc. @jptosso

[jptosso]

But what if we are on detectOnly and corasza status is 403 but the app's status is 200?

[M4tteoP]

That specific case should be taken care of by the logic that fixes the mr.Disruptive_ propagation just merged with #792: https://github.com/corazawaf/coraza/blob/v3/dev/internal/corazawaf/transaction.go#L489-L497. Basically, the matched rule is marked as Disruptive only if the engine is on, not only if we have a disruptive action.

Generally speaking, I agree, the status in the end is enforced by the connector, we should propagate it from there, but it is something that I think we are not doing, considering that we are just hardcoding status codes 🤔

[jcchavezs]

Can we have this explicit with a test?

…

On Thu, 18 May 2023, 18:18 Matteo Pace, ***@***.***> wrote: That specific case should be taken care by the logic that fixes the mr.Disruptive_ propagation just merged with #792 <#792>: https://github.com/corazawaf/coraza/blob/v3/dev/internal/corazawaf/transaction.go#L489-L497. Basically the matched rule is marked as Disruptive only if the engine is on, not only if we have a disruptive action. Generally speaking, I agree, the status in the end is enforced by the connector, we should propagate it from there, but it is something that I think we are not doing considering that we are just hardcoding status codes 🤔 — Reply to this email directly, view it on GitHub <#795 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAUGLF6SHYXLLMK33MLXGZDVZANCNFSM6AAAAAAYD7E23Y> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[anuraaga]

Since we provide the transaction id to the connector, maybe it's just the job of the connector to log status in its normal http access log way with that ID included rather than trying to pass back to waf, which we've found to be challenging. Especially in a proxy, the real status code may end up being changed by a different filter anyways causing a mismatch between waf and http log.

[jcchavezs]

So as far as I understand even if we pass the right one, is still meaningless as exposed by @anuraaga. Shall we remove it then? cc @M4tteoP

[M4tteoP]

Removing it would be a change like this: #800.
I agree that printing the error code should be at least delegated to the connector when it enforces the interruption (but still it would be not necessarily the real status code)