audit: `H` should populate also with error logs. #1310

M4tteoP · 2025-02-19T23:22:03Z

Part of #1305.
Address point 1 of #1305 (comment)

codecov · 2025-02-19T23:29:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.99%. Comparing base (cb80190) to head (b5f4154).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1310      +/-   ##
==========================================
+ Coverage   81.94%   81.99%   +0.04%     
==========================================
  Files         170      170              
  Lines        9777     9803      +26     
==========================================
+ Hits         8012     8038      +26     
  Misses       1518     1518              
  Partials      247      247

Flag	Coverage Δ
coraza.rule.case_sensitive_args_keys	`81.95% <100.00%> (+0.04%)`	⬆️
coraza.rule.multiphase_valuation	`81.99% <100.00%> (+0.04%)`	⬆️
coraza.rule.no_regex_multiline	`81.93% <100.00%> (+0.04%)`	⬆️
default	`81.99% <100.00%> (+0.04%)`	⬆️
examples+	`16.50% <0.00%> (-0.05%)`	⬇️
examples+coraza.rule.case_sensitive_args_keys	`81.95% <100.00%> (+0.04%)`	⬆️
examples+coraza.rule.multiphase_valuation	`81.83% <100.00%> (+0.04%)`	⬆️
examples+coraza.rule.no_regex_multiline	`81.85% <100.00%> (+0.04%)`	⬆️
examples+memoize_builders	`81.95% <100.00%> (+0.04%)`	⬆️
examples+no_fs_access	`81.28% <100.00%> (+0.04%)`	⬆️
ftw	`81.99% <100.00%> (+0.04%)`	⬆️
memoize_builders	`82.08% <100.00%> (+0.04%)`	⬆️
no_fs_access	`81.44% <100.00%> (+0.04%)`	⬆️
tinygo	`81.96% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jcchavezs · 2025-03-08T11:33:44Z

types/rule_match.go

@@ -47,4 +47,6 @@ type MatchedRule interface {
 	AuditLog() string

 	ErrorLog() string
+
+	// TODO(4.x): Add Log()


Do you mind elaborating why?

I remember that with #848 we agreed that Log() was indeed helpful to be exported, but then to avoid breaking the Coraza v3.* API adding a Log() method to the MatchedRule interface we provided the workaround via the type assertion. So I guess that the proper outcome is to have it properly exported.
See also

coraza/internal/corazawaf/transaction.go

Lines 1499 to 1503 in e29a849

// Log action is required to log a matched rule on both error log and audit log

// An assertion has to be done to check if the MatchedRule implements the Log() function before calling Log()

// It is performed to avoid breaking the Coraza v3.* API adding a Log() method to the MatchedRule interface

mrWithlog, ok := mr.(*corazarules.MatchedRule)

if ok && mrWithlog.Log() {

jcchavezs · 2025-03-08T11:34:19Z

internal/auditlog/formats.go

+			for _, alEntry := range al.Messages() {
+				alWithErrMsg, ok := alEntry.(auditLogWithErrMesg)
+				if ok && alWithErrMsg.ErrorMessage() != "" {
+					res.WriteByte('\n')


Suggested change

res.WriteByte('\n')

fmt.Fprintln(res, "")

I can't use it as is with a string builder (complains of missing io.Writer). We are using in several places res.WriteByte('\n'), is it a performance or readability suggestion?

it is because the method is in the pointer not in the value. The main use here is to make this working on windows but I see this has been used before so we can keep it.

I see, thanks!

M4tteoP · 2025-03-14T23:55:50Z

Fixed go.sum conflicts

M4tteoP mentioned this pull request Feb 19, 2025

[owasp-modsecurity compatibility] SecAuditLogParts flags #1305

Open

M4tteoP force-pushed the audit_populate_H_error_logs branch from 1098a44 to df5b689 Compare March 2, 2025 22:14

M4tteoP marked this pull request as ready for review March 2, 2025 22:21

M4tteoP requested a review from a team as a code owner March 2, 2025 22:21

M4tteoP force-pushed the audit_populate_H_error_logs branch 2 times, most recently from 2169a67 to 560063e Compare March 6, 2025 23:12

jcchavezs reviewed Mar 8, 2025

View reviewed changes

M4tteoP force-pushed the audit_populate_H_error_logs branch from 560063e to 42601f5 Compare March 14, 2025 23:55

M4tteoP mentioned this pull request Mar 15, 2025

chore: update to golang 1.23.6 #1313

Merged

tty2 and others added 3 commits March 17, 2025 21:29

Tests with the expected behavior from Roman

4b71325

audit: populate H section with error logs

6b20bf3

improve test, don't specify go patch version

07c6a2d

M4tteoP force-pushed the audit_populate_H_error_logs branch from 42601f5 to 07c6a2d Compare March 17, 2025 21:01

Merge branch 'main' into audit_populate_H_error_logs

8e2f68d

jcchavezs approved these changes Mar 20, 2025

View reviewed changes

M4tteoP added 3 commits March 20, 2025 22:46

Merge branch 'main' into audit_populate_H_error_logs

ec9fb4d

Merge branch 'main' into audit_populate_H_error_logs

9273f3f

Merge branch 'main' into audit_populate_H_error_logs

b5f4154

M4tteoP merged commit 57eb446 into main Mar 21, 2025
72 checks passed

M4tteoP deleted the audit_populate_H_error_logs branch March 21, 2025 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audit: `H` should populate also with error logs. #1310

audit: `H` should populate also with error logs. #1310

M4tteoP commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

jcchavezs Mar 8, 2025

M4tteoP Mar 14, 2025

jcchavezs Mar 8, 2025

M4tteoP Mar 14, 2025

jcchavezs Mar 20, 2025

M4tteoP Mar 20, 2025

M4tteoP commented Mar 14, 2025

	// Log action is required to log a matched rule on both error log and audit log
	// An assertion has to be done to check if the MatchedRule implements the Log() function before calling Log()
	// It is performed to avoid breaking the Coraza v3.* API adding a Log() method to the MatchedRule interface
	mrWithlog, ok := mr.(*corazarules.MatchedRule)
	if ok && mrWithlog.Log() {

audit: H should populate also with error logs. #1310

audit: H should populate also with error logs. #1310

Conversation

M4tteoP commented Feb 19, 2025 • edited Loading

codecov bot commented Feb 19, 2025 • edited Loading

Codecov Report

jcchavezs Mar 8, 2025

Choose a reason for hiding this comment

M4tteoP Mar 14, 2025

Choose a reason for hiding this comment

jcchavezs Mar 8, 2025

Choose a reason for hiding this comment

M4tteoP Mar 14, 2025

Choose a reason for hiding this comment

jcchavezs Mar 20, 2025

Choose a reason for hiding this comment

M4tteoP Mar 20, 2025

Choose a reason for hiding this comment

M4tteoP commented Mar 14, 2025

audit: `H` should populate also with error logs. #1310

audit: `H` should populate also with error logs. #1310

M4tteoP commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading