-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add Suspicious Request Blocking test without path_params #3305
base: main
Are you sure you want to change the base?
Add Suspicious Request Blocking test without path_params #3305
Conversation
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.
./format.sh
is your friend
Sorry, fixed in 0ebb371 |
"""Test that blocked requests are blocked before being processed""" | ||
assert self.block_req2.status_code == 403 | ||
interfaces.library.assert_waf_attack(self.block_req2, rule="tst-037-013") | ||
interfaces.library.validate_spans(self.block_req2, _assert_custom_event_tag_absence()) |
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.
"""Test that blocked requests are blocked before being processed""" | |
assert self.block_req2.status_code == 403 | |
interfaces.library.assert_waf_attack(self.block_req2, rule="tst-037-013") | |
interfaces.library.validate_spans(self.block_req2, _assert_custom_event_tag_absence()) | |
"""Test that blocked requests are blocked before being processed""" | |
assert self.block_req2.status_code == 403 | |
interfaces.library.assert_waf_attack(self.block_req2, rule="tst-037-013") | |
interfaces.library.validate_spans(self.block_req2, _assert_custom_event_tag_absence()) |
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.
Fixed in 2bfd6af
tests/appsec/blocking_rule.json
Outdated
"address": "server.request.uri.raw" | ||
} | ||
], | ||
"regex": "wX1GdUiWdVdoklf0pYBi5kQApO9i77tN" |
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.
To be honest, I think cryptic values make it hard to read the tests itself. Instead I would like to see something, which will immediately give me a sign in the tests, that I see a value, which suppose to trigger something.
Ideas:
- malicious-uri
- security-violation-uri
- is-attack-uri
- ...
I know that naming is hard, but you will see how tests change after that
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.
The idea behind magic strings used by @christophe-papazian was to be sure that these would never accidentally be used by other rules or other tests. When a request matches two different rules, the behaviour is undefined, which is why I choose different magic strings, and which is why it must be different than in the test with path_params. But if you think it should be clearer, we could do something like "malicious-uri-wX1GdUiWdVdoklf0pYBi5kQApO9i77tN" so it would give more info while still being unique. But then we should also change it in the original test too
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.
Alright, then we can add the "salt" to the good-meaning value, like
malicious-uri-wX1GdUiWdVdoklf0pYBi5kQApO9i77tN
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.
👏🏼 Well done. Let's goooo!
Motivation
Rack does not send anything to the path_params address (Rails and Sinatra does) but we still want to test blocking on multiple request addresses
Changes
On the advice of @christophe-papazian I wrote a similar test without path-params. All the weblogs that passes these tests with path_params should also pass them without, but it will also enable testing that feature on a pure Rack app
Reviewer checklist
[<language>]
, double-check that only<language>
is impacted by the changebuild-XXX-image
label is present