-
Notifications
You must be signed in to change notification settings - Fork 411
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
chore(iast): refactor iast request context by core.context #10988
base: main
Are you sure you want to change the base?
Conversation
…efactor_asm_request_context_to_core
…efactor_asm_request_context_to_core
…efactor_asm_request_context_to_core
…efactor_asm_request_context_to_core
…sm_request_context
…efactor_asm_request_context_to_core
ccc8a9e
to
e4cc5f4
Compare
# log_messages = [record.message for record in caplog.get_records("call")] | ||
# if not any(message.startswith("[IAST] no vulnerability quota") for message in log_messages): | ||
# pytest.fail() |
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.
# log_messages = [record.message for record in caplog.get_records("call")] | |
# if not any(message.startswith("[IAST] no vulnerability quota") for message in log_messages): | |
# pytest.fail() |
for message in log_messages: | ||
if IAST_VALID_LOG.search(message): | ||
pytest.fail(message) | ||
# TODO(avara1986: Django raises message like "no quota", its ok |
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.
Maybe create a Jira task instead?
# try_wrap_function_wrapper( | ||
# "starlette.datastructures", | ||
# "Headers.__getitem__", | ||
# functools.partial(if_iast_taint_returned_object_for, OriginType.HEADER), | ||
# ) | ||
# try_wrap_function_wrapper( | ||
# "starlette.datastructures", | ||
# "Headers.get", | ||
# functools.partial(if_iast_taint_returned_object_for, OriginType.HEADER), | ||
# ) | ||
# try_wrap_function_wrapper( | ||
# "fastapi", | ||
# "Header", | ||
# functools.partial(_patched_fastapi_function, OriginType.HEADER), | ||
# ) | ||
# _set_metric_iast_instrumented_source(OriginType.HEADER) |
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.
Is this be related to the FastAPI issue? Maybe just remove it?
# try_wrap_function_wrapper( | |
# "starlette.datastructures", | |
# "Headers.__getitem__", | |
# functools.partial(if_iast_taint_returned_object_for, OriginType.HEADER), | |
# ) | |
# try_wrap_function_wrapper( | |
# "starlette.datastructures", | |
# "Headers.get", | |
# functools.partial(if_iast_taint_returned_object_for, OriginType.HEADER), | |
# ) | |
# try_wrap_function_wrapper( | |
# "fastapi", | |
# "Header", | |
# functools.partial(_patched_fastapi_function, OriginType.HEADER), | |
# ) | |
# _set_metric_iast_instrumented_source(OriginType.HEADER) |
@@ -139,6 +142,9 @@ def is_pyobject_tainted(pyobject: Any) -> bool: | |||
|
|||
def taint_pyobject(pyobject: Any, source_name: Any, source_value: Any, source_origin=None) -> Any: | |||
# Pyobject must be Text with len > 1 |
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.
This comment is now far from the logic it explains
This PR continues the work Christophe started in PR #10899, focusing on a refactor of the IAST context. Key highlights of this refactor include:
IASTEnvironment
class has been introduced, which containsrequest_enabled
andIastSpanReporter
. These no longer depend on spans, only on the core context.on_span_start
.with core.context_with_data
, and when the WSGI request ends, it calls the listenercontext.ended.wsgi.__call__
. Previously, when calling Span Processoron_span_finish
, the context was sometimes lost. Now, it ensures that the context hasn’t already been reported to avoid this issue, as seen in cases like the Django tests, whereSpan Processor.on_span_finish
is called instead ofcontext.ended.wsgi.__call__
.thread_local struct ThreadContextCache_
has been temporarily removed to align C++ context behavior with Python’s. This is reflected in the testtest_oce_concurrent_requests_futures_in_spans
. That is a rollback of fix(iast): improve overhead control logic #8452._patched_fastapi_function
and Header patching was removed due to an error in FastAPI v0.92is_iast_request_enabled
to avoid unnecessary operations.AppSecIastSpanProcessor.is_span_analyzed
has been moved tois_iast_request_enabled
._asm_request_context.listen
has been renamed to_asm_request_context.asm_listen
to avoid confusion with otherlisten
methods._asm_request_context.in_context
has been renamed to_asm_request_context.in_asm_context
to differentiate it fromin_iast_context
.DDWaf_result
fromddtrace.appsec._ddwaf
has been moved inside theif TYPE_CHECKING
block to prevent circular imports caused by this refactor.RecursionError: maximum recursion depth exceeded in comparison
in some Python versions https://gitlab.ddbuild.io/DataDog/apm-reliability/dd-trace-py/-/jobs/668530031Tests:
test_weak_hash_new_with_child_span
has been removed because we no longer depend on spans or child spans.oce._enabled = True
has been removed.tests/tracer/test_trace_utils.py
had to be updated and was moved totaint_sinks/test_insecure_cookie.py
, so it requires validation from the @DataDog/apm-sdk-api-python team.Checklist
Reviewer Checklist