Skip to content

Commit 55b277e

Browse files
christophe-papaziantaegyunkim
authored andcommitted
chore(aap): improvements for django endpoint discovery (#14280)
- ensure no endpoint (route, method) is send twice in the same payload - add threat test suite support for endpoint discovery and check for resource compatibility between telemetry payload and span attributes - minor test improvements APPSEC-58374 ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
1 parent 938b227 commit 55b277e

File tree

5 files changed

+75
-10
lines changed

5 files changed

+75
-10
lines changed

ddtrace/contrib/internal/django/patch.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ def _gather_block_metadata(request, request_headers, ctx: core.ExecutionContext)
456456
if user_agent:
457457
metadata[http.USER_AGENT] = user_agent
458458
except Exception as e:
459-
log.warning("Could not gather some metadata on blocked request: %s", str(e)) # noqa: G200
459+
log.warning("Could not gather some metadata on blocked request: %s", str(e))
460460
core.dispatch("django.block_request_callback", (ctx, metadata, config_django, url, query))
461461

462462

ddtrace/internal/endpoints.py

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import dataclasses
22
from time import monotonic
3-
from typing import List
3+
from typing import Set
44

55

66
@dataclasses.dataclass(frozen=True)
@@ -9,11 +9,17 @@ class HttpEndPoint:
99
path: str
1010
resource_name: str = dataclasses.field(default="")
1111
operation_name: str = dataclasses.field(default="http.request")
12+
_hash: int = dataclasses.field(init=False, repr=False)
1213

1314
def __post_init__(self) -> None:
1415
super().__setattr__("method", self.method.upper())
1516
if not self.resource_name:
1617
super().__setattr__("resource_name", f"{self.method} {self.path}")
18+
# cache hash result
19+
super().__setattr__("_hash", hash((self.method, self.path)))
20+
21+
def __hash__(self) -> int:
22+
return self._hash
1723

1824

1925
@dataclasses.dataclass()
@@ -24,7 +30,7 @@ class HttpEndPointsCollection:
2430
It maintains a maximum size and drops endpoints after a certain time period in case of a hot reload of the server.
2531
"""
2632

27-
endpoints: List[HttpEndPoint] = dataclasses.field(default_factory=list, init=False)
33+
endpoints: Set[HttpEndPoint] = dataclasses.field(default_factory=set, init=False)
2834
is_first: bool = dataclasses.field(default=True, init=False)
2935
drop_time_seconds: float = dataclasses.field(default=90.0, init=False)
3036
last_modification_time: float = dataclasses.field(default_factory=monotonic, init=False)
@@ -45,12 +51,12 @@ def add_endpoint(
4551
current_time = monotonic()
4652
if current_time - self.last_modification_time > self.drop_time_seconds:
4753
self.reset()
48-
self.endpoints.append(
54+
self.endpoints.add(
4955
HttpEndPoint(method=method, path=path, resource_name=resource_name, operation_name=operation_name)
5056
)
5157
elif len(self.endpoints) < self.max_size_length:
5258
self.last_modification_time = current_time
53-
self.endpoints.append(
59+
self.endpoints.add(
5460
HttpEndPoint(method=method, path=path, resource_name=resource_name, operation_name=operation_name)
5561
)
5662

@@ -61,16 +67,16 @@ def flush(self, max_length: int) -> dict:
6167
if max_length >= len(self.endpoints):
6268
res = {
6369
"is_first": self.is_first,
64-
"endpoints": [dataclasses.asdict(ep) for ep in self.endpoints],
70+
"endpoints": list(map(dataclasses.asdict, self.endpoints)),
6571
}
6672
self.reset()
6773
return res
6874
else:
75+
batch = [self.endpoints.pop() for _ in range(max_length)]
6976
res = {
7077
"is_first": self.is_first,
71-
"endpoints": [dataclasses.asdict(ep) for ep in self.endpoints[:max_length]],
78+
"endpoints": [dataclasses.asdict(ep) for ep in batch],
7279
}
73-
self.endpoints = self.endpoints[max_length:]
7480
self.is_first = False
7581
self.last_modification_time = monotonic()
7682
return res

tests/appsec/contrib_appsec/conftest.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,20 @@ def get(name):
6464
yield get
6565

6666

67+
@pytest.fixture
68+
def find_resource(test_spans, root_span):
69+
# checking both root spans and web spans for the tag
70+
def find(resource_name):
71+
for span in test_spans.spans:
72+
if span.parent_id is None or span.span_type == "web":
73+
res = span._resource[0]
74+
if res == resource_name:
75+
return True
76+
return False
77+
78+
yield find
79+
80+
6781
@pytest.fixture
6882
def get_metric(root_span):
6983
yield lambda name: root_span().get_metric(name)

tests/appsec/contrib_appsec/django_app/urls.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,13 @@ def rasp(request, endpoint: str):
154154
if param.startswith("cmda"):
155155
cmd = query_params[param]
156156
try:
157-
res.append(f'cmd stdout: {subprocess.run([cmd, "-c", "3", "localhost"])}')
157+
res.append(f'cmd stdout: {subprocess.run([cmd, "-c", "3", "localhost"], timeout=0.5)}')
158158
except Exception as e:
159159
res.append(f"Error: {e}")
160160
elif param.startswith("cmds"):
161161
cmd = query_params[param]
162162
try:
163-
res.append(f"cmd stdout: {subprocess.run(cmd)}")
163+
res.append(f"cmd stdout: {subprocess.run(cmd, timeout=0.5)}")
164164
except Exception as e:
165165
res.append(f"Error: {e}")
166166
tracer.current_span()._local_root.set_tag("rasp.request.done", endpoint)

tests/appsec/contrib_appsec/utils.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,51 @@ def test_simple_attack_timeout(self, interface: Interface, root_span, get_metric
181181
assert len(args_list) == 1
182182
assert ("waf_timeout", "true") in args_list[0][4]
183183

184+
def test_api_endpoint_discovery(self, interface: Interface, find_resource):
185+
"""Check that API endpoint discovery works in the framework.
186+
187+
Also ensure the resource name is set correctly.
188+
"""
189+
if interface.name != "django":
190+
pytest.skip("API endpoint discovery is only supported in Django")
191+
from ddtrace.settings.asm import endpoint_collection
192+
193+
def parse(path: str) -> str:
194+
import re
195+
196+
# django substitutions to make a url path from route
197+
if re.match(r"^\^.*\$$", path):
198+
path = path[1:-1]
199+
path = re.sub(r"<int:param_int>", "123", path)
200+
path = re.sub(r"<str:param_str>", "abc", path)
201+
if path.endswith("/?"):
202+
path = path[:-2]
203+
return "/" + path
204+
205+
with override_global_config(dict(_asm_enabled=True)):
206+
self.update_tracer(interface)
207+
# required to load the endpoints
208+
interface.client.get("/")
209+
collection = endpoint_collection.endpoints
210+
assert collection
211+
for ep in collection:
212+
assert ep.method
213+
# path could be empty, but must be a string
214+
assert isinstance(ep.path, str)
215+
assert ep.resource_name
216+
assert ep.operation_name
217+
if ep.method not in ("GET", "*", "POST"):
218+
continue
219+
path = parse(ep.path)
220+
response = (
221+
interface.client.post(path, {"data": "content"})
222+
if ep.method == "POST"
223+
else interface.client.get(path)
224+
)
225+
assert self.status(response) in (200, 401), f"ep.path failed: {ep.path} -> {path}"
226+
resource = "GET" + ep.resource_name[1:] if ep.resource_name.startswith("* ") else ep.resource_name
227+
assert find_resource(resource)
228+
184229
@pytest.mark.parametrize("asm_enabled", [True, False])
185230
@pytest.mark.parametrize(
186231
("user_agent", "priority"),

0 commit comments

Comments
 (0)