8000 feat: add variable to skip response rules by fzipi · Pull Request #3944 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add variable to skip response rules #3944

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 6 commits into from
Nov 26, 2024
Merged

feat: add variable to skip response rules #3944

merged 6 commits into from
Nov 26, 2024

Conversation

fzipi
Copy link
Member
@fzipi fzipi commented Nov 23, 2024

what

  • add variable to skip checking response rules
  • this makes explicit for users the choice of disabling it, instead of relying on SecResponseBodyAccess being On by default.

why

  • make configuration more verbose about the problems users might face by enabling responses

references

todo

  • add links to RFDoS documentation
  • create followup issue in the documentation repository

@fzipi fzipi requested a review from a team November 23, 2024 16:43
@fzipi fzipi force-pushed the feat/add-variable-disable-response-checking branch from 6e714ca to 06bcc6a Compare November 23, 2024 16:48
@dune73
Copy link
Member
dune73 commented Nov 23, 2024

Good thinking. I like the approach and I think this is useful.

But it is a lot of skip rules.

What about skipping to the beginning of 959 in a single skip? (Or the end of 959.)

@fzipi
Copy link
Member Author
fzipi commented Nov 23, 2024

Definitely a better approach to just skip to the end instead.

theseion
theseion requested changes Nov 24, 2024
View reviewed changes
@fzipi fzipi force-pushed the feat/add-variable-disable-response-checking branch from fee9051 to 8ac9913 Compare November 24, 2024 13:50
@fzipi
Copy link
Member Author
fzipi commented Nov 24, 2024

@M4tteoP For the quantitative tests to work (and post the comment) we should use the pull_request_target event, but it will only work from the main branch.

@fzipi fzipi force-pushed the feat/add-variable-disable-response-checking branch from 8ac9913 to a4b3bbc Compare November 24, 2024 13:57
fzipi and others added 4 commits November 24, 2024 11:00
@fzipi
feat: add variable to skip response rules
7c663b1 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi @theseion
Update crs-setup.conf.example
1fc8028 
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
@fzipi
fix: skipAfter 959 evaluation
721b553 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi
docs: update setup with reference
456a8c3 
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi fzipi force-pushed the feat/add-variable-disable-response-checking branch from a4b3bbc to 456a8c3 Compare November 24, 2024 14:00
@github-actions GitHub Actions
Copy link
Contributor

📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

@fzipi
Copy link
Member Author
fzipi commented Nov 25, 2024

Looks like working now! 🥳

@fzipi
Copy link
Member Author
fzipi commented Nov 26, 2024

@dune73 Any other comments? Shall we merge?

@dune73
Copy link
Member
dune73 commented Nov 26, 2024

This looks good. So fair with me to merge.

One word of caution, you may still want to consider. The variable is called crs_skip_response_analysis, yet you are skipping the analysis AND the blocking decision.

I have been thinking about situations where you would want to skip the former but not the latter. So far I can't see such a situation, but conceptually, there is a chance such a situation exists and you might prefer to skip to the beginning of 959.

@fzipi
Copy link
Member Author
fzipi commented Nov 26, 2024

Sure. I don't see how variables might be incremented in the case of CRS, but other rules might come into play so we can add it back.

@fzipi
Copy link
Member Author
fzipi commented Nov 26, 2024

Add jump in a4b139c.

8000

@fzipi fzipi requested a review from theseion November 26, 2024 14:01
@dune73
Copy link
Member
dune73 commented Nov 26, 2024

Very good. Thank you.

@dune73 dune73 added this pull request to the merge queue Nov 26, 2024
Merged via the queue into coreruleset:main with commit 4c6929d Nov 26, 2024
6 checks passed
@fzipi fzipi mentioned this pull request Dec 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theseion theseion Awaiting requested review from theseion

Assignees
No one assigned
Labels
release:important
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fzipi @dune73 @theseion
0