Skip to content

Commit c90d7fe

Browse files
fix(aap): track user default arguments must not generate security event [backport 3.9] (#13743)
Backport 0940d73 from #13742 to 3.9. Ensure track_user to not generate additional security events when used with expected metadata (name, email, scope or role). Also: - update (and fix) related unit tests APPSEC-58040 ## 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) Co-authored-by: Christophe Papazian <[email protected]>
1 parent 593d5d5 commit c90d7fe

File tree

3 files changed

+32
-27
lines changed

3 files changed

+32
-27
lines changed

ddtrace/appsec/track_user_sdk.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,10 @@ def track_user(
7474
if login:
7575
span.set_tag_str(_constants.APPSEC.USER_LOGIN_USERNAME, str(login))
7676
meta = metadata or {}
77-
usr_name = meta.get("name") or meta.get("usr.name")
78-
usr_email = meta.get("email") or meta.get("usr.email")
79-
usr_scope = meta.get("scope") or meta.get("usr.scope")
80-
usr_role = meta.get("role") or meta.get("usr.role")
77+
usr_name = meta.pop("name", None) or meta.pop("usr.name", None)
78+
usr_email = meta.pop("email", None) or meta.pop("usr.email", None)
79+
usr_scope = meta.pop("scope", None) or meta.pop("usr.scope", None)
80+
usr_role = meta.pop("role", None) or meta.pop("usr.role", None)
8181
_trace_utils.set_user(
8282
None,
8383
user_id,
@@ -88,8 +88,8 @@ def track_user(
8888
session_id=session_id,
8989
may_block=False,
9090
)
91-
if metadata:
92-
_trace_utils.track_custom_event(None, "auth_sdk", metadata=metadata)
91+
if meta:
92+
_trace_utils.track_custom_event(None, "auth_sdk", metadata=meta)
9393
span.set_tag_str(_constants.APPSEC.AUTO_LOGIN_EVENTS_COLLECTION_MODE, _constants.LOGIN_EVENTS_MODE.SDK)
9494
if _asm_request_context.in_asm_context():
9595
custom_data = {
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
AAP: This fix resolves an issue where track_user was generating additional unexpected security activity for customers.

tests/appsec/appsec/test_appsec_trace_utils.py

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -230,16 +230,16 @@ def test_set_user_blocked(self):
230230
role="usr.role",
231231
scope="usr.scope",
232232
)
233-
assert span.get_tag(user.ID)
234-
assert span.get_tag(user.EMAIL)
235-
assert span.get_tag(user.SESSION_ID)
236-
assert span.get_tag(user.NAME)
237-
assert span.get_tag(user.ROLE)
238-
assert span.get_tag(user.SCOPE)
239-
assert span.get_tag(user.SESSION_ID)
240-
assert span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_COLLECTION_MODE) == LOGIN_EVENTS_MODE.SDK
241-
assert span.get_tag("usr.id") == str(self._BLOCKED_USER)
242-
assert is_blocked(span)
233+
assert span.get_tag(user.ID)
234+
assert span.get_tag(user.EMAIL)
235+
assert span.get_tag(user.SESSION_ID)
236+
assert span.get_tag(user.NAME)
237+
assert span.get_tag(user.ROLE)
238+
assert span.get_tag(user.SCOPE)
239+
assert span.get_tag(user.SESSION_ID)
240+
assert span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_COLLECTION_MODE) == LOGIN_EVENTS_MODE.SDK
241+
assert span.get_tag("usr.id") == str(self._BLOCKED_USER)
242+
assert is_blocked(span)
243243

244244
def test_track_user_blocked(self):
245245
with asm_context(tracer=self.tracer, span_name="fake_span", config=config_good_rules) as span:
@@ -250,21 +250,22 @@ def test_track_user_blocked(self):
250250
metadata={
251251
"email": "usr.email",
252252
"name": "usr.name",
253-
"session_id": "usr.session_id",
254253
"role": "usr.role",
255254
"scope": "usr.scope",
256255
},
257256
)
258-
assert span.get_tag(user.ID)
259-
assert span.get_tag(user.EMAIL)
260-
assert span.get_tag(user.SESSION_ID)
261-
assert span.get_tag(user.NAME)
262-
assert span.get_tag(user.ROLE)
263-
assert span.get_tag(user.SCOPE)
264-
assert span.get_tag(user.SESSION_ID)
265-
assert span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_COLLECTION_MODE) == LOGIN_EVENTS_MODE.SDK
266-
assert span.get_tag("usr.id") == str(self._BLOCKED_USER)
267-
assert is_blocked(span)
257+
assert span.get_tag(user.ID)
258+
assert span.get_tag(user.EMAIL)
259+
assert span.get_tag(user.SESSION_ID)
260+
assert span.get_tag(user.NAME)
261+
assert span.get_tag(user.ROLE)
262+
assert span.get_tag(user.SCOPE)
263+
assert span.get_tag(user.SESSION_ID)
264+
assert span.get_tag(APPSEC.AUTO_LOGIN_EVENTS_COLLECTION_MODE) == LOGIN_EVENTS_MODE.SDK
265+
# assert metadata tags are not set for usual data
266+
assert span.get_tag("appsec.events.auth_sdk.track") is None
267+
assert span.get_tag("usr.id") == str(self._BLOCKED_USER)
268+
assert is_blocked(span)
268269

269270
def test_no_span_doesnt_raise(self):
270271
from ddtrace.trace import tracer

0 commit comments

Comments
 (0)