Skip to content
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

Add basic blocking tests #510

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Add basic blocking tests #510

merged 3 commits into from
Dec 19, 2022

Conversation

cataphract
Copy link
Contributor

No description provided.

@cataphract cataphract requested a review from a team as a code owner September 22, 2022 14:15
@cbeauchesne
Copy link
Collaborator

Changing it as a draft, as we are spammed by PR on slack. Please set it back to nroaml once it's ready

@cbeauchesne cbeauchesne marked this pull request as draft September 22, 2022 16:14
@cbeauchesne
Copy link
Collaborator

I've updated your branch, there were an issue fixed by #519

@cbeauchesne
Copy link
Collaborator

There is now an IP_BLOCKIG_SCENARIO, can I close this PR @cataphract ?

@cataphract
Copy link
Contributor Author

There is now an IP_BLOCKIG_SCENARIO, can I close this PR @cataphract ?

This one mainly tests that the response respects the Accept headers, so they're mostly orthogonal.

@cbeauchesne
Copy link
Collaborator

ok, so what can I do to help moving forward on this one ?

@cataphract cataphract force-pushed the glopes/blocking branch 2 times, most recently from 62adc71 to 6de9f33 Compare November 4, 2022 12:30
@cataphract cataphract marked this pull request as ready for review November 4, 2022 12:30
@cataphract cataphract force-pushed the glopes/blocking branch 4 times, most recently from b015f2c to 9980198 Compare November 7, 2022 16:28
cbeauchesne
cbeauchesne requested changes Nov 8, 2022
View reviewed changes
scenarios/appsec/waf/test_blocking.py Outdated
def test_blocking_appsec_blocked_tag(self):
"""Tag ddappsec.blocked is set when blocking"""
r = self.weblog_get("/waf/", headers={"User-Agent": "Arachni/v1", "Accept": "*/*"})
assert r.status_code == 403
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert r.status_code == 403
interfaces.library.add_assertion(r.status_code == 403)

And same for all others synchronous assertions

@cbeauchesne cbeauchesne marked this pull request as draft November 8, 2022 13:32
@cbeauchesne
Copy link
Collaborator

checking the failure

@cbeauchesne
Copy link
Collaborator

I've added a small fix that prevent synchronous failures

@cbeauchesne
Copy link
Collaborator

Seems to be good, just need to skiping open liberty

@cbeauchesne
Copy link
Collaborator

@cataphract does this PR still relevant ?

@cataphract cataphract mentioned this pull request Dec 14, 2022
Merged
@cataphract cataphract force-pushed the glopes/blocking branch 2 times, most recently from 0221f88 to a202c08 Compare December 15, 2022 15:57
@cataphract cataphract marked this pull request as ready for review December 15, 2022 19:25
@cataphract
Copy link
Contributor Author

cataphract commented Dec 15, 2022 •
edited
Loading

@cataphract does this PR still relevant ?

@cbeauchesne Sure. I've updated it. Can you approve and merge?

cbeauchesne
cbeauchesne requested changes Dec 16, 2022
View reviewed changes
run.sh Outdated Show resolved Hide resolved
run.sh Outdated Show resolved Hide resolved
tests/appsec/waf/test_blocking.py Outdated
@bug(context.library < "java@0.115.0" and context.weblog_variant == "spring-boot-undertow", reason="npe")
def test_no_accept(self):
"""Blocking without an accept header"""
r = weblog.get("/waf/", headers={"User-Agent": "Arachni/v1"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a method setup_no_accept and send the request in it (save it as self.r_not_accept) ?

tests/appsec/waf/test_blocking.py Outdated

def test_accept_all(self):
"""Blocking with Accept: */*"""
r = weblog.get("/waf/", headers={"User-Agent": "Arachni/v1", "Accept": "*/*"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment, add a metohd setup_accept_all and send request inside it.

tests/appsec/waf/test_blocking.py Outdated
def test_accept_partial_json(self):
"""Blocking with Accept: application/*"""
# */* should be ignored because there are more specific matches for text/html and application/json
r = weblog.get(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idem

tests/appsec/waf/test_blocking.py Outdated

def test_accept_partial_html(self):
"""Blocking with Accept: text/*"""
r = weblog.get(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

tests/appsec/waf/test_blocking.py Outdated

def test_accept_full_json(self):
"""Blocking with Accept: application/json"""
r = weblog.get(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

tests/appsec/waf/test_blocking.py Outdated

def test_accept_full_html(self):
"""Blocking with Accept: text/html"""
r = weblog.get(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

tests/test_standard_tags.py Outdated Show resolved Hide resolved
@cbeauchesne
Copy link
Collaborator

I've converted the PR as a draft (we ares spammed on slack by PR to be reviwed. Please set it back to ready once comment are fixed :)

@cbeauchesne cbeauchesne marked this pull request as draft December 16, 2022 10:43
@cataphract cataphract force-pushed the glopes/blocking branch 2 times, most recently from ad23db8 to 6222ed0 Compare December 16, 2022 13:54
cbeauchesne
cbeauchesne requested changes Dec 16, 2022
View reviewed changes
tests/test_standard_tags.py Outdated
@@ -38,7 +38,6 @@ def test_method_trace(self):
@released(dotnet="2.13.0", golang="1.40.0", java="0.107.1", nodejs="3.0.0")
@released(php="0.76.0", python="1.6.0rc1.dev", ruby="?")
@rfc("https://datadoghq.atlassian.net/wiki/spaces/APS/pages/2490990623/QueryString+-+Sensitive+Data+Obfuscation")
@bug(weblog_variant="spring-boot-undertow", reason="APMJAVA-877")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MMh, now it's removed? I may have added a dummy comment, or is it some merge issue ?

utils/scripts/generate-github-action-file.py Outdated
@@ -216,6 +217,8 @@ def add_main_job(i, workflow, needs, scenarios):

if scenario == "TRACE_PROPAGATION_STYLE_W3C": # TODO: fix weblog to allow this value for old tracer
step["if"] = "${{ matrix.variant.library != 'python' }}" # TODO
elif scenario == "APPSEC_BLOCKING":
step["if"] = "${{ matrix.library == 'java' }}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? if the test if flagged as fail, it's ok to run it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can run it, but we're gonna waste CPU because it won't pass outside java

@cataphract
Copy link
Contributor Author

@CharlesMasson this should be ready now. Failures seem are unrelated.

@cbeauchesne cbeauchesne marked this pull request as ready for review December 19, 2022 16:59
@cbeauchesne cbeauchesne merged commit 95bfe15 into main Dec 19, 2022
@cbeauchesne cbeauchesne deleted the glopes/blocking branch December 19, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbeauchesne cbeauchesne cbeauchesne 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.
2 participants
@cataphract @cbeauchesne