Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix/feat: Macro expansions, error logs redundancy, support
msg
/logdata
in inner rules #792fix/feat: Macro expansions, error logs redundancy, support
msg
/logdata
in inner rules #792Changes from 2 commits
9c5914f
d072361
574b828
2a1d472
7ffbfe0
c105d11
61043a1
57166bf
ad38730
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 133 in internal/corazarules/rule_match.go
Codecov / codecov/patch
internal/corazarules/rule_match.go#L132-L133
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.
If you want to use printf with strings.Builder, I think you can still avoid the extra allocation with something like
fmt.Fprintf(log, "[msg_match_%d %q] [data_match_%d %q]", n, msg, n, data)
Applies throughout the PR
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.
Also it's a bit surprising to be printing the same value
n
twiceCheck warning on line 140 in internal/corazarules/rule_match.go
Codecov / codecov/patch
internal/corazarules/rule_match.go#L140
Check warning on line 174 in internal/corazarules/rule_match.go
Codecov / codecov/patch
internal/corazarules/rule_match.go#L174
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.
nit, swaps parameters order just to keep consistency across
mr.matchData()
andmr.writeDetails()
Check warning on line 200 in internal/corazarules/rule_match.go
Codecov / codecov/patch
internal/corazarules/rule_match.go#L200
Check warning on line 201 in internal/corazawaf/rule.go
Codecov / codecov/patch
internal/corazawaf/rule.go#L200-L201
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.
<3
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 don't fully get this test. The title (
TestTagsAreNotPrintedTwice
) seems like that wants to avoid the same tag appearing more than once inside the logs, but then we are trying to find the same tag twice. Originally it was enforcing being just one, but it has been changed with https://github.com/corazawaf/coraza/pull/220/files#diff-090979aa58246451224457e41b61bf533048a2e7091724b5db0345c04f5ef561L248-R249.Currently, the test is trying to match the following:
And it seems that the ending part is very lengthy and not relevant (Current PR tries to avoid redundancy, therefore it does not print it):
cc. @piyushroshan
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.
Ping @jptosso
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.
For a very unfortunate combination of rules triggered,
932200-13
test is now failing. It is intended to check that the correct log has been printed, looking for the wrong onefound within MATCHED_VAR
. The latter string is matched not inside the932200
log, but inside a942440
that is triggered alongside it. The latter match is legit, but it results to be a bit more verbose compared to ModsecMATCHED_VARS:ARGS:host
vsARGS:host
, therefore the test is marked as failing. A tested workaround would be just looking forfound within MATCHED_VAR:
instead offound within MATCHED_VAR
. I will evolve this discussion CRS side and update the exclusion comment with the related issue/pr.