8000 fix: adding more missing tags and ver actions by azurit · Pull Request #3593 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: adding more missing tags and ver actions #3593

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 2, 2024
Merged

fix: adding more missing tags and ver actions #3593

merged 2 commits into from
Mar 2, 2024

Conversation

azurit
Copy link
Member
@azurit azurit commented Mar 1, 2024
  • Adding missing tag OWASP_CRS to everywhere.
  • Adding missing ver action to everywhere.
  • Reordering tags in some rules to unify tag order.

@azurit azurit requested a review from dune73 March 1, 2024 18:00
@dune73
Copy link
Member
dune73 commented Mar 1, 2024

Thank you for the PR.

This looks good, but I did not check the completeness nor the details of the change.

@theseion
Copy link
Contributor
theseion commented Mar 2, 2024

What's the rationale for reordering? I can't find any pattern that makes sense to me, it's not alphabetical, at least.

@azurit
Copy link
Member Author
azurit commented Mar 2, 2024 •
edited
Loading 8000

What's the rationale for reordering? I can't find any pattern that makes sense to me, it's not alphabetical, at least.

@theseion I reordered only paranoia-level/* and PCI/* tags to match the order which most rules are using (now all):

tag:'paranoia-level/*',\
tag:'OWASP_CRS',\
tag:'capec/*',\
tag:'PCI/*',\

@theseion
Copy link
Contributor
theseion commented Mar 2, 2024

Makes sense. Maybe we should follow up with a PR for canonical tag ordering (e.g., alphabetical).

@azurit
Copy link
Member Author
azurit commented Mar 2, 2024

Not sure if alphabetical ordering is the right one for tags but we definitely should give some guidence for this.

@dune73
Copy link
Member
dune73 commented Mar 2, 2024

I'm with @azurit here.

@airween
Copy link
Contributor
airween commented Mar 2, 2024

@theseion I reordered only paranoia-level/* and PCI/* tags to match the order which most rules are using (now all):

do we want to check the correct order in CI pipeline with our crs-rule-check? (If yes, we need to define the rule which describes the order.)

For the PR: why do we want to add OWASP-CRS tag to every rule? I mean to admin rules too...

@theseion
Copy link
Contributor
theseion commented Mar 2, 2024

do we want to check the correct order in CI pipeline with our crs-rule-check? (If yes, we need to define the rule which describes the order.)

Yes, I would. But first we need to agree on the ordering mechanism.

For the PR: why do we want to add OWASP-CRS tag to every rule? I mean to admin rules too...

That was part of the discussion in the last chat. If you disable by tag you should assume that you disable everything. The "admin" rules don't make sense without the rest IMO and it's actually strange that they would remain enabled when I've disabled "all CRS rules".

@azurit
Copy link
Member Author
azurit commented Mar 2, 2024

Is this suitable for merging?

@theseion
Copy link
Contributor
theseion commented Mar 2, 2024

I think so, yes.

@azurit azurit merged commit 2804976 into coreruleset:main Mar 2, 2024
@github-actions github-actions bot mentioned this pull request Mar 3, 2024
Merged
@dune73 dune73 mentioned this pull request Mar 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dune73 dune73 dune73 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@azurit @dune73 @theseion @airween
0