Skip to content

Commit fddc8ba

Browse files
fzipiCopilotM4tteoPCopilot
authored
feat: implement SecUploadKeepFiles directive (#1557)
* feat: implement SecUploadKeepFiles with RelevantOnly support Add UploadKeepFilesStatus type supporting On, Off, and RelevantOnly values for the SecUploadKeepFiles directive. When set to On, uploaded files are preserved after transaction close. When set to RelevantOnly, files are kept only if rules matched during the transaction. Closes #1550 * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @M4tteoP Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * docs: update SecUploadKeepFiles in coraza.conf-recommended Remove the "not supported" note and document the RelevantOnly option. * fix: filter nolog rules in RelevantOnly upload keep files check RelevantOnly now only considers rules with Log enabled, matching the same filtering used for audit log part K. This prevents CRS initialization rules (nolog) from making RelevantOnly behave like On. * fix: require SecUploadDir when SecUploadKeepFiles is enabled Add validation in WAF.Validate() to ensure SecUploadDir is configured when SecUploadKeepFiles is set to On or RelevantOnly, matching the ModSecurity requirement. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: directive docs Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * fix: correct two compile errors in SecUploadKeepFiles implementation (#1560) * Initial plan * fix: correct lint errors - HasAccessToFS is a bool not a function, fix wrong constant name Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * fix: gofmt Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: skip SecUploadKeepFiles tests when no_fs_access build tag is set The upload keep files tests expected success for On/RelevantOnly modes, but the implementation correctly rejects these when filesystem access is disabled. Guard these test cases behind environment.HasAccessToFS. --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
1 parent 607fd73 commit fddc8ba

File tree

8 files changed

+255
-17
lines changed

8 files changed

+255
-17
lines changed

coraza.conf-recommended

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,11 @@ SecDataDir /tmp/
121121
#
122122
#SecUploadDir /opt/coraza/var/upload/
123123

124-
# If On, the WAF will store the uploaded files in the SecUploadDir
125-
# directory.
126-
# Note: SecUploadKeepFiles is currently NOT supported by Coraza
124+
# Controls whether intercepted uploaded files will be kept after
125+
# transaction is processed. Possible values: On, Off, RelevantOnly.
126+
# RelevantOnly will keep files only when a matching rule is logged (rules with 'nolog' do not qualify).
127127
#
128-
#SecUploadKeepFiles Off
128+
#SecUploadKeepFiles RelevantOnly
129129

130130
# Uploaded files are by default created with permissions that do not allow
131131
# any other user to access them. You may need to relax that if you want to

internal/corazawaf/transaction.go

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1455,6 +1455,18 @@ func (tx *Transaction) MatchedRules() []types.MatchedRule {
14551455
return tx.matchedRules
14561456
}
14571457

1458+
// hasLogRelevantMatchedRules returns true if any matched rule has Log enabled.
1459+
// Rules with nolog (e.g. CRS initialization rules) are excluded, matching
1460+
// the same filtering used for audit log part K.
1461+
func (tx *Transaction) hasLogRelevantMatchedRules() bool {
1462+
for _, mr := range tx.matchedRules {
1463+
if mrWithLog, ok := mr.(*corazarules.MatchedRule); ok && mrWithLog.Log() {
1464+
return true
1465+
}
1466+
}
1467+
return false
1468+
}
1469+
14581470
func (tx *Transaction) LastPhase() types.RulePhase {
14591471
return tx.lastPhase
14601472
}
@@ -1628,12 +1640,20 @@ func (tx *Transaction) Close() error {
16281640

16291641
var errs []error
16301642
if environment.HasAccessToFS {
1631-
// TODO(jcchavezs): filesTmpNames should probably be a new kind of collection that
1632-
// is aware of the files and then attempt to delete them when the collection
1633-
// is resetted or an item is removed.
1634-
for _, file := range tx.variables.filesTmpNames.Get("") {
1635-
if err := os.Remove(file); err != nil {
1636-
errs = append(errs, fmt.Errorf("removing temporary file: %v", err))
1643+
// UploadKeepFilesRelevantOnly keeps temporary files only when there are
1644+
// log-relevant matched rules (i.e., rules that would be logged; rules
1645+
// with actions such as "nolog" are intentionally excluded here).
1646+
keepFiles := tx.WAF.UploadKeepFiles == types.UploadKeepFilesOn ||
1647+
(tx.WAF.UploadKeepFiles == types.UploadKeepFilesRelevantOnly && tx.hasLogRelevantMatchedRules())
1648+
1649+
if !keepFiles {
1650+
// TODO(jcchavezs): filesTmpNames should probably be a new kind of collection that
1651+
// is aware of the files and then attempt to delete them when the collection
1652+
// is resetted or an item is removed.
1653+
for _, file := range tx.variables.filesTmpNames.Get("") {
1654+
if err := os.Remove(file); err != nil {
1655+
errs = append(errs, fmt.Errorf("removing temporary file: %v", err))
1656+
}
16371657
}
16381658
}
16391659
}

internal/corazawaf/transaction_test.go

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"fmt"
99
"io"
1010
"net/http"
11+
"os"
1112
"regexp"
1213
"runtime/debug"
1314
"strconv"
@@ -1857,6 +1858,121 @@ func TestCloseFails(t *testing.T) {
18571858
}
18581859
}
18591860

1861+
func TestUploadKeepFiles(t *testing.T) {
1862+
if !environment.HasAccessToFS {
1863+
t.Skip("skipping test as it requires access to filesystem")
1864+
}
1865+
1866+
createTmpFile := func(t *testing.T) string {
1867+
t.Helper()
1868+
f, err := os.CreateTemp(t.TempDir(), "crztest*")
1869+
if err != nil {
1870+
t.Fatal(err)
1871+
}
1872+
name := f.Name()
1873+
if err := f.Close(); err != nil {
1874+
t.Fatalf("failed to close temp file: %v", err)
1875+
}
1876+
return name
1877+
}
1878+
1879+
t.Run("Off deletes files", func(t *testing.T) {
1880+
waf := NewWAF()
1881+
waf.UploadKeepFiles = types.UploadKeepFilesOff
1882+
tx := waf.NewTransaction()
1883+
tmpFile := createTmpFile(t)
1884+
1885+
col := tx.Variables().FilesTmpNames().(*collections.Map)
1886+
col.Add("", tmpFile)
1887+
1888+
if err := tx.Close(); err != nil {
1889+
t.Fatal(err)
1890+
}
1891+
1892+
if _, err := os.Stat(tmpFile); !os.IsNotExist(err) {
1893+
t.Fatal("expected temp file to be deleted when UploadKeepFiles is Off")
1894+
}
1895+
})
1896+
1897+
t.Run("On keeps files", func(t *testing.T) {
1898+
waf := NewWAF()
1899+
waf.UploadKeepFiles = types.UploadKeepFilesOn
1900+
tx := waf.NewTransaction()
1901+
tmpFile := createTmpFile(t)
1902+
1903+
col := tx.Variables().FilesTmpNames().(*collections.Map)
1904+
col.Add("", tmpFile)
1905+
1906+
if err := tx.Close(); err != nil {
1907+
t.Fatal(err)
1908+
}
1909+
1910+
if _, err := os.Stat(tmpFile); err != nil {
1911+
t.Fatal("expected temp file to be kept when UploadKeepFiles is On")
1912+
}
1913+
})
1914+
1915+
t.Run("RelevantOnly keeps files when log rules matched", func(t *testing.T) {
1916+
waf := NewWAF()
1917+
waf.UploadKeepFiles = types.UploadKeepFilesRelevantOnly
1918+
tx := waf.NewTransaction()
1919+
tmpFile := createTmpFile(t)
1920+
1921+
// Simulate a matched rule with Log enabled
1922+
tx.matchedRules = append(tx.matchedRules, &corazarules.MatchedRule{Log_: true})
1923+
1924+
col := tx.Variables().FilesTmpNames().(*collections.Map)
1925+
col.Add("", tmpFile)
1926+
1927+
if err := tx.Close(); err != nil {
1928+
t.Fatal(err)
1929+
}
1930+
1931+
if _, err := os.Stat(tmpFile); err != nil {
1932+
t.Fatal("expected temp file to be kept when UploadKeepFiles is RelevantOnly and log rules matched")
1933+
}
1934+
})
1935+
1936+
t.Run("RelevantOnly deletes files when only nolog rules matched", func(t *testing.T) {
1937+
waf := NewWAF()
1938+
waf.UploadKeepFiles = types.UploadKeepFilesRelevantOnly
1939+
tx := waf.NewTransaction()
1940+
tmpFile := createTmpFile(t)
1941+
1942+
// Simulate a matched rule with Log disabled (e.g. CRS initialization rules)
1943+
tx.matchedRules = append(tx.matchedRules, &corazarules.MatchedRule{Log_: false})
1944+
1945+
col := tx.Variables().FilesTmpNames().(*collections.Map)
1946+
col.Add("", tmpFile)
1947+
1948+
if err := tx.Close(); err != nil {
1949+
t.Fatal(err)
1950+
}
1951+
1952+
if _, err := os.Stat(tmpFile); !os.IsNotExist(err) {
1953+
t.Fatal("expected temp file to be deleted when UploadKeepFiles is RelevantOnly and only nolog rules matched")
1954+
}
1955+
})
1956+
1957+
t.Run("RelevantOnly deletes files when no rules matched", func(t *testing.T) {
1958+
waf := NewWAF()
1959+
waf.UploadKeepFiles = types.UploadKeepFilesRelevantOnly
1960+
tx := waf.NewTransaction()
1961+
tmpFile := createTmpFile(t)
1962+
1963+
col := tx.Variables().FilesTmpNames().(*collections.Map)
1964+
col.Add("", tmpFile)
1965+
1966+
if err := tx.Close(); err != nil {
1967+
t.Fatal(err)
1968+
}
1969+
1970+
if _, err := os.Stat(tmpFile); !os.IsNotExist(err) {
1971+
t.Fatal("expected temp file to be deleted when UploadKeepFiles is RelevantOnly and no rules matched")
1972+
}
1973+
})
1974+
}
1975+
18601976
func TestRequestFilename(t *testing.T) {
18611977
tests := []struct {
18621978
name string

internal/corazawaf/waf.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,9 @@ type WAF struct {
9494
// Path to store data files (ex. cache)
9595
DataDir string
9696

97-
// If true, the WAF will store the uploaded files in the UploadDir
98-
// directory
99-
UploadKeepFiles bool
97+
// UploadKeepFiles controls whether uploaded files are kept after the transaction.
98+
// On: always keep, Off: always delete (default), RelevantOnly: keep only if log-relevant rules matched (excluding nolog rules).
99+
UploadKeepFiles types.UploadKeepFilesStatus
100100
// UploadFileMode instructs the waf to set the file mode for uploaded files
101101
UploadFileMode fs.FileMode
102102
// UploadFileLimit is the maximum size of the uploaded file to be stored
@@ -446,6 +446,16 @@ func (w *WAF) Validate() error {
446446
return errors.New("request body json depth limit should be bigger than 0")
447447
}
448448

449+
if environment.HasAccessToFS {
450+
if w.UploadKeepFiles != types.UploadKeepFilesOff && w.UploadDir == "" {
451+
return errors.New("SecUploadDir is required when SecUploadKeepFiles is enabled")
452+
}
453+
} else {
454+
if w.UploadKeepFiles != types.UploadKeepFilesOff {
455+
return errors.New("SecUploadKeepFiles requires filesystem access, which is not available in this build")
456+
}
457+
}
458+
449459
return nil
450460
}
451461

internal/corazawaf/waf_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ import (
77
"io"
88
"os"
99
"testing"
10+
11+
"github.com/corazawaf/coraza/v3/internal/environment"
12+
"github.com/corazawaf/coraza/v3/types"
1013
)
1114

1215
func TestNewTransaction(t *testing.T) {
@@ -107,6 +110,39 @@ func TestValidate(t *testing.T) {
107110
},
108111
}
109112

113+
if environment.HasAccessToFS {
114+
testCases["upload keep files on without upload dir"] = struct {
115+
customizer func(*WAF)
116+
expectErr bool
117+
}{
118+
expectErr: true,
119+
customizer: func(w *WAF) {
120+
w.UploadKeepFiles = types.UploadKeepFilesOn
121+
w.UploadDir = ""
122+
},
123+
}
124+
testCases["upload keep files relevant only without upload dir"] = struct {
125+
customizer func(*WAF)
126+
expectErr bool
127+
}{
128+
expectErr: true,
129+
customizer: func(w *WAF) {
130+
w.UploadKeepFiles = types.UploadKeepFilesRelevantOnly
131+
w.UploadDir = ""
132+
},
133+
}
134+
testCases["upload keep files on with upload dir"] = struct {
135+
customizer func(*WAF)
136+
expectErr bool
137+
}{
138+
expectErr: false,
139+
customizer: func(w *WAF) {
140+
w.UploadKeepFiles = types.UploadKeepFilesOn
141+
w.UploadDir = "/tmp"
142+
},
143+
}
144+
}
145+
110146
for name, tCase := range testCases {
111147
t.Run(name, func(t *testing.T) {
112148
waf := NewWAF()

internal/seclang/directives.go

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -935,12 +935,34 @@ func directiveSecDataDir(options *DirectiveOptions) error {
935935
return nil
936936
}
937937

938+
// Description: Configures whether intercepted files will be kept after the transaction is processed.
939+
// Syntax: SecUploadKeepFiles On|RelevantOnly|Off
940+
// Default: Off
941+
// ---
942+
// The `SecUploadKeepFiles` directive is used to configure whether intercepted files are
943+
// preserved on disk after the transaction is processed.
944+
// This directive requires the storage directory to be defined (using `SecUploadDir`).
945+
//
946+
// Possible values are:
947+
// - On: Keep all uploaded files.
948+
// - Off: Do not keep uploaded files.
949+
// - RelevantOnly: Keep only uploaded files that matched at least one rule that would be
950+
// logged (excluding rules with the `nolog` action).
938951
func directiveSecUploadKeepFiles(options *DirectiveOptions) error {
939-
b, err := parseBoolean(options.Opts)
952+
if len(options.Opts) == 0 {
953+
return errEmptyOptions
954+
}
955+
956+
status, err := types.ParseUploadKeepFilesStatus(options.Opts)
940957
if err != nil {
941958
return err
942959
}
943-
options.WAF.UploadKeepFiles = b
960+
961+
if !environment.HasAccessToFS && status != types.UploadKeepFilesOff {
962+
return fmt.Errorf("SecUploadKeepFiles: cannot enable keeping uploaded files: filesystem access is disabled")
963+
}
964+
965+
options.WAF.UploadKeepFiles = status
944966
return nil
945967
}
946968

@@ -967,6 +989,11 @@ func directiveSecUploadFileLimit(options *DirectiveOptions) error {
967989
return err
968990
}
969991

992+
// Description: Configures the directory where uploaded files will be stored.
993+
// Syntax: SecUploadDir /path/to/dir
994+
// Default: ""
995+
// ---
996+
// This directive is required when enabling SecUploadKeepFiles.
970997
func directiveSecUploadDir(options *DirectiveOptions) error {
971998
if len(options.Opts) == 0 {
972999
return errEmptyOptions

internal/seclang/directives_test.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,7 @@ func TestDirectives(t *testing.T) {
154154
"SecUploadKeepFiles": {
155155
{"", expectErrorOnDirective},
156156
{"Ox", expectErrorOnDirective},
157-
{"On", func(w *corazawaf.WAF) bool { return w.UploadKeepFiles }},
158-
{"Off", func(w *corazawaf.WAF) bool { return !w.UploadKeepFiles }},
157+
{"Off", func(w *corazawaf.WAF) bool { return w.UploadKeepFiles == types.UploadKeepFilesOff }},
159158
},
160159
"SecUploadFileMode": {
161160
{"", expectErrorOnDirective},
@@ -317,6 +316,10 @@ func TestDirectives(t *testing.T) {
317316
{"/tmp-non-existing", expectErrorOnDirective},
318317
{os.TempDir(), func(w *corazawaf.WAF) bool { return w.UploadDir == os.TempDir() }},
319318
}
319+
directiveCases["SecUploadKeepFiles"] = append(directiveCases["SecUploadKeepFiles"],
320+
directiveCase{"On", func(w *corazawaf.WAF) bool { return w.UploadKeepFiles == types.UploadKeepFilesOn }},
321+
directiveCase{"RelevantOnly", func(w *corazawaf.WAF) bool { return w.UploadKeepFiles == types.UploadKeepFilesRelevantOnly }},
322+
)
320323
}
321324

322325
for name, dCases := range directiveCases {

types/waf.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,32 @@ func (re RuleEngineStatus) String() string {
7878
return "unknown"
7979
}
8080

81+
// UploadKeepFilesStatus represents the status of the upload keep files directive.
82+
type UploadKeepFilesStatus int
83+
84+
const (
85+
// UploadKeepFilesOff will delete all uploaded files after transaction (default)
86+
UploadKeepFilesOff UploadKeepFilesStatus = iota
87+
// UploadKeepFilesOn will keep all uploaded files after transaction
88+
UploadKeepFilesOn
89+
// UploadKeepFilesRelevantOnly will keep uploaded files only if a log-relevant rule matched
90+
// (that is, a matched rule with logging enabled, excluding rules marked with nolog).
91+
UploadKeepFilesRelevantOnly
92+
)
93+
94+
// ParseUploadKeepFilesStatus parses the upload keep files status
95+
func ParseUploadKeepFilesStatus(s string) (UploadKeepFilesStatus, error) {
96+
switch strings.ToLower(s) {
97+
case "on":
98+
return UploadKeepFilesOn, nil
99+
case "off":
100+
return UploadKeepFilesOff, nil
101+
case "relevantonly":
102+
return UploadKeepFilesRelevantOnly, nil
103+
}
104+
return -1, fmt.Errorf("invalid upload keep files status: %q", s)
105+
}
106+
81107
// BodyLimitAction represents the action to take when
82108
// the body size exceeds the configured limit.
83109
type BodyLimitAction int

0 commit comments

Comments
 (0)