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

[Nodejs] Update iast stack trace tests #3746

Merged
merged 6 commits into from
Jan 23, 2025
+44 −16
Merged

[Nodejs] Update iast stack trace tests #3746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d47cbe
update iast stack trace tests
IlyasShabi Dec 26, 2024
a926b73
Fix mongodb stack trace test
IlyasShabi Jan 2, 2025
47906a6
fix dd-trace-js verion
IlyasShabi Jan 14, 2025
55253da
revert to master branch
IlyasShabi Jan 15, 2025
e6f903f
fix dd-trace-js verisons
IlyasShabi Jan 23, 2025
b3c0dc7
Merge branch 'main' into iast-stack-trace-nodejs
IlyasShabi Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 41 additions & 14 deletions manifests/nodejs.yml
View file
Open in desktop
IlyasShabi marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ refs:
- &ref_5_27_0 '>=5.27.0 || ^4.51.0'
- &ref_5_29_0 '>=5.29.0 || ^4.53.0' # express 5 support
- &ref_5_30_0 '>=5.30.0 || ^4.54.0'
- &ref_5_32_0 '>=5.32.0 || ^4.56.0'
- &ref_5_32_0 '>=5.32.0'
- &ref_5_33_0 '>=5.33.0'

tests/:
apm_tracing_e2e/:
Expand Down Expand Up @@ -95,12 +96,16 @@ tests/:
TestCodeInjection:
'*': *ref_5_20_0
nextjs: missing_feature
TestCodeInjection_StackTrace: missing_feature
TestCodeInjection_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_command_injection.py:
TestCommandInjection:
'*': *ref_3_11_0
nextjs: missing_feature
TestCommandInjection_StackTrace: missing_feature
TestCommandInjection_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_email_html_injection.py:
TestEmailHtmlInjection: missing_feature
TestEmailHtmlInjection_StackTrace: missing_feature
Expand Down Expand Up @@ -137,7 +142,9 @@ tests/:
'*': *ref_5_26_0
express5: *ref_5_29_0 # test uses querystring
nextjs: missing_feature
TestHeaderInjection_StackTrace: missing_feature
TestHeaderInjection_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_hsts_missing_header.py:
Test_HstsMissingHeader:
'*': *ref_4_8_0
Expand All @@ -158,7 +165,9 @@ tests/:
TestLDAPInjection:
'*': *ref_4_1_0
nextjs: missing_feature
TestLDAPInjection_StackTrace: missing_feature
TestLDAPInjection_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_no_httponly_cookie.py:
TestNoHttponlyCookie:
'*': *ref_4_3_0
Expand All @@ -179,25 +188,33 @@ tests/:
TestNoSqlMongodbInjection:
'*': *ref_4_17_0
nextjs: missing_feature
TestNoSqlMongodbInjection_StackTrace: missing_feature
TestNoSqlMongodbInjection_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_path_traversal.py:
TestPathTraversal:
'*': *ref_3_19_0
nextjs: missing_feature
TestPathTraversal_StackTrace: missing_feature
TestPathTraversal_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_reflection_injection.py:
TestReflectionInjection: missing_feature
TestReflectionInjection_StackTrace: missing_feature
test_sql_injection.py:
TestSqlInjection:
'*': *ref_3_11_0
nextjs: missing_feature
TestSqlInjection_StackTrace: missing_feature
TestSqlInjection_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_ssrf.py:
TestSSRF:
'*': *ref_4_1_0
nextjs: missing_feature
TestSSRF_StackTrace: missing_feature
TestSSRF_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_template_injection.py:
TestTemplateInjection:
'*': *ref_5_26_0
Expand All @@ -214,32 +231,42 @@ tests/:
TestUnvalidatedHeader:
'*': *ref_4_3_0
nextjs: missing_feature
TestUnvalidatedHeader_StackTrace: missing_feature
TestUnvalidatedHeader_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
TestUnvalidatedRedirect:
'*': *ref_4_3_0
nextjs: missing_feature
TestUnvalidatedRedirect_StackTrace: missing_feature
TestUnvalidatedRedirect_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_unvalidated_redirect_forward.py:
TestUnvalidatedForward: missing_feature
TestUnvalidatedForward_StackTrace: missing_feature
test_weak_cipher.py:
TestWeakCipher:
'*': *ref_3_6_0
nextjs: missing_feature
TestWeakCipher_StackTrace: missing_feature
TestWeakCipher_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_weak_hash.py:
TestDeduplication:
'*': *ref_3_11_0
nextjs: missing_feature
TestWeakHash:
'*': *ref_3_11_0
nextjs: missing_feature
TestWeakHash_StackTrace: missing_feature
TestWeakHash_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_weak_randomness.py:
TestWeakRandomness:
'*': *ref_5_1_0
nextjs: missing_feature
TestWeakRandomness_StackTrace: missing_feature
TestWeakRandomness_StackTrace:
'*': *ref_5_33_0
nextjs: missing_feature
test_xcontent_sniffing.py:
Test_XContentSniffing:
'*': *ref_4_8_0
Expand Down
1 change: 1 addition & 0 deletions tests/appsec/iast/sink/test_nosql_mongodb_injection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def test_telemetry_metric_executed_sink(self):
@rfc(
"https://docs.google.com/document/d/1ga7yCKq2htgcwgQsInYZKktV0hNlv4drY9XzSxT-o5U/edit?tab=t.0#heading=h.d0f5wzmlfhat"
)
@scenarios.integrations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @IlyasShabi , @uurien, @iunanua

For later PR/reviews => this change modifies a test. I'm quite confident that it'll be ok because the integration scenario is very close to the default scenario. But rule of thumb, it's better to not use a prefix ([nodejs]) in the title in that use case, to be 100% sure we don't break anything. Otherwise, we'll face complaint in apm-shared-testing 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cbeauchesne the nosql_mongodb_injection stack trace test was not working due to this missing scenario, which is why we introduced this line.
As you can see in the PR history, I ran tests for all languages to ensure nothing was broken, and then I switched to [Nodejs] for minor changes, like updating dd-trace-js versions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :) I didn't noticed this succesion of event 👍

AGTM, as long as no issue is introduced 😉

@features.iast_stack_trace
class TestNoSqlMongodbInjection_StackTrace:
"""Validate stack trace generation"""
Expand Down
4 changes: 2 additions & 2 deletions tests/appsec/iast/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ def validate_stack_traces(request):
locationFrame = None
for frame in stack_trace["frames"]:
# We are looking for the frame that corresponds to the location of the vulnerability, we will need to update this to cover all tracers
# currently support: Java, Python
# currently support: Java, Python, Node.js
if (
stack_trace["language"] == "java"
and (
Expand All @@ -244,7 +244,7 @@ def validate_stack_traces(request):
and location["line"] == frame["line"]
)
) or (
stack_trace["language"] == "python"
stack_trace["language"] in ("python", "nodejs")
and (frame.get("file", "").endswith(location["path"]) and location["line"] == frame["line"])
):
locationFrame = frame
Expand Down
Loading