-
-
Couldn't load subscription status.
- Fork 4.5k
fix(aci milestone 3): fake IDs in serializer if new models don't have old model equivalents #101974
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| OFFSET = 10**9 | ||
|
|
||
|
|
||
| def get_fake_id_from_object_id(obj_id: int) -> int: | ||
| """ | ||
| Return object_id + 1 billion to avoid any ID collisions with the model whose ID we're faking. | ||
| """ | ||
| return obj_id + OFFSET | ||
|
|
||
|
|
||
| def get_object_id_from_fake_id(fake_id: int) -> int: | ||
| """ | ||
| Undo the object_id + offset operation to recover the object ID. | ||
| """ | ||
| return fake_id - OFFSET |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,10 @@ | |
|
|
||
| from sentry.api.serializers import Serializer, serialize | ||
| from sentry.incidents.endpoints.serializers.alert_rule import AlertRuleSerializerResponse | ||
| from sentry.incidents.endpoints.serializers.utils import ( | ||
| get_fake_id_from_object_id, | ||
| get_object_id_from_fake_id, | ||
| ) | ||
| from sentry.incidents.endpoints.serializers.workflow_engine_data_condition import ( | ||
| WorkflowEngineDataConditionSerializer, | ||
| ) | ||
|
|
@@ -77,9 +81,13 @@ def add_triggers_and_actions( | |
| errors = [] | ||
| alert_rule_id = serialized.get("alertRuleId") | ||
| assert alert_rule_id | ||
| detector_id = AlertRuleDetector.objects.values_list("detector_id", flat=True).get( | ||
| alert_rule_id=alert_rule_id | ||
| ) | ||
| try: | ||
| detector_id = AlertRuleDetector.objects.values_list("detector_id", flat=True).get( | ||
| alert_rule_id=alert_rule_id | ||
| ) | ||
| except AlertRuleDetector.DoesNotExist: | ||
| detector_id = get_object_id_from_fake_id(int(alert_rule_id)) | ||
|
|
||
| detector = detectors[int(detector_id)] | ||
| alert_rule_triggers = result[detector].setdefault("triggers", []) | ||
|
|
||
|
|
@@ -320,17 +328,22 @@ def get_attrs( | |
|
|
||
| def serialize(self, obj: Detector, attrs, user, **kwargs) -> AlertRuleSerializerResponse: | ||
| triggers = attrs.get("triggers", []) | ||
| alert_rule_detector_id = None | ||
| alert_rule_id = None | ||
|
|
||
| if triggers: | ||
| alert_rule_detector_id = triggers[0].get("alertRuleId") | ||
| alert_rule_id = triggers[0].get("alertRuleId") | ||
| else: | ||
| alert_rule_detector_id = AlertRuleDetector.objects.values_list( | ||
| "alert_rule_id", flat=True | ||
| ).get(detector=obj) | ||
| try: | ||
| alert_rule_id = AlertRuleDetector.objects.values_list( | ||
| "alert_rule_id", flat=True | ||
| ).get(detector=obj) | ||
|
Comment on lines
+337
to
+339
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Manual Review Advice: A vulnerability from this advisory is reachable if you are using Django with MySQL or MariaDB Fix: Upgrade this library to at least version 5.2.7 at sentry/uv.lock:305.
🎉 Fixed in commit 8c89442 🎉
Comment on lines
+337
to
+339
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Manual Review Advice: A vulnerability from this advisory is reachable if you are using Django with MySQL or MariaDB Fix: Upgrade this library to at least version 5.2.7 at sentry/uv.lock:305.
🍰 Fixed in commit e878183 🍰 |
||
| except AlertRuleDetector.DoesNotExist: | ||
| # this detector does not have an analog in the old system, | ||
| # but we need to return *something* | ||
| alert_rule_id = get_fake_id_from_object_id(obj.id) | ||
|
|
||
| data: AlertRuleSerializerResponse = { | ||
| "id": str(alert_rule_detector_id), | ||
| "id": str(alert_rule_id), | ||
| "name": obj.name, | ||
| "organizationId": str(obj.project.organization_id), | ||
| "status": ( | ||
|
|
||
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.
High severity vulnerability may affect your project—review required:
Line 59 lists a dependency (django) with a known High severity vulnerability.
ℹ️ Why this matters
Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
References: GHSA, CVE
To resolve this comment:
Check if you are using Django with MySQL or MariaDB.
/fp we don't use this [condition]💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasonsYou can view more details on this finding in the Semgrep AppSec Platform here.