-
-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: TIME variables support #1223
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import ( | |
"strconv" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
"github.com/corazawaf/coraza/v3/collection" | ||
"github.com/corazawaf/coraza/v3/debuglog" | ||
|
@@ -67,6 +68,23 @@ func TestTxSetters(t *testing.T) { | |
|
||
validateMacroExpansion(exp, tx, t) | ||
} | ||
|
||
func TestTxTime(t *testing.T) { | ||
tx := makeTransactionTimestamped(t) | ||
exp := map[string]string{ | ||
"%{TIME}": "15:27:34", | ||
"%{TIME_DAY}": "18", | ||
"%{TIME_EPOCH}": fmt.Sprintf("%d", tx.Timestamp/1e9), // 1731943654 in UTC, may differ in local timezone | ||
"%{TIME_HOUR}": "15", | ||
"%{TIME_MIN}": "27", | ||
"%{TIME_MON}": "11", | ||
"%{TIME_SEC}": "34", | ||
"%{TIME_WDAY}": "1", | ||
"%{TIME_YEAR}": "2024", | ||
} | ||
validateMacroExpansion(exp, tx, t) | ||
} | ||
|
||
func TestTxMultipart(t *testing.T) { | ||
tx := NewWAF().NewTransaction() | ||
body := []string{ | ||
|
@@ -1360,6 +1378,18 @@ func makeTransaction(t testing.TB) *Transaction { | |
return tx | ||
} | ||
|
||
func makeTransactionTimestamped(t testing.TB) *Transaction { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder by testing.TB There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same interface as in |
||
t.Helper() | ||
tx := NewWAF().NewTransaction() | ||
timestamp, err := time.ParseInLocation(time.DateTime, "2024-11-18 15:27:34", time.Local) | ||
if err != nil { | ||
panic(err) | ||
} | ||
tx.Timestamp = timestamp.UnixNano() | ||
tx.setTimeVariables() | ||
return tx | ||
} | ||
|
||
func makeTransactionMultipart(t *testing.T) *Transaction { | ||
if t != nil { | ||
t.Helper() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,6 +242,7 @@ func (w *WAF) newTransaction(opts Options) *Transaction { | |
tx.variables.duration.Set("0") | ||
tx.variables.highestSeverity.Set("0") | ||
tx.variables.uniqueID.Set(tx.id) | ||
tx.setTimeVariables() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't better to just remove this call and generate the value for each variable at access time instead? Kind of being more an init than set. The lazy collections idea is generic and will work. I would like to see if we add extra memory/benchmarks. Another option is to have specific time collections that receive the timestamp, and the function to apply when setting and only evaluate when getting. I guess in the end is more or less the same? |
||
|
||
tx.debugLogger.Debug().Msg("Transaction started") | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am inclined to say we can come up with a map for this instead of doing the conversion since these values are deterministic. Still worth to write a benchmark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you propose to use a
TIME_MAP
variable with keys such ashour
,day
,month
,year
, and so on? Did I get this right?If so, this approach would further widen the compatibility gap between ModSecurity and Coraza, whereas the main goal of this PR is to reduce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I wasn't clear enough. I was advocating for having a map in memory e.g.
instead of doing the
strconv.Itoa
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of benchmarking, I run BenchmarkTransactionCreation - because all the values are set in the constructor.
Before changes:
After changes:
However, the results vary widely from run to run. Am I missing something? Maybe, you could explain in more detail, what should I benchmark?