fix: BodyLimit related documented default values, default RequestBodyLimitAction, adds some tests #895

M4tteoP · 2023-10-29T14:26:40Z

This PR:

Fixes SecRequestBodyLimit, SecRequestBodyInMemoryLimit, and SecResponseBodyLimit documented default values.
Fixes the default RequestBodyLimitAction
Explicits that SecRequestBodyNoFilesLimit is not implemented

Even if relying on the provided coraza.conf-recommended file makes all these default values not used, we should still keep consistency between the actual code and its documentation

codecov · 2023-10-29T14:30:14Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4ff1f76) 82.72% compared to head (64cdef0) 83.22%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
internal/corazawaf/rule.go	84.84%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
+ Coverage   82.72%   83.22%   +0.50%     
==========================================
  Files         162      162              
  Lines        9080     7552    -1528     
==========================================
- Hits         7511     6285    -1226     
+ Misses       1319     1019     -300     
+ Partials      250      248       -2

Flag	Coverage Δ
default	`83.22% <87.80%> (+5.38%)`	⬆️
examples	`83.22% <87.80%> (+56.79%)`	⬆️
ftw	`83.22% <87.80%> (+35.85%)`	⬆️
ftw-multiphase	`83.22% <87.80%> (+33.67%)`	⬆️
tinygo	`83.22% <87.80%> (+7.81%)`	⬆️

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.

M4tteoP · 2023-10-29T14:31:24Z

internal/corazawaf/waf.go

@@ -276,6 +277,7 @@ func NewWAF() *WAF {
 		RuleEngine:                types.RuleEngineOn,
 		RequestBodyAccess:         false,
 		RequestBodyLimit:          _1gb,
+		RequestBodyLimitAction:    types.BodyLimitActionReject,


As per https://coraza.io/docs/seclang/directives/#secrequestbodylimitaction, the default value for the request body limit action is Reject. We can't rely on the zero value of the BodyLimit action because we want to have two different default values for RequestBodyLimitAction (reject) and ResponseBodyLimitAction (processPartial). The first one has been explicited here

M4tteoP · 2023-10-29T14:33:00Z

internal/seclang/directives.go

@@ -229,7 +229,7 @@ func directiveSecResponseBodyAccess(options *DirectiveOptions) error {
 }

 // Description: Configures the maximum request body size Coraza will accept for buffering.
-// Default: 134217728 (131072 KB)
+// Default: 1073741824 (1024 MiB)


See

coraza/internal/corazawaf/waf.go

Line 278 in 4b1b82d

RequestBodyLimit: _1gb,

Some confusion here. _1gb is the hard limit and not the default one (1gb for the default one is cray IMO). Maybe it is better to set to -1 and fail if not set? cc @anuraaga

Since 6482ab1, seems like it is effectively not just the hard limit, but also the default one. Here I wanted to at least align the doc with the implementation.
I agree that it is not the best default value: I'm okay with both setting the default value to the one that users are used from ModSec, or making it a requirement.

This is basically the evolution of #612 (comment)

Yeah, it is the default one without applying the recommended conf. I think making body limit so huge could be problematic so maybe we align the documented default values with the ones in code? cc @anuraaga @jptosso

I agree about aligning code to doc

internal/seclang/directives.go

M4tteoP · 2023-10-29T14:34:24Z

internal/seclang/directives.go

@@ -448,7 +448,7 @@ func directiveSecRequestBodyLimitAction(options *DirectiveOptions) error {
 }

 // Description: Configures the maximum request body size that Coraza will store in memory.
-// Default: 131072 (128 KB)
+// Default: defaults to RequestBodyLimit


See

coraza/internal/corazawaf/waf.go

Lines 183 to 187 in 4b1b82d

// if no requestBodyInMemoryLimit has been set we default to the

var requestBodyInMemoryLimit int64 = w.RequestBodyLimit

if w.requestBodyInMemoryLimit != nil {

requestBodyInMemoryLimit = int64(*w.requestBodyInMemoryLimit)

}

M4tteoP · 2023-10-29T22:42:13Z

coraza.conf-recommended

@@ -45,7 +45,7 @@ SecRequestBodyLimit 13107200

 SecRequestBodyInMemoryLimit 131072

-SecRequestBodyNoFilesLimit 131072
+# SecRequestBodyNoFilesLimit 131072


Being not implemented, I think we should at least comment it out, not letting users think that it is an enforced configuration

Let's add a comment too, # Coraza currently does not read this parameter I think

Could you also open an issue for this? Are we supporting it @jptosso ?

I added the following comment: # SecRequestBodyNoFilesLimit is currently not supported by Coraza. As far as I can see, we actually just read the parameter, but we do not enforce any logic based on this.
Opened issue #896 about it.

anuraaga · 2023-10-30T01:10:56Z

coraza.conf-recommended

@@ -45,7 +45,7 @@ SecRequestBodyLimit 13107200

 SecRequestBodyInMemoryLimit 131072

-SecRequestBodyNoFilesLimit 131072
+# SecRequestBodyNoFilesLimit 131072


Let's add a comment too, # Coraza currently does not read this parameter I think

…a.conf-recommended

M4tteoP · 2023-12-13T15:28:10Z

Anyone PTAL, I think it is ready to be merged

jcchavezs · 2024-02-05T22:36:00Z

internal/seclang/directives.go

@@ -876,6 +876,8 @@ func directiveSecUploadDir(options *DirectiveOptions) error {
 	return nil
 }

+// Not implemented yet


Not sure this will work for autogeneration. Everything not being key:value (e.g. Description: Configures....) should go after the

// ---

Thanks, moved as // Note: not implemented yet as the last line

M4tteoP changed the title ~~Fix bodylimits doc~~ Fix BodyLimit related documentd default values, default RequestBodyLimitAction, adds some tests Oct 29, 2023

M4tteoP changed the title ~~Fix BodyLimit related documentd default values, default RequestBodyLimitAction, adds some tests~~ fix: BodyLimit related documentd default values, default RequestBodyLimitAction, adds some tests Oct 29, 2023

M4tteoP commented Oct 29, 2023

View reviewed changes

internal/seclang/directives.go Outdated Show resolved Hide resolved

M4tteoP commented Oct 29, 2023

View reviewed changes

M4tteoP added 2 commits October 29, 2023 15:36

fixes documented default values and default requestbodyLimit

f48ecbe

mod tidy

6be7862

M4tteoP force-pushed the fix_bodylimits_doc branch from eee0aa9 to 6be7862 Compare October 29, 2023 14:37

M4tteoP changed the title ~~fix: BodyLimit related documentd default values, default RequestBodyLimitAction, adds some tests~~ fix: BodyLimit related documented default values, default RequestBodyLimitAction, adds some tests Oct 29, 2023

M4tteoP marked this pull request as ready for review October 29, 2023 14:39

M4tteoP requested a review from a team as a code owner October 29, 2023 14:39

comments out SecRequestBodyNoFilesLimit

fea7b97

M4tteoP commented Oct 29, 2023

View reviewed changes

anuraaga approved these changes Oct 30, 2023

View reviewed changes

M4tteoP mentioned this pull request Oct 30, 2023

Support SecRequestBodyNoFilesLimit directive #896

Open

Add comment about not implemented SecRequestBodyNoFilesLimit in coraz…

9ee9745

…a.conf-recommended

M4tteoP force-pushed the fix_bodylimits_doc branch from b8c0246 to 9ee9745 Compare October 30, 2023 16:52

M4tteoP added 5 commits November 3, 2023 14:45

Aligns RequestBodyLimit and ResponseBodyLimit defaults values to doc

072eff9

Merge branch 'main' into fix_bodylimits_doc

4fe9dec

adds link to issue in TODO comment

9f975d8

Merge branch 'main' into fix_bodylimits_doc

03e751d

Merge branch 'main' into fix_bodylimits_doc

7f35990

M4tteoP requested a review from a team December 13, 2023 15:28

Merge branch 'main' into fix_bodylimits_doc

05750ad

jcchavezs reviewed Feb 5, 2024

View reviewed changes

M4tteoP added 2 commits February 5, 2024 23:38

fix comment for autogeneration

f19f581

Merge branch 'main' into fix_bodylimits_doc

64cdef0

jcchavezs merged commit 3c15624 into corazawaf:main Feb 6, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: BodyLimit related documented default values, default RequestBodyLimitAction, adds some tests #895

fix: BodyLimit related documented default values, default RequestBodyLimitAction, adds some tests #895

M4tteoP commented Oct 29, 2023 •

edited

Loading

codecov bot commented Oct 29, 2023 •

edited

Loading

M4tteoP Oct 29, 2023

M4tteoP Oct 29, 2023

jcchavezs Oct 30, 2023 •

edited

Loading

M4tteoP Oct 30, 2023

jcchavezs Nov 3, 2023

M4tteoP Nov 3, 2023

M4tteoP Oct 29, 2023

M4tteoP Oct 29, 2023

anuraaga Oct 30, 2023

jcchavezs Oct 30, 2023

M4tteoP Oct 30, 2023 •

edited

Loading

anuraaga Oct 30, 2023

M4tteoP commented Dec 13, 2023

jcchavezs Feb 5, 2024

M4tteoP Feb 5, 2024

	// if no requestBodyInMemoryLimit has been set we default to the
	var requestBodyInMemoryLimit int64 = w.RequestBodyLimit
	if w.requestBodyInMemoryLimit != nil {
	requestBodyInMemoryLimit = int64(*w.requestBodyInMemoryLimit)
	}

fix: BodyLimit related documented default values, default RequestBodyLimitAction, adds some tests #895

fix: BodyLimit related documented default values, default RequestBodyLimitAction, adds some tests #895

Conversation

M4tteoP commented Oct 29, 2023 • edited Loading

codecov bot commented Oct 29, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4tteoP Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4tteoP commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4tteoP commented Oct 29, 2023 •

edited

Loading

codecov bot commented Oct 29, 2023 •

edited

Loading

jcchavezs Oct 30, 2023 •

edited

Loading

M4tteoP Oct 30, 2023 •

edited

Loading