Repository: corazawaf/coraza (go)

Issue:
AuditLog initialized for every logging related directive
Every audit log directive calls Init

https://github.com/corazawaf/coraza/blob/v3/dev/internal/seclang/directives.go#L656

This is despite there being heavyweight stuff like file opening happening during init

https://github.com/corazawaf/coraza/blob/v3/dev/auditlog/concurrent_writer.go#L41

We need to change it so log is only initialized once at the end, we did something similar recently. It would be good to find all similar cases and squash them

Pull request description:
chore: avoids initializing the audit log on every directive.
Audit log is more tricky than debuglog as there are many variables in play: StorageDir, dirMode, fileMode, file. Hence we can do the immutable pattern because that would be overkill.

Closes #698

Patch:
diff --git a/actions/ctl.go b/actions/ctl.go
index 42137f1..b50fc3a 100644
--- a/actions/ctl.go
+++ b/actions/ctl.go
@@ -355,7 +355,7 @@ func parseCtl(data string) (ctlFunctionType, string, variables.RuleVariable, str
 	return act, value, collection, strings.TrimSpace(colkey), nil
 }
 
-func rangeToInts(rules []*corazawaf.Rule, input string) ([]int, error) {
+func rangeToInts(rules []corazawaf.Rule, input string) ([]int, error) {
 	if len(input) == 0 {
 		return nil, errors.New("empty input")
 	}
diff --git a/auditlog/concurrent_writer.go b/auditlog/concurrent_writer.go
index a5dfe8b..abf49df 100644
--- a/auditlog/concurrent_writer.go
+++ b/auditlog/concurrent_writer.go
@@ -17,38 +17,42 @@ import (
 )
 
 type concurrentWriter struct {
-	mux           *sync.RWMutex
-	auditlogger   *log.Logger
-	auditDir      string
-	auditDirMode  fs.FileMode
-	auditFileMode fs.FileMode
-	formatter     Formatter
-	closer        func() error
+	mux         *sync.RWMutex
+	log         *log.Logger
+	logDir      string
+	logDirMode  fs.FileMode
+	logFileMode fs.FileMode
+	formatter   Formatter
+	io.Closer
 }
 
 func (cl *concurrentWriter) Init(c Config) error {
-	cl.auditFileMode = c.FileMode
-	cl.auditDir = c.Dir
-	cl.auditDirMode = c.DirMode
+	if c.File == "" {
+		cl.Closer = noopCloser{}
+		return nil
+	}
+
+	cl.logFileMode = c.FileMode
+	cl.logDir = c.Dir
+	cl.logDirMode = c.DirMode
 	cl.formatter = c.Formatter
 	cl.mux = &sync.RWMutex{}
 
-	w := io.Discard
-	if c.File != "" {
-		f, err := os.OpenFile(c.File, os.O_CREATE|os.O_WRONLY|os.O_APPEND, cl.auditFileMode)
-		if err != nil {
-			return err
-		}
-		w = f
-		cl.closer = f.Close
-	} else {
-		cl.closer = func() error { return nil }
+	f, err := os.OpenFile(c.File, os.O_CREATE|os.O_WRONLY|os.O_APPEND, cl.logFileMode)
+	if err != nil {
+		return err
 	}
-	cl.auditlogger = log.New(w, "", 0)
+	cl.Closer = f
+
+	cl.log = log.New(f, "", 0)
 	return nil
 }
 
 func (cl concurrentWriter) Write(al *Log) error {
+	if cl.formatter == nil {
+		return nil
+	}
+
 	// 192.168.3.130 192.168.3.1 - - [22/Aug/2009:13:24:20 +0100] "GET / HTTP/1.1" 200 56 "-" "-" SojdH8AAQEAAAugAQAAAAAA "-" /20090822/20090822-1324/20090822-132420-SojdH8AAQEAAAugAQAAAAAA 0 1248
 	t := time.Unix(0, al.Transaction.UnixTimestamp)
 
@@ -56,38 +60,34 @@ func (cl concurrentWriter) Write(al *Log) error {
 	ymdhm := ymd + t.Format("-1504")
 	filename := ymdhm + t.Format("05") + "-" + al.Transaction.ID
 
-	logdir := path.Join(cl.auditDir, ymd, ymdhm)
-	if err := os.MkdirAll(logdir, cl.auditDirMode); err != nil {
+	logdir := path.Join(cl.logDir, ymd, ymdhm)
+	if err := os.MkdirAll(logdir, cl.logDirMode); err != nil {
 		return err
 	}
 
-	jsdata, err := cl.formatter(al)
+	formattedAL, err := cl.formatter(al)
 	if err != nil {
 		return err
 	}
 
 	filepath := path.Join(logdir, filename)
-	if err = os.WriteFile(filepath, jsdata, cl.auditFileMode); err != nil {
+	if err = os.WriteFile(filepath, formattedAL, cl.logFileMode); err != nil {
 		return err
 	}
 
 	cl.mux.Lock()
 	defer cl.mux.Unlock()
 
-	cl.auditlogger.Printf("%s %s - - [%s]", al.Transaction.ClientIP, al.Transaction.HostIP, al.Transaction.Timestamp)
+	cl.log.Printf("%s %s - - [%s]", al.Transaction.ClientIP, al.Transaction.HostIP, al.Transaction.Timestamp)
 	if al.Transaction.Request != nil {
-		cl.auditlogger.Printf(` "%s %s %s"`, al.Transaction.Request.Method, al.Transaction.Request.URI, al.Transaction.Request.HTTPVersion)
+		cl.log.Printf(` "%s %s %s"`, al.Transaction.Request.Method, al.Transaction.Request.URI, al.Transaction.Request.HTTPVersion)
 	}
 	if al.Transaction.Response != nil {
-		cl.auditlogger.Printf(` %d`, al.Transaction.Response.Status)
+		cl.log.Printf(` %d`, al.Transaction.Response.Status)
 	}
-	cl.auditlogger.Printf("%s - %s\n", al.Transaction.ID, filepath)
+	cl.log.Printf("%s - %s\n", al.Transaction.ID, filepath)
 
 	return nil
 }
 
-func (cl *concurrentWriter) Close() error {
-	return cl.closer()
-}
-
 var _ Writer = (*concurrentWriter)(nil)
diff --git a/auditlog/noop_closer.go b/auditlog/noop_closer.go
new file mode 100644
index 0000000..643c87f
--- /dev/null
+++ b/auditlog/noop_closer.go
@@ -0,0 +1,5 @@
+package auditlog
+
+type noopCloser struct{}
+
+func (noopCloser) Close() error { return nil }
diff --git a/auditlog/serial_writer.go b/auditlog/serial_writer.go
index 979ba56..45d47f6 100644
--- a/auditlog/serial_writer.go
+++ b/auditlog/serial_writer.go
@@ -14,29 +14,28 @@ import (
 
 // serialWriter is used to store logs in a single file
 type serialWriter struct {
-	closer    func() error
+	io.Closer
 	log       log.Logger
 	formatter Formatter
 }
 
 func (sl *serialWriter) Init(c Config) error {
+	if c.File == "" {
+		sl.Closer = noopCloser{}
+		return nil
+	}
+
 	fileMode := c.FileMode
 	sl.formatter = c.Formatter
 
-	var w io.Writer
-	if c.File != "" {
-		f, err := os.OpenFile(c.File, os.O_APPEND|os.O_CREATE|os.O_WRONLY, fileMode)
-		if err != nil {
-			return err
-		}
-		w = f
-		sl.closer = f.Close
-	} else {
-		w = io.Discard
-		sl.closer = func() error { return nil }
+	f, err := os.OpenFile(c.File, os.O_APPEND|os.O_CREATE|os.O_WRONLY, fileMode)
+	if err != nil {
+		return err
 	}
+	sl.Closer = f
+
 	sl.log.SetFlags(0)
-	sl.log.SetOutput(w)
+	sl.log.SetOutput(f)
 	return nil
 }
 
@@ -53,8 +52,4 @@ func (sl *serialWriter) Write(al *Log) error {
 	return nil
 }
 
-func (sl *serialWriter) Close() error {
-	return sl.closer()
-}
-
 var _ Writer = (*serialWriter)(nil)
diff --git a/config.go b/config.go
index 3cb98f2..2e651f2 100644
--- a/config.go
+++ b/config.go
@@ -208,12 +208,12 @@ func (c *wafConfig) WithResponseBodyMimeTypes(mimeTypes []string) WAFConfig {
 type auditLogConfig struct {
 	relevantOnly bool
 	parts        types.AuditLogParts
-	logger       auditlog.Writer
+	writer       auditlog.Writer
 }
 
 func (c *auditLogConfig) LogRelevantOnly() AuditLogConfig {
 	ret := c.clone()
-	c.relevantOnly = true
+	ret.relevantOnly = true
 	return ret
 }
 
@@ -225,7 +225,7 @@ func (c *auditLogConfig) WithParts(parts types.AuditLogParts) AuditLogConfig {
 
 func (c *auditLogConfig) WithLogger(logger auditlog.Writer) AuditLogConfig {
 	ret := c.clone()
-	ret.logger = logger
+	ret.writer = logger
 	return ret
 }
 
diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go
index 050ba45..76af7d9 100644
--- a/internal/corazawaf/rule.go
+++ b/internal/corazawaf/rule.go
@@ -512,6 +512,7 @@ func (r *Rule) executeTransformations(value string) (string, []error) {
 }
 
 // NewRule returns a new initialized rule
+// By default, the rule is set to phase 2
 func NewRule() *Rule {
 	return &Rule{
 		RuleMetadata: corazarules.RuleMetadata{
diff --git a/internal/corazawaf/rulegroup.go b/internal/corazawaf/rulegroup.go
index 96c9415..214f22c 100644
--- a/internal/corazawaf/rulegroup.go
+++ b/internal/corazawaf/rulegroup.go
@@ -18,7 +18,7 @@ import (
 // It is not concurrent safe, so it's not recommended to use it
 // after compilation
 type RuleGroup struct {
-	rules []*Rule
+	rules []Rule
 }
 
 // Add a rule to the collection
@@ -29,7 +29,7 @@ func (rg *RuleGroup) Add(rule *Rule) error {
 		return nil
 	}
 
-	if rg.FindByID(rule.ID_) != nil && rule.ID_ != 0 {
+	if rule.ID_ != 0 && rg.FindByID(rule.ID_) != nil {
 		return fmt.Errorf("there is a another rule with id %d", rule.ID_)
 	}
 
@@ -45,20 +45,20 @@ func (rg *RuleGroup) Add(rule *Rule) error {
 		}
 	}
 
-	rg.rules = append(rg.rules, rule)
+	rg.rules = append(rg.rules, *rule)
 	return nil
 }
 
 // GetRules returns the slice of rules,
-func (rg *RuleGroup) GetRules() []*Rule {
+func (rg *RuleGroup) GetRules() []Rule {
 	return rg.rules
 }
 
 // FindByID return a Rule with the requested Id
 func (rg *RuleGroup) FindByID(id int) *Rule {
-	for _, r := range rg.rules {
+	for i, r := range rg.rules {
 		if r.ID_ == id {
-			return r
+			return &rg.rules[i]
 		}
 	}
 	return nil
@@ -67,34 +67,33 @@ func (rg *RuleGroup) FindByID(id int) *Rule {
 // DeleteByID removes a rule by it's Id
 func (rg *RuleGroup) DeleteByID(id int) {
 	for i, r := range rg.rules {
-		if r != nil && r.ID_ == id {
-			copy(rg.rules[i:], rg.rules[i+1:])
-			rg.rules[len(rg.rules)-1] = nil
-			rg.rules = rg.rules[:len(rg.rules)-1]
+		if r.ID_ == id {
+			rg.rules = append(rg.rules[:i], rg.rules[i+1:]...)
+			return
 		}
 	}
 }
 
-// FindByMsg returns a slice of rules that matches the msg
-func (rg *RuleGroup) FindByMsg(msg string) []*Rule {
-	var rules []*Rule
+// DeleteByMsg deletes rules with the given message.
+func (rg *RuleGroup) DeleteByMsg(msg string) {
+	var kept []Rule
 	for _, r := range rg.rules {
-		if r.Msg.String() == msg {
-			rules = append(rules, r)
+		if r.Msg.String() != msg {
+			kept = append(kept, r)
 		}
 	}
-	return rules
+	rg.rules = kept
 }
 
-// FindByTag returns a slice of rules that matches the tag
-func (rg *RuleGroup) FindByTag(tag string) []*Rule {
-	var rules []*Rule
+// DeleteByTag deletes rules with the given tag.
+func (rg *RuleGroup) DeleteByTag(tag string) {
+	var kept []Rule
 	for _, r := range rg.rules {
-		if strings.InSlice(tag, r.Tags_) {
-			rules = append(rules, r)
+		if !strings.InSlice(tag, r.Tags_) {
+			kept = append(kept, r)
 		}
 	}
-	return rules
+	rg.rules = kept
 }
 
 // Count returns the count of rules
@@ -102,11 +101,6 @@ func (rg *RuleGroup) Count() int {
 	return len(rg.rules)
 }
 
-// Clear will remove each and every rule stored
-func (rg *RuleGroup) Clear() {
-	rg.rules = []*Rule{}
-}
-
 // Eval rules for the specified phase, between 1 and 5
 // Rules are evaluated in syntactic order and the evaluation finishes
 // as soon as an interruption has been triggered.
@@ -124,7 +118,8 @@ func (rg *RuleGroup) Eval(phase types.RulePhase, tx *Transaction) bool {
 		delete(transformationCache, k)
 	}
 RulesLoop:
-	for _, r := range tx.WAF.Rules.GetRules() {
+	for i := range rg.rules {
+		r := &rg.rules[i]
 		// if there is already an interruption and the phase isn't logging
 		// we break the loop
 		if tx.interruption != nil && phase != types.PhaseLogging {
@@ -226,9 +221,7 @@ RulesLoop:
 // You might use this function to replace the rules
 // and "reload" the WAF
 func NewRuleGroup() RuleGroup {
-	return RuleGroup{
-		rules: []*Rule{},
-	}
+	return RuleGroup{}
 }
 
 type transformationKey struct {
diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
index eff912c..a6bceda 100644
--- a/internal/corazawaf/transaction.go
+++ b/internal/corazawaf/transaction.go
@@ -1246,13 +1246,11 @@ func (tx *Transaction) ProcessLogging() {
 	tx.debugLogger.Debug().
 		Msg("Transaction marked for audit logging")
 
-	if writer := tx.WAF.AuditLogWriter; writer != nil {
-		// We don't log if there is an empty audit logger
-		if err := writer.Write(tx.AuditLog()); err != nil {
-			tx.debugLogger.Error().
-				Err(err).
-				Msg("Failed to write audit log")
-		}
+	// We don't log if there is an empty audit logger
+	if err := tx.WAF.AuditLogWriter().Write(tx.AuditLog()); err != nil {
+		tx.debugLogger.Error().
+			Err(err).
+			Msg("Failed to write audit log")
 	}
 }
 
diff --git a/internal/corazawaf/waf.go b/internal/corazawaf/waf.go
index 9c3fc5f..24e3cda 100644
--- a/internal/corazawaf/waf.go
+++ b/internal/corazawaf/waf.go
@@ -37,12 +37,6 @@ type WAF struct {
 	// ruleGroup object, contains all rules and helpers
 	Rules RuleGroup
 
-	// Audit mode status
-	AuditEngine types.AuditEngineStatus
-
-	// Array of logging parts to be used
-	AuditLogParts types.AuditLogParts
-
 	// If true, transactions will have access to the request body
 	RequestBodyAccess bool
 
@@ -70,9 +64,6 @@ type WAF struct {
 	// Add significant rule components to audit log
 	ComponentNames []string
 
-	// Contains the regular expression for relevant status audit logging
-	AuditLogRelevantStatus *regexp.Regexp
-
 	// If true WAF engine will fail when remote rules cannot be loaded
 	AbortOnRemoteRulesFail bool
 
@@ -120,8 +111,22 @@ type WAF struct {
 
 	ErrorLogCb func(rule types.MatchedRule)
 
-	// AuditLogWriter is used to write audit logs
-	AuditLogWriter auditlog.Writer
+	// Audit mode status
+	AuditEngine types.AuditEngineStatus
+
+	// Array of logging parts to be used
+	AuditLogParts types.AuditLogParts
+
+	// Contains the regular expression for relevant status audit logging
+	AuditLogRelevantStatus *regexp.Regexp
+
+	auditLogWriter auditlog.Writer
+
+	// AuditLogWriterConfig is configuration of audit logging, populated by multiple directives and consumed by
+	// SecAuditLog.
+	AuditLogWriterConfig auditlog.Config
+
+	auditLogWriterInitialized bool
 }
 
 // NewTransaction Creates a new initialized transaction for this WAF instance
@@ -264,13 +269,15 @@ func NewWAF() *WAF {
 		// Initializing pool for transactions
 		txPool: sync.NewPool(func() interface{} { return new(Transaction) }),
 		// These defaults are unavoidable as they are zero values for the variables
-		RuleEngine:         types.RuleEngineOn,
-		RequestBodyAccess:  false,
-		RequestBodyLimit:   _1gb,
-		ResponseBodyAccess: false,
-		ResponseBodyLimit:  _1gb,
-		AuditLogWriter:     logWriter,
-		Logger:             logger,
+		RuleEngine:                types.RuleEngineOn,
+		RequestBodyAccess:         false,
+		RequestBodyLimit:          _1gb,
+		ResponseBodyAccess:        false,
+		ResponseBodyLimit:         _1gb,
+		auditLogWriter:            logWriter,
+		auditLogWriterInitialized: false,
+		AuditLogWriterConfig:      auditlog.NewConfig(),
+		Logger:                    logger,
 	}
 
 	if environment.HasAccessToFS {
@@ -295,6 +302,40 @@ func (w *WAF) SetDebugLogLevel(lvl debuglog.Level) error {
 	return nil
 }
 
+// SetAuditLogWriter sets the audit log writer
+func (w *WAF) SetAuditLogWriter(alw auditlog.Writer) {
+	w.auditLogWriter = alw
+}
+
+// AuditLogWriter returns the audit log writer. If the writer is not initialized,
+// it will be initialized
+func (w *WAF) AuditLogWriter() auditlog.Writer {
+	if !w.auditLogWriterInitialized {
+		if err := w.auditLogWriter.Init(w.AuditLogWriterConfig); err != nil {
+			w.Logger.Error().Err(err).Msg("Failed to initialize audit log")
+		}
+	}
+
+	return w.auditLogWriter
+}
+
+// InitAuditLogWriter initializes the audit log writer. If the writer is already
+// initialized, it will return an error as initializing the audit log writer twice
+// seems to be a bug.
+func (w *WAF) InitAuditLogWriter() error {
+	if w.auditLogWriterInitialized {
+		return errors.New("audit log writer already initialized")
+	}
+
+	if err := w.auditLogWriter.Init(w.AuditLogWriterConfig); err != nil {
+		return err
+	}
+
+	w.auditLogWriterInitialized = true
+
+	return nil
+}
+
 // SetErrorCallback sets the callback function for error logging
 // The error callback receives all the error data and some
 // helpers to write modsecurity style logs
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
index 50d3fca..1d2f3da 100644
--- a/internal/seclang/directives.go
+++ b/internal/seclang/directives.go
@@ -25,14 +25,11 @@ import (
 // TODO(anuraaga): Propagation of config probably should be separated from a directive's options.
 type DirectiveOptions struct {
 	WAF      *corazawaf.WAF
+	Raw      string
 	Opts     string
 	Path     []string
 	Datasets map[string][]string
 
-	// AuditLog is configuration of audit logging, populated by multiple directives and consumed by
-	// SecAuditLog.
-	AuditLog auditlog.Config
-
 	// Parser is configuration of the parser, populated by multiple directives and consumed by
 	// directives that parse.
 	Parser ParserConfig
@@ -140,6 +137,7 @@ func directiveSecAction(options *DirectiveOptions) error {
 		WithOperator: false,
 		WAF:          options.WAF,
 		ParserConfig: options.Parser,
+		Raw:          options.Raw,
 		Directive:    "SecAction",
 		Data:         options.Opts,
 	})
@@ -180,6 +178,7 @@ func directiveSecRule(options *DirectiveOptions) error {
 		WithOperator: true,
 		WAF:          options.WAF,
 		ParserConfig: options.Parser,
+		Raw:          options.Raw,
 		Directive:    "SecRule",
 		Data:         options.Opts,
 	})
@@ -310,9 +309,7 @@ func directiveSecRuleRemoveByTag(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	for _, r := range options.WAF.Rules.FindByTag(options.Opts) {
-		options.WAF.Rules.DeleteByID(r.ID_)
-	}
+	options.WAF.Rules.DeleteByTag(options.Opts)
 	return nil
 }
 
@@ -321,9 +318,7 @@ func directiveSecRuleRemoveByMsg(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	for _, r := range options.WAF.Rules.FindByMsg(options.Opts) {
-		options.WAF.Rules.DeleteByID(r.ID_)
-	}
+	options.WAF.Rules.DeleteByMsg(options.Opts)
 	return nil
 }
 
@@ -583,10 +578,8 @@ func directiveSecAuditLog(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	options.AuditLog.File = options.Opts
-	if err := options.WAF.AuditLogWriter.Init(options.AuditLog); err != nil {
-		return err
-	}
+	options.WAF.AuditLogWriterConfig.File = options.Opts
+
 	return nil
 }
 
@@ -599,10 +592,8 @@ func directiveSecAuditLogType(options *DirectiveOptions) error {
 	if err != nil {
 		return err
 	}
-	if err := writer.Init(options.AuditLog); err != nil {
-		return err
-	}
-	options.WAF.AuditLogWriter = writer
+	options.WAF.SetAuditLogWriter(writer)
+
 	return nil
 }
 
@@ -619,11 +610,8 @@ func directiveSecAuditLogFormat(options *DirectiveOptions) error {
 	if err != nil {
 		return err
 	}
-	options.AuditLog.Formatter = formatter
+	options.WAF.AuditLogWriterConfig.Formatter = formatter
 
-	if err := options.WAF.AuditLogWriter.Init(options.AuditLog); err != nil {
-		return err
-	}
 	return nil
 }
 
@@ -632,10 +620,8 @@ func directiveSecAuditLogDir(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	options.AuditLog.Dir = options.Opts
-	if err := options.WAF.AuditLogWriter.Init(options.AuditLog); err != nil {
-		return err
-	}
+	options.WAF.AuditLogWriterConfig.Dir = options.Opts
+
 	return nil
 }
 
@@ -660,10 +646,8 @@ func directiveSecAuditLogDirMode(options *DirectiveOptions) error {
 	if err != nil {
 		return err
 	}
-	options.AuditLog.DirMode = fs.FileMode(auditLogDirMode)
-	if err := options.WAF.AuditLogWriter.Init(options.AuditLog); err != nil {
-		return err
-	}
+	options.WAF.AuditLogWriterConfig.DirMode = fs.FileMode(auditLogDirMode)
+
 	return nil
 }
 
@@ -686,10 +670,8 @@ func directiveSecAuditLogFileMode(options *DirectiveOptions) error {
 	if err != nil {
 		return err
 	}
-	options.AuditLog.FileMode = fs.FileMode(auditLogFileMode)
-	if err := options.WAF.AuditLogWriter.Init(options.AuditLog); err != nil {
-		return err
-	}
+	options.WAF.AuditLogWriterConfig.FileMode = fs.FileMode(auditLogFileMode)
+
 	return nil
 }
 
diff --git a/internal/seclang/parser.go b/internal/seclang/parser.go
index 79f91e4..1606f49 100644
--- a/internal/seclang/parser.go
+++ b/internal/seclang/parser.go
@@ -12,7 +12,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/corazawaf/coraza/v3/auditlog"
 	"github.com/corazawaf/coraza/v3/internal/corazawaf"
 	"github.com/corazawaf/coraza/v3/internal/environment"
 	"github.com/corazawaf/coraza/v3/internal/io"
@@ -135,8 +134,7 @@ func (p *Parser) FromString(data string) error {
 
 func (p *Parser) evaluateLine(l string) error {
 	if l == "" || l[0] == '#' {
-		// at this point we should not receive an empty line or a commented line
-		return errors.New("invalid line")
+		panic("invalid line")
 	}
 	// first we get the directive
 	dir, opts, _ := strings.Cut(l, " ")
@@ -163,6 +161,7 @@ func (p *Parser) evaluateLine(l string) error {
 		return p.log(fmt.Sprintf("unknown directive %q", directive))
 	}
 
+	p.options.Raw = l
 	p.options.Opts = opts
 	p.options.Parser.LastLine = p.currentLine
 	p.options.Parser.ConfigFile = p.currentFile
@@ -206,7 +205,6 @@ func NewParser(waf *corazawaf.WAF) *Parser {
 		options: &DirectiveOptions{
 			WAF:      waf,
 			Datasets: make(map[string][]string),
-			AuditLog: auditlog.NewConfig(),
 		},
 		root: io.OSFS{},
 	}
diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index 6fb483c..9e229af 100644
--- a/internal/seclang/rule_parser.go
+++ b/internal/seclang/rule_parser.go
@@ -17,7 +17,7 @@ import (
 	"github.com/corazawaf/coraza/v3/types/variables"
 )
 
-var defaultActionsPhase2 = []string{"phase:2,log,auditlog,pass"}
+var defaultActionsPhase2 = "phase:2,log,auditlog,pass"
 
 type ruleAction struct {
 	Key   string
@@ -172,10 +172,8 @@ func (p *RuleParser) ParseOperator(operator string) error {
 	// We clone strings to ensure a slice into larger rule definition isn't kept in
 	// memory just to store operator information.
 	opRaw, opdataRaw, _ := strings.Cut(operator, " ")
-	opRaw = strings.Clone(opRaw)
 	op := strings.TrimSpace(opRaw)
 	opdata := strings.TrimSpace(opdataRaw)
-	opdata = strings.Clone(opdata)
 
 	if op[0] == '@' {
 		// we trim @
@@ -230,12 +228,23 @@ func (p *RuleParser) ParseDefaultActions(actions string) error {
 		if action.Atype == rules.ActionTypeDisruptive {
 			defaultDisruptive = action.Key
 		}
+		// SecDefaultActions can not contain metadata actions
+		if action.Atype == rules.ActionTypeMetadata {
+			return fmt.Errorf("SecDefaultAction must not contain metadata actions: %s", actions)
+		}
+		// Transformations are not suitable to be part of the default actions defined by SecDefaultActions
+		if action.Key == "t" {
+			return fmt.Errorf("SecDefaultAction must not contain transformation actions: %s", actions)
+		}
 	}
 	if phase == 0 {
-		return errors.New("SecDefaultAction must contain a phase")
+		return fmt.Errorf("SecDefaultAction must contain a phase")
 	}
 	if defaultDisruptive == "" {
-		return errors.New("SecDefaultAction must contain a disruptive action: " + actions)
+		return fmt.Errorf("SecDefaultAction must contain a disruptive action: %s", actions)
+	}
+	if p.defaultActions[types.RulePhase(phase)] != nil {
+		return fmt.Errorf("SecDefaultAction already defined for this phase: %s", actions)
 	}
 	p.defaultActions[types.RulePhase(phase)] = act
 	return nil
@@ -264,6 +273,7 @@ func (p *RuleParser) ParseActions(actions string) error {
 		}
 	}
 
+	// if the rule is missing the phase, the default phase assigned is phase 2 (See NewRule())
 	phase := p.rule.Phase_
 
 	defaults := p.defaultActions[phase]
@@ -296,6 +306,7 @@ type RuleOptions struct {
 	WithOperator bool
 	WAF          *corazawaf.WAF
 	ParserConfig ParserConfig
+	Raw          string
 	Directive    string
 	Data         string
 }
@@ -315,19 +326,27 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 		rule:           corazawaf.NewRule(),
 		defaultActions: map[types.RulePhase][]ruleAction{},
 	}
-
-	defaultActions := defaultActionsPhase2
+	var defaultActionsRaw []string
+	// Default actions are persisted only inside the ParserConfig, therefore they are parsed every time a rule is parsed
+	// and not just once when the SecDefaultAction is read.
 	if options.ParserConfig.HasRuleDefaultActions {
-		defaultActions = options.ParserConfig.RuleDefaultActions
+		defaultActionsRaw = options.ParserConfig.RuleDefaultActions
 	}
 	disabledRuleOperators := options.ParserConfig.DisabledRuleOperators
+	for _, da := range defaultActionsRaw {
 
-	for _, da := range defaultActions {
 		err = rp.ParseDefaultActions(da)
 		if err != nil {
 			return nil, err
 		}
 	}
+	// If no default actions for phase 2 are defined, defaultActionsPhase2 variable (hardcoded default actions for phase 2) is used.
+	if rp.defaultActions[types.PhaseRequestBody] == nil {
+		err = rp.ParseDefaultActions(defaultActionsPhase2)
+		if err != nil {
+			return nil, err
+		}
+	}
 	actions := ""
 
 	if options.WithOperator {
@@ -358,7 +377,7 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 		}
 	}
 	rule := rp.Rule()
-	rule.Raw_ = fmt.Sprintf("%s %s", options.Directive, options.Data)
+	rule.Raw_ = options.Raw
 	rule.File_ = options.ParserConfig.ConfigFile
 	rule.Line_ = options.ParserConfig.LastLine
 
@@ -435,7 +454,8 @@ func getLastRuleExpectingChain(w *corazawaf.WAF) *corazawaf.Rule {
 	if len(rules) == 0 {
 		return nil
 	}
-	lastRule := rules[len(rules)-1]
+
+	lastRule := &rules[len(rules)-1]
 	parent := lastRule
 	for parent.Chain != nil {
 		parent = parent.Chain
@@ -444,6 +464,7 @@ func getLastRuleExpectingChain(w *corazawaf.WAF) *corazawaf.Rule {
 	if parent.HasChain && parent.Chain == nil {
 		return lastRule
 	}
+
 	return nil
 }
 
@@ -514,8 +535,8 @@ func parseActions(actions string) ([]ruleAction, error) {
 }
 
 func appendRuleAction(res []ruleAction, key string, val string, disruptiveActionIndex int) ([]ruleAction, int, error) {
-	key = strings.TrimSpace(key)
-	val = strings.TrimSpace(val)
+	key = strings.ToLower(strings.TrimSpace(key))
+	val = strings.TrimSpace(val) // We may want to keep case sensitive values (e.g. Messages)
 	val = utils.MaybeRemoveQuotes(val)
 	f, err := actionsmod.Get(key)
 	if err != nil {
diff --git a/waf.go b/waf.go
index 2ad7d4d..348cafa 100644
--- a/waf.go
+++ b/waf.go
@@ -58,7 +58,6 @@ func NewWAF(config WAFConfig) (WAF, error) {
 	}
 
 	if a := c.auditLog; a != nil {
-		// TODO(anuraaga): Can't override AuditEngineOn from rules to off this way.
 		if a.relevantOnly {
 			waf.AuditEngine = types.AuditEngineRelevantOnly
 		} else {
@@ -67,11 +66,15 @@ func NewWAF(config WAFConfig) (WAF, error) {
 
 		waf.AuditLogParts = a.parts
 
-		if a.logger != nil {
-			waf.AuditLogWriter = a.logger
+		if a.writer != nil {
+			waf.SetAuditLogWriter(a.writer)
 		}
 	}
 
+	if err := waf.InitAuditLogWriter(); err != nil {
+		return nil, fmt.Errorf("invalid WAF config from audit log: %w", err)
+	}
+
 	if c.requestBodyAccess {
 		waf.RequestBodyAccess = true
 	}


Test patch:
diff --git a/actions/ctl_test.go b/actions/ctl_test.go
index fd10599..ca9a443 100644
--- a/actions/ctl_test.go
+++ b/actions/ctl_test.go
@@ -424,7 +424,7 @@ func TestParseCtl(t *testing.T) {
 
 }
 func TestCtlParseRange(t *testing.T) {
-	rules := []*corazawaf.Rule{
+	rules := []corazawaf.Rule{
 		{
 			RuleMetadata: corazarules.RuleMetadata{
 				ID_: 5,
diff --git a/config_test.go b/config_test.go
index 1d329e9..f6e3ef6 100644
--- a/config_test.go
+++ b/config_test.go
@@ -122,22 +122,19 @@ func TestConfigLogger(t *testing.T) {
 		LogRelevantOnly().
 		WithLogger(logger).
 		WithParts([]types.AuditLogPart("abcdedf"))
+
 	cfg := NewWAFConfig().WithAuditLog(logCfg)
 	waf, err := NewWAF(cfg)
 	if err != nil {
 		t.Fatal(err)
 	}
 	w := waf.(wafWrapper)
-	// TODO(jptosso): this is not working, but there is a comment in the code
-	/*
-		if w.waf.AuditEngine != types.AuditEngineRelevantOnly {
-			t.Errorf("expected audit engine to be relevant only")
-		}
-	*/
-	if w.waf.AuditLogWriter == nil {
-		t.Errorf("expected audit log writer to be set")
-	}
+
 	if w.waf.AuditLogParts == nil {
 		t.Errorf("expected audit log parts to be set")
 	}
+
+	if w.waf.AuditEngine != types.AuditEngineRelevantOnly {
+		t.Errorf("expected audit engine to be relevant only")
+	}
 }
diff --git a/internal/corazawaf/rulegroup_test.go b/internal/corazawaf/rulegroup_test.go
index d6f87f8..8bf13f4 100644
--- a/internal/corazawaf/rulegroup_test.go
+++ b/internal/corazawaf/rulegroup_test.go
@@ -27,15 +27,20 @@ func TestRG(t *testing.T) {
 		t.Error("Failed to add rule to rulegroup")
 	}
 
-	if len(rg.FindByMsg("test")) != 1 {
-		t.Error("Failed to find rules by msg")
+	rg.DeleteByTag("test")
+	if rg.Count() != 0 {
+		t.Error("Failed to remove rule from rulegroup")
+	}
+
+	if err := rg.Add(r); err != nil {
+		t.Error("Failed to add rule to rulegroup")
 	}
 
-	if len(rg.FindByTag("test")) != 1 {
-		t.Error("Failed to find rules by tag")
+	if rg.Count() != 1 {
+		t.Error("Failed to add rule to rulegroup")
 	}
 
-	rg.DeleteByID(1)
+	rg.DeleteByMsg("test")
 	if rg.Count() != 0 {
 		t.Error("Failed to remove rule from rulegroup")
 	}
diff --git a/internal/seclang/directives_log_test.go b/internal/seclang/directives_log_test.go
index ef688a5..b148b79 100644
--- a/internal/seclang/directives_log_test.go
+++ b/internal/seclang/directives_log_test.go
@@ -23,8 +23,9 @@ import (
 
 func TestSecAuditLogDirectivesConcurrent(t *testing.T) {
 	waf := corazawaf.NewWAF()
-	auditpath := t.TempDir()
 	parser := NewParser(waf)
+
+	auditpath := t.TempDir()
 	if err := parser.FromString(fmt.Sprintf(`
 	SecAuditLog %s
 	SecAuditLogFormat json
@@ -33,20 +34,18 @@ func TestSecAuditLogDirectivesConcurrent(t *testing.T) {
 	SecAuditLogFileMode 0777
 	SecAuditLogType concurrent
 	`, filepath.Join(auditpath, "audit.log"), auditpath)); err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
+
 	id := utils.RandomString(10)
-	if waf.AuditLogWriter == nil {
-		t.Error("Invalid audit logger (nil)")
-		return
-	}
-	if err := waf.AuditLogWriter.Write(&auditlog.Log{
+
+	if err := waf.AuditLogWriter().Write(&auditlog.Log{
 		Parts: types.AuditLogParts("ABCDEFGHIJKZ"),
 		Transaction: auditlog.Transaction{
 			ID: id,
 		},
 	}); err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 	f, err := findFileContaining(auditpath, id)
 	if err != nil {
diff --git a/internal/seclang/rule_parser_test.go b/internal/seclang/rule_parser_test.go
index 4761fc2..a9af315 100644
--- a/internal/seclang/rule_parser_test.go
+++ b/internal/seclang/rule_parser_test.go
@@ -126,10 +126,83 @@ func TestSecRuleUpdateTargetVariableNegation(t *testing.T) {
 	}
 }
 
+func TestDefaultActionsErrors(t *testing.T) {
+	testCases := map[string]struct {
+		rules string
+	}{
+		"SecDefaultAction with bad actions": {
+			rules: `SecDefaultAction "logauditlog,pass"`,
+		},
+		"Missing phase": {
+			rules: `SecDefaultAction "log,auditlog,pass"`,
+		},
+		"Bad phase": {
+			rules: `SecDefaultAction "phase:A,log,auditlog,pass"`,
+		},
+		"Missing disruptive action": {
+			rules: `SecDefaultAction "phase:1,log,auditlog"`,
+		},
+		"SecDefaultAction with metadata action": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass,msg:'metadata action'"`,
+		},
+		"SecDefaultAction with a transformation": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass,t:none"`,
+		},
+		"SecDefaultAction with a transformation uppercase": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass,T:NoNe"`,
+		},
+		"Multiple SecDefaultAction for the same phase": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass"
+			SecDefaultAction "phase:1,nolog,noauditlog,pass"`,
+		},
+	}
+	for name, tCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			dummySecAction := `
+			SecAction "id:1,phase:1" `
+			waf := corazawaf.NewWAF()
+			p := NewParser(waf)
+			// SecDefaultActions are parsed only when a rule is parsed, thus we add a dummy SecAction
+			err := p.FromString(tCase.rules + dummySecAction)
+			if err == nil {
+				t.Error("expected error")
+			}
+		})
+	}
+}
+
+func TestDefaultActionsForPhase2Overridable(t *testing.T) {
+	waf := corazawaf.NewWAF()
+	p := NewParser(waf)
+	// A SecDefaultAction at phase:2 defined by the user has to override the hardcoded defaultActionsPhase2
+	err := p.FromString(`
+	SecDefaultAction "phase:2,nolog,noauditlog,pass"
+	SecAction "id:1,noauditlog"
+	SecAction "id:2,phase:1"`)
+	if err != nil {
+		t.Errorf("unexpected error: %s", err.Error())
+	}
+	if waf.Rules.GetRules()[0].Log != false {
+		t.Error("failed to set log to false")
+	}
+	if waf.Rules.GetRules()[0].Audit != false {
+		t.Error("failed to set audit to false")
+	}
+	if waf.Rules.GetRules()[1].Log != false {
+		t.Error("phase 1 rules shouldn't have log set by default actions of different phases")
+	}
+	if waf.Rules.GetRules()[1].Audit != false {
+		t.Error("phase 1 rules shouldn't have log set by default actions of different phases")
+	}
+}
+
 func TestDefaultActionsForPhase2(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	p := NewParser(waf)
+	// Via defaultActionsPhase2 variable, the default actions for phase 2 are hardcoded in Coraza.
+	// Only a SecDefaultAction of the same phase should override it.
 	err := p.FromString(`
+	SecDefaultAction "phase:3,log,auditlog,pass"
 	SecAction "id:1,phase:2"
 	SecAction "id:2,phase:1"`)
 	if err != nil {
@@ -143,7 +216,7 @@ func TestDefaultActionsForPhase2(t *testing.T) {
 	}
 
 	if waf.Rules.GetRules()[1].Log || waf.Rules.GetRules()[1].Audit {
-		t.Error("phase 1 rules shouldn't have log set by default actions")
+		t.Error("phase 1 rules shouldn't have log set by default actions of different phases")
 	}
 }
 
diff --git a/testing/auditlog_test.go b/testing/auditlog_test.go
index 0a52f9f..8d1616f 100644
--- a/testing/auditlog_test.go
+++ b/testing/auditlog_test.go
@@ -26,10 +26,10 @@ func TestAuditLogMessages(t *testing.T) {
 	// generate a random tmp file
 	file, err := os.Create(filepath.Join(t.TempDir(), "tmp.log"))
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 	if err := parser.FromString(fmt.Sprintf("SecAuditLog %s", file.Name())); err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 	if err := parser.FromString(`
 		SecRuleEngine DetectionOnly
@@ -39,9 +39,10 @@ func TestAuditLogMessages(t *testing.T) {
 		SecAuditLogParts ABCDEFGHIJKZ
 		SecRule ARGS "@unconditionalMatch" "id:1,phase:1,log,msg:'unconditional match'"
 	`); err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
 	defer os.Remove(file.Name())
+
 	tx := waf.NewTransaction()
 	tx.AddGetRequestArgument("test", "test")
 	tx.ProcessRequestHeaders()
@@ -62,7 +63,7 @@ func TestAuditLogMessages(t *testing.T) {
 		t.Error(err)
 	}
 	if len(al2.Messages) != 1 {
-		t.Errorf("Expected 1 message, got %d", len(al2.Messages))
+		t.Fatalf("Expected 1 message, got %d", len(al2.Messages))
 	}
 	if al2.Messages[0].Message != "unconditional match" {
 		t.Errorf("Expected message %q, got %q", "unconditional match", al2.Messages[0].Message)
@@ -126,8 +127,9 @@ func TestAuditLogRelevantOnlyOk(t *testing.T) {
 		SecAuditLogRelevantStatus ".*"
 		SecRule ARGS "@unconditionalMatch" "id:1,phase:1,log,msg:'unconditional match'"
 	`); err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
+
 	tx := waf.NewTransaction()
 	tx.AddGetRequestArgument("test", "test")
 	tx.ProcessRequestHeaders()
diff --git a/testing/engine/disruptive_actions.go b/testing/engine/disruptive_actions.go
index e662820..2f18887 100644
--- a/testing/engine/disruptive_actions.go
+++ b/testing/engine/disruptive_actions.go
@@ -304,7 +304,9 @@ SecRule REQUEST_URI "/redirect5$" "phase:5,id:51,log,status:302,redirect:https:/
 SecRule REQUEST_URI "/deny5$" "phase:5,id:52,log,status:500,deny"
 SecRule REQUEST_URI "/drop5$" "phase:5,id:53,log,drop"
 
-SecDefaultAction phase:2,deny,status:501,log,logdata:'WOOOP_BLOCKED_BY_CORAZA_TEST'
-SecRule REQUEST_URI "/default/block" "id:103,block"
+# Rule 103 is missing the phase, therefore phase:2 is implicitly applied with its related default actions
+# So we will expect a deny with 501 response for the blocking action.
+SecDefaultAction "phase:2,deny,status:501,log"
+SecRule REQUEST_URI "/default/block" "id:103,block,logdata:'WOOOP_BLOCKED_BY_CORAZA_TEST'"
 `,
 })